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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

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

Jan