Re: [PATCH v2 5/5] amd/iommu: force atomic updates of remapping entries

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

On Wed, Jul 19, 2023 at 02:46:49PM +0200, Jan Beulich wrote:
> On 18.07.2023 14:43, Roger Pau Monne wrote:
> > Disable XT (x2APIC) support and thus 128 IRTE entries if cmpxchg16b is
> > not available, do so in order to avoid having to disable the IRTE (and
> > possibly loose interrupts) when updating the entry.  Note this also
> > removes the usage of a while loop in order to disable the entry, since
> > RemapEn is no longer toggled when updating.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > x2APIC support was added late enough on AMD that CPUs that I believe
> > all models that have x2APIC must also have CX16.
> > 
> > The AMD-Vi manual contains the following advice in the "2.3.2 Making
> > Guest Interrupt Virtualization Changes" section:
> > 
> > """
> > If RemapEn=1 before the change, the following steps may be followed to
> > change interrupt virtualization information:
> >  * Set RemapEn=0 in the entry to disable interrupt virtualization;
> >    device-initiated interrupt requests for the DeviceID and vector are
> >    processed as IO_PAGE_FAULT events.
> >  * Update the fields in the IRTE and invalidate the interrupt table
> >    (see Section 2.4.5 [INVALIDATE_INTERRUPT_TABLE]).
> >  * Set RemapEn=1 to virtualize interrupts from the device.
> > """
> > 
> > However if the update of the IRTE is done atomically I assume that
> > setting RemapEn=0 is not longer necessary, and that the
> > INVALIDATE_INTERRUPT_TABLE command can be executed after the entry has
> > been (atomically) updated.
> 
> There's one additional prereq to this: The IOMMU also needs to read
> 128-bit IRTEs atomically. I'm afraid we can't take this for given
> without it being said explicitly in the spec (I've just gone through
> and looked at all occurrences of "atomic", without finding anything
> applicable to IRTEs). If this can be resolved, I think I'm fine with
> the patch.

Hm, indeed I was taking for granted that the IOMMU will read the entry
atomically.

I was also worried about the IOMMU caching only parts of the IRTE, and
thus even when doing an atomic read of the full entry it could still
get inconsistent data if using previously cached parts of the entry.
The text in INVALIDATE_INTERRUPT_TABLE seems to suggest that IRTE
fields could be cached on a field by field basis.

I will raise a question with AMD in order to try to figure out whether
the IOMMU will do atomic reads, and also cache the full entry.

Thanks, Roger.