Hi,
On 19/08/2023 01:28, Vikram Garhwal wrote:
Remove master device from the IOMMU. This will be helpful when removing the
overlay nodes using dynamic programming during run time.
Signed-off-by: Vikram Garhwal
Acked-by: Jan Beulich
---
Changes from v7:
Add check if IOMMU is enabled.
Fix indentation of fail.
---
---
xen/drivers/passthrough/device_tree.c | 44 +++
xen/include/xen/iommu.h | 1 +
2 files changed, 45 insertions(+)
diff --git a/xen/drivers/passthrough/device_tree.c
b/xen/drivers/passthrough/device_tree.c
index 096ef2dd68..4cb32dc0b3 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -128,6 +128,50 @@ 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;
+
+if ( !iommu_enabled )
+return 1;
The caller doesn't seem to check if the error code is > 0. So can we
instead return a -ERRNO?
If you want to continue to return a value > 0 then I think it should be
documented in a comment like we did for iommu_add_dt_device().
+
+if ( !ops )
+return -EOPNOTSUPP;
+
+spin_lock(_lock);
+
+if ( iommu_dt_device_is_assigned_locked(np) )
+{
+rc = -EBUSY;
+goto fail;
+}
+
+/*
+ * The driver which supports generic IOMMU DT bindings must have this
+ * callback implemented.
+ */
I have questioned this message in v7 and I still question it. I guess
you copied the comment on top of add_device(), this was add there
because we have a different way to add legacy device.
But here there are no such requirement. In fact, you are not adding the
the callback to all the IOMMU drivers... Yet all of them support the
generic IOMMU DT bindings.
+if ( !ops->remove_device )
+{
+rc = -EOPNOTSUPP;
+goto fail;
+}
+
+/*
+ * Remove master device from the IOMMU if latter is present and available.
I read this as this will not return an error if the device is protected.
However, AFAICT, the implement in the SMMU driver provided in this
series will return an error. So I would suggest to replace this sentence
with:
de-register the device from the IOMMU driver.
+ * The driver is responsible for removing is_protected flag.
Can you add an assert in the 'if ( !rc )' block to confirm that
is_protected was effectively removed. Something like:
ASSERT(!dt_device_is_protected(dev));
This would help to confirm the driver is respecting what you expect.
+ */
+rc = ops->remove_device(0, dev);
+
+if ( !rc )
+iommu_fwspec_free(dev);
+
+ fail:
+spin_unlock(_lock);
+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 110693c59f..a8e9bc9a2d 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -233,6 +233,7 @@ int iommu_add_dt_device(struct dt_device_node *np);
int iommu_do_dt_domctl(struct xen_domctl *domctl, struct domain *d,
XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl);
+int iommu_remove_dt_device(struct dt_device_node *np);
#endif /* HAS_DEVICE_TREE */
Cheers,
--
Julien Grall
