| Message ID | 15-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023/8/3 8:08, Jason Gunthorpe wrote: > I've avoided doing this because there is no way to make this happen > without an intrusion into the core code. Up till now this has avoided > needing the core code's probe path with some hackery - but now that > default domains are becoming mandatory it is unavoidable. The core probe > path must be run to set the default_domain, only it can do it. Without > a default domain iommufd can't use the group. > > Make it so that iommufd selftest can create a real iommu driver and bind > it only to is own private bus. Add iommu_device_register_bus() as a core > code helper to make this possible. It simply sets the right pointers and > registers the notifier block. The mock driver then works like any normal > driver should, with probe triggered by the bus ops > > When the bus->iommu_ops stuff is fully unwound we can probably do better > here and remove this special case. > > Remove set_platform_dma_ops from selftest and make it use a BLOCKED > default domain. > > Signed-off-by: Jason Gunthorpe<jgg@nvidia.com> > --- > drivers/iommu/iommu-priv.h | 16 +++ > drivers/iommu/iommu.c | 43 +++++++ > drivers/iommu/iommufd/iommufd_private.h | 5 +- > drivers/iommu/iommufd/main.c | 8 +- > drivers/iommu/iommufd/selftest.c | 149 +++++++++++++----------- > 5 files changed, 152 insertions(+), 69 deletions(-) > create mode 100644 drivers/iommu/iommu-priv.h > > diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h > new file mode 100644 > index 00000000000000..1cbc04b9cf7297 > --- /dev/null > +++ b/drivers/iommu/iommu-priv.h > @@ -0,0 +1,16 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. > + */ > +#ifndef __IOMMU_PRIV_H > +#define __IOMMU_PRIV_H > + > +#include <linux/iommu.h> > + > +int iommu_device_register_bus(struct iommu_device *iommu, > + const struct iommu_ops *ops, struct bus_type *bus, > + struct notifier_block *nb); > +void iommu_device_unregister_bus(struct iommu_device *iommu, > + struct bus_type *bus, > + struct notifier_block *nb); > + > +#endif > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index a1a93990b3a211..7fae866af0db7a 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -36,6 +36,7 @@ > #include "dma-iommu.h" > > #include "iommu-sva.h" > +#include "iommu-priv.h" > > static struct kset *iommu_group_kset; > static DEFINE_IDA(iommu_group_ida); > @@ -290,6 +291,48 @@ void iommu_device_unregister(struct iommu_device *iommu) > } > EXPORT_SYMBOL_GPL(iommu_device_unregister); > > +#if IS_ENABLED(CONFIG_IOMMUFD_TEST) > +void iommu_device_unregister_bus(struct iommu_device *iommu, > + struct bus_type *bus, > + struct notifier_block *nb) > +{ > + bus_unregister_notifier(bus, nb); > + iommu_device_unregister(iommu); > +} > +EXPORT_SYMBOL_GPL(iommu_device_unregister_bus); > + > +/* > + * Register an iommu driver against a single bus. This is only used by iommufd > + * selftest to create a mock iommu driver. The caller must provide > + * some memory to hold a notifier_block. > + */ > +int iommu_device_register_bus(struct iommu_device *iommu, > + const struct iommu_ops *ops, struct bus_type *bus, > + struct notifier_block *nb) > +{ > + int err; > + > + iommu->ops = ops; > + nb->notifier_call = iommu_bus_notifier; > + err = bus_register_notifier(bus, nb); > + if (err) > + return err; > + > + spin_lock(&iommu_device_lock); > + list_add_tail(&iommu->list, &iommu_device_list); > + spin_unlock(&iommu_device_lock); > + > + bus->iommu_ops = ops; > + err = bus_iommu_probe(bus); > + if (err) { > + iommu_device_unregister_bus(iommu, bus, nb); > + return err; > + } > + return 0; > +} > +EXPORT_SYMBOL_GPL(iommu_device_register_bus); > +#endif Although #if IS_ENABLED(CONFIG_IOMMUFD_TEST) ... ... #endif doesn't look so comfortable, this is also the most concise method that I can think of. So, Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
On 2023/8/3 8:08, Jason Gunthorpe wrote: > +/* > + * Register an iommu driver against a single bus. This is only used by iommufd > + * selftest to create a mock iommu driver. The caller must provide > + * some memory to hold a notifier_block. > + */ > +int iommu_device_register_bus(struct iommu_device *iommu, > + const struct iommu_ops *ops, struct bus_type *bus, > + struct notifier_block *nb) > +{ > + int err; > + > + iommu->ops = ops; > + nb->notifier_call = iommu_bus_notifier; > + err = bus_register_notifier(bus, nb); > + if (err) > + return err; > + > + spin_lock(&iommu_device_lock); > + list_add_tail(&iommu->list, &iommu_device_list); > + spin_unlock(&iommu_device_lock); > + > + bus->iommu_ops = ops; > + err = bus_iommu_probe(bus); By the way, bus_iommu_probe() has been changed in iommu-next, so it needs to be rebased here. Best regards, baolu
On Wed, Aug 02, 2023 at 09:08:02PM -0300, Jason Gunthorpe wrote: > I've avoided doing this because there is no way to make this happen > without an intrusion into the core code. Up till now this has avoided > needing the core code's probe path with some hackery - but now that > default domains are becoming mandatory it is unavoidable. The core probe > path must be run to set the default_domain, only it can do it. Without > a default domain iommufd can't use the group. > > Make it so that iommufd selftest can create a real iommu driver and bind > it only to is own private bus. Add iommu_device_register_bus() as a core > code helper to make this possible. It simply sets the right pointers and > registers the notifier block. The mock driver then works like any normal > driver should, with probe triggered by the bus ops > > When the bus->iommu_ops stuff is fully unwound we can probably do better > here and remove this special case. > > Remove set_platform_dma_ops from selftest and make it use a BLOCKED > default domain. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/iommu-priv.h | 16 +++ > drivers/iommu/iommu.c | 43 +++++++ > drivers/iommu/iommufd/iommufd_private.h | 5 +- > drivers/iommu/iommufd/main.c | 8 +- > drivers/iommu/iommufd/selftest.c | 149 +++++++++++++----------- > 5 files changed, 152 insertions(+), 69 deletions(-) > create mode 100644 drivers/iommu/iommu-priv.h Since this series will miss this kernel again I've taken this patch into the iommufd tree to fix the broken selftest. It needed two edits to make it work out of the context of this series The core code still requires empty free functions: +@@ drivers/iommu/iommufd/selftest.c: static struct iommu_domain *mock_domain_alloc(unsigned int iommu_domain_type) + if (iommu_domain_type == IOMMU_DOMAIN_BLOCKED) + return &mock_blocking_domain; --static void mock_domain_blocking_free(struct iommu_domain *domain) --{ --} -- - static int mock_domain_nop_attach(struct iommu_domain *domain, - struct device *dev) - { +@@ drivers/iommu/iommufd/selftest.c: static void mock_domain_set_plaform_dma_ops(struct device *dev) + */ } - static const struct iommu_domain_ops mock_blocking_ops = { -- .free = mock_domain_blocking_free, - .attach_dev = mock_domain_nop_attach, - }; - And we can't use default_domain so rely on a NULL default domain and set_platform_dma_ops: -@@ drivers/iommu/iommufd/selftest.c: static int mock_domain_nop_attach(struct iommu_domain *domain, +- if (WARN_ON(iommu_domain_type != IOMMU_DOMAIN_UNMANAGED)) ++ if (iommu_domain_type != IOMMU_DOMAIN_UNMANAGED) + return NULL; + + mock = kzalloc(sizeof(*mock), GFP_KERNEL); -@@ drivers/iommu/iommufd/selftest.c: static bool mock_domain_capable(struct device *dev, enum iommu_cap cap) - return cap == IOMMU_CAP_CACHE_COHERENCY; - } - --static void mock_domain_set_plaform_dma_ops(struct device *dev) +static struct iommu_device mock_iommu_device = { +}; + +static struct iommu_device *mock_probe_device(struct device *dev) - { -- /* -- * mock doesn't setup default domains because we can't hook into the -- * normal probe path -- */ ++{ + return &mock_iommu_device; - } - ++} ++ static const struct iommu_ops mock_ops = { -+ /* -+ * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type() -+ * because it is zero. -+ */ -+ .default_domain = &mock_blocking_domain, .owner = THIS_MODULE, .pgsize_bitmap = MOCK_IO_PAGE_SIZE, .domain_alloc = mock_domain_alloc, .capable = mock_domain_capable, -- .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, + .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, + .device_group = generic_device_group, + .probe_device = mock_probe_device, .default_domain_ops = Otherwise it works the same and solves the same problem. There is a small merge conflict with the fixes in Joerg's tree around the bus_iommu_probe Thanks, Jason
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h new file mode 100644 index 00000000000000..1cbc04b9cf7297 --- /dev/null +++ b/drivers/iommu/iommu-priv.h @@ -0,0 +1,16 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. + */ +#ifndef __IOMMU_PRIV_H +#define __IOMMU_PRIV_H + +#include <linux/iommu.h> + +int iommu_device_register_bus(struct iommu_device *iommu, + const struct iommu_ops *ops, struct bus_type *bus, + struct notifier_block *nb); +void iommu_device_unregister_bus(struct iommu_device *iommu, + struct bus_type *bus, + struct notifier_block *nb); + +#endif diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index a1a93990b3a211..7fae866af0db7a 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -36,6 +36,7 @@ #include "dma-iommu.h" #include "iommu-sva.h" +#include "iommu-priv.h" static struct kset *iommu_group_kset; static DEFINE_IDA(iommu_group_ida); @@ -290,6 +291,48 @@ void iommu_device_unregister(struct iommu_device *iommu) } EXPORT_SYMBOL_GPL(iommu_device_unregister); +#if IS_ENABLED(CONFIG_IOMMUFD_TEST) +void iommu_device_unregister_bus(struct iommu_device *iommu, + struct bus_type *bus, + struct notifier_block *nb) +{ + bus_unregister_notifier(bus, nb); + iommu_device_unregister(iommu); +} +EXPORT_SYMBOL_GPL(iommu_device_unregister_bus); + +/* + * Register an iommu driver against a single bus. This is only used by iommufd + * selftest to create a mock iommu driver. The caller must provide + * some memory to hold a notifier_block. + */ +int iommu_device_register_bus(struct iommu_device *iommu, + const struct iommu_ops *ops, struct bus_type *bus, + struct notifier_block *nb) +{ + int err; + + iommu->ops = ops; + nb->notifier_call = iommu_bus_notifier; + err = bus_register_notifier(bus, nb); + if (err) + return err; + + spin_lock(&iommu_device_lock); + list_add_tail(&iommu->list, &iommu_device_list); + spin_unlock(&iommu_device_lock); + + bus->iommu_ops = ops; + err = bus_iommu_probe(bus); + if (err) { + iommu_device_unregister_bus(iommu, bus, nb); + return err; + } + return 0; +} +EXPORT_SYMBOL_GPL(iommu_device_register_bus); +#endif + static struct dev_iommu *dev_iommu_get(struct device *dev) { struct dev_iommu *param = dev->iommu; diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h index b38e67d1988bdb..368f66c63a239a 100644 --- a/drivers/iommu/iommufd/iommufd_private.h +++ b/drivers/iommu/iommufd/iommufd_private.h @@ -303,7 +303,7 @@ extern size_t iommufd_test_memory_limit; void iommufd_test_syz_conv_iova_id(struct iommufd_ucmd *ucmd, unsigned int ioas_id, u64 *iova, u32 *flags); bool iommufd_should_fail(void); -void __init iommufd_test_init(void); +int __init iommufd_test_init(void); void iommufd_test_exit(void); bool iommufd_selftest_is_mock_dev(struct device *dev); #else @@ -316,8 +316,9 @@ static inline bool iommufd_should_fail(void) { return false; } -static inline void __init iommufd_test_init(void) +static inline int __init iommufd_test_init(void) { + return 0; } static inline void iommufd_test_exit(void) { diff --git a/drivers/iommu/iommufd/main.c b/drivers/iommu/iommufd/main.c index 3fbe636c3d8a69..042d45cc0b1c0d 100644 --- a/drivers/iommu/iommufd/main.c +++ b/drivers/iommu/iommufd/main.c @@ -437,8 +437,14 @@ static int __init iommufd_init(void) if (ret) goto err_misc; } - iommufd_test_init(); + ret = iommufd_test_init(); + if (ret) + goto err_vfio_misc; return 0; + +err_vfio_misc: + if (IS_ENABLED(CONFIG_IOMMUFD_VFIO_CONTAINER)) + misc_deregister(&vfio_misc_dev); err_misc: misc_deregister(&iommu_misc_dev); return ret; diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c index 74c2076105d486..d2b59a1157441c 100644 --- a/drivers/iommu/iommufd/selftest.c +++ b/drivers/iommu/iommufd/selftest.c @@ -9,14 +9,17 @@ #include <linux/file.h> #include <linux/anon_inodes.h> #include <linux/fault-inject.h> +#include <linux/platform_device.h> #include <uapi/linux/iommufd.h> +#include "../iommu-priv.h" #include "io_pagetable.h" #include "iommufd_private.h" #include "iommufd_test.h" static DECLARE_FAULT_ATTR(fail_iommufd); static struct dentry *dbgfs_root; +static struct platform_device *selftest_iommu_dev; size_t iommufd_test_memory_limit = 65536; @@ -108,10 +111,6 @@ struct selftest_obj { }; }; -static void mock_domain_blocking_free(struct iommu_domain *domain) -{ -} - static int mock_domain_nop_attach(struct iommu_domain *domain, struct device *dev) { @@ -119,7 +118,6 @@ static int mock_domain_nop_attach(struct iommu_domain *domain, } static const struct iommu_domain_ops mock_blocking_ops = { - .free = mock_domain_blocking_free, .attach_dev = mock_domain_nop_attach, }; @@ -268,20 +266,26 @@ static bool mock_domain_capable(struct device *dev, enum iommu_cap cap) return cap == IOMMU_CAP_CACHE_COHERENCY; } -static void mock_domain_set_plaform_dma_ops(struct device *dev) +static struct iommu_device mock_iommu_device = { +}; + +static struct iommu_device *mock_probe_device(struct device *dev) { - /* - * mock doesn't setup default domains because we can't hook into the - * normal probe path - */ + return &mock_iommu_device; } static const struct iommu_ops mock_ops = { + /* + * IOMMU_DOMAIN_BLOCKED cannot be returned from def_domain_type() + * because it is zero. + */ + .default_domain = &mock_blocking_domain, .owner = THIS_MODULE, .pgsize_bitmap = MOCK_IO_PAGE_SIZE, .domain_alloc = mock_domain_alloc, .capable = mock_domain_capable, - .set_platform_dma_ops = mock_domain_set_plaform_dma_ops, + .device_group = generic_device_group, + .probe_device = mock_probe_device, .default_domain_ops = &(struct iommu_domain_ops){ .free = mock_domain_free, @@ -292,10 +296,6 @@ static const struct iommu_ops mock_ops = { }, }; -static struct iommu_device mock_iommu_device = { - .ops = &mock_ops, -}; - static inline struct iommufd_hw_pagetable * get_md_pagetable(struct iommufd_ucmd *ucmd, u32 mockpt_id, struct mock_iommu_domain **mock) @@ -316,22 +316,29 @@ get_md_pagetable(struct iommufd_ucmd *ucmd, u32 mockpt_id, return hwpt; } -static struct bus_type iommufd_mock_bus_type = { - .name = "iommufd_mock", - .iommu_ops = &mock_ops, +struct mock_bus_type { + struct bus_type bus; + struct notifier_block nb; }; +static struct mock_bus_type iommufd_mock_bus_type = { + .bus = { + .name = "iommufd_mock", + }, +}; + +static atomic_t mock_dev_num; + static void mock_dev_release(struct device *dev) { struct mock_dev *mdev = container_of(dev, struct mock_dev, dev); + atomic_dec(&mock_dev_num); kfree(mdev); } static struct mock_dev *mock_dev_create(void) { - struct iommu_group *iommu_group; - struct dev_iommu *dev_iommu; struct mock_dev *mdev; int rc; @@ -341,51 +348,18 @@ static struct mock_dev *mock_dev_create(void) device_initialize(&mdev->dev); mdev->dev.release = mock_dev_release; - mdev->dev.bus = &iommufd_mock_bus_type; - - iommu_group = iommu_group_alloc(); - if (IS_ERR(iommu_group)) { - rc = PTR_ERR(iommu_group); - goto err_put; - } + mdev->dev.bus = &iommufd_mock_bus_type.bus; rc = dev_set_name(&mdev->dev, "iommufd_mock%u", - iommu_group_id(iommu_group)); + atomic_inc_return(&mock_dev_num)); if (rc) - goto err_group; - - /* - * The iommu core has no way to associate a single device with an iommu - * driver (heck currently it can't even support two iommu_drivers - * registering). Hack it together with an open coded dev_iommu_get(). - * Notice that the normal notifier triggered iommu release process also - * does not work here because this bus is not in iommu_buses. - */ - mdev->dev.iommu = kzalloc(sizeof(*dev_iommu), GFP_KERNEL); - if (!mdev->dev.iommu) { - rc = -ENOMEM; - goto err_group; - } - mutex_init(&mdev->dev.iommu->lock); - mdev->dev.iommu->iommu_dev = &mock_iommu_device; + goto err_put; rc = device_add(&mdev->dev); if (rc) - goto err_dev_iommu; - - rc = iommu_group_add_device(iommu_group, &mdev->dev); - if (rc) - goto err_del; - iommu_group_put(iommu_group); + goto err_put; return mdev; -err_del: - device_del(&mdev->dev); -err_dev_iommu: - kfree(mdev->dev.iommu); - mdev->dev.iommu = NULL; -err_group: - iommu_group_put(iommu_group); err_put: put_device(&mdev->dev); return ERR_PTR(rc); @@ -393,11 +367,7 @@ static struct mock_dev *mock_dev_create(void) static void mock_dev_destroy(struct mock_dev *mdev) { - iommu_group_remove_device(&mdev->dev); - device_del(&mdev->dev); - kfree(mdev->dev.iommu); - mdev->dev.iommu = NULL; - put_device(&mdev->dev); + device_unregister(&mdev->dev); } bool iommufd_selftest_is_mock_dev(struct device *dev) @@ -443,9 +413,14 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd, /* Userspace must destroy the device_id to destroy the object */ cmd->mock_domain.out_hwpt_id = pt_id; cmd->mock_domain.out_stdev_id = sobj->obj.id; + rc = iommufd_ucmd_respond(ucmd, sizeof(*cmd)); + if (rc) + goto out_detach; iommufd_object_finalize(ucmd->ictx, &sobj->obj); - return iommufd_ucmd_respond(ucmd, sizeof(*cmd)); + return 0; +out_detach: + iommufd_device_detach(idev); out_unbind: iommufd_device_unbind(idev); out_mdev: @@ -992,15 +967,57 @@ bool iommufd_should_fail(void) return should_fail(&fail_iommufd, 1); } -void __init iommufd_test_init(void) +int __init iommufd_test_init(void) { + struct platform_device_info pdevinfo = { + .name = "iommufd_selftest_iommu", + }; + int rc; + dbgfs_root = fault_create_debugfs_attr("fail_iommufd", NULL, &fail_iommufd); - WARN_ON(bus_register(&iommufd_mock_bus_type)); + + selftest_iommu_dev = platform_device_register_full(&pdevinfo); + if (IS_ERR(selftest_iommu_dev)) { + rc = PTR_ERR(selftest_iommu_dev); + goto err_dbgfs; + } + + rc = bus_register(&iommufd_mock_bus_type.bus); + if (rc) + goto err_platform; + + rc = iommu_device_sysfs_add(&mock_iommu_device, + &selftest_iommu_dev->dev, NULL, "%s", + dev_name(&selftest_iommu_dev->dev)); + if (rc) + goto err_bus; + + rc = iommu_device_register_bus(&mock_iommu_device, &mock_ops, + &iommufd_mock_bus_type.bus, + &iommufd_mock_bus_type.nb); + if (rc) + goto err_sysfs; + return 0; + +err_sysfs: + iommu_device_sysfs_remove(&mock_iommu_device); +err_bus: + bus_unregister(&iommufd_mock_bus_type.bus); +err_platform: + platform_device_del(selftest_iommu_dev); +err_dbgfs: + debugfs_remove_recursive(dbgfs_root); + return rc; } void iommufd_test_exit(void) { + iommu_device_sysfs_remove(&mock_iommu_device); + iommu_device_unregister_bus(&mock_iommu_device, + &iommufd_mock_bus_type.bus, + &iommufd_mock_bus_type.nb); + bus_unregister(&iommufd_mock_bus_type.bus); + platform_device_del(selftest_iommu_dev); debugfs_remove_recursive(dbgfs_root); - bus_unregister(&iommufd_mock_bus_type); }
I've avoided doing this because there is no way to make this happen without an intrusion into the core code. Up till now this has avoided needing the core code's probe path with some hackery - but now that default domains are becoming mandatory it is unavoidable. The core probe path must be run to set the default_domain, only it can do it. Without a default domain iommufd can't use the group. Make it so that iommufd selftest can create a real iommu driver and bind it only to is own private bus. Add iommu_device_register_bus() as a core code helper to make this possible. It simply sets the right pointers and registers the notifier block. The mock driver then works like any normal driver should, with probe triggered by the bus ops When the bus->iommu_ops stuff is fully unwound we can probably do better here and remove this special case. Remove set_platform_dma_ops from selftest and make it use a BLOCKED default domain. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/iommu-priv.h | 16 +++ drivers/iommu/iommu.c | 43 +++++++ drivers/iommu/iommufd/iommufd_private.h | 5 +- drivers/iommu/iommufd/main.c | 8 +- drivers/iommu/iommufd/selftest.c | 149 +++++++++++++----------- 5 files changed, 152 insertions(+), 69 deletions(-) create mode 100644 drivers/iommu/iommu-priv.h