Re: [PATCH v3 02/12] iommu: Introduce iommu_paging_domain_alloc_flags()

[Yi Liu <yi.l.liu@intel.com>]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> Currently drivers calls iommu_paging_domain_alloc(dev) to get an
> UNMANAGED domain. This is not sufficient to support PASID with
> UNMANAGED domain as some HW like AMD requires certain page table type
> to support PASIDs.
> 
> Also domain_alloc_paging() passes device as param for domain

The below may look clearer to reader. :)
"Also the domain_alloc_paging op only passes device as param for domain"

> allocation. This is not sufficient for AMD driver to decide the right
> page table.
> > Hence add iommu_paging_domain_alloc_flags() API which takes flags as
> parameter. Driver can pass additional parameter to indicate type of
> domain required, etc. iommu_paging_domain_alloc_flags() internally calls
> appropriate callback function to allocate a domain.

how about mentioning that the domain_alloc_user op accepts flags from
user which meets the AMD driver's requirement? Hence, it's clear why
we are not extending the domain_alloc_paging op to accept flags. :)

> 
> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
> [Added description - Vasant]
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++----
>   include/linux/iommu.h | 14 +++++++++++---
>   2 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 811748bec4b0..e13d64ffd14f 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -2027,20 +2027,42 @@ struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
>   /**
> - * iommu_paging_domain_alloc() - Allocate a paging domain
> + * iommu_paging_domain_alloc_flags() - Allocate a paging domain
>    * @dev: device for which the domain is allocated
> + * @flags: Bitmap of iommufd_hwpt_alloc_flags

maybe "enum iommufd_hwpt_alloc_flags" would be more accurate.

>    *
>    * Allocate a paging domain which will be managed by a kernel driver. Return
>    * allocated domain if successful, or a ERR pointer for failure.

nit: s/a/an/

with above minor comments, this patch LGTM.

Reviewed-by: Yi Liu <yi.l.liu@intel.com>

>    */
> -struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
> +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> +						     unsigned int flags)
>   {
> +	const struct iommu_ops *ops;
> +	struct iommu_domain *domain;
> +
>   	if (!dev_has_iommu(dev))
>   		return ERR_PTR(-ENODEV);
>   
> -	return __iommu_domain_alloc(dev_iommu_ops(dev), dev, IOMMU_DOMAIN_UNMANAGED);
> +	ops = dev_iommu_ops(dev);
> +
> +	if (ops->domain_alloc_paging && !flags)
> +		domain = ops->domain_alloc_paging(dev);
> +	else if (ops->domain_alloc_user)
> +		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> +	else if (ops->domain_alloc && !flags)
> +		domain = ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
> +	else
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	if (IS_ERR(domain))
> +		return domain;
> +	if (!domain)
> +		return ERR_PTR(-ENOMEM);
> +
> +	iommu_domain_init(domain, IOMMU_DOMAIN_UNMANAGED, ops);
> +	return domain;
>   }
> -EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc);
> +EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
>   
>   void iommu_domain_free(struct iommu_domain *domain)
>   {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index bd722f473635..42243183e81d 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -511,8 +511,6 @@ static inline int __iommu_copy_struct_from_user_array(
>    *                the caller iommu_domain_alloc() returns.
>    * @domain_alloc_user: Allocate an iommu domain corresponding to the input
>    *                     parameters as defined in include/uapi/linux/iommufd.h.
> - *                     Unlike @domain_alloc, it is called only by IOMMUFD and
> - *                     must fully initialize the new domain before return.
>    *                     Upon success, if the @user_data is valid and the @parent
>    *                     points to a kernel-managed domain, the new domain must be
>    *                     IOMMU_DOMAIN_NESTED type; otherwise, the @parent must be
> @@ -789,7 +787,11 @@ extern bool iommu_present(const struct bus_type *bus);
>   extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
>   extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
>   extern struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus);
> -struct iommu_domain *iommu_paging_domain_alloc(struct device *dev);
> +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev, unsigned int flags);
> +static inline struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
> +{
> +	return iommu_paging_domain_alloc_flags(dev, 0);
> +}
>   extern void iommu_domain_free(struct iommu_domain *domain);
>   extern int iommu_attach_device(struct iommu_domain *domain,
>   			       struct device *dev);
> @@ -1096,6 +1098,12 @@ static inline struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus
>   	return NULL;
>   }
>   
> +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> +						     unsigned int flags)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
>   static inline struct iommu_domain *iommu_paging_domain_alloc(struct device *dev)
>   {
>   	return ERR_PTR(-ENODEV);

-- 
Regards,
Yi Liu