Re: [PATCH v4 20/22] xen/arm: vIOMMU vSID->pSID mapping layer

[Oleksandr Tyshchenko <olekstysh@gmail.com>]

On 10/7/26 03:07, Milan Djokic wrote:

Hello Milan

> Introduce a vIOMMU mapping layer to support passthrough of IOMMU devices
> attached to different physical IOMMUs, including devices with the same
> Stream ID.
> 
> Add a new generic vIOMMU API, viommu_allocate_vsid(), which allocates a
> guest vSID and maps it to the input pSID and the backing physical SMMU.
> 
> Once the mapping is established, the guest uses the vSID for stage-1
> commands, and Xen translates the vSID to the corresponding pSID before
> processing the command on the physical SMMU.
> 
> Implement usage of the new API for dom0less guests. Allocate vSIDs when
> creating the guest device tree and replace the original pSIDs with vSIDs
> that are subsequently used by the guest driver.
> 
> Signed-off-by: Milan Djokic <milan_djokic@epam.com>
> ---
>   xen/arch/arm/include/asm/viommu.h       |  12 +++
>   xen/common/device-tree/dom0less-build.c |  33 +++++--
>   xen/drivers/passthrough/arm/smmu-v3.c   |  10 ++-
>   xen/drivers/passthrough/arm/viommu.c    |   9 ++
>   xen/drivers/passthrough/arm/vsmmu-v3.c  | 109 +++++++++++++++++++++++-
>   xen/include/xen/iommu.h                 |   6 +-
>   6 files changed, 165 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/viommu.h b/xen/arch/arm/include/asm/viommu.h
> index 5ae21042f0..3d0eb92655 100644
> --- a/xen/arch/arm/include/asm/viommu.h
> +++ b/xen/arch/arm/include/asm/viommu.h
> @@ -34,6 +34,16 @@ struct viommu_ops {
>        * Called during domain destruction to free resources used by vIOMMU.
>        */
>       int (*relinquish_resources)(struct domain *d);
> +
> +    /*
> +     * Allocate free vSID/vRID for the guest device and establish vID->pID mapping
> +     * Called during domain device assignment.
> +     * Returns 0 on success and sets vsid argument to newly allocated vSID/vRID
> +     * mapped to physical ID (id argument).
> +     * Negative error code returned if allocation fails.
> +     */
> +    int (*allocate_vsid)(struct domain *d, uint32_t id, uint32_t phandle,
> +                         uint32_t *vsid);
>   };
>   
>   struct viommu_desc {
> @@ -54,6 +64,8 @@ extern struct list_head host_iommu_list;
>   int domain_viommu_init(struct domain *d, uint8_t viommu_type);
>   int viommu_relinquish_resources(struct domain *d);
>   uint8_t viommu_get_type(void);
> +int viommu_allocate_vsid(struct domain *d, uint32_t id, uint32_t phandle,
> +                         uint32_t *vsid);
>   void add_to_host_iommu_list(paddr_t addr, paddr_t size,
>                               const struct dt_device_node *node,
>                               const void *iommu);
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 7280bcf72b..673bcbbd1c 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -31,6 +31,8 @@
>   #include <xen/static-memory.h>
>   #include <xen/static-shmem.h>
>   
> +#include <asm/viommu.h>
> +
>   #define XENSTORE_PFN_LATE_ALLOC UINT64_MAX
>   
>   static domid_t __initdata xs_domid = DOMID_INVALID;
> @@ -332,11 +334,12 @@ static int __init handle_prop_pfdt(struct kernel_info *kinfo,
>   }
>   
>   #ifdef CONFIG_ARM_VIRTUAL_IOMMU
> -static void __init modify_pfdt_node(void *pfdt, int nodeoff)
> +static void __init modify_pfdt_node(void *pfdt, int nodeoff, struct domain *d)
>   {
>       int proplen, i, rc;
>       const fdt32_t *prop;
>       fdt32_t *prop_c;
> +    uint32_t psid, phandle, vsid;
>   
>       prop = fdt_getprop(pfdt, nodeoff, "iommus", &proplen);
>       if ( !prop )
> @@ -346,10 +349,23 @@ static void __init modify_pfdt_node(void *pfdt, int nodeoff)
>       if ( !prop_c )
>           return;
>   
> +    /*
> +     * Assign <vIOMMU vSID> pairs to iommus property and establish
> +     * vSID->pSID mappings
> +     */
>       for ( i = 0; i < proplen / 8; ++i )
>       {
> +        phandle = fdt32_to_cpu(prop[i * 2]);
> +        psid = fdt32_to_cpu(prop[i * 2 + 1]);
> +        rc = viommu_allocate_vsid(d, psid, phandle, &vsid);
> +        if ( rc )
> +        {
> +            dprintk(XENLOG_ERR, "(DOMID=%u) Failed to allocate vSID for "
> +                    "pSID: %u, pIOMMU: %u", d->domain_id, psid, phandle);
> +            return;

As I mentioned in prev commit #19/22, in that case the guest DT keeps 
the host phandle and pSID while the domain creates normally since the 
failure is ignored. I think, we need to propagate the error and fail 
domain creation. What do you think?


> +        }
>           prop_c[i * 2] = cpu_to_fdt32(GUEST_PHANDLE_VSMMUV3);
> -        prop_c[i * 2 + 1] = prop[i * 2 + 1];
> +        prop_c[i * 2 + 1] = cpu_to_fdt32(vsid);
>       }
>   
>       rc = fdt_setprop(pfdt, nodeoff, "iommus", prop_c, proplen);
> @@ -362,7 +378,8 @@ static void __init modify_pfdt_node(void *pfdt, int nodeoff)
>       return;
>   }
>   #else
> -    static inline void __init modify_pfdt_node(void *pfdt, int nodeoff)
> +    static inline void __init modify_pfdt_node(void *pfdt, int nodeoff,
> +                                               struct domain *d)
>       {
>           return;
>       }
> @@ -371,7 +388,7 @@ static void __init modify_pfdt_node(void *pfdt, int nodeoff)
>   static int __init scan_pfdt_node(struct kernel_info *kinfo, void *pfdt,
>                                    int nodeoff,
>                                    uint32_t address_cells, uint32_t size_cells,
> -                                 bool scan_passthrough_prop)
> +                                 bool scan_passthrough_prop, struct domain *d)
>   {
>       int rc = 0;
>       void *fdt = kinfo->fdt;
> @@ -394,9 +411,9 @@ static int __init scan_pfdt_node(struct kernel_info *kinfo, void *pfdt,
>       node_next = fdt_first_subnode(pfdt, nodeoff);
>       while ( node_next > 0 )
>       {
> -        modify_pfdt_node(pfdt, node_next);
> +        modify_pfdt_node(pfdt, node_next, d);
>           rc = scan_pfdt_node(kinfo, pfdt, node_next, address_cells, size_cells,
> -                            scan_passthrough_prop);
> +                            scan_passthrough_prop, d);
>           if ( rc )
>               return rc;
>   
> @@ -483,7 +500,7 @@ static int __init domain_handle_dtb_boot_module(struct domain *d,
>               res = scan_pfdt_node(kinfo, pfdt, node_next,
>                                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
>                                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> -                                 false);
> +                                 false, d);
>               if ( res )
>                   goto out;
>               continue;
> @@ -493,7 +510,7 @@ static int __init domain_handle_dtb_boot_module(struct domain *d,
>               res = scan_pfdt_node(kinfo, pfdt, node_next,
>                                    DT_ROOT_NODE_ADDR_CELLS_DEFAULT,
>                                    DT_ROOT_NODE_SIZE_CELLS_DEFAULT,
> -                                 true);
> +                                 true, d);
>               if ( res )
>                   goto out;
>               continue;
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index e44f872d2d..297d2e5e7b 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -3048,7 +3048,7 @@ static struct arm_smmu_device *arm_smmu_get_by_dev(const struct device *dev)
>   }
>   
>   static struct iommu_domain *arm_smmu_get_domain_by_sid(struct domain *d,
> -				u32 sid)
> +				u32 sid, const struct arm_smmu_device *smmu)
>   {
>   	int i;
>   	unsigned long flags;
> @@ -3064,6 +3064,9 @@ static struct iommu_domain *arm_smmu_get_domain_by_sid(struct domain *d,
>   	list_for_each_entry(io_domain, &xen_domain->contexts, list) {
>   		smmu_domain = to_smmu_domain(io_domain);
>   
> +		if (smmu_domain->smmu != smmu)
> +			continue;
> +
>   		spin_lock_irqsave(&smmu_domain->devices_lock, flags);
>   		list_for_each_entry(master, &smmu_domain->devices, domain_head) {
>   			for (i = 0; i < master->num_streams; i++) {
> @@ -3292,14 +3295,15 @@ static void arm_smmu_iommu_xen_domain_teardown(struct domain *d)
>   }
>   
>   static int arm_smmu_attach_guest_config(struct domain *d, uint32_t sid,
> -		const struct iommu_guest_config *cfg)
> +				const struct iommu_guest_config *cfg,
> +				const struct arm_smmu_device *smmu)
>   {
>   	int ret = -EINVAL;
>   	unsigned long flags;
>   	struct arm_smmu_master *master;
>   	struct arm_smmu_domain *smmu_domain;
>   	struct arm_smmu_xen_domain *xen_domain = dom_iommu(d)->arch.priv;
> -	struct iommu_domain *io_domain = arm_smmu_get_domain_by_sid(d, sid);
> +	struct iommu_domain *io_domain = arm_smmu_get_domain_by_sid(d, sid, smmu);
>   
>   	if (!io_domain)
>   		return -ENODEV;
> diff --git a/xen/drivers/passthrough/arm/viommu.c b/xen/drivers/passthrough/arm/viommu.c
> index 407e68e62d..6c97acb049 100644
> --- a/xen/drivers/passthrough/arm/viommu.c
> +++ b/xen/drivers/passthrough/arm/viommu.c
> @@ -75,6 +75,15 @@ int viommu_relinquish_resources(struct domain *d)
>       return cur_viommu->ops->relinquish_resources(d);
>   }
>   
> +int viommu_allocate_vsid(struct domain *d, uint32_t id, uint32_t phandle,
> +                         uint32_t *vsid)
> +{
> +    if ( !cur_viommu )
> +        return -ENODEV;
> +
> +    return cur_viommu->ops->allocate_vsid(d, id, phandle, vsid);
> +}
> +
>   uint8_t viommu_get_type(void)
>   {
>       if ( !cur_viommu || host_iommu_list_incomplete ||
> diff --git a/xen/drivers/passthrough/arm/vsmmu-v3.c b/xen/drivers/passthrough/arm/vsmmu-v3.c
> index b71511c2eb..53fd7f2d63 100644
> --- a/xen/drivers/passthrough/arm/vsmmu-v3.c
> +++ b/xen/drivers/passthrough/arm/vsmmu-v3.c
> @@ -85,6 +85,8 @@
>   #define smmu_get_ste_s1ctxptr(x)    FIELD_PREP(STRTAB_STE_0_S1CTXPTR_MASK, \
>                                       FIELD_GET(STRTAB_STE_0_S1CTXPTR_MASK, x))
>   
> +#define MAX_VSID   (1 << SMMU_IDR1_SIDSIZE)
> +
>   /* event queue entry */
>   struct arm_smmu_evtq_ent {
>       /* Common fields */
> @@ -132,6 +134,14 @@ struct arm_vsmmu_queue {
>       uint8_t     max_n_shift;
>   };
>   
> +/* vSID->pSID mapping entry */
> +struct vsid_entry {
> +    bool        valid;
> +    uint32_t    vsid;
> +    uint32_t    psid;
> +    struct arm_smmu_device *smmu;
> +};
> +
>   struct virt_smmu {
>       struct      domain *d;
>       struct      list_head viommu_list;
> @@ -156,6 +166,7 @@ struct virt_smmu {
>       spinlock_t  cr2_lock;
>       spinlock_t  strtab_cfg_lock;
>       spinlock_t  irq_cfg_lock;
> +    struct vsid_entry *vsids;
>   };
>   
>   /* Helper functions */
> @@ -545,6 +556,49 @@ static int arm_vsmmu_handle_cfgi_ste(struct virt_smmu *smmu, uint64_t *cmdptr)
>       struct arm_vsmmu_s1_trans_cfg s1_cfg = {0};
>       uint32_t sid = smmu_cmd_get_sid(cmdptr[0]);
>       struct iommu_guest_config guest_cfg = {0};
> +    struct arm_smmu_device *phys_smmu = NULL;
> +    struct host_iommu *host_iommu;
> +    uint32_t psid;
> +    struct arm_smmu_evtq_ent ent = {
> +        .opcode = EVT_ID_BAD_STREAMID,
> +        .sid = sid,
> +        .c_bad_ste_streamid = {
> +            .ssid = 0,
> +            .ssv = false,
> +        },
> +    };
> +
> +    /* vIOMMU->pIOMMU identity mapped for HW domain */
> +    if ( is_hardware_domain(d) )
> +    {
> +        psid = sid;
> +        list_for_each_entry(host_iommu, &host_iommu_list, entry)
> +        {
> +            if ( host_iommu->addr == smmu->addr )
> +            {
> +                phys_smmu = (struct arm_smmu_device *) host_iommu->iommu;
> +                break;
> +            }
> +        }
> +    }
> +    else {
> +        /* vSID out of range or not mapped to pSID */
> +        if ( sid >= MAX_VSID || !smmu->vsids[sid].valid )
> +        {
> +            arm_vsmmu_send_event(smmu, &ent);
> +            return -EINVAL;
> +        }
> +
> +        psid = smmu->vsids[sid].psid;
> +        phys_smmu = smmu->vsids[sid].smmu;
> +    }
> +
> +    /* vSID mapped to invalid physical SMMU */
> +    if (!phys_smmu)
> +    {
> +        arm_vsmmu_send_event(smmu, &ent);
> +        return -EINVAL;
> +    }
>   
>       ret = arm_vsmmu_find_ste(smmu, sid, ste);
>       if ( ret )
> @@ -565,7 +619,7 @@ static int arm_vsmmu_handle_cfgi_ste(struct virt_smmu *smmu, uint64_t *cmdptr)
>       else
>           guest_cfg.config = ARM_SMMU_DOMAIN_S1;
>   
> -    ret = hd->platform_ops->attach_guest_config(d, sid, &guest_cfg);
> +    ret = hd->platform_ops->attach_guest_config(d, psid, &guest_cfg, phys_smmu);
>       if ( ret )
>           return ret;
>   
> @@ -1164,6 +1218,9 @@ static int vsmmuv3_init_single(struct domain *d, paddr_t addr,
>       smmu->addr = addr;
>       smmu->cmdq.ent_size = CMDQ_ENT_DWORDS * DWORDS_BYTES;
>       smmu->evtq.ent_size = EVTQ_ENT_DWORDS * DWORDS_BYTES;
> +    smmu->vsids = xzalloc_array(struct vsid_entry, MAX_VSID);

^^^ is 65536 × 24 bytes = 1.5 MiB per vSMMU allocated contiguously even 
if, lets say, the domain has two devices?

And the hwdom allocates vsids[MAX_VSID] per pSMMU even though the hwdom 
never uses it (vIOMMU->pIOMMU identity mapped for HW domain) if I got 
the code right.


> +    if ( !smmu->vsids )
> +        return -ENOMEM;

Leaks smmu when the vsids allocation fails.

and failure of vgic_reserve_virq() (non-visible in the context) leaks vsids.

>   
>       spin_lock_init(&smmu->cmd_queue_lock);
>       spin_lock_init(&smmu->evt_queue_lock);
> @@ -1254,8 +1311,9 @@ static int vsmmuv3_relinquish_resources(struct domain *d)
>       if ( list_head_is_null(&d->arch.viommu_list) )
>           return 0;
>   
> -    list_for_each_entry_safe(pos, temp, &d->arch.viommu_list, viommu_list )
> +    list_for_each_entry_safe(pos, temp, &d->arch.viommu_list, viommu_list)
>       {
> +        xfree(pos->vsids);
>           list_del(&pos->viommu_list);
>           xfree(pos);
>       }
> @@ -1263,8 +1321,55 @@ static int vsmmuv3_relinquish_resources(struct domain *d)
>       return 0;
>   }
>   
> +int vsmmuv3_allocate_vsid(struct domain *d, uint32_t id, uint32_t phandle,
> +                          uint32_t *vsid)
> +{
> +    uint32_t i = 0;
> +    struct virt_smmu *smmu;
> +    struct host_iommu *host_iommu;
> +    struct arm_smmu_device *phys_smmu = NULL;
> +
> +    if ( list_head_is_null(&d->arch.viommu_list) )
> +        return -ENODEV;
> +
> +    smmu = list_first_entry(&d->arch.viommu_list, struct virt_smmu, viommu_list);
> +
> +    /*
> +     * Find the physical SMMU corresponding to the device-tree phandle.
> +     */
> +    list_for_each_entry(host_iommu, &host_iommu_list, entry)
> +    {
> +        if ( host_iommu->dt_node->phandle == phandle )
> +        {
> +            phys_smmu = (struct arm_smmu_device *) host_iommu->iommu;
> +            break;
> +        }
> +    }
> +
> +    if ( !phys_smmu )
> +        return -ENODEV;
> +
> +    /* Get first free vSID index */
> +    while ( i < MAX_VSID && smmu->vsids[i].valid )
> +        i++;
> +
> +    /* Max number of vSIDs already allocated? */
> +    if ( i == MAX_VSID )
> +        return -ENOMEM;
> +
> +    /* Establish vSID->pSID mapping */
> +    smmu->vsids[i].valid = true;
> +    smmu->vsids[i].vsid = i;
> +    smmu->vsids[i].psid = id;
> +    smmu->vsids[i].smmu = phys_smmu;
> +    *vsid = smmu->vsids[i].vsid;
> +
> +    return 0;
> +}
> +
>   static const struct viommu_ops vsmmuv3_ops = {
>       .domain_init = domain_vsmmuv3_init,
> +    .allocate_vsid = vsmmuv3_allocate_vsid,
>       .relinquish_resources = vsmmuv3_relinquish_resources,
>   };
>   
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 253f8bb6cb..c94ee4f08e 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -312,7 +312,10 @@ static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
>   #endif /* HAS_DEVICE_TREE_DISCOVERY */
>   
>   struct page_info;
> +#ifdef CONFIG_ARM
>   struct iommu_guest_config;
> +struct arm_smmu_device;
> +#endif
>   
>   /*
>    * Any non-zero value returned from callbacks of this type will cause the
> @@ -391,7 +394,8 @@ struct iommu_ops {
>   
>   #ifdef CONFIG_ARM
>       int (*attach_guest_config)(struct domain *d, uint32_t sid,
> -                               const struct iommu_guest_config *cfg);
> +                               const struct iommu_guest_config *cfg,
> +                               const struct arm_smmu_device *smmu);
>   #endif
>   };
>