[PATCH v2 0/3] Support attaching PASID to the blocked_domain

[PATCH v2 0/3] Support attaching PASID to the blocked_domain

[Yi Liu]

During the review of iommufd pasid series, Kevin and Jason suggested
attaching PASID to the blocked domain hence replacing the usage of
remove_dev_pasid() op [1]. This makes sense as it makes the PASID path
aligned with the RID path which attaches the RID to the blocked_domain
when it is to be blocked. To do it, it requires passing the old domain
to the iommu driver. This has been done in [2].

This series makes the Intel iommu driver and ARM SMMUv3 driver support
attaching PASID to the blocked domain. While the AMD iommu driver does
not have the blocked domain yet, so still uses the remove_dev_pasid() op.

[1] https://lore.kernel.org/linux-iommu/20240816130202.GB2032816@nvidia.com/
[2] https://lore.kernel.org/linux-iommu/20241018055402.23277-2-yi.l.liu@intel.com/

v2:
 - Add Kevin's r-b
 - Adjust the order of patch 03 of v1, it should be the first patch (Baolu)

v1: https://lore.kernel.org/linux-iommu/20240912130653.11028-1-yi.l.liu@intel.com/

Regards,
	Yi Liu

Jason Gunthorpe (1):
  iommu/arm-smmu-v3: Make the blocked domain support PASID

Yi Liu (2):
  iommu: Add a wrapper for remove_dev_pasid
  iommu/vt-d: Make the blocked domain support PASID

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
 drivers/iommu/intel/iommu.c                 | 19 ++++++++-----
 drivers/iommu/intel/pasid.c                 |  3 ++-
 drivers/iommu/iommu.c                       | 30 ++++++++++++++++-----
 4 files changed, 45 insertions(+), 19 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu]

The iommu drivers are on the way to drop the remove_dev_pasid op by
extending the blocked_domain to support PASID. However, this cannot be
done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
supported it, while the AMD iommu driver has not yet. During this
transition, the IOMMU core needs to support both ways to destroy the
attachment of device/PASID and domain.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f3f81c04b8fb..9266e4ebebc2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3324,6 +3324,28 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 
+/*
+ * This is gated by AMD's blocked domain pasid support, it should be
+ * dropped once AMD iommu driver is ready.
+ */
+static void iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
+				   struct iommu_domain *domain)
+{
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	struct iommu_domain *blocked_domain = ops->blocked_domain;
+	int ret = 1;
+
+	if (blocked_domain->ops->set_dev_pasid) {
+		ret = blocked_domain->ops->set_dev_pasid(blocked_domain,
+							 dev, pasid, domain);
+	} else if (ops->remove_dev_pasid) {
+		ops->remove_dev_pasid(dev, pasid, domain);
+		ret = 0;
+	}
+
+	WARN_ON(ret);
+}
+
 static int __iommu_set_group_pasid(struct iommu_domain *domain,
 				   struct iommu_group *group, ioasid_t pasid)
 {
@@ -3342,11 +3364,9 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
 err_revert:
 	last_gdev = device;
 	for_each_group_device(group, device) {
-		const struct iommu_ops *ops = dev_iommu_ops(device->dev);
-
 		if (device == last_gdev)
 			break;
-		ops->remove_dev_pasid(device->dev, pasid, domain);
+		iommu_remove_dev_pasid(device->dev, pasid, domain);
 	}
 	return ret;
 }
@@ -3356,11 +3376,9 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 				       struct iommu_domain *domain)
 {
 	struct group_device *device;
-	const struct iommu_ops *ops;
 
 	for_each_group_device(group, device) {
-		ops = dev_iommu_ops(device->dev);
-		ops->remove_dev_pasid(device->dev, pasid, domain);
+		iommu_remove_dev_pasid(device->dev, pasid, domain);
 	}
 }
 
-- 
2.34.1

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
> The iommu drivers are on the way to drop the remove_dev_pasid op by
> extending the blocked_domain to support PASID. However, this cannot be
> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
> supported it, while the AMD iommu driver has not yet. During this
> transition, the IOMMU core needs to support both ways to destroy the
> attachment of device/PASID and domain.

Let's just fix AMD?

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 9e25b92c68affa..806849cc997631 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2437,10 +2437,18 @@ static int blocked_domain_attach_device(struct iommu_domain *domain,
 	return 0;
 }
 
+static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid)
+{
+	amd_iommu_remove_dev_pasid(dev, pasid, domain);
+	return 0;
+}
+
 static struct iommu_domain blocked_domain = {
 	.type = IOMMU_DOMAIN_BLOCKED,
 	.ops = &(const struct iommu_domain_ops) {
 		.attach_dev     = blocked_domain_attach_device,
+		.set_dev_pasid  = blocked_domain_set_dev_pasid,
 	}
 };
 
Jason

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu]

On 2024/10/18 22:39, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>> extending the blocked_domain to support PASID. However, this cannot be
>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>> supported it, while the AMD iommu driver has not yet. During this
>> transition, the IOMMU core needs to support both ways to destroy the
>> attachment of device/PASID and domain.
> 
> Let's just fix AMD?

cool.

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 9e25b92c68affa..806849cc997631 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2437,10 +2437,18 @@ static int blocked_domain_attach_device(struct iommu_domain *domain,
>   	return 0;
>   }
>   
> +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
> +					struct device *dev, ioasid_t pasid)
> +{
> +	amd_iommu_remove_dev_pasid(dev, pasid, domain);
> +	return 0;
> +}
> +
>   static struct iommu_domain blocked_domain = {
>   	.type = IOMMU_DOMAIN_BLOCKED,
>   	.ops = &(const struct iommu_domain_ops) {
>   		.attach_dev     = blocked_domain_attach_device,
> +		.set_dev_pasid  = blocked_domain_set_dev_pasid,
>   	}
>   };
>   
> Jason

-- 
Regards,
Yi Liu

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Jason Gunthorpe]

On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
> On 2024/10/18 22:39, Jason Gunthorpe wrote:
> > On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
> > > The iommu drivers are on the way to drop the remove_dev_pasid op by
> > > extending the blocked_domain to support PASID. However, this cannot be
> > > done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
> > > supported it, while the AMD iommu driver has not yet. During this
> > > transition, the IOMMU core needs to support both ways to destroy the
> > > attachment of device/PASID and domain.
> > 
> > Let's just fix AMD?
> 
> cool.

You could probably do better on this and fixup
amd_iommu_remove_dev_pasid() to have the right signature directly,
like the other drivers did

Jason

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu]

On 2024/10/21 20:33, Jason Gunthorpe wrote:
> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>> supported it, while the AMD iommu driver has not yet. During this
>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>> attachment of device/PASID and domain.
>>>
>>> Let's just fix AMD?
>>
>> cool.
> 
> You could probably do better on this and fixup
> amd_iommu_remove_dev_pasid() to have the right signature directly,
> like the other drivers did

It might make sense to move the amd_iommu_remove_dev_pasid() to the
drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().


diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
index b11b014fa82d..55ac1ad10fb3 100644
--- a/drivers/iommu/amd/amd_iommu.h
+++ b/drivers/iommu/amd/amd_iommu.h
@@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom);
  int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
  			    struct device *dev, ioasid_t pasid,
  			    struct iommu_domain *old);
-void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				struct iommu_domain *domain);
+void remove_pdom_dev_pasid(struct protection_domain *pdom,
+			   struct device *dev, ioasid_t pasid);

  /* SVA/PASID */
  bool amd_iommu_pasid_supported(void);
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 8364cd6fa47d..f807c4956a75 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct 
iommu_domain *domain,
  	return 0;
  }

+static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid,
+					struct iommu_domain *old)
+{
+	struct protection_domain *pdom = to_pdomain(old);
+	unsigned long flags;
+
+	if (old->type != IOMMU_DOMAIN_SVA)
+		return -EINVAL;
+
+	if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
+		return 0;
+
+	pdom = to_pdomain(domain);
+
+	spin_lock_irqsave(&pdom->lock, flags);
+
+	/* Remove PASID from dev_data_list */
+	remove_pdom_dev_pasid(pdom, dev, pasid);
+
+	spin_unlock_irqrestore(&pdom->lock, flags);
+	return 0;
+}
+
  static struct iommu_domain blocked_domain = {
  	.type = IOMMU_DOMAIN_BLOCKED,
  	.ops = &(const struct iommu_domain_ops) {
diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
index 8c73a30c2800..c43c7286c872 100644
--- a/drivers/iommu/amd/pasid.c
+++ b/drivers/iommu/amd/pasid.c
@@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data 
*pdom_dev_data)
  }

  /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */
-static void remove_pdom_dev_pasid(struct protection_domain *pdom,
-				  struct device *dev, ioasid_t pasid)
+void remove_pdom_dev_pasid(struct protection_domain *pdom,
+			   struct device *dev, ioasid_t pasid)
  {
  	struct pdom_dev_data *pdom_dev_data;
  	struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
@@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
  	return ret;
  }

-void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				struct iommu_domain *domain)
-{
-	struct protection_domain *sva_pdom;
-	unsigned long flags;
-
-	if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
-		return;
-
-	sva_pdom = to_pdomain(domain);
-
-	spin_lock_irqsave(&sva_pdom->lock, flags);
-
-	/* Remove PASID from dev_data_list */
-	remove_pdom_dev_pasid(sva_pdom, dev, pasid);
-
-	spin_unlock_irqrestore(&sva_pdom->lock, flags);
-}
-
  static void iommu_sva_domain_free(struct iommu_domain *domain)
  {
  	struct protection_domain *sva_pdom = to_pdomain(domain);


-- 
Regards,
Yi Liu

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Vasant Hegde]

Hi Yi,


On 10/22/2024 6:21 PM, Yi Liu wrote:
> On 2024/10/21 20:33, Jason Gunthorpe wrote:
>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>>> supported it, while the AMD iommu driver has not yet. During this
>>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>>> attachment of device/PASID and domain.
>>>>
>>>> Let's just fix AMD?
>>>
>>> cool.
>>
>> You could probably do better on this and fixup
>> amd_iommu_remove_dev_pasid() to have the right signature directly,
>> like the other drivers did
> 
> It might make sense to move the amd_iommu_remove_dev_pasid() to the
> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().

I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in
pasid.c only.

> 
> 
> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
> index b11b014fa82d..55ac1ad10fb3 100644
> --- a/drivers/iommu/amd/amd_iommu.h
> +++ b/drivers/iommu/amd/amd_iommu.h
> @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom);
>  int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>                  struct device *dev, ioasid_t pasid,
>                  struct iommu_domain *old);
> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
> -                struct iommu_domain *domain);
> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
> +               struct device *dev, ioasid_t pasid);
> 
>  /* SVA/PASID */
>  bool amd_iommu_pasid_supported(void);
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 8364cd6fa47d..f807c4956a75 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct
> iommu_domain *domain,
>      return 0;
>  }
> 

May be we should add comment here or at least explain it in patch description.
Otherwise it may create confusion. Something like below


Remove PASID from old domain and device GCR3 table. No need to attach PASID to
blocked domain as clearing PASID from GCR3 table will make sure all DMAs for
that PASID is blocked.





> +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
> +                    struct device *dev, ioasid_t pasid,
> +                    struct iommu_domain *old)
> +{
> +    struct protection_domain *pdom = to_pdomain(old);
> +    unsigned long flags;
> +
> +    if (old->type != IOMMU_DOMAIN_SVA)
> +        return -EINVAL;
> +
> +    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
> +        return 0;
> +
> +    pdom = to_pdomain(domain);

This is redundant as you already set pdom to old domain.

> +
> +    spin_lock_irqsave(&pdom->lock, flags);
> +
> +    /* Remove PASID from dev_data_list */
> +    remove_pdom_dev_pasid(pdom, dev, pasid);
> +
> +    spin_unlock_irqrestore(&pdom->lock, flags);
> +    return 0;
> +}
> +
>  static struct iommu_domain blocked_domain = {
>      .type = IOMMU_DOMAIN_BLOCKED,
>      .ops = &(const struct iommu_domain_ops) {
> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
> index 8c73a30c2800..c43c7286c872 100644
> --- a/drivers/iommu/amd/pasid.c
> +++ b/drivers/iommu/amd/pasid.c
> @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data)
>  }
> 
>  /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */
> -static void remove_pdom_dev_pasid(struct protection_domain *pdom,
> -                  struct device *dev, ioasid_t pasid)
> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
> +               struct device *dev, ioasid_t pasid)
>  {
>      struct pdom_dev_data *pdom_dev_data;
>      struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
> @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>      return ret;
>  }
> 
> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
> -                struct iommu_domain *domain)
> -{
> -    struct protection_domain *sva_pdom;
> -    unsigned long flags;
> -
> -    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
> -        return;
> -
> -    sva_pdom = to_pdomain(domain);
> -
> -    spin_lock_irqsave(&sva_pdom->lock, flags);
> -
> -    /* Remove PASID from dev_data_list */
> -    remove_pdom_dev_pasid(sva_pdom, dev, pasid);
> -
> -    spin_unlock_irqrestore(&sva_pdom->lock, flags);
> -}
> -
>  static void iommu_sva_domain_free(struct iommu_domain *domain)
>  {
>      struct protection_domain *sva_pdom = to_pdomain(domain);
> 
> 

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Yi Liu]

On 2024/10/23 19:10, Vasant Hegde wrote:
> Hi Yi,
> 
> 
> On 10/22/2024 6:21 PM, Yi Liu wrote:
>> On 2024/10/21 20:33, Jason Gunthorpe wrote:
>>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>>>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>>>> supported it, while the AMD iommu driver has not yet. During this
>>>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>>>> attachment of device/PASID and domain.
>>>>>
>>>>> Let's just fix AMD?
>>>>
>>>> cool.
>>>
>>> You could probably do better on this and fixup
>>> amd_iommu_remove_dev_pasid() to have the right signature directly,
>>> like the other drivers did
>>
>> It might make sense to move the amd_iommu_remove_dev_pasid() to the
>> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().
> 
> I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in
> pasid.c only.

ok. If so, we may just let the blocked_domain_set_dev_pasid() call
amd_iommu_remove_dev_pasid().

>>
>>
>> diff --git a/drivers/iommu/amd/amd_iommu.h b/drivers/iommu/amd/amd_iommu.h
>> index b11b014fa82d..55ac1ad10fb3 100644
>> --- a/drivers/iommu/amd/amd_iommu.h
>> +++ b/drivers/iommu/amd/amd_iommu.h
>> @@ -54,8 +54,8 @@ void amd_iommu_domain_free(struct iommu_domain *dom);
>>   int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>>                   struct device *dev, ioasid_t pasid,
>>                   struct iommu_domain *old);
>> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>> -                struct iommu_domain *domain);
>> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> +               struct device *dev, ioasid_t pasid);
>>
>>   /* SVA/PASID */
>>   bool amd_iommu_pasid_supported(void);
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 8364cd6fa47d..f807c4956a75 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2437,6 +2437,30 @@ static int blocked_domain_attach_device(struct
>> iommu_domain *domain,
>>       return 0;
>>   }
>>
> 
> May be we should add comment here or at least explain it in patch description.
> Otherwise it may create confusion. Something like below
> 
> 
> Remove PASID from old domain and device GCR3 table. No need to attach PASID to
> blocked domain as clearing PASID from GCR3 table will make sure all DMAs for
> that PASID is blocked.

got it.

> 
> 
> 
> 
>> +static int blocked_domain_set_dev_pasid(struct iommu_domain *domain,
>> +                    struct device *dev, ioasid_t pasid,
>> +                    struct iommu_domain *old)
>> +{
>> +    struct protection_domain *pdom = to_pdomain(old);
>> +    unsigned long flags;
>> +
>> +    if (old->type != IOMMU_DOMAIN_SVA)
>> +        return -EINVAL;
>> +
>> +    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
>> +        return 0;
>> +
>> +    pdom = to_pdomain(domain);
> 
> This is redundant as you already set pdom to old domain.

yes.

>> +
>> +    spin_lock_irqsave(&pdom->lock, flags);
>> +
>> +    /* Remove PASID from dev_data_list */
>> +    remove_pdom_dev_pasid(pdom, dev, pasid);
>> +
>> +    spin_unlock_irqrestore(&pdom->lock, flags);
>> +    return 0;
>> +}
>> +
>>   static struct iommu_domain blocked_domain = {
>>       .type = IOMMU_DOMAIN_BLOCKED,
>>       .ops = &(const struct iommu_domain_ops) {
>> diff --git a/drivers/iommu/amd/pasid.c b/drivers/iommu/amd/pasid.c
>> index 8c73a30c2800..c43c7286c872 100644
>> --- a/drivers/iommu/amd/pasid.c
>> +++ b/drivers/iommu/amd/pasid.c
>> @@ -39,8 +39,8 @@ static void remove_dev_pasid(struct pdom_dev_data *pdom_dev_data)
>>   }
>>
>>   /* Clear PASID from device GCR3 table and remove pdom_dev_data from list */
>> -static void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> -                  struct device *dev, ioasid_t pasid)
>> +void remove_pdom_dev_pasid(struct protection_domain *pdom,
>> +               struct device *dev, ioasid_t pasid)
>>   {
>>       struct pdom_dev_data *pdom_dev_data;
>>       struct iommu_dev_data *dev_data = dev_iommu_priv_get(dev);
>> @@ -145,25 +145,6 @@ int iommu_sva_set_dev_pasid(struct iommu_domain *domain,
>>       return ret;
>>   }
>>
>> -void amd_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>> -                struct iommu_domain *domain)
>> -{
>> -    struct protection_domain *sva_pdom;
>> -    unsigned long flags;
>> -
>> -    if (!is_pasid_valid(dev_iommu_priv_get(dev), pasid))
>> -        return;
>> -
>> -    sva_pdom = to_pdomain(domain);
>> -
>> -    spin_lock_irqsave(&sva_pdom->lock, flags);
>> -
>> -    /* Remove PASID from dev_data_list */
>> -    remove_pdom_dev_pasid(sva_pdom, dev, pasid);
>> -
>> -    spin_unlock_irqrestore(&sva_pdom->lock, flags);
>> -}
>> -
>>   static void iommu_sva_domain_free(struct iommu_domain *domain)
>>   {
>>       struct protection_domain *sva_pdom = to_pdomain(domain);
>>
>>

-- 
Regards,
Yi Liu

Re: [PATCH v2 1/3] iommu: Add a wrapper for remove_dev_pasid

[Vasant Hegde]

Yi,


On 10/29/2024 10:50 AM, Yi Liu wrote:
> On 2024/10/23 19:10, Vasant Hegde wrote:
>> Hi Yi,
>>
>>
>> On 10/22/2024 6:21 PM, Yi Liu wrote:
>>> On 2024/10/21 20:33, Jason Gunthorpe wrote:
>>>> On Mon, Oct 21, 2024 at 05:35:38PM +0800, Yi Liu wrote:
>>>>> On 2024/10/18 22:39, Jason Gunthorpe wrote:
>>>>>> On Thu, Oct 17, 2024 at 10:58:22PM -0700, Yi Liu wrote:
>>>>>>> The iommu drivers are on the way to drop the remove_dev_pasid op by
>>>>>>> extending the blocked_domain to support PASID. However, this cannot be
>>>>>>> done in one shot. So far, the Intel iommu and the ARM SMMUv3 driver have
>>>>>>> supported it, while the AMD iommu driver has not yet. During this
>>>>>>> transition, the IOMMU core needs to support both ways to destroy the
>>>>>>> attachment of device/PASID and domain.
>>>>>>
>>>>>> Let's just fix AMD?
>>>>>
>>>>> cool.
>>>>
>>>> You could probably do better on this and fixup
>>>> amd_iommu_remove_dev_pasid() to have the right signature directly,
>>>> like the other drivers did
>>>
>>> It might make sense to move the amd_iommu_remove_dev_pasid() to the
>>> drivers/iommu/amd/iommu.c and make it to be the blocked_domain_set_dev_pasid().
>>
>> I wanted to keep all PASID code in pasid.c. I'd say for now lets keep it in
>> pasid.c only.
> 
> ok. If so, we may just let the blocked_domain_set_dev_pasid() call
> amd_iommu_remove_dev_pasid().

Sure. Lets do that for now.

-Vasant

[PATCH v2 2/3] iommu/arm-smmu-v3: Make the blocked domain support PASID

[Yi Liu]

From: Jason Gunthorpe <jgg@nvidia.com>

The blocked domain is used to park RID to be blocking DMA state. This
can be extended to PASID as well. By this, the remove_dev_pasid() op
of ARM SMMUv3 can be dropped.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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 f70165f544df..ae68c7b7fcd5 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2961,13 +2961,12 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master,
 	return ret;
 }
 
-static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-				      struct iommu_domain *domain)
+static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
+					   struct device *dev, ioasid_t pasid,
+					   struct iommu_domain *old_domain)
 {
+	struct arm_smmu_domain *smmu_domain = to_smmu_domain(old_domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-	struct arm_smmu_domain *smmu_domain;
-
-	smmu_domain = to_smmu_domain(domain);
 
 	mutex_lock(&arm_smmu_asid_lock);
 	arm_smmu_clear_cd(master, pasid);
@@ -2988,6 +2987,7 @@ static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 		    sid_domain->type == IOMMU_DOMAIN_BLOCKED)
 			sid_domain->ops->attach_dev(sid_domain, dev);
 	}
+	return 0;
 }
 
 static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
@@ -3069,6 +3069,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
 
 static const struct iommu_domain_ops arm_smmu_blocked_ops = {
 	.attach_dev = arm_smmu_attach_dev_blocked,
+	.set_dev_pasid = arm_smmu_blocking_set_dev_pasid,
 };
 
 static struct iommu_domain arm_smmu_blocked_domain = {
@@ -3497,7 +3498,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= arm_smmu_device_group,
 	.of_xlate		= arm_smmu_of_xlate,
 	.get_resv_regions	= arm_smmu_get_resv_regions,
-	.remove_dev_pasid	= arm_smmu_remove_dev_pasid,
 	.dev_enable_feat	= arm_smmu_dev_enable_feature,
 	.dev_disable_feat	= arm_smmu_dev_disable_feature,
 	.page_response		= arm_smmu_page_response,
-- 
2.34.1

Re: [PATCH v2 2/3] iommu/arm-smmu-v3: Make the blocked domain support PASID

[Nicolin Chen]

On Thu, Oct 17, 2024 at 10:58:23PM -0700, Yi Liu wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> The blocked domain is used to park RID to be blocking DMA state. This
> can be extended to PASID as well. By this, the remove_dev_pasid() op
> of ARM SMMUv3 can be dropped.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Signed-off-by: Yi Liu <yi.l.liu@intel.com>

Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>

[PATCH v2 3/3] iommu/vt-d: Make the blocked domain support PASID

[Yi Liu]

The blocked domain can be extended to park PASID of a device to be the
DMA blocking state. By this the remove_dev_pasid() op is dropped.

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Signed-off-by: Yi Liu <yi.l.liu@intel.com>
---
 drivers/iommu/intel/iommu.c | 19 +++++++++++++------
 drivers/iommu/intel/pasid.c |  3 ++-
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index d089ac148a7e..f814fadcf34e 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3412,10 +3412,15 @@ static int blocking_domain_attach_dev(struct iommu_domain *domain,
 	return 0;
 }
 
+static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old);
+
 static struct iommu_domain blocking_domain = {
 	.type = IOMMU_DOMAIN_BLOCKED,
 	.ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= blocking_domain_attach_dev,
+		.set_dev_pasid	= blocking_domain_set_dev_pasid,
 	}
 };
 
@@ -4282,8 +4287,9 @@ static void domain_remove_dev_pasid(struct iommu_domain *domain,
 	kfree(dev_pasid);
 }
 
-static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
-					 struct iommu_domain *domain)
+static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
+					 struct device *dev, ioasid_t pasid,
+					 struct iommu_domain *old)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
@@ -4292,10 +4298,12 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
 				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
 
 	/* Identity domain has no meta data for pasid. */
-	if (domain->type == IOMMU_DOMAIN_IDENTITY)
-		return;
+	if (old->type == IOMMU_DOMAIN_IDENTITY)
+		goto out;
 
-	domain_remove_dev_pasid(domain, dev, pasid);
+	domain_remove_dev_pasid(old, dev, pasid);
+out:
+	return 0;
 }
 
 static struct dev_pasid_info *
@@ -4653,7 +4661,6 @@ const struct iommu_ops intel_iommu_ops = {
 	.dev_disable_feat	= intel_iommu_dev_disable_feat,
 	.is_attach_deferred	= intel_iommu_is_attach_deferred,
 	.def_domain_type	= device_def_domain_type,
-	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,
 	.pgsize_bitmap		= SZ_4K,
 #ifdef CONFIG_INTEL_IOMMU_SVM
 	.page_response		= intel_svm_page_response,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index ce0a3bf701df..abf2f54e847e 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -238,7 +238,8 @@ devtlb_invalidation_with_pasid(struct intel_iommu *iommu,
 
 /*
  * Caller can request to drain PRQ in this helper if it hasn't done so,
- * e.g. in a path which doesn't follow remove_dev_pasid().
+ * e.g. in a path which doesn't follow the set_dev_pasid() op of the
+ * blocked domain.
  * Return the pasid entry pointer if the entry is found or NULL if no
  * entry found.
  */
-- 
2.34.1

Re: [PATCH v2 3/3] iommu/vt-d: Make the blocked domain support PASID

[Jason Gunthorpe]

On Thu, Oct 17, 2024 at 10:58:24PM -0700, Yi Liu wrote:

> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
> -					 struct iommu_domain *domain)
> +static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
> +					 struct device *dev, ioasid_t pasid,
> +					 struct iommu_domain *old)
>  {
>  	struct device_domain_info *info = dev_iommu_priv_get(dev);
>  	struct intel_iommu *iommu = info->iommu;
> @@ -4292,10 +4298,12 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>  				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>  
>  	/* Identity domain has no meta data for pasid. */
> -	if (domain->type == IOMMU_DOMAIN_IDENTITY)
> -		return;
> +	if (old->type == IOMMU_DOMAIN_IDENTITY)
> +		goto out;

Just return 0

Jason

Re: [PATCH v2 3/3] iommu/vt-d: Make the blocked domain support PASID

[Yi Liu]

On 2024/10/18 23:54, Jason Gunthorpe wrote:
> On Thu, Oct 17, 2024 at 10:58:24PM -0700, Yi Liu wrote:
> 
>> -static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>> -					 struct iommu_domain *domain)
>> +static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
>> +					 struct device *dev, ioasid_t pasid,
>> +					 struct iommu_domain *old)
>>   {
>>   	struct device_domain_info *info = dev_iommu_priv_get(dev);
>>   	struct intel_iommu *iommu = info->iommu;
>> @@ -4292,10 +4298,12 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid,
>>   				    INTEL_PASID_TEARDOWN_DRAIN_PRQ);
>>   
>>   	/* Identity domain has no meta data for pasid. */
>> -	if (domain->type == IOMMU_DOMAIN_IDENTITY)
>> -		return;
>> +	if (old->type == IOMMU_DOMAIN_IDENTITY)
>> +		goto out;
> 
> Just return 0
> 

got it.

-- 
Regards,
Yi Liu

Re: [PATCH v2 0/3] Support attaching PASID to the blocked_domain

[Vasant Hegde]

Hi Yi,


On 10/18/2024 11:28 AM, Yi Liu wrote:
> During the review of iommufd pasid series, Kevin and Jason suggested
> attaching PASID to the blocked domain hence replacing the usage of
> remove_dev_pasid() op [1]. This makes sense as it makes the PASID path
> aligned with the RID path which attaches the RID to the blocked_domain
> when it is to be blocked. To do it, it requires passing the old domain
> to the iommu driver. This has been done in [2].

I understand attaching RID to blocked_domain. But I am not getting why
we have to do same for PASID. In remove_dev_pasid() path we clear the entry in
PASID table (AMD case GCR3 table). So no further access is allowed anyway.

Is it just to align with RID flow -OR- do we have any other reason?


-Vasant


> 
> This series makes the Intel iommu driver and ARM SMMUv3 driver support
> attaching PASID to the blocked domain. While the AMD iommu driver does
> not have the blocked domain yet, so still uses the remove_dev_pasid() op.
> 
> [1] https://lore.kernel.org/linux-iommu/20240816130202.GB2032816@nvidia.com/
> [2] https://lore.kernel.org/linux-iommu/20241018055402.23277-2-yi.l.liu@intel.com/
> 
> v2:
>  - Add Kevin's r-b
>  - Adjust the order of patch 03 of v1, it should be the first patch (Baolu)
> 
> v1: https://lore.kernel.org/linux-iommu/20240912130653.11028-1-yi.l.liu@intel.com/
> 
> Regards,
> 	Yi Liu
> 
> Jason Gunthorpe (1):
>   iommu/arm-smmu-v3: Make the blocked domain support PASID
> 
> Yi Liu (2):
>   iommu: Add a wrapper for remove_dev_pasid
>   iommu/vt-d: Make the blocked domain support PASID
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
>  drivers/iommu/intel/iommu.c                 | 19 ++++++++-----
>  drivers/iommu/intel/pasid.c                 |  3 ++-
>  drivers/iommu/iommu.c                       | 30 ++++++++++++++++-----
>  4 files changed, 45 insertions(+), 19 deletions(-)
> 

Re: [PATCH v2 0/3] Support attaching PASID to the blocked_domain

[Yi Liu]

On 2024/10/22 17:44, Vasant Hegde wrote:
> Hi Yi,
> 
> 
> On 10/18/2024 11:28 AM, Yi Liu wrote:
>> During the review of iommufd pasid series, Kevin and Jason suggested
>> attaching PASID to the blocked domain hence replacing the usage of
>> remove_dev_pasid() op [1]. This makes sense as it makes the PASID path
>> aligned with the RID path which attaches the RID to the blocked_domain
>> when it is to be blocked. To do it, it requires passing the old domain
>> to the iommu driver. This has been done in [2].
> 
> I understand attaching RID to blocked_domain. But I am not getting why
> we have to do same for PASID. In remove_dev_pasid() path we clear the entry in
> PASID table (AMD case GCR3 table). So no further access is allowed anyway.
> 
> Is it just to align with RID flow -OR- do we have any other reason?

yes, this is also my understanding.:)

Regards,
Yi Liu

> 
> -Vasant
> 
> 
>>
>> This series makes the Intel iommu driver and ARM SMMUv3 driver support
>> attaching PASID to the blocked domain. While the AMD iommu driver does
>> not have the blocked domain yet, so still uses the remove_dev_pasid() op.
>>
>> [1] https://lore.kernel.org/linux-iommu/20240816130202.GB2032816@nvidia.com/
>> [2] https://lore.kernel.org/linux-iommu/20241018055402.23277-2-yi.l.liu@intel.com/
>>
>> v2:
>>   - Add Kevin's r-b
>>   - Adjust the order of patch 03 of v1, it should be the first patch (Baolu)
>>
>> v1: https://lore.kernel.org/linux-iommu/20240912130653.11028-1-yi.l.liu@intel.com/
>>
>> Regards,
>> 	Yi Liu
>>
>> Jason Gunthorpe (1):
>>    iommu/arm-smmu-v3: Make the blocked domain support PASID
>>
>> Yi Liu (2):
>>    iommu: Add a wrapper for remove_dev_pasid
>>    iommu/vt-d: Make the blocked domain support PASID
>>
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 12 ++++-----
>>   drivers/iommu/intel/iommu.c                 | 19 ++++++++-----
>>   drivers/iommu/intel/pasid.c                 |  3 ++-
>>   drivers/iommu/iommu.c                       | 30 ++++++++++++++++-----
>>   4 files changed, 45 insertions(+), 19 deletions(-)
>>

-- 
Regards,
Yi Liu