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

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

On Mon, Sep 26, 2022 at 06:03:24PM +0000, Andrew Cooper wrote:
> On 22/09/2022 17: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.
> >
> > 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().
> 
> This, and the perf problem Citrix have found, is caused by a more
> fundamental bug which I identified during XSA-402.
> 
> Xen is written with assumption that cacheability other than WB is
> dependent on having devices.  While this is perhaps true of current
> configurations available, it is a layering violation, and will cease
> being true in order to support encrypted RAM (and by extension,
> encrypted VMs).

I assumed this was done as a performance improvement (or hack).

> At the moment, we know the IOMMU-ness of a domain right from the outset,
> but the cacheability permits are dynamic, based on the non-emptyness of
> the domain's device list, ioport list, and various others.

Well, as long as there's an IOMMU assigned cacheability will be
calculated taking into account the guest MTRR values, and won't
be forced to WB, regardless of the emptiness of the device or IO
ports/memory capability lists.

Just setting `passthrough=1` on the guest config file without any
devices actually passed through should prevent the forcing WB.

The use case of allowing io memory or ports to be assigned to an HVM
guest without an IOMMU has always confused me, but I guess if it's
there it's because it's used by someone.

> All the memory_type_changed() calls here are to cover the fact that the
> original design was buggy by not having the cacheability-ness part of
> domain create in the first place.
> 
> The appropriate fix, but definitely 4.18 work at this point, is to have
> a new CDF flag which permits the use of non-WB cacheability.
> 
> For testing purposes alone, turning it on on an otherwise "plain VM" is
> useful (its how I actually debugged XSA-402, and the only sane way to go
> about investigating the MTRR per disasters for VGPU VMs[1]), 

Wasn't it enough to set `passthrough=1` in order to enable
cacheability for debugging purposes?  (not that I oppose to adding the
flag, just curious why enabling the IOMMU wasn't enough).

> but for
> regular usecases, it wants cross-checking with the IOMMU flag (and
> encrypted VM flag in the future), and for all dynamic list checks to
> turn into a simple 'd->config & CDF_full_cacheability'.
> 
> This way, we delete all calls to memory_type_changed() which are trying
> to cover the various dynamic lists becoming empty/non-empty, and we
> remove several ordering-of-hypercalls bugs where non-cacheable mappings
> can't actually be created on a VM declared to have an IOMMU until a
> device has actually been assigned to start with.

It should be possible with current code to create non-cacheable
mappings on a VM as long as the IOMMU is enabled, regardless of
whether no devices are assigned to the VM.

> ~Andrew
> 
> [1] MTRR handling is also buggy with reduced cacheability, causing some
> areas of RAM to be used UC; notably the grant table.  This manifests as
> PV device perf being worse than qemu-emulated device perf, only when a
> GPU is added to a VM[2].  Instead of fixing this properly, it was hacked
> around by forcing IPAT=1 for Xenheap pages, which only "fixed" the
> problem on Intel (AMD has no equivalent mechanism), and needs reverting
> and fixing properly (i.e. get the vMTRR layout working correctly) to
> support VMs with encrypted RAM.

My understanding of the original problem was slightly different: we
place the grant table in a BAR region of the xenpci device, and hence
the gMTRRs are set to UC by the guest.  The workaround in Xen is to
cope with existing Windows guest drivers not setting the gMTRR values
for the grant table frames to WB.

Even if we calculate all the cache attributes correctly we would still
need the 'hack'.

> 
> [2] There's a second bug with memory_type_changed() in that it causes
> dreadful system performance during VM migration, which is something to
> do with the interaction of restoring vMTRRs for a VM that has a device
> but isn't running yet.  This still needs investigating, and I suspect
> it's got a similar root cause.

XenServer seems to have a custom patch for this:

https://github.com/xenserver/xen.pg/blob/XS-8.3.x/patches/0001-x86-HVM-Avoid-cache-flush-operations-during-hvm_load.patch

But we could likely avoid the flush completely if the VM hasn't been
started yet.

Thanks, Roger.