Enable PASID for DMA API users

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/iommu.h +6 -0

index 369f05c2a4e2..fde5b933dbe3 100644
@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
  * @aux_get_pasid: get the pasid given an aux-domain
  * @sva_bind: Bind process address space to device
  * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
  * @cache_invalidate: invalidate translation caches

@@ -296,6 +298,10 @@ struct iommu_ops {
 	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct
 *mm,
 				      void *drvdata);
 	void (*sva_unbind)(struct iommu_sva *handle);
+	int (*attach_dev_pasid)(struct iommu_domain *domain,
+				struct device *dev, ioasid_t id);
+	void (*detach_dev_pasid)(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t id);
 	u32 (*sva_get_pasid)(struct iommu_sva *handle);
 

int (*page_response)(struct device *dev,
--
2.25.1

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/iommu.h +0 -0

index 369f05c2a4e2..fde5b933dbe3 100644

include/linux/iommu.h +4 -0

  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
  * @cache_invalidate: invalidate translation caches

@@ -296,6 +298,10 @@ struct iommu_ops {
 	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
 				      void *drvdata);
 	void (*sva_unbind)(struct iommu_sva *handle);
+	int (*attach_dev_pasid)(struct iommu_domain *domain,
+				struct device *dev, ioasid_t id);
+	void (*detach_dev_pasid)(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t id);
 	u32 (*sva_get_pasid)(struct iommu_sva *handle);
 
 	int (*page_response)(struct device *dev,

Signature · 1 line

2.25.1

From: Jean-Philippe Brucker <[email protected]>
Sent: Tuesday, March 15, 2022 7:27 PM

Quoted 17 lines · Jacob Pan

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

include/linux/iommu.h struct iommu_iotlb_gather { · line 231

Quoted 10 lines

include/linux/iommu.h +2 -0

index 369f05c2a4e2..fde5b933dbe3 100644
@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
  * @aux_get_pasid: get the pasid given an aux-domain
  * @sva_bind: Bind process address space to device
  * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device

Isn't that operation "assign a PASID to a domain" instead? In patch 5, the domain is already attached to the device, so set_domain_pasid() might be clearer and to the point. If the IOMMU driver did the allocation we could also avoid patch 1.

If I understand correctly this series is not about a generic PASID API that allows drivers to manage multiple DMA address spaces, because there still doesn't seem to be any interest in that. It's about the specific IDXD use-case, so let's focus on that. We can introduce a specialized call such as (iommu|dma)_set_device_pasid(), which will be easy to consolidate later into a more generic "dma_enable_pasid()" API if that ever seems useful.

Thanks,
Jean

include/linux/iommu.h struct iommu_ops { · line 298

Quoted 5 lines

include/linux/iommu.h +0 -0

  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
  * @cache_invalidate: invalidate translation caches

@@ -296,6 +298,10 @@ struct iommu_ops {
 	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct

*mm,

Quoted 12 lines

include/linux/iommu.h +4 -1

 				      void *drvdata);
 	void (*sva_unbind)(struct iommu_sva *handle);
+	int (*attach_dev_pasid)(struct iommu_domain *domain,
+				struct device *dev, ioasid_t id);
+	void (*detach_dev_pasid)(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t id);
 	u32 (*sva_get_pasid)(struct iommu_sva *handle);
 
 	int (*page_response)(struct device *dev,
--

2.25.1

Quoted 21 lines

From: Jean-Philippe Brucker <[email protected]>
Sent: Tuesday, March 15, 2022 7:27 PM

Quoted 17 lines · Jacob Pan

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 include/linux/iommu.h | 6 ++++++
 1 file changed, 6 insertions(+)

Quoted 7 lines

Quoted 7 lines

include/linux/iommu.h +0 -0

index 369f05c2a4e2..fde5b933dbe3 100644
@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
  * @aux_get_pasid: get the pasid given an aux-domain
  * @sva_bind: Bind process address space to device

Quoted 8 lines

include/linux/iommu.h struct iommu_iotlb_gather { · line 231

Quoted 3 lines

include/linux/iommu.h +2 -0

  * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device  

Isn't that operation "assign a PASID to a domain" instead? In patch 5, the domain is already attached to the device, so set_domain_pasid() might be clearer and to the point. If the IOMMU driver did the allocation we could also avoid patch 1.

iiuc this API can also work for future SIOV usage where each mdev attached to the domain has its own pasid. "assigning a PASID to a domain" sounds like going back to the previous aux domain approach which has one PASID per domain and that PASID is used on all devices attached to the aux domain...

Quoted 9 lines

If I understand correctly this series is not about a generic PASID API that allows drivers to manage multiple DMA address spaces, because there still doesn't seem to be any interest in that. It's about the specific IDXD use-case, so let's focus on that. We can introduce a specialized call such as (iommu|dma)_set_device_pasid(), which will be easy to consolidate later into a more generic "dma_enable_pasid()" API if that ever seems useful.

Quoted 18 lines

Thanks,
Jean

Quoted 6 lines

include/linux/iommu.h +0 -0

  * @sva_get_pasid: Get PASID associated to a SVA handle
  * @page_response: handle page request response
  * @cache_invalidate: invalidate translation caches

@@ -296,6 +298,10 @@ struct iommu_ops {
 	struct iommu_sva *(*sva_bind)(struct device *dev, struct

mm_struct

*mm,

Quoted 8 lines

include/linux/iommu.h +4 -0

 				      void *drvdata);
 	void (*sva_unbind)(struct iommu_sva *handle);
+	int (*attach_dev_pasid)(struct iommu_domain *domain,
+				struct device *dev, ioasid_t id);
+	void (*detach_dev_pasid)(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t id);
 	u32 (*sva_get_pasid)(struct iommu_sva *handle);

Quoted 4 lines

Quoted 4 lines

int (*page_response)(struct device *dev,
--
2.25.1

Quoted 21 lines

From: Jean-Philippe Brucker<[email protected]>
Sent: Tuesday, March 15, 2022 7:27 PM

Quoted 17 lines · Jacob Pan

From: Lu Baolu<[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu<[email protected]>

Signed-off-by Jacob Pan<[email protected]>

---
  include/linux/iommu.h | 6 ++++++
  1 file changed, 6 insertions(+)

Quoted 14 lines

include/linux/iommu.h struct iommu_iotlb_gather { · line 231

Quoted 10 lines

include/linux/iommu.h +2 -0

index 369f05c2a4e2..fde5b933dbe3 100644
@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
   * @aux_get_pasid: get the pasid given an aux-domain
   * @sva_bind: Bind process address space to device
   * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device

Isn't that operation "assign a PASID to a domain" instead? In patch 5, the domain is already attached to the device, so set_domain_pasid() might be clearer and to the point. If the IOMMU driver did the allocation we could also avoid patch 1.

iiuc this API can also work for future SIOV usage where each mdev attached to the domain has its own pasid. "assigning a PASID to a domain" sounds like going back to the previous aux domain approach which has one PASID per domain and that PASID is used on all devices attached to the aux domain...

Quoted 33 lines · Kevin

Quoted 20 lines

From: Jean-Philippe Brucker<[email protected]>
Sent: Tuesday, March 15, 2022 7:27 PM

Quoted 16 lines · Jacob Pan

From: Lu Baolu<[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu<[email protected]>

Signed-off-by Jacob Pan<[email protected]>

  include/linux/iommu.h | 6 ++++++
  1 file changed, 6 insertions(+)

Quoted 13 lines

include/linux/iommu.h struct iommu_iotlb_gather { · line 231

Quoted 9 lines

include/linux/iommu.h +3 -0

index 369f05c2a4e2..fde5b933dbe3 100644
+++ b/include/linux/iommu.h

@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
   * @aux_get_pasid: get the pasid given an aux-domain
   * @sva_bind: Bind process address space to device
   * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device

Isn't that operation "assign a PASID to a domain" instead? In patch 5, the domain is already attached to the device, so set_domain_pasid() might be clearer and to the point. If the IOMMU driver did the allocation we could also avoid patch 1.

Quoted 5 lines

iiuc this API can also work for future SIOV usage where each mdev attached to the domain has its own pasid. "assigning a PASID to a domain" sounds like going back to the previous aux domain approach which has one PASID per domain and that PASID is used on all devices attached to the aux domain...

This also works for SVA as far as I can see. The sva_bind essentially is to attach an SVA domain to the PASID of a device. The sva_bind/unbind ops could be removed with these two new callbacks.

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
  include/linux/iommu.h | 6 ++++++
  1 file changed, 6 insertions(+)

include/linux/iommu.h +2 -0

index 369f05c2a4e2..fde5b933dbe3 100644
@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
   * @aux_get_pasid: get the pasid given an aux-domain
   * @sva_bind: Bind process address space to device
   * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
   * @sva_get_pasid: Get PASID associated to a SVA handle
   * @page_response: handle page request response
   * @cache_invalidate: invalidate translation caches

Quoted 31 lines · Jacob Pan

From: Lu Baolu <[email protected]>

An IOMMU domain represents an address space which can be attached by devices that perform DMA within a domain. However, for platforms with PASID capability the domain attachment needs be handled at device+PASID level. There can be multiple PASIDs within a device and multiple devices attached to a given domain.
This patch introduces a new IOMMU op which support device, PASID, and IOMMU domain attachment. The immediate use case is for PASID capable devices to perform DMA under DMA APIs.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

  include/linux/iommu.h | 6 ++++++
  1 file changed, 6 insertions(+)

include/linux/iommu.h +3 -0

index 369f05c2a4e2..fde5b933dbe3 100644
+++ b/include/linux/iommu.h

@@ -227,6 +227,8 @@ struct iommu_iotlb_gather {
   * @aux_get_pasid: get the pasid given an aux-domain
   * @sva_bind: Bind process address space to device
   * @sva_unbind: Unbind process address space from device
+ * @attach_dev_pasid: attach an iommu domain to a pasid of device
+ * @detach_dev_pasid: detach an iommu domain from a pasid of device
   * @sva_get_pasid: Get PASID associated to a SVA handle
   * @page_response: handle page request response
   * @cache_invalidate: invalidate translation caches

@@ -296,6 +298,10 @@ struct iommu_ops {
  	struct iommu_sva *(*sva_bind)(struct device *dev, struct mm_struct *mm,
  				      void *drvdata);

include/linux/iommu.h struct iommu_ops { · line 304

Quoted 5 lines

include/linux/iommu.h +4 -0

  	void (*sva_unbind)(struct iommu_sva *handle);
+	int (*attach_dev_pasid)(struct iommu_domain *domain,
+				struct device *dev, ioasid_t id);
+	void (*detach_dev_pasid)(struct iommu_domain *domain,
+				 struct device *dev, ioasid_t id);

As we have introduced iommu_domain_ops, these callbacks should be part of the domain ops.

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach to the correct IOMMU domains.
The attach operation involves the following:
- programming the PASID into device's PASID table
- tracking device domain and the PASID relationship
- managing IOTLB and device TLB invalidations

This patch extends DMAR domain and device domain info with xarrays to track per domain and per device PASIDs. It provides the flexibility to be used beyond DMA API PASID support.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/iommu/intel/iommu.c | 194
+++++++++++++++++++++++++++++++++++-
 include/linux/intel-iommu.h |  12 ++-
 2 files changed, 202 insertions(+), 4 deletions(-)

drivers/iommu/intel/iommu.c +0 -0

index 881f8361eca2..9267194eaed3 100644

drivers/iommu/intel/iommu.c +19 -3

+					u64 address,
+				     ioasid_t pasid, unsigned int mask)
+{
+	u16 sid, qdep;
+
+	if (!info || !info->ats_enabled)
+		return;
+
+	sid = info->bus << 8 | info->devfn;
+	qdep = info->ats_qdep;
+	qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
+				 pasid, qdep, address, mask);
+}
+
 static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
 				  u64 addr, unsigned mask)
 {
 	unsigned long flags;
 	struct device_domain_info *info;
 	struct subdev_domain_info *sinfo;
+	unsigned long pasid;
+	struct pasid_info *pinfo;
 
 	if (!domain->has_iotlb_device)
 		return;
 
 	spin_lock_irqsave(&device_domain_lock, flags);
-	list_for_each_entry(info, &domain->devices, link)
-		__iommu_flush_dev_iotlb(info, addr, mask);
-
+	list_for_each_entry(info, &domain->devices, link) {
+		/*
+		 * We cannot use PASID based devTLB invalidation on

RID2PASID

Quoted 31 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

On VT-d platforms with scalable mode enabled, devices issue DMA requests with PASID need to attach to the correct IOMMU domains.
The attach operation involves the following:
- programming the PASID into device's PASID table
- tracking device domain and the PASID relationship
- managing IOTLB and device TLB invalidations

This patch extends DMAR domain and device domain info with xarrays to track per domain and per device PASIDs. It provides the flexibility to be used beyond DMA API PASID support.

Signed-off-by Lu Baolu <[email protected]>

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/iommu/intel/iommu.c | 194
+++++++++++++++++++++++++++++++++++-
 include/linux/intel-iommu.h |  12 ++-
 2 files changed, 202 insertions(+), 4 deletions(-)

drivers/iommu/intel/iommu.c +0 -0

index 881f8361eca2..9267194eaed3 100644
@@ -1622,20 +1622,48 @@ static void __iommu_flush_dev_iotlb(struct
 device_domain_info *info,
 			   qdep, addr, mask);
 }
 

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info, · line 1625

Quoted 1 line

drivers/iommu/intel/iommu.c +1 -0

+static void __iommu_flush_dev_piotlb(struct device_domain_info *info,  

piotlb is confusing, better be:

	__iommu_flush_dev_iotlb_pasid()

Quoted 35 lines

drivers/iommu/intel/iommu.c +19 -3

+					u64 address,
+				     ioasid_t pasid, unsigned int mask)
+{
+	u16 sid, qdep;
+
+	if (!info || !info->ats_enabled)
+		return;
+
+	sid = info->bus << 8 | info->devfn;
+	qdep = info->ats_qdep;
+	qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
+				 pasid, qdep, address, mask);
+}
+
 static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
 				  u64 addr, unsigned mask)
 {
 	unsigned long flags;
 	struct device_domain_info *info;
 	struct subdev_domain_info *sinfo;
+	unsigned long pasid;
+	struct pasid_info *pinfo;
 
 	if (!domain->has_iotlb_device)
 		return;
 
 	spin_lock_irqsave(&device_domain_lock, flags);
-	list_for_each_entry(info, &domain->devices, link)
-		__iommu_flush_dev_iotlb(info, addr, mask);
-
+	list_for_each_entry(info, &domain->devices, link) {
+		/*
+		 * We cannot use PASID based devTLB invalidation on

RID2PASID

drivers/iommu/intel/iommu.c +1 -0

+		 * Device does not understand RID2PASID/0. This is

Quoted 1 line

different

Lack of a conjunction word between 'RID2PASID' and 'Device'.

and what is RID2PASID/0? It would be clearer to point out that RID2PASID is visible only within the iommu to mark out requests without PASID, thus this PASID value should never be sent to the device side.

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info, · line 1658

Quoted 3 lines

drivers/iommu/intel/iommu.c +1 -0

+		 * than IOTLB invalidation where RID2PASID is also

used for

drivers/iommu/intel/iommu.c +1 -0

+		 * tagging.  

Then it would be obvious because IOTLB is iommu internal agent thus takes use of RID2PASID for tagging.

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info, · line 1661

Quoted 3 lines

drivers/iommu/intel/iommu.c +3 -0

+		 */
+		xa_for_each(&info->pasids, pasid, pinfo) {
+			if (!pasid)  

this should be compared to PASID_RID2PASID (though it's zero today).

Quoted 5 lines

drivers/iommu/intel/iommu.c +1 -0

+				__iommu_flush_dev_iotlb(info, addr,

mask);

drivers/iommu/intel/iommu.c +2 -0

+			else
+				__iommu_flush_dev_piotlb(info, addr,

pasid, mask);

drivers/iommu/intel/iommu.c static void __iommu_flush_dev_iotlb(struct device_domain_info *info, · line 1669

Quoted 5 lines

drivers/iommu/intel/iommu.c +2 -0

+		}
+	}
 	list_for_each_entry(sinfo, &domain->subdevices, link_domain) {
 		info = get_domain_info(sinfo->pdev);
 		__iommu_flush_dev_iotlb(info, addr, mask);  

Thanks
Kevin

drivers/iommu/intel/iommu.c +21 -0

+	if (entry) {
+		pinfo = entry;
+	} else {
+		pinfo = kzalloc(sizeof(*pinfo), GFP_ATOMIC);
+		if (!pinfo)
+			return -ENOMEM;
+		pinfo->pasid = pasid;
+		/* Store the new PASID info in the per domain array */
+		ret = xa_err(__xa_store(&dmar_domain->pasids, pasid, pinfo,
+			     GFP_ATOMIC));
+		if (ret)
+			goto xa_store_err;
+	}
+	/* Store PASID in per device-domain array, this is for tracking devTLB */
+	ret = xa_err(xa_store(&info->pasids, pasid, pinfo, GFP_ATOMIC));
+	if (ret)
+		goto xa_store_err;
+
+	atomic_inc(&pinfo->users);
+	xa_unlock(&dmar_domain->pasids);
+

Review

Quoted 11 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or per

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of unique

PASIDs and

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has

PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have PASID

0, 1, 2.

(quoted diff) +1 -0

+	 */  

A 2d array of xarray's seems like a poor data structure for this task.

AFACIT this wants to store a list of (device, pasid) tuples, so a simple linked list, 1d xarray vector or a red black tree seems more appropriate..

Quoted 28 lines

drivers/iommu/intel/iommu.c +9 -0

+	if (entry) {
+		pinfo = entry;
+	} else {
+		pinfo = kzalloc(sizeof(*pinfo), GFP_ATOMIC);
+		if (!pinfo)
+			return -ENOMEM;
+		pinfo->pasid = pasid;
+		/* Store the new PASID info in the per domain array */
+		ret = xa_err(__xa_store(&dmar_domain->pasids, pasid,

pinfo,

drivers/iommu/intel/iommu.c +5 -0

+			     GFP_ATOMIC));
+		if (ret)
+			goto xa_store_err;
+	}
+	/* Store PASID in per device-domain array, this is for

tracking devTLB */

(quoted diff) +1 -0

+	ret = xa_err(xa_store(&info->pasids, pasid, pinfo,

GFP_ATOMIC));

drivers/iommu/intel/iommu.c +10 -0

+	if (ret)
+		goto xa_store_err;
+
+	atomic_inc(&pinfo->users);
+	xa_unlock(&dmar_domain->pasids);
+
+	return 0;
+
+xa_store_err:
+	xa_unlock(&dmar_domain->pasids);

drivers/iommu/intel/iommu.c static bool dev_is_real_dma_subdevice(struct device *dev) · line 2705

Quoted 6 lines

drivers/iommu/intel/iommu.c +6 -0

+	spin_lock_irqsave(&iommu->lock, flags);
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+	spin_unlock_irqrestore(&iommu->lock, flags);
+
+	if (!atomic_read(&pinfo->users)) {
+		__xa_erase(&dmar_domain->pasids, pasid);  

This isn't locked right

Hi Jason,

Quoted 15 lines · Jason Gunthorpe

Review

Quoted 11 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or per

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of unique

PASIDs and

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has

PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have PASID

0, 1, 2.

(quoted diff) +1 -0

+	 */  

A 2d array of xarray's seems like a poor data structure for this task.

Quoted 4 lines

AFACIT this wants to store a list of (device, pasid) tuples, so a simple linked list, 1d xarray vector or a red black tree seems more appropriate..

Agreed.
It might need some surgery for dmar_domain and device_domain_info, which already has a simple device list. I am trying to leverage the existing data struct, let me take a closer look.

Quoted 19 lines · Jacob Pan

Hi Jason,

Quoted 15 lines · Jason Gunthorpe

Review

Quoted 11 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or per

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of

unique PASIDs and

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has

PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have

PASID 0, 1, 2.

(quoted diff) +1 -0

+	 */    

A 2d array of xarray's seems like a poor data structure for this task.

Quoted 8 lines

Quoted 4 lines

AFACIT this wants to store a list of (device, pasid) tuples, so a simple linked list, 1d xarray vector or a red black tree seems more appropriate..

Agreed.
It might need some surgery for dmar_domain and device_domain_info, which already has a simple device list. I am trying to leverage the existing data struct, let me take a closer look.

Maybe the core code should provide this data structure in the iommu_domain.

Figuring out what stuff is attached is something every driver has to do right?

I guess a list of (device, pasid) tuples as you suggested could work but it will have duplicated device entries since each device could have multiple PASIDs. right?

I would expect real applications will try to use the same PASID for the same IOVA map to optimize IOTLB caching.

Kernel users (ring0) can supply any PASID when they use the ENQCMDS instruction. Is that what you mean when you say "real applications"?

From: Jason Gunthorpe <[email protected]>
Sent: Thursday, March 17, 2022 8:04 AM

Quoted 3 lines · Tony

Kernel users (ring0) can supply any PASID when they use the ENQCMDS instruction. Is that what you mean when you say "real applications"?

I'm not talking about ENQCMD at all

I'm saying I don't see much utility to have two PASIDs assigned to the same device that perform the same IOVA translation.

So I would expect the list of attachments in an iommu_domain to have exactly one device in it and multiple PASIDs to the same device from the same iommu_domain is a oddball corner case we don't need to optimize for, beyond being able to allow it to happen.

may remember more detail here) and we didn't hear strong interest from customer on it. So this is just FYI for theoretical possibility and I'm fine with even disallowing it before those issues are resolved.

Quoted 3 lines · Jacob Pan

I guess a list of (device, pasid) tuples as you suggested could work but it will have duplicated device entries since each device could have multiple PASIDs. right?

Is assigning the same iommu_domain to multiple PASIDs of the same device something worth optimizing for?

I would expect real applications will try to use the same PASID for the same IOVA map to optimize IOTLB caching.

Is there a use case for that I'm missing?

Otherwise your explanation is what I was imagining as well.

I would also think about expanding your struct so that the device driver can track per-device per-domain data as well, that seems useful IIRC?

ie put a 'sizeof_iommu_dev_pasid_data' in the domain->ops and allocate that much memory so the driver can use the trailer space for its own purpose.

Quoted 5 lines

I would expect real applications will try to use the same PASID for the same IOVA map to optimize IOTLB caching.

Is there a use case for that I'm missing?

Yes. it would be more efficient for PASID selective domain TLB flush. But on VT-d IOTLB is also tagged by domain ID, domain flush can use DID if there are many PASIDs. Not sure about other archs. Agree that optimizing PASIDs for TLB flush should be a common goal.

Quoted 6 lines

Otherwise your explanation is what I was imagining as well.

I would also think about expanding your struct so that the device driver can track per-device per-domain data as well, that seems useful IIRC?

yes, at least both VT-d and FSL drivers have struct device_domain_info.

Quoted 4 lines

ie put a 'sizeof_iommu_dev_pasid_data' in the domain->ops and allocate that much memory so the driver can use the trailer space for its own purpose.

That sounds great to have but not sure i understood correctly how to do it.

Do you mean for each vendor driver's struct device_domain_info (or equivalent), we carve out sizeof_iommu_dev_pasid_data as common data, then the rest of the space is vendor specific? I don't feel I get your point, could you elaborate?

Quoted 9 lines · Jacob Pan

Quoted 5 lines

I would expect real applications will try to use the same PASID for the same IOVA map to optimize IOTLB caching.

Is there a use case for that I'm missing?

Yes. it would be more efficient for PASID selective domain TLB flush. But on VT-d IOTLB is also tagged by domain ID, domain flush can use DID if there are many PASIDs. Not sure about other archs. Agree that optimizing PASIDs for TLB flush should be a common goal.

If you sort the list of (device, pasid) tuples can something like VT-d collapse all the same devices and just issue one DID invalidation:

list_for_each()

    if (itm->device == last_invalidated_device)
          continue;
    invalidate(itm->device);
    last_invalidated_device = itm->device;

While something that was per-pasid could issue per-pasid invalidations from the same data structure?

Quoted 12 lines

Quoted 6 lines

Otherwise your explanation is what I was imagining as well.

I would also think about expanding your struct so that the device driver can track per-device per-domain data as well, that seems useful IIRC?

yes, at least both VT-d and FSL drivers have struct device_domain_info.

Quoted 4 lines

ie put a 'sizeof_iommu_dev_pasid_data' in the domain->ops and allocate that much memory so the driver can use the trailer space for its own purpose.

Quoted 7 lines

That sounds great to have but not sure i understood correctly how to do it.

Do you mean for each vendor driver's struct device_domain_info (or equivalent), we carve out sizeof_iommu_dev_pasid_data as common data, then the rest of the space is vendor specific? I don't feel I get your point, could you elaborate?

I've seen it done two ways..

With a flex array:

 struct iommu_device_data {
     struct list_head list
     ioasid_t pasid;
     struct device *dev;
     [..]
     u64 device_data[];

}

 struct intel_device_data {
      [..]

}
struct iommu_device_data *dev_data;
struct intel_device_data *intel_data = (void *)&dev_data->device_data;

Or with container of:

 struct iommu_device_data {
     struct list_head list
     ioasid_t pasid;
     struct device *dev;
     [..]

}

struct intel_device_data {
struct iommu_device_data iommu; // must be first [...]
}
struct iommu_device_data *dev_data;
struct intel_device_data *intel_data = container_of(dev_data, struct
intel_device_data, iommu);

In either case you'd add a size_t to the domain->ops specifying how much extra memory for the core code to allocate when it manages the datastructure. The first case allocates based on struct_size, the second case allocates what is specified.

Look at INIT_RDMA_OBJ_SIZE() for some more complicated example how the latter can work. I like it because it has the nice container_of pattern in drivers, the downside is it requires a BUILD_BUG_ON to check that the driver ordered its struct properly.

The point is to consolidate all the code for allocating and walking the data structure without having to force two allocations and extra pointer indirections on performance paths.

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, March 15, 2022 10:33 PM

Review

Quoted 2 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with shared or

per

Review

Quoted 1 line

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of unique PASIDs

and

drivers/iommu/intel/iommu.c static bool dev_is_real_dma_subdevice(struct device *dev) · line 2673

Quoted 4 lines

drivers/iommu/intel/iommu.c +4 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has PASID 0, 1;
+	 * device B has PASID 0, 2. Then the domain would have PASID 0, 1, 2.
+	 */

A 2d array of xarray's seems like a poor data structure for this task.

AFACIT this wants to store a list of (device, pasid) tuples, so a simple linked list, 1d xarray vector or a red black tree seems more appropriate..

Quoted 11 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, March 15, 2022 10:33 PM

Quoted 3 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or

per

Quoted 2 lines

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of

unique PASIDs

and

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has

PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have PASID

0, 1, 2.

(quoted diff) +1 -0

+	 */  

A 2d array of xarray's seems like a poor data structure for this task.

besides that it also doesn't work when we support per-device PASID allocation in the future. In that case merging device PASIDs together is conceptually wrong.

Quoted 5 lines

AFACIT this wants to store a list of (device, pasid) tuples, so a simple linked list, 1d xarray vector or a red black tree seems more appropriate..

this tuple can well serve per-device PASID. 😊

Thanks
Kevin

From: Jacob Pan
Sent: Thursday, March 17, 2022 5:02 AM

Hi Kevin,

Quoted 11 lines · Kevin

Quoted 11 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, March 15, 2022 10:33 PM

Quoted 3 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or

per

Quoted 2 lines

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of

unique PASIDs

and

Quoted 9 lines

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A has

PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have PASID

0, 1, 2.

(quoted diff) +1 -0

+	 */

A 2d array of xarray's seems like a poor data structure for this task.

Perhaps i mis-presented here, I am not using 2D array. It is an 1D xarray for domain PASIDs only. Then I use the existing device list in each domain, adding another xa to track per-device-domain PASIDs.

Quoted 4 lines

besides that it also doesn't work when we support per-device PASID allocation in the future. In that case merging device PASIDs together is conceptually wrong.

Sorry, could you elaborate? If we do per-dev PASID allocation, we could use the ioasid_set for each pdev, right?

Quoted 36 lines

From: Jacob Pan
Sent: Thursday, March 17, 2022 5:02 AM

Hi Kevin,

Quoted 11 lines · Kevin

Quoted 11 lines

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, March 15, 2022 10:33 PM

Quoted 3 lines · Jacob Pan

(quoted diff) +2 -0

+	/*
+	 * Each domain could have multiple devices attached with

shared or

per

Quoted 2 lines

(quoted diff) +1 -0

+	 * device PASIDs. At the domain level, we keep track of

unique PASIDs

and

Quoted 10 lines

Quoted 9 lines

Review

Quoted 6 lines

(quoted diff) +2 -0

+	 * device user count.
+	 * E.g. If a domain has two devices attached, device A

has PASID 0, 1;

(quoted diff) +1 -0

+	 * device B has PASID 0, 2. Then the domain would have

PASID 0, 1, 2.

(quoted diff) +1 -0

+	 */  

A 2d array of xarray's seems like a poor data structure for this task.

Perhaps i mis-presented here, I am not using 2D array. It is an 1D xarray for domain PASIDs only. Then I use the existing device list in each domain, adding another xa to track per-device-domain PASIDs.

Quoted 4 lines

besides that it also doesn't work when we support per-device PASID allocation in the future. In that case merging device PASIDs together is conceptually wrong.

Quoted 2 lines

Sorry, could you elaborate? If we do per-dev PASID allocation, we could use the ioasid_set for each pdev, right?

My point is simply about the comment above which says the domain will have PASID 0, 1, 2 when there is [devA, PASID0] and [devB, PASID0]. You can maintain a single PASID list only when it's globally allocated cross devices. otherwise this has to be a tuple including device and PASID.

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

drivers/iommu/intel/iommu.c +3 -0

+static int intel_iommu_attach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t pasid)
+{

Quoted 10 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

(quoted diff) +2 -0

+static int intel_iommu_attach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev, ioasid_t

pasid) +{

drivers/iommu/intel/iommu.c +5 -0

+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct device_domain_info *info = get_domain_info(dev);
+	struct intel_iommu *iommu = info->iommu;
+	struct pasid_info *pinfo;
+	unsigned long flags;

drivers/iommu/intel/iommu.c static bool dev_is_real_dma_subdevice(struct device *dev) · line 2631

Quoted 5 lines

drivers/iommu/intel/iommu.c +5 -0

+	int ret = 0;
+	void *entry;
+
+	if (!info)
+		return -ENODEV;  

btw this interface only works in scalable mode. Lack of a check to return error on legacy mode here.

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

With the availability of a generic device-PASID-domain attachment API, there's no need to special case RID2PASID. Use the API to replace duplicated code.

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/iommu/intel/iommu.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

drivers/iommu/intel/iommu.c +0 -0

index 9267194eaed3..f832b7599d21 100644

Quoted 21 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

With the availability of a generic device-PASID-domain attachment API, there's no need to special case RID2PASID. Use the API to replace duplicated code.

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/iommu/intel/iommu.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

drivers/iommu/intel/iommu.c +0 -0

index 9267194eaed3..f832b7599d21 100644
@@ -1683,9 +1683,6 @@ static void domain_flush_piotlb(struct
 intel_iommu *iommu,
 		qi_flush_piotlb(iommu, did, domain->default_pasid,
 				addr, npages, ih);
 

drivers/iommu/intel/iommu.c static void domain_flush_piotlb(struct intel_iommu *iommu, · line 1688

Quoted 4 lines

drivers/iommu/intel/iommu.c +0 -3

-	if (!list_empty(&domain->devices))
-		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr,
 npages, ih);
-  

this should be rebased on top of Baolu's "iommu cleanup and refactoring" series which has removed the entire domain_flush_piotlb().

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

From: Dave Jiang <[email protected]>

The idxd driver always gated the pasid enabling under a single knob and this assumption is incorrect. The pasid used for kernel operation can be independently toggled and has no dependency on the user pasid (and vice versa). Split the two so they are independent "enabled" flags.

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>

---
  drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++
  include/linux/dma-iommu.h |  7 +++++
  include/linux/iommu.h     |  9 ++++++
  3 files changed, 81 insertions(+)

drivers/iommu/dma-iommu.c +65 -0

index b22034975301..d0ff1a34b1b6 100644
@@ -39,6 +39,8 @@ enum iommu_dma_cookie_type {
  	IOMMU_DMA_MSI_COOKIE,
  };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
  struct iommu_dma_cookie {
  	enum iommu_dma_cookie_type	type;
  	union {

@@ -370,6 +372,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
  	domain->iova_cookie = NULL;
  }
 
+/**
+ * iommu_enable_pasid_dma --Enable in-kernel DMA request with PASID
+ * @dev:	Device to be enabled
+ *
+ * DMA request with PASID will be mapped the same way as the legacy DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA map, the supervisor PASID will point to the same IOVA
+ * page table.
+ *
+ * @return the kernel PASID to be used for DMA or INVALID_IOASID on failure
+ */
+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+	max = iommu_get_dev_pasid_max(dev);
+	if (!max)
+		return -EINVAL;
+
+	id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+	if (id == INVALID_IOASID)
+		return -ENOMEM;
+
+	ret = dom->ops->attach_dev_pasid(dom, dev, id);
+	if (ret) {
+		ioasid_put(id);
+		return ret;
+	}
+	*pasid = id;
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_enable_pasid_dma);
+
+/**
+ * iommu_disable_pasid_dma --Disable in-kernel DMA request with PASID
+ * @dev:	Device's PASID DMA to be disabled
+ *
+ * It is the device driver's responsibility to ensure no more incoming DMA
+ * requests with the kernel PASID before calling this function. IOMMU driver
+ * ensures PASID cache, IOTLBs related to the kernel PASID are cleared and
+ * drained.
+ *
+ * @return 0 on success or error code on failure
+ */
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid)
+{
+	struct iommu_domain *dom;
+
+	/* TODO: check the given PASID is within the ioasid_set */
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom->ops->detach_dev_pasid)
+		return;
+	dom->ops->detach_dev_pasid(dom, dev, pasid);
+	ioasid_put(pasid);
+}
+EXPORT_SYMBOL(iommu_disable_pasid_dma);
+
  /**
   * iommu_dma_get_resv_regions - Reserved region driver helper
   * @dev: Device from iommu_get_resv_regions()

include/linux/dma-iommu.h +7 -0

index 24607dc3c2ac..e6cb9b52a420 100644
@@ -18,6 +18,13 @@ int iommu_get_dma_cookie(struct iommu_domain *domain);
  int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
  void iommu_put_dma_cookie(struct iommu_domain *domain);
 
+/*
+ * For devices that can do DMA request with PASID, setup a system PASID.
+ * Address modes (IOVA, PA) are selected by the platform code.
+ */
+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid);
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid);
+
  /* Setup call for arch DMA mapping code */
  void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
  int iommu_dma_init_fq(struct iommu_domain *domain);

include/linux/iommu.h +2 -0

index fde5b933dbe3..fb011722e4f8 100644
@@ -395,6 +395,15 @@ static inline void iommu_set_dev_pasid_max(struct device *dev,
 
  	param->pasid_max = max;
  }
+static inline ioasid_t iommu_get_dev_pasid_max(struct device *dev)
+{

Quoted 12 lines · Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

Quoted 4 lines

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Surely the main point of PASIDs is to be able to use more than one of them?

Quoted 18 lines · Robin Murphy

Quoted 16 lines · Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Quoted 2 lines

Surely the main point of PASIDs is to be able to use more than one of them?

IMHO, not for the DMA API.

I can't think of good reasons why a single in-kernel device should require more than one iommu_domain for use by the DMA API. Even with the SIOV cases we have been looking at we don't really see a use case for more than one DMA API iommu_domain on a single physical device. Do you know of something on the horizon?

From my view the main point of PASIDs is to assign iommu_domains that are not used by the DMA API.

IMHO it is a device mis-design of IDXD to require all DMA be PASID tagged. Devices should be able to do DMA on their RID when the PCI

function is controlled by a kernel driver. I see this driver facing API as addressing a device quirk by aliasing the DMA API of the RID into a PASID and that is really all it is good for.

In any case I think we are better to wait for an actual user for multi DMA API iommu_domains to come forward before we try to build an API for it.

Quoted 2 lines

IMHO it is a device mis-design of IDXD to require all DMA be PASID tagged. Devices should be able to do DMA on their RID when the PCI

IDXD can do DMA w/ RID, the PASID requirement is only for shared WQ where ENQCMDS is used. ENQCMDS has the benefit of avoiding locking where work submission is done from multiple CPUs.
Tony, Dave?

Quoted 3 lines

In any case I think we are better to wait for an actual user for multi DMA API iommu_domains to come forward before we try to build an API for it.

What would you recommend in the interim?

Quoted 7 lines · Jacob Pan

Quoted 2 lines

IMHO it is a device mis-design of IDXD to require all DMA be PASID tagged. Devices should be able to do DMA on their RID when the PCI

IDXD can do DMA w/ RID, the PASID requirement is only for shared WQ where ENQCMDS is used. ENQCMDS has the benefit of avoiding locking where work submission is done from multiple CPUs.
Tony, Dave?

This is what I mean, it has an operating mode you want to use from the kernel driver that cannot do RID DMA. It is a HW mis-design, IMHO.

Something like PASID0 in the ENQCMDS should have triggered RID DMA.

Quoted 5 lines

Quoted 3 lines

In any case I think we are better to wait for an actual user for multi DMA API iommu_domains to come forward before we try to build an API for it.

What would you recommend in the interim?

Oh, I mean this approach at a high level is fine - I was saying we shouldn't try to broaden it like Robin was suggesting without a driver that needs multiple iommu_domains for the DMA API.

Jason

From: Jacob Pan <[email protected]>
Sent: Wednesday, March 16, 2022 5:24 AM

Hi Jason,

Quoted 12 lines · Jason Gunthorpe

Quoted 7 lines · Jacob Pan

Quoted 2 lines

IMHO it is a device mis-design of IDXD to require all DMA be PASID tagged. Devices should be able to do DMA on their RID when the PCI

IDXD can do DMA w/ RID, the PASID requirement is only for shared WQ where ENQCMDS is used. ENQCMDS has the benefit of avoiding locking where work submission is done from multiple CPUs.
Tony, Dave?

This is what I mean, it has an operating mode you want to use from the kernel driver that cannot do RID DMA. It is a HW mis-design, IMHO.

Quoted 3 lines

Something like PASID0 in the ENQCMDS should have triggered RID DMA.

That would simplify things a lot, it would be just a device change I think. From IA pov, only ENQCMD will #GP if PASID==0. I will bring this back to HW team to consider for future generations.

From: Jason Gunthorpe <[email protected]>
Sent: Tuesday, March 15, 2022 10:22 PM

Quoted 13 lines · Robin Murphy

Quoted 12 lines · Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

Quoted 2 lines

Quoted 2 lines

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs

as

Quoted 5 lines

Quoted 2 lines

is. There is no difference in dma_handle between without PASID and with PASID.

Surely the main point of PASIDs is to be able to use more than one of them?

IMHO, not for the DMA API.

I can't think of good reasons why a single in-kernel device should require more than one iommu_domain for use by the DMA API. Even with the SIOV cases we have been looking at we don't really see a use case for more than one DMA API iommu_domain on a single physical device.

Do you know of something on the horizon?

From my view the main point of PASIDs is to assign iommu_domains that are not used by the DMA API.

IMHO it is a device mis-design of IDXD to require all DMA be PASID tagged. Devices should be able to do DMA on their RID when the PCI function is controlled by a kernel driver. I see this driver facing API as addressing a device quirk by aliasing the DMA API of the RID into a PASID and that is really all it is good for.

In any case I think we are better to wait for an actual user for multi DMA API iommu_domains to come forward before we try to build an API for it.

Jason

1) When the kernel wants a more scalable way of using IDXD e.g. having multiple CPUs simultaneously submitting works in a lockless way to a shared work queue via a new instruction (ENQCMD) which carries PASID.

2) When the host wants to share a workqueue between multiple VMs. In that case the virtual IDXD device exposed to each VM will only support the shared workqueue mode. Only in this case the DMA API in the guest must be attached by a PASID as ENQCMD is the only way to submit works.

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>

 drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++
 include/linux/iommu.h     |  9 ++++++
 3 files changed, 81 insertions(+)

drivers/iommu/dma-iommu.c +13 -0

index b22034975301..d0ff1a34b1b6 100644
+++ b/drivers/iommu/dma-iommu.c

@@ -39,6 +39,8 @@ enum iommu_dma_cookie_type {
 	IOMMU_DMA_MSI_COOKIE,
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 struct iommu_dma_cookie {
 	enum iommu_dma_cookie_type	type;
 	union {

@@ -370,6 +372,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/**
+ * iommu_enable_pasid_dma --Enable in-kernel DMA request with PASID
+ * @dev:	Device to be enabled
+ *
+ * DMA request with PASID will be mapped the same way as the legacy DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA map, the supervisor PASID will point to the same IOVA
+ * page table.
+ *
+ * @return the kernel PASID to be used for DMA or INVALID_IOASID on failure

Quoted 54 lines · Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>

 drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |  7 +++++
 include/linux/iommu.h     |  9 ++++++
 3 files changed, 81 insertions(+)

drivers/iommu/dma-iommu.c +8 -0

index b22034975301..d0ff1a34b1b6 100644
+++ b/drivers/iommu/dma-iommu.c

@@ -39,6 +39,8 @@ enum iommu_dma_cookie_type {
 	IOMMU_DMA_MSI_COOKIE,
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 struct iommu_dma_cookie {
 	enum iommu_dma_cookie_type	type;
 	union {

@@ -370,6 +372,69 @@ void iommu_put_dma_cookie(struct iommu_domain
 *domain) domain->iova_cookie = NULL;
 }
 
+/**
+ * iommu_enable_pasid_dma --Enable in-kernel DMA request with PASID
+ * @dev:	Device to be enabled
+ *
+ * DMA request with PASID will be mapped the same way as the legacy

DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA map, the supervisor PASID will point to the same IOVA
+ * page table.
+ *
+ * @return the kernel PASID to be used for DMA or INVALID_IOASID on failure
+ */

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 394

Quoted 9 lines

drivers/iommu/dma-iommu.c +9 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;  

Given the purpose of this API I think it should assert that the device has the DMA API in-use using the machinery from the other series.

ie this should not be used to clone non-DMA API iommu_domains..

Quoted 8 lines

include/linux/dma-iommu.h +1 -0

index 24607dc3c2ac..e6cb9b52a420 100644
+++ b/include/linux/dma-iommu.h

@@ -18,6 +18,13 @@ int iommu_get_dma_cookie(struct iommu_domain
 *domain); int iommu_get_msi_cookie(struct iommu_domain *domain,

dma_addr_t base); void iommu_put_dma_cookie(struct iommu_domain *domain);
+/*

include/linux/dma-iommu.h int iommu_get_dma_cookie(struct iommu_domain *domain); · line 26

Quoted 6 lines

+ * For devices that can do DMA request with PASID, setup a system PASID.
+ * Address modes (IOVA, PA) are selected by the platform code.
+ */

include/linux/dma-iommu.h +2 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid);
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid);  

The functions already have a kdoc, don't need two..

Quoted 13 lines

drivers/iommu/dma-iommu.c void iommu_put_dma_cookie(struct iommu_domain *domain) · line 394

Quoted 9 lines

drivers/iommu/dma-iommu.c +9 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;  

Given the purpose of this API I think it should assert that the device has the DMA API in-use using the machinery from the other series.

Quoted 2 lines

ie this should not be used to clone non-DMA API iommu_domains..

Let me try to confirm the specific here. I should check domain type and rejects all others except IOMMU_DOMAIN_DMA type, right? Should also allow IOMMU_DOMAIN_IDENTITY.

That makes sense.

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>

---
  drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++
  include/linux/dma-iommu.h |  7 +++++
  include/linux/iommu.h     |  9 ++++++
  3 files changed, 81 insertions(+)

drivers/iommu/dma-iommu.c +2 -0

index b22034975301..d0ff1a34b1b6 100644
@@ -39,6 +39,8 @@ enum iommu_dma_cookie_type {
  	IOMMU_DMA_MSI_COOKIE,
  };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
  struct iommu_dma_cookie {
  	enum iommu_dma_cookie_type	type;
  	union {

@@ -370,6 +372,69 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
  	domain->iova_cookie = NULL;
  }
 

drivers/iommu/dma-iommu.c +29 -0

+ */
+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+	max = iommu_get_dev_pasid_max(dev);
+	if (!max)
+		return -EINVAL;
+
+	id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+	if (id == INVALID_IOASID)
+		return -ENOMEM;
+
+	ret = dom->ops->attach_dev_pasid(dom, dev, id);
+	if (ret) {
+		ioasid_put(id);
+		return ret;
+	}
+	*pasid = id;
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_enable_pasid_dma);
+

drivers/iommu/dma-iommu.c +14 -0

+ */
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid)
+{
+	struct iommu_domain *dom;
+
+	/* TODO: check the given PASID is within the ioasid_set */
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom->ops->detach_dev_pasid)
+		return;
+	dom->ops->detach_dev_pasid(dom, dev, pasid);
+	ioasid_put(pasid);
+}
+EXPORT_SYMBOL(iommu_disable_pasid_dma);
+
  /**
   * iommu_dma_get_resv_regions - Reserved region driver helper
   * @dev: Device from iommu_get_resv_regions()

include/linux/dma-iommu.h +0 -0

index 24607dc3c2ac..e6cb9b52a420 100644

include/linux/dma-iommu.h +3 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid);
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid);
+
  /* Setup call for arch DMA mapping code */
  void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
  int iommu_dma_init_fq(struct iommu_domain *domain);

include/linux/iommu.h +2 -0

index fde5b933dbe3..fb011722e4f8 100644
@@ -395,6 +395,15 @@ static inline void iommu_set_dev_pasid_max(struct device *dev,
 
  	param->pasid_max = max;
  }
+static inline ioasid_t iommu_get_dev_pasid_max(struct device *dev)
+{

Quoted 50 lines · Jacob Pan

DMA mapping API is the de facto standard for in-kernel DMA. It operates on a per device/RID basis which is not PASID-aware.

Some modern devices such as Intel Data Streaming Accelerator, PASID is required for certain work submissions. To allow such devices use DMA mapping API, we need the following functionalities:
1. Provide device a way to retrieve a PASID for work submission within the kernel
2. Enable the kernel PASID on the IOMMU for the device
3. Attach the kernel PASID to the device's default DMA domain, let it be IOVA or physical address in case of pass-through.

This patch introduces a driver facing API that enables DMA API PASID usage. Once enabled, device drivers can continue to use DMA APIs as is. There is no difference in dma_handle between without PASID and with PASID.

Signed-off-by Jacob Pan <[email protected]>

---
  drivers/iommu/dma-iommu.c | 65 +++++++++++++++++++++++++++++++++++++++
  include/linux/dma-iommu.h |  7 +++++
  include/linux/iommu.h     |  9 ++++++
  3 files changed, 81 insertions(+)

drivers/iommu/dma-iommu.c +7 -0

index b22034975301..d0ff1a34b1b6 100644
@@ -39,6 +39,8 @@ enum iommu_dma_cookie_type {
  	IOMMU_DMA_MSI_COOKIE,
  };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
  struct iommu_dma_cookie {
  	enum iommu_dma_cookie_type	type;
  	union {

@@ -370,6 +372,69 @@ void iommu_put_dma_cookie(struct iommu_domain
 *domain) domain->iova_cookie = NULL;
  }
 
+/**
+ * iommu_enable_pasid_dma --Enable in-kernel DMA request with PASID
+ * @dev:	Device to be enabled
+ *
+ * DMA request with PASID will be mapped the same way as the legacy

DMA.
+ * If the device is in pass-through, PASID will also pass-through. If the
+ * device is in IOVA map, the supervisor PASID will point to the same

Quoted 5 lines

IOVA
+ * page table.
+ *
+ * @return the kernel PASID to be used for DMA or INVALID_IOASID on failure

The comment on the return value should be rephrased according to the real code.

Quoted 36 lines

+ */

drivers/iommu/dma-iommu.c +33 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+	max = iommu_get_dev_pasid_max(dev);
+	if (!max)
+		return -EINVAL;
+
+	id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+	if (id == INVALID_IOASID)
+		return -ENOMEM;
+
+	ret = dom->ops->attach_dev_pasid(dom, dev, id);
+	if (ret) {
+		ioasid_put(id);
+		return ret;
+	}
+	*pasid = id;
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_enable_pasid_dma);
+
+/**
+ * iommu_disable_pasid_dma --Disable in-kernel DMA request with PASID
+ * @dev:	Device's PASID DMA to be disabled
+ *
+ * It is the device driver's responsibility to ensure no more incoming

DMA
+ * requests with the kernel PASID before calling this function. IOMMU

Quoted 6 lines

driver
+ * ensures PASID cache, IOTLBs related to the kernel PASID are cleared and
+ * drained.
+ *
+ * @return 0 on success or error code on failure

Ditto.

Quoted 24 lines

+ */

drivers/iommu/dma-iommu.c +13 -0

+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid)
+{
+	struct iommu_domain *dom;
+
+	/* TODO: check the given PASID is within the ioasid_set */
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom->ops->detach_dev_pasid)
+		return;
+	dom->ops->detach_dev_pasid(dom, dev, pasid);
+	ioasid_put(pasid);
+}
+EXPORT_SYMBOL(iommu_disable_pasid_dma);
+
  /**
   * iommu_dma_get_resv_regions - Reserved region driver helper
   * @dev: Device from iommu_get_resv_regions()

include/linux/dma-iommu.h +0 -0

index 24607dc3c2ac..e6cb9b52a420 100644
@@ -18,6 +18,13 @@ int iommu_get_dma_cookie(struct iommu_domain
 *domain); int iommu_get_msi_cookie(struct iommu_domain *domain,

dma_addr_t base); void iommu_put_dma_cookie(struct iommu_domain

Quoted 6 lines

*domain);
+/*
+ * For devices that can do DMA request with PASID, setup a system PASID.
+ * Address modes (IOVA, PA) are selected by the platform code.
+ */

No need for a comment here. Or move it to the function if need.

Quoted 24 lines

include/linux/dma-iommu.h +3 -0

+int iommu_enable_pasid_dma(struct device *dev, ioasid_t *pasid);
+void iommu_disable_pasid_dma(struct device *dev, ioasid_t pasid);
+
  /* Setup call for arch DMA mapping code */
  void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64

dma_limit); int iommu_dma_init_fq(struct iommu_domain *domain);

include/linux/iommu.h +9 -0

index fde5b933dbe3..fb011722e4f8 100644
@@ -395,6 +395,15 @@ static inline void iommu_set_dev_pasid_max(struct
 device *dev, 
  	param->pasid_max = max;
  }
+static inline ioasid_t iommu_get_dev_pasid_max(struct device *dev)
+{
+	struct dev_iommu *param = dev->iommu;
+
+	if (WARN_ON(!param))
+		return 0;
+
+	return param->pasid_max;
+}
 

include/linux/iommu.h static inline void iommu_set_dev_pasid_max(struct device *dev, · line 409

Quoted 2 lines

include/linux/iommu.h +0 -0

  int iommu_device_register(struct iommu_device *iommu,
  			  const struct iommu_ops *ops,  

Best regards,
baolu

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

The current in-kernel supervisor PASID support is based on the SVM/SVA machinery in SVA lib. The binding between a kernel PASID and kernel mapping has many flaws. See discussions in the link below.

This patch enables in-kernel DMA by switching from SVA lib to the standard DMA mapping APIs. Since both DMA requests with and without PASIDs are mapped identically, there is no change to how DMA APIs are used after the kernel PASID is enabled.

Link https://lore.kernel.org/linux-

iommu/[email protected]/

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/dma/idxd/idxd.h  |  1 -
 drivers/dma/idxd/init.c  | 34 +++++++++-------------------------
 drivers/dma/idxd/sysfs.c |  7 -------
 3 files changed, 9 insertions(+), 33 deletions(-)

drivers/dma/idxd/idxd.h +0 -1

index da72eb15f610..a09ab4a6e1c1 100644
@@ -276,7 +276,6 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
 	unsigned int pasid;
 
 	int num_groups;

drivers/dma/idxd/init.c +1 -0

index 08a5f4310188..5d1f8dd4abf6 100644
@@ -16,6 +16,7 @@
 #include <linux/idr.h>
 #include <linux/intel-svm.h>
 #include <linux/iommu.h>
+#include <linux/dma-iommu.h>
 #include <uapi/linux/idxd.h>
 #include <linux/dmaengine.h>
 #include "../dmaengine.h"

drivers/dma/idxd/init.c +8 -22

 {
-	int flags;
-	unsigned int pasid;
-	struct iommu_sva *sva;
+	u32 pasid;
+	int ret;
 
-	flags = SVM_FLAG_SUPERVISOR_MODE;
-
-	sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL, &flags);
-	if (IS_ERR(sva)) {
-		dev_warn(&idxd->pdev->dev,
-			 "iommu sva bind failed: %ld\n", PTR_ERR(sva));
-		return PTR_ERR(sva);
-	}
-
-	pasid = iommu_sva_get_pasid(sva);
-	if (pasid == IOMMU_PASID_INVALID) {
-		iommu_sva_unbind_device(sva);
-		return -ENODEV;
+	ret = iommu_enable_pasid_dma(&idxd->pdev->dev, &pasid);
+	if (ret) {
+		dev_err(&idxd->pdev->dev, "No DMA PASID %d\n", ret);
+		return ret;
 	}
-
-	idxd->sva = sva;
 	idxd->pasid = pasid;
-	dev_dbg(&idxd->pdev->dev, "system pasid: %u\n", pasid);
+
 	return 0;
 }
 
 static void idxd_disable_system_pasid(struct idxd_device *idxd)
 {
-
-	iommu_sva_unbind_device(idxd->sva);
-	idxd->sva = NULL;
+	iommu_disable_pasid_dma(&idxd->pdev->dev, idxd->pasid);
 }
 

static int idxd_probe(struct idxd_device *idxd)

drivers/dma/idxd/init.c +0 -1

 	}
-
 	idxd_read_caps(idxd);
 	idxd_read_table_offsets(idxd);
 

drivers/dma/idxd/sysfs.c +0 -7

index 7e19ab92b61a..fde6656695ba 100644
@@ -839,13 +839,6 @@ static ssize_t wq_name_store(struct device *dev,
 	if (strlen(buf) > WQ_NAME_SIZE || strlen(buf) == 0)
 		return -EINVAL;
 
-	/*
-	 * This is temporarily placed here until we have SVM support for
-	 * dmaengine.
-	 */
-	if (wq->type == IDXD_WQT_KERNEL && device_pasid_enabled(wq-
 >idxd))
-		return -EOPNOTSUPP;
-
 	memset(wq->name, 0, WQ_NAME_SIZE + 1);
 	strncpy(wq->name, buf, WQ_NAME_SIZE);

strreplace(wq->name, '\n', '\0');
--
2.25.1

Quoted 48 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 15, 2022 1:07 PM

The current in-kernel supervisor PASID support is based on the SVM/SVA machinery in SVA lib. The binding between a kernel PASID and kernel mapping has many flaws. See discussions in the link below.

This patch enables in-kernel DMA by switching from SVA lib to the standard DMA mapping APIs. Since both DMA requests with and without PASIDs are mapped identically, there is no change to how DMA APIs are used after the kernel PASID is enabled.

Link https://lore.kernel.org/linux-

iommu/[email protected]/

Signed-off-by Jacob Pan <[email protected]>

---
 drivers/dma/idxd/idxd.h  |  1 -
 drivers/dma/idxd/init.c  | 34 +++++++++-------------------------
 drivers/dma/idxd/sysfs.c |  7 -------
 3 files changed, 9 insertions(+), 33 deletions(-)

drivers/dma/idxd/idxd.h +0 -1

index da72eb15f610..a09ab4a6e1c1 100644
@@ -276,7 +276,6 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
 	unsigned int pasid;
 
 	int num_groups;

drivers/dma/idxd/init.c +1 -0

index 08a5f4310188..5d1f8dd4abf6 100644
@@ -16,6 +16,7 @@
 #include <linux/idr.h>
 #include <linux/intel-svm.h>
 #include <linux/iommu.h>
+#include <linux/dma-iommu.h>
 #include <uapi/linux/idxd.h>
 #include <linux/dmaengine.h>
 #include "../dmaengine.h"

@@ -466,36 +467,22 @@ static struct idxd_device *idxd_alloc(struct

pci_dev *pdev, struct idxd_driver_d

Quoted 1 line

static int idxd_enable_system_pasid(struct idxd_device *idxd)

idxd_enable_pasid_dma() since system pasid is a confusing term now? Or just remove the idxd specific wrappers and have the caller to call iommu_enable_pasid_dma() directly given the simple logic here.

Quoted 41 lines

{

drivers/dma/idxd/init.c +8 -22

-	int flags;
-	unsigned int pasid;
-	struct iommu_sva *sva;
+	u32 pasid;
+	int ret;
 
-	flags = SVM_FLAG_SUPERVISOR_MODE;
-
-	sva = iommu_sva_bind_device(&idxd->pdev->dev, NULL, &flags);
-	if (IS_ERR(sva)) {
-		dev_warn(&idxd->pdev->dev,
-			 "iommu sva bind failed: %ld\n", PTR_ERR(sva));
-		return PTR_ERR(sva);
-	}
-
-	pasid = iommu_sva_get_pasid(sva);
-	if (pasid == IOMMU_PASID_INVALID) {
-		iommu_sva_unbind_device(sva);
-		return -ENODEV;
+	ret = iommu_enable_pasid_dma(&idxd->pdev->dev, &pasid);
+	if (ret) {
+		dev_err(&idxd->pdev->dev, "No DMA PASID %d\n", ret);
+		return ret;
 	}
-
-	idxd->sva = sva;
 	idxd->pasid = pasid;
-	dev_dbg(&idxd->pdev->dev, "system pasid: %u\n", pasid);
+
 	return 0;
 }
 
 static void idxd_disable_system_pasid(struct idxd_device *idxd)
 {
-
-	iommu_sva_unbind_device(idxd->sva);
-	idxd->sva = NULL;
+	iommu_disable_pasid_dma(&idxd->pdev->dev, idxd->pasid);
 }

drivers/dma/idxd/init.c static int idxd_probe(struct idxd_device *idxd) · line 514

Quoted 8 lines

static int idxd_probe(struct idxd_device *idxd)

drivers/dma/idxd/init.c +0 -2

@@ -524,10 +511,7 @@ static int idxd_probe(struct idxd_device *idxd)
 		} else {
 			dev_warn(dev, "Unable to turn on SVA
 feature.\n"); }
-	} else if (!sva) {
-		dev_warn(dev, "User forced SVA off via module
 param.\n");  

why removing above 2 lines? they are related to a module param thus not affected by the logic in this series.

Quoted 19 lines

drivers/dma/idxd/init.c +0 -1

 	}
-
 	idxd_read_caps(idxd);
 	idxd_read_table_offsets(idxd);

drivers/dma/idxd/sysfs.c +0 -2

index 7e19ab92b61a..fde6656695ba 100644
@@ -839,13 +839,6 @@ static ssize_t wq_name_store(struct device *dev,
 	if (strlen(buf) > WQ_NAME_SIZE || strlen(buf) == 0)
 		return -EINVAL;
 
-	/*
-	 * This is temporarily placed here until we have SVM support

for

drivers/dma/idxd/sysfs.c +0 -3

-	 * dmaengine.
-	 */
-	if (wq->type == IDXD_WQT_KERNEL && device_pasid_enabled(wq-  

Quoted 8 lines

Quoted 1 line

idxd))

drivers/dma/idxd/sysfs.c +0 -3

-		return -EOPNOTSUPP;
-
 	memset(wq->name, 0, WQ_NAME_SIZE + 1);
 	strncpy(wq->name, buf, WQ_NAME_SIZE);
 	strreplace(wq->name, '\n', '\0');
--

2.25.1