diff mbox series

[v3,4/7] iommu: Switch __iommu_domain_alloc() to device ops

Message ID 404d8395cf4252c6fe6d98f317f3570127451778.1694693889.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series Iommu: Retire bus ops | expand

Commit Message

Robin Murphy Sept. 15, 2023, 4:58 p.m. UTC 
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.

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>

---

v3: Make sure blocking domains are covered as well
---
 drivers/iommu/iommu.c | 50 +++++++++++++++++++++++++++++--------------
 1 file changed, 34 insertions(+), 16 deletions(-)

Comments

Baolu Lu Sept. 18, 2023, 6:10 a.m. UTC | #1 
On 9/16/23 12:58 AM, Robin Murphy wrote:
> @@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
>   }
>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>   
> -static struct iommu_domain *__iommu_domain_alloc(const 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;
>   	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>   
> -	if (!ops)
> -		return NULL;
> -
>   	domain = ops->domain_alloc(alloc_type);
>   	if (!domain)
>   		return NULL;
> @@ -2030,9 +2025,28 @@ static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
>   	return domain;
>   }
>   
> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
> +{
> +	struct device **alloc_dev = data;
> +
> +	if (!dev_has_iommu(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(const 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;

__iommu_domain_alloc_dev() always returns 0. Hence above if condition
will never be true. Perhaps, in __iommu_domain_alloc_dev(),

	if (WARN_ON(*alloc_dev && dev_iommu_ops(dev) !=
             dev_iommu_ops(*alloc_dev))
		return -EPERM;

?

> +
> +	return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);

Is it possible that all devices on this bus have dev_has_iommu() to be
false? If so, we probably need something like below:

	if (!dev_has_iommu(dev))
		return -ENODEV;

?

>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
> @@ -3228,13 +3242,17 @@ 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);
> +	/* noiommu groups should never be here */
> +	if (WARN_ON(!dev_has_iommu(dev)))
> +		return -ENODEV;
> +
> +	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;
>   	}

Best regards,
baolu
Robin Murphy Sept. 18, 2023, 10:36 a.m. UTC | #2 
On 2023-09-18 07:10, Baolu Lu wrote:
> On 9/16/23 12:58 AM, Robin Murphy wrote:
>> @@ -1997,16 +1995,13 @@ void iommu_set_fault_handler(struct 
>> iommu_domain *domain,
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>> -static struct iommu_domain *__iommu_domain_alloc(const 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;
>>       unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
>> -    if (!ops)
>> -        return NULL;
>> -
>>       domain = ops->domain_alloc(alloc_type);
>>       if (!domain)
>>           return NULL;
>> @@ -2030,9 +2025,28 @@ static struct iommu_domain 
>> *__iommu_domain_alloc(const struct bus_type *bus,
>>       return domain;
>>   }
>> +static int __iommu_domain_alloc_dev(struct device *dev, void *data)
>> +{
>> +    struct device **alloc_dev = data;
>> +
>> +    if (!dev_has_iommu(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(const 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;
> 
> __iommu_domain_alloc_dev() always returns 0. Hence above if condition
> will never be true. Perhaps, in __iommu_domain_alloc_dev(),

Oh bugger, seems I screwed up the unnecessarily overcomplicated rebase 
that I made for myself, and managed to put this back to the v1 code, so 
it's just wrong (bus_for_each_dev() can return an error itself if the 
bus isn't properly initialised, but it also returns success if the bus 
has no devices, which was handled properly in v2 that you actually R-b'd).

> 
>      if (WARN_ON(*alloc_dev && dev_iommu_ops(dev) !=
>              dev_iommu_ops(*alloc_dev))
>          return -EPERM;

I went back and forth on this initially, but in the end I figured since 
the other patches are meant to be making the rest of the public API 
sufficiently robust, then if someone does try it with multiple drivers 
before full support can be finished, they can at least have some chance 
of getting the desired result, rather than a guarantee of not. I am 
still open to being convinced otherwise, though.

Thanks,
Robin.

> 
> ?
> 
>> +
>> +    return __iommu_domain_alloc(dev, IOMMU_DOMAIN_UNMANAGED);
> 
> Is it possible that all devices on this bus have dev_has_iommu() to be
> false? If so, we probably need something like below:
> 
>      if (!dev_has_iommu(dev))
>          return -ENODEV;
> 
> ?
> 
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>> @@ -3228,13 +3242,17 @@ 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);
>> +    /* noiommu groups should never be here */
>> +    if (WARN_ON(!dev_has_iommu(dev)))
>> +        return -ENODEV;
>> +
>> +    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;
>>       }
> 
> Best regards,
> baolu
diff mbox series

Patch

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f4cc91227b22..29ebb4b57df4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -96,7 +96,7 @@  static const char * const iommu_group_resv_type_string[] = {
 static int iommu_bus_notifier(struct notifier_block *nb,
 			      unsigned long action, void *data);
 static void iommu_release_device(struct device *dev);
-static struct iommu_domain *__iommu_domain_alloc(const 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);
@@ -1745,12 +1745,11 @@  static int iommu_get_def_domain_type(struct device *dev)
 }
 
 static struct iommu_domain *
-__iommu_group_alloc_default_domain(const struct bus_type *bus,
-				   struct iommu_group *group, int req_type)
+__iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
 	if (group->default_domain && group->default_domain->type == req_type)
 		return group->default_domain;
-	return __iommu_domain_alloc(bus, req_type);
+	return __iommu_domain_alloc(iommu_group_first_dev(group), req_type);
 }
 
 /*
@@ -1760,23 +1759,22 @@  __iommu_group_alloc_default_domain(const struct bus_type *bus,
 static struct iommu_domain *
 iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
-	const struct bus_type *bus = iommu_group_first_dev(group)->bus;
 	struct iommu_domain *dom;
 
 	lockdep_assert_held(&group->mutex);
 
 	if (req_type)
-		return __iommu_group_alloc_default_domain(bus, group, req_type);
+		return __iommu_group_alloc_default_domain(group, req_type);
 
 	/* The driver gave no guidance on what type to use, try the default */
-	dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
+	dom = __iommu_group_alloc_default_domain(group, iommu_def_domain_type);
 	if (dom)
 		return dom;
 
 	/* Otherwise IDENTITY and DMA_FQ defaults will try DMA */
 	if (iommu_def_domain_type == IOMMU_DOMAIN_DMA)
 		return NULL;
-	dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA);
+	dom = __iommu_group_alloc_default_domain(group, IOMMU_DOMAIN_DMA);
 	if (!dom)
 		return NULL;
 
@@ -1997,16 +1995,13 @@  void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
-static struct iommu_domain *__iommu_domain_alloc(const 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;
 	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
 
-	if (!ops)
-		return NULL;
-
 	domain = ops->domain_alloc(alloc_type);
 	if (!domain)
 		return NULL;
@@ -2030,9 +2025,28 @@  static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
 	return domain;
 }
 
+static int __iommu_domain_alloc_dev(struct device *dev, void *data)
+{
+	struct device **alloc_dev = data;
+
+	if (!dev_has_iommu(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(const 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);
 
@@ -3228,13 +3242,17 @@  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);
+	/* noiommu groups should never be here */
+	if (WARN_ON(!dev_has_iommu(dev)))
+		return -ENODEV;
+
+	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;
 	}