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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.01.16 at 15:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> 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.

I don't see where such failure would come from:
{ept_,}p2m_type_to_flags() silently suppress the mapping
becoming writable. What am I overlooking?

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

Oh, so you want "access" to also be taken into account. Not
sure that's worth it right now - r/o MMIO mappings shouldn't
occur very often (and map_mmio_regions() passes
->default_access anyway).

Jan


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