[4/8] iommu: Switch __iommu_domain_alloc() to device ops
Commit Message
In all the places we allocate default domains, we have (or can easily
get hold of) a device from which to resolve the right IOMMU ops; only
the public iommu_domain_alloc() interface actually depends on bus ops.
Reworking the public API is a big enough mission in its own right, but
in the meantime we can still decouple it from bus ops internally to move
forward.
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
drivers/iommu/iommu.c | 57 ++++++++++++++++++++++++++++---------------
1 file changed, 37 insertions(+), 20 deletions(-)
Comments
On Thu, Jan 19, 2023 at 07:18:22PM +0000, Robin Murphy wrote:
> -static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> +static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
> unsigned type)
> {
> - const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
> + const struct iommu_ops *ops = dev_iommu_ops(dev);
> struct iommu_domain *domain;
>
> - if (!ops)
> - return NULL;
> -
> domain = ops->domain_alloc(type);
> if (!domain)
> return NULL;
> @@ -1970,9 +1968,28 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> return domain;
> }
>
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> + struct device **alloc_dev = data;
> +
> + if (!device_iommu_mapped(dev))
> + return 0;
Is 0 the right thing? see below
> +
> + WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != dev_iommu_ops(*alloc_dev),
> + "Multiple IOMMU drivers present, which the public IOMMU API can't fully support yet. This may not work as expected, sorry!\n");
if (WARN_ONCE(..))
return -EINVAL
So that iommu_domain_alloc fails?
> + *alloc_dev = dev;
> + return 0;
> +}
> +
> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
> {
> - return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> + struct device *dev = NULL;
> +
> + if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
> + return NULL;
eg shouldn't iommu_domain_alloc() return NULL if any devices are
!device_iommu_mapped ?
Jason
On 19/01/2023 7:26 pm, Jason Gunthorpe wrote:
> On Thu, Jan 19, 2023 at 07:18:22PM +0000, Robin Murphy wrote:
>
>> -static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>> +static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
>> unsigned type)
>> {
>> - const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
>> + const struct iommu_ops *ops = dev_iommu_ops(dev);
>> struct iommu_domain *domain;
>>
>> - if (!ops)
>> - return NULL;
>> -
>> domain = ops->domain_alloc(type);
>> if (!domain)
>> return NULL;
>> @@ -1970,9 +1968,28 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>> return domain;
>> }
>>
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> + struct device **alloc_dev = data;
>> +
>> + if (!device_iommu_mapped(dev))
>> + return 0;
>
> Is 0 the right thing? see below
Yes, the idea here is to always double-check the whole bus.
>> +
>> + WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != dev_iommu_ops(*alloc_dev),
>> + "Multiple IOMMU drivers present, which the public IOMMU API can't fully support yet. This may not work as expected, sorry!\n");
>
> if (WARN_ONCE(..))
> return -EINVAL
>
> So that iommu_domain_alloc fails?
The current behaviour is that if you have multiple different IOMMUs
present, then only one driver succeeds in registering, effectively at
random depending on probe order. To get predictable behaviour where
iommu_domain_alloc() (and indeed the whole IOMMU API) works for a
specific device, you have to manage your kernel config or modules to
only load the driver for the correct IOMMU.
After patch #4, we allow all the drivers to register, but the net effect
on the public API is still the same - it only works successfully for one
driver, effectively at random - and the same solution - don't load the
other drivers, or at least load them in an appropriate order relative to
the client drivers - still applies. On those grounds it seems a fair
compromise until we can sort iommu_domain_alloc() itself. As far as I'm
aware there are still no immediate real-world users for this - upstream
support for Rockchip RK3588 is still in very early days, and a long way
off being complete enough for users to get interested in trying to play
with the Arm SMMUs in there (leading to disappointment that VFIO won't
work since they're non-coherent...)
>> + *alloc_dev = dev;
>> + return 0;
>> +}
>> +
>> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
>> {
>> - return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
>> + struct device *dev = NULL;
>> +
>> + if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
>> + return NULL;
>
> eg shouldn't iommu_domain_alloc() return NULL if any devices are
> !device_iommu_mapped ?
No, that would even break the normal single-driver case, because it's
always been the case that not all devices on e.g. the platform bus are
iommu_mapped. That's largely why bus ops are a rubbish abstraction.
Even with multiple drivers, we can still allocate a domain here which
will work fine with *some* devices, and safely fail to work with others,
so I don't see that we'd gain much from being unnecessarily restrictive.
Thanks,
Robin.
@@ -88,7 +88,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
unsigned long action, void *data);
static int iommu_alloc_default_domain(struct iommu_group *group,
struct device *dev);
-static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
unsigned type);
static int __iommu_attach_device(struct iommu_domain *domain,
struct device *dev);
@@ -1620,15 +1620,15 @@ static int iommu_get_def_domain_type(struct device *dev)
return 0;
}
-static int iommu_group_alloc_default_domain(struct bus_type *bus,
- struct iommu_group *group,
+static int iommu_group_alloc_default_domain(struct iommu_group *group,
+ struct device *dev,
unsigned int type)
{
struct iommu_domain *dom;
- dom = __iommu_domain_alloc(bus, type);
+ dom = __iommu_domain_alloc(dev, type);
if (!dom && type != IOMMU_DOMAIN_DMA) {
- dom = __iommu_domain_alloc(bus, IOMMU_DOMAIN_DMA);
+ dom = __iommu_domain_alloc(dev, IOMMU_DOMAIN_DMA);
if (dom)
pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
type, group->name);
@@ -1653,7 +1653,7 @@ static int iommu_alloc_default_domain(struct iommu_group *group,
type = iommu_get_def_domain_type(dev) ? : iommu_def_domain_type;
- return iommu_group_alloc_default_domain(dev->bus, group, type);
+ return iommu_group_alloc_default_domain(group, dev, type);
}
/**
@@ -1766,8 +1766,7 @@ static int probe_get_default_domain_type(struct device *dev, void *data)
return 0;
}
-static void probe_alloc_default_domain(struct bus_type *bus,
- struct iommu_group *group)
+static void probe_alloc_default_domain(struct iommu_group *group)
{
struct __group_domain_type gtype;
@@ -1777,10 +1776,12 @@ static void probe_alloc_default_domain(struct bus_type *bus,
__iommu_group_for_each_dev(group, >ype,
probe_get_default_domain_type);
- if (!gtype.type)
+ if (!gtype.type) {
gtype.type = iommu_def_domain_type;
+ gtype.dev = iommu_group_first_dev(group);
+ }
- iommu_group_alloc_default_domain(bus, group, gtype.type);
+ iommu_group_alloc_default_domain(group, gtype.dev, gtype.type);
}
@@ -1854,7 +1855,7 @@ int bus_iommu_probe(struct bus_type *bus)
list_del_init(&group->entry);
/* Try to allocate default domain */
- probe_alloc_default_domain(bus, group);
+ probe_alloc_default_domain(group);
if (!group->default_domain) {
mutex_unlock(&group->mutex);
@@ -1943,15 +1944,12 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
}
EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
-static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
+static struct iommu_domain *__iommu_domain_alloc(struct device *dev,
unsigned type)
{
- const struct iommu_ops *ops = bus ? bus->iommu_ops : NULL;
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
struct iommu_domain *domain;
- if (!ops)
- return NULL;
-
domain = ops->domain_alloc(type);
if (!domain)
return NULL;
@@ -1970,9 +1968,28 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
return domain;
}
+static int __iommu_domain_alloc_dev(struct device *dev, void *data)
+{
+ struct device **alloc_dev = data;
+
+ if (!device_iommu_mapped(dev))
+ return 0;
+
+ WARN_ONCE(*alloc_dev && dev_iommu_ops(dev) != dev_iommu_ops(*alloc_dev),
+ "Multiple IOMMU drivers present, which the public IOMMU API can't fully support yet. This may not work as expected, sorry!\n");
+
+ *alloc_dev = dev;
+ return 0;
+}
+
struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
{
- return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
+ struct device *dev = NULL;
+
+ if (bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev))
+ return NULL;
+
+ return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
}
EXPORT_SYMBOL_GPL(iommu_domain_alloc);
@@ -2918,7 +2935,7 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
}
/* Sets group->default_domain to the newly allocated domain */
- ret = iommu_group_alloc_default_domain(dev->bus, group, type);
+ ret = iommu_group_alloc_default_domain(group, dev, type);
if (ret)
goto out;
@@ -3132,13 +3149,13 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
if (group->blocking_domain)
return 0;
- group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_BLOCKED);
+ group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_BLOCKED);
if (!group->blocking_domain) {
/*
* For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
* create an empty domain instead.
*/
- group->blocking_domain = __iommu_domain_alloc(dev->bus, IOMMU_DOMAIN_UNMANAGED);
+ group->blocking_domain = __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
if (!group->blocking_domain)
return -EINVAL;
}