Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops

[Robin Murphy <robin.murphy@arm.com>]

On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
> When user requests to change IOMMU domain to a new type, IOMMU generic
> layer checks the requested type against the default domain type returned
> by vendor-specific IOMMU driver.
> 
> However, there is only one default domain type, and current mechanism
> does not allow if the requested type does not match the default type.

I don't really follow the reasoning here. If a driver's def_domain_type 
callback returns a specific type, it's saying that the device *has* to 
have that specific domain type for driver/platform-specific reasons. If 
that's not the case, then the driver shouldn't say so in the first place.

> Introducing check_domain_type_supported() callback in iommu_ops,
> which allows IOMMU generic layer to check with vendor-specific IOMMU driver
> whether the requested type is supported. This allows user to request
> types other than the default type.

Note also that you're only adding this in the sysfs path - what about 
the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?

AFAICS there shouldn't need to be any core-level changes to support 
this. We already have drivers which don't support passthrough at all, so 
conditionally not supporting it should be no big deal. What should 
happen currently is that def_domain_type returns 0 for "don't care", 
then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so 
iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.

Thanks,
Robin.

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>   drivers/iommu/iommu.c | 13 ++++++++++++-
>   include/linux/iommu.h |  2 ++
>   2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f2c45b85b9fc..4afb956ce083 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1521,6 +1521,16 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   
> +static bool iommu_domain_type_supported(struct device *dev, int type)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +
> +	if (ops->domain_type_supported)
> +		return ops->domain_type_supported(dev, type);
> +
> +	return true;
> +}
> +
>   static int iommu_get_def_domain_type(struct device *dev)
>   {
>   	const struct iommu_ops *ops = dev_iommu_ops(dev);
> @@ -2937,7 +2947,8 @@ static int iommu_change_dev_def_domain(struct iommu_group *group,
>   		 * domain the device was booted with
>   		 */
>   		type = dev_def_dom ? : iommu_def_domain_type;
> -	} else if (dev_def_dom && type != dev_def_dom) {
> +	} else if (!iommu_domain_type_supported(dev, type) ||
> +		   (dev_def_dom && type != dev_def_dom)) {
>   		dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
>   				    iommu_domain_type_str(type));
>   		ret = -EINVAL;
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index fecb72e1b11b..40c47ab15005 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -214,6 +214,7 @@ struct iommu_iotlb_gather {
>    *		- IOMMU_DOMAIN_IDENTITY: must use an identity domain
>    *		- IOMMU_DOMAIN_DMA: must use a dma domain
>    *		- 0: use the default setting
> + * @domain_type_supported: check if the specified domain type is supported
>    * @default_domain_ops: the default ops for domains
>    * @pgsize_bitmap: bitmap of all possible supported page sizes
>    * @owner: Driver module providing these ops
> @@ -252,6 +253,7 @@ struct iommu_ops {
>   			     struct iommu_page_response *msg);
>   
>   	int (*def_domain_type)(struct device *dev);
> +	bool (*domain_type_supported)(struct device *dev, int type);
>   
>   	const struct iommu_domain_ops *default_domain_ops;
>   	unsigned long pgsize_bitmap;
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu