Re: [Xen-devel] [PATCH v5] x86/p2m: use large pages for MMIO mappings

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.01.16 at 15:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 27/01/16 13:37, Jan Beulich wrote:
>>>>> On 27.01.16 at 13:32, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 25/01/16 16:18, Jan Beulich wrote:
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>> @@ -2491,7 +2491,7 @@ static int vmx_alloc_vlapic_mapping(stru
>>>>      share_xen_page_with_guest(pg, d, XENSHARE_writable);
>>>>      d->arch.hvm_domain.vmx.apic_access_mfn = mfn;
>>>>      set_mmio_p2m_entry(d, paddr_to_pfn(APIC_DEFAULT_PHYS_BASE), _mfn(mfn),
>>>> -                       p2m_get_hostp2m(d)->default_access);
>>>> +                       PAGE_ORDER_4K, p2m_get_hostp2m(d)->default_access);
>>>>  
>>> This should ASSERT() success, in case we make further changes to the
>>> error handling.
>> Maybe, but since it didn't before I don't see why this couldn't /
>> shouldn't be an independent future patch.
> 
> Can be.  IMO it is a bug that it isn't already checked.  (-ENOMEM when
> allocating p2m leaves perhaps?)

Indeed, albeit that means ASSERT() wouldn't be right anyway. I
hope the VMX maintainers monitor this and will prepare a patch...

>>>> --- a/xen/arch/x86/mm/p2m.c
>>>> +++ b/xen/arch/x86/mm/p2m.c
>>>> @@ -899,48 +899,62 @@ void p2m_change_type_range(struct domain
>>>>      p2m_unlock(p2m);
>>>>  }
>>>>  
>>>> -/* Returns: 0 for success, -errno for failure */
>>>> +/*
>>>> + * Returns:
>>>> + *    0        for success
>>>> + *    -errno   for failure
>>>> + *    order+1  for caller to retry with order (guaranteed smaller than
>>>> + *             the order value passed in)
>>>> + */
>>>>  static int set_typed_p2m_entry(struct domain *d, unsigned long gfn, mfn_t 
>>>> mfn,
>>>> -                               p2m_type_t gfn_p2mt, p2m_access_t access)
>>>> +                               unsigned int order, p2m_type_t gfn_p2mt,
>>>> +                               p2m_access_t access)
>>>>  {
>>>>      int rc = 0;
>>>>      p2m_access_t a;
>>>>      p2m_type_t ot;
>>>>      mfn_t omfn;
>>>> +    unsigned int cur_order = 0;
>>>>      struct p2m_domain *p2m = p2m_get_hostp2m(d);
>>>>  
>>>>      if ( !paging_mode_translate(d) )
>>>>          return -EIO;
>>>>  
>>>> -    gfn_lock(p2m, gfn, 0);
>>>> -    omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, NULL, NULL);
>>>> +    gfn_lock(p2m, gfn, order);
>>>> +    omfn = p2m->get_entry(p2m, gfn, &ot, &a, 0, &cur_order, NULL);
>>>> +    if ( cur_order < order )
>>>> +    {
>>>> +        gfn_unlock(p2m, gfn, order);
>>>> +        return cur_order + 1;
>>> Your comment states that the return value is guarenteed to be less than
>>> the passed-in order, but this is not the case here.  cur_order could, in
>>> principle, be only 1 less than order, at which point your documentation
>>> is incorrect.
>>>
>>> Does this rely on the x86 architectural orders to function as documented?
>> No. Maybe the comment text is ambiguous, but I don't see how to
>> improve it without making it too lengthy: The return value is
>> <order>+1, telling the caller to retry with <order>, which is
>> guaranteed to be less than the order that got passed in. I.e. taking
>> the variable naming above, the caller would have to retry with
>> cur_order, which - due to the if() - is smaller than order.
> 
> Ah - I see.  The text is indeed confusing.  How about:
> 
> "1 + new order: for caller to retry with smaller order (guaranteed to be
> smaller than order passed in)"

Okay.

>>>>  int set_mmio_p2m_entry(struct domain *d, unsigned long gfn, mfn_t mfn,
>>>> -                       p2m_access_t access)
>>>> +                       unsigned int order, p2m_access_t access)
>>>>  {
>>>> -    return set_typed_p2m_entry(d, gfn, mfn, p2m_mmio_direct, access);
>>>> +    if ( order &&
>>>> +         rangeset_overlaps_range(mmio_ro_ranges, mfn_x(mfn),
>>>> +                                 mfn_x(mfn) + (1UL << order) - 1) &&
>>>> +         !rangeset_contains_range(mmio_ro_ranges, mfn_x(mfn),
>>>> +                                  mfn_x(mfn) + (1UL << order) - 1) )
>>>> +        return order;
>>> Should this not be a hard error?  Even retrying with a lower order is
>>> going fail.
>> Why? The latest when order == 0, rangeset_overlaps_range()
>> will return the same as rangeset_contains_range(), and hence
>> the condition above will always be false (one of the two reasons
>> for checking order first here).
> 
> It isn't the order check which is an issue.
> 
> One way or another, if the original (mfn/order) fails the rangeset
> checks, the overall call is going to fail, but it will be re-executed
> repeatedly with an order decreasing to 0.  Wouldn't it be better just to
> short-circuit this back&forth?

But this won't necessarily go down to order 0. Short-circuiting
would mean taking PAGE_ORDER_2M and PAGE_ORDER_1G into
account here, which would imo severely hamper readability.

> Relatedly, is there actually anything wrong with making a superpage
> read-only mapping over some scattered read-only 4K pages?

I'm afraid I don't understand: "scattered pages" and "superpage
mapping" don't seem to fit together for me.

Jan

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