[v2,5/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>
---
v2: - Explain the mitigation better in the warning message
- Fix theoretical bug if alloc_dev is never assigned because the
bus has no devices
- Use new dev_iommu_ops_valid() since in theory VFIO noiommu makes
device_iommu_mapped() -> dev_iommu_ops() an unsafe assumption
[Baolu]
drivers/iommu/iommu.c | 59 ++++++++++++++++++++++++++++---------------
1 file changed, 39 insertions(+), 20 deletions(-)
Comments
Hi Robin,
On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
wrote:
>
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> + struct device **alloc_dev = data;
> +
> + if (!dev_iommu_ops_valid(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. You may still need to disable one or more to
> get the expected result here, 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;
> +
> + /* We always check the whole bus, so the return value isn't
> useful */
> + bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
> + if (!dev)
> + return NULL;
Since __iommu_domain_alloc_dev() will always return 0, bus_for_each_dev()
will never breakout until the whole dev list is iterated over. If so, would
dev only record the last one? i.e. prior results get overwritten. Maybe a
misunderstood the logic.
> + return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
> }
> EXPORT_SYMBOL_GPL(iommu_domain_alloc);
Thanks,
Jacob
On 2023-01-26 23:22, Jacob Pan wrote:
> Hi Robin,
>
> On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
> wrote:
>
>>
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> + struct device **alloc_dev = data;
>> +
>> + if (!dev_iommu_ops_valid(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. You may still need to disable one or more to
>> get the expected result here, 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;
>> +
>> + /* We always check the whole bus, so the return value isn't
>> useful */
>> + bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
>> + if (!dev)
>> + return NULL;
> Since __iommu_domain_alloc_dev() will always return 0, bus_for_each_dev()
> will never breakout until the whole dev list is iterated over. If so, would
> dev only record the last one? i.e. prior results get overwritten. Maybe a
> misunderstood the logic.
Yes, as the comment points out, the intent is to walk the whole bus to
check it for consistency. Beyond that, we just need *a* device with
IOMMU ops; it doesn't matter at all which one it is. It happens to be
the last one off the list because that's what fell out of writing the
fewest lines of code.
(You could argue that there's no need to repeat the full walk if the
WARN_ONCE has already fired, but I'd rather keep the behaviour simple
and consistent - this is only meant to be a short-term solution, and
it's not a performance-critical path)
Thanks,
Robin.
>
>> + return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
>> }
>> EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>
> Thanks,
>
> Jacob
Hi Robin,
On Fri, 27 Jan 2023 11:42:27 +0000, Robin Murphy <robin.murphy@arm.com>
wrote:
> On 2023-01-26 23:22, Jacob Pan wrote:
> > Hi Robin,
> >
> > On Thu, 26 Jan 2023 18:26:20 +0000, Robin Murphy <robin.murphy@arm.com>
> > wrote:
> >
> >>
> >> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> >> +{
> >> + struct device **alloc_dev = data;
> >> +
> >> + if (!dev_iommu_ops_valid(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. You may still need to disable one
> >> or more to get the expected result here, 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;
> >> +
> >> + /* We always check the whole bus, so the return value isn't
> >> useful */
> >> + bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
> >> + if (!dev)
> >> + return NULL;
> > Since __iommu_domain_alloc_dev() will always return 0,
> > bus_for_each_dev() will never breakout until the whole dev list is
> > iterated over. If so, would dev only record the last one? i.e. prior
> > results get overwritten. Maybe a misunderstood the logic.
>
> Yes, as the comment points out, the intent is to walk the whole bus to
> check it for consistency. Beyond that, we just need *a* device with
> IOMMU ops; it doesn't matter at all which one it is. It happens to be
> the last one off the list because that's what fell out of writing the
> fewest lines of code.
>
> (You could argue that there's no need to repeat the full walk if the
> WARN_ONCE has already fired, but I'd rather keep the behaviour simple
> and consistent - this is only meant to be a short-term solution, and
> it's not a performance-critical path)
That make sense now, thank you for the explanation.
Jacob
On 2023/1/27 2:26, Robin Murphy wrote:
> 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>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Best regards,
baolu
On Thu, Jan 26, 2023 at 06:26:20PM +0000, Robin Murphy wrote:
> @@ -1980,9 +1978,30 @@ 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 (!dev_iommu_ops_valid(dev))
> + return 0;
This valid check is necessary to be done in __iommu_domain_alloc()
because the callback coming from iommu_device_claim_dma_owner() never
validated that the device/group has an iommu driver:
> @@ -3120,13 +3139,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);
Jason
@@ -89,7 +89,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);
@@ -1641,15 +1641,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);
@@ -1674,7 +1674,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);
}
/**
@@ -1787,8 +1787,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;
@@ -1798,10 +1797,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);
}
@@ -1864,7 +1865,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);
@@ -1953,15 +1954,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;
@@ -1980,9 +1978,30 @@ 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 (!dev_iommu_ops_valid(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. You may still need to disable one or more to get the expected result here, 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;
+
+ /* We always check the whole bus, so the return value isn't useful */
+ bus_for_each_dev(bus, NULL, &dev, __iommu_domain_alloc_dev);
+ if (!dev)
+ return NULL;
+
+ return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
}
EXPORT_SYMBOL_GPL(iommu_domain_alloc);
@@ -2906,7 +2925,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;
@@ -3120,13 +3139,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;
}