[PATCH v2 0/8] iommu: Domain allocation enhancements

[PATCH v2 0/8] 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 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.

@Baolu,
  With this changes (patch 1), to allocate PASID capable device core
  will call domain_alloc_user() interface. Do we need any changes to
  intel driver?

@Jason,
  Do we need any changes to arm-smm-v3 driver?


Thanks everyone who reviewed previous version and provided valuable feedbacks.

This series is based on top of iommu/next and protection domain free fix [1].
[1] https://lore.kernel.org/linux-iommu/0-v1-ad9884ee5f5b+da-amd_iopgtbl_fix_jgg@nvidia.com/

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

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 (3):
  iommu: Refactor __iommu_domain_alloc()
  iommu: Introduce iommu_paging_domain_alloc_flags()
  iommu: Add new flag to explictly request PASID capable domain

Vasant Hegde (5):
  iommu/amd: Separate page table setup from domain allocation
  iommu/amd: Pass page table type as param to pdom_setup_pgtable()
  iommu/amd: Enhance domain_alloc_user() to allocate PASID capable
    domain
  iommu/amd: Add iommu_ops->domain_alloc_paging support
  iommu/amd: Implement global identity domain

 drivers/iommu/amd/amd_iommu.h |   1 +
 drivers/iommu/amd/init.c      |   3 +
 drivers/iommu/amd/iommu.c     | 133 ++++++++++++++++++++++++----------
 drivers/iommu/iommu.c         | 105 ++++++++++++++++++++-------
 include/linux/iommu.h         |  16 +++-
 include/uapi/linux/iommufd.h  |   6 ++
 6 files changed, 194 insertions(+), 70 deletions(-)

-- 
2.31.1

[PATCH v2 1/8] 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>
---
 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 ed6c5cb60c5a..4118d3b15310 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 v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Baolu Lu]

On 9/11/24 6:19 PM, 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.
> 
> 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>
> ---
>   drivers/iommu/iommu.c | 46 +++++++++++++++++++++++--------------------
>   1 file changed, 25 insertions(+), 21 deletions(-)

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

Thanks,
baolu

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jacob Pan]

Hi Vasant,

On Wed, 11 Sep 2024 10:19:04 +0000
Vasant Hegde <vasant.hegde@amd.com> wrote:

> @@ -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;
> +		}
> +	}
> +
nit, if a user changes default domain types via sysfs and the current
domain is already DMA API type, i.e. DMA to DMA-FQ, I think it causes an
extra round of iommu_get/put_dma_cookie() compared with doing this in
allocation time.

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Vasant Hegde]

Hi Jacob,


On 9/13/2024 9:32 AM, Jacob Pan wrote:
> Hi Vasant,
> 
> On Wed, 11 Sep 2024 10:19:04 +0000
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
>> @@ -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;
>> +		}
>> +	}
>> +
> nit, if a user changes default domain types via sysfs and the current
> domain is already DMA API type, i.e. DMA to DMA-FQ, I think it causes an
> extra round of iommu_get/put_dma_cookie() compared with doing this in
> allocation time.

DMA -> DMA_FQ will just initialize flush queue and exit. It will not enter
iommu_setup_default_domain(). In case of DMA_FQ -> DMA it will do new domain
allocation. Am I missing something here?


-Vasant

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jacob Pan]

Hi Vasant,

On Thu, 26 Sep 2024 15:47:48 +0530
Vasant Hegde <vasant.hegde@amd.com> wrote:

> Hi Jacob,
> 
> 
> On 9/13/2024 9:32 AM, Jacob Pan wrote:
> > Hi Vasant,
> > 
> > On Wed, 11 Sep 2024 10:19:04 +0000
> > Vasant Hegde <vasant.hegde@amd.com> wrote:
> >   
> >> @@ -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;
> >> +		}
> >> +	}
> >> +  
> > nit, if a user changes default domain types via sysfs and the
> > current domain is already DMA API type, i.e. DMA to DMA-FQ, I think
> > it causes an extra round of iommu_get/put_dma_cookie() compared
> > with doing this in allocation time.  
> 
> DMA -> DMA_FQ will just initialize flush queue and exit. It will not
> enter iommu_setup_default_domain(). In case of DMA_FQ -> DMA it will
> do new domain allocation. Am I missing something here?
You are right. No extra get/put_dma_cookie there.
My other concern is that this change made domain alloc/free asymmetrical
in terms of get/put_dma_cookie.

Currently, we have ftrace like this when I do:
echo DMA > /sys/bus/pci/devices/0000\:38\:00.0/iommu_group/type

    iommu_setup_default_domain() {
      __iommu_domain_alloc() {
        intel_iommu_domain_alloc();
        iommu_get_dma_cookie();
      }
      iommu_domain_free() {
         iommu_put_dma_cookie();
        intel_iommu_domain_free();
       }

With this change, it will be:
    iommu_setup_default_domain() {
        iommu_get_dma_cookie();
	__iommu_domain_alloc() {
        intel_iommu_domain_alloc();
      }
      iommu_domain_free() {
         iommu_put_dma_cookie();
        intel_iommu_domain_free();
       }

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Vasant Hegde]

Hi Jacob,

On 9/30/2024 11:25 PM, Jacob Pan wrote:
> Hi Vasant,
> 
> On Thu, 26 Sep 2024 15:47:48 +0530
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
>> Hi Jacob,
>>
>>
>> On 9/13/2024 9:32 AM, Jacob Pan wrote:
>>> Hi Vasant,
>>>
>>> On Wed, 11 Sep 2024 10:19:04 +0000
>>> Vasant Hegde <vasant.hegde@amd.com> wrote:
>>>   
>>>> @@ -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;
>>>> +		}
>>>> +	}
>>>> +  
>>> nit, if a user changes default domain types via sysfs and the
>>> current domain is already DMA API type, i.e. DMA to DMA-FQ, I think
>>> it causes an extra round of iommu_get/put_dma_cookie() compared
>>> with doing this in allocation time.  
>>
>> DMA -> DMA_FQ will just initialize flush queue and exit. It will not
>> enter iommu_setup_default_domain(). In case of DMA_FQ -> DMA it will
>> do new domain allocation. Am I missing something here?
> You are right. No extra get/put_dma_cookie there.
> My other concern is that this change made domain alloc/free asymmetrical
> in terms of get/put_dma_cookie.

That's correct. And this should be fine right? These are internal functions.

-Vasant

> 
> Currently, we have ftrace like this when I do:
> echo DMA > /sys/bus/pci/devices/0000\:38\:00.0/iommu_group/type
> 
>     iommu_setup_default_domain() {
>       __iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>         iommu_get_dma_cookie();
>       }
>       iommu_domain_free() {
>          iommu_put_dma_cookie();
>         intel_iommu_domain_free();
>        }
> 
> With this change, it will be:
>     iommu_setup_default_domain() {
>         iommu_get_dma_cookie();
> 	__iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>       }
>       iommu_domain_free() {
>          iommu_put_dma_cookie();
>         intel_iommu_domain_free();
>        }
> 

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jacob Pan]

Hi Vasant,

On Tue, 1 Oct 2024 10:01:31 +0530
Vasant Hegde <vasant.hegde@amd.com> wrote:

> Hi Jacob,
> 
> On 9/30/2024 11:25 PM, Jacob Pan wrote:
> > Hi Vasant,
> > 
> > On Thu, 26 Sep 2024 15:47:48 +0530
> > Vasant Hegde <vasant.hegde@amd.com> wrote:
> >   
> >> Hi Jacob,
> >>
> >>
> >> On 9/13/2024 9:32 AM, Jacob Pan wrote:  
> >>> Hi Vasant,
> >>>
> >>> On Wed, 11 Sep 2024 10:19:04 +0000
> >>> Vasant Hegde <vasant.hegde@amd.com> wrote:
> >>>     
> >>>> @@ -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;
> >>>> +		}
> >>>> +	}
> >>>> +    
> >>> nit, if a user changes default domain types via sysfs and the
> >>> current domain is already DMA API type, i.e. DMA to DMA-FQ, I
> >>> think it causes an extra round of iommu_get/put_dma_cookie()
> >>> compared with doing this in allocation time.    
> >>
> >> DMA -> DMA_FQ will just initialize flush queue and exit. It will
> >> not enter iommu_setup_default_domain(). In case of DMA_FQ -> DMA
> >> it will do new domain allocation. Am I missing something here?  
> > You are right. No extra get/put_dma_cookie there.
> > My other concern is that this change made domain alloc/free
> > asymmetrical in terms of get/put_dma_cookie.  
> 
> That's correct. And this should be fine right? These are internal
> functions.
> 

There are no functional issues. I don't have strong opinion other than
maintaining symmetry reduces the likelihood of errors.

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jason Gunthorpe]

On Mon, Sep 30, 2024 at 10:55:23AM -0700, Jacob Pan wrote:
> Currently, we have ftrace like this when I do:
> echo DMA > /sys/bus/pci/devices/0000\:38\:00.0/iommu_group/type
> 
>     iommu_setup_default_domain() {
>       __iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>         iommu_get_dma_cookie();
>       }
>       iommu_domain_free() {
>          iommu_put_dma_cookie();
>         intel_iommu_domain_free();
>        }
> 
> With this change, it will be:
>     iommu_setup_default_domain() {
>         iommu_get_dma_cookie();
> 	__iommu_domain_alloc() {
>         intel_iommu_domain_alloc();
>       }

?

iommu_get_dma_cookie() accepts a domain pointer, how can it be after
intel_iommu_domain_alloc() ? That would be a bug, right?

Jason

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jacob Pan]

Hi Jason,

On Wed, 2 Oct 2024 11:19:51 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Mon, Sep 30, 2024 at 10:55:23AM -0700, Jacob Pan wrote:
> > Currently, we have ftrace like this when I do:
> > echo DMA > /sys/bus/pci/devices/0000\:38\:00.0/iommu_group/type
> > 
> >     iommu_setup_default_domain() {
> >       __iommu_domain_alloc() {
> >         intel_iommu_domain_alloc();
> >         iommu_get_dma_cookie();
> >       }
> >       iommu_domain_free() {
> >          iommu_put_dma_cookie();
> >         intel_iommu_domain_free();
> >        }
> > 
> > With this change, it will be:
> >     iommu_setup_default_domain() {
> >         iommu_get_dma_cookie();
> > 	__iommu_domain_alloc() {
> >         intel_iommu_domain_alloc();
> >       }  
> 
> ?
> 
> iommu_get_dma_cookie() accepts a domain pointer, how can it be after
> intel_iommu_domain_alloc() ? That would be a bug, right?
My mistake, iommu_get_dma_cookie() is *after* __iommu_domain_alloc()
with this change. No issues there. Sorry about the confusion. I was
trying to show the asymmetrical part but missed the order.

Re: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:04AM +0000, 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.
> 
> 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>
> ---
>  drivers/iommu/iommu.c | 46 +++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 21 deletions(-)

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

Jason

RE: [PATCH v2 1/8] iommu: Refactor __iommu_domain_alloc()

[Tian, Kevin]

> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Wednesday, September 11, 2024 6:19 PM
> 
> 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()

s/setup/to/

> as
> its required in DMA API mode only.

s/its/it's/

> 
> 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: Kevin Tian <kevin.tian@intel.com>

[PATCH v2 2/8] 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>
---
 drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++----
 include/linux/iommu.h | 16 +++++++++++++---
 2 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4118d3b15310..5d1cef36f633 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..907957a88470 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -511,8 +511,8 @@ 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.
+ *                     Unlike @domain_alloc, it 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 +789,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 +1100,12 @@ static inline struct iommu_domain *iommu_domain_alloc(const struct bus_type *bus
 	return NULL;
 }
 
+struct inline 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 v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Baolu Lu]

On 9/11/24 6:19 PM, Vasant Hegde wrote:
>   /**
> - * 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);

How about

	if (flags) {
		if (!ops->domain_alloc_user)
			return ERR_PTR(-EOPNOTSUPP);
		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);
	}

?

> +
> +	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);

Thanks,
baolu

Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Vasant Hegde]

Hi Baolu,

On 9/12/2024 9:34 AM, Baolu Lu wrote:
> On 9/11/24 6:19 PM, Vasant Hegde wrote:
>>   /**
>> - * 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);
> 
> How about
> 
>     if (flags) {
>         if (!ops->domain_alloc_user)
>             return ERR_PTR(-EOPNOTSUPP);
>         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);
>     }
> 
> ?

Sure. This is bit more explicit. I will fix it.

-Vasant

RE: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, September 12, 2024 12:05 PM
> 
> On 9/11/24 6:19 PM, Vasant Hegde wrote:
> >   /**
> > - * 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);
> 
> How about
> 
> 	if (flags) {
> 		if (!ops->domain_alloc_user)
> 			return ERR_PTR(-EOPNOTSUPP);
> 		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);
> 	}
> 

There is slight semantics difference.

Original code picks domain_alloc_user() if !domain_alloc_paging
and !flags.

but your code will pick domain_alloc().

Based on discussions in this thread domain_alloc_user/ext is the
preferred one. Then below is the right order?

	If (ops->domain_alloc_user) {
		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
	} else if (flags) {
		return ERR_PTR(-EOPNOTSUPP);
	} 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);
 	}

Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 08:24:44AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Thursday, September 12, 2024 12:05 PM
> > 
> > On 9/11/24 6:19 PM, Vasant Hegde wrote:
> > >   /**
> > > - * 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);
> > 
> > How about
> > 
> > 	if (flags) {
> > 		if (!ops->domain_alloc_user)
> > 			return ERR_PTR(-EOPNOTSUPP);
> > 		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);
> > 	}
> > 
> 
> There is slight semantics difference.
> 
> Original code picks domain_alloc_user() if !domain_alloc_paging
> and !flags.
> 
> but your code will pick domain_alloc().
> 
> Based on discussions in this thread domain_alloc_user/ext is the
> preferred one. Then below is the right order?
> 
> 	If (ops->domain_alloc_user) {
> 		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> 	} else if (flags) {
> 		return ERR_PTR(-EOPNOTSUPP);
> 	} 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);
>  	}

This can work too, but I think we need a few more patches still to get
there?

The order above makes sense if drivers implement both
ops->domain_alloc_paging/user, then you'd want to prefer the more
narrow one.

Once drivers don't implement both then it would make sense to switch
to your version

Jason

RE: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, October 15, 2024 8:32 PM
> 
> On Tue, Oct 15, 2024 at 08:24:44AM +0000, Tian, Kevin wrote:
> > > From: Baolu Lu <baolu.lu@linux.intel.com>
> > > Sent: Thursday, September 12, 2024 12:05 PM
> > >
> > > On 9/11/24 6:19 PM, Vasant Hegde wrote:
> > > >   /**
> > > > - * 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);
> > >
> > > How about
> > >
> > > 	if (flags) {
> > > 		if (!ops->domain_alloc_user)
> > > 			return ERR_PTR(-EOPNOTSUPP);
> > > 		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);
> > > 	}
> > >
> >
> > There is slight semantics difference.
> >
> > Original code picks domain_alloc_user() if !domain_alloc_paging
> > and !flags.
> >
> > but your code will pick domain_alloc().
> >
> > Based on discussions in this thread domain_alloc_user/ext is the
> > preferred one. Then below is the right order?
> >
> > 	If (ops->domain_alloc_user) {
> > 		domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> > 	} else if (flags) {
> > 		return ERR_PTR(-EOPNOTSUPP);
> > 	} 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);
> >  	}
> 
> This can work too, but I think we need a few more patches still to get
> there?
> 
> The order above makes sense if drivers implement both
> ops->domain_alloc_paging/user, then you'd want to prefer the more
> narrow one.
> 
> Once drivers don't implement both then it would make sense to switch
> to your version
> 

Okay, that makes sense.

Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:05AM +0000, 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>
> ---
>  drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++----
>  include/linux/iommu.h | 16 +++++++++++++---
>  2 files changed, 39 insertions(+), 7 deletions(-)

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

Jason

Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Jacob Pan]

Hi Vasant,

On Wed, 11 Sep 2024 10:19:05 +0000
Vasant Hegde <vasant.hegde@amd.com> wrote:

> -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);
Since this can be outside iommufd, the following comment needs update.

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9842c6a797dc..6aeb6d9c985f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -517,7 +517,7 @@ 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
+ *                     Unlike @domain_alloc, it
  *                     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

> +	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);

Re: [PATCH v2 2/8] iommu: Introduce iommu_paging_domain_alloc_flags()

[Vasant Hegde]

Hi Jacob,


On 10/10/2024 2:44 AM, Jacob Pan wrote:
> Hi Vasant,
> 
> On Wed, 11 Sep 2024 10:19:05 +0000
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
>> -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);
> Since this can be outside iommufd, the following comment needs update.

ACK. I will update.

-Vasant

[PATCH v2 3/8] 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>
---
 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 5d1cef36f633..f3f1244fe90a 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 4dde745cfb7e..4afcd3e2eac4 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 v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Baolu Lu]

On 9/11/24 6:19 PM, 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>
> ---
>   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 5d1cef36f633..f3f1244fe90a 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);

This changes default domain allocation paths.

Previously, the default domain was allocated by domain_alloc_paging or
domain_alloc. Now, it will be allocated through domain_alloc_user,
which, based on my understanding, is intended for user domain
allocation.

Thanks,
baolu

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

Hi Baolu,


On 9/12/2024 9:44 AM, Baolu Lu wrote:
> On 9/11/24 6:19 PM, 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>
>> ---
>>   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 5d1cef36f633..f3f1244fe90a 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);
> 
> This changes default domain allocation paths.
> 
> Previously, the default domain was allocated by domain_alloc_paging or
> domain_alloc. Now, it will be allocated through domain_alloc_user,
> which, based on my understanding, is intended for user domain
> allocation.

Right. This was discussed in during v1 [1] and Jason suggested to rename this
API to domain_alloc_paging_extended() after everything settles.

That's why in cover letter I asked you and Jason whether we need any changes to
intel and ARM SMM3 driver.

-Vasant

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

On 9/26/2024 3:59 PM, Vasant Hegde wrote:
> Hi Baolu,
> 
> 
> On 9/12/2024 9:44 AM, Baolu Lu wrote:
>> On 9/11/24 6:19 PM, 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>
>>> ---
>>>   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 5d1cef36f633..f3f1244fe90a 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);
>>
>> This changes default domain allocation paths.
>>
>> Previously, the default domain was allocated by domain_alloc_paging or
>> domain_alloc. Now, it will be allocated through domain_alloc_user,
>> which, based on my understanding, is intended for user domain
>> allocation.
> 
> Right. This was discussed in during v1 [1] and Jason suggested to rename this
> API to domain_alloc_paging_extended() after everything settles.

Sorry. Missed to add the link

[1] https://lore.kernel.org/linux-iommu/20240821163147.GZ3468552@ziepe.ca/

-Vasant

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

On Thu, Sep 26, 2024 at 04:31:52PM +0530, Vasant Hegde wrote:
> >> Previously, the default domain was allocated by domain_alloc_paging or
> >> domain_alloc. Now, it will be allocated through domain_alloc_user,
> >> which, based on my understanding, is intended for user domain
> >> allocation.
> > 
> > Right. This was discussed in during v1 [1] and Jason suggested to rename this
> > API to domain_alloc_paging_extended() after everything settles.
> 
> Sorry. Missed to add the link
> 
> [1] https://lore.kernel.org/linux-iommu/20240821163147.GZ3468552@ziepe.ca/

Yeah, I think this is the right way to go. We just misnamed the _user
op, lets correct it.

We end up with a simple op that most drivers use and then the more
complex op that iommufd capable drivers will implement.

We work to merge things so drivers only implement one of the two ops.

Jason

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jacob Pan]

Hi Jason,

On Wed, 2 Oct 2024 11:23:31 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Thu, Sep 26, 2024 at 04:31:52PM +0530, Vasant Hegde wrote:
> > >> Previously, the default domain was allocated by
> > >> domain_alloc_paging or domain_alloc. Now, it will be allocated
> > >> through domain_alloc_user, which, based on my understanding, is
> > >> intended for user domain allocation.  
> > > 
> > > Right. This was discussed in during v1 [1] and Jason suggested to
> > > rename this API to domain_alloc_paging_extended() after
> > > everything settles.  
> > 
> > Sorry. Missed to add the link
> > 
> > [1]
> > https://lore.kernel.org/linux-iommu/20240821163147.GZ3468552@ziepe.ca/
> >
Slightly off topic, in [1], you mentioned the following flow for
iommu_group_alloc_default_domain()
{
...
	if (req_type || iommu_def_domain_type == IOMMU_DOMAIN_IDENTITY)
{ dom = iommu_group_alloc_identity_domain();
		if (req_type || !IS_ERR(dom))
			return dom;
		/*
		 * if iommu_def_domain_type == IDENTITY fails then fall
through
		 * to PAGING
		 */
	}

If we were to have a vIOMMU that only supports "identity" and "sva"
domains, there would be no need to fall back to PAGING. I don't see any
issues if we take care of vendor driver iommu_ops accordingly. But
wanted to see if I missed anything.
 

> Yeah, I think this is the right way to go. We just misnamed the _user
> op, lets correct it.
> 
> We end up with a simple op that most drivers use and then the more
> complex op that iommufd capable drivers will implement.
> 
> We work to merge things so drivers only implement one of the two ops.
> 
> Jason

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

> Slightly off topic, in [1], you mentioned the following flow for
> iommu_group_alloc_default_domain()
> {
> ...
> 	if (req_type || iommu_def_domain_type == IOMMU_DOMAIN_IDENTITY)
> { dom = iommu_group_alloc_identity_domain();
> 		if (req_type || !IS_ERR(dom))
> 			return dom;
> 		/*
> 		 * if iommu_def_domain_type == IDENTITY fails then fall
> through
> 		 * to PAGING
> 		 */
> 	}
> 
> If we were to have a vIOMMU that only supports "identity" and "sva"
> domains, there would be no need to fall back to PAGING.

That's illegal. If you support SVA the driver can and should support
PAGING. I don't want to see SVA being special.

There must always be a DMA API domain of some kind, if identity
doesn't work then paging must, otherwise the device is unusable.

Jason

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jacob Pan]

Hi Jason,

On Wed, 2 Oct 2024 16:07:05 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> > Slightly off topic, in [1], you mentioned the following flow for
> > iommu_group_alloc_default_domain()
> > {
> > ...
> > 	if (req_type || iommu_def_domain_type ==
> > IOMMU_DOMAIN_IDENTITY) { dom = iommu_group_alloc_identity_domain();
> > 		if (req_type || !IS_ERR(dom))
> > 			return dom;
> > 		/*
> > 		 * if iommu_def_domain_type == IDENTITY fails then
> > fall through
> > 		 * to PAGING
> > 		 */
> > 	}
> > 
> > If we were to have a vIOMMU that only supports "identity" and "sva"
> > domains, there would be no need to fall back to PAGING.  
> 
> That's illegal. If you support SVA the driver can and should support
> PAGING. I don't want to see SVA being special.
> 
> There must always be a DMA API domain of some kind, if identity
> doesn't work then paging must, otherwise the device is unusable.
> 

I also think that the PAGING domain is an integral part of any IOMMU
driver and must be included. However, from a use case point of view, we
can already have a configuration where, under a single IOMMU, some
devices use SVA while others (e.g., trusted devices) use the identity
domain.

My question is more about the incremental development of the pvIOMMU
guest driver. Since the PAGING domain requires more infrastructure than
SVA (e.g. may leverage your Generic Page Table work for constructing
S1; please correct me if that is not the intended usage), would it make
sense to add the PAGING domain after SVA and identity domain support?"

Thanks,

Jacob

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:06AM +0000, Vasant Hegde wrote:
> +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);

Can we get rid of the type parameter here? Like this:

       if (req_type & __IOMMU_DOMAIN_PAGING) {
               dom =  __iommu_paging_domain_alloc_flags(dev,
                               dev->iommu->max_pasids ?
			       IOMMU_HWPT_ALLOC_PASID : 0);
		if (IS_ERR(dom))
		   return dom;
		dom->type = req_type;
       }

Maybe at the end of the series, I guess you want AMD to not have a
bisection issue?

I think this looks OK otherwise

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

Jason

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Vasant Hegde]

Jason,


On 10/3/2024 12:53 AM, Jason Gunthorpe wrote:
> On Wed, Sep 11, 2024 at 10:19:06AM +0000, Vasant Hegde wrote:
>> +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);
> 
> Can we get rid of the type parameter here? Like this:

This will create original problem for AMD driver. i. e. for VFIO we want to
allocate V1 page table (at least for now).


> 
>        if (req_type & __IOMMU_DOMAIN_PAGING) {
>                dom =  __iommu_paging_domain_alloc_flags(dev,
>                                dev->iommu->max_pasids ?
> 			       IOMMU_HWPT_ALLOC_PASID : 0);
> 		if (IS_ERR(dom))
> 		   return dom;
> 		dom->type = req_type;
>        }
> 
> Maybe at the end of the series, I guess you want AMD to not have a
> bisection issue?
> 
> I think this looks OK otherwise
> 
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks

-Vasant

Re: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Jason Gunthorpe]

On Fri, Oct 04, 2024 at 01:42:24PM +0530, Vasant Hegde wrote:

> >> +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);
> > 
> > Can we get rid of the type parameter here? Like this:
> 
> This will create original problem for AMD driver. i. e. for VFIO we want to
> allocate V1 page table (at least for now).

At this point yes, but at the end of the series no, as the page table
type decision should only be based on IOMMU_HWPT_ALLOC_PASID

And this is why I wonder if it really matters for bisection purposes
as both options will be functionally fine.

Jason

RE: [PATCH v2 3/8] iommu: Add new flag to explictly request PASID capable domain

[Tian, Kevin]

> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Wednesday, September 11, 2024 6:19 PM
> 
> @@ -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.
>   */

The last part is not very clear. Probably you meant that even if a domain
is intended to attach to RID first, it should set the flag if it may be attached
to PASID later?

Otherwise,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v2 4/8] 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>
---
 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 2d0a681a2052..6844040b3702 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2264,28 +2264,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;
@@ -2297,7 +2305,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) {
@@ -2308,20 +2316,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)
@@ -2344,6 +2346,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);
@@ -2363,6 +2366,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

Re: [PATCH v2 4/8] iommu/amd: Separate page table setup from domain allocation

[Jacob Pan]

Hi Vasant,

On Wed, 11 Sep 2024 10:19:07 +0000
Vasant Hegde <vasant.hegde@amd.com> wrote:

> 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>
> ---
>  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 2d0a681a2052..6844040b3702 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2264,28 +2264,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;
> @@ -2297,7 +2305,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) {
> @@ -2308,20 +2316,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)
> @@ -2344,6 +2346,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);
> @@ -2363,6 +2366,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;

Reviewed-by: Jacob Pan <jacob.pan@linux.microsoft.com>

Re: [PATCH v2 4/8] iommu/amd: Separate page table setup from domain allocation

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:07AM +0000, Vasant Hegde wrote:
> 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>
> ---
>  drivers/iommu/amd/iommu.c | 42 ++++++++++++++++++++++++---------------
>  1 file changed, 26 insertions(+), 16 deletions(-)

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

Jason

[PATCH v2 5/8] 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>
---
 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 6844040b3702..cfeb3202ceee 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2285,28 +2285,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:
@@ -2318,6 +2303,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)
@@ -2326,9 +2312,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 */
@@ -2341,7 +2327,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;
@@ -2366,7 +2353,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);
@@ -2374,7 +2361,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;
 
@@ -2392,8 +2379,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;
 
@@ -2411,7 +2406,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 v2 5/8] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Jacob Pan]

Hi Vasant,

On Wed, 11 Sep 2024 10:19:08 +0000
Vasant Hegde <vasant.hegde@amd.com> wrote:

> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2285,28 +2285,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;
Now that you pass in the pgtable format explicitly, I think you can
delete 'type' parameter and move this check to do_iommu_domain_alloc().
As you mentioned in [4/8], pdom_setup_pgtable() is factored out for
setting up page table only.

Other than that,
Reviewed-by: Jacob Pan <jacob.pan@linux.microsoft.com>

Re: [PATCH v2 5/8] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Vasant Hegde]

Jacob,


On 9/14/2024 3:09 AM, Jacob Pan wrote:
> Hi Vasant,
> 
> On Wed, 11 Sep 2024 10:19:08 +0000
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2285,28 +2285,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;
> Now that you pass in the pgtable format explicitly, I think you can
> delete 'type' parameter and move this check to do_iommu_domain_alloc().
> As you mentioned in [4/8], pdom_setup_pgtable() is factored out for
> setting up page table only.

I could have added check in do_iommu_domain_alloc(). As just kept it as is
because eventually we can just remove this check. (Once I fix SVA domain and
after having global static identity domain).

-Vasant

Re: [PATCH v2 5/8] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Jacob Pan]

Hi Vasant,

On Thu, 26 Sep 2024 15:55:14 +0530
Vasant Hegde <vasant.hegde@amd.com> wrote:

> Jacob,
> 
> 
> On 9/14/2024 3:09 AM, Jacob Pan wrote:
> > Hi Vasant,
> > 
> > On Wed, 11 Sep 2024 10:19:08 +0000
> > Vasant Hegde <vasant.hegde@amd.com> wrote:
> >   
> >> --- a/drivers/iommu/amd/iommu.c
> >> +++ b/drivers/iommu/amd/iommu.c
> >> @@ -2285,28 +2285,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;  
> > Now that you pass in the pgtable format explicitly, I think you can
> > delete 'type' parameter and move this check to
> > do_iommu_domain_alloc(). As you mentioned in [4/8],
> > pdom_setup_pgtable() is factored out for setting up page table
> > only.  
> 
> I could have added check in do_iommu_domain_alloc(). As just kept it
> as is because eventually we can just remove this check. (Once I fix
> SVA domain and after having global static identity domain).
> 
sounds reasonable me.

Thanks,

Jacob

Re: [PATCH v2 5/8] iommu/amd: Pass page table type as param to pdom_setup_pgtable()

[Jason Gunthorpe]

On Fri, Sep 13, 2024 at 02:39:45PM -0700, Jacob Pan wrote:
> Hi Vasant,
> 
> On Wed, 11 Sep 2024 10:19:08 +0000
> Vasant Hegde <vasant.hegde@amd.com> wrote:
> 
> > --- a/drivers/iommu/amd/iommu.c
> > +++ b/drivers/iommu/amd/iommu.c
> > @@ -2285,28 +2285,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;
> Now that you pass in the pgtable format explicitly, I think you can
> delete 'type' parameter and move this check to do_iommu_domain_alloc().
> As you mentioned in [4/8], pdom_setup_pgtable() is factored out for
> setting up page table only.

It would be nice but I think the type is stil encoding the DMA API
flag at this point. Adjusting that could be moved to where the cookie
code ended up

Jason

[PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Allocate domain with v2 page table if IOMMU_HWPT_ALLOC_PASID flag is
passed to ops->domain_alloc_user().

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index cfeb3202ceee..1c9e539f15a7 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2401,12 +2401,23 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 			    const struct iommu_user_data *user_data)
 
 {
+	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
 	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
+	int pgtable = AMD_IOMMU_V1;
 
 	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
 		return ERR_PTR(-EOPNOTSUPP);
 
-	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
+	/* Allocate v2 page table if IOMMU and device supports PASID. */
+	if (flags & IOMMU_HWPT_ALLOC_PASID) {
+		if (!amd_iommu_pasid_supported() ||
+		    !pdev_pasid_supported(dev_data))
+			return ERR_PTR(-EINVAL);
+
+		pgtable = AMD_IOMMU_V2;
+	}
+
+	return do_iommu_domain_alloc(type, dev, flags, pgtable);
 }
 
 void amd_iommu_domain_free(struct iommu_domain *dom)
-- 
2.31.1

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:09AM +0000, Vasant Hegde wrote:
> Allocate domain with v2 page table if IOMMU_HWPT_ALLOC_PASID flag is
> passed to ops->domain_alloc_user().
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index cfeb3202ceee..1c9e539f15a7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2401,12 +2401,23 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>  			    const struct iommu_user_data *user_data)
>  
>  {
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
> +	int pgtable = AMD_IOMMU_V1;
>  
>  	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
>  		return ERR_PTR(-EOPNOTSUPP);
>  
> -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> +		if (!amd_iommu_pasid_supported() ||
> +		    !pdev_pasid_supported(dev_data))

The test to disable v2 support uses:

	if (!check_feature(FEATURE_GIOSUP) ||
	    !check_feature(FEATURE_GT)) {

But this doesn't check FEATURE_GIOSUP, just GT? Is it OK?

Jason

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Jason,


On 10/3/2024 1:01 AM, Jason Gunthorpe wrote:
> On Wed, Sep 11, 2024 at 10:19:09AM +0000, Vasant Hegde wrote:
>> Allocate domain with v2 page table if IOMMU_HWPT_ALLOC_PASID flag is
>> passed to ops->domain_alloc_user().
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index cfeb3202ceee..1c9e539f15a7 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2401,12 +2401,23 @@ amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>>  			    const struct iommu_user_data *user_data)
>>  
>>  {
>> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
>> +	int pgtable = AMD_IOMMU_V1;
>>  
>>  	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent || user_data)
>>  		return ERR_PTR(-EOPNOTSUPP);
>>  
>> -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
>> +	/* Allocate v2 page table if IOMMU and device supports PASID. */
>> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
>> +		if (!amd_iommu_pasid_supported() ||
>> +		    !pdev_pasid_supported(dev_data))
> 
> The test to disable v2 support uses:
> 
> 	if (!check_feature(FEATURE_GIOSUP) ||
> 	    !check_feature(FEATURE_GT)) {
> 
> But this doesn't check FEATURE_GIOSUP, just GT? Is it OK?

Ah! You are right. To support V2 page table, yes. Will fix it.

-Vasant

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Jason Gunthorpe]

On Fri, Oct 04, 2024 at 01:48:16PM +0530, Vasant Hegde wrote:
> >> -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> >> +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> >> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> >> +		if (!amd_iommu_pasid_supported() ||
> >> +		    !pdev_pasid_supported(dev_data))
> > 
> > The test to disable v2 support uses:
> > 
> > 	if (!check_feature(FEATURE_GIOSUP) ||
> > 	    !check_feature(FEATURE_GT)) {
> > 
> > But this doesn't check FEATURE_GIOSUP, just GT? Is it OK?
> 
> Ah! You are right. To support V2 page table, yes. Will fix it.

I think you should eventually define more useful feature flags:

FEATURE_AMD_PGTBL_V1
FEATURE_AMD_PGTBL_V2
FEATURE_AMD_PASID

etc

GIOSUP is too narrow

Then all the figuring out can be done once in the init code. There is
also some CC stuff changing this as well :\

Jason

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Jason,


On 10/4/2024 6:18 PM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2024 at 01:48:16PM +0530, Vasant Hegde wrote:
>>>> -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
>>>> +	/* Allocate v2 page table if IOMMU and device supports PASID. */
>>>> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
>>>> +		if (!amd_iommu_pasid_supported() ||
>>>> +		    !pdev_pasid_supported(dev_data))
>>>
>>> The test to disable v2 support uses:
>>>
>>> 	if (!check_feature(FEATURE_GIOSUP) ||
>>> 	    !check_feature(FEATURE_GT)) {
>>>
>>> But this doesn't check FEATURE_GIOSUP, just GT? Is it OK?
>>
>> Ah! You are right. To support V2 page table, yes. Will fix it.
> 
> I think you should eventually define more useful feature flags:
> 
> FEATURE_AMD_PGTBL_V1
> FEATURE_AMD_PGTBL_V2
> FEATURE_AMD_PASID
> 
> etc
> 
> GIOSUP is too narrow
> 
> Then all the figuring out can be done once in the init code. There is
> also some CC stuff changing this as well :\

Right. I have few things to improve in page select stuff. I will try to cover
these things as well.

-Vasant

RE: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Tian, Kevin]

> From: Vasant Hegde <vasant.hegde@amd.com>
> Sent: Wednesday, September 11, 2024 6:19 PM
> 
> Allocate domain with v2 page table if IOMMU_HWPT_ALLOC_PASID flag is
> passed to ops->domain_alloc_user().
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index cfeb3202ceee..1c9e539f15a7 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2401,12 +2401,23 @@ amd_iommu_domain_alloc_user(struct device
> *dev, u32 flags,
>  			    const struct iommu_user_data *user_data)
> 
>  {
> +	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>  	unsigned int type = IOMMU_DOMAIN_UNMANAGED;
> +	int pgtable = AMD_IOMMU_V1;
> 
>  	if ((flags & ~IOMMU_HWPT_ALLOC_DIRTY_TRACKING) || parent ||
> user_data)
>  		return ERR_PTR(-EOPNOTSUPP);

Hmm is this series actually tested? Above check rejects the new
PASID flag...

> 
> -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> +		if (!amd_iommu_pasid_supported() ||
> +		    !pdev_pasid_supported(dev_data))
> +			return ERR_PTR(-EINVAL);

Just check the iommu capability here? IMHO it's a sane case to first
create a PASID-compatible domain for a device which doesn't support
PASID and then attach the domain to a PASID-capable device later.

IIUC the v2 format can be attached to any RID.

> +
> +		pgtable = AMD_IOMMU_V2;
> +	}
> +
> +	return do_iommu_domain_alloc(type, dev, flags, pgtable);
>  }
> 
>  void amd_iommu_domain_free(struct iommu_domain *dom)
> --
> 2.31.1

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Jason Gunthorpe]

On Tue, Oct 15, 2024 at 08:41:48AM +0000, Tian, Kevin wrote:

> > -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> > +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> > +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> > +		if (!amd_iommu_pasid_supported() ||
> > +		    !pdev_pasid_supported(dev_data))
> > +			return ERR_PTR(-EINVAL);
> 
> Just check the iommu capability here? IMHO it's a sane case to first
> create a PASID-compatible domain for a device which doesn't support
> PASID and then attach the domain to a PASID-capable device later.

The point of this series is to allow AMD to select the v1 format as
often as possible because v1 has better performance than v2

Jason

RE: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Tuesday, October 15, 2024 8:40 PM
> 
> On Tue, Oct 15, 2024 at 08:41:48AM +0000, Tian, Kevin wrote:
> 
> > > -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> > > +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> > > +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> > > +		if (!amd_iommu_pasid_supported() ||
> > > +		    !pdev_pasid_supported(dev_data))
> > > +			return ERR_PTR(-EINVAL);
> >
> > Just check the iommu capability here? IMHO it's a sane case to first
> > create a PASID-compatible domain for a device which doesn't support
> > PASID and then attach the domain to a PASID-capable device later.
> 
> The point of this series is to allow AMD to select the v1 format as
> often as possible because v1 has better performance than v2
> 

If the user wants better performance then it should not set the flag.

Not sure the kernel driver should enforce a policy preferring 
performance over an user opt which is compatible to any RID...

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Jason Gunthorpe]

On Wed, Oct 16, 2024 at 02:48:04AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Tuesday, October 15, 2024 8:40 PM
> > 
> > On Tue, Oct 15, 2024 at 08:41:48AM +0000, Tian, Kevin wrote:
> > 
> > > > -	return do_iommu_domain_alloc(type, dev, flags, AMD_IOMMU_V1);
> > > > +	/* Allocate v2 page table if IOMMU and device supports PASID. */
> > > > +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> > > > +		if (!amd_iommu_pasid_supported() ||
> > > > +		    !pdev_pasid_supported(dev_data))
> > > > +			return ERR_PTR(-EINVAL);
> > >
> > > Just check the iommu capability here? IMHO it's a sane case to first
> > > create a PASID-compatible domain for a device which doesn't support
> > > PASID and then attach the domain to a PASID-capable device later.
> > 
> > The point of this series is to allow AMD to select the v1 format as
> > often as possible because v1 has better performance than v2
>
> If the user wants better performance then it should not set the flag.

Well, I do tend to agree with that, you mean remove the
pdev_pasid_supported() ?

Jason

RE: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, October 16, 2024 11:29 PM
> 
> On Wed, Oct 16, 2024 at 02:48:04AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > Sent: Tuesday, October 15, 2024 8:40 PM
> > >
> > > On Tue, Oct 15, 2024 at 08:41:48AM +0000, Tian, Kevin wrote:
> > >
> > > > > -	return do_iommu_domain_alloc(type, dev, flags,
> AMD_IOMMU_V1);
> > > > > +	/* Allocate v2 page table if IOMMU and device supports
> PASID. */
> > > > > +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
> > > > > +		if (!amd_iommu_pasid_supported() ||
> > > > > +		    !pdev_pasid_supported(dev_data))
> > > > > +			return ERR_PTR(-EINVAL);
> > > >
> > > > Just check the iommu capability here? IMHO it's a sane case to first
> > > > create a PASID-compatible domain for a device which doesn't support
> > > > PASID and then attach the domain to a PASID-capable device later.
> > >
> > > The point of this series is to allow AMD to select the v1 format as
> > > often as possible because v1 has better performance than v2
> >
> > If the user wants better performance then it should not set the flag.
> 
> Well, I do tend to agree with that, you mean remove the
> pdev_pasid_supported() ?
> 

yes

Re: [PATCH v2 6/8] iommu/amd: Enhance domain_alloc_user() to allocate PASID capable domain

[Vasant Hegde]

Hi Kevin,


On 10/17/2024 11:41 AM, Tian, Kevin wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>> Sent: Wednesday, October 16, 2024 11:29 PM
>>
>> On Wed, Oct 16, 2024 at 02:48:04AM +0000, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>>> Sent: Tuesday, October 15, 2024 8:40 PM
>>>>
>>>> On Tue, Oct 15, 2024 at 08:41:48AM +0000, Tian, Kevin wrote:
>>>>
>>>>>> -	return do_iommu_domain_alloc(type, dev, flags,
>> AMD_IOMMU_V1);
>>>>>> +	/* Allocate v2 page table if IOMMU and device supports
>> PASID. */
>>>>>> +	if (flags & IOMMU_HWPT_ALLOC_PASID) {
>>>>>> +		if (!amd_iommu_pasid_supported() ||
>>>>>> +		    !pdev_pasid_supported(dev_data))
>>>>>> +			return ERR_PTR(-EINVAL);
>>>>>
>>>>> Just check the iommu capability here? IMHO it's a sane case to first
>>>>> create a PASID-compatible domain for a device which doesn't support
>>>>> PASID and then attach the domain to a PASID-capable device later.
>>>>
>>>> The point of this series is to allow AMD to select the v1 format as
>>>> often as possible because v1 has better performance than v2
>>>
>>> If the user wants better performance then it should not set the flag.
>>
>> Well, I do tend to agree with that, you mean remove the
>> pdev_pasid_supported() ?
>>
> 
> yes

Yeah. That should be fine I guess.  Will fix it.

-Vasant

[PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Vasant Hegde]

Add support to allocate paging domain. Note that core will call
domain_alloc_user() to allocate PASID capable domain. Hence its not
checking device capability and allocates page table based on
amd_iommu_pgtable.

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1c9e539f15a7..e6b4460c485d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2395,6 +2395,16 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
 	return domain;
 }
 
+/*
+ * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
+ * call domain_alloc_user() interface to allocate PASID capable domain.
+ */
+static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
+{
+	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
+				     dev, 0, amd_iommu_pgtable);
+}
+
 static struct iommu_domain *
 amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 			    struct iommu_domain *parent,
@@ -2858,6 +2868,7 @@ const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.blocked_domain = &blocked_domain,
 	.domain_alloc = amd_iommu_domain_alloc,
+	.domain_alloc_paging = amd_iommu_domain_alloc_paging,
 	.domain_alloc_user = amd_iommu_domain_alloc_user,
 	.domain_alloc_sva = amd_iommu_domain_alloc_sva,
 	.probe_device = amd_iommu_probe_device,
-- 
2.31.1

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:10AM +0000, Vasant Hegde wrote:
> Add support to allocate paging domain. Note that core will call
> domain_alloc_user() to allocate PASID capable domain. Hence its not
> checking device capability and allocates page table based on
> amd_iommu_pgtable.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 1c9e539f15a7..e6b4460c485d 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2395,6 +2395,16 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
>  	return domain;
>  }
>  
> +/*
> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
> + * call domain_alloc_user() interface to allocate PASID capable domain.
> + */
> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
> +{
> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
> +				     dev, 0, amd_iommu_pgtable);
> +}

I think you should avoid this. The _user one (aka extended) should
handle everthing. It doesn't really make sense anyhow, as the legacy
VFIO path is going to take this branch as it will have 0 flags.

Can we just get rid of amd_iommu_pgtable ? The logic to do feature
negotiation with HW support needs to be embedded inside
amd_iommu_domain_alloc_user() anyhow

And the two kernel command line options seem totally useless now that
we select the page table format correctly automatically. As you've
explained the v1 format is always better.

Maybe the kernel command line options should be revised to force ONLY
v2 format and ONLY v1 format, but that still wouldn't need this
function. But also maybe they should just be removed.

Jason

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Vasant Hegde]

Jason,

On 10/3/2024 1:03 AM, Jason Gunthorpe wrote:
> On Wed, Sep 11, 2024 at 10:19:10AM +0000, Vasant Hegde wrote:
>> Add support to allocate paging domain. Note that core will call
>> domain_alloc_user() to allocate PASID capable domain. Hence its not
>> checking device capability and allocates page table based on
>> amd_iommu_pgtable.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>  drivers/iommu/amd/iommu.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 1c9e539f15a7..e6b4460c485d 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2395,6 +2395,16 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
>>  	return domain;
>>  }
>>  
>> +/*
>> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
>> + * call domain_alloc_user() interface to allocate PASID capable domain.
>> + */
>> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
>> +{
>> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
>> +				     dev, 0, amd_iommu_pgtable);
>> +}
> 
> I think you should avoid this. The _user one (aka extended) should
> handle everthing. It doesn't really make sense anyhow, as the legacy
> VFIO path is going to take this branch as it will have 0 flags.

I should have been bit more clear in cover letter.

I thought eventually we want to remove ops->domain_alloc() interface. Hence I
introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
this series. I was planning to do it in later.

If we are fine to keep ops->domain_alloc() then I can just drop this patch.

> 
> Can we just get rid of amd_iommu_pgtable ? The logic to do feature
> negotiation with HW support needs to be embedded inside
> amd_iommu_domain_alloc_user() anyhow
> 
> And the two kernel command line options seem totally useless now that
> we select the page table format correctly automatically. As you've
> explained the v1 format is always better.
> 
> Maybe the kernel command line options should be revised to force ONLY
> v2 format and ONLY v1 format, but that still wouldn't need this
> function. But also maybe they should just be removed.

Yeah. Some enforcement and cleanup is needed in that path. Its in my list.

Before that I need to fix few more things like amd_iommu_def_domain_type(). I
have few patches internally. Will post it after this series.

-Vasant

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Jason Gunthorpe]

On Fri, Oct 04, 2024 at 05:25:20PM +0530, Vasant Hegde wrote:

> >> +/*
> >> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
> >> + * call domain_alloc_user() interface to allocate PASID capable domain.
> >> + */
> >> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
> >> +{
> >> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
> >> +				     dev, 0, amd_iommu_pgtable);
> >> +}
> > 
> > I think you should avoid this. The _user one (aka extended) should
> > handle everthing. It doesn't really make sense anyhow, as the legacy
> > VFIO path is going to take this branch as it will have 0 flags.
> 
> I should have been bit more clear in cover letter.
> 
> I thought eventually we want to remove ops->domain_alloc() interface. Hence I
> introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
> this series. I was planning to do it in later.

We do want to remove domain_alloc

> If we are fine to keep ops->domain_alloc() then I can just drop this patch.

But why not just remove it and not add amd_iommu_domain_alloc_paging ?

Your prior patch does this:

+	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);

So if !ops->domain_alloc_paging && ops->domain_alloc_user it calls
ops->domain_alloc_user()

You just need ops->domain_alloc_user(flags == 0) to allocate the
fasted/best page table with no-PASID support (ie v1 in most cases) and
everything is fine.

Don't add amd_iommu_domain_alloc_paging(), just add the above
paragraph of logic to ops->domain_alloc_user()

The point of all this fixing was to stop using IOMMU_DOMAIN_DMA to
select a page table format and instead rely on IOMMU_HWPT_ALLOC_PASID
to make the selection. The core code and VFIO will set
IOMMU_HWPT_ALLOC_PASID properly for what it wants to do. I expect the
driver to rely on IOMMU_HWPT_ALLOC_PASID exclusively.

Jason

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Vasant Hegde]

Jason,


On 10/4/2024 6:26 PM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2024 at 05:25:20PM +0530, Vasant Hegde wrote:
> 
>>>> +/*
>>>> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
>>>> + * call domain_alloc_user() interface to allocate PASID capable domain.
>>>> + */
>>>> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
>>>> +{
>>>> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
>>>> +				     dev, 0, amd_iommu_pgtable);
>>>> +}
>>>
>>> I think you should avoid this. The _user one (aka extended) should
>>> handle everthing. It doesn't really make sense anyhow, as the legacy
>>> VFIO path is going to take this branch as it will have 0 flags.
>>
>> I should have been bit more clear in cover letter.
>>
>> I thought eventually we want to remove ops->domain_alloc() interface. Hence I
>> introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
>> this series. I was planning to do it in later.
> 
> We do want to remove domain_alloc
> 
>> If we are fine to keep ops->domain_alloc() then I can just drop this patch.
> 
> But why not just remove it and not add amd_iommu_domain_alloc_paging ?

Because we have __iommu_domain_alloc() which calls domain_alloc_paging /
domain_alloc().

> 
> Your prior patch does this:
> 
> +	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);
> 
> So if !ops->domain_alloc_paging && ops->domain_alloc_user it calls
> ops->domain_alloc_user()
> 
> You just need ops->domain_alloc_user(flags == 0) to allocate the
> fasted/best page table with no-PASID support (ie v1 in most cases) and
> everything is fine.

This is fine. I can add it.

> 
> Don't add amd_iommu_domain_alloc_paging(), just add the above
> paragraph of logic to ops->domain_alloc_user()
> 
> The point of all this fixing was to stop using IOMMU_DOMAIN_DMA to
> select a page table format and instead rely on IOMMU_HWPT_ALLOC_PASID
> to make the selection. The core code and VFIO will set
> IOMMU_HWPT_ALLOC_PASID properly for what it wants to do. I expect the
> driver to rely on IOMMU_HWPT_ALLOC_PASID exclusively.

Sure. Will fix it. But as said above, we still need domain_alloc_paging() or
domain_alloc() interface.


-Vasant

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Jason Gunthorpe]

On Fri, Oct 04, 2024 at 08:00:15PM +0530, Vasant Hegde wrote:
> >> I thought eventually we want to remove ops->domain_alloc() interface. Hence I
> >> introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
> >> this series. I was planning to do it in later.
> > 
> > We do want to remove domain_alloc
> > 
> >> If we are fine to keep ops->domain_alloc() then I can just drop this patch.
> > 
> > But why not just remove it and not add amd_iommu_domain_alloc_paging ?
> 
> Because we have __iommu_domain_alloc() which calls domain_alloc_paging /
> domain_alloc().

Oh, I see what you are worried about. __iommu_domain_alloc() is almost
never called now that most of Lu's patches are merged, and it is not
needed for any x86 platform case.

Here are two more patches you can grab that will remove it from the
remaining core cases and then you don't need to worry about it,
everything for AMD will go through __iommu_paging_domain_alloc_flags():

  https://github.com/jgunthorpe/linux/commits/for-vasant

Lu should be able to delete the function this cycle as well.

Also I noticed a little mistake:

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2071,7 +2071,7 @@ static struct iommu_domain *__iommu_paging_domain_alloc_flags(struct device *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);
+               domain = ops->domain_alloc(type);
        else
                return ERR_PTR(-EOPNOTSUPP);

Jason

Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Vasant Hegde]

Hi Jason,


On 10/4/2024 9:01 PM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2024 at 08:00:15PM +0530, Vasant Hegde wrote:
>>>> I thought eventually we want to remove ops->domain_alloc() interface. Hence I
>>>> introduced domain_alloc_paging(). I didn't remove amd_iommu_domain_alloc() in
>>>> this series. I was planning to do it in later.
>>>
>>> We do want to remove domain_alloc
>>>
>>>> If we are fine to keep ops->domain_alloc() then I can just drop this patch.
>>>
>>> But why not just remove it and not add amd_iommu_domain_alloc_paging ?
>>
>> Because we have __iommu_domain_alloc() which calls domain_alloc_paging /
>> domain_alloc().
> 
> Oh, I see what you are worried about. __iommu_domain_alloc() is almost
> never called now that most of Lu's patches are merged, and it is not
> needed for any x86 platform case.
> 
> Here are two more patches you can grab that will remove it from the
> remaining core cases and then you don't need to worry about it,
> everything for AMD will go through __iommu_paging_domain_alloc_flags():
> 
>   https://github.com/jgunthorpe/linux/commits/for-vasant

Sure. Will pick those patches and get the v3 soon.

-Vasant

[PATCH v2 8/8] 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>
---
 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 6386fa4556d9..983a9d109206 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 43131c3a2172..8517a854c331 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2172,6 +2172,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 e6b4460c485d..6b1bb07e0aae 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);
 
@@ -2262,6 +2265,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;
@@ -2276,10 +2287,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;
 }
@@ -2469,6 +2477,25 @@ static struct iommu_domain blocked_domain = {
 	}
 };
 
+static struct protection_domain identity_domain;
+
+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)
 {
@@ -2867,6 +2894,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_paging = amd_iommu_domain_alloc_paging,
 	.domain_alloc_user = amd_iommu_domain_alloc_user,
-- 
2.31.1

Re: [PATCH v2 8/8] iommu/amd: Implement global identity domain

[Jason Gunthorpe]

On Wed, Sep 11, 2024 at 10:19:11AM +0000, Vasant Hegde wrote:
> +static struct protection_domain identity_domain;
> +
> +const struct iommu_domain_ops identity_domain_ops = {
> +	.attach_dev = amd_iommu_attach_device,
> +};

It is fine for now, but attach_dev really should be a function that
just does identity and what is left should really just only do
paging. Same for blocked. That simplifies the flows quite a bit..

It would also be really nice if the identity_domain and blocked_domain
was not a struct protection_domain like all the other drivers have
managed..

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

Jason

Re: [PATCH v2 8/8] iommu/amd: Implement global identity domain

[Vasant Hegde]

On 10/3/2024 1:06 AM, Jason Gunthorpe wrote:
> On Wed, Sep 11, 2024 at 10:19:11AM +0000, Vasant Hegde wrote:
>> +static struct protection_domain identity_domain;
>> +
>> +const struct iommu_domain_ops identity_domain_ops = {
>> +	.attach_dev = amd_iommu_attach_device,
>> +};
> 
> It is fine for now, but attach_dev really should be a function that
> just does identity and what is left should really just only do
> paging. Same for blocked. That simplifies the flows quite a bit..
> 
> It would also be really nice if the identity_domain and blocked_domain
> was not a struct protection_domain like all the other drivers have
> managed..

We support SVA with identity domain. So for now I had to create
protection_domain as identity domain.

I will revisit after fixing attach domain code.

-Vasant

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

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Vasant Hegde]

Jason, Baolu,

On 9/11/2024 3:49 PM, 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 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.
> 
> @Baolu,
>   With this changes (patch 1), to allocate PASID capable device core
>   will call domain_alloc_user() interface. Do we need any changes to
>   intel driver?

@Baolu,
  Looks like intel driver works fine with this change. Is that correct -OR- do
we need any changes to intel_iommu_domain_alloc_user() ?


> 
> @Jason,
>   Do we need any changes to arm-smm-v3 driver?

@Jason,

Looking into SMM3 driver it looks like some changes required. Does something
like below works?


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)


-Vasant

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe]

On Wed, Oct 02, 2024 at 11:00:32AM +0530, Vasant Hegde wrote:

> Looking into SMM3 driver it looks like some changes required. Does something
> like below works?
> 
> 
> 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);
> +

Yes, I think that is good enough for now.

Thanks,
Jason

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Vasant Hegde]

Jason,


On 10/2/2024 7:54 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2024 at 11:00:32AM +0530, Vasant Hegde wrote:
> 
>> Looking into SMM3 driver it looks like some changes required. Does something
>> like below works?
>>
>>
>> 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);
>> +
> 
> Yes, I think that is good enough for now.

Thanks! I will add this patch in next version.

-Vasant

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Baolu Lu]

On 2024/10/2 13:30, Vasant Hegde wrote:
> On 9/11/2024 3:49 PM, 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 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.
>>
>> @Baolu,
>>    With this changes (patch 1), to allocate PASID capable device core
>>    will call domain_alloc_user() interface. Do we need any changes to
>>    intel driver?
> @Baolu,
>    Looks like intel driver works fine with this change. Is that correct -OR- do
> we need any changes to intel_iommu_domain_alloc_user() ?

I still have a question about the following change:

https://lore.kernel.org/linux-iommu/a3e09a90-0b63-4332-9e25-a5832d5ec8b2@linux.intel.com/

This change might cause a functional regression when it comes to nested
translation. In nested translation mode, the user page table (e.g.,
created and managed by a guest VM for guest kernel DMA) must be in the
first-stage page table format. Then, it can be nested on a second-stage
page table managed by the host kernel.

Currently, the kernel automatically selects the page table formats. For
example, the Intel IOMMU driver always uses the first-stage page table
for guest kernel DMA. After this change, this assumption no longer holds
true. This means the kernel might use a second-stage page table for
guest kernel DMA, breaking nested translation.

Thanks,
baolu

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Vasant Hegde]

Hi Baolu,


On 10/9/2024 8:17 AM, Baolu Lu wrote:
> On 2024/10/2 13:30, Vasant Hegde wrote:
>> On 9/11/2024 3:49 PM, 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 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.
>>>
>>> @Baolu,
>>>    With this changes (patch 1), to allocate PASID capable device core
>>>    will call domain_alloc_user() interface. Do we need any changes to
>>>    intel driver?
>> @Baolu,
>>    Looks like intel driver works fine with this change. Is that correct -OR- do
>> we need any changes to intel_iommu_domain_alloc_user() ?
> 
> I still have a question about the following change:
> 
> https://lore.kernel.org/linux-iommu/a3e09a90-0b63-4332-9e25-a5832d5ec8b2@linux.intel.com/
> 
> This change might cause a functional regression when it comes to nested
> translation. In nested translation mode, the user page table (e.g.,
> created and managed by a guest VM for guest kernel DMA) must be in the
> first-stage page table format. Then, it can be nested on a second-stage
> page table managed by the host kernel.
> 
> Currently, the kernel automatically selects the page table formats. For
> example, the Intel IOMMU driver always uses the first-stage page table
> for guest kernel DMA. After this change, this assumption no longer holds
> true. This means the kernel might use a second-stage page table for
> guest kernel DMA, breaking nested translation.

Hmmm. I assumed after discussion in v1 series you are fine. Looks like I misread it?

IIUC intel driver wants something like below.

  - Host : DMA API Mode : first level page table
    (PASID is supported in both level, so you don't need PASID capability check)
  - Host UNMANAGED (VFIO Passthrough) : second level page table
  - Guest : first level page table
  - Nested : second level page table

Is this the correct understanding?

W/ this series (domain_alloc_user() interface), you will not be able to decide
right page table (second level page table) for UNMANAGED domain. Is that correct?

-Vasant

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe]

On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:

> > This change might cause a functional regression when it comes to nested
> > translation. In nested translation mode, the user page table (e.g.,
> > created and managed by a guest VM for guest kernel DMA) must be in the
> > first-stage page table format. Then, it can be nested on a second-stage
> > page table managed by the host kernel.
> > 
> > Currently, the kernel automatically selects the page table formats. For
> > example, the Intel IOMMU driver always uses the first-stage page table
> > for guest kernel DMA. After this change, this assumption no longer holds
> > true. This means the kernel might use a second-stage page table for
> > guest kernel DMA, breaking nested translation.
> 
> Hmmm. I assumed after discussion in v1 series you are fine. Looks like I misread it?

It is a bug in the implementation in the intel driver.

intel_iommu_domain_alloc_user() should always allocate a page table
that works, if you are in a guest context it must allocate a first
level/guest page table otherwise nested VFIO will be broken.

For this reason Intel driver should always allocate the guest
compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
specified.

Something like this:

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef5e..cfae6c2973e0ee 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1440,7 +1440,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
  * Check and return whether first level is used by default for
  * DMA translation.
  */
-static bool first_level_by_default(unsigned int type)
+static bool first_level_by_default(void)
 {
 	/* Only SL is available in legacy mode */
 	if (!scalable_mode_support())
@@ -1450,8 +1450,8 @@ static bool first_level_by_default(unsigned int type)
 	if (intel_cap_flts_sanity() ^ intel_cap_slts_sanity())
 		return intel_cap_flts_sanity();
 
-	/* Both levels are available, decide it based on domain type */
-	return type != IOMMU_DOMAIN_UNMANAGED;
+	/* Both levels are available, use FL */
+	return true;
 }
 
 static struct dmar_domain *alloc_domain(unsigned int type)
@@ -1463,7 +1463,7 @@ static struct dmar_domain *alloc_domain(unsigned int type)
 		return NULL;
 
 	domain->nid = NUMA_NO_NODE;
-	if (first_level_by_default(type))
+	if (first_level_by_default())
 		domain->use_first_level = true;
 	INIT_LIST_HEAD(&domain->devices);
 	INIT_LIST_HEAD(&domain->dev_pasids);
@@ -3287,8 +3287,7 @@ int __init intel_iommu_init(void)
 		 * is likely to be much lower than the overhead of synchronizing
 		 * the virtual and physical IOMMU page-tables.
 		 */
-		if (cap_caching_mode(iommu->cap) &&
-		    !first_level_by_default(IOMMU_DOMAIN_DMA)) {
+		if (cap_caching_mode(iommu->cap) && !first_level_by_default()) {
 			pr_info_once("IOMMU batching disallowed due to virtualization\n");
 			iommu_set_dma_strict();
 		}
@@ -3530,6 +3529,7 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	struct intel_iommu *iommu = info->iommu;
 	struct dmar_domain *dmar_domain;
 	struct iommu_domain *domain;
+	bool first_level;
 
 	/* Must be NESTING domain */
 	if (parent) {
@@ -3541,13 +3541,18 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	if (flags &
 	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
 		return ERR_PTR(-EOPNOTSUPP);
+	if ((flags & IOMMU_HWPT_ALLOC_NEST_PARENT) && !intel_cap_slts_sanity())
+		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);
 
-	/* Do not use first stage for user domain translation. */
-	dmar_domain = paging_domain_alloc(dev, false);
+	if (flags & IOMMU_HWPT_ALLOC_NEST_PARENT)
+		first_level = false;
+	else
+		first_level = first_level_by_default();
+	dmar_domain = paging_domain_alloc(dev, first_level);
 	if (IS_ERR(dmar_domain))
 		return ERR_CAST(dmar_domain);
 	domain = &dmar_domain->domain;

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Baolu Lu]

On 2024/10/9 20:15, Jason Gunthorpe wrote:
> On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
> 
>>> This change might cause a functional regression when it comes to nested
>>> translation. In nested translation mode, the user page table (e.g.,
>>> created and managed by a guest VM for guest kernel DMA) must be in the
>>> first-stage page table format. Then, it can be nested on a second-stage
>>> page table managed by the host kernel.
>>>
>>> Currently, the kernel automatically selects the page table formats. For
>>> example, the Intel IOMMU driver always uses the first-stage page table
>>> for guest kernel DMA. After this change, this assumption no longer holds
>>> true. This means the kernel might use a second-stage page table for
>>> guest kernel DMA, breaking nested translation.
>> Hmmm. I assumed after discussion in v1 series you are fine. Looks like I misread it?
> It is a bug in the implementation in the intel driver.
> 
> intel_iommu_domain_alloc_user() should always allocate a page table
> that works, if you are in a guest context it must allocate a first
> level/guest page table otherwise nested VFIO will be broken.
> 
> For this reason Intel driver should always allocate the guest
> compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
> specified.
> 
> Something like this:

This will break the existing dirty page tracking functionality. Intel
IOMMU only supports enabling or disabling dirty page tracking at the
second-stage page table.

/*
  * Set up dirty tracking on a second only or nested translation type.
  */
int intel_pasid_setup_dirty_tracking(struct intel_iommu *iommu,
                                      struct device *dev, u32 pasid,
                                      bool enabled)
{
[...]
         pgtt = pasid_pte_get_pgtt(pte);
         if (pgtt != PASID_ENTRY_PGTT_SL_ONLY &&
             pgtt != PASID_ENTRY_PGTT_NESTED) {
                 spin_unlock(&iommu->lock);
                 dev_err_ratelimited(
                         dev,
                         "Dirty tracking not supported on translation 
type %d\n",
                         pgtt);
                 return -EOPNOTSUPP;
         }

Thanks,
baolu

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Baolu Lu]

On 2024/10/10 14:40, Baolu Lu wrote:
> On 2024/10/9 20:15, Jason Gunthorpe wrote:
>> On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
>>
>>>> This change might cause a functional regression when it comes to nested
>>>> translation. In nested translation mode, the user page table (e.g.,
>>>> created and managed by a guest VM for guest kernel DMA) must be in the
>>>> first-stage page table format. Then, it can be nested on a second-stage
>>>> page table managed by the host kernel.
>>>>
>>>> Currently, the kernel automatically selects the page table formats. For
>>>> example, the Intel IOMMU driver always uses the first-stage page table
>>>> for guest kernel DMA. After this change, this assumption no longer 
>>>> holds
>>>> true. This means the kernel might use a second-stage page table for
>>>> guest kernel DMA, breaking nested translation.
>>> Hmmm. I assumed after discussion in v1 series you are fine. Looks 
>>> like I misread it?
>> It is a bug in the implementation in the intel driver.
>>
>> intel_iommu_domain_alloc_user() should always allocate a page table
>> that works, if you are in a guest context it must allocate a first
>> level/guest page table otherwise nested VFIO will be broken.
>>
>> For this reason Intel driver should always allocate the guest
>> compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
>> specified.
>>
>> Something like this:
> 
> This will break the existing dirty page tracking functionality. Intel
> IOMMU only supports enabling or disabling dirty page tracking at the
> second-stage page table.

So, perhaps something like below?

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 1497f3112b12..e11dde259afa 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -544,6 +544,8 @@ enum {
                                 ecap_slads((iommu)->ecap))
  #define nested_supported(iommu)        (sm_supported(iommu) && 
         \
                                  ecap_nest((iommu)->ecap))
+#define flts_supported(iommu)  (sm_supported(iommu) &&                 \
+                                ecap_flts((iommu)->ecap))

  struct pasid_entry;
  struct pasid_state_entry;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 9f6b0780f2ef..d1a7378489a4 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3530,6 +3530,7 @@ intel_iommu_domain_alloc_user(struct device *dev, 
u32 flags,
         struct intel_iommu *iommu = info->iommu;
         struct dmar_domain *dmar_domain;
         struct iommu_domain *domain;
+       bool first_stage;

         /* Must be NESTING domain */
         if (parent) {
@@ -3546,8 +3547,14 @@ intel_iommu_domain_alloc_user(struct device *dev, 
u32 flags,
         if (user_data || (dirty_tracking && !ssads_supported(iommu)))
                 return ERR_PTR(-EOPNOTSUPP);

-       /* Do not use first stage for user domain translation. */
-       dmar_domain = paging_domain_alloc(dev, false);
+       /*
+        * Always allocate the guest compatible page table unless
+        * IOMMU_HWPT_ALLOC_NEST_PARENT or IOMMU_HWPT_ALLOC_DIRTY_TRACKING
+        * is specified.
+        */
+       first_stage = (nested_parent || dirty_tracking) ?
+                       false : flts_supported(iommu);
+       dmar_domain = paging_domain_alloc(dev, first_stage);
         if (IS_ERR(dmar_domain))
                 return ERR_CAST(dmar_domain);
         domain = &dmar_domain->domain;

Thanks,
baolu

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe]

On Thu, Oct 10, 2024 at 02:48:09PM +0800, Baolu Lu wrote:
> On 2024/10/10 14:40, Baolu Lu wrote:
> > On 2024/10/9 20:15, Jason Gunthorpe wrote:
> > > On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
> > > 
> > > > > This change might cause a functional regression when it comes to nested
> > > > > translation. In nested translation mode, the user page table (e.g.,
> > > > > created and managed by a guest VM for guest kernel DMA) must be in the
> > > > > first-stage page table format. Then, it can be nested on a second-stage
> > > > > page table managed by the host kernel.
> > > > > 
> > > > > Currently, the kernel automatically selects the page table formats. For
> > > > > example, the Intel IOMMU driver always uses the first-stage page table
> > > > > for guest kernel DMA. After this change, this assumption no
> > > > > longer holds
> > > > > true. This means the kernel might use a second-stage page table for
> > > > > guest kernel DMA, breaking nested translation.
> > > > Hmmm. I assumed after discussion in v1 series you are fine.
> > > > Looks like I misread it?
> > > It is a bug in the implementation in the intel driver.
> > > 
> > > intel_iommu_domain_alloc_user() should always allocate a page table
> > > that works, if you are in a guest context it must allocate a first
> > > level/guest page table otherwise nested VFIO will be broken.
> > > 
> > > For this reason Intel driver should always allocate the guest
> > > compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
> > > specified.
> > > 
> > > Something like this:
> > 
> > This will break the existing dirty page tracking functionality. Intel
> > IOMMU only supports enabling or disabling dirty page tracking at the
> > second-stage page table.
> 
> So, perhaps something like below?
> 
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 1497f3112b12..e11dde259afa 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -544,6 +544,8 @@ enum {
>                                 ecap_slads((iommu)->ecap))
>  #define nested_supported(iommu)        (sm_supported(iommu) &&         \
>                                  ecap_nest((iommu)->ecap))
> +#define flts_supported(iommu)  (sm_supported(iommu) &&                 \
> +                                ecap_flts((iommu)->ecap))
> 
>  struct pasid_entry;
>  struct pasid_state_entry;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9f6b0780f2ef..d1a7378489a4 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3530,6 +3530,7 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
> flags,
>         struct intel_iommu *iommu = info->iommu;
>         struct dmar_domain *dmar_domain;
>         struct iommu_domain *domain;
> +       bool first_stage;
> 
>         /* Must be NESTING domain */
>         if (parent) {
> @@ -3546,8 +3547,14 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
> flags,
>         if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>                 return ERR_PTR(-EOPNOTSUPP);
> 
> -       /* Do not use first stage for user domain translation. */
> -       dmar_domain = paging_domain_alloc(dev, false);
> +       /*
> +        * Always allocate the guest compatible page table unless
> +        * IOMMU_HWPT_ALLOC_NEST_PARENT or IOMMU_HWPT_ALLOC_DIRTY_TRACKING
> +        * is specified.
> +        */
> +       first_stage = (nested_parent || dirty_tracking) ?
> +                       false : flts_supported(iommu);

That makes sense, but these flags still need to be rejected if the
second level is not supported in the HW.

+	if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) && !intel_cap_slts_sanity())
+		return ERR_PTR(-EOPNOTSUPP);

I also think my version was cleaner as we should be using
first_level_by_default() consistently to make that decision.

Also don't care for ternary expressions :)

Jason

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Baolu Lu]

On 2024/10/10 19:38, Jason Gunthorpe wrote:
> On Thu, Oct 10, 2024 at 02:48:09PM +0800, Baolu Lu wrote:
>> On 2024/10/10 14:40, Baolu Lu wrote:
>>> On 2024/10/9 20:15, Jason Gunthorpe wrote:
>>>> On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
>>>>
>>>>>> This change might cause a functional regression when it comes to nested
>>>>>> translation. In nested translation mode, the user page table (e.g.,
>>>>>> created and managed by a guest VM for guest kernel DMA) must be in the
>>>>>> first-stage page table format. Then, it can be nested on a second-stage
>>>>>> page table managed by the host kernel.
>>>>>>
>>>>>> Currently, the kernel automatically selects the page table formats. For
>>>>>> example, the Intel IOMMU driver always uses the first-stage page table
>>>>>> for guest kernel DMA. After this change, this assumption no
>>>>>> longer holds
>>>>>> true. This means the kernel might use a second-stage page table for
>>>>>> guest kernel DMA, breaking nested translation.
>>>>> Hmmm. I assumed after discussion in v1 series you are fine.
>>>>> Looks like I misread it?
>>>> It is a bug in the implementation in the intel driver.
>>>>
>>>> intel_iommu_domain_alloc_user() should always allocate a page table
>>>> that works, if you are in a guest context it must allocate a first
>>>> level/guest page table otherwise nested VFIO will be broken.
>>>>
>>>> For this reason Intel driver should always allocate the guest
>>>> compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
>>>> specified.
>>>>
>>>> Something like this:
>>> This will break the existing dirty page tracking functionality. Intel
>>> IOMMU only supports enabling or disabling dirty page tracking at the
>>> second-stage page table.
>> So, perhaps something like below?
>>
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 1497f3112b12..e11dde259afa 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -544,6 +544,8 @@ enum {
>>                                  ecap_slads((iommu)->ecap))
>>   #define nested_supported(iommu)        (sm_supported(iommu) &&         \
>>                                   ecap_nest((iommu)->ecap))
>> +#define flts_supported(iommu)  (sm_supported(iommu) &&                 \
>> +                                ecap_flts((iommu)->ecap))
>>
>>   struct pasid_entry;
>>   struct pasid_state_entry;
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 9f6b0780f2ef..d1a7378489a4 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -3530,6 +3530,7 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
>> flags,
>>          struct intel_iommu *iommu = info->iommu;
>>          struct dmar_domain *dmar_domain;
>>          struct iommu_domain *domain;
>> +       bool first_stage;
>>
>>          /* Must be NESTING domain */
>>          if (parent) {
>> @@ -3546,8 +3547,14 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
>> flags,
>>          if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>>                  return ERR_PTR(-EOPNOTSUPP);
>>
>> -       /* Do not use first stage for user domain translation. */
>> -       dmar_domain = paging_domain_alloc(dev, false);
>> +       /*
>> +        * Always allocate the guest compatible page table unless
>> +        * IOMMU_HWPT_ALLOC_NEST_PARENT or IOMMU_HWPT_ALLOC_DIRTY_TRACKING
>> +        * is specified.
>> +        */
>> +       first_stage = (nested_parent || dirty_tracking) ?
>> +                       false : flts_supported(iommu);
> That makes sense, but these flags still need to be rejected if the
> second level is not supported in the HW.
> 
> +	if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) && !intel_cap_slts_sanity())
> +		return ERR_PTR(-EOPNOTSUPP);
> 
> I also think my version was cleaner as we should be using
> first_level_by_default() consistently to make that decision.
> 
> Also don't care for ternary expressions 🙂

Okay, seems that we are on the same page now. :-)

I have a series to add domain_alloc_paging support in the Intel iommu
driver. I will convert the change in a formal patch and put it in that
series for further review.

Thanks,
baolu

RE: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, October 9, 2024 8:16 PM
> 
> On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
> 
> > > This change might cause a functional regression when it comes to nested
> > > translation. In nested translation mode, the user page table (e.g.,
> > > created and managed by a guest VM for guest kernel DMA) must be in
> the
> > > first-stage page table format. Then, it can be nested on a second-stage
> > > page table managed by the host kernel.
> > >
> > > Currently, the kernel automatically selects the page table formats. For
> > > example, the Intel IOMMU driver always uses the first-stage page table
> > > for guest kernel DMA. After this change, this assumption no longer holds
> > > true. This means the kernel might use a second-stage page table for
> > > guest kernel DMA, breaking nested translation.
> >
> > Hmmm. I assumed after discussion in v1 series you are fine. Looks like I
> misread it?
> 
> It is a bug in the implementation in the intel driver.
> 
> intel_iommu_domain_alloc_user() should always allocate a page table
> that works, if you are in a guest context it must allocate a first
> level/guest page table otherwise nested VFIO will be broken.

I don't think it's a bug. If nested is broken then VMM can fall back
to the shadowing approach. If the VMM only supports nested
then the only way to guarantee it is to not advertise 2nd-level
capability on the vIOMMU.

The VT-d spec never says that guest software must use 1st-level.

> 
> For this reason Intel driver should always allocate the guest
> compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
> specified.
> 

But I'm fine with this *policy* change.

Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe]

On Fri, Oct 11, 2024 at 05:06:29AM +0000, Tian, Kevin wrote:

> > It is a bug in the implementation in the intel driver.
> > 
> > intel_iommu_domain_alloc_user() should always allocate a page table
> > that works, if you are in a guest context it must allocate a first
> > level/guest page table otherwise nested VFIO will be broken.
> 
> I don't think it's a bug. If nested is broken then VMM can fall back
> to the shadowing approach. If the VMM only supports nested
> then the only way to guarantee it is to not advertise 2nd-level
> capability on the vIOMMU.

Right, which is the bug, the alloc function isn't checking the
capabilities of the HW before going ahead to allocate a table format.

So it can choose a table format the HW doesn't advertise support
for. Whoops.

Jason

RE: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Friday, October 11, 2024 7:39 PM
> 
> On Fri, Oct 11, 2024 at 05:06:29AM +0000, Tian, Kevin wrote:
> 
> > > It is a bug in the implementation in the intel driver.
> > >
> > > intel_iommu_domain_alloc_user() should always allocate a page table
> > > that works, if you are in a guest context it must allocate a first
> > > level/guest page table otherwise nested VFIO will be broken.
> >
> > I don't think it's a bug. If nested is broken then VMM can fall back
> > to the shadowing approach. If the VMM only supports nested
> > then the only way to guarantee it is to not advertise 2nd-level
> > capability on the vIOMMU.
> 
> Right, which is the bug, the alloc function isn't checking the
> capabilities of the HW before going ahead to allocate a table format.
> 
> So it can choose a table format the HW doesn't advertise support
> for. Whoops.
> 

That's certainly a bug.

My comment was about " if you are in a guest context it must
allocate a first level/guest page table otherwise nested VFIO will
be broken ", which is a policy thing in supported caps.