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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/01/16 14:40, Jan Beulich wrote:
>
>>>>>  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.

Even when this check starts passing, the subsequent
set_typed_p2m_entry() will fail for writeable mappings, after having
constructed small pages up to the boundary of the RO region.

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

If there is a single 4K page in the RO region, and the caller attempts
to create a RO 2M superpage which includes the 4K region, these checks
will force the use of 4K mappings even though the 2M mapping would be fine.

~Andrew