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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

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), 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).

~Andrew

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