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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.04.2023 22:46, Andrew Cooper wrote:
> 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.

I agree in principle, but you're not asking that I actually amend this
single-line change with all the work you outline, are you? To be quite
honest, what you ask for really is something the VT-d maintainer(s)
(i.e. Kevin as it presently stands) ought to be doing (or really have
done long ago) ...

Jan

> 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