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

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

On 19-Oct-26 10:59, 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/
> 
> 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    | 72 +++++++++++++++++++++++-
>  xen/drivers/passthrough/arm/smmu.c       |  4 +-
>  xen/include/xen/device_tree.h            |  5 +-
>  4 files changed, 77 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..e1af615985 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1493,6 +1493,75 @@ 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 ( d )
> +     {
> +             /*
> +              * 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));
> +
> +             if ( arm_smmu_deassign_dev(d, devfn, dev) )
> +                     dev_err(dev, "Removing anyway\n");
> +     }
On a DT path d is always NULL. On a PCI path d is always non-NULL. So this
entire block and ...

> +
> +     arm_smmu_disable_pasid(master);
... this call can be moved to dev_is_pci branch.

> +
> +     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);
No need to call this for PCI device.

With the remarks fixed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal