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@epam.com>
---
Tested on QEMU with SRIOV series[1] by repeatedly enabling/disabling
VFs.

[1]: https://patchew.org/Xen/cover.1772806036.git.mykyta._5Fpoturai@epam.com/

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    | 65 +++++++++++++++++++++++-
 xen/drivers/passthrough/arm/smmu.c       |  4 +-
 xen/include/xen/device_tree.h            |  5 +-
 4 files changed, 70 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..a46c421399 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -1493,6 +1493,68 @@ 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;
+	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);
+
+	/*
+	 * 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) )
+	    iommu_fwspec_free(dev);
+
+	return 0;
+}
+
 static int arm_smmu_add_device(u8 devfn, struct device *dev)
 {
 	int i, ret;
@@ -1571,7 +1633,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 +2929,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.51.2

On 4/22/26 12:57, Mykyta Poturai wrote:

Hello Mykyta

> 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@epam.com>
> ---
> Tested on QEMU with SRIOV series[1] by repeatedly enabling/disabling
> VFs.
> 
> [1]: https://patchew.org/Xen/cover.1772806036.git.mykyta._5Fpoturai@epam.com/
> 
> 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

Thanks, this looks much better now. Mostly looks ok to me, but I have 
several comments.


> ---
>   xen/drivers/passthrough/arm/ipmmu-vmsa.c |  2 +-
>   xen/drivers/passthrough/arm/smmu-v3.c    | 65 +++++++++++++++++++++++-
>   xen/drivers/passthrough/arm/smmu.c       |  4 +-
>   xen/include/xen/device_tree.h            |  5 +-
>   4 files changed, 70 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..a46c421399 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -1493,6 +1493,68 @@ 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)

Inconsistent devfn type: arm_smmu_remove_device() uses u8,
but int (*remove_device)(uint8_t devfn, device_t *dev); uses uint8_t.


> +{
> +	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;
> +
> +	if ( IS_ENABLED(CONFIG_HAS_PCI) && dev_is_pci(dev) )
> +	{
> +		struct pci_dev *pdev = dev_to_pci(dev);

arm_smmu_assign_dev() has ASSERT(pcidevs_locked()) in its PCI path, 
while arm_smmu_remove_device() does not. pci_remove_device() holds 
pcidevs_lock() before calling iommu_remove_device().

So, I would add the same assertion here to document the requirement and 
guard against future callers.



> +
> +		/* 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;

Unless I am mistaken, for DT devices, the deassign logic is unreachable 
dead code. iommu_remove_dt_device() returns -EBUSY if the device is 
still assigned, guaranteeing master->domain == NULL. This means that ...


> +	}
> +
> +	if ( d )
> +	{

  ... this block only has effect for PCI.


I would consider adding a comment and ASSERT, something like:
/*
  * 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));


the rest looks good


> +		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);
> +
> +	/*
> +	 * 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) )
> +	    iommu_fwspec_free(dev);
> +
> +	return 0;
> +}
> +
>   static int arm_smmu_add_device(u8 devfn, struct device *dev)
>   {
>   	int i, ret;
> @@ -1571,7 +1633,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 +2929,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)