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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan