Re: [Xen-devel] [PATCH 3/9] AMD/IOMMU: use bit field for IRTE

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

On 13/06/2019 14:23, Jan Beulich wrote:
> At the same time restrict its scope to just the single source file
> actually using it, and abstract accesses by introducing a union of
> pointers. (A union of the actual table entries is not used to make it
> impossible to [wrongly, once the 128-bit form gets added] perform
> pointer arithmetic / array accesses on derived types.)
>
> Also move away from updating the entries piecemeal: Construct a full new
> entry, and write it out.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> It would have been nice to use write_atomic() or ACCESS_ONCE() for the
> actual writes, but both cast the value to a scalar one, which doesn't
> suit us here (and I also didn't want to make the compound type a union
> with a raw member just for this).

Actually, having looked at the following patch, I think it would be
better to union with a uint32_t raw, so that we can use...

> @@ -101,47 +118,44 @@ static unsigned int alloc_intremap_entry
>      return slot;
>  }
>  
> -static u32 *get_intremap_entry(int seg, int bdf, int offset)
> +static union irte_ptr get_intremap_entry(unsigned int seg, unsigned int bdf,
> +                                         unsigned int offset)
>  {
> -    u32 *table = get_ivrs_mappings(seg)[bdf].intremap_table;
> +    union irte_ptr table = {
> +        .raw = get_ivrs_mappings(seg)[bdf].intremap_table
> +    };
> +
> +    ASSERT(table.raw && (offset < INTREMAP_ENTRIES));
>  
> -    ASSERT( (table != NULL) && (offset < INTREMAP_ENTRIES) );
> +    table.basic += offset;
>  
> -    return table + offset;
> +    return table;
>  }
>  
> -static void free_intremap_entry(int seg, int bdf, int offset)
> +static void free_intremap_entry(unsigned int seg, unsigned int bdf, unsigned int offset)
>  {
> -    u32 *entry = get_intremap_entry(seg, bdf, offset);
> +    union irte_ptr entry = get_intremap_entry(seg, bdf, offset);
> +
> +    *entry.basic = (struct irte_basic){};

ACCESS_ONCE(*entry.basic.raw) = 0;

and...

>  
> -    memset(entry, 0, sizeof(u32));
>      __clear_bit(offset, get_ivrs_mappings(seg)[bdf].intremap_inuse);
>  }
>  
> -static void update_intremap_entry(u32* entry, u8 vector, u8 int_type,
> -    u8 dest_mode, u8 dest)
> -{
> -    set_field_in_reg_u32(IOMMU_CONTROL_ENABLED, 0,
> -                            INT_REMAP_ENTRY_REMAPEN_MASK,
> -                            INT_REMAP_ENTRY_REMAPEN_SHIFT, entry);
> -    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
> -                            INT_REMAP_ENTRY_SUPIOPF_MASK,
> -                            INT_REMAP_ENTRY_SUPIOPF_SHIFT, entry);
> -    set_field_in_reg_u32(int_type, *entry,
> -                            INT_REMAP_ENTRY_INTTYPE_MASK,
> -                            INT_REMAP_ENTRY_INTTYPE_SHIFT, entry);
> -    set_field_in_reg_u32(IOMMU_CONTROL_DISABLED, *entry,
> -                            INT_REMAP_ENTRY_REQEOI_MASK,
> -                            INT_REMAP_ENTRY_REQEOI_SHIFT, entry);
> -    set_field_in_reg_u32((u32)dest_mode, *entry,
> -                            INT_REMAP_ENTRY_DM_MASK,
> -                            INT_REMAP_ENTRY_DM_SHIFT, entry);
> -    set_field_in_reg_u32((u32)dest, *entry,
> -                            INT_REMAP_ENTRY_DEST_MAST,
> -                            INT_REMAP_ENTRY_DEST_SHIFT, entry);
> -    set_field_in_reg_u32((u32)vector, *entry,
> -                            INT_REMAP_ENTRY_VECTOR_MASK,
> -                            INT_REMAP_ENTRY_VECTOR_SHIFT, entry);
> +static void update_intremap_entry(union irte_ptr entry, unsigned int vector,
> +                                  unsigned int int_type,
> +                                  unsigned int dest_mode, unsigned int dest)
> +{
> +    struct irte_basic basic = {
> +        .remap_en = 1,
> +        .sup_io_pf = 0,
> +        .int_type = int_type,
> +        .rq_eoi = 0,
> +        .dm = dest_mode,
> +        .dest = dest,
> +        .vector = vector,
> +    };
> +
> +    *entry.basic = basic;

ACCESS_ONCE(*entry.basic.raw) = basic.raw.

The problem is in an unoptimised case, structure assignment in
implemented with memcpy(), which may be implemented as `rep stosb` which
may result in a spliced write with the enable bit set first.

Using a union with raw allows for the use of ACCESS_ONCE(), which forces
the compiler to implement them as 32bit single mov's.

~Andrew

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