Re: [PATCH] x86/hvm: don't treat MMIO pages as special ones regarding cache attributes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2020 12:34, Roger Pau Monné wrote:
> On Thu, Sep 10, 2020 at 11:27:49AM +0200, Jan Beulich wrote:
>> On 09.09.2020 16:50, Roger Pau Monne wrote:
>>> MMIO regions below the maximum address on the memory map can have a
>>> backing page struct that's shared with dom_io (see x86
>>> arch_init_memory and it's usage of share_xen_page_with_guest), and
>>> thus also fulfill the is_special_page check because the page has the
>>> Xen heap bit set.
>>>
>>> This is incorrect for MMIO regions when is_special_page is used by
>>> epte_get_entry_emt, as it will force direct MMIO regions mapped into
>>> the guest p2m to have the cache attributes set to write-back.
>>>
>>> Add an extra check in epte_get_entry_emt in order to detect pages
>>> shared with dom_io (ie: MMIO regions) and don't force them to
>>> write-back cache type on that case.
>>
>> Did you consider the alternative of not marking those pages as Xen
>> heap ones? In particular when looking at it from this angle I
>> consider it at least odd for non-RAM (or more precisely non-heap)
>> pages to get marked this way.
> 
> I wasn't sure whether this could cause issues in other places of the
> code that would rely on this fact and such change seemed more risky
> IMO.

As said - I don't think there is, but I've not done a full audit.

>> And I can't currently see anything
>> requiring them to be marked as such - them being owned by DomIO is
>> all that's needed as it seems.
> 
> Should those pages then simply be assigned to dom_io and set the
> appropriate flags (PGC_allocated | 1), or should
> share_xen_page_with_guest be modified to not set the PGC_xen_heap
> flag?

Either approach may be okay, I think. It would really depend on
how little of share_xen_page_with_guest() suffices for the dom_io
assignment.

> I see that such addition was done in a2b4b8c2041, but I'm afraid I
> don't fully understand why share_xen_page_with_guest needs to mark
> pages as Xen heap.

I see Paul has already answer this part. You can't drop the setting
of the flag, but you could make it dependent upon d != dom_io.

Jan