Hi all,
The bus ops removal relies on iommu_ops_from_fwnode(NULL) finding the
"global" hardware driver ops on relevant platforms. Unfortunately I
overlooked that this will also match the IOMMUFD mock driver, such that
it can end up claiming to be the IOMMU for real platform etc. devices
which don't have an IOMMU described by a fwspec. This mini-series
addresses that.
I've split it so patch #1 is minimal fix that mitigates the issue and
could be applied independently, then patch #2 then builds on it to take
full advantage of the new machinery and coexist as a normal IOMMU
driver. However they could be squashed if preferred.
Thanks,
Robin.
Robin Murphy (2):
iommufd/selftest: Use a fwnode to distinguish devices
iommufd/selftest: Use normal IOMMU registration
drivers/iommu/iommu-priv.h | 7 -----
drivers/iommu/iommu.c | 46 +++-------------------------
drivers/iommu/iommufd/iommufd_test.h | 2 ++
drivers/iommu/iommufd/selftest.c | 45 +++++++++++++--------------
4 files changed, 27 insertions(+), 73 deletions(-)
--
2.39.2.101.g768bb238c484.dirty
With bus ops gone, the trick of registering against a specific bus no
longer really works, and we start getting given devices from other buses
to probe, which leads to spurious groups for devices with no IOMMU on
arm64, but may inadvertently steal devices from the real IOMMU on Intel,
AMD or S390. Driver coexistence is based on the fwspec mechanism, so
register with a non-NULL fwnode and give mock devices a corresponding
fwspec, to let the IOMMU core distinguish things correctly for us.
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommufd/selftest.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index 5d93434003d8..f46ce0f8808d 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -432,7 +432,11 @@ static bool mock_domain_capable(struct device *dev, enum iommu_cap cap)
return false;
}
+static struct fwnode_handle mock_fwnode = {
+};
+
static struct iommu_device mock_iommu_device = {
+ .fwnode = &mock_fwnode,
};
static struct iommu_device *mock_probe_device(struct device *dev)
@@ -569,12 +573,17 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
if (rc)
goto err_put;
+ rc = iommu_fwspec_init(&mdev->dev, &mock_fwnode, &mock_ops);
+ if (rc)
+ goto err_put;
+
rc = device_add(&mdev->dev);
if (rc)
goto err_put;
return mdev;
err_put:
+ iommu_fwspec_free(&mdev->dev);
put_device(&mdev->dev);
return ERR_PTR(rc);
}
--
2.39.2.101.g768bb238c484.dirty
The IOMMU core now supports coexistence of fwspec-based drivers, which
the mock driver now is, so let's bring the mock bus into iommu_buses,
drop the special interface, and use the normal registration flow. The
one concession we have to make is to ensure that the mock bus is
registered early enough so that bus_for_each_dev() doesn't error out
for other IOMMU drivers registering before iommufd_test_init() runs.
Signed-off-by: Robin Murphy <[email protected]>
---
drivers/iommu/iommu-priv.h | 7 -----
drivers/iommu/iommu.c | 46 +++-------------------------
drivers/iommu/iommufd/iommufd_test.h | 2 ++
drivers/iommu/iommufd/selftest.c | 36 ++++++++--------------
4 files changed, 18 insertions(+), 73 deletions(-)
diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index 2024a2313348..663bd4fa166f 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -20,11 +20,4 @@ static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
int iommu_group_replace_domain(struct iommu_group *group,
struct iommu_domain *new_domain);
-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 /* __LINUX_IOMMU_PRIV_H */
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 824989874dee..559999cfd9d4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -35,8 +35,8 @@
#include "dma-iommu.h"
#include "iommu-priv.h"
-
#include "iommu-sva.h"
+#include "iommufd/iommufd_test.h"
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
@@ -165,6 +165,9 @@ static const struct bus_type * const iommu_buses[] = {
#ifdef CONFIG_CDX_BUS
&cdx_bus_type,
#endif
+#ifdef CONFIG_IOMMUFD_TEST
+ &iommufd_mock_bus_type,
+#endif
};
/*
@@ -289,47 +292,6 @@ 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);
-
- 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_test.h b/drivers/iommu/iommufd/iommufd_test.h
index 7910fbe1962d..a2d2c55a9315 100644
--- a/drivers/iommu/iommufd/iommufd_test.h
+++ b/drivers/iommu/iommufd/iommufd_test.h
@@ -148,4 +148,6 @@ struct iommu_hwpt_selftest {
__u32 iotlb;
};
+extern struct bus_type iommufd_mock_bus_type;
+
#endif
diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index f46ce0f8808d..bb09abb5bcbb 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -530,15 +530,8 @@ get_md_pagetable_nested(struct iommufd_ucmd *ucmd, u32 mockpt_id,
return hwpt;
}
-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",
- },
+struct bus_type iommufd_mock_bus_type = {
+ .name = "iommufd_mock",
};
static atomic_t mock_dev_num;
@@ -566,7 +559,7 @@ static struct mock_dev *mock_dev_create(unsigned long dev_flags)
device_initialize(&mdev->dev);
mdev->flags = dev_flags;
mdev->dev.release = mock_dev_release;
- mdev->dev.bus = &iommufd_mock_bus_type.bus;
+ mdev->dev.bus = &iommufd_mock_bus_type;
rc = dev_set_name(&mdev->dev, "iommufd_mock%u",
atomic_inc_return(&mock_dev_num));
@@ -1327,6 +1320,12 @@ bool iommufd_should_fail(void)
return should_fail(&fail_iommufd, 1);
}
+int __init iommufd_bus_init(void)
+{
+ return bus_register(&iommufd_mock_bus_type);
+}
+subsys_initcall(iommufd_bus_init);
+
int __init iommufd_test_init(void)
{
struct platform_device_info pdevinfo = {
@@ -1343,27 +1342,19 @@ int __init iommufd_test_init(void)
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;
+ goto err_platform;
- rc = iommu_device_register_bus(&mock_iommu_device, &mock_ops,
- &iommufd_mock_bus_type.bus,
- &iommufd_mock_bus_type.nb);
+ rc = iommu_device_register(&mock_iommu_device, &mock_ops, NULL);
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_unregister(selftest_iommu_dev);
err_dbgfs:
@@ -1374,10 +1365,7 @@ int __init iommufd_test_init(void)
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);
+ iommu_device_unregister(&mock_iommu_device);
platform_device_unregister(selftest_iommu_dev);
debugfs_remove_recursive(dbgfs_root);
}
--
2.39.2.101.g768bb238c484.dirty
On Tue, Nov 28, 2023 at 10:42:12AM +0000, Robin Murphy wrote:
> The IOMMU core now supports coexistence of fwspec-based drivers, which
> the mock driver now is, so let's bring the mock bus into iommu_buses,
> drop the special interface, and use the normal registration flow. The
> one concession we have to make is to ensure that the mock bus is
> registered early enough so that bus_for_each_dev() doesn't error out
> for other IOMMU drivers registering before iommufd_test_init() runs.
This makes iommufd non-modular which becomes a total PITA for development :(
Jason
On Tue, Nov 28, 2023 at 10:42:11AM +0000, Robin Murphy wrote:
> With bus ops gone, the trick of registering against a specific bus no
> longer really works, and we start getting given devices from other buses
> to probe,
Make sense
> which leads to spurious groups for devices with no IOMMU on
> arm64,
I'm not sure I'm fully understanding what this means?
I guess that the mock driver is matching random things once it starts
being called all the time because this is missing:
static struct iommu_device *mock_probe_device(struct device *dev)
{
+ if (dev->bus != &iommufd_mock_bus_type)
+ return -ENODEV;
return &mock_iommu_device;
}
Is that sufficient to solve the problem?
> but may inadvertently steal devices from the real IOMMU on Intel,
> AMD or S390.
AMD/Intel/S390 drivers already reject bus's they don't understand.
Intel's device_to_iommu() will fail because
for_each_active_dev_scope() will never match the mock device.
amd fails because check_device() -> get_device_sbdf_id() fails due to
no PCI and not get_acpihid_device_id().
s390 fails because !dev_is_pci(dev).
The fwspec drivers should all fail if they don't have a fwspec, and
they shouldn't for mock bus devices since it doesn't implement
dma_configure.
Jason
On 28/11/2023 2:35 pm, Jason Gunthorpe wrote:
> On Tue, Nov 28, 2023 at 10:42:12AM +0000, Robin Murphy wrote:
>> The IOMMU core now supports coexistence of fwspec-based drivers, which
>> the mock driver now is, so let's bring the mock bus into iommu_buses,
>> drop the special interface, and use the normal registration flow. The
>> one concession we have to make is to ensure that the mock bus is
>> registered early enough so that bus_for_each_dev() doesn't error out
>> for other IOMMU drivers registering before iommufd_test_init() runs.
>
> This makes iommufd non-modular which becomes a total PITA for development :(
Oh fiddle, I misread the makefile, and indeed this doesn't work at all,
sorry (turns out it fails to even build for IOMMUFD=m...) Guess I should
have been more wary of how suspiciously easy it seemed :(
Dynamic bus registration in general would be a neat thing to explore at
some point, since the static iommu_buses array isn't my most favourite
part of the whole business, but I guess we leave this as-is for now.
Robin.
On Tue, Nov 28, 2023 at 02:53:03PM +0000, Robin Murphy wrote:
> Dynamic bus registration in general would be a neat thing to explore at some
> point, since the static iommu_buses array isn't my most favourite part of
> the whole business, but I guess we leave this as-is for now.
I have an idea for that :) For later!
Jason
On 28/11/2023 2:43 pm, Jason Gunthorpe wrote:
> On Tue, Nov 28, 2023 at 10:42:11AM +0000, Robin Murphy wrote:
>> With bus ops gone, the trick of registering against a specific bus no
>> longer really works, and we start getting given devices from other buses
>> to probe,
>
> Make sense
>
>> which leads to spurious groups for devices with no IOMMU on
>> arm64,
>
> I'm not sure I'm fully understanding what this means?
It means on my arm64 ACPI system, random platform devices which are
created after iommufd_test_init() has run get successfully probed by the
mock driver, unexpectedly:
root@crazy-taxi:~# ls /sys/kernel/iommu_groups/*/devices
/sys/kernel/iommu_groups/0/devices:
0000:07:00.0
/sys/kernel/iommu_groups/1/devices:
'Fixed MDIO bus.0'
/sys/kernel/iommu_groups/10/devices:
0001:00:00.0
/sys/kernel/iommu_groups/2/devices:
0000:04:05.0
/sys/kernel/iommu_groups/3/devices:
0000:08:00.0
/sys/kernel/iommu_groups/4/devices:
0000:09:00.0
/sys/kernel/iommu_groups/5/devices:
0001:01:00.0
/sys/kernel/iommu_groups/6/devices:
alarmtimer.2.auto
/sys/kernel/iommu_groups/7/devices:
psci-cpuidle
/sys/kernel/iommu_groups/8/devices:
snd-soc-dummy
/sys/kernel/iommu_groups/9/devices:
0000:00:00.0 0000:01:00.0 0000:02:08.0 0000:02:10.0 0000:02:11.0
0000:02:12.0 0000:02:13.0 0000:02:14.0 0000:03:00.0
root@crazy-taxi:~# cat /sys/kernel/iommu_groups/*/type
DMA
blocked
DMA
DMA
DMA
DMA
DMA
blocked
blocked
blocked
DMA
> I guess that the mock driver is matching random things once it starts
> being called all the time because this is missing:
>
> static struct iommu_device *mock_probe_device(struct device *dev)
> {
> + if (dev->bus != &iommufd_mock_bus_type)
> + return -ENODEV;
> return &mock_iommu_device;
> }
>
> Is that sufficient to solve the problem?
Unfortunately not...
>> but may inadvertently steal devices from the real IOMMU on Intel,
>> AMD or S390.
>
> AMD/Intel/S390 drivers already reject bus's they don't understand.
>
> Intel's device_to_iommu() will fail because
> for_each_active_dev_scope() will never match the mock device.
>
> amd fails because check_device() -> get_device_sbdf_id() fails due to
> no PCI and not get_acpihid_device_id().
>
> s390 fails because !dev_is_pci(dev).
Indeed, but then when such probes do fail, they've failed for good. We
don't have any way to somehow dig up the mock driver's ops and try
again, so the selftest ends up broken (i.e. the real driver "steals" the
mock devices, in the inverse of the case I was concerned about if the
mock driver somehow manages to register first).
The assumption was as commented in the code, that there would only ever
be one driver per system *not* using fwnodes, but as I say I missed the
mock driver when considering that. To be fair, I'm not sure it even
existed when I *first* wrote that code :)
I did intend coexistence to work on x86 too, where the "other" driver
would be virtio-iommu using fwnodes, so aligning the mock driver that
way seemed far neater than any more special-case hacks in core code.
> The fwspec drivers should all fail if they don't have a fwspec, and
> they shouldn't for mock bus devices since it doesn't implement
> dma_configure.
Right, the selftests still work fine on my arm64 system (and the
spurious groups happen to be benign since those aren't real DMA-capable
device anyway), but I expect they're busted on x86/s390 with today's -next.
Thanks,
Robin.
On Tue, Nov 28, 2023 at 04:02:42PM +0000, Robin Murphy wrote:
> On 28/11/2023 2:43 pm, Jason Gunthorpe wrote:
> > On Tue, Nov 28, 2023 at 10:42:11AM +0000, Robin Murphy wrote:
> > > With bus ops gone, the trick of registering against a specific bus no
> > > longer really works, and we start getting given devices from other buses
> > > to probe,
> >
> > Make sense
> >
> > > which leads to spurious groups for devices with no IOMMU on
> > > arm64,
> >
> > I'm not sure I'm fully understanding what this means?
>
> It means on my arm64 ACPI system, random platform devices which are created
> after iommufd_test_init() has run get successfully probed by the mock
> driver, unexpectedly:
Okay that is what I guessed
> > I guess that the mock driver is matching random things once it starts
> > being called all the time because this is missing:
> >
> > static struct iommu_device *mock_probe_device(struct device *dev)
> > {
> > + if (dev->bus != &iommufd_mock_bus_type)
> > + return -ENODEV;
> > return &mock_iommu_device;
> > }
> >
> > Is that sufficient to solve the problem?
>
> Unfortunately not...
I see, so we create the other problem that without bus ops we don't
get to have two 'global' drivers and with the above mock won't probe
on x86.
> I did intend coexistence to work on x86 too, where the "other" driver would
> be virtio-iommu using fwnodes, so aligning the mock driver that way seemed
> far neater than any more special-case hacks in core code.
Lets just do the above and what I suggested earlier. This is from a
WIP tree I have, it shows the idea but needs other stuff to work. If
you agree I'll pull its parts out and post a clean version of them.
commit 51c9a54cc111b4b31af6a0527015db82e782e1d3
Author: Jason Gunthorpe <[email protected]>
Date: Tue Nov 28 11:54:47 2023 -0400
iommu: Call all drivers if there is no fwspec
Real systems only have one ops, so this effectively invokes the single op
in the system to probe each device. If there are multiple ops we invoke
each one once, and drivers that don't understand the struct device should
return -ENODEV.
Signed-off-by: Jason Gunthorpe <[email protected]>
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 7468a64778931b..54e3f14429b3b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -241,6 +241,26 @@ static int remove_iommu_group(struct device *dev, void *data)
return 0;
}
+static void iommu_device_add(struct iommu_device *iommu)
+{
+ struct iommu_device *cur;
+
+ /*
+ * Keep the iommu_device_list grouped by ops so that
+ * iommu_find_init_device() works efficiently.
+ */
+ mutex_lock(&iommu_probe_device_lock);
+ list_for_each_entry(cur, &iommu_device_list, list) {
+ if (cur->ops == iommu->ops) {
+ list_add(&iommu->list, &cur->list);
+ goto out;
+ }
+ }
+ list_add(&iommu->list, &iommu_device_list);
+out:
+ mutex_unlock(&iommu_probe_device_lock);
+}
+
/**
* iommu_device_register() - Register an IOMMU hardware instance
* @iommu: IOMMU handle for the instance
@@ -262,9 +282,7 @@ int iommu_device_register(struct iommu_device *iommu,
if (hwdev)
iommu->fwnode = dev_fwnode(hwdev);
- mutex_lock(&iommu_probe_device_lock);
- list_add_tail(&iommu->list, &iommu_device_list);
- mutex_unlock(&iommu_probe_device_lock);
+ iommu_device_add(iommu);
for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++)
err = bus_iommu_probe(iommu_buses[i]);
@@ -502,6 +520,29 @@ static void iommu_deinit_device(struct device *dev)
DEFINE_MUTEX(iommu_probe_device_lock);
+static int iommu_find_init_device(struct iommu_probe_info *pinf)
+{
+ const struct iommu_ops *ops = NULL;
+ struct iommu_device *iommu;
+ int ret;
+
+ lockdep_assert_held(&iommu_probe_device_lock);
+
+ /*
+ * Each unique ops gets a chance to claim the device, -ENODEV means the
+ * driver does not support the device.
+ */
+ list_for_each_entry(iommu, &iommu_device_list, list) {
+ if (iommu->ops != ops) {
+ ops = iommu->ops;
+ ret = iommu_init_device(pinf, iommu->ops);
+ if (ret != -ENODEV)
+ return ret;
+ }
+ }
+ return -ENODEV;
+}
+
static int __iommu_probe_device(struct iommu_probe_info *pinf)
{
struct device *dev = pinf->dev;
@@ -524,13 +565,6 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
ops = fwspec->ops;
if (!ops)
return -ENODEV;
- } else {
- struct iommu_device *iommu;
-
- iommu = iommu_device_from_fwnode(NULL);
- if (!iommu)
- return -ENODEV;
- ops = iommu->ops;
}
/*
@@ -546,7 +580,10 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
if (dev->iommu_group)
return 0;
- ret = iommu_init_device(pinf, ops);
+ if (ops)
+ ret = iommu_init_device(pinf, ops);
+ else
+ ret = iommu_find_init_device(pinf);
if (ret)
return ret;
On 28/11/2023 4:33 pm, Jason Gunthorpe wrote:
> On Tue, Nov 28, 2023 at 04:02:42PM +0000, Robin Murphy wrote:
>> On 28/11/2023 2:43 pm, Jason Gunthorpe wrote:
>>> On Tue, Nov 28, 2023 at 10:42:11AM +0000, Robin Murphy wrote:
>>>> With bus ops gone, the trick of registering against a specific bus no
>>>> longer really works, and we start getting given devices from other buses
>>>> to probe,
>>>
>>> Make sense
>>>
>>>> which leads to spurious groups for devices with no IOMMU on
>>>> arm64,
>>>
>>> I'm not sure I'm fully understanding what this means?
>>
>> It means on my arm64 ACPI system, random platform devices which are created
>> after iommufd_test_init() has run get successfully probed by the mock
>> driver, unexpectedly:
>
> Okay that is what I guessed
>
>>> I guess that the mock driver is matching random things once it starts
>>> being called all the time because this is missing:
>>>
>>> static struct iommu_device *mock_probe_device(struct device *dev)
>>> {
>>> + if (dev->bus != &iommufd_mock_bus_type)
>>> + return -ENODEV;
>>> return &mock_iommu_device;
>>> }
>>>
>>> Is that sufficient to solve the problem?
>>
>> Unfortunately not...
>
> I see, so we create the other problem that without bus ops we don't
> get to have two 'global' drivers and with the above mock won't probe
> on x86.
>
>> I did intend coexistence to work on x86 too, where the "other" driver would
>> be virtio-iommu using fwnodes, so aligning the mock driver that way seemed
>> far neater than any more special-case hacks in core code.
>
> Lets just do the above and what I suggested earlier. This is from a
> WIP tree I have, it shows the idea but needs other stuff to work. If
> you agree I'll pull its parts out and post a clean version of them.
>
> commit 51c9a54cc111b4b31af6a0527015db82e782e1d3
> Author: Jason Gunthorpe <[email protected]>
> Date: Tue Nov 28 11:54:47 2023 -0400
>
> iommu: Call all drivers if there is no fwspec
>
> Real systems only have one ops, so this effectively invokes the single op
> in the system to probe each device. If there are multiple ops we invoke
> each one once, and drivers that don't understand the struct device should
> return -ENODEV.
You see this is exactly the kind of complexity I *don't* want, since the
only thing it would foreseeably benefit is the one special case of the
IOMMUFD selftest, which can far more trivially just adopt the other of
the two "standard" usage models we have. I've been trying to get *away*
from having to have boilerplate checks in all the drivers, and this
would require bringing back a load of the ones I've just removed :(
As I said before, I really want to avoid the perf_event_init model of
calling round every driver saying "hey, do you want this?" since it's
also error-prone if any of those drivers doesn't get the boilerplate
exactly right and inadvertently fails to reject something it should
have. The difference with perf is that it has the notion of generic
events which *can* be handled by more than one driver. We do not, and
conceivably never will, have that for IOMMU client devices, so we can
realistically make the core code responsible for calling the right
driver by construction, and since we're now mostly there already, it
seem by far the most sensible thing to continue in that direction.
Thanks,
Robin.
> Signed-off-by: Jason Gunthorpe <[email protected]>
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 7468a64778931b..54e3f14429b3b4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -241,6 +241,26 @@ static int remove_iommu_group(struct device *dev, void *data)
> return 0;
> }
>
> +static void iommu_device_add(struct iommu_device *iommu)
> +{
> + struct iommu_device *cur;
> +
> + /*
> + * Keep the iommu_device_list grouped by ops so that
> + * iommu_find_init_device() works efficiently.
> + */
> + mutex_lock(&iommu_probe_device_lock);
> + list_for_each_entry(cur, &iommu_device_list, list) {
> + if (cur->ops == iommu->ops) {
> + list_add(&iommu->list, &cur->list);
> + goto out;
> + }
> + }
> + list_add(&iommu->list, &iommu_device_list);
> +out:
> + mutex_unlock(&iommu_probe_device_lock);
> +}
> +
> /**
> * iommu_device_register() - Register an IOMMU hardware instance
> * @iommu: IOMMU handle for the instance
> @@ -262,9 +282,7 @@ int iommu_device_register(struct iommu_device *iommu,
> if (hwdev)
> iommu->fwnode = dev_fwnode(hwdev);
>
> - mutex_lock(&iommu_probe_device_lock);
> - list_add_tail(&iommu->list, &iommu_device_list);
> - mutex_unlock(&iommu_probe_device_lock);
> + iommu_device_add(iommu);
>
> for (int i = 0; i < ARRAY_SIZE(iommu_buses) && !err; i++)
> err = bus_iommu_probe(iommu_buses[i]);
> @@ -502,6 +520,29 @@ static void iommu_deinit_device(struct device *dev)
>
> DEFINE_MUTEX(iommu_probe_device_lock);
>
> +static int iommu_find_init_device(struct iommu_probe_info *pinf)
> +{
> + const struct iommu_ops *ops = NULL;
> + struct iommu_device *iommu;
> + int ret;
> +
> + lockdep_assert_held(&iommu_probe_device_lock);
> +
> + /*
> + * Each unique ops gets a chance to claim the device, -ENODEV means the
> + * driver does not support the device.
> + */
> + list_for_each_entry(iommu, &iommu_device_list, list) {
> + if (iommu->ops != ops) {
> + ops = iommu->ops;
> + ret = iommu_init_device(pinf, iommu->ops);
> + if (ret != -ENODEV)
> + return ret;
> + }
> + }
> + return -ENODEV;
> +}
> +
> static int __iommu_probe_device(struct iommu_probe_info *pinf)
> {
> struct device *dev = pinf->dev;
> @@ -524,13 +565,6 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
> ops = fwspec->ops;
> if (!ops)
> return -ENODEV;
> - } else {
> - struct iommu_device *iommu;
> -
> - iommu = iommu_device_from_fwnode(NULL);
> - if (!iommu)
> - return -ENODEV;
> - ops = iommu->ops;
> }
>
> /*
> @@ -546,7 +580,10 @@ static int __iommu_probe_device(struct iommu_probe_info *pinf)
> if (dev->iommu_group)
> return 0;
>
> - ret = iommu_init_device(pinf, ops);
> + if (ops)
> + ret = iommu_init_device(pinf, ops);
> + else
> + ret = iommu_find_init_device(pinf);
> if (ret)
> return ret;
>
On Tue, Nov 28, 2023 at 05:36:33PM +0000, Robin Murphy wrote:
> You see this is exactly the kind of complexity I *don't* want, since the
> only thing it would foreseeably benefit is the one special case of the
> IOMMUFD selftest, which can far more trivially just adopt the other of the
> two "standard" usage models we have. I've been trying to get *away* from
> having to have boilerplate checks in all the drivers, and this would require
> bringing back a load of the ones I've just removed :(
I don't think we need to bring back the fwspec checks you removed, the
loop just needs to keep the NULL check:
+ list_for_each_entry(iommu, &iommu_device_list, list) {
+ if (iommu->ops != ops && !iommu->fwnode) {
+ ops = iommu->ops;
+ ret = iommu_init_device(pinf, iommu->ops);
+ if (ret != -ENODEV)
+ return ret;
+ }
+ }
Iterate over all the global driver ops only. Drivers with a fwnode
will never be called without a fwspec.
Also, does omap have problems now too? omap seems to set fwnode but
does some slightly different open coded non-fwspec parsing that worked
at bus time? Is it still OK? Does fwspec even find ops in omap's FW
description (ie it looks like it make iommu-cells optional or
something)?
> As I said before, I really want to avoid the perf_event_init model of
> calling round every driver saying "hey, do you want this?" since it's also
> error-prone if any of those drivers doesn't get the boilerplate exactly
> right and inadvertently fails to reject something it should have.
The core missed an API that every driver needs: give me the struct
iommu_driver* the FW has referenced.
Instead every driver open codes something like
arm_smmu_get_by_fwnode(), or much worse.
If we force the drivers to say
iommu_driver = iommu_fw_give_me_my_driver(dev, ops)
Then we automatically have a place to do all the rejection checks we
need, and driver's can't inadvertently skip this because they really
can't work without the iommu_driver at all.
Anyhow, I completed the series I talked about yesterday. It turned out
really nice I think, especially the driver facing API is much
cleaner. I'm just going through the last bits before I share it.
Jason
Hi Robin,
kernel test robot noticed the following build errors:
[auto build test ERROR on next-20231128]
[cannot apply to joro-iommu/next v6.7-rc3 v6.7-rc2 v6.7-rc1 linus/master v6.7-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommufd-selftest-Use-a-fwnode-to-distinguish-devices/20231128-185058
base: next-20231128
patch link: https://lore.kernel.org/r/44ee6854da69e86b208f49752f60a4c18205c32a.1701165201.git.robin.murphy%40arm.com
patch subject: [PATCH 2/2] iommufd/selftest: Use normal IOMMU registration
config: parisc-allmodconfig (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
hppa-linux-ld: drivers/iommu/iommufd/selftest.o: in function `iommufd_bus_init':
>> (.init.text+0x10): multiple definition of `init_module'; drivers/iommu/iommufd/main.o:(.init.text+0x10): first defined here
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Robin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on next-20231128]
[cannot apply to joro-iommu/next v6.7-rc3 v6.7-rc2 v6.7-rc1 linus/master v6.7-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommufd-selftest-Use-a-fwnode-to-distinguish-devices/20231128-185058
base: next-20231128
patch link: https://lore.kernel.org/r/44ee6854da69e86b208f49752f60a4c18205c32a.1701165201.git.robin.murphy%40arm.com
patch subject: [PATCH 2/2] iommufd/selftest: Use normal IOMMU registration
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> drivers/iommu/iommufd/selftest.c:1323:12: warning: no previous prototype for function 'iommufd_bus_init' [-Wmissing-prototypes]
int __init iommufd_bus_init(void)
^
drivers/iommu/iommufd/selftest.c:1323:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int __init iommufd_bus_init(void)
^
static
drivers/iommu/iommufd/selftest.c:515:1: warning: unused function 'get_md_pagetable_nested' [-Wunused-function]
get_md_pagetable_nested(struct iommufd_ucmd *ucmd, u32 mockpt_id,
^
2 warnings generated.
vim +/iommufd_bus_init +1323 drivers/iommu/iommufd/selftest.c
1322
> 1323 int __init iommufd_bus_init(void)
1324 {
1325 return bus_register(&iommufd_mock_bus_type);
1326 }
1327 subsys_initcall(iommufd_bus_init);
1328
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Robin,
kernel test robot noticed the following build errors:
[auto build test ERROR on next-20231128]
[cannot apply to joro-iommu/next v6.7-rc3 v6.7-rc2 v6.7-rc1 linus/master v6.7-rc4]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/iommufd-selftest-Use-a-fwnode-to-distinguish-devices/20231128-185058
base: next-20231128
patch link: https://lore.kernel.org/r/44ee6854da69e86b208f49752f60a4c18205c32a.1701165201.git.robin.murphy%40arm.com
patch subject: [PATCH 2/2] iommufd/selftest: Use normal IOMMU registration
config: microblaze-allmodconfig (https://download.01.org/0day-ci/archive/20231206/[email protected]/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231206/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
microblaze-linux-ld: drivers/iommu/iommufd/selftest.o: in function `iommufd_bus_init':
>> (.init.text+0x0): multiple definition of `init_module'; drivers/iommu/iommufd/main.o:main.o:(.init.text+0x0): first defined here
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki