[PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Currently domain_alloc_paging() passes device as param for domain
allocation. While this is sufficient for some HW vendor, its not
sufficent for others.

AMD IOMMU has two different page tables (v1 and v2). For DMA API mode it
wants to allocate page table based on device capability. V2 for PASID
capable device and v1 for rest of the devices. For UNMANAGED domain, it
wants to continue to enforce v1 page table as its cache efficient. Hence
include 'domain type' as parameter to domain_alloc_paging().

While at it also add 'flag' as additional parameter. So that any page
table specific quirks (like IO_PGTABLE_QUIRK_*) can be passed to vendor
driver. Once we have this we can remove ops->set_pgtable_quirks()
interface.

Note:
  Intent of this patch is to discuss/finalize the domain_alloc_paging()
  ops. Once we agree on interfaces I will fix other drivers and send proper
  patch series. That means with vendor driver config this doesn't
  compile.

@Robin,
  Once we have this patch and Baolu's series [1], we can enhance
  iommu_paging_domain_alloc() to include page table quirks and then we can
  remove ops->set_pgtable_quirks(). I hope this works for ARM driver
  (arm/arm-smmu/arm-smmu.c).

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

Thanks everyone for looking into RFC patch and giving valuable suggestions.

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

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

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b19e8c0f48fa..240cca8bed21 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2429,6 +2429,31 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
 	return domain;
 }
 
+static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev, u32 type, u32 flags)
+{
+	struct iommu_dev_data *dev_data;
+	int pgtable = amd_iommu_pgtable;
+
+	if (dev)
+		dev_data = dev_iommu_priv_get(dev);
+
+	/*
+	 * - Force V1 page table for UNMANAGED domain.
+	 * - Use V2 page table for PASID capable device except when :
+	 *   - SNP is enabled, because it prohibits DTE[Mode]=0
+	 *   - amd_iommu=pgtbl_v[1/2] kernel command line is passed
+	 */
+	if (type == IOMMU_DOMAIN_UNMANAGED) {
+		pgtable = AMD_IOMMU_V1;
+	} else if (dev && dev_is_pci(dev) && pdev_pasid_supported(dev_data) &&
+		   !amd_iommu_force_isolation && !amd_iommu_snp_en) {
+		pgtable = AMD_IOMMU_V2;
+	}
+
+	/* TODO: Pass pgtable as param */
+	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA, dev, 0);
+}
+
 static struct iommu_domain *
 amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
 			    struct iommu_domain *parent,
@@ -2860,6 +2885,7 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.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,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ed6c5cb60c5a..d8a67b39a4cb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1946,7 +1946,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
 	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
 		return ops->blocked_domain;
 	else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
-		domain = ops->domain_alloc_paging(dev);
+		domain = ops->domain_alloc_paging(dev, type, 0);
 	else if (ops->domain_alloc)
 		domain = ops->domain_alloc(alloc_type);
 	else
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4d47f2c33311..72383f6bdd9f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -565,7 +565,8 @@ struct iommu_ops {
 	struct iommu_domain *(*domain_alloc_user)(
 		struct device *dev, u32 flags, struct iommu_domain *parent,
 		const struct iommu_user_data *user_data);
-	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
+	struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
+						    u32 iommu_domain_type, u32 flags);
 	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
 						 struct mm_struct *mm);
 
-- 
2.31.1

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Baolu Lu]

On 2024/8/1 22:45, Vasant Hegde wrote:
> Currently domain_alloc_paging() passes device as param for domain
> allocation. While this is sufficient for some HW vendor, its not
> sufficent for others.
> 
> AMD IOMMU has two different page tables (v1 and v2). For DMA API mode it
> wants to allocate page table based on device capability. V2 for PASID
> capable device and v1 for rest of the devices. For UNMANAGED domain, it
> wants to continue to enforce v1 page table as its cache efficient. Hence
> include 'domain type' as parameter to domain_alloc_paging().
> 
> While at it also add 'flag' as additional parameter. So that any page
> table specific quirks (like IO_PGTABLE_QUIRK_*) can be passed to vendor
> driver. Once we have this we can remove ops->set_pgtable_quirks()
> interface.
> 
> Note:
>    Intent of this patch is to discuss/finalize the domain_alloc_paging()
>    ops. Once we agree on interfaces I will fix other drivers and send proper
>    patch series. That means with vendor driver config this doesn't
>    compile.
> 
> @Robin,
>    Once we have this patch and Baolu's series [1], we can enhance
>    iommu_paging_domain_alloc() to include page table quirks and then we can
>    remove ops->set_pgtable_quirks(). I hope this works for ARM driver
>    (arm/arm-smmu/arm-smmu.c).
> 
> RFC v1 : https://lore.kernel.org/linux-iommu/7e249bc6-c578-40f0-aca7-835149a0ad39@amd.com/
> 
> Thanks everyone for looking into RFC patch and giving valuable suggestions.
> 
> [1] https://lore.kernel.org/linux-iommu/20240610085555.88197-2-baolu.lu@linux.intel.com/

That patch has been merged for v6.11-rc1.

> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>   drivers/iommu/amd/iommu.c | 26 ++++++++++++++++++++++++++
>   drivers/iommu/iommu.c     |  2 +-
>   include/linux/iommu.h     |  3 ++-
>   3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b19e8c0f48fa..240cca8bed21 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2429,6 +2429,31 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
>   	return domain;
>   }
>   
> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev, u32 type, u32 flags)
> +{
> +	struct iommu_dev_data *dev_data;
> +	int pgtable = amd_iommu_pgtable;
> +
> +	if (dev)
> +		dev_data = dev_iommu_priv_get(dev);
> +
> +	/*
> +	 * - Force V1 page table for UNMANAGED domain.
> +	 * - Use V2 page table for PASID capable device except when :
> +	 *   - SNP is enabled, because it prohibits DTE[Mode]=0
> +	 *   - amd_iommu=pgtbl_v[1/2] kernel command line is passed
> +	 */
> +	if (type == IOMMU_DOMAIN_UNMANAGED) {
> +		pgtable = AMD_IOMMU_V1;
> +	} else if (dev && dev_is_pci(dev) && pdev_pasid_supported(dev_data) &&
> +		   !amd_iommu_force_isolation && !amd_iommu_snp_en) {
> +		pgtable = AMD_IOMMU_V2;
> +	}
> +
> +	/* TODO: Pass pgtable as param */
> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA, dev, 0);
> +}
> +
>   static struct iommu_domain *
>   amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>   			    struct iommu_domain *parent,
> @@ -2860,6 +2885,7 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
>   const struct iommu_ops amd_iommu_ops = {
>   	.capable = amd_iommu_capable,
>   	.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,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index ed6c5cb60c5a..d8a67b39a4cb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1946,7 +1946,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
>   	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
>   		return ops->blocked_domain;
>   	else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
> -		domain = ops->domain_alloc_paging(dev);
> +		domain = ops->domain_alloc_paging(dev, type, 0);
>   	else if (ops->domain_alloc)
>   		domain = ops->domain_alloc(alloc_type);
>   	else
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 4d47f2c33311..72383f6bdd9f 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -565,7 +565,8 @@ struct iommu_ops {
>   	struct iommu_domain *(*domain_alloc_user)(
>   		struct device *dev, u32 flags, struct iommu_domain *parent,
>   		const struct iommu_user_data *user_data);
> -	struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
> +	struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
> +						    u32 iommu_domain_type, u32 flags);

I still can't see a value to pass the domain type in this callback.
Different domain could have different domain allocation callback, hence
the domain type has already been implied.

For the paging domain, there should be no difference between DMA and
UNMNANAGED from iommu driver's point of view.

>   	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
>   						 struct mm_struct *mm);
>   

Thanks,
baolu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Hi Baolu,


On 8/2/2024 6:14 AM, Baolu Lu wrote:
> On 2024/8/1 22:45, Vasant Hegde wrote:
>> Currently domain_alloc_paging() passes device as param for domain
>> allocation. While this is sufficient for some HW vendor, its not
>> sufficent for others.
>>
>> AMD IOMMU has two different page tables (v1 and v2). For DMA API mode it
>> wants to allocate page table based on device capability. V2 for PASID
>> capable device and v1 for rest of the devices. For UNMANAGED domain, it
>> wants to continue to enforce v1 page table as its cache efficient. Hence
>> include 'domain type' as parameter to domain_alloc_paging().
>>
>> While at it also add 'flag' as additional parameter. So that any page
>> table specific quirks (like IO_PGTABLE_QUIRK_*) can be passed to vendor
>> driver. Once we have this we can remove ops->set_pgtable_quirks()
>> interface.
>>
>> Note:
>>    Intent of this patch is to discuss/finalize the domain_alloc_paging()
>>    ops. Once we agree on interfaces I will fix other drivers and send proper
>>    patch series. That means with vendor driver config this doesn't
>>    compile.
>>
>> @Robin,
>>    Once we have this patch and Baolu's series [1], we can enhance
>>    iommu_paging_domain_alloc() to include page table quirks and then we can
>>    remove ops->set_pgtable_quirks(). I hope this works for ARM driver
>>    (arm/arm-smmu/arm-smmu.c).
>>
>> RFC v1 :
>> https://lore.kernel.org/linux-iommu/7e249bc6-c578-40f0-aca7-835149a0ad39@amd.com/
>>
>> Thanks everyone for looking into RFC patch and giving valuable suggestions.
>>
>> [1]
>> https://lore.kernel.org/linux-iommu/20240610085555.88197-2-baolu.lu@linux.intel.com/
> 
> That patch has been merged for v6.11-rc1.

Thanks!

> 
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 26 ++++++++++++++++++++++++++
>>   drivers/iommu/iommu.c     |  2 +-
>>   include/linux/iommu.h     |  3 ++-
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index b19e8c0f48fa..240cca8bed21 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2429,6 +2429,31 @@ static struct iommu_domain
>> *amd_iommu_domain_alloc(unsigned int type)
>>       return domain;
>>   }
>>   +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device
>> *dev, u32 type, u32 flags)
>> +{
>> +    struct iommu_dev_data *dev_data;
>> +    int pgtable = amd_iommu_pgtable;
>> +
>> +    if (dev)
>> +        dev_data = dev_iommu_priv_get(dev);
>> +
>> +    /*
>> +     * - Force V1 page table for UNMANAGED domain.
>> +     * - Use V2 page table for PASID capable device except when :
>> +     *   - SNP is enabled, because it prohibits DTE[Mode]=0
>> +     *   - amd_iommu=pgtbl_v[1/2] kernel command line is passed
>> +     */
>> +    if (type == IOMMU_DOMAIN_UNMANAGED) {
>> +        pgtable = AMD_IOMMU_V1;
>> +    } else if (dev && dev_is_pci(dev) && pdev_pasid_supported(dev_data) &&
>> +           !amd_iommu_force_isolation && !amd_iommu_snp_en) {
>> +        pgtable = AMD_IOMMU_V2;
>> +    }
>> +
>> +    /* TODO: Pass pgtable as param */
>> +    return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA, dev, 0);
>> +}
>> +
>>   static struct iommu_domain *
>>   amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>>                   struct iommu_domain *parent,
>> @@ -2860,6 +2885,7 @@ static int amd_iommu_dev_disable_feature(struct device
>> *dev,
>>   const struct iommu_ops amd_iommu_ops = {
>>       .capable = amd_iommu_capable,
>>       .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,
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index ed6c5cb60c5a..d8a67b39a4cb 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1946,7 +1946,7 @@ static struct iommu_domain *__iommu_domain_alloc(const
>> struct iommu_ops *ops,
>>       else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
>>           return ops->blocked_domain;
>>       else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
>> -        domain = ops->domain_alloc_paging(dev);
>> +        domain = ops->domain_alloc_paging(dev, type, 0);
>>       else if (ops->domain_alloc)
>>           domain = ops->domain_alloc(alloc_type);
>>       else
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 4d47f2c33311..72383f6bdd9f 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -565,7 +565,8 @@ struct iommu_ops {
>>       struct iommu_domain *(*domain_alloc_user)(
>>           struct device *dev, u32 flags, struct iommu_domain *parent,
>>           const struct iommu_user_data *user_data);
>> -    struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
>> +    struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
>> +                            u32 iommu_domain_type, u32 flags);
> 
> I still can't see a value to pass the domain type in this callback.
> Different domain could have different domain allocation callback, hence
> the domain type has already been implied.
> 
> For the paging domain, there should be no difference between DMA and
> UNMNANAGED from iommu driver's point of view.

That's true. Its all paging domain. But we need a way to indicate the desired
capability like PASID.

I thoughts we can use `type` for allocating domain and then `flag` to pass the
quirks. Otherwise we have to club everything in `flags` itself.

Something like below works ?

  - DMA-API domain : flag - DOMAIN_ALLOC_FLAG_PASID
    If both device and IOMMU supports PASID it will allocate PASID capable
    domain (Like AMD case domain with V2 page table). Else it will alloate
    non-pasid capable domain (In AMD case it will be domain with v1 page table).

  - UNMANAGED domain : Do not pass *_PASID support flag
    Since PASID flag is *not* passed, driver will decide best suitable page
    table (in AMD case, we will allocate V1 page table)


-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Fri, Aug 02, 2024 at 11:23:52AM +0530, Vasant Hegde wrote:

> >> +    struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
> >> +                            u32 iommu_domain_type, u32 flags);
> > 
> > I still can't see a value to pass the domain type in this callback.
> > Different domain could have different domain allocation callback, hence
> > the domain type has already been implied.

+1

> > For the paging domain, there should be no difference between DMA and
> > UNMNANAGED from iommu driver's point of view.
> 
> That's true. Its all paging domain. But we need a way to indicate the desired
> capability like PASID.
 
> I thoughts we can use `type` for allocating domain and then `flag` to pass the
> quirks. Otherwise we have to club everything in `flags` itself.
> 
> Something like below works ?
> 
>   - DMA-API domain : flag - DOMAIN_ALLOC_FLAG_PASID
>     If both device and IOMMU supports PASID it will allocate PASID capable
>     domain (Like AMD case domain with V2 page table). Else it will alloate
>     non-pasid capable domain (In AMD case it will be domain with v1 page table).

This is a much larger problem. The DMA API domain is created way early
before any drivers are bound. If it doesn't support PASID then no
drivers will get to use PASID at all.

You'd need to figure out some way to switch the DMA API domain on the
fly around when a PASID wanting driver binds. This might be reasonable
since PASID devices tend to need single device groups to work at all
and we could conceivably switch the group under driver control during
early binding.

For now we expect that the DMA API domain will support PASID if the
underyling device supports PASID, that is the only way this can work
today. Meaning domain_alloc_paging() must always return something that
can enable PASID.

>   - UNMANAGED domain : Do not pass *_PASID support flag
>     Since PASID flag is *not* passed, driver will decide best suitable page
>     table (in AMD case, we will allocate V1 page table)

And here this is only VFIO. When you figure out with Alex and Yi how
VFIO will decide to do PASID or not then pass that indication through
the existing flag argument on domain_alloc_user(). At least this is
pretty simple.

But as I said before, the important thing from your perspective is
that VFIO does not default-on PASID support!

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/6/2024 6:04 PM, Jason Gunthorpe wrote:
> On Fri, Aug 02, 2024 at 11:23:52AM +0530, Vasant Hegde wrote:
> 
>>>> +    struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
>>>> +                            u32 iommu_domain_type, u32 flags);
>>>
>>> I still can't see a value to pass the domain type in this callback.
>>> Different domain could have different domain allocation callback, hence
>>> the domain type has already been implied.
> 
> +1
> 
>>> For the paging domain, there should be no difference between DMA and
>>> UNMNANAGED from iommu driver's point of view.
>>
>> That's true. Its all paging domain. But we need a way to indicate the desired
>> capability like PASID.
>  
>> I thoughts we can use `type` for allocating domain and then `flag` to pass the
>> quirks. Otherwise we have to club everything in `flags` itself.
>>
>> Something like below works ?
>>
>>   - DMA-API domain : flag - DOMAIN_ALLOC_FLAG_PASID
>>     If both device and IOMMU supports PASID it will allocate PASID capable
>>     domain (Like AMD case domain with V2 page table). Else it will alloate
>>     non-pasid capable domain (In AMD case it will be domain with v1 page table).
> 
> This is a much larger problem. The DMA API domain is created way early
> before any drivers are bound. If it doesn't support PASID then no
> drivers will get to use PASID at all.
> 
> You'd need to figure out some way to switch the DMA API domain on the
> fly around when a PASID wanting driver binds. This might be reasonable
> since PASID devices tend to need single device groups to work at all
> and we could conceivably switch the group under driver control during
> early binding.
> 
> For now we expect that the DMA API domain will support PASID if the
> underyling device supports PASID, that is the only way this can work
> today. Meaning domain_alloc_paging() must always return something that
> can enable PASID.

This is the fair expectation for DMA API mode. That works for AMD driver as well.

I only have issue with allocating right page table for UNMANAGED domain. In this
case just 'dev' param is not sufficient. Ops should send some sort of indication
that its UNMANAGED domain (like its non DMA API domain). Otherwise AMD driver
cannot allocate right page table.

My understanding is we essentially have two modes for device passthrough.
  1 - Existing interface where vfio driver manages page table (call
iommu_paging_domain_alloc()). No PASID support in this mode.

  With this RFC I am trying to solve this scenario.

  2 - via iommufd - It will use domain_alloc_user() ops. This is the interface
where we want to support PASID w/ passthrough device.

   This is where you/Yi suggested to start discussing with Alex on Yi's PASID
support series. I am running behind. I will start looking into Yi's thread soon.

Is my understanding correct?


> 
>>   - UNMANAGED domain : Do not pass *_PASID support flag
>>     Since PASID flag is *not* passed, driver will decide best suitable page
>>     table (in AMD case, we will allocate V1 page table)
> 
> And here this is only VFIO. When you figure out with Alex and Yi how
> VFIO will decide to do PASID or not then pass that indication through
> the existing flag argument on domain_alloc_user(). At least this is
> pretty simple.

Sure. I believe this  is for scenario 2. I will look into that soon.


-Vasant

> 
> But as I said before, the important thing from your perspective is
> that VFIO does not default-on PASID support!
> 
> Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, Aug 06, 2024 at 08:11:19PM +0530, Vasant Hegde wrote:

> I only have issue with allocating right page table for UNMANAGED domain. In this
> case just 'dev' param is not sufficient. Ops should send some sort of indication
> that its UNMANAGED domain (like its non DMA API domain). Otherwise AMD driver
> cannot allocate right page table.

Be specific, you mean when VFIO allocates it because VFIO is almost
the exclusive user of UNMANAGED domains on x86 hardware.

> My understanding is we essentially have two modes for device passthrough.
>   1 - Existing interface where vfio driver manages page table (call
> iommu_paging_domain_alloc()). No PASID support in this mode.

>   With this RFC I am trying to solve this scenario.

Honestly I'd prefer you change VFIO type 1 to call domain_alloc_user()
instead of hacking domain_alloc_paging().

This is a lot more explicit than trying to push new meaning onto
UNMANAGED which is trying to get itself removed.

>   2 - via iommufd - It will use domain_alloc_user() ops. This is the interface
> where we want to support PASID w/ passthrough device.
> 
>    This is where you/Yi suggested to start discussing with Alex on Yi's PASID
> support series. I am running behind. I will start looking into Yi's thread soon.
> 
> Is my understanding correct?

Broadly, but I'd rather VFIO exclusively use domain_alloc_user() and
it can accomodate both cases. So add a flag to indicate if PASID
should be enabled.

It makes sense to me that type 1 would just always disable PASID
support, it should just be done explicitly without the word
"UNMANAGED".

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Baolu Lu]

On 2024/8/7 1:32, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 08:11:19PM +0530, Vasant Hegde wrote:
> 
>> I only have issue with allocating right page table for UNMANAGED domain. In this
>> case just 'dev' param is not sufficient. Ops should send some sort of indication
>> that its UNMANAGED domain (like its non DMA API domain). Otherwise AMD driver
>> cannot allocate right page table.
> Be specific, you mean when VFIO allocates it because VFIO is almost
> the exclusive user of UNMANAGED domains on x86 hardware.
> 
>> My understanding is we essentially have two modes for device passthrough.
>>    1 - Existing interface where vfio driver manages page table (call
>> iommu_paging_domain_alloc()). No PASID support in this mode.
>>    With this RFC I am trying to solve this scenario.
> Honestly I'd prefer you change VFIO type 1 to call domain_alloc_user()
> instead of hacking domain_alloc_paging().
> 
> This is a lot more explicit than trying to push new meaning onto
> UNMANAGED which is trying to get itself removed.
> 
>>    2 - via iommufd - It will use domain_alloc_user() ops. This is the interface
>> where we want to support PASID w/ passthrough device.
>>
>>     This is where you/Yi suggested to start discussing with Alex on Yi's PASID
>> support series. I am running behind. I will start looking into Yi's thread soon.
>>
>> Is my understanding correct?
> Broadly, but I'd rather VFIO exclusively use domain_alloc_user() and
> it can accomodate both cases. So add a flag to indicate if PASID
> should be enabled.
> 
> It makes sense to me that type 1 would just always disable PASID
> support, it should just be done explicitly without the word
> "UNMANAGED".

Convert paging domain to user domain allocation appears to be a
reasonable solution. But I think it's not VFIO-only, vhost-vdpa is
another case, right?

Thanks,
baolu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/6/2024 11:02 PM, Jason Gunthorpe wrote:
> On Tue, Aug 06, 2024 at 08:11:19PM +0530, Vasant Hegde wrote:
> 
>> I only have issue with allocating right page table for UNMANAGED domain. In this
>> case just 'dev' param is not sufficient. Ops should send some sort of indication
>> that its UNMANAGED domain (like its non DMA API domain). Otherwise AMD driver
>> cannot allocate right page table.
> 
> Be specific, you mean when VFIO allocates it because VFIO is almost
> the exclusive user of UNMANAGED domains on x86 hardware.
> 
>> My understanding is we essentially have two modes for device passthrough.
>>   1 - Existing interface where vfio driver manages page table (call
>> iommu_paging_domain_alloc()). No PASID support in this mode.
> 
>>   With this RFC I am trying to solve this scenario.
> 
> Honestly I'd prefer you change VFIO type 1 to call domain_alloc_user()
> instead of hacking domain_alloc_paging().
> 
> This is a lot more explicit than trying to push new meaning onto
> UNMANAGED which is trying to get itself removed.

Sure. If that works for all driver then we can change that. How about something
like below patch?

> 
>>   2 - via iommufd - It will use domain_alloc_user() ops. This is the interface
>> where we want to support PASID w/ passthrough device.
>>
>>    This is where you/Yi suggested to start discussing with Alex on Yi's PASID
>> support series. I am running behind. I will start looking into Yi's thread soon.
>>
>> Is my understanding correct?
> 
> Broadly, but I'd rather VFIO exclusively use domain_alloc_user() and
> it can accomodate both cases. So add a flag to indicate if PASID
> should be enabled.
> 
> It makes sense to me that type 1 would just always disable PASID
> support, it should just be done explicitly without the word
> "UNMANAGED".

Sure.

> 
> Jason


-------

commit 312c2a99897937834f6a679b6f9956288e8c93de
Author: Vasant Hegde <vasant.hegde@amd.com>
Date:   Wed Aug 7 14:42:24 2024 +0530

    vfio: User iommu_ops->domain_alloc_user() to allocate IOMMU domain

    - Include uapi/linux/iommufd.h in vfio_iommu_type1.c so that we can use
      HWPT flags.
    - Move dev_iommu_ops() from iommu-priv.h to linux/iommu.h

    Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index de5b54eaa8bf..2a052cf4b000 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -6,17 +6,6 @@

 #include <linux/iommu.h>

-static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
-{
-	/*
-	 * Assume that valid ops must be installed if iommu_probe_device()
-	 * has succeeded. The device ops are essentially for internal use
-	 * within the IOMMU subsystem itself, so we should be able to trust
-	 * ourselves not to misuse the helper.
-	 */
-	return dev->iommu->iommu_dev->ops;
-}
-
 const struct iommu_ops *iommu_ops_from_fwnode(const struct fwnode_handle *fwnode);

 static inline const struct iommu_ops *iommu_fwspec_ops(struct iommu_fwspec *fwspec)
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 0960699e7554..56cb1b6312aa 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -37,6 +37,7 @@
 #include <linux/vfio.h>
 #include <linux/workqueue.h>
 #include <linux/notifier.h>
+#include <uapi/linux/iommufd.h>
 #include "vfio.h"

 #define DRIVER_VERSION  "0.2"
@@ -2134,8 +2135,14 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu
*iommu,
 static int vfio_iommu_domain_alloc(struct device *dev, void *data)
 {
 	struct iommu_domain **domain = data;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	u32 flags = 0;
+
+	if (ops->domain_alloc_user)
+		*domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
+	else
+		*domain = iommu_paging_domain_alloc(dev);

-	*domain = iommu_paging_domain_alloc(dev);
 	return 1; /* Don't iterate */
 }

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 72383f6bdd9f..f911c112eead 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1048,6 +1048,17 @@ static inline void *dev_iommu_priv_get(struct device *dev)
 		return NULL;
 }

+static inline const struct iommu_ops *dev_iommu_ops(struct device *dev)
+{
+	/*
+	 * Assume that valid ops must be installed if iommu_probe_device()
+	 * has succeeded. The device ops are essentially for internal use
+	 * within the IOMMU subsystem itself, so we should be able to trust
+	 * ourselves not to misuse the helper.
+	 */
+	return dev->iommu->iommu_dev->ops;
+}
+
 void dev_iommu_priv_set(struct device *dev, void *priv);

 extern struct mutex iommu_probe_device_lock;
diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 4dde745cfb7e..6f622eece1f5 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -359,11 +359,14 @@ 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 : If set, allocate a HWPT that support PASID
+ *
  */
 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,
 };

 /**

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Hi Baolu,


On 8/7/2024 11:19 AM, Baolu Lu wrote:
> On 2024/8/7 1:32, Jason Gunthorpe wrote:
>> On Tue, Aug 06, 2024 at 08:11:19PM +0530, Vasant Hegde wrote:
>>
>>> I only have issue with allocating right page table for UNMANAGED domain. In this
>>> case just 'dev' param is not sufficient. Ops should send some sort of indication
>>> that its UNMANAGED domain (like its non DMA API domain). Otherwise AMD driver
>>> cannot allocate right page table.
>> Be specific, you mean when VFIO allocates it because VFIO is almost
>> the exclusive user of UNMANAGED domains on x86 hardware.
>>
>>> My understanding is we essentially have two modes for device passthrough.
>>>    1 - Existing interface where vfio driver manages page table (call
>>> iommu_paging_domain_alloc()). No PASID support in this mode.
>>>    With this RFC I am trying to solve this scenario.
>> Honestly I'd prefer you change VFIO type 1 to call domain_alloc_user()
>> instead of hacking domain_alloc_paging().
>>
>> This is a lot more explicit than trying to push new meaning onto
>> UNMANAGED which is trying to get itself removed.
>>
>>>    2 - via iommufd - It will use domain_alloc_user() ops. This is the interface
>>> where we want to support PASID w/ passthrough device.
>>>
>>>     This is where you/Yi suggested to start discussing with Alex on Yi's PASID
>>> support series. I am running behind. I will start looking into Yi's thread soon.
>>>
>>> Is my understanding correct?
>> Broadly, but I'd rather VFIO exclusively use domain_alloc_user() and
>> it can accomodate both cases. So add a flag to indicate if PASID
>> should be enabled.
>>
>> It makes sense to me that type 1 would just always disable PASID
>> support, it should just be done explicitly without the word
>> "UNMANAGED".
> 
> Convert paging domain to user domain allocation appears to be a
> reasonable solution. But I think it's not VFIO-only, vhost-vdpa is
> another case, right?

You mean change iommu_paging_domain_alloc() interface and add some sort of the
flag that caller can pass it ?

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 01:49:02PM +0800, Baolu Lu wrote:

> Convert paging domain to user domain allocation appears to be a
> reasonable solution. But I think it's not VFIO-only, vhost-vdpa is
> another case, right?

I suspect vhost-vdpa doesn't use PCI devices with a PASID cap.

Hopefully they will support iommufd before that becomes an issue :)

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 03:00:50PM +0530, Vasant Hegde wrote:
> 
> commit 312c2a99897937834f6a679b6f9956288e8c93de
> Author: Vasant Hegde <vasant.hegde@amd.com>
> Date:   Wed Aug 7 14:42:24 2024 +0530
> 
>     vfio: User iommu_ops->domain_alloc_user() to allocate IOMMU domain
> 
>     - Include uapi/linux/iommufd.h in vfio_iommu_type1.c so that we can use
>       HWPT flags.
>     - Move dev_iommu_ops() from iommu-priv.h to linux/iommu.h
> 
>     Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Yeah, this seems like a good start

> @@ -2134,8 +2135,14 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu
> *iommu,
>  static int vfio_iommu_domain_alloc(struct device *dev, void *data)
>  {
>  	struct iommu_domain **domain = data;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	u32 flags = 0;
> +
> +	if (ops->domain_alloc_user)
> +		*domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> +	else
> +		*domain = iommu_paging_domain_alloc(dev);

I suggest to wrapper this in a EXPORT_SYMBOL rather than expose
dev_iommu_ops ?

> @@ -359,11 +359,14 @@ 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 : If set, allocate a HWPT that support PASID
> + *
>   */

Needs more words, maybe even two flags?

 @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
 with no PASID, the device will support later attaching a PASID as
 well. Some HW requires a specific domain format on the device to
 allow PASID to work.

 @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
 domain attached to the device must have also been allocated with
 IOMMU_HWPT_ALLOC_DEV_PASID.

And then we'd want to enforce this rule in iommufd to force userspace
to use the flags properly. Otherwise AMD will be permanently broken :\

I'm guessing that userspace will never want to share a PASID domain
and a device domain, and current iommu HW will treat both flags as the
same functionality (use the AMD v2 domain)

Yi will need to accomodate these flags in his series..

SMMUv3 support would look like this:

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 9bc50bded5af72..c2e46a352d6b09 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3052,7 +3052,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
                           const struct iommu_user_data *user_data)
 {
        struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+                                IOMMU_HWPT_ALLOC_DEV_PASID |
+                                IOMMU_HWPT_ALLOC_PASID;
        struct arm_smmu_domain *smmu_domain;
        int ret;
 
@@ -3065,6 +3067,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
        if (!smmu_domain)
                return ERR_PTR(-ENOMEM);
 
+       if (flags & (IOMMU_HWPT_ALLOC_DEV_PASID | IOMMU_HWPT_ALLOC_PASID)) {
+               if (!(master->smmu->features & ARM_SMMU_FEAT_TRANS_S1)) {
+                       ret = -EOPNOTSUPP;
+                       goto err_free;
+               }
+               smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+       }
+
        smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
        smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
        ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);

(though this will clash with the nesting series I just sent)

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/7/2024 7:29 PM, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 03:00:50PM +0530, Vasant Hegde wrote:
>>
>> commit 312c2a99897937834f6a679b6f9956288e8c93de
>> Author: Vasant Hegde <vasant.hegde@amd.com>
>> Date:   Wed Aug 7 14:42:24 2024 +0530
>>
>>     vfio: User iommu_ops->domain_alloc_user() to allocate IOMMU domain
>>
>>     - Include uapi/linux/iommufd.h in vfio_iommu_type1.c so that we can use
>>       HWPT flags.
>>     - Move dev_iommu_ops() from iommu-priv.h to linux/iommu.h
>>
>>     Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> Yeah, this seems like a good start
> 
>> @@ -2134,8 +2135,14 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu
>> *iommu,
>>  static int vfio_iommu_domain_alloc(struct device *dev, void *data)
>>  {
>>  	struct iommu_domain **domain = data;
>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +	u32 flags = 0;
>> +
>> +	if (ops->domain_alloc_user)
>> +		*domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
>> +	else
>> +		*domain = iommu_paging_domain_alloc(dev);
> 
> I suggest to wrapper this in a EXPORT_SYMBOL rather than expose
> dev_iommu_ops ?

Sure. We can do that.

> 
>> @@ -359,11 +359,14 @@ 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 : If set, allocate a HWPT that support PASID
>> + *
>>   */
> 
> Needs more words, maybe even two flags?
> 
>  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
>  with no PASID, the device will support later attaching a PASID as
>  well. Some HW requires a specific domain format on the device to
>  allow PASID to work.

I will add this.

> 
>  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
>  domain attached to the device must have also been allocated with
>  IOMMU_HWPT_ALLOC_DEV_PASID.

Why do we need this flag? Previous one is already allocated PASID capable
domain. Is this similar to SVA domain?

> 
> And then we'd want to enforce this rule in iommufd to force userspace
> to use the flags properly. Otherwise AMD will be permanently broken :\
> 
> I'm guessing that userspace will never want to share a PASID domain
> and a device domain, and current iommu HW will treat both flags as the
> same functionality (use the AMD v2 domain)
> 
> Yi will need to accomodate these flags in his series..
> 
> SMMUv3 support would look like this:

Thanks! I will put it as part of this series.


@Baolu, Do we need to make any changes to intel driver?

> 
> 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 9bc50bded5af72..c2e46a352d6b09 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3052,7 +3052,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>                            const struct iommu_user_data *user_data)
>  {
>         struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> -       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
> +                                IOMMU_HWPT_ALLOC_DEV_PASID |
> +                                IOMMU_HWPT_ALLOC_PASID;
>         struct arm_smmu_domain *smmu_domain;
>         int ret;
>  
> @@ -3065,6 +3067,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>         if (!smmu_domain)
>                 return ERR_PTR(-ENOMEM);
>  
> +       if (flags & (IOMMU_HWPT_ALLOC_DEV_PASID | IOMMU_HWPT_ALLOC_PASID)) {
> +               if (!(master->smmu->features & ARM_SMMU_FEAT_TRANS_S1)) {
> +                       ret = -EOPNOTSUPP;
> +                       goto err_free;
> +               }
> +               smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
> +       }
> +
>         smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
>         smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
>         ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);
> 
> (though this will clash with the nesting series I just sent)

If your series goes first then I can rebase it. Lets see.

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
> > Needs more words, maybe even two flags?
> > 
> >  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
> >  with no PASID, the device will support later attaching a PASID as
> >  well. Some HW requires a specific domain format on the device to
> >  allow PASID to work.
> 
> I will add this.
>
> >  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
> >  domain attached to the device must have also been allocated with
> >  IOMMU_HWPT_ALLOC_DEV_PASID.
> 
> Why do we need this flag? Previous one is already allocated PASID capable
> domain. Is this similar to SVA domain?

IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
domain you will attach to the RID

IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to attach
this domain to a PASID

They are actually different operations from a user perspective. Today
every iommu driver implements them with the same logic..

So I thought perhaps they should be seperate, at least if we get some
really crazy HW down the road the uAPI will still work. uAPI is a bit
more tricky because you can't change it.

> @Baolu, Do we need to make any changes to intel driver?

I think Intel ignores this issue, they have nice HW and their RID can
use both of their supported domain formats in all cases.

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 03:29:47PM -0300, Jason Gunthorpe wrote:
> > @Baolu, Do we need to make any changes to intel driver?
> 
> I think Intel ignores this issue, they have nice HW and their RID can
> use both of their supported domain formats in all cases.

On second thought it does need to accept, but ignore the new flags..

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 07, 2024 at 10:16:29PM -0300, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 03:29:47PM -0300, Jason Gunthorpe wrote:
> > > @Baolu, Do we need to make any changes to intel driver?
> > 
> > I think Intel ignores this issue, they have nice HW and their RID can
> > use both of their supported domain formats in all cases.
> 
> On second thought it does need to accept, but ignore the new flags..

And probably HW that does not support PASID should reject the
IOMMU_HWPT_ALLOC_PASID flag at least.

Maybe IOMMU_HWPT_ALLOC_DEV_PASID can be reasonably ignored if the HW
doesn't support PASID.

That would mean the SMMUv3 hunk I sent doesn't have quite the right
validation.

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,

On 8/7/2024 11:59 PM, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
>>> Needs more words, maybe even two flags?
>>>
>>>  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
>>>  with no PASID, the device will support later attaching a PASID as
>>>  well. Some HW requires a specific domain format on the device to
>>>  allow PASID to work.
>>
>> I will add this.
>>
>>>  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
>>>  domain attached to the device must have also been allocated with
>>>  IOMMU_HWPT_ALLOC_DEV_PASID.
>>
>> Why do we need this flag? Previous one is already allocated PASID capable
>> domain. Is this similar to SVA domain?
> 
> IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
> domain you will attach to the RID
> 
> IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to attach
> this domain to a PASID
> 
> They are actually different operations from a user perspective. Today
> every iommu driver implements them with the same logic..
> 
> So I thought perhaps they should be seperate, at least if we get some
> really crazy HW down the road the uAPI will still work. uAPI is a bit
> more tricky because you can't change it.

Oh yeah. uAPI is always tricky. No harm in adding extra flag in advance. I will
update the patch.

-Vasant

> 
>> @Baolu, Do we need to make any changes to intel driver?
> 
> I think Intel ignores this issue, they have nice HW and their RID can
> use both of their supported domain formats in all cases.
> 
> Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/8/2024 6:38 PM, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 10:16:29PM -0300, Jason Gunthorpe wrote:
>> On Wed, Aug 07, 2024 at 03:29:47PM -0300, Jason Gunthorpe wrote:
>>>> @Baolu, Do we need to make any changes to intel driver?
>>>
>>> I think Intel ignores this issue, they have nice HW and their RID can
>>> use both of their supported domain formats in all cases.
>>
>> On second thought it does need to accept, but ignore the new flags..
> 
> And probably HW that does not support PASID should reject the
> IOMMU_HWPT_ALLOC_PASID flag at least.


You mean error out if IOMMU or device doesnot support PASID?


> 
> Maybe IOMMU_HWPT_ALLOC_DEV_PASID can be reasonably ignored if the HW
> doesn't support PASID.


You mean make it as optional flag? Meaning if HW doesn't support PASID then
still allocate a domain without PASID capability?


-Vasant

> 
> That would mean the SMMUv3 hunk I sent doesn't have quite the right
> validation.
> 
> Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Fri, Aug 09, 2024 at 12:13:47PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/8/2024 6:38 PM, Jason Gunthorpe wrote:
> > On Wed, Aug 07, 2024 at 10:16:29PM -0300, Jason Gunthorpe wrote:
> >> On Wed, Aug 07, 2024 at 03:29:47PM -0300, Jason Gunthorpe wrote:
> >>>> @Baolu, Do we need to make any changes to intel driver?
> >>>
> >>> I think Intel ignores this issue, they have nice HW and their RID can
> >>> use both of their supported domain formats in all cases.
> >>
> >> On second thought it does need to accept, but ignore the new flags..
> > 
> > And probably HW that does not support PASID should reject the
> > IOMMU_HWPT_ALLOC_PASID flag at least.
> 
> 
> You mean error out if IOMMU or device doesnot support PASID?

Just the iommu, yes.

Like in AMD terms if there is no AMDv2 page table support then fail
it.

> > Maybe IOMMU_HWPT_ALLOC_DEV_PASID can be reasonably ignored if the HW
> > doesn't support PASID.
>  
> You mean make it as optional flag? Meaning if HW doesn't support PASID then
> still allocate a domain without PASID capability?

Yes, for the RID domain it seems harmless to ignore, I'm not really
sure on this though.

Otherwise we'd want to report pasid support through a INFO return so
userspace knows if it should specify this flag.

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/9/2024 7:14 PM, Jason Gunthorpe wrote:
> On Fri, Aug 09, 2024 at 12:13:47PM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 8/8/2024 6:38 PM, Jason Gunthorpe wrote:
>>> On Wed, Aug 07, 2024 at 10:16:29PM -0300, Jason Gunthorpe wrote:
>>>> On Wed, Aug 07, 2024 at 03:29:47PM -0300, Jason Gunthorpe wrote:
>>>>>> @Baolu, Do we need to make any changes to intel driver?
>>>>>
>>>>> I think Intel ignores this issue, they have nice HW and their RID can
>>>>> use both of their supported domain formats in all cases.
>>>>
>>>> On second thought it does need to accept, but ignore the new flags..
>>>
>>> And probably HW that does not support PASID should reject the
>>> IOMMU_HWPT_ALLOC_PASID flag at least.
>>
>>
>> You mean error out if IOMMU or device doesnot support PASID?
> 
> Just the iommu, yes.
> 
> Like in AMD terms if there is no AMDv2 page table support then fail
> it.
> 
>>> Maybe IOMMU_HWPT_ALLOC_DEV_PASID can be reasonably ignored if the HW
>>> doesn't support PASID.
>>  
>> You mean make it as optional flag? Meaning if HW doesn't support PASID then
>> still allocate a domain without PASID capability?
> 
> Yes, for the RID domain it seems harmless to ignore, I'm not really
> sure on this though.
> 
> Otherwise we'd want to report pasid support through a INFO return so
> userspace knows if it should specify this flag.

Let me summarize and see if I got everything right.

- Enhance iommufd_get_hw_info ioctl to get device PASID capability
  HW driver will implement iommu_ops->capable() to check PASID capability

- Userspace will use above info and calls iommu_ops->domain_alloc_user() with
  appropriate flag (If PASID capability present then pass
  IOMMU_HWPT_ALLOC_DEV_PASID as flag).

- HW driver : If IOMMU and dev supports PASID then allocate PASID capable
  domain, else allocate normal domain.

- When attaching PASID to device (RID), PASID attachment will pass only domain
supports PASID, else it will fail.

- IOMMU_HWPT_ALLOC_PASID  flag is optional:
  If we endup implementing this flag, then it will allocate PASID capable domain
if HW IOMMU supports it (It will not check device capability).


Does this flow makes sense?

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Yi Liu]

On 2024/8/7 21:59, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 03:00:50PM +0530, Vasant Hegde wrote:
>>
>> commit 312c2a99897937834f6a679b6f9956288e8c93de
>> Author: Vasant Hegde <vasant.hegde@amd.com>
>> Date:   Wed Aug 7 14:42:24 2024 +0530
>>
>>      vfio: User iommu_ops->domain_alloc_user() to allocate IOMMU domain
>>
>>      - Include uapi/linux/iommufd.h in vfio_iommu_type1.c so that we can use
>>        HWPT flags.
>>      - Move dev_iommu_ops() from iommu-priv.h to linux/iommu.h
>>
>>      Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> 
> Yeah, this seems like a good start
> 
>> @@ -2134,8 +2135,14 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu
>> *iommu,
>>   static int vfio_iommu_domain_alloc(struct device *dev, void *data)
>>   {
>>   	struct iommu_domain **domain = data;
>> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
>> +	u32 flags = 0;
>> +
>> +	if (ops->domain_alloc_user)
>> +		*domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
>> +	else
>> +		*domain = iommu_paging_domain_alloc(dev);
> 
> I suggest to wrapper this in a EXPORT_SYMBOL rather than expose
> dev_iommu_ops ?
> 
>> @@ -359,11 +359,14 @@ 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 : If set, allocate a HWPT that support PASID
>> + *
>>    */
> 
> Needs more words, maybe even two flags?
> 
>   @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
>   with no PASID, the device will support later attaching a PASID as
>   well. Some HW requires a specific domain format on the device to
>   allow PASID to work.
> 
>   @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
>   domain attached to the device must have also been allocated with
>   IOMMU_HWPT_ALLOC_DEV_PASID.
> 
> And then we'd want to enforce this rule in iommufd to force userspace
> to use the flags properly. Otherwise AMD will be permanently broken :\
> 
> I'm guessing that userspace will never want to share a PASID domain
> and a device domain, and current iommu HW will treat both flags as the
> same functionality (use the AMD v2 domain)
> 
> Yi will need to accomodate these flags in his series..

sure. This appears to be a part of the iommufd pasid series. :)


-- 
Regards,
Yi Liu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Yi Liu]

On 2024/8/8 02:29, Jason Gunthorpe wrote:
> On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
>>> Needs more words, maybe even two flags?
>>>
>>>   @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a device,
>>>   with no PASID, the device will support later attaching a PASID as
>>>   well. Some HW requires a specific domain format on the device to
>>>   allow PASID to work.
>>
>> I will add this.
>>
>>>   @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
>>>   domain attached to the device must have also been allocated with
>>>   IOMMU_HWPT_ALLOC_DEV_PASID.
>>
>> Why do we need this flag? Previous one is already allocated PASID capable
>> domain. Is this similar to SVA domain?
> 
> IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
> domain you will attach to the RID
> 
> IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to attach
> this domain to a PASID
> 
> They are actually different operations from a user perspective. Today
> every iommu driver implements them with the same logic..
> 
> So I thought perhaps they should be seperate, at least if we get some
> really crazy HW down the road the uAPI will still work. uAPI is a bit
> more tricky because you can't change it.

Hi Jason,

could you map the above two flags to the actual usage a bit? I'm not
quite getting why we need two flags here as well. :(

-- 
Regards,
Yi Liu

RE: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, August 8, 2024 2:30 AM
> 
> On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
> > > Needs more words, maybe even two flags?
> > >
> > >  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a
> device,
> > >  with no PASID, the device will support later attaching a PASID as
> > >  well. Some HW requires a specific domain format on the device to
> > >  allow PASID to work.
> >
> > I will add this.
> >
> > >  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID.
> The
> > >  domain attached to the device must have also been allocated with
> > >  IOMMU_HWPT_ALLOC_DEV_PASID.
> >
> > Why do we need this flag? Previous one is already allocated PASID capable
> > domain. Is this similar to SVA domain?
> 
> IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
> domain you will attach to the RID
> 
> IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to attach
> this domain to a PASID
> 
> They are actually different operations from a user perspective. Today
> every iommu driver implements them with the same logic..
> 
> So I thought perhaps they should be seperate, at least if we get some
> really crazy HW down the road the uAPI will still work. uAPI is a bit
> more tricky because you can't change it.
> 

I'm not sure it's necessary to push this burden to all existing IOMMUs
which don't have such requirement.

Can we simplify it by just supporting one flag and then incrementally
add another one when a future demand comes and advocate such
new requirement by GET_HW_INFO?

Honestly speaking it's not very intuitive to understand the rationale
behind having two flags and I don't think asking for change in userspace
to support really crazy HW is an unreasonable request comparing
to the burden maintaining a heavier uAPI for things which may never
happen. 😊

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, Aug 13, 2024 at 09:40:32AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Thursday, August 8, 2024 2:30 AM
> > 
> > On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
> > > > Needs more words, maybe even two flags?
> > > >
> > > >  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a
> > device,
> > > >  with no PASID, the device will support later attaching a PASID as
> > > >  well. Some HW requires a specific domain format on the device to
> > > >  allow PASID to work.
> > >
> > > I will add this.
> > >
> > > >  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID.
> > The
> > > >  domain attached to the device must have also been allocated with
> > > >  IOMMU_HWPT_ALLOC_DEV_PASID.
> > >
> > > Why do we need this flag? Previous one is already allocated PASID capable
> > > domain. Is this similar to SVA domain?
> > 
> > IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
> > domain you will attach to the RID
> > 
> > IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to attach
> > this domain to a PASID
> > 
> > They are actually different operations from a user perspective. Today
> > every iommu driver implements them with the same logic..
> > 
> > So I thought perhaps they should be seperate, at least if we get some
> > really crazy HW down the road the uAPI will still work. uAPI is a bit
> > more tricky because you can't change it.
> > 
> 
> I'm not sure it's necessary to push this burden to all existing IOMMUs
> which don't have such requirement.

If we are making a common uAPI then we need to have all the HW act the
same. No implicit behaviors that people accidently rely on.

So if we say you need flags before domains can be used with PASID then
the core code must check those flags for all HW even if it is a
driver-NOP.

So what are the flags you want?

Flags = 0 means the domain cannot be attached to a PASID
Flags = 0 means the domain cannot be attached to the RID while PASID
is in use

The core code should enforce this rule always.

Above that we minimally need

Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
PASID

And you need to decide if the RID usage is bundled with the above as
well.

> Can we simplify it by just supporting one flag and then incrementally
> add another one when a future demand comes and advocate such
> new requirement by GET_HW_INFO?

Well you end up not supporting any special cases that would need
it. It is uAPI so it is good to always be mindful that things are well
defined and clear.

If you want one flag then it still has to do both behaviors and you
still have to describe and enforce both behaviors. So I don't know
there is a lot of savings, or that it is clearer.

I hope nobody ever needs two flags, that would be terrible HW..

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Mon, Aug 12, 2024 at 02:51:56PM +0530, Vasant Hegde wrote:

> Let me summarize and see if I got everything right.
> 
> - Enhance iommufd_get_hw_info ioctl to get device PASID capability
>   HW driver will implement iommu_ops->capable() to check PASID capability
>
> - Userspace will use above info and calls iommu_ops->domain_alloc_user() with
>   appropriate flag (If PASID capability present then pass
>   IOMMU_HWPT_ALLOC_DEV_PASID as flag).
> 
> - HW driver : If IOMMU and dev supports PASID then allocate PASID capable
>   domain, else allocate normal domain.
> 
> - When attaching PASID to device (RID), PASID attachment will pass only domain
> supports PASID, else it will fail.
> 
> - IOMMU_HWPT_ALLOC_PASID  flag is optional:
>   If we endup implementing this flag, then it will allocate PASID capable domain
> if HW IOMMU supports it (It will not check device capability).

So how do you get a PASID domain with only one flag? You specify
IOMMU_HWPT_ALLOC_DEV_PASID and expect it to work for PASID attach too?

Jason

RE: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 14, 2024 12:21 AM
> 
> On Tue, Aug 13, 2024 at 09:40:32AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > Sent: Thursday, August 8, 2024 2:30 AM
> > >
> > > On Wed, Aug 07, 2024 at 10:22:00PM +0530, Vasant Hegde wrote:
> > > > > Needs more words, maybe even two flags?
> > > > >
> > > > >  @IOMMU_HWPT_ALLOC_DEV_PASID: When the domain is used on a
> > > device,
> > > > >  with no PASID, the device will support later attaching a PASID as
> > > > >  well. Some HW requires a specific domain format on the device to
> > > > >  allow PASID to work.
> > > >
> > > > I will add this.
> > > >
> > > > >  @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID.
> > > The
> > > > >  domain attached to the device must have also been allocated with
> > > > >  IOMMU_HWPT_ALLOC_DEV_PASID.
> > > >
> > > > Why do we need this flag? Previous one is already allocated PASID
> capable
> > > > domain. Is this similar to SVA domain?
> > >
> > > IOMMU_HWPT_ALLOC_DEV_PASID is the flag you specify to allocate the
> > > domain you will attach to the RID
> > >
> > > IOMMU_HWPT_ALLOC_PASID is the flag you specify if you intend to
> attach
> > > this domain to a PASID
> > >
> > > They are actually different operations from a user perspective. Today
> > > every iommu driver implements them with the same logic..
> > >
> > > So I thought perhaps they should be seperate, at least if we get some
> > > really crazy HW down the road the uAPI will still work. uAPI is a bit
> > > more tricky because you can't change it.
> > >
> >
> > I'm not sure it's necessary to push this burden to all existing IOMMUs
> > which don't have such requirement.
> 
> If we are making a common uAPI then we need to have all the HW act the
> same. No implicit behaviors that people accidently rely on.
> 
> So if we say you need flags before domains can be used with PASID then
> the core code must check those flags for all HW even if it is a
> driver-NOP.
> 
> So what are the flags you want?
> 
> Flags = 0 means the domain cannot be attached to a PASID
> Flags = 0 means the domain cannot be attached to the RID while PASID
> is in use

From my understanding there are only two type of formats so far:

  - a format only working with RID (V1 in AMD, S2 in ARM)
  - a format working with both RID and PASID (V2 in AMD, S1 in ARM,
   S1/S2 in Intel)

With that in mind we only need one bit to indicate whether a domain
allows PASID attach. The user learns the PASID capability via
GET_HW_INFO and should set or clear the PASID flag consistently
for all non-nesting domains attached to a same device (RID, or RID+PASID).

for AMD/ARM this consistency is mandatory as one device can only be
configured to a single format. no mix.

for Intel there's more flexibility but I don't see a reason to mix S1/S2
between RID and PASID, and now both are fixed to S2 for UNMANAGED.

> 
> The core code should enforce this rule always.
> 
> Above that we minimally need
> 
> Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
> PASID
> 
> And you need to decide if the RID usage is bundled with the above as
> well.

A single bit IOMMU_HWPT_ALLOC_PASID is what I meant. Setting it
means the domain can be attached to both RID and PASID, i.e. RID
attach is always implied.

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/13/2024 9:52 PM, Jason Gunthorpe wrote:
> On Mon, Aug 12, 2024 at 02:51:56PM +0530, Vasant Hegde wrote:
> 
>> Let me summarize and see if I got everything right.
>>
>> - Enhance iommufd_get_hw_info ioctl to get device PASID capability
>>   HW driver will implement iommu_ops->capable() to check PASID capability
>>
>> - Userspace will use above info and calls iommu_ops->domain_alloc_user() with
>>   appropriate flag (If PASID capability present then pass
>>   IOMMU_HWPT_ALLOC_DEV_PASID as flag).
>>
>> - HW driver : If IOMMU and dev supports PASID then allocate PASID capable
>>   domain, else allocate normal domain.
>>
>> - When attaching PASID to device (RID), PASID attachment will pass only domain
>> supports PASID, else it will fail.
>>
>> - IOMMU_HWPT_ALLOC_PASID  flag is optional:
>>   If we endup implementing this flag, then it will allocate PASID capable domain
>> if HW IOMMU supports it (It will not check device capability).
> 
> So how do you get a PASID domain with only one flag? You specify
> IOMMU_HWPT_ALLOC_DEV_PASID and expect it to work for PASID attach too?

Yes. I am thinking IOMMU_HWPT_ALLOC_DEV_PASID will check both device and IOMMU
capability. So later if we want to attach PASID it should work fine.

Are you thinking something different?

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Aug 14, 2024 at 02:38:58AM +0000, Tian, Kevin wrote:

> > Flags = 0 means the domain cannot be attached to a PASID
> > Flags = 0 means the domain cannot be attached to the RID while PASID
> > is in use
> 
> From my understanding there are only two type of formats so far:
> 
>   - a format only working with RID (V1 in AMD, S2 in ARM)
>   - a format working with both RID and PASID (V2 in AMD, S1 in ARM,
>    S1/S2 in Intel)
> 
> With that in mind we only need one bit to indicate whether a domain
> allows PASID attach. The user learns the PASID capability via
> GET_HW_INFO and should set or clear the PASID flag consistently
> for all non-nesting domains attached to a same device (RID, or RID+PASID).
> 
> for AMD/ARM this consistency is mandatory as one device can only be
> configured to a single format. no mix.
> 
> for Intel there's more flexibility but I don't see a reason to mix S1/S2
> between RID and PASID, and now both are fixed to S2 for UNMANAGED.
> 
> > 
> > The core code should enforce this rule always.
> > 
> > Above that we minimally need
> > 
> > Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
> > PASID
> > 
> > And you need to decide if the RID usage is bundled with the above as
> > well.
> 
> A single bit IOMMU_HWPT_ALLOC_PASID is what I meant. Setting it
> means the domain can be attached to both RID and PASID, i.e. RID
> attach is always implied.

OK, lets try it like that then

I also know of no HW that would need it to be different on the RID

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason, Kevin,


On 8/15/2024 4:10 AM, Jason Gunthorpe wrote:
> On Wed, Aug 14, 2024 at 02:38:58AM +0000, Tian, Kevin wrote:
> 
>>> Flags = 0 means the domain cannot be attached to a PASID
>>> Flags = 0 means the domain cannot be attached to the RID while PASID
>>> is in use
>>
>> From my understanding there are only two type of formats so far:
>>
>>   - a format only working with RID (V1 in AMD, S2 in ARM)
>>   - a format working with both RID and PASID (V2 in AMD, S1 in ARM,
>>    S1/S2 in Intel)
>>
>> With that in mind we only need one bit to indicate whether a domain
>> allows PASID attach. The user learns the PASID capability via
>> GET_HW_INFO and should set or clear the PASID flag consistently
>> for all non-nesting domains attached to a same device (RID, or RID+PASID).
>>
>> for AMD/ARM this consistency is mandatory as one device can only be
>> configured to a single format. no mix.
>>
>> for Intel there's more flexibility but I don't see a reason to mix S1/S2
>> between RID and PASID, and now both are fixed to S2 for UNMANAGED.
>>
>>>
>>> The core code should enforce this rule always.
>>>
>>> Above that we minimally need
>>>
>>> Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
>>> PASID
>>>
>>> And you need to decide if the RID usage is bundled with the above as
>>> well.
>>
>> A single bit IOMMU_HWPT_ALLOC_PASID is what I meant. Setting it
>> means the domain can be attached to both RID and PASID, i.e. RID
>> attach is always implied.
> 
> OK, lets try it like that then
> 
> I also know of no HW that would need it to be different on the RID

Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
domain if both IOMMU and device supports the feature. If not allocate normal
domain -OR- return error?

Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
PASID capability. Is that correct?

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Yi Liu]

On 2024/8/15 11:28, Vasant Hegde wrote:
> Jason, Kevin,
> 
> 
> On 8/15/2024 4:10 AM, Jason Gunthorpe wrote:
>> On Wed, Aug 14, 2024 at 02:38:58AM +0000, Tian, Kevin wrote:
>>
>>>> Flags = 0 means the domain cannot be attached to a PASID
>>>> Flags = 0 means the domain cannot be attached to the RID while PASID
>>>> is in use
>>>
>>>  From my understanding there are only two type of formats so far:
>>>
>>>    - a format only working with RID (V1 in AMD, S2 in ARM)
>>>    - a format working with both RID and PASID (V2 in AMD, S1 in ARM,
>>>     S1/S2 in Intel)
>>>
>>> With that in mind we only need one bit to indicate whether a domain
>>> allows PASID attach. The user learns the PASID capability via
>>> GET_HW_INFO and should set or clear the PASID flag consistently
>>> for all non-nesting domains attached to a same device (RID, or RID+PASID).
>>>
>>> for AMD/ARM this consistency is mandatory as one device can only be
>>> configured to a single format. no mix.
>>>
>>> for Intel there's more flexibility but I don't see a reason to mix S1/S2
>>> between RID and PASID, and now both are fixed to S2 for UNMANAGED.
>>>
>>>>
>>>> The core code should enforce this rule always.
>>>>
>>>> Above that we minimally need
>>>>
>>>> Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
>>>> PASID
>>>>
>>>> And you need to decide if the RID usage is bundled with the above as
>>>> well.
>>>
>>> A single bit IOMMU_HWPT_ALLOC_PASID is what I meant. Setting it
>>> means the domain can be attached to both RID and PASID, i.e. RID
>>> attach is always implied.
>>
>> OK, lets try it like that then
>>
>> I also know of no HW that would need it to be different on the RID
> 
> Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
> domain if both IOMMU and device supports the feature. If not allocate normal
> domain -OR- return error?
> 
> Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
> PASID capability. Is that correct?

yes as the below link. I would include it in the iommufd pasid series. For
the flag mentioned here, looks like Jason wants it to be in the iommufd
pasid series as well. I would incorporate it then. :)

https://lore.kernel.org/linux-iommu/b37a7336-36af-4ffc-a50f-c9b578cd9bda@intel.com/#t

-- 
Regards,
Yi Liu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Baolu Lu]

On 2024/8/15 11:28, Vasant Hegde wrote:
> Jason, Kevin,
> 
> 
> On 8/15/2024 4:10 AM, Jason Gunthorpe wrote:
>> On Wed, Aug 14, 2024 at 02:38:58AM +0000, Tian, Kevin wrote:
>>
>>>> Flags = 0 means the domain cannot be attached to a PASID
>>>> Flags = 0 means the domain cannot be attached to the RID while PASID
>>>> is in use
>>>  From my understanding there are only two type of formats so far:
>>>
>>>    - a format only working with RID (V1 in AMD, S2 in ARM)
>>>    - a format working with both RID and PASID (V2 in AMD, S1 in ARM,
>>>     S1/S2 in Intel)
>>>
>>> With that in mind we only need one bit to indicate whether a domain
>>> allows PASID attach. The user learns the PASID capability via
>>> GET_HW_INFO and should set or clear the PASID flag consistently
>>> for all non-nesting domains attached to a same device (RID, or RID+PASID).
>>>
>>> for AMD/ARM this consistency is mandatory as one device can only be
>>> configured to a single format. no mix.
>>>
>>> for Intel there's more flexibility but I don't see a reason to mix S1/S2
>>> between RID and PASID, and now both are fixed to S2 for UNMANAGED.
>>>
>>>> The core code should enforce this rule always.
>>>>
>>>> Above that we minimally need
>>>>
>>>> Flags = IOMMU_HWPT_ALLOC_PASID allows the domain to be attached to a
>>>> PASID
>>>>
>>>> And you need to decide if the RID usage is bundled with the above as
>>>> well.
>>> A single bit IOMMU_HWPT_ALLOC_PASID is what I meant. Setting it
>>> means the domain can be attached to both RID and PASID, i.e. RID
>>> attach is always implied.
>> OK, lets try it like that then
>>
>> I also know of no HW that would need it to be different on the RID
> Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
> domain if both IOMMU and device supports the feature. If not allocate normal
> domain -OR- return error?
> 
> Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
> PASID capability. Is that correct?

I suppose that this also includes converting paging domain to user
domain allocation in VFIO (ideally vhost-vdpa as well), right?

Thanks,
baolu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Thu, Aug 15, 2024 at 08:58:14AM +0530, Vasant Hegde wrote:
> > I also know of no HW that would need it to be different on the RID
> 
> Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
> domain if both IOMMU and device supports the feature. If not allocate normal
> domain -OR- return error?

I think fail if the IOMMU does not support PASID.

> Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
> PASID capability. Is that correct?

Yes. Some flag to indicate the device and iommu together are able to
support PASID.

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Thu, Aug 15, 2024 at 12:58:41PM +0800, Yi Liu wrote:
> > Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
> > domain if both IOMMU and device supports the feature. If not allocate normal
> > domain -OR- return error?
> > 
> > Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
> > PASID capability. Is that correct?
> 
> yes as the below link. I would include it in the iommufd pasid series. For
> the flag mentioned here, looks like Jason wants it to be in the iommufd
> pasid series as well. I would incorporate it then. :)
> 
> https://lore.kernel.org/linux-iommu/b37a7336-36af-4ffc-a50f-c9b578cd9bda@intel.com/#t

You two should coordinate some submission here :)

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
> I suppose that this also includes converting paging domain to user
> domain allocation in VFIO (ideally vhost-vdpa as well), right?

Another thought is to add a flag to iommu_paging_domain_alloc()..

Maybe like the below. The small duplication with
__iommu_domain_alloc() would go away when iommu_domain_alloc() finally
goes, then we'd be able to directly call iommu_paging_domain_alloc().

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ed6c5cb60c5aee..7c028d036e8a1a 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;
 }
 
@@ -2031,20 +2027,40 @@ 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 = dev_iommu_ops(dev);
+	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);
+	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)
 {
@@ -2965,6 +2981,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
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 4d47f2c3331185..8648be89834014 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);
@@ -1098,6 +1102,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);

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
> On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
>> I suppose that this also includes converting paging domain to user
>> domain allocation in VFIO (ideally vhost-vdpa as well), right?
> 
> Another thought is to add a flag to iommu_paging_domain_alloc()..
> 
> Maybe like the below. The small duplication with
> __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
> goes, then we'd be able to directly call iommu_paging_domain_alloc().

This will work. But we have to define NO_PASID flag and pass it from vfio.

IOMMU_HWPT_ALLOC_NO_PASID
IOMMU_HWPT_ALLOC_PASID

is that fine?

-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Yi Liu]

On 2024/8/15 21:06, Jason Gunthorpe wrote:
> On Thu, Aug 15, 2024 at 12:58:41PM +0800, Yi Liu wrote:
>>> Ok. Single macro with `IOMMU_HWPT_ALLOC_PASID` which allocates PASID capable
>>> domain if both IOMMU and device supports the feature. If not allocate normal
>>> domain -OR- return error?
>>>
>>> Also I would assume we want to enhance iommufd_get_hw_info ioctl to get device
>>> PASID capability. Is that correct?
>>
>> yes as the below link. I would include it in the iommufd pasid series. For
>> the flag mentioned here, looks like Jason wants it to be in the iommufd
>> pasid series as well. I would incorporate it then. :)
>>
>> https://lore.kernel.org/linux-iommu/b37a7336-36af-4ffc-a50f-c9b578cd9bda@intel.com/#t
> 
> You two should coordinate some submission here :)

yeah, will work with Vasant on it when this thread is finalized.

-- 
Regards,
Yi Liu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Yi Liu]

On 2024/8/16 16:17, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
>> On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
>>> I suppose that this also includes converting paging domain to user
>>> domain allocation in VFIO (ideally vhost-vdpa as well), right?
>>
>> Another thought is to add a flag to iommu_paging_domain_alloc()..
>>
>> Maybe like the below. The small duplication with
>> __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
>> goes, then we'd be able to directly call iommu_paging_domain_alloc().
> 
> This will work. But we have to define NO_PASID flag and pass it from vfio.
> 
> IOMMU_HWPT_ALLOC_NO_PASID
> IOMMU_HWPT_ALLOC_PASID
> 
> is that fine?

this flag should be set by userspace. is it? I don't know how to pass it
in the existing vfio container uapis.

-- 
Regards,
Yi Liu

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Fri, Aug 16, 2024 at 01:47:52PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
> > On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
> >> I suppose that this also includes converting paging domain to user
> >> domain allocation in VFIO (ideally vhost-vdpa as well), right?
> > 
> > Another thought is to add a flag to iommu_paging_domain_alloc()..
> > 
> > Maybe like the below. The small duplication with
> > __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
> > goes, then we'd be able to directly call iommu_paging_domain_alloc().
> 
> This will work. But we have to define NO_PASID flag and pass it from vfio.

flags = 0 means NO_PASID

I didn't show it but the dma api domain's allocation path would have
to specify IOMMU_HWPT_ALLOC_PASID to make it work.

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,


On 8/17/2024 12:07 AM, Jason Gunthorpe wrote:
> On Fri, Aug 16, 2024 at 01:47:52PM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
>>> On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
>>>> I suppose that this also includes converting paging domain to user
>>>> domain allocation in VFIO (ideally vhost-vdpa as well), right?
>>>
>>> Another thought is to add a flag to iommu_paging_domain_alloc()..
>>>
>>> Maybe like the below. The small duplication with
>>> __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
>>> goes, then we'd be able to directly call iommu_paging_domain_alloc().
>>
>> This will work. But we have to define NO_PASID flag and pass it from vfio.
> 
> flags = 0 means NO_PASID
> 
> I didn't show it but the dma api domain's allocation path would have
> to specify IOMMU_HWPT_ALLOC_PASID to make it work.


That's fine. Then in your patch we have to prioritize domain_alloc_user() over
domain_alloc_paging. Change will look something like below (paste the code path
that's changing).


2037 struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
2038                                                      unsigned int flags)
2039 {
2040         const struct iommu_ops *ops = dev_iommu_ops(dev);
2041         struct iommu_domain *domain;
2042
2043         if (!dev_has_iommu(dev))
2044                 return ERR_PTR(-ENODEV);
2045
2046         if (ops->domain_alloc_user)
2047                 domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
2048         else if (ops->domain_alloc_paging && !flags)
2049                 domain = ops->domain_alloc_paging(dev);


-Vasant

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe]

On Mon, Aug 19, 2024 at 01:57:11PM +0530, Vasant Hegde wrote:
> Jason,
> 
> 
> On 8/17/2024 12:07 AM, Jason Gunthorpe wrote:
> > On Fri, Aug 16, 2024 at 01:47:52PM +0530, Vasant Hegde wrote:
> >> Jason,
> >>
> >>
> >> On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
> >>> On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
> >>>> I suppose that this also includes converting paging domain to user
> >>>> domain allocation in VFIO (ideally vhost-vdpa as well), right?
> >>>
> >>> Another thought is to add a flag to iommu_paging_domain_alloc()..
> >>>
> >>> Maybe like the below. The small duplication with
> >>> __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
> >>> goes, then we'd be able to directly call iommu_paging_domain_alloc().
> >>
> >> This will work. But we have to define NO_PASID flag and pass it from vfio.
> > 
> > flags = 0 means NO_PASID
> > 
> > I didn't show it but the dma api domain's allocation path would have
> > to specify IOMMU_HWPT_ALLOC_PASID to make it work.
> 
> 
> That's fine. Then in your patch we have to prioritize domain_alloc_user() over
> domain_alloc_paging. Change will look something like below (paste the code path
> that's changing).
> 
> 
> 2037 struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
> 2038                                                      unsigned int flags)
> 2039 {
> 2040         const struct iommu_ops *ops = dev_iommu_ops(dev);
> 2041         struct iommu_domain *domain;
> 2042
> 2043         if (!dev_has_iommu(dev))
> 2044                 return ERR_PTR(-ENODEV);
> 2045
> 2046         if (ops->domain_alloc_user)
> 2047                 domain = ops->domain_alloc_user(dev, flags, NULL, NULL);

I mean to make it explicit that the DMA API is requesting PASID:

--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1589,8 +1589,18 @@ 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, dev->iommu->max_pasids ? IOMMUFD_HWPT_PASID : 0);
        return __iommu_group_domain_alloc(group, req_type);
 }

Jason

Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Vasant Hegde]

Jason,



On 8/19/2024 11:22 PM, Jason Gunthorpe wrote:
> On Mon, Aug 19, 2024 at 01:57:11PM +0530, Vasant Hegde wrote:
>> Jason,
>>
>>
>> On 8/17/2024 12:07 AM, Jason Gunthorpe wrote:
>>> On Fri, Aug 16, 2024 at 01:47:52PM +0530, Vasant Hegde wrote:
>>>> Jason,
>>>>
>>>>
>>>> On 8/15/2024 7:17 PM, Jason Gunthorpe wrote:
>>>>> On Thu, Aug 15, 2024 at 12:59:13PM +0800, Baolu Lu wrote:
>>>>>> I suppose that this also includes converting paging domain to user
>>>>>> domain allocation in VFIO (ideally vhost-vdpa as well), right?
>>>>>
>>>>> Another thought is to add a flag to iommu_paging_domain_alloc()..
>>>>>
>>>>> Maybe like the below. The small duplication with
>>>>> __iommu_domain_alloc() would go away when iommu_domain_alloc() finally
>>>>> goes, then we'd be able to directly call iommu_paging_domain_alloc().
>>>>
>>>> This will work. But we have to define NO_PASID flag and pass it from vfio.
>>>
>>> flags = 0 means NO_PASID
>>>
>>> I didn't show it but the dma api domain's allocation path would have
>>> to specify IOMMU_HWPT_ALLOC_PASID to make it work.
>>
>>
>> That's fine. Then in your patch we have to prioritize domain_alloc_user() over
>> domain_alloc_paging. Change will look something like below (paste the code path
>> that's changing).
>>
>>
>> 2037 struct iommu_domain *iommu_paging_domain_alloc_flags(struct device *dev,
>> 2038                                                      unsigned int flags)
>> 2039 {
>> 2040         const struct iommu_ops *ops = dev_iommu_ops(dev);
>> 2041         struct iommu_domain *domain;
>> 2042
>> 2043         if (!dev_has_iommu(dev))
>> 2044                 return ERR_PTR(-ENODEV);
>> 2045
>> 2046         if (ops->domain_alloc_user)
>> 2047                 domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> 
> I mean to make it explicit that the DMA API is requesting PASID:

Sure. I think we are in good shape to get the proper patch.
Will post the patch soon.

-Vasant


> 
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1589,8 +1589,18 @@ 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, dev->iommu->max_pasids ? IOMMUFD_HWPT_PASID : 0);
>         return __iommu_group_domain_alloc(group, req_type);
>  }
> 
> Jason