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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 25-Aug-26 09:39, Mykyta Poturai 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>
> ---
> 
> Tested on QEMU with SRIOV series[1] by repeatedly enabling/disabling
> VFs.
> 
> [1]: https://patchew.org/Xen/cover.1772806036.git.mykyta._5Fpoturai@xxxxxxxx/
> 
> 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    | 77 +++++++++++++++++++++++-
>  xen/drivers/passthrough/arm/smmu.c       |  4 +-
>  xen/include/xen/device_tree.h            |  5 +-
>  4 files changed, 82 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..3578f36259 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1493,6 +1493,80 @@ 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;
> +     struct domain *d = NULL;
> +
> +     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());
> +
> +             d = pdev->domain;
> +     }
> +     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);
> +             if ( master->domain && master->domain->d )
> +                     d = master->domain->d;
Given the comment below and `ASSERT(dev_is_pci(dev))`, `d` can never be != NULL
in DT case, so these two lines can be dropped.

> +     }
> +
> +     if ( d )
> +     {
> +             int ret;
> +
> +             /*
> +              * For DT devices, iommu_remove_dt_device() returns -EBUSY if 
> the
> +              * device is still assigned, so d is always NULL on the DT path.
> +              */
> +             ASSERT(dev_is_pci(dev));
> +
> +             ret = arm_smmu_deassign_dev(d, devfn, dev);
> +             /* This should never fail because we already checked the domain 
> */
> +             ASSERT(!ret);
You checked the domain but the assignment takes place before
`arm_smmu_attach_dev()` can fail in .assign_device, so I think it's still
possible and the ASSERT should be dropped.

> +     }
> +
> +     arm_smmu_disable_pasid(master);
> +
> +     dev_info(dev, "Removed master device (SMMUv3 %s StreamIds %u)\n",
> +              dev_name(fwspec->iommu_dev), fwspec->num_ids);
> +
> +     xfree(master);
> +     dev_iommu_priv_set(dev, NULL);
> +
> +     /*
> +      * For DT devices the fwspec is freed by iommu subsystem, but for PCI
> +      * devices we need to free it here
> +      */
> +     if ( IS_ENABLED(CONFIG_HAS_PCI) && dev_is_pci(dev) )
No need for IS_ENABLED here.

> +         iommu_fwspec_free(dev);
Spaces but should be tabs.

~Michal