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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

> On 15 Apr 2026, at 14:54, Luca Fancellu <Luca.Fancellu@xxxxxxx> wrote:
> 
> Hi Mykyta,
> 
>> On 14 Apr 2026, at 14:15, 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.
>> 
>> Rework dt_device_set_protected to accept a boolean parameter, update
>> callsites.
>> 
> 
> Should ...
> 
>> Tested on QEMU with SRIOV series[1] by repeatedly enabling/disabling
>> VFs.
>> 
>> [1]: https://patchew.org/Xen/cover.1772806036.git.mykyta._5Fpoturai@xxxxxxxx/
> 
> this be omitted from commit message?
> 
>> 
>> Signed-off-by: Mykyta Poturai <mykyta_poturai@xxxxxxxx>
>> ---
>> 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    | 57 +++++++++++++++++++++++-
>> xen/drivers/passthrough/arm/smmu.c       |  4 +-
>> xen/include/xen/device_tree.h            |  5 ++-
>> 4 files changed, 62 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..8e080cd7d0 100644
>> --- a/xen/drivers/passthrough/arm/smmu-v3.c
>> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
>> @@ -1493,6 +1493,60 @@ 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(u8 devfn, struct device *dev)
>> +{
>> + struct arm_smmu_master *master;
>> + struct iommu_fwspec *fwspec;
> 
> I’m looking into the arm_smmu_remove_device, at some point there we allocate

Apologies, I meant arm_smmu_add_device here

> the iommu_fwspec, but we are not using iommu_fwspec_free() here, I’ve tried
> to look around and I’m not able to see what frees that structure apart form
> iommu_remove_dt_device().
> But pci_remove_device() -> iommu_remove_device() can call this function as 
> well, I’m not sure I’ve understood
> correctly the framework here so maybe someone with more PCI experience can 
> help
> @Stewart Hildebrand ?
> 
> 
>> + struct domain *d = NULL;
>> +
>> + fwspec = dev_iommu_fwspec_get(dev);
>> + if ( !fwspec )
>> + return -ENODEV;
>> +
>> + master = dev_iommu_priv_get(dev);
>> + if ( !master )
>> + return -ENODEV;
>> +
>> + if ( IS_ENABLED(CONFIG_HAS_PCI) && dev_is_pci(dev) )
>> + {
>> + struct pci_dev *pdev = dev_to_pci(dev);
>> +
>> + /* Ignore calls for phantom functions */
>> + if ( devfn != pdev->devfn )
>> + return 0;
>> +
>> + d = pdev->domain;
>> + }
>> + else
>> + {
>> + 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;
>> + }
>> +
>> + if ( d )
>> + {
>> + int ret = arm_smmu_deassign_dev(d, devfn, dev);
>> + /* This should never fail because we already checked the domain */
>> + ASSERT(!ret);
>> + }
>> +
>> + 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);
>> + return 0;
>> +}
>> +
> 
> Cheers,
> Luca
>