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.vik...@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
