Re: [RFC PATCH v7 04/14] iommu: Move IOMMU domain related structures to (arch_)iommu_context

On 20.11.2025 12:09, Teddy Astie wrote:
> Preparatory work for IOMMU redesign.
> 
> Introduce a new structure (arch_)iommu_context that will hold all
> per-IOMMU context related informations for the IOMMU drivers.
> 
> Signed-off-by Teddy Astie <[email protected]>

It's hard to see what feedback you're expecting here. For an RFC, I'm not
going to point out all the style issues. One remark, perhaps:

> --- a/xen/arch/arm/include/asm/iommu.h
> --- a/xen/arch/x86/include/asm/iommu.h
> +++ b/xen/arch/x86/include/asm/iommu.h
> @@ -31,22 +31,21 @@ typedef uint64_t daddr_t;
>  #define dfn_to_daddr(dfn) __dfn_to_daddr(dfn_x(dfn))
>  #define daddr_to_dfn(daddr) _dfn(__daddr_to_dfn(daddr))
>  
> -struct arch_iommu
> -{
> -    spinlock_t mapping_lock; /* io page table lock */
> -    struct {
> -        struct page_list_head list;
> -        spinlock_t lock;
> -    } pgtables;
> +struct iommu_context;
>  
> +struct arch_iommu_context
> +{
> +    struct page_list_head pgtables;
>      struct list_head identity_maps;
>  
> +
> +    spinlock_t mapping_lock; /* io page table lock */
> +
>      union {
>          /* Intel VT-d */
>          struct {
>              uint64_t pgd_maddr; /* io page directory machine address */
> -            unsigned int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
> -            unsigned long *iommu_bitmap; /* bitmap of iommu(s) that the domain uses */
> +            unsigned long *iommu_bitmap; /* bitmap of iommu(s) that the context uses */
>          } vtd;
>          /* AMD IOMMU */
>          struct {
> @@ -56,6 +55,24 @@ struct arch_iommu
>      };
>  };
>  
> +struct arch_iommu
> +{
> +    /* Queue for freeing pages */
> +    struct page_list_head free_queue;
> +
> +    union {
> +        /* Intel VT-d */
> +        struct {
> +            unsigned int agaw; /* adjusted guest address width, 0 is level 2 30-bit */
> +        } vtd;
> +        /* AMD IOMMU */
> +        struct {
> +            unsigned int paging_mode;
> +            struct guest_iommu *g_iommu;
> +        };
> +    };
> +};
> +
>  extern struct iommu_ops iommu_ops;
>  
>  # include <asm/alternative.h>
> @@ -109,10 +126,10 @@ static inline void iommu_disable_x2apic(void)
>          iommu_vcall(&iommu_ops, disable_x2apic);
>  }
>  
> -int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma,
> -                           paddr_t base, paddr_t end,
> +int iommu_identity_mapping(struct domain *d, struct iommu_context *ctx,
> +                           p2m_access_t p2ma, paddr_t base, paddr_t end,
>                             unsigned int flag);
> -void iommu_identity_map_teardown(struct domain *d);
> +void iommu_identity_map_teardown(struct domain *d, struct iommu_context *ctx);

At the example of these: I think it shouldn't be necessary to pass both a
context and a domain into a function. The context likely should have a
back-pointer to the domain.

Jan