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).
>
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -23,6 +23,23 @@
>  #include <asm/io_apic.h>
>  #include <xen/keyhandler.h>
>  
> +struct irte_basic {

I'd suggest irte_32, to go with irte_128 in the following patch. 

The 128bit format is also used for posted interrupts, and isn't specific
to x2apic support.

Furthermore, calling it irte_full isn't a term I can see in the manual,
and is falling into the naming trap that USB currently lives in.

> +    unsigned int remap_en:1;
> +    unsigned int sup_io_pf:1;
> +    unsigned int int_type:3;
> +    unsigned int rq_eoi:1;
> +    unsigned int dm:1;
> +    unsigned int guest_mode:1; /* MBZ */
> +    unsigned int dest:8;
> +    unsigned int vector:8;
> +    unsigned int :8;
> +};
> +
> +union irte_ptr {
> +    void *raw;
> +    struct irte_basic *basic;
> +};
> +
>  #define INTREMAP_TABLE_ORDER    1
>  #define INTREMAP_LENGTH 0xB
>  #define INTREMAP_ENTRIES (1 << INTREMAP_LENGTH)
> @@ -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)

As this is changing, s/offset/entry/ to avoid any confusion where offset
might be in units of bytes.

~Andrew

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