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

  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(-)
  

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
  

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