[PATCH v3 00/12] iommu: Domain allocation enhancements

[PATCH v3 00/12] iommu: Domain allocation enhancements

[Vasant Hegde]

This series adds iommu_paging_domain_alloc_flags() which takes flags to
pass additional details for domain allocation (like domain with PASID
support). Also adjusts core code to handler variuos domain allocations.

Also updates AMD IOMMU driver domain allocation code. With this by
default it will allocate domain with V2 page table for PASID capable
device and v1 page table for rest of the devices.


TODO:
@Jason, @Kevin, @Baolu,
  With this series if driver implements domain_alloc_user() then we assume
  IOMMU always supports PASID. If IOMMU doesn't support PASID (ex: AMD driver
  not supporting V2 page table) and device is PASID capable, then driver will
  return error. Device probe will fail.

  I think if driver return error, core layer should retry without
  IOMMU_HWPT_ALLOC_PASID flag. Other option is indivisual driver in error path
  of domain_alloc_user allocating non-PASID capable domain.

  I prefer first option. Just wanted to discuss before implementing. Any preferred
  option here? OR any other better way to solve this?


Note :
  vt-d specific changes is handled in separate series [1].

[1] https://lore.kernel.org/linux-iommu/20241011042722.73930-8-baolu.lu@linux.intel.com/

@Baolu,
  Do you want me to include above patch in this series so that it doesn't break intel driver?

Thanks everyone who reviewed previous version and provided valuable feedbacks.


This series is based on top of v6.12-rc3.

This is also available at github :
  https://github.com/AMDESE/linux-iommu/tree/iommu_paging_domain_v3


Changes from V2 -> v3:
  - Added Review-by tags
  - Fixed warnings reported by 0-DAY CI
  - Added patch to enhance arm_smmu_domain_alloc_user() to support IOMMU_HWPT_ALLOC_PASID flag
  - Added new patch to check GIOSUP/GTSUP
  - Added patch to move V2 page table check to early_amd_iommu_init()
  - Added patch to adjust blocked/identity domain allocation in core layer
  - Dropped "iommu/amd: Add iommu_ops->domain_alloc_paging support"

V2 : https://lore.kernel.org/linux-iommu/20240911101911.6269-1-vasant.hegde@amd.com/T/#t

Changes from v1 -> v2:
  - Rebased on top of iommu/next
  - Split patches into smaller patches
  - Added global static identity domain support

V1 : https://lore.kernel.org/linux-iommu/20240821133554.7405-1-vasant.hegde@amd.com/

RFC v2: https://lore.kernel.org/linux-iommu/fc8715e4-fb44-433b-a42a-7804118ebc57@amd.com/T/#t

RFC v1 : https://lore.kernel.org/linux-iommu/7e249bc6-c578-40f0-aca7-835149a0ad39@amd.com/


Jason Gunthorpe (5):
  iommu: Refactor __iommu_domain_alloc()
  iommu: Introduce iommu_paging_domain_alloc_flags()
  iommu: Add new flag to explictly request PASID capable domain
  iommu: Put domain allocation in __iommu_group_alloc_blocking_domain()
  iommu: Create __iommu_alloc_identity_domain()

Vasant Hegde (7):
  iommu/arm-smmu-v3: Enhance domain_alloc_user() to allocate PASID
    capable domain
  iommu/amd: Add helper function to check GIOSUP/GTSUP
  iommu/amd: Move V2 page table support check to early_amd_iommu_init()
  iommu/amd: Separate page table setup from domain allocation
  iommu/amd: Pass page table type as param to pdom_setup_pgtable()
  iommu/amd: Enhance amd_iommu_domain_alloc_user()
  iommu/amd: Implement global identity domain

 drivers/iommu/amd/amd_iommu.h               |   8 +-
 drivers/iommu/amd/init.c                    |  18 ++-
 drivers/iommu/amd/iommu.c                   | 125 +++++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |   3 +
 drivers/iommu/iommu.c                       | 171 +++++++++++++-------
 include/linux/iommu.h                       |  14 +-
 include/uapi/linux/iommufd.h                |   6 +
 7 files changed, 240 insertions(+), 105 deletions(-)

-- 
2.31.1

[PATCH v3 01/12] iommu: Refactor __iommu_domain_alloc()

[Vasant Hegde]

From: Jason Gunthorpe <jgg@ziepe.ca>

Following patch will introduce iommu_paging_domain_alloc_flags() API.
Hence move domain init code to separate function so that it can be
resued.

Also move iommu_get_dma_cookie() setup iommu_setup_default_domain() as
its required in DMA API mode only.

Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
[Split the patch and added description - Vasant]
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
 drivers/iommu/iommu.c | 46 +++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 83c8e617a2c5..811748bec4b0 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1934,6 +1934,22 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
 }
 EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
 
+static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
+			      const struct iommu_ops *ops)
+{
+	domain->type = type;
+	domain->owner = ops;
+	if (!domain->ops)
+		domain->ops = ops->default_domain_ops;
+
+	/*
+	 * If not already set, assume all sizes by default; the driver
+	 * may override this later
+	 */
+	if (!domain->pgsize_bitmap)
+		domain->pgsize_bitmap = ops->pgsize_bitmap;
+}
+
 static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 						 struct device *dev,
 						 unsigned int type)
@@ -1962,27 +1978,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	domain->type = type;
-	domain->owner = ops;
-	/*
-	 * If not already set, assume all sizes by default; the driver
-	 * may override this later
-	 */
-	if (!domain->pgsize_bitmap)
-		domain->pgsize_bitmap = ops->pgsize_bitmap;
-
-	if (!domain->ops)
-		domain->ops = ops->default_domain_ops;
-
-	if (iommu_is_dma_domain(domain)) {
-		int rc;
-
-		rc = iommu_get_dma_cookie(domain);
-		if (rc) {
-			iommu_domain_free(domain);
-			return ERR_PTR(rc);
-		}
-	}
+	iommu_domain_init(domain, type, ops);
 	return domain;
 }
 
@@ -2965,6 +2961,14 @@ static int iommu_setup_default_domain(struct iommu_group *group,
 	if (group->default_domain == dom)
 		return 0;
 
+	if (iommu_is_dma_domain(dom)) {
+		ret = iommu_get_dma_cookie(dom);
+		if (ret) {
+			iommu_domain_free(dom);
+			return ret;
+		}
+	}
+
 	/*
 	 * IOMMU_RESV_DIRECT and IOMMU_RESV_DIRECT_RELAXABLE regions must be
 	 * mapped before their device is attached, in order to guarantee
-- 
2.31.1

Re: [PATCH v3 01/12] iommu: Refactor __iommu_domain_alloc()

[Yi Liu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> Following patch will introduce iommu_paging_domain_alloc_flags() API.
> Hence move domain init code to separate function so that it can be
> resued.

A nit: s/resued/reused/

> 
> Also move iommu_get_dma_cookie() setup iommu_setup_default_domain() as
> its required in DMA API mode only.

a nit: s/its/it's/

Otherwise, LGTM.

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

> 
> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
> [Split the patch and added description - Vasant]
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>   drivers/iommu/iommu.c | 46 +++++++++++++++++++++++--------------------
>   1 file changed, 25 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 83c8e617a2c5..811748bec4b0 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1934,6 +1934,22 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
>   }
>   EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>   
> +static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
> +			      const struct iommu_ops *ops)
> +{
> +	domain->type = type;
> +	domain->owner = ops;
> +	if (!domain->ops)
> +		domain->ops = ops->default_domain_ops;
> +
> +	/*
> +	 * If not already set, assume all sizes by default; the driver
> +	 * may override this later
> +	 */
> +	if (!domain->pgsize_bitmap)
> +		domain->pgsize_bitmap = ops->pgsize_bitmap;
> +}
> +
>   static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
>   						 struct device *dev,
>   						 unsigned int type)
> @@ -1962,27 +1978,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
>   	if (!domain)
>   		return ERR_PTR(-ENOMEM);
>   
> -	domain->type = type;
> -	domain->owner = ops;
> -	/*
> -	 * If not already set, assume all sizes by default; the driver
> -	 * may override this later
> -	 */
> -	if (!domain->pgsize_bitmap)
> -		domain->pgsize_bitmap = ops->pgsize_bitmap;
> -
> -	if (!domain->ops)
> -		domain->ops = ops->default_domain_ops;
> -
> -	if (iommu_is_dma_domain(domain)) {
> -		int rc;
> -
> -		rc = iommu_get_dma_cookie(domain);
> -		if (rc) {
> -			iommu_domain_free(domain);
> -			return ERR_PTR(rc);
> -		}
> -	}
> +	iommu_domain_init(domain, type, ops);
>   	return domain;
>   }
>   
> @@ -2965,6 +2961,14 @@ static int iommu_setup_default_domain(struct iommu_group *group,
>   	if (group->default_domain == dom)
>   		return 0;
>   
> +	if (iommu_is_dma_domain(dom)) {
> +		ret = iommu_get_dma_cookie(dom);
> +		if (ret) {
> +			iommu_domain_free(dom);
> +			return ret;
> +		}
> +	}
> +
>   	/*
>   	 * IOMMU_RESV_DIRECT and IOMMU_RESV_DIRECT_RELAXABLE regions must be
>   	 * mapped before their device is attached, in order to guarantee

-- 
Regards,
Yi Liu

Re: [PATCH v3 01/12] iommu: Refactor __iommu_domain_alloc()

[Vasant Hegde]

Hi Yi,

On 10/21/2024 12:59 PM, Yi Liu wrote:
> On 2024/10/17 22:01, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Following patch will introduce iommu_paging_domain_alloc_flags() API.
>> Hence move domain init code to separate function so that it can be
>> resued.
> 
> A nit: s/resued/reused/

Ack. Will fix it.
> 
>>
>> Also move iommu_get_dma_cookie() setup iommu_setup_default_domain() as
>> its required in DMA API mode only.
> 
> a nit: s/its/it's/
> 
> Otherwise, LGTM.
> 
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>

Thanks
-Vasant

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

[Vasant Hegde]

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
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.

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
  *
  * Allocate a paging domain which will be managed by a kernel driver. Return
  * allocated domain if successful, or a ERR pointer for failure.
  */
-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);
-- 
2.31.1

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

[Baolu Lu]

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

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

> 
> 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
>    *
>    * Allocate a paging domain which will be managed by a kernel driver. Return
>    * allocated domain if successful, or a ERR pointer for failure.
>    */
> -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);

Above works.

If we want to deprecate domain_alloc and make domain_alloc_user a
generic callback for paging domain allocation, perhaps we can make it
like below:

	if (flags && !ops->domain_alloc_user)
		return ERR_PTR(-EOPNOTSUPP);

	if (ops->domain_alloc_user)
		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
	else if (ops->domain_alloc_paging)
		domain = ops->domain_alloc_paging(dev);
	else if (ops->domain_alloc)
		domain = ops->domain_alloc(IOMMU_DOMAIN_UNMANAGED);
	else
		return ERR_PTR(-EOPNOTSUPP);

Thanks,
baolu

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

[Yi Liu]

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

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

[Vasant Hegde]

Yi,


On 10/21/2024 1:17 PM, Yi Liu wrote:
> 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"

Better. Fixed.

> 
>> 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. :)

I have added below sentence instead.

Instead of extending ops->domain_alloc_paging() it was decided to
enhance ops->domain_alloc_user() so that caller can pass various
additional 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.

Ack.

> 
>>    *
>>    * 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/

Fixed.

-Vasant

[PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

From: Jason Gunthorpe <jgg@ziepe.ca>

Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
If both IOMMU and device supports PASID it will allocate domain.
Otherwise return error.

Also modify __iommu_group_alloc_default_domain() to call
iommu_paging_domain_alloc_flags() with appropriate flag when allocating
paging domain.

Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
[Added __iommu_paging_domain_alloc_flags() and description - Vasant]
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
 drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------
 include/uapi/linux/iommufd.h |  6 +++++
 2 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e13d64ffd14f..107016b3ba3d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -32,6 +32,7 @@
 #include <trace/events/iommu.h>
 #include <linux/sched/mm.h>
 #include <linux/msi.h>
+#include <uapi/linux/iommufd.h>
 
 #include "dma-iommu.h"
 #include "iommu-priv.h"
@@ -99,6 +100,9 @@ static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
 				struct iommu_group *group);
+static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
+						       unsigned int type,
+						       unsigned int flags);
 
 enum {
 	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
@@ -1589,8 +1593,19 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 static struct iommu_domain *
 __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
+	struct device *dev = iommu_group_first_dev(group);
+
 	if (group->default_domain && group->default_domain->type == req_type)
 		return group->default_domain;
+
+	/*
+	 * When allocating the DMA API domain assume that the driver is going to
+	 * use PASID and make sure the RID's domain is PASID compatible.
+	 */
+	if (req_type & __IOMMU_DOMAIN_PAGING)
+		return __iommu_paging_domain_alloc_flags(dev, req_type,
+				dev->iommu->max_pasids ? IOMMU_HWPT_ALLOC_PASID : 0);
+
 	return __iommu_group_domain_alloc(group, req_type);
 }
 
@@ -2026,16 +2041,9 @@ struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
 }
 EXPORT_SYMBOL_GPL(iommu_domain_alloc);
 
-/**
- * iommu_paging_domain_alloc_flags() - Allocate a paging domain
- * @dev: device for which the domain is allocated
- * @flags: Bitmap of iommufd_hwpt_alloc_flags
- *
- * Allocate a paging domain which will be managed by a kernel driver. Return
- * allocated domain if successful, or a ERR pointer for failure.
- */
-struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
-						     unsigned int flags)
+static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
+						       unsigned int type,
+						       unsigned int flags)
 {
 	const struct iommu_ops *ops;
 	struct iommu_domain *domain;
@@ -2059,9 +2067,24 @@ struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	iommu_domain_init(domain, IOMMU_DOMAIN_UNMANAGED, ops);
+	iommu_domain_init(domain, type, ops);
 	return 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
+ *
+ * Allocate a paging domain which will be managed by a kernel driver. Return
+ * allocated domain if successful, or a ERR pointer for failure.
+ */
+struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
+						     unsigned int flags)
+{
+	return __iommu_paging_domain_alloc_flags(dev,
+					 IOMMU_DOMAIN_UNMANAGED, flags);
+}
 EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
 
 void iommu_domain_free(struct iommu_domain *domain)
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 72010f71c5e4..d1b07f4d9d32 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
  *                                   enforced on device attachment
  * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
  *                             valid.
+ * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
+ *                          domain can be attached to any PASID on the device.
+ *                          Any domain attached to the non-PASID part of the
+ *                          device must also be flaged, otherwise attaching a
+ *                          PASID will blocked.
  */
 enum iommufd_hwpt_alloc_flags {
 	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
 	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
 	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
+	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
 };
 
 /**
-- 
2.31.1

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Baolu Lu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe<jgg@ziepe.ca>
> 
> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
> If both IOMMU and device supports PASID it will allocate domain.
> Otherwise return error.
> 
> Also modify __iommu_group_alloc_default_domain() to call
> iommu_paging_domain_alloc_flags() with appropriate flag when allocating
> paging domain.
> 
> Signed-off-by: Jason Gunthorpe<jgg@ziepe.ca>
> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe<jgg@nvidia.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> ---
>   drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------
>   include/uapi/linux/iommufd.h |  6 +++++
>   2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e13d64ffd14f..107016b3ba3d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -32,6 +32,7 @@
>   #include <trace/events/iommu.h>
>   #include <linux/sched/mm.h>
>   #include <linux/msi.h>
> +#include <uapi/linux/iommufd.h>
>   
>   #include "dma-iommu.h"
>   #include "iommu-priv.h"
> @@ -99,6 +100,9 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
>   static int __iommu_attach_group(struct iommu_domain *domain,
>   				struct iommu_group *group);
> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
> +						       unsigned int type,
> +						       unsigned int flags);
>   
>   enum {
>   	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
> @@ -1589,8 +1593,19 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   static struct iommu_domain *
>   __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   {
> +	struct device *dev = iommu_group_first_dev(group);
> +
>   	if (group->default_domain && group->default_domain->type == req_type)
>   		return group->default_domain;
> +
> +	/*
> +	 * When allocating the DMA API domain assume that the driver is going to
> +	 * use PASID and make sure the RID's domain is PASID compatible.
> +	 */
> +	if (req_type & __IOMMU_DOMAIN_PAGING)
> +		return __iommu_paging_domain_alloc_flags(dev, req_type,
> +				dev->iommu->max_pasids ? IOMMU_HWPT_ALLOC_PASID : 0);

Perhaps we need below change in intel iommu driver, otherwise the
default domain allocation will return failure.

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef..d7546ab18e18 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3539,12 +3539,16 @@ intel_iommu_domain_alloc_user(struct device 
*dev, u32 flags,
         }

         if (flags &
-           (~(IOMMU_HWPT_ALLOC_NEST_PARENT | 
IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
+           (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
+              IOMMU_HWPT_ALLOC_DIRTY_TRACKING) |
+              IOMMU_HWPT_ALLOC_PASID))
                 return ERR_PTR(-EOPNOTSUPP);
         if (nested_parent && !nested_supported(iommu))
                 return ERR_PTR(-EOPNOTSUPP);
         if (user_data || (dirty_tracking && !ssads_supported(iommu)))
                 return ERR_PTR(-EOPNOTSUPP);
+       if ((flags & IOMMU_HWPT_ALLOC_PASID) && !pasid_supported(iommu))
+               return ERR_PTR(-EOPNOTSUPP);

         /* Do not use first stage for user domain translation. */
         dmar_domain = paging_domain_alloc(dev, false);

Thanks,
baolu

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

On Fri, Oct 18, 2024 at 11:43:02AM +0800, Baolu Lu wrote:
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9f6b0780f2ef..d7546ab18e18 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3539,12 +3539,16 @@ intel_iommu_domain_alloc_user(struct device *dev,
> u32 flags,
>         }
> 
>         if (flags &
> -           (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
> IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
> +           (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
> +              IOMMU_HWPT_ALLOC_DIRTY_TRACKING) |
> +              IOMMU_HWPT_ALLOC_PASID))
>                 return ERR_PTR(-EOPNOTSUPP);
>         if (nested_parent && !nested_supported(iommu))
>                 return ERR_PTR(-EOPNOTSUPP);
>         if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>                 return ERR_PTR(-EOPNOTSUPP);
> +       if ((flags & IOMMU_HWPT_ALLOC_PASID) && !pasid_supported(iommu))
> +               return ERR_PTR(-EOPNOTSUPP);

I think the decision here should only be about supported domain
type. Like on AMD if the iommu instance doesn't support v2 then it
should fail.

If there is some other thing that says the instance supports pasid,
that should be checked during attach time only.

IIRC intel can use either page table format for pasid, so it probably
doesn't need to do anything here?

Jason

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Baolu Lu]

On 2024/10/18 20:43, Jason Gunthorpe wrote:
> On Fri, Oct 18, 2024 at 11:43:02AM +0800, Baolu Lu wrote:
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 9f6b0780f2ef..d7546ab18e18 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3539,12 +3539,16 @@ intel_iommu_domain_alloc_user(struct device *dev,
>> u32 flags,
>>          }
>>
>>          if (flags &
>> -           (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
>> +           (~(IOMMU_HWPT_ALLOC_NEST_PARENT |
>> +              IOMMU_HWPT_ALLOC_DIRTY_TRACKING) |
>> +              IOMMU_HWPT_ALLOC_PASID))
>>                  return ERR_PTR(-EOPNOTSUPP);
>>          if (nested_parent && !nested_supported(iommu))
>>                  return ERR_PTR(-EOPNOTSUPP);
>>          if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>>                  return ERR_PTR(-EOPNOTSUPP);
>> +       if ((flags & IOMMU_HWPT_ALLOC_PASID) && !pasid_supported(iommu))
>> +               return ERR_PTR(-EOPNOTSUPP);
> I think the decision here should only be about supported domain
> type. Like on AMD if the iommu instance doesn't support v2 then it
> should fail.
> 
> If there is some other thing that says the instance supports pasid,
> that should be checked during attach time only.
> 
> IIRC intel can use either page table format for pasid, so it probably
> doesn't need to do anything here?

yes.

Thanks,
baolu

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:28PM +0000, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
> If both IOMMU and device supports PASID it will allocate domain.
> Otherwise return error.

Do we want the "and device" language here?

I guess we need to decide and make it consistent.

This is if the iommu driver/core code should fail the flag if the
device doesn't have a PCI PASID cap.

> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>   *                                   enforced on device attachment
>   * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>   *                             valid.
> + * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
> + *                          domain can be attached to any PASID on the device.
> + *                          Any domain attached to the non-PASID part of the
> + *                          device must also be flaged, otherwise attaching a
> + *                          PASID will blocked.
>   */

And the choice needs to be documented here

Jason

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

Jason,


On 10/18/2024 7:46 PM, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 02:01:28PM +0000, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
>> If both IOMMU and device supports PASID it will allocate domain.
>> Otherwise return error.
> 
> Do we want the "and device" language here?

Yeah. Forgot to update the description. I have dropped "and device" part.

Based on discussion in cover letter thread, expectation is if driver doesn't
support PASID it will return -EOPNOTSUPP.

In DMA-API mode core will check for above error and requests driver to allocate
domain without `flags`

> 
> I guess we need to decide and make it consistent.
> 
> This is if the iommu driver/core code should fail the flag if the
> device doesn't have a PCI PASID cap.
> 
>> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>>   *                                   enforced on device attachment
>>   * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>>   *                             valid.
>> + * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
>> + *                          domain can be attached to any PASID on the device.
>> + *                          Any domain attached to the non-PASID part of the
>> + *                          device must also be flaged, otherwise attaching a
>> + *                          PASID will blocked.
>>   */
> 
> And the choice needs to be documented here

Yes.

-Vasant

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Yi Liu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
> If both IOMMU and device supports PASID it will allocate domain.
> Otherwise return error.

I think my iommufd pasid series depends on the flag added here. As Jason
suggested[1], I would add a check in iommufd to ensure all the domains that
used in the pasid path are allocated with the IOMMU_HWPT_ALLOC_PASID flag.

[1] 
https://lore.kernel.org/linux-iommu/5a6c2676-256a-4fa5-b9a0-e433d4e933c9@intel.com/

> 
> Also modify __iommu_group_alloc_default_domain() to call
> iommu_paging_domain_alloc_flags() with appropriate flag when allocating
> paging domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>   drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------
>   include/uapi/linux/iommufd.h |  6 +++++
>   2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e13d64ffd14f..107016b3ba3d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -32,6 +32,7 @@
>   #include <trace/events/iommu.h>
>   #include <linux/sched/mm.h>
>   #include <linux/msi.h>
> +#include <uapi/linux/iommufd.h>
>   
>   #include "dma-iommu.h"
>   #include "iommu-priv.h"
> @@ -99,6 +100,9 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
>   static int __iommu_attach_group(struct iommu_domain *domain,
>   				struct iommu_group *group);
> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
> +						       unsigned int type,
> +						       unsigned int flags);
>   
>   enum {
>   	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
> @@ -1589,8 +1593,19 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   static struct iommu_domain *
>   __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   {
> +	struct device *dev = iommu_group_first_dev(group);
> +
>   	if (group->default_domain && group->default_domain->type == req_type)
>   		return group->default_domain;
> +
> +	/*
> +	 * When allocating the DMA API domain assume that the driver is going to
> +	 * use PASID and make sure the RID's domain is PASID compatible.
> +	 */
> +	if (req_type & __IOMMU_DOMAIN_PAGING)
> +		return __iommu_paging_domain_alloc_flags(dev, req_type,
> +				dev->iommu->max_pasids ? IOMMU_HWPT_ALLOC_PASID : 0);
> +
>   	return __iommu_group_domain_alloc(group, req_type);
>   }
>   
> @@ -2026,16 +2041,9 @@ struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
> -/**
> - * iommu_paging_domain_alloc_flags() - Allocate a paging domain
> - * @dev: device for which the domain is allocated
> - * @flags: Bitmap of iommufd_hwpt_alloc_flags
> - *
> - * Allocate a paging domain which will be managed by a kernel driver. Return
> - * allocated domain if successful, or a ERR pointer for failure.
> - */
> -struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> -						     unsigned int flags)
> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
> +						       unsigned int type,
> +						       unsigned int flags)
>   {
>   	const struct iommu_ops *ops;
>   	struct iommu_domain *domain;
> @@ -2059,9 +2067,24 @@ struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
>   	if (!domain)
>   		return ERR_PTR(-ENOMEM);
>   
> -	iommu_domain_init(domain, IOMMU_DOMAIN_UNMANAGED, ops);
> +	iommu_domain_init(domain, type, ops);
>   	return 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
> + *
> + * Allocate a paging domain which will be managed by a kernel driver. Return
> + * allocated domain if successful, or a ERR pointer for failure.
> + */
> +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> +						     unsigned int flags)
> +{
> +	return __iommu_paging_domain_alloc_flags(dev,
> +					 IOMMU_DOMAIN_UNMANAGED, flags);
> +}
>   EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
>   
>   void iommu_domain_free(struct iommu_domain *domain)
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 72010f71c5e4..d1b07f4d9d32 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>    *                                   enforced on device attachment
>    * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>    *                             valid.
> + * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
> + *                          domain can be attached to any PASID on the device.
> + *                          Any domain attached to the non-PASID part of the
> + *                          device must also be flaged, otherwise attaching a
> + *                          PASID will blocked.

Just a double check.

Can I interpret this as "all the domains used by a pasid-capable device
(both the RID and the PASIDs) should be allocated with this flag set"?

>    */
>   enum iommufd_hwpt_alloc_flags {
>   	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>   	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
>   	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
> +	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
>   };
>   
>   /**

-- 
Regards,
Yi Liu

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

Hi Yi,


On 10/21/2024 1:27 PM, Yi Liu wrote:
> On 2024/10/17 22:01, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
>> If both IOMMU and device supports PASID it will allocate domain.
>> Otherwise return error.
> 
> I think my iommufd pasid series depends on the flag added here. As Jason
> suggested[1], I would add a check in iommufd to ensure all the domains that
> used in the pasid path are allocated with the IOMMU_HWPT_ALLOC_PASID flag.
> 
> [1]
> https://lore.kernel.org/linux-iommu/5a6c2676-256a-4fa5-b9a0-e433d4e933c9@intel.com/
> 

Sure. Hopefully we have sorted out all the issues. I will post v4 soon.

>>
>> Also modify __iommu_group_alloc_default_domain() to call
>> iommu_paging_domain_alloc_flags() with appropriate flag when allocating
>> paging domain.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
>> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>>   drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------


.../...

>> +}
>>   EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
>>     void iommu_domain_free(struct iommu_domain *domain)
>> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
>> index 72010f71c5e4..d1b07f4d9d32 100644
>> --- a/include/uapi/linux/iommufd.h
>> +++ b/include/uapi/linux/iommufd.h
>> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>>    *                                   enforced on device attachment
>>    * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>>    *                             valid.
>> + * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
>> + *                          domain can be attached to any PASID on the device.
>> + *                          Any domain attached to the non-PASID part of the
>> + *                          device must also be flaged, otherwise attaching a
>> + *                          PASID will blocked.
> 
> Just a double check.
> 
> Can I interpret this as "all the domains used by a pasid-capable device
> (both the RID and the PASIDs) should be allocated with this flag set"?

If IOMMU supports PASID, then yes. Otherwise it will fallback to non-PASID
domain in DMA-API mode.

-Vasant

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Yi Liu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@ziepe.ca>
> 
> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
> If both IOMMU and device supports PASID it will allocate domain.
> Otherwise return error.
> 
> Also modify __iommu_group_alloc_default_domain() to call
> iommu_paging_domain_alloc_flags() with appropriate flag when allocating
> paging domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
>   drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------
>   include/uapi/linux/iommufd.h |  6 +++++
>   2 files changed, 40 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index e13d64ffd14f..107016b3ba3d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -32,6 +32,7 @@
>   #include <trace/events/iommu.h>
>   #include <linux/sched/mm.h>
>   #include <linux/msi.h>
> +#include <uapi/linux/iommufd.h>
>   
>   #include "dma-iommu.h"
>   #include "iommu-priv.h"
> @@ -99,6 +100,9 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>   				 struct device *dev);
>   static int __iommu_attach_group(struct iommu_domain *domain,
>   				struct iommu_group *group);
> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
> +						       unsigned int type,
> +						       unsigned int flags);

nit:

+static struct iommu_domain *
+__iommu_paging_domain_alloc_flags(struct device *dev, unsigned int type,
+				   unsigned int flags);

>   
>   enum {
>   	IOMMU_SET_DOMAIN_MUST_SUCCEED = 1 << 0,
> @@ -1589,8 +1593,19 @@ EXPORT_SYMBOL_GPL(fsl_mc_device_group);
>   static struct iommu_domain *
>   __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>   {
> +	struct device *dev = iommu_group_first_dev(group);
> +
>   	if (group->default_domain && group->default_domain->type == req_type)
>   		return group->default_domain;
> +
> +	/*
> +	 * When allocating the DMA API domain assume that the driver is going to
> +	 * use PASID and make sure the RID's domain is PASID compatible.
> +	 */
> +	if (req_type & __IOMMU_DOMAIN_PAGING)
> +		return __iommu_paging_domain_alloc_flags(dev, req_type,
> +				dev->iommu->max_pasids ? IOMMU_HWPT_ALLOC_PASID : 0);
> +
>   	return __iommu_group_domain_alloc(group, req_type);
>   }
>   
> @@ -2026,16 +2041,9 @@ struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus)
>   }
>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>   
> -/**
> - * iommu_paging_domain_alloc_flags() - Allocate a paging domain
> - * @dev: device for which the domain is allocated
> - * @flags: Bitmap of iommufd_hwpt_alloc_flags
> - *
> - * Allocate a paging domain which will be managed by a kernel driver. Return
> - * allocated domain if successful, or a ERR pointer for failure.
> - */
> -struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> -						     unsigned int flags)
> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *dev,
> +						       unsigned int type,
> +						       unsigned int flags)
>   {
>   	const struct iommu_ops *ops;
>   	struct iommu_domain *domain;
> @@ -2059,9 +2067,24 @@ struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
>   	if (!domain)
>   		return ERR_PTR(-ENOMEM);
>   
> -	iommu_domain_init(domain, IOMMU_DOMAIN_UNMANAGED, ops);
> +	iommu_domain_init(domain, type, ops);
>   	return 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
> + *
> + * Allocate a paging domain which will be managed by a kernel driver. Return
> + * allocated domain if successful, or a ERR pointer for failure.
> + */
> +struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> +						     unsigned int flags)
> +{
> +	return __iommu_paging_domain_alloc_flags(dev,
> +					 IOMMU_DOMAIN_UNMANAGED, flags);
> +}
>   EXPORT_SYMBOL_GPL(iommu_paging_domain_alloc_flags);
>   
>   void iommu_domain_free(struct iommu_domain *domain)
> diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
> index 72010f71c5e4..d1b07f4d9d32 100644
> --- a/include/uapi/linux/iommufd.h
> +++ b/include/uapi/linux/iommufd.h
> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>    *                                   enforced on device attachment
>    * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>    *                             valid.
> + * @IOMMU_HWPT_ALLOC_PASID: Requests a domain that can be used with PASID. The
> + *                          domain can be attached to any PASID on the device.
> + *                          Any domain attached to the non-PASID part of the
> + *                          device must also be flaged, otherwise attaching a
> + *                          PASID will blocked.
>    */
>   enum iommufd_hwpt_alloc_flags {
>   	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>   	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
>   	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
> +	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
>   };
>   
>   /**

-- 
Regards,
Yi Liu

Re: [PATCH v3 03/12] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

On 10/21/2024 1:29 PM, Yi Liu wrote:
> On 2024/10/17 22:01, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Introduce new flag (IOMMU_HWPT_ALLOC_PASID) to domain_alloc_users() ops.
>> If both IOMMU and device supports PASID it will allocate domain.
>> Otherwise return error.
>>
>> Also modify __iommu_group_alloc_default_domain() to call
>> iommu_paging_domain_alloc_flags() with appropriate flag when allocating
>> paging domain.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
>> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>>   drivers/iommu/iommu.c        | 45 +++++++++++++++++++++++++++---------
>>   include/uapi/linux/iommufd.h |  6 +++++
>>   2 files changed, 40 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index e13d64ffd14f..107016b3ba3d 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -32,6 +32,7 @@
>>   #include <trace/events/iommu.h>
>>   #include <linux/sched/mm.h>
>>   #include <linux/msi.h>
>> +#include <uapi/linux/iommufd.h>
>>     #include "dma-iommu.h"
>>   #include "iommu-priv.h"
>> @@ -99,6 +100,9 @@ static int __iommu_attach_device(struct iommu_domain *domain,
>>                    struct device *dev);
>>   static int __iommu_attach_group(struct iommu_domain *domain,
>>                   struct iommu_group *group);
>> +static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device
>> *dev,
>> +                               unsigned int type,
>> +                               unsigned int flags);
> 
> nit:
> 
> +static struct iommu_domain *
> +__iommu_paging_domain_alloc_flags(struct device *dev, unsigned int type,
> +                   unsigned int flags);

Fixed.

Thanks
-Vasant

[PATCH v3 04/12] iommu/arm-smmu-v3: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Core layer is modified to call domain_alloc_user() to allocate PASID
capable domain. Enhance arm_smmu_domain_alloc_user() to allocate
PASID capable domain based on the 'flags' parameter.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 737c5b882355..4d4af11c5fda 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3088,6 +3088,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
 	struct arm_smmu_domain *smmu_domain;
 	int ret;
 
+	if (flags & IOMMU_HWPT_ALLOC_PASID)
+		return arm_smmu_domain_alloc_paging(dev);
+
 	if (flags & ~PAGING_FLAGS)
 		return ERR_PTR(-EOPNOTSUPP);
 	if (parent || user_data)
-- 
2.31.1

Re: [PATCH v3 04/12] iommu/arm-smmu-v3: Enhance domain_alloc_user() to allocate PASID capable domain

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:29PM +0000, Vasant Hegde wrote:
> Core layer is modified to call domain_alloc_user() to allocate PASID
> capable domain. Enhance arm_smmu_domain_alloc_user() to allocate
> PASID capable domain based on the 'flags' parameter.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 737c5b882355..4d4af11c5fda 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3088,6 +3088,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  	struct arm_smmu_domain *smmu_domain;
>  	int ret;
>  
> +	if (flags & IOMMU_HWPT_ALLOC_PASID)
> +		return arm_smmu_domain_alloc_paging(dev);
> +
>  	if (flags & ~PAGING_FLAGS)
>  		return ERR_PTR(-EOPNOTSUPP);

Swap the order, it needs to be after checking PAGING_FLAGS and add it
to PAGING_FLAGS so it is checked.

Still need to reject bad flags.

Jason

Re: [PATCH v3 04/12] iommu/arm-smmu-v3: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Jason,


On 10/18/2024 7:53 PM, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 02:01:29PM +0000, Vasant Hegde wrote:
>> Core layer is modified to call domain_alloc_user() to allocate PASID
>> capable domain. Enhance arm_smmu_domain_alloc_user() to allocate
>> PASID capable domain based on the 'flags' parameter.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 737c5b882355..4d4af11c5fda 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -3088,6 +3088,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>>  	struct arm_smmu_domain *smmu_domain;
>>  	int ret;
>>  
>> +	if (flags & IOMMU_HWPT_ALLOC_PASID)
>> +		return arm_smmu_domain_alloc_paging(dev);
>> +
>>  	if (flags & ~PAGING_FLAGS)
>>  		return ERR_PTR(-EOPNOTSUPP);
> 
> Swap the order, it needs to be after checking PAGING_FLAGS and add it
> to PAGING_FLAGS so it is checked.
> 
> Still need to reject bad flags.

Ok sure. Will fix it.

-Vasant

[PATCH v3 05/12] iommu/amd: Add helper function to check GIOSUP/GTSUP

[Vasant Hegde]

amd_iommu_gt_ppr_supported() only checks for GTSUP. To support PASID
with V2 page table we need GIOSUP as well. Hence add new helper function
to check GIOSUP/GTSUP.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/amd_iommu.h | 7 ++++++-
 drivers/iommu/amd/init.c      | 3 +--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 6386fa4556d9..3b6c6332b09f 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -118,9 +118,14 @@ static inline bool check_feature2(u64 mask)
 	return (amd_iommu_efr2 & mask);
 }
 
+static inline bool amd_iommu_v2_pgtbl_supported(void)
+{
+	return (check_feature(FEATURE_GIOSUP) && check_feature(FEATURE_GT));
+}
+
 static inline bool amd_iommu_gt_ppr_supported(void)
 {
-	return (check_feature(FEATURE_GT) &&
+	return (amd_iommu_v2_pgtbl_supported() &&
 		check_feature(FEATURE_PPR) &&
 		check_feature(FEATURE_EPHSUP));
 }
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 43131c3a2172..7ae478d95e2b 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2071,8 +2071,7 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 	init_iommu_perf_ctr(iommu);
 
 	if (amd_iommu_pgtable == AMD_IOMMU_V2) {
-		if (!check_feature(FEATURE_GIOSUP) ||
-		    !check_feature(FEATURE_GT)) {
+		if (!amd_iommu_v2_pgtbl_supported()) {
 			pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
 			amd_iommu_pgtable = AMD_IOMMU_V1;
 		}
-- 
2.31.1

Re: [PATCH v3 05/12] iommu/amd: Add helper function to check GIOSUP/GTSUP

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:30PM +0000, Vasant Hegde wrote:
> amd_iommu_gt_ppr_supported() only checks for GTSUP. To support PASID
> with V2 page table we need GIOSUP as well. Hence add new helper function
> to check GIOSUP/GTSUP.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/amd_iommu.h | 7 ++++++-
>  drivers/iommu/amd/init.c      | 3 +--
>  2 files changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 06/12] iommu/amd: Move V2 page table support check to early_amd_iommu_init()

[Vasant Hegde]

amd_iommu_pgtable validation has to be done before calling
iommu_snp_enable(). It can be done immediately after reading IOMMU
features. Hence move this check to early_amd_iommu_init().

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/init.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 7ae478d95e2b..7c883be22559 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2070,13 +2070,6 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 
 	init_iommu_perf_ctr(iommu);
 
-	if (amd_iommu_pgtable == AMD_IOMMU_V2) {
-		if (!amd_iommu_v2_pgtbl_supported()) {
-			pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
-			amd_iommu_pgtable = AMD_IOMMU_V1;
-		}
-	}
-
 	if (is_rd890_iommu(iommu->dev)) {
 		int i, j;
 
@@ -3090,6 +3083,13 @@ static int __init early_amd_iommu_init(void)
 	    FIELD_GET(FEATURE_GATS, amd_iommu_efr) == GUEST_PGTABLE_5_LEVEL)
 		amd_iommu_gpt_level = PAGE_MODE_5_LEVEL;
 
+	if (amd_iommu_pgtable == AMD_IOMMU_V2) {
+		if (!amd_iommu_v2_pgtbl_supported()) {
+			pr_warn("Cannot enable v2 page table for DMA-API. Fallback to v1.\n");
+			amd_iommu_pgtable = AMD_IOMMU_V1;
+		}
+	}
+
 	/* Disable any previously enabled IOMMUs */
 	if (!is_kdump_kernel() || amd_iommu_disabled)
 		disable_iommus();
-- 
2.31.1

Re: [PATCH v3 06/12] iommu/amd: Move V2 page table support check to early_amd_iommu_init()

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:31PM +0000, Vasant Hegde wrote:
> amd_iommu_pgtable validation has to be done before calling
> iommu_snp_enable(). It can be done immediately after reading IOMMU
> features. Hence move this check to early_amd_iommu_init().
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/init.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 07/12] iommu/amd: Separate page table setup from domain allocation

[Vasant Hegde]

Currently protection_domain_alloc() allocates domain and also sets up
page table. Page table setup is required for PAGING domain only. Domain
type like SVA doesn't need page table. Hence move page table setup code
to separate function.

Also SVA domain allocation path does not call pdom_setup_pgtable().
Hence remove IOMMU_DOMAIN_SVA type check.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jacob Pan <jacob.pan@linux.microsoft.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/iommu.c | 42 ++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8364cd6fa47d..6285fd1afd50 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2265,28 +2265,36 @@ void protection_domain_free(struct protection_domain *domain)
 
 struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 {
-	struct io_pgtable_ops *pgtbl_ops;
 	struct protection_domain *domain;
-	int pgtable;
 
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return NULL;
 
 	domain->id = domain_id_alloc();
-	if (!domain->id)
-		goto err_free;
+	if (!domain->id) {
+		kfree(domain);
+		return NULL;
+	}
 
 	spin_lock_init(&domain->lock);
 	INIT_LIST_HEAD(&domain->dev_list);
 	INIT_LIST_HEAD(&domain->dev_data_list);
 	domain->iop.pgtbl.cfg.amd.nid = nid;
 
+	return domain;
+}
+
+static int pdom_setup_pgtable(struct protection_domain *domain,
+			      unsigned int type)
+{
+	struct io_pgtable_ops *pgtbl_ops;
+	int pgtable;
+
 	switch (type) {
 	/* No need to allocate io pgtable ops in passthrough mode */
 	case IOMMU_DOMAIN_IDENTITY:
-	case IOMMU_DOMAIN_SVA:
-		return domain;
+		return 0;
 	case IOMMU_DOMAIN_DMA:
 		pgtable = amd_iommu_pgtable;
 		break;
@@ -2298,7 +2306,7 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 		pgtable = AMD_IOMMU_V1;
 		break;
 	default:
-		goto err_id;
+		return -EINVAL;
 	}
 
 	switch (pgtable) {
@@ -2309,20 +2317,14 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 		domain->pd_mode = PD_MODE_V2;
 		break;
 	default:
-		goto err_id;
+		return -EINVAL;
 	}
-
 	pgtbl_ops =
 		alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl.cfg, domain);
 	if (!pgtbl_ops)
-		goto err_id;
+		return -ENOMEM;
 
-	return domain;
-err_id:
-	domain_id_free(domain->id);
-err_free:
-	kfree(domain);
-	return NULL;
+	return 0;
 }
 
 static inline u64 dma_max_address(void)
@@ -2345,6 +2347,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	struct protection_domain *domain;
 	struct amd_iommu *iommu = NULL;
+	int ret;
 
 	if (dev)
 		iommu = get_amd_iommu_from_dev(dev);
@@ -2364,6 +2367,13 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
+	ret = pdom_setup_pgtable(domain, type);
+	if (ret) {
+		domain_id_free(domain->id);
+		kfree(domain);
+		return ERR_PTR(ret);
+	}
+
 	domain->domain.geometry.aperture_start = 0;
 	domain->domain.geometry.aperture_end   = dma_max_address();
 	domain->domain.geometry.force_aperture = true;
-- 
2.31.1

[PATCH v3 08/12] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Vasant Hegde]

Current code forces v1 page table for UNMANAGED domain and global page
table type (amd_iommu_pgtable) for rest of paging domain.

Following patch series adds support for domain_alloc_paging() ops. Also
enhances domain_alloc_user() to allocate page table based on 'flags.

Hence pass page table type as parameter to pdomain_setup_pgtable(). So
that caller can decide right page table type. Also update
dma_max_address() to take pgtable as parameter.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jacob Pan <jacob.pan@linux.microsoft.com>
---
 drivers/iommu/amd/iommu.c | 43 +++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 6285fd1afd50..cb6a72564c23 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2286,28 +2286,13 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 }
 
 static int pdom_setup_pgtable(struct protection_domain *domain,
-			      unsigned int type)
+			      unsigned int type, int pgtable)
 {
 	struct io_pgtable_ops *pgtbl_ops;
-	int pgtable;
 
-	switch (type) {
 	/* No need to allocate io pgtable ops in passthrough mode */
-	case IOMMU_DOMAIN_IDENTITY:
+	if (!(type & __IOMMU_DOMAIN_PAGING))
 		return 0;
-	case IOMMU_DOMAIN_DMA:
-		pgtable = amd_iommu_pgtable;
-		break;
-	/*
-	 * Force IOMMU v1 page table when allocating
-	 * domain for pass-through devices.
-	 */
-	case IOMMU_DOMAIN_UNMANAGED:
-		pgtable = AMD_IOMMU_V1;
-		break;
-	default:
-		return -EINVAL;
-	}
 
 	switch (pgtable) {
 	case AMD_IOMMU_V1:
@@ -2319,6 +2304,7 @@ static int pdom_setup_pgtable(struct protection_domain *domain,
 	default:
 		return -EINVAL;
 	}
+
 	pgtbl_ops =
 		alloc_io_pgtable_ops(pgtable, &domain->iop.pgtbl.cfg, domain);
 	if (!pgtbl_ops)
@@ -2327,9 +2313,9 @@ static int pdom_setup_pgtable(struct protection_domain *domain,
 	return 0;
 }
 
-static inline u64 dma_max_address(void)
+static inline u64 dma_max_address(int pgtable)
 {
-	if (amd_iommu_pgtable == AMD_IOMMU_V1)
+	if (pgtable == AMD_IOMMU_V1)
 		return ~0ULL;
 
 	/* V2 with 4/5 level page table */
@@ -2342,7 +2328,8 @@ static bool amd_iommu_hd_support(struct amd_iommu *iommu)
 }
 
 static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
-						  struct device *dev, u32 flags)
+						  struct device *dev,
+						  u32 flags, int pgtable)
 {
 	bool dirty_tracking = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
 	struct protection_domain *domain;
@@ -2367,7 +2354,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
 
-	ret = pdom_setup_pgtable(domain, type);
+	ret = pdom_setup_pgtable(domain, type, pgtable);
 	if (ret) {
 		domain_id_free(domain->id);
 		kfree(domain);
@@ -2375,7 +2362,7 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 	}
 
 	domain->domain.geometry.aperture_start = 0;
-	domain->domain.geometry.aperture_end   = dma_max_address();
+	domain->domain.geometry.aperture_end   = dma_max_address(pgtable);
 	domain->domain.geometry.force_aperture = true;
 	domain->domain.pgsize_bitmap = domain->iop.pgtbl.cfg.pgsize_bitmap;
 
@@ -2393,8 +2380,16 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
 static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
 {
 	struct iommu_domain *domain;
+	int pgtable = amd_iommu_pgtable;
+
+	/*
+	 * Force IOMMU v1 page table when allocating
+	 * domain for pass-through devices.
+	 */
+	if (type == IOMMU_DOMAIN_UNMANAGED)
+		pgtable = AMD_IOMMU_V1;
 
-	domain = do_iommu_domain_alloc(type, NULL, 0);
+	domain = do_iommu_domain_alloc(type, NULL, 0, pgtable);
 	if (IS_ERR(domain))
 		return NULL;
 
@@ -2412,7 +2407,7 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return do_iommu_domain_alloc(type, dev, flags);
+	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
 }
 
 void amd_iommu_domain_free(struct iommu_domain *dom)
-- 
2.31.1

Re: [PATCH v3 08/12] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:33PM +0000, Vasant Hegde wrote:
> Current code forces v1 page table for UNMANAGED domain and global page
> table type (amd_iommu_pgtable) for rest of paging domain.
> 
> Following patch series adds support for domain_alloc_paging() ops. Also
> enhances domain_alloc_user() to allocate page table based on 'flags.
> 
> Hence pass page table type as parameter to pdomain_setup_pgtable(). So
> that caller can decide right page table type. Also update
> dma_max_address() to take pgtable as parameter.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> Reviewed-by: Jacob Pan <jacob.pan@linux.microsoft.com>
> ---
>  drivers/iommu/amd/iommu.c | 43 +++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

[PATCH v3 09/12] iommu/amd: Enhance amd_iommu_domain_alloc_user()

[Vasant Hegde]

Previous patch enhanced core layer to check device PASID capability and
pass right flags to ops->domain_alloc_user().

Enhance amd_iommu_domain_alloc_user() to allocate domain with
appropriate page table based on flags parameter.
  - If flag is empty then allocate domain with default page table type.
    This will eventually replace ops->domain_alloc().
    For UNMANAGED domain, core will call this interface with flags=0. So
    AMD driver will continue to allocate V1 page table.

  - If IOMMU_HWPT_ALLOC_PASID flag is passed then allocate domain with v2
    page table.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/amd/iommu.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cb6a72564c23..87eff85c0c23 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2404,6 +2404,20 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 {
 	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
 
+	/* Allocate domain with default page table */
+	if (!flags) {
+		return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
+					     dev, 0, amd_iommu_pgtable);
+	}
+
+	/* Allocate domain with v2 page table if IOMMU supports PASID. */
+	if (flags & IOMMU_HWPT_ALLOC_PASID) {
+		if (!amd_iommu_pasid_supported())
+			return ERR_PTR(-EINVAL);
+
+		return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V2);
+	}
+
 	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
-- 
2.31.1

Re: [PATCH v3 09/12] iommu/amd: Enhance amd_iommu_domain_alloc_user()

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:34PM +0000, Vasant Hegde wrote:
> Previous patch enhanced core layer to check device PASID capability and
> pass right flags to ops->domain_alloc_user().
> 
> Enhance amd_iommu_domain_alloc_user() to allocate domain with
> appropriate page table based on flags parameter.
>   - If flag is empty then allocate domain with default page table type.
>     This will eventually replace ops->domain_alloc().
>     For UNMANAGED domain, core will call this interface with flags=0. So
>     AMD driver will continue to allocate V1 page table.
> 
>   - If IOMMU_HWPT_ALLOC_PASID flag is passed then allocate domain with v2
>     page table.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index cb6a72564c23..87eff85c0c23 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2404,6 +2404,20 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>  {
>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
>  
> +	/* Allocate domain with default page table */
> +	if (!flags) {
> +		return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
> +					     dev, 0, amd_iommu_pgtable);
> +	}

Extra {} ?

> +	/* Allocate domain with v2 page table if IOMMU supports PASID. */
> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> +		if (!amd_iommu_pasid_supported())
> +			return ERR_PTR(-EINVAL);

Depending on what we decide this could be
amd_iommu_v2_pgtbl_supported()

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH v3 09/12] iommu/amd: Enhance amd_iommu_domain_alloc_user()

[Vasant Hegde]

Jason,


On 10/18/2024 7:57 PM, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 02:01:34PM +0000, Vasant Hegde wrote:
>> Previous patch enhanced core layer to check device PASID capability and
>> pass right flags to ops->domain_alloc_user().
>>
>> Enhance amd_iommu_domain_alloc_user() to allocate domain with
>> appropriate page table based on flags parameter.
>>   - If flag is empty then allocate domain with default page table type.
>>     This will eventually replace ops->domain_alloc().
>>     For UNMANAGED domain, core will call this interface with flags=0. So
>>     AMD driver will continue to allocate V1 page table.
>>
>>   - If IOMMU_HWPT_ALLOC_PASID flag is passed then allocate domain with v2
>>     page table.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index cb6a72564c23..87eff85c0c23 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2404,6 +2404,20 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>>  {
>>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
>>  
>> +	/* Allocate domain with default page table */
>> +	if (!flags) {
>> +		return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
>> +					     dev, 0, amd_iommu_pgtable);
>> +	}
> 
> Extra {} ?

For multi line I preferred {}.

> 
>> +	/* Allocate domain with v2 page table if IOMMU supports PASID. */
>> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
>> +		if (!amd_iommu_pasid_supported())
>> +			return ERR_PTR(-EINVAL);
> 
> Depending on what we decide this could be
> amd_iommu_v2_pgtbl_supported()

Sure.

> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks
-Vasant

RE: [PATCH v3 09/12] iommu/amd: Enhance amd_iommu_domain_alloc_user()

[Tian, Kevin]

> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Thursday, October 17, 2024 10:02 PM
> 
> +
> +	/* Allocate domain with v2 page table if IOMMU supports PASID. */
> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> +		if (!amd_iommu_pasid_supported())
> +			return ERR_PTR(-EINVAL);
> +
> +		return do_iommu_domain_alloc(type, dev, flags,
> AMD_IOMMU_V2);
> +	}
> +
>  	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent ||
> user_data)
>  		return ERR_PTR(-EOPNOTSUPP);

swap the order and enhance above check to cover _PASID too.

Re: [PATCH v3 09/12] iommu/amd: Enhance amd_iommu_domain_alloc_user()

[Vasant Hegde]

Kevin,,


On 10/22/2024 1:18 PM, Tian, Kevin wrote:
>> From: Vasant Hegde <vasant.hegde@amd.com>
>> Sent: Thursday, October 17, 2024 10:02 PM
>>
>> +
>> +	/* Allocate domain with v2 page table if IOMMU supports PASID. */
>> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
>> +		if (!amd_iommu_pasid_supported())
>> +			return ERR_PTR(-EINVAL);
>> +
>> +		return do_iommu_domain_alloc(type, dev, flags,
>> AMD_IOMMU_V2);
>> +	}
>> +
>>  	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent ||
>> user_data)
>>  		return ERR_PTR(-EOPNOTSUPP);
> 
> swap the order and enhance above check to cover _PASID too.

Sure. Will fix it.

-Vasant

[PATCH v3 10/12] iommu/amd: Implement global identity domain

[Vasant Hegde]

Implement global identity domain. All devices groups in identity domain
will share this domain.

In attach device path, based on device capability it will allocate per
device domain ID and GCR3 table. So that it can support SVA.

Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/amd/amd_iommu.h |  1 +
 drivers/iommu/amd/init.c      |  3 +++
 drivers/iommu/amd/iommu.c     | 36 +++++++++++++++++++++++++++++++----
 3 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index 3b6c6332b09f..38509e1019e9 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -46,6 +46,7 @@ extern int amd_iommu_gpt_level;
 extern unsigned long amd_iommu_pgsize_bitmap;
 
 /* Protection domain ops */
+void amd_iommu_init_identity_domain(void);
 struct protection_domain *protection_domain_alloc(unsigned int type, int nid);
 void protection_domain_free(struct protection_domain *domain);
 struct iommu_domain *amd_iommu_domain_alloc_sva(struct device *dev,
diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index 7c883be22559..45fdba48d54d 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2164,6 +2164,9 @@ static int __init amd_iommu_init_pci(void)
 	struct amd_iommu_pci_seg *pci_seg;
 	int ret;
 
+	/* Init global identity domain before registering IOMMU */
+	amd_iommu_init_identity_domain();
+
 	for_each_iommu(iommu) {
 		ret = iommu_init_pci(iommu);
 		if (ret) {
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 87eff85c0c23..893b8ac82fa4 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -74,6 +74,9 @@ struct kmem_cache *amd_iommu_irq_cache;
 
 static void detach_device(struct device *dev);
 
+static int amd_iommu_attach_device(struct iommu_domain *dom,
+				   struct device *dev);
+
 static void set_dte_entry(struct amd_iommu *iommu,
 			  struct iommu_dev_data *dev_data);
 
@@ -2263,6 +2266,14 @@ void protection_domain_free(struct protection_domain *domain)
 	kfree(domain);
 }
 
+static void protection_domain_init(struct protection_domain *domain, int nid)
+{
+	spin_lock_init(&domain->lock);
+	INIT_LIST_HEAD(&domain->dev_list);
+	INIT_LIST_HEAD(&domain->dev_data_list);
+	domain->iop.pgtbl.cfg.amd.nid = nid;
+}
+
 struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 {
 	struct protection_domain *domain;
@@ -2277,10 +2288,7 @@ struct protection_domain *protection_domain_alloc(unsigned int type, int nid)
 		return NULL;
 	}
 
-	spin_lock_init(&domain->lock);
-	INIT_LIST_HEAD(&domain->dev_list);
-	INIT_LIST_HEAD(&domain->dev_data_list);
-	domain->iop.pgtbl.cfg.amd.nid = nid;
+	protection_domain_init(domain, nid);
 
 	return domain;
 }
@@ -2463,6 +2471,25 @@ static struct iommu_domain blocked_domain = {
 	}
 };
 
+static struct protection_domain identity_domain;
+
+static const struct iommu_domain_ops identity_domain_ops = {
+	.attach_dev = amd_iommu_attach_device,
+};
+
+void amd_iommu_init_identity_domain(void)
+{
+	struct iommu_domain *domain = &identity_domain.domain;
+
+	domain->type = IOMMU_DOMAIN_IDENTITY;
+	domain->ops = &identity_domain_ops;
+	domain->owner = &amd_iommu_ops;
+
+	identity_domain.id = domain_id_alloc();
+
+	protection_domain_init(&identity_domain, NUMA_NO_NODE);
+}
+
 static int amd_iommu_attach_device(struct iommu_domain *dom,
 				   struct device *dev)
 {
@@ -2861,6 +2888,7 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.blocked_domain = &blocked_domain,
+	.identity_domain = &identity_domain.domain,
 	.domain_alloc = amd_iommu_domain_alloc,
 	.domain_alloc_user = amd_iommu_domain_alloc_user,
 	.domain_alloc_sva = amd_iommu_domain_alloc_sva,
-- 
2.31.1

[PATCH v3 11/12] iommu: Put domain allocation in __iommu_group_alloc_blocking_domain()

[Vasant Hegde]

From: Jason Gunthorpe <jgg@nvidia.com>

There is no longer a reason to call __iommu_domain_alloc() to allocate
the blocking domain. All drivers that support a native blocking domain
provide it via the ops, for other drivers we should call
iommu_paging_domain_alloc().

__iommu_group_alloc_blocking_domain() is the only place that allocates
an BLOCKED domain, so move the ops->blocked_domain logic there.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/iommu.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 107016b3ba3d..a007fdfd2fee 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1974,8 +1974,6 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 
 	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
 		return ops->identity_domain;
-	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
-		return ops->blocked_domain;
 	else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
 		domain = ops->domain_alloc_paging(dev);
 	else if (ops->domain_alloc)
@@ -3201,22 +3199,25 @@ void iommu_device_unuse_default_domain(struct device *dev)
 
 static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
 {
+	struct device *dev = iommu_group_first_dev(group);
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
 	struct iommu_domain *domain;
 
 	if (group->blocking_domain)
 		return 0;
 
-	domain = __iommu_group_domain_alloc(group, IOMMU_DOMAIN_BLOCKED);
-	if (IS_ERR(domain)) {
-		/*
-		 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
-		 * create an empty domain instead.
-		 */
-		domain = __iommu_group_domain_alloc(group,
-						    IOMMU_DOMAIN_UNMANAGED);
-		if (IS_ERR(domain))
-			return PTR_ERR(domain);
+	if (ops->blocked_domain) {
+		group->blocking_domain = ops->blocked_domain;
+		return 0;
 	}
+
+	/*
+	 * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED create an
+	 * empty PAGING domain instead.
+	 */
+	domain = iommu_paging_domain_alloc(dev);
+	if (IS_ERR(domain))
+		return PTR_ERR(domain);
 	group->blocking_domain = domain;
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v3 11/12] iommu: Put domain allocation in __iommu_group_alloc_blocking_domain()

[Baolu Lu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe<jgg@nvidia.com>
> 
> There is no longer a reason to call __iommu_domain_alloc() to allocate
> the blocking domain. All drivers that support a native blocking domain
> provide it via the ops, for other drivers we should call
> iommu_paging_domain_alloc().
> 
> __iommu_group_alloc_blocking_domain() is the only place that allocates
> an BLOCKED domain, so move the ops->blocked_domain logic there.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
> ---
>   drivers/iommu/iommu.c | 25 +++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Thanks,
baolu

[PATCH v3 12/12] iommu: Create __iommu_alloc_identity_domain()

[Vasant Hegde]

From: Jason Gunthorpe <jgg@nvidia.com>

Consolidate all the code to create an IDENTITY domain into one
function. This the legacy __iommu_domain_alloc() path from all core code
paths, and preps it for final removal.

BLOCKED/IDENTITY/PAGING are now always allocated via a type specific
function.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
---
 drivers/iommu/iommu.c | 43 ++++++++++++++++++++++++++++---------------
 1 file changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index a007fdfd2fee..dfb573d8f213 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -94,8 +94,6 @@ 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_group_domain_alloc(struct iommu_group *group, unsigned int type);
 static int __iommu_attach_device(struct iommu_domain *domain,
 				 struct device *dev);
 static int __iommu_attach_group(struct iommu_domain *domain,
@@ -137,6 +135,8 @@ static struct group_device *iommu_group_alloc_device(struct iommu_group *group,
 						     struct device *dev);
 static void __iommu_group_free_device(struct iommu_group *group,
 				      struct group_device *grp_dev);
+static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
+			      const struct iommu_ops *ops);
 
 #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)		\
 struct iommu_group_attribute iommu_group_attr_##_name =		\
@@ -1590,6 +1590,28 @@ struct iommu_group *fsl_mc_device_group(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(fsl_mc_device_group);
 
+static struct iommu_domain *__iommu_alloc_identity_domain(struct device *dev)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	struct iommu_domain *domain;
+
+	if (ops->identity_domain)
+		return ops->identity_domain;
+
+	/* Older drivers create the identity domain via */
+	if (!ops->domain_alloc)
+		return ERR_PTR(-EOPNOTSUPP);
+
+	domain = ops->domain_alloc(IOMMU_DOMAIN_IDENTITY);
+	if (IS_ERR(domain))
+		return domain;
+	if (!domain)
+		return ERR_PTR(-ENOMEM);
+
+	iommu_domain_init(domain, IOMMU_DOMAIN_IDENTITY, ops);
+	return domain;
+}
+
 static struct iommu_domain *
 __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 {
@@ -1605,8 +1627,9 @@ __iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
 	if (req_type & __IOMMU_DOMAIN_PAGING)
 		return __iommu_paging_domain_alloc_flags(dev, req_type,
 				dev->iommu->max_pasids ? IOMMU_HWPT_ALLOC_PASID : 0);
-
-	return __iommu_group_domain_alloc(group, req_type);
+	if (req_type == IOMMU_DOMAIN_IDENTITY)
+		return __iommu_alloc_identity_domain(dev);
+	return ERR_PTR(-EINVAL);
 }
 
 /*
@@ -1972,9 +1995,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 	struct iommu_domain *domain;
 	unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
 
-	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
-		return ops->identity_domain;
-	else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
+	if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
 		domain = ops->domain_alloc_paging(dev);
 	else if (ops->domain_alloc)
 		domain = ops->domain_alloc(alloc_type);
@@ -1995,14 +2016,6 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 	return domain;
 }
 
-static struct iommu_domain *
-__iommu_group_domain_alloc(struct iommu_group *group, unsigned int type)
-{
-	struct device *dev = iommu_group_first_dev(group);
-
-	return __iommu_domain_alloc(dev_iommu_ops(dev), dev, type);
-}
-
 static int __iommu_domain_alloc_dev(struct device *dev, void *data)
 {
 	const struct iommu_ops **ops = data;
-- 
2.31.1

Re: [PATCH v3 12/12] iommu: Create __iommu_alloc_identity_domain()

[Baolu Lu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> From: Jason Gunthorpe<jgg@nvidia.com>
> 
> Consolidate all the code to create an IDENTITY domain into one
> function. This the legacy __iommu_domain_alloc() path from all core code
> paths, and preps it for final removal.
> 
> BLOCKED/IDENTITY/PAGING are now always allocated via a type specific
> function.
> 
> Signed-off-by: Jason Gunthorpe<jgg@nvidia.com>
> Signed-off-by: Vasant Hegde<vasant.hegde@amd.com>
> ---
>   drivers/iommu/iommu.c | 43 ++++++++++++++++++++++++++++---------------
>   1 file changed, 28 insertions(+), 15 deletions(-)

Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com>

Thanks,
baolu

Re: [PATCH v3 12/12] iommu: Create __iommu_alloc_identity_domain()

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:37PM +0000, Vasant Hegde wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Consolidate all the code to create an IDENTITY domain into one
> function. This the legacy __iommu_domain_alloc() path from all core code
> paths, and preps it for final removal.

Ah some grammar:

Consolidate all the code to create an IDENTITY domain into one
function. This removes the legacy __iommu_domain_alloc() path from all
core code paths, and preps it for final removal.

> +static struct iommu_domain *__iommu_alloc_identity_domain(struct device *dev)
> +{
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	struct iommu_domain *domain;
> +
> +	if (ops->identity_domain)
> +		return ops->identity_domain;
> +
> +	/* Older drivers create the identity domain via */

via ops->domain_alloc()

Jason

Re: [PATCH v3 12/12] iommu: Create __iommu_alloc_identity_domain()

[Vasant Hegde]

On 10/18/2024 8:02 PM, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 02:01:37PM +0000, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@nvidia.com>
>>
>> Consolidate all the code to create an IDENTITY domain into one
>> function. This the legacy __iommu_domain_alloc() path from all core code
>> paths, and preps it for final removal.
> 
> Ah some grammar:
> 
> Consolidate all the code to create an IDENTITY domain into one
> function. This removes the legacy __iommu_domain_alloc() path from all
> core code paths, and preps it for final removal.

Ack. Fixed.

> 
>> +static struct iommu_domain *__iommu_alloc_identity_domain(struct device *dev)
>> +{
>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +	struct iommu_domain *domain;
>> +
>> +	if (ops->identity_domain)
>> +		return ops->identity_domain;
>> +
>> +	/* Older drivers create the identity domain via */
> 
> via ops->domain_alloc()

Fixed.

-Vasant

Re: [PATCH v3 00/12] iommu: Domain allocation enhancements

[Baolu Lu]

On 2024/10/17 22:01, Vasant Hegde wrote:
> This series adds iommu_paging_domain_alloc_flags() which takes flags to
> pass additional details for domain allocation (like domain with PASID
> support). Also adjusts core code to handler variuos domain allocations.
> 
> Also updates AMD IOMMU driver domain allocation code. With this by
> default it will allocate domain with V2 page table for PASID capable
> device and v1 page table for rest of the devices.
> 
> 
> TODO:
> @Jason, @Kevin, @Baolu,
>    With this series if driver implements domain_alloc_user() then we assume
>    IOMMU always supports PASID. If IOMMU doesn't support PASID (ex: AMD driver

I don't think this assumption is true for all iommu drivers. In my
opinion, domain_alloc_user() will eventually be a common callback used
by the iommu core to allocate paging domains. It includes an allocation
flag that specifies various requirements, including PASID, which is a
special requirement for the amd iommu driver.

>    not supporting V2 page table) and device is PASID capable, then driver will
>    return error. Device probe will fail.
> 
>    I think if driver return error, core layer should retry without
>    IOMMU_HWPT_ALLOC_PASID flag. Other option is indivisual driver in error path
>    of domain_alloc_user allocating non-PASID capable domain.
> 
>    I prefer first option. Just wanted to discuss before implementing. Any preferred
>    option here? OR any other better way to solve this?
> 
> 
> Note :
>    vt-d specific changes is handled in separate series [1].
> 
> [1]https://lore.kernel.org/linux-iommu/20241011042722.73930-8- 
> baolu.lu@linux.intel.com/
> 
> @Baolu,
>    Do you want me to include above patch in this series so that it doesn't break intel driver?

I haven't read through the whole series yet, but let's avoid creating
unnecessary dependencies.

Thanks,
baolu

Re: [PATCH v3 00/12] iommu: Domain allocation enhancements

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 02:01:25PM +0000, Vasant Hegde wrote:
> This series adds iommu_paging_domain_alloc_flags() which takes flags to
> pass additional details for domain allocation (like domain with PASID
> support). Also adjusts core code to handler variuos domain allocations.
> 
> Also updates AMD IOMMU driver domain allocation code. With this by
> default it will allocate domain with V2 page table for PASID capable
> device and v1 page table for rest of the devices.
> 
> 
> TODO:
> @Jason, @Kevin, @Baolu,
>   With this series if driver implements domain_alloc_user() then we assume
>   IOMMU always supports PASID. If IOMMU doesn't support PASID (ex: AMD driver
>   not supporting V2 page table) and device is PASID capable, then driver will
>   return error. Device probe will fail.

We shouldn't fail device probe just because someone plugs in a device
with a PCI PASID capability into an old CPU..
 
>   I think if driver return error, core layer should retry without
>   IOMMU_HWPT_ALLOC_PASID flag. 

But this makes sense, and it solves the other problem where if the
driver doesn't know how to support IOMMU_HWPT_ALLOC_PASID it will also
fail. Retrying with 0 flags is correct there too.

>   Other option is indivisual driver in error path
>   of domain_alloc_user allocating non-PASID capable domain.

This would turn IOMMU_HWPT_ALLOC_PASID into a hint, where you might
not get back a PASID capable domain type. That seems like a slightly
worse direction

>   I prefer first option. Just wanted to discuss before implementing. Any preferred
>   option here? OR any other better way to solve this?

I think you should fail in the driver with EOPNOTSUPP and then the
core code allocating a DMA domain should detect EOPNOTSUPP and try
again with a 0 flags. Is that what you mean by first option?

> Note :
>   vt-d specific changes is handled in separate series [1].
> 
> [1] https://lore.kernel.org/linux-iommu/20241011042722.73930-8-baolu.lu@linux.intel.com/
>
> @Baolu,
>   Do you want me to include above patch in this series so that it doesn't break intel driver?

I think trying to avoid inter-dependencies right now is a good idea,
so keep the order where 0 flags calls domain_alloc() first and intel
domain_alloc_user() will always fail with EOPNOTSUPP.

We can sort this out next cycle when more stuff is merged

Jason

Re: [PATCH v3 00/12] iommu: Domain allocation enhancements

[Vasant Hegde]

Jason,


On 10/18/2024 6:20 PM, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 02:01:25PM +0000, Vasant Hegde wrote:
>> This series adds iommu_paging_domain_alloc_flags() which takes flags to
>> pass additional details for domain allocation (like domain with PASID
>> support). Also adjusts core code to handler variuos domain allocations.
>>
>> Also updates AMD IOMMU driver domain allocation code. With this by
>> default it will allocate domain with V2 page table for PASID capable
>> device and v1 page table for rest of the devices.
>>
>>
>> TODO:
>> @Jason, @Kevin, @Baolu,
>>   With this series if driver implements domain_alloc_user() then we assume
>>   IOMMU always supports PASID. If IOMMU doesn't support PASID (ex: AMD driver
>>   not supporting V2 page table) and device is PASID capable, then driver will
>>   return error. Device probe will fail.
> 
> We shouldn't fail device probe just because someone plugs in a device
> with a PCI PASID capability into an old CPU..
>  
>>   I think if driver return error, core layer should retry without
>>   IOMMU_HWPT_ALLOC_PASID flag. 
> 
> But this makes sense, and it solves the other problem where if the
> driver doesn't know how to support IOMMU_HWPT_ALLOC_PASID it will also
> fail. Retrying with 0 flags is correct there too.
> 
>>   Other option is indivisual driver in error path
>>   of domain_alloc_user allocating non-PASID capable domain.
> 
> This would turn IOMMU_HWPT_ALLOC_PASID into a hint, where you might
> not get back a PASID capable domain type. That seems like a slightly
> worse direction
> 
>>   I prefer first option. Just wanted to discuss before implementing. Any preferred
>>   option here? OR any other better way to solve this?
> 
> I think you should fail in the driver with EOPNOTSUPP and then the
> core code allocating a DMA domain should detect EOPNOTSUPP and try
> again with a 0 flags. Is that what you mean by first option?

Yep! this is what I meant. Let me fix the code.

> 
>> Note :
>>   vt-d specific changes is handled in separate series [1].
>>
>> [1] https://lore.kernel.org/linux-iommu/20241011042722.73930-8-baolu.lu@linux.intel.com/
>>
>> @Baolu,
>>   Do you want me to include above patch in this series so that it doesn't break intel driver?
> 
> I think trying to avoid inter-dependencies right now is a good idea,
> so keep the order where 0 flags calls domain_alloc() first and intel
> domain_alloc_user() will always fail with EOPNOTSUPP.
> 
> We can sort this out next cycle when more stuff is merged

Sure. I was worried about breaking intel driver! Hence I had asked this.
With above changes hopefully it doesn't break things for intel driver.

-Vasant

RE: [PATCH v3 00/12] iommu: Domain allocation enhancements

[Tian, Kevin]

> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Monday, October 21, 2024 5:09 PM
> 
> On 10/18/2024 6:20 PM, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 02:01:25PM +0000, Vasant Hegde wrote:
> >> This series adds iommu_paging_domain_alloc_flags() which takes flags to
> >> pass additional details for domain allocation (like domain with PASID
> >> support). Also adjusts core code to handler variuos domain allocations.
> >>
> >> Also updates AMD IOMMU driver domain allocation code. With this by
> >> default it will allocate domain with V2 page table for PASID capable
> >> device and v1 page table for rest of the devices.
> >>
> >>
> >> TODO:
> >> @Jason, @Kevin, @Baolu,
> >>   With this series if driver implements domain_alloc_user() then we
> assume
> >>   IOMMU always supports PASID. If IOMMU doesn't support PASID (ex:
> AMD driver
> >>   not supporting V2 page table) and device is PASID capable, then driver
> will
> >>   return error. Device probe will fail.
> >
> > We shouldn't fail device probe just because someone plugs in a device
> > with a PCI PASID capability into an old CPU..
> >
> >>   I think if driver return error, core layer should retry without
> >>   IOMMU_HWPT_ALLOC_PASID flag.
> >
> > But this makes sense, and it solves the other problem where if the
> > driver doesn't know how to support IOMMU_HWPT_ALLOC_PASID it will
> also
> > fail. Retrying with 0 flags is correct there too.
> >
> >>   Other option is indivisual driver in error path
> >>   of domain_alloc_user allocating non-PASID capable domain.
> >
> > This would turn IOMMU_HWPT_ALLOC_PASID into a hint, where you might
> > not get back a PASID capable domain type. That seems like a slightly
> > worse direction
> >
> >>   I prefer first option. Just wanted to discuss before implementing. Any
> preferred
> >>   option here? OR any other better way to solve this?
> >
> > I think you should fail in the driver with EOPNOTSUPP and then the
> > core code allocating a DMA domain should detect EOPNOTSUPP and try
> > again with a 0 flags. Is that what you mean by first option?
> 
> Yep! this is what I meant. Let me fix the code.
> 

I'm generally OK with this version. With above and others' comments
fixed the next version should be good to go.

Re: [PATCH v3 00/12] iommu: Domain allocation enhancements

[Vasant Hegde]

Kevin,


On 10/22/2024 1:20 PM, Tian, Kevin wrote:
>> From: Vasant Hegde <vasant.hegde@amd.com>
>> Sent: Monday, October 21, 2024 5:09 PM
>>


.../...


>>>
>>> I think you should fail in the driver with EOPNOTSUPP and then the
>>> core code allocating a DMA domain should detect EOPNOTSUPP and try
>>> again with a 0 flags. Is that what you mean by first option?
>>
>> Yep! this is what I meant. Let me fix the code.
>>
> 
> I'm generally OK with this version. With above and others' comments
> fixed the next version should be good to go.

Thanks! I will address review comment and post v4 soon.

-Vasant