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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Sep 26, 2022 at 04:50:22PM +0200, Roger Pau Monné wrote:
> 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.

Correction: the Arm memory_type_changed() needs to stay, as
iomem_{permit,deny}_access() is common code.

Regards, Roger.