Re: [Xen-devel] [PATCH] x86: correct create_bounce_frame

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.05.17 at 20:58, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/05/17 16:42, Jan Beulich wrote:
>>>>> On 02.05.17 at 16:12, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 02/05/17 14:22, Jan Beulich wrote:
>>>> @@ -345,15 +344,20 @@ UNLIKELY_START(z, create_bounce_frame_ba
>>>>  __UNLIKELY_END(create_bounce_frame_bad_bounce_ip)
>>>>          movq  %rax,UREGS_rip+8(%rsp)
>>>>          ret
>>>> -        _ASM_EXTABLE(.Lft2,  domain_crash_page_fault_32)
>>>> -        _ASM_EXTABLE(.Lft3,  domain_crash_page_fault_24)
>>>> -        _ASM_EXTABLE(.Lft4,  domain_crash_page_fault_8)
>>>> -        _ASM_EXTABLE(.Lft5,  domain_crash_page_fault_16)
>>>> -        _ASM_EXTABLE(.Lft6,  domain_crash_page_fault)
>>>> -        _ASM_EXTABLE(.Lft7,  domain_crash_page_fault)
>>>> +        _ASM_EXTABLE(.Lft2,  domain_crash_page_fault_48)
>>>> +        _ASM_EXTABLE(.Lft3,  domain_crash_page_fault_40)
>>>> +        _ASM_EXTABLE(.Lft4,  domain_crash_page_fault_24)
>>>> +        _ASM_EXTABLE(.Lft5,  domain_crash_page_fault_32)
>>>> +        _ASM_EXTABLE(.Lft6,  domain_crash_page_fault_16)
>>>> +        _ASM_EXTABLE(.Lft7,  domain_crash_page_fault_16)
>>>>          _ASM_EXTABLE(.Lft12, domain_crash_page_fault_8)
>>> Given that you have altered the notation for a delta from (%rsi) (which
>>> is a good improvement), these labels should be similarly altered for
>>> consistency.
>> Actually, looking at this again I'm no longer sure factoring out the
>> 8 here would be a good idea. I'm also not sure I understand you
>> saying "Given that you have altered the notation for a delta from
>> (%rsi)" - the notation didn't change at all, the numeric tag has
>> always been expressing the adjustment needed for %rsi to
>> represent the actual failing memory address.
> 
> First of all, this point is not obvious at all to people reading the
> code who don't already know the meaning.  If it were new code today, I'd
> insist on a comment explaining what the numeric tag means.

I'm not at all opposed to adding a comment, if you think that helps.

> As for the change in notation, you very much have changed it. You have
> factored 8 out of it, which visually emphases the number of stack slots
> away from (%rsi) is used.

That's notation of the mainline code, not the fixup one. The fixup
code continues to be a sequence of additions of 8 to %rsi.

> The change in notation is, in principle, a good thing because it makes
> the code easier to read and correlate with the comment.
> 
> However, such a change is only an improvement if it is implemented
> consistently (hence the request to use 1*8 and 0*8 for the other (%rsi)
> references),

As said before, if this is the only way to get an ack on the patch, I
can do this part despite not agreeing with the argument. As we've
had such situations a number of times in the past, I think we finally
need to have a more general discussion here. I'll kick off a separate
thread.

> and if it doesn't complicate other areas of the code.  By
> expressing the (%rsi) references in terms of slots, but the fixup lables
> in terms of bytes, you have taken a complicated and non-obvious
> relationship and made it even more complicated to review, which is a net
> detriment (and hence the request to express the labels in the same units
> as the code they refer to).

So did you really consider how the alternative would look like:

domain_crash_page_fault_6:
        addq  $8,%rsi
domain_crash_page_fault_5:
        addq  $8,%rsi
domain_crash_page_fault_4:
        addq  $8,%rsi
domain_crash_page_fault_3:
        addq  $8,%rsi
domain_crash_page_fault_2:
        addq  $8,%rsi
domain_crash_page_fault_1:
        addq  $8,%rsi
domain_crash_page_fault:
        ...

I don't think this is any more or any less clear than what we have.
As said, using label names of the form
domain_crash_page_fault_<N>*8 is not going to work with other
than pretty recent binutils, and I'm unconvinced the alternative
of e.g. domain_crash_page_fault_<N>x8 is really helpful. Plus in
either case the question then would whether (a) you'd expect the
final label to use 0 to replace <N> (instead of not having any tag
at all, as it is currently) and (b) whether you'd insist on all the 8s
to become 1*8.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel