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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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.

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

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.

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

> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > I feel it's a bit weird to have calls to memory_type_changed() in
> > common domctl code - for once the domctl that trigger the call doesn't
> > change memory types, just adds or removes ranges from iomem_caps
> > (which in turn affects the behaviour of epte_get_entry_emt()).
> 
> Do you have a better suggestion?

No, not really, because we need the return value of
cache_flush_permitted() before and after the changes, so it's not as
easy as introducing a single helper sadly.

Thanks, Roger.