Re: [Xen-devel] [PATCH] x86/mm: Reduce debug overhead of __virt_to_maddr()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/08/17 16:07, Jan Beulich wrote:
>>>> On 16.08.17 at 16:22, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 16/08/17 15:14, Andrew Cooper wrote:
>>> On 16/08/17 15:11, Jan Beulich wrote:
>>>>>>> On 16.08.17 at 15:58, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> --- a/xen/include/asm-x86/x86_64/page.h
>>>>> +++ b/xen/include/asm-x86/x86_64/page.h
>>>>> @@ -51,13 +51,15 @@ extern unsigned long xen_virt_end;
>>>>>  
>>>>>  static inline unsigned long __virt_to_maddr(unsigned long va)
>>>>>  {
>>>>> -    ASSERT(va >= XEN_VIRT_START);
>>>>>      ASSERT(va < DIRECTMAP_VIRT_END);
>>>>>      if ( va >= DIRECTMAP_VIRT_START )
>>>>>          va -= DIRECTMAP_VIRT_START;
>>>>>      else
>>>>>      {
>>>>> -        ASSERT(va < XEN_VIRT_END);
>>>>> +        BUILD_BUG_ON(XEN_VIRT_END - XEN_VIRT_START != GB(1));
>>>>> +        ASSERT(((long)va >> (PAGE_ORDER_1G + PAGE_SHIFT)) ==
>>>>> +               ((long)XEN_VIRT_START >> (PAGE_ORDER_1G + PAGE_SHIFT)));
>>>> Do you really need the casts here? I.e. what's wrong here with
>>>> doing unsigned long arithmetic?
>>> Oh - good point.  This took more than one attempt to get right, and I
>>> first thought I had a sign extension problem.  The actual problem was a
>>> (lack of) + PAGE_SHIFT.
>>>
>>> The other thing to know is that  __virt_to_maddr() is used before the
>>> IDT is set up, so your only signal of something being wrong is a triple
>>> fault.  Let me double check without the casts, but I think it should be
>>> fine.
>> Ok - so it does function when using unsigned arithmetic.
>>
>> However, the generated code is better with signed arithmetic, as
>> ((long)XEN_VIRT_START >> 39) fix in a 32bit sign-extended immediate,
>> whereas XEN_VIRT_START >> 39 needs a movabs.
> Why would that be? Shifting out 39 bits means 25 significant bits
> are left out of the original 64. Or wait - isn't it 30 rather than 39?
> In that case indeed 34 significant bits would remain. In that case
> I'd be fine with the casts left in place, as long as at least the
> commit message (a code comment may be better to keep people
> like me from being tempted to remove the casts as ugly and
> apparently unnecessary) says why.

I'm clearly doing very well at counting today.  I do mean 30 bits (order
18 + page shift of 12).

The generated code is this:

ffff82d0802ff923:       48 89 c2                mov    %rax,%rdx
ffff82d0802ff926:       48 c1 fa 1e             sar    $0x1e,%rdx
ffff82d0802ff92a:       48 81 fa 42 0b fe ff    cmp   
$0xfffffffffffe0b42,%rdx

While there are 34 significant bits from this shift, the top 16 of them
are strictly set, meaning there are only 28 usefully significant bits.

FYI, the unsigned case looks like this:

ffff82d0802ffb12:       48 89 c1                mov    %rax,%rcx
ffff82d0802ffb15:       48 c1 e9 1e             shr    $0x1e,%rcx
ffff82d0802ffb19:       48 ba 42 0b fe ff 03    movabs $0x3fffe0b42,%rdx
ffff82d0802ffb20:       00 00 00
ffff82d0802ffb23:       48 39 d1                cmp    %rdx,%rcx

Are you happy with the following comment?

/* Signed arithmetic in so ((long)XEN_VIRT_START >> 30) fits in an imm32. */

~Andrew

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