RE: [PATCH 1/6] x86/iommu: re-arrange arch_iommu to separate common fields...

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 24 July 2020 18:29
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Lukasz Hawrylko <lukasz.hawrylko@linux.intel.com>; Jan Beulich
> <jbeulich@suse.com>; Wei Liu <wl@xen.org>; Roger Pau Monné <roger.pau@citrix.com>; Kevin Tian
> <kevin.tian@intel.com>
> Subject: Re: [PATCH 1/6] x86/iommu: re-arrange arch_iommu to separate common fields...
> 
> On 24/07/2020 17:46, Paul Durrant wrote:
> > diff --git a/xen/include/asm-x86/iommu.h b/xen/include/asm-x86/iommu.h
> > index 6c9d5e5632..a7add5208c 100644
> > --- a/xen/include/asm-x86/iommu.h
> > +++ b/xen/include/asm-x86/iommu.h
> > @@ -45,16 +45,23 @@ typedef uint64_t daddr_t;
> >
> >  struct arch_iommu
> >  {
> > -    u64 pgd_maddr;                 /* io page directory machine address */
> > -    spinlock_t mapping_lock;            /* io page table lock */
> > -    int agaw;     /* adjusted guest address width, 0 is level 2 30-bit */
> > -    u64 iommu_bitmap;              /* bitmap of iommu(s) that the domain uses */
> > -    struct list_head mapped_rmrrs;
> > -
> > -    /* amd iommu support */
> > -    int paging_mode;
> > -    struct page_info *root_table;
> > -    struct guest_iommu *g_iommu;
> > +    spinlock_t mapping_lock; /* io page table lock */
> > +
> > +    union {
> > +        /* Intel VT-d */
> > +        struct {
> > +            u64 pgd_maddr; /* io page directory machine address */
> > +            int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
> > +            u64 iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
> > +            struct list_head mapped_rmrrs;
> > +        } vtd;
> > +        /* AMD IOMMU */
> > +        struct {
> > +            int paging_mode;
> > +            struct page_info *root_table;
> > +            struct guest_iommu *g_iommu;
> > +        } amd_iommu;
> > +    };
> 
> The naming split here is weird.
> 
> Ideally we'd have struct {vtd,amd}_iommu in appropriate headers, and
> this would be simply
> 
> union {
>     struct vtd_iommu vtd;
>     struct amd_iommu amd;
> };
> 
> If this isn't trivial to arrange, can we at least s/amd_iommu/amd/ here ?

I was in two minds. I tried to look for a TLA for the AMD IOMMU and 'amd' seemed a little too non-descript. I don't really mind though if there's a strong preference to shorted it.
I can certainly try moving the struct definitions into the implementation headers.

  Paul

> 
> ~Andrew