Re: [Xen-devel] [PATCH v3 08/14] AMD/IOMMU: introduce 128-bit IRTE non-guest-APIC IRTE format

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 16/07/2019 17:38, Jan Beulich wrote:
> This is in preparation of actually enabling x2APIC mode, which requires
> this wider IRTE format to be used.
>
> A specific remark regarding the first hunk changing
> amd_iommu_ioapic_update_ire(): This bypass was introduced for XSA-36,
> i.e. by 94d4a1119d ("AMD,IOMMU: Clean up old entries in remapping
> tables when creating new one"). Other code introduced by that change has
> meanwhile disappeared or further changed, and I wonder if - rather than
> adding an x2apic_enabled check to the conditional - the bypass couldn't
> be deleted altogether. For now the goal is to affect the non-x2APIC
> paths as little as possible.

There are plenty of mistakes with XSA-36.  Reading the XSA back, the
MITIGATION section gets the sense of the iommu=amd-iommu-perdev-intremap
boolean the wrong way around.  Oh well...

SP5100 erratum 28 only requires that the IDE and SATA devices share
tables, not that every device on the whole system shares tables.

With the proposed work to perform IOMMU assignment by group rather than
individually, this will naturally fall out as a quirk requiring the two
devices to be grouped, at which point we can drop all remnants of global
remapping tables.

In this case, I'm not sure it is worth caring about shared-table mode on
x2apic-capable systems.  0 people will be using that mode.  That said,
if its easier to wait until the IOMMU changes to make this adjustment,
then fine.

> @@ -142,7 +178,15 @@ static void free_intremap_entry(const st
>   {
>       union irte_ptr entry = get_intremap_entry(iommu, bdf, index);
>   
> -    ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
> +    if ( iommu->ctrl.ga_en )
> +    {
> +        ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
> +        /* Low half (containing RemapEn) needs to be cleared first. */
> +        barrier();

While this will function on x86, I still consider this buggy.  From a
conceptual point of view, barrier() is not the correct construction to
use, whereas smp_wmb() is.

As this is the only remaining issue, with it fixed in each location,
Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel