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

["Durrant, Paul" <pdurrant@xxxxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 26 July 2020 09:14
> To: paul@xxxxxxx
> Cc: 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx; Durrant, Paul
> <pdurrant@xxxxxxxxxxxx>; 'Lukasz Hawrylko' <lukasz.hawrylko@xxxxxxxxxxxxxxx>; 
> 'Wei Liu' <wl@xxxxxxx>;
> 'Roger Pau Monné' <roger.pau@xxxxxxxxxx>; 'Kevin Tian' <kevin.tian@xxxxxxxxx>
> Subject: RE: [EXTERNAL] [PATCH 1/6] x86/iommu: re-arrange arch_iommu to 
> separate common fields...
> 
> CAUTION: This email originated from outside of the organization. Do not click 
> links or open
> attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> On 24.07.2020 20:49, Paul Durrant wrote:
> >> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> Sent: 24 July 2020 18:29
> >>
> >> 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.
> 
> +1 for shortening in some way. Even amd_vi would already be better imo,
> albeit I'm with Andrew and would think just amd is fine here (and
> matches how things are in the file system structure).
> 

OK, I'll shorten to 'amd'.

> While at it, may I ask that you also switch the plain "int" fields to
> "unsigned int" - I think that's doable for both of them.
> 

Sure, I can do that.

  Paul

> Jan