iommu_remove_dt_device function is introduced for supporting dynamic programming
i.e. adding and removing a node during runtime. When user removes the device
node, iommu_remove_dt_device() removes the device entry from smmu-masters too
using following steps:
    1. Find if SMMU master exists for the device node.
    2. Remove the SMMU master.

Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
---
 xen/drivers/passthrough/arm/smmu.c    | 53 +++++++++++++++++++++++++++++++++++
 xen/drivers/passthrough/device_tree.c | 30 ++++++++++++++++++++
 xen/include/xen/iommu.h               |  2 ++
 3 files changed, 85 insertions(+)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index c9dfc4c..7b615bc 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -816,6 +816,17 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
 	return 0;
 }
 
+static int remove_smmu_master(struct arm_smmu_device *smmu,
+			      struct arm_smmu_master *master)
+{
+	if (!(smmu->masters.rb_node))
+		return -ENOENT;
+
+	rb_erase(&master->node, &smmu->masters);
+
+	return 0;
+}
+
 static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 					 struct device *dev,
 					 struct iommu_fwspec *fwspec)
@@ -853,6 +864,31 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
 	return insert_smmu_master(smmu, master);
 }
 
+static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
+					 struct device *dev)
+{
+	struct arm_smmu_master *master;
+	struct device_node *dev_node = dev_get_dev_node(dev);
+	int ret;
+
+	master = find_smmu_master(smmu, dev_node);
+	if (master == NULL) {
+		dev_err(dev,
+			"No registrations found for master device %s\n",
+			dev_node->name);
+		return -EINVAL;
+	}
+
+	ret = remove_smmu_master(smmu, master);
+
+	if (ret)
+		return ret;
+
+	master->of_node = NULL;
+	kfree(master);
+	return 0;
+}
+
 static int register_smmu_master(struct arm_smmu_device *smmu,
 				struct device *dev,
 				struct of_phandle_args *masterspec)
@@ -876,6 +912,22 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 					     fwspec);
 }
 
+static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
+{
+	struct arm_smmu_device *smmu;
+	struct iommu_fwspec *fwspec;
+
+	fwspec = dev_iommu_fwspec_get(dev);
+	if (fwspec == NULL)
+		return -ENXIO;
+
+	smmu = find_smmu(fwspec->iommu_dev);
+	if (smmu == NULL)
+		return -ENXIO;
+
+	return arm_smmu_dt_remove_device_legacy(smmu, dev);
+}
+
 static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2876,6 +2928,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
     .add_device = arm_smmu_dt_add_device_generic,
+    .remove_device = arm_smmu_dt_remove_device_generic,
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index 98f2aa0..37f4945 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -127,6 +127,36 @@ int iommu_release_dt_devices(struct domain *d)
     return 0;
 }
 
+int iommu_remove_dt_device(struct dt_device_node *np)
+{
+    const struct iommu_ops *ops = iommu_get_ops();
+    struct device *dev = dt_to_dev(np);
+    int rc = 1;
+
+    if ( !ops )
+        return -EINVAL;
+
+    if ( iommu_dt_device_is_assigned(np) )
+        return -EPERM;
+
+    /*
+     * The driver which supports generic IOMMU DT bindings must have
+     * these callback implemented.
+     */
+    if ( !ops->remove_device )
+        return -EINVAL;
+
+    /*
+     * Remove master device from the IOMMU if latter is present and available.
+     */
+    rc = ops->remove_device(0, dev);
+
+    if ( rc == 0 )
+        iommu_fwspec_free(dev);
+
+    return rc;
+}
+
 int iommu_add_dt_device(struct dt_device_node *np)
 {
     const struct iommu_ops *ops = iommu_get_ops();
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 6b2cdff..c4d5d12 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -215,6 +215,8 @@ int iommu_release_dt_devices(struct domain *d);
  */
 int iommu_add_dt_device(struct dt_device_node *np);
 
+int iommu_remove_dt_device(struct dt_device_node *np);
+
 int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
                        XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
 
-- 
2.7.4

Hi Vikram,

On 02/09/2021 07:05, Vikram Garhwal wrote:
> iommu_remove_dt_device function is introduced for supporting dynamic programming
> i.e. adding and removing a node during runtime. When user removes the device
> node, iommu_remove_dt_device() removes the device entry from smmu-masters too
> using following steps:
>      1. Find if SMMU master exists for the device node.
>      2. Remove the SMMU master.

I would prefer if this patch is split in two:
   * Part 1: Add the generic helper
   * Part 2: Implement the callback for the SMMU

> 
> Signed-off-by: Vikram Garhwal <fnu.vikram@xilinx.com>
> ---
>   xen/drivers/passthrough/arm/smmu.c    | 53 +++++++++++++++++++++++++++++++++++
>   xen/drivers/passthrough/device_tree.c | 30 ++++++++++++++++++++
>   xen/include/xen/iommu.h               |  2 ++
>   3 files changed, 85 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index c9dfc4c..7b615bc 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -816,6 +816,17 @@ static int insert_smmu_master(struct arm_smmu_device *smmu,
>   	return 0;
>   }
>   
> +static int remove_smmu_master(struct arm_smmu_device *smmu,
> +			      struct arm_smmu_master *master)
> +{
> +	if (!(smmu->masters.rb_node))
> +		return -ENOENT;
> +
> +	rb_erase(&master->node, &smmu->masters);
> +
> +	return 0;
> +}
> +
>   static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
>   					 struct device *dev,
>   					 struct iommu_fwspec *fwspec)
> @@ -853,6 +864,31 @@ static int arm_smmu_dt_add_device_legacy(struct arm_smmu_device *smmu,
>   	return insert_smmu_master(smmu, master);
>   }
>   
> +static int arm_smmu_dt_remove_device_legacy(struct arm_smmu_device *smmu,
> +					 struct device *dev)
> +{
> +	struct arm_smmu_master *master;
> +	struct device_node *dev_node = dev_get_dev_node(dev);
> +	int ret;
> +
> +	master = find_smmu_master(smmu, dev_node);
> +	if (master == NULL) {
> +		dev_err(dev,
> +			"No registrations found for master device %s\n",
> +			dev_node->name);
> +		return -EINVAL;
> +	}
> +
> +	ret = remove_smmu_master(smmu, master);

Can you add a comment why you are not clearing the dev_node->is_protected?

> +
> +	if (ret)
> +		return ret;
> +
> +	master->of_node = NULL;

NIT: This is a bit pointless to do given that you are freeing master 
right after.

> +	kfree(master);
> +	return 0;
> +}
> +
>   static int register_smmu_master(struct arm_smmu_device *smmu,
>   				struct device *dev,
>   				struct of_phandle_args *masterspec)
> @@ -876,6 +912,22 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   					     fwspec);
>   }
>   
> +static int arm_smmu_dt_remove_device_generic(u8 devfn, struct device *dev)
> +{
> +	struct arm_smmu_device *smmu;
> +	struct iommu_fwspec *fwspec;
> +
> +	fwspec = dev_iommu_fwspec_get(dev);
> +	if (fwspec == NULL)
> +		return -ENXIO;
> +
> +	smmu = find_smmu(fwspec->iommu_dev);
> +	if (smmu == NULL)
> +		return -ENXIO;
> +
> +	return arm_smmu_dt_remove_device_legacy(smmu, dev);
> +}
> +
>   static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
>   {
>   	struct arm_smmu_device *smmu;
> @@ -2876,6 +2928,7 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>       .init = arm_smmu_iommu_domain_init,
>       .hwdom_init = arm_smmu_iommu_hwdom_init,
>       .add_device = arm_smmu_dt_add_device_generic,
> +    .remove_device = arm_smmu_dt_remove_device_generic,
>       .teardown = arm_smmu_iommu_domain_teardown,
>       .iotlb_flush = arm_smmu_iotlb_flush,
>       .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index 98f2aa0..37f4945 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -127,6 +127,36 @@ int iommu_release_dt_devices(struct domain *d)
>       return 0;
>   }
>   
> +int iommu_remove_dt_device(struct dt_device_node *np)
> +{
> +    const struct iommu_ops *ops = iommu_get_ops();
> +    struct device *dev = dt_to_dev(np);
> +    int rc = 1;

You set rc to 1 but AFAICT the value is never used.

> +
> +    if ( !ops )
> +        return -EINVAL;

It would be better to return -EOPNOSUPP.

> +
> +    if ( iommu_dt_device_is_assigned(np) )
> +        return -EPERM;
> +
> +    /*
> +     * The driver which supports generic IOMMU DT bindings must have
> +     * these callback implemented.
> +     */

I think we should make ops->remove_device optional.

> +    if ( !ops->remove_device )
> +        return -EINVAL;
It would be better to return -EOPNOSUPP.

> +
> +    /*
> +     * Remove master device from the IOMMU if latter is present and available.
> +     */
> +    rc = ops->remove_device(0, dev);

This will need to be interlocked with other IOMMU request.

> +
> +    if ( rc == 0 )
> +        iommu_fwspec_free(dev);
> +
> +    return rc;
> +}
> +
>   int iommu_add_dt_device(struct dt_device_node *np)
>   {
>       const struct iommu_ops *ops = iommu_get_ops();
> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
> index 6b2cdff..c4d5d12 100644
> --- a/xen/include/xen/iommu.h
> +++ b/xen/include/xen/iommu.h
> @@ -215,6 +215,8 @@ int iommu_release_dt_devices(struct domain *d);
>    */
>   int iommu_add_dt_device(struct dt_device_node *np);
>   
> +int iommu_remove_dt_device(struct dt_device_node *np);
> +
>   int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>                          XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>   
> 

Cheers,

-- 
Julien Grall