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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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