Re: [PATCH 4/5] VT-d: restrict iommu_flush_all() to cache writeback

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/04/2023 11:46 am, Jan Beulich wrote:
> We don't need to invalidate caches here; all we're after is that earlier
> writes have made it to main memory (and aiui even that just in case).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> This, aiui, being an analogue to uses of iommu_sync_cache() (just not
> range restricted), I wonder whether it shouldn't be conditional upon
> iommu_non_coherent. Then again I'm vaguely under the impression that
> we had been here before, possibly even as far as questioning the need
> for this call altogether.

I'd far rather we fix it property than continue to massage around the
sides of known-broken logic.

Coherency, or not, of the memory accesses of an IOMMU is a per-IOMMU
property, not a system-wide property.  What the iommu_non_coherent
global boolean currently does for us is enforce cache maintenance on all
IOMMUs, even the coherent ones, when any single IOMMU in the system is
non-coherent.

Inside the IOMMU driver, cache maintenance should depend on iommu->ecap
alone, disregarding anything else.  This will improve the performance on
any system with mixed coherent and non-coherent IOMMUs, without any
other behavioural impact.

The complication comes in in p2m-ept when we're sharing EPT and IOMMU
pagetables, because the EPT can be accessed by more than one IOMMU if
the guest has devices behind different IOMMUs.

But we know the devices assigned to the domain.  They're almost always
static for the lifetime of the domain, and generally single device only,
so there may be value in considering a per-domain "I've got a
non-coherent IOMMU" flag, because we absolutely don't want to be walking
the list of attached IOMMUs on each EPT update.


But...

SandyBridge era systems are the only ones I'm aware of where the IOMMUs
advertise themselves as non-coherent.  And on that generation we quirk
the superpage capability off anyway, meaning they are ineligible for
HAP-PT sharing anyway.

And if we are doing HAP-PT sharing, the cache maintenance for regular
EPT updates will be crippling for CPU performance, especially as the
software bit updates are not relevant to the IOMMU anyway.

A better option would be to simply disallow HAP-PT sharing when there's
a non-coherent IOMMU in the system, or (slightly more fine grained) to
disallow adding a device behind a non-coherent IOMMU to a domain using
HAP-PT sharing (as there's a disable now anyway).

Either will reduce the complexity of Xen's code without any loss of
functionality in real systems AFAICT.

~Andrew