Re: [PATCH v6] xen/arm: smmuv3: Add support for removing devices

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Mykyta,

> On 27 Oct 2026, at 14:31, Mykyta Poturai <Mykyta_Poturai@xxxxxxxx> wrote:
> 
> Allow for removing devices from SMMUv3. arm_smmu_deassign_dev handles
> most of the work by disabling ATS and zeroing STEs. Additionally, unset
> the dt_device_is_protected flag and free no longer needed smmu_master.
> Free iommu_fwspec for PCI devices only, for DT devices it is handled by
> generic IOMMU layer.
> 
> Rework dt_device_set_protected to accept a boolean parameter, update
> callsites.
> 
> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

For SMMU side of things:

Asked-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>

Cheers
Bertrand

> ---
> 
> Tested on QEMU with SRIOV series[1] by repeatedly enabling/disabling
> VFs.
> 
> [1]: https://patchew.org/Xen/cover.1772806036.git.mykyta._5Fpoturai@xxxxxxxx/
> 
> V5->V6:
> * move deassign and disable_pasid into the PCI branch
> * set dev_iommu_priv to NULL for DT devices only
> * collect RB
> 
> V4->V5:
> * drop dead domain lookup on the DT path
> * remove assert on  arm_smmu_deassign_dev error
> * drop redundant IS_ENABLED(CONFIG_HAS_PCI)
> * fix indentation
> 
> V3->V4:
> * s/u8/uint8_t/
> * assert pcidevs_locked
> * assert deassignment is only reachable by PCI
> * fix build error with HAS_PCI=n
> 
> V2->V3:
> * free fwspec for pci devices
> * remove testing note from commit message
> 
> V1->V2:
> * check for phantom functions
> * simplify pci/dt device split
> * improve error handling
> * don't try to free master for unprotected devices
> * rework dt_device_set_protected
> ---
> xen/drivers/passthrough/arm/ipmmu-vmsa.c |  2 +-
> xen/drivers/passthrough/arm/smmu-v3.c    | 60 +++++++++++++++++++++++-
> xen/drivers/passthrough/arm/smmu.c       |  4 +-
> xen/include/xen/device_tree.h            |  5 +-
> 4 files changed, 65 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/ipmmu-vmsa.c 
> b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> index fa9ab9cb13..0648f9b407 100644
> --- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> +++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
> @@ -1367,7 +1367,7 @@ static int ipmmu_add_device(u8 devfn, struct device 
> *dev)
>         }
> 
>         /* Let Xen know that the master device is protected by an IOMMU. */
> -        dt_device_set_protected(dev_to_dt(dev));
> +        dt_device_set_protected(dev_to_dt(dev), true);
>     }
> #ifdef CONFIG_HAS_PCI
>     if ( dev_is_pci(dev) )
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> b/xen/drivers/passthrough/arm/smmu-v3.c
> index bf153227db..232df2421c 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1493,6 +1493,63 @@ static int arm_smmu_assign_dev(struct domain *d, u8 
> devfn, struct device *dev,
> static int arm_smmu_deassign_dev(struct domain *d, uint8_t devfn,
>                                struct device *dev);
> 
> +static int arm_smmu_remove_device(uint8_t devfn, struct device *dev)
> +{
> +     struct arm_smmu_master *master;
> +     struct iommu_fwspec *fwspec;
> +
> +     fwspec = dev_iommu_fwspec_get(dev);
> +     if ( !fwspec )
> +             return -ENODEV;
> +
> +     master = dev_iommu_priv_get(dev);
> +     if ( !master )
> +             return -ENODEV;
> +
> +#ifdef CONFIG_HAS_PCI
> +     if ( dev_is_pci(dev) )
> +     {
> +             struct pci_dev *pdev = dev_to_pci(dev);
> +
> +             /* Ignore calls for phantom functions */
> +             if ( devfn != pdev->devfn )
> +                     return 0;
> +
> +             ASSERT(pcidevs_locked());
> +
> +             if ( arm_smmu_deassign_dev(pdev->domain, devfn, dev) )
> +                     dev_err(dev, "Removing anyway\n");
> +
> +             arm_smmu_disable_pasid(master);
> +     }
> +     else
> +#endif
> +     {
> +             if ( !dt_device_is_protected(dev_to_dt(dev)) )
> +             {
> +                     dev_err(dev, "Not added to SMMUv3\n");
> +                     return -ENODEV;
> +             }
> +
> +             dt_device_set_protected(dev_to_dt(dev), false);
> +             dev_iommu_priv_set(dev, NULL);
> +     }
> +
> +     dev_info(dev, "Removed master device (SMMUv3 %s StreamIds %u)\n",
> +              dev_name(fwspec->iommu_dev), fwspec->num_ids);
> +
> +     xfree(master);
> +
> +     /*
> +      * For DT devices the fwspec is freed by iommu subsystem, but for PCI
> +      * devices we need to free it here
> +      */
> +     if ( dev_is_pci(dev) )
> +             iommu_fwspec_free(dev);
> +
> +     return 0;
> +}
> +
> static int arm_smmu_add_device(u8 devfn, struct device *dev)
> {
>       int i, ret;
> @@ -1571,7 +1628,7 @@ static int arm_smmu_add_device(u8 devfn, struct device 
> *dev)
>               }
> 
>               /* Let Xen know that the master device is protected by an 
> IOMMU. */
> -             dt_device_set_protected(dev_to_dt(dev));
> +             dt_device_set_protected(dev_to_dt(dev), true);
>       }
> 
>       dev_info(dev, "Added master device (SMMUv3 %s StreamIds %u)\n",
> @@ -2867,6 +2924,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>       .unmap_page             = arm_iommu_unmap_page,
>       .dt_xlate               = arm_smmu_dt_xlate,
>       .add_device             = arm_smmu_add_device,
> +     .remove_device          = arm_smmu_remove_device,
> };
> 
> static __init int arm_smmu_dt_init(struct dt_device_node *dev,
> diff --git a/xen/drivers/passthrough/arm/smmu.c 
> b/xen/drivers/passthrough/arm/smmu.c
> index d63c901551..4d2f71f152 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -825,7 +825,7 @@ static int arm_smmu_dt_add_device_legacy(struct 
> arm_smmu_device *smmu,
>       if ( !dev_is_pci(dev) )
>       {
>               /* Xen: Let Xen know that the device is protected by an SMMU */
> -             dt_device_set_protected(dev_node);
> +             dt_device_set_protected(dev_node, true);
>       }
> 
>       for (i = 0; i < fwspec->num_ids; ++i) {
> @@ -862,7 +862,7 @@ static int arm_smmu_dt_remove_device_legacy(struct 
> arm_smmu_device *smmu,
> 
>       if ( !dev_is_pci(dev) )
>               /* Protected by dt_host_lock and dtdevs_lock as caller holds 
> these locks. */
> -             dev_node->is_protected = false;
> +             dt_device_set_protected(dev_node, false);
> 
>       kfree(master);
>       return 0;
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index 06d7643622..76ae1e674a 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -300,9 +300,10 @@ static inline domid_t dt_device_used_by(const struct 
> dt_device_node *device)
>     return device->used_by;
> }
> 
> -static inline void dt_device_set_protected(struct dt_device_node *device)
> +static inline void dt_device_set_protected(struct dt_device_node *device,
> +                                           bool protected)
> {
> -    device->is_protected = true;
> +    device->is_protected = protected;
> }
> 
> static inline bool dt_device_is_protected(const struct dt_device_node *device)
> -- 
> 2.55.0