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

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

On Mon, Sep 26, 2022 at 09:33:10AM +0200, Jan Beulich wrote:
> 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.

I guess any OS that moves BARs around will also trigger such code
paths, but that might not be very common.  I can add something to the
description.

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

Maybe it would be better to add the calls to memory_type_changed()
directly in iomem_{permit,deny}_access() and
ioports_{permit,deny}_access itself?

That would also allow to remove the noop Arm memory_type_changed()
halper.

Thanks, Roger.