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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

On the whole, I'd prefer to keep patch as is.

~Andrew

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