Re: [PATCH] x86/ept: limit calls to memory_type_changed()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.09.2022 10:35, Roger Pau Monné wrote:
> On Thu, Sep 22, 2022 at 09:21:59PM +0200, Jan Beulich wrote:
>> On 22.09.2022 18:05, Roger Pau Monne wrote:
>>> memory_type_changed() is currently only implemented for Intel EPT, and
>>> results in the invalidation of EMT attributes on all the entries in
>>> the EPT page tables.  Such invalidation causes EPT_MISCONFIG vmexits
>>> when the guest tries to access any gfns for the first time, which
>>> results in the recalculation of the EMT for the accessed page.  The
>>> vmexit and the recalculations are expensive, and as such should be
>>> avoided when possible.
>>>
>>> Remove the call to memory_type_changed() from
>>> XEN_DOMCTL_memory_mapping: there are no modifications of the
>>> iomem_caps ranges anymore that could alter the return of
>>> cache_flush_permitted() from that domctl.
>>
>> I certainly agree - this was an oversight when the two aspects were
>> split. One might argue this is a (performance) fix to the earlier
>> commit, and hence might want to go on its own with a Fixes: tag.
> 
> Was wondering myself, didn't add the 'Fixes:' tag because of the extra
> content.
> 
>>> Calls to memory_type_changed() resulting from changes to the domain
>>> iomem_caps or ioport_caps ranges are only relevant for EMT
>>> calculations if the IOMMU is not enabled, and the call has resulted in
>>> a change to the return value of cache_flush_permitted().
>>
>> I'm less certain here: These shouldn't be frequent operations, so
>> their impact on the guest should be limited?
> 
> Citrix has an use case for vGPU where IOMMU regions are added and
> removed during guest runtime.  Such functionality makes uses of both
> XEN_DOMCTL_iomem_permission and XEN_DOMCTL_memory_mapping.

I see. Maybe this would want saying in the description, to express
that there's little expected benefit for upstream.

> While the memory_type_changed() call in XEN_DOMCTL_memory_mapping
> seems to be the most problematic performance wise, I though it was
> nice to try to avoid memory_type_changed() as much as possible, as
> those tax the guest quite heavily with EPT_MISCONFIG faults and the
> recalculation logic.

Trying to avoid this is certainly desirable, I agree. But we need
to make sure that it's not "easy" to break things by touching one
place but leaving others alone which really would need keeping in
sync. Therefore I'd see such added logic as acceptable only if the
risk towards future changes is sufficiently low.

>> And if we were to restrict the calls, I think we need to clearly
>> tie together the various places which need updating together in
>> case e.g. the condition in epte_get_entry_emt() is changed.
>> Minimally by way of comments, but maybe by way of a small helper
>> function (for which I can't seem to be able to think of a good
>> name) sitting next to epte_get_entry_emt().
> 
> Such helper function is also kind of problematic, as it would have to
> live in p2m-ept.c but be used in domctl.c and x86/domctl.c?  It would
> have to go through the p2m_domain indirection structure.

It would need abstraction at the arch level as well as for !HVM configs
on x86. I'm not sure the indirection layer would actually be needed, as
the contents of the function - despite wanting placing in p2m-ept.c -
isn't really vendor dependent. (If AMD/SVM gained a need for a similar
helper, things would nee re-evaluating.)

> Do you have any suggestions about how the function should look like?
> I'm afraid the fact it needs the previous cache_flush_permitted()
> value makes it kind of weird to encapsulate.

Indeed.

> I've attempted to add comments to make it clear why the new checks are
> added, but I would also need to add a comment to epte_get_entry_emt()
> to notice changes in the condition need to be propagated to call sites
> of memory_type_changed().

Right - it may suffice to have one more extensive comment, but _all_
involved parties will need to have at least a cross reference such
that one can easily find all pieces of code needing to be kept in sync.

Jan