Re: [Xen-devel] [PATCH v2 05/10] AMD/IOMMU: introduce 128-bit IRTE non-guest-APIC IRTE format

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 27/06/2019 16:21, Jan Beulich wrote:
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -40,12 +40,45 @@ union irte32 {
>
> -#define INTREMAP_TABLE_ORDER    1
> +union irte_cptr {
> +    const void *ptr;
> +    const union irte32 *ptr32;
> +    const union irte128 *ptr128;
> +} __transparent__;
> +
> +#define INTREMAP_TABLE_ORDER (irte_mode == irte32 ? 1 : 3)

This is problematic for irte_mode == irteUNK.  As this "constant" is
used in exactly two places, I'd suggest a tiny static function along the
same lines as {get,update}_intremap_entry(), which can sensibly prevent
code looking for a size before irte_mode is set up.

> @@ -142,7 +187,21 @@ static void free_intremap_entry(unsigned
>  {
>      union irte_ptr entry = get_intremap_entry(seg, bdf, index);
>  
> -    ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
> +    switch ( irte_mode )
> +    {
> +    case irte32:
> +        ACCESS_ONCE(entry.ptr32->raw[0]) = 0;
> +        break;
> +
> +    case irte128:
> +        ACCESS_ONCE(entry.ptr128->raw[0]) = 0;
> +        barrier();

smp_wmb().

Using barrier here isn't technically correct, because what matters is
the external visibility of the write.

It functions correctly on x86 because smp_wmb() is barrier(), but this
code doesn't work correctly on e.g. ARM.

I'd go further and leave an explanation.

smp_wmb(); /* Ensure the clear of .remap_en is visible to the IOMMU
first. */

> @@ -444,9 +601,9 @@ static int update_intremap_entry_from_ms
>      unsigned long flags;
>      union irte_ptr entry;
>      u16 req_id, alias_id;
> -    u8 delivery_mode, dest, vector, dest_mode;
> +    uint8_t delivery_mode, vector, dest_mode;

For the ioapic version, you used unsigned int, rather than uint8_t.  I'd
expect them to at least be consistent.

~Andrew

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