[PATCH v4 0/6] Enable PASID for DMA API users

[PATCH v4 0/6] Enable PASID for DMA API users

[Jacob Pan]

Some modern accelerators such as Intel's Data Streaming Accelerator (DSA)
require PASID in DMA requests to be operational. Specifically, the work
submissions with ENQCMD on shared work queues require PASIDs. The use cases
include both user DMA with shared virtual addressing (SVA) and in-kernel
DMA similar to legacy DMA w/o PASID. Here we address the latter.

DMA mapping API is the de facto standard for in-kernel DMA. However, it
operates on a per device or Requester ID(RID) basis which is not
PASID-aware. To leverage DMA API for devices relies on PASIDs, this
patchset introduces the following APIs

1. A driver facing API that enables DMA API PASID usage:
iommu_attach_dma_pasid(struct device *dev, ioasid_t &pasid);
2. VT-d driver default domain op that allows attaching device-domain-PASID

Once PASID DMA is enabled and attached to the appropriate IOMMU domain,
device drivers can continue to use DMA APIs as-is. There is no difference
in terms of mapping in dma_handle between without PASID and with PASID.
The DMA mapping performed by IOMMU will be identical for both requests, let
it be IOVA or PA in case of pass-through.

In addition, this set converts DSA driver in-kernel DMA with PASID from SVA
lib to DMA API. There have been security and functional issues with the
kernel SVA approach:
(https://lore.kernel.org/linux-iommu/20210511194726.GP1002214@nvidia.com/)
The highlights are as the following:
 - The lack of IOTLB synchronization upon kernel page table updates.
   (vmalloc, module/BPF loading, CONFIG_DEBUG_PAGEALLOC etc.)
 - Other than slight more protection, using kernel virtual address (KVA)
has little advantage over physical address. There are also no use cases yet
where DMA engines need kernel virtual addresses for in-kernel DMA.

Subsequently, cleanup is done around the usage of sva_bind_device() for
in-kernel DMA. Removing special casing code in VT-d driver and tightening
SVA lib API.

This work and idea behind it is a collaboration with many people, many
thanks to Baolu Lu, Jason Gunthorpe, Dave Jiang, and others.


ChangeLog:
v4
	- Rebased on "Baolu's SVA and IOPF refactoring" series v6.
	(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v6)
	- Fixed locking for protecting iommu domain PASID data
	- Use iommu_attach_device_pasid() API instead of calling domain
	  ops directly. This will leverage the common pasid_array that
	  replaces driver specific storage in device_domain_info.
	- Added a helper function to do look up in pasid_array from
	  domain

v3
	- Rebased on "Baolu's SVA and IOPF refactoring" series v5.
	(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v5)
	This version is significantly simplified by leveraging IOMMU domain
	ops, attach_dev_pasid() op is implemented differently on a DMA domain
	than on a SVA domain.
	We currently have no need to support multiple PASIDs per DMA domain.
	(https://lore.kernel.org/lkml/20220315142216.GV11336@nvidia.com/).
	Removed PASID-device list from V2, a PASID field is introduced to
	struct iommu_domain instead. It is intended for DMA requests with
	PASID by all devices attached to the domain.

v2
	- Do not reserve a special PASID for DMA API usage. Use IOASID
	  allocation instead.
	- Introduced a generic device-pasid-domain attachment IOMMU op.
	  Replaced the DMA API only IOMMU op.
	- Removed supervisor SVA support in VT-d
	- Removed unused sva_bind_device parameters
	- Use IOMMU specific data instead of struct device to store PASID
	  info




*** SUBJECT HERE ***

*** BLURB HERE ***

Jacob Pan (6):
  iommu: Add a per domain PASID for DMA API
  iommu: Add a helper to do PASID lookup from domain
  iommu/vt-d: Implement domain ops for attach_dev_pasid
  iommu: Add PASID support for DMA mapping API users
  dmaengine: idxd: Use DMA API for in-kernel DMA with PASID
  iommu/vt-d: Delete unused SVM flag

 drivers/dma/idxd/idxd.h     |   1 -
 drivers/dma/idxd/init.c     |  34 +++--------
 drivers/dma/idxd/sysfs.c    |   7 ---
 drivers/iommu/dma-iommu.c   | 114 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/iommu.c |  72 ++++++++++++++++++++++-
 drivers/iommu/intel/svm.c   |   2 +-
 drivers/iommu/iommu.c       |  22 +++++++
 include/linux/dma-iommu.h   |   3 +
 include/linux/intel-svm.h   |  13 ----
 include/linux/iommu.h       |   8 ++-
 10 files changed, 226 insertions(+), 50 deletions(-)

-- 
2.25.1

[PATCH v4 0/6] Enable PASID for DMA API users

[Jacob Pan]

Some modern accelerators such as Intel's Data Streaming Accelerator (DSA)
require PASID in DMA requests to be operational. Specifically, the work
submissions with ENQCMD on shared work queues require PASIDs. The use cases
include both user DMA with shared virtual addressing (SVA) and in-kernel
DMA similar to legacy DMA w/o PASID. Here we address the latter.

DMA mapping API is the de facto standard for in-kernel DMA. However, it
operates on a per device or Requester ID(RID) basis which is not
PASID-aware. To leverage DMA API for devices relies on PASIDs, this
patchset introduces the following APIs

1. A driver facing API that enables DMA API PASID usage:
iommu_attach_dma_pasid(struct device *dev, ioasid_t &pasid);
2. VT-d driver default domain op that allows attaching device-domain-PASID

Once PASID DMA is enabled and attached to the appropriate IOMMU domain,
device drivers can continue to use DMA APIs as-is. There is no difference
in terms of mapping in dma_handle between without PASID and with PASID.
The DMA mapping performed by IOMMU will be identical for both requests, let
it be IOVA or PA in case of pass-through.

In addition, this set converts DSA driver in-kernel DMA with PASID from SVA
lib to DMA API. There have been security and functional issues with the
kernel SVA approach:
(https://lore.kernel.org/linux-iommu/20210511194726.GP1002214@nvidia.com/)
The highlights are as the following:
 - The lack of IOTLB synchronization upon kernel page table updates.
   (vmalloc, module/BPF loading, CONFIG_DEBUG_PAGEALLOC etc.)
 - Other than slight more protection, using kernel virtual address (KVA)
has little advantage over physical address. There are also no use cases yet
where DMA engines need kernel virtual addresses for in-kernel DMA.

Subsequently, cleanup is done around the usage of sva_bind_device() for
in-kernel DMA. Removing special casing code in VT-d driver and tightening
SVA lib API.

This work and idea behind it is a collaboration with many people, many
thanks to Baolu Lu, Jason Gunthorpe, Dave Jiang, and others.


ChangeLog:
v4
	- Rebased on "Baolu's SVA and IOPF refactoring" series v6.
	(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v6)
	- Fixed locking for protecting iommu domain PASID data
	- Use iommu_attach_device_pasid() API instead of calling domain
	  ops directly. This will leverage the common pasid_array that
	  replaces driver specific storage in device_domain_info.
	- Added a helper function to do look up in pasid_array from
	  domain

v3
	- Rebased on "Baolu's SVA and IOPF refactoring" series v5.
	(https://github.com/LuBaolu/intel-iommu/commits/iommu-sva-refactoring-v5)
	This version is significantly simplified by leveraging IOMMU domain
	ops, attach_dev_pasid() op is implemented differently on a DMA domain
	than on a SVA domain.
	We currently have no need to support multiple PASIDs per DMA domain.
	(https://lore.kernel.org/lkml/20220315142216.GV11336@nvidia.com/).
	Removed PASID-device list from V2, a PASID field is introduced to
	struct iommu_domain instead. It is intended for DMA requests with
	PASID by all devices attached to the domain.

v2
	- Do not reserve a special PASID for DMA API usage. Use IOASID
	  allocation instead.
	- Introduced a generic device-pasid-domain attachment IOMMU op.
	  Replaced the DMA API only IOMMU op.
	- Removed supervisor SVA support in VT-d
	- Removed unused sva_bind_device parameters
	- Use IOMMU specific data instead of struct device to store PASID
	  info




*** SUBJECT HERE ***

*** BLURB HERE ***

Jacob Pan (6):
  iommu: Add a per domain PASID for DMA API
  iommu: Add a helper to do PASID lookup from domain
  iommu/vt-d: Implement domain ops for attach_dev_pasid
  iommu: Add PASID support for DMA mapping API users
  dmaengine: idxd: Use DMA API for in-kernel DMA with PASID
  iommu/vt-d: Delete unused SVM flag

 drivers/dma/idxd/idxd.h     |   1 -
 drivers/dma/idxd/init.c     |  34 +++--------
 drivers/dma/idxd/sysfs.c    |   7 ---
 drivers/iommu/dma-iommu.c   | 114 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/iommu.c |  72 ++++++++++++++++++++++-
 drivers/iommu/intel/svm.c   |   2 +-
 drivers/iommu/iommu.c       |  22 +++++++
 include/linux/dma-iommu.h   |   3 +
 include/linux/intel-svm.h   |  13 ----
 include/linux/iommu.h       |   8 ++-
 10 files changed, 226 insertions(+), 50 deletions(-)

-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

DMA requests tagged with PASID can target individual IOMMU domains.
Introduce a domain-wide PASID for DMA API, it will be used on the same
mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
identity domain.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 include/linux/iommu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9405034e3013..36ad007084cc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -106,6 +106,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
 						      void *data);
 	void *fault_data;
+	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */
+	atomic_t dma_pasid_users;
 };
 
 static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
-- 
2.25.1

[PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

DMA requests tagged with PASID can target individual IOMMU domains.
Introduce a domain-wide PASID for DMA API, it will be used on the same
mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
identity domain.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 include/linux/iommu.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 9405034e3013..36ad007084cc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -106,6 +106,8 @@ struct iommu_domain {
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
 						      void *data);
 	void *fault_data;
+	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */
+	atomic_t dma_pasid_users;
 };
 
 static inline bool iommu_is_dma_domain(struct iommu_domain *domain)
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

IOMMU group maintains a PASID array which stores the associated IOMMU
domains. This patch introduces a helper function to do domain to PASID
look up. It will be used by TLB flush and device-PASID attach verification.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
 include/linux/iommu.h |  6 +++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 00d0262a1fe9..22f44833db64 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
 
 	return domain;
 }
+
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}
+	iommu_group_put(group);
+
+	return pasid;
+}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 36ad007084cc..c0440a4be699 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
 			       struct device *dev, ioasid_t pasid);
 struct iommu_domain *
 iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
-
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
 {
 	return NULL;
 }
+static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	return INVALID_IOASID;
+}
 #endif /* CONFIG_IOMMU_API */
 
 #ifdef CONFIG_IOMMU_SVA
-- 
2.25.1

[PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

IOMMU group maintains a PASID array which stores the associated IOMMU
domains. This patch introduces a helper function to do domain to PASID
look up. It will be used by TLB flush and device-PASID attach verification.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
 include/linux/iommu.h |  6 +++++-
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 00d0262a1fe9..22f44833db64 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
 
 	return domain;
 }
+
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	struct iommu_domain *tdomain;
+	struct iommu_group *group;
+	unsigned long index;
+	ioasid_t pasid = INVALID_IOASID;
+
+	group = iommu_group_get(dev);
+	if (!group)
+		return pasid;
+
+	xa_for_each(&group->pasid_array, index, tdomain) {
+		if (domain == tdomain) {
+			pasid = index;
+			break;
+		}
+	}
+	iommu_group_put(group);
+
+	return pasid;
+}
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 36ad007084cc..c0440a4be699 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
 			       struct device *dev, ioasid_t pasid);
 struct iommu_domain *
 iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
-
+ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
 #else /* CONFIG_IOMMU_API */
 
 struct iommu_ops {};
@@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
 {
 	return NULL;
 }
+static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
+{
+	return INVALID_IOASID;
+}
 #endif /* CONFIG_IOMMU_API */
 
 #ifdef CONFIG_IOMMU_SVA
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

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

This patch add attach_dev_pasid functions to the default domain ops which
is used by DMA and identity domain types. It could be extended to support
other domain types whenever necessary.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1c2c92b657c7..75615c105fdf 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
 				    u64 addr, unsigned int mask)
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
 
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);
+	if (pasid != INVALID_IOASID) {
+		qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
+					 pasid, qdep, addr, mask);
+	}
 	qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
 			   qdep, addr, mask);
 }
@@ -1591,6 +1597,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 	unsigned int mask = ilog2(aligned_pages);
 	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
 	u16 did = domain->iommu_did[iommu->seq_id];
+	struct iommu_domain *iommu_domain = &domain->domain;
 
 	BUG_ON(pages == 0);
 
@@ -1599,6 +1606,9 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 
 	if (domain_use_first_level(domain)) {
 		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr, pages, ih);
+		/* flush additional kernel DMA PASIDs attached */
+		if (iommu_domain->dma_pasid)
+			qi_flush_piotlb(iommu, did, iommu_domain->dma_pasid, addr, pages, ih);
 	} else {
 		unsigned long bitmask = aligned_pages - 1;
 
@@ -4255,6 +4265,7 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	struct dmar_domain *domain;
 	struct intel_iommu *iommu;
 	unsigned long flags;
+	ioasid_t pasid;
 
 	assert_spin_locked(&device_domain_lock);
 
@@ -4265,10 +4276,15 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	domain = info->domain;
 
 	if (info->dev && !dev_is_real_dma_subdevice(info->dev)) {
-		if (dev_is_pci(info->dev) && sm_supported(iommu))
+		if (dev_is_pci(info->dev) && sm_supported(iommu)) {
 			intel_pasid_tear_down_entry(iommu, info->dev,
 					PASID_RID2PASID, false);
-
+			pasid = iommu_get_pasid_from_domain(info->dev,
+							    &info->domain->domain);
+			if (pasid != INVALID_IOASID)
+				intel_pasid_tear_down_entry(iommu, info->dev,
+							    pasid, false);
+		}
 		iommu_disable_dev_iotlb(info);
 		domain_context_clear(info);
 		intel_pasid_free_table(info->dev);
@@ -4904,6 +4920,56 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	}
 }
 
+static int intel_iommu_attach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	if (!sm_supported(iommu) || !info)
+		return -ENODEV;
+
+	if (WARN_ON(pasid == PASID_RID2PASID))
+		return -EINVAL;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	spin_lock(&iommu->lock);
+	if (hw_pass_through && domain_type_is_si(dmar_domain))
+		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
+						     dev, pasid);
+	else if (domain_use_first_level(dmar_domain))
+		ret = domain_setup_first_level(iommu, dmar_domain,
+					       dev, pasid);
+	else
+		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
+						     dev, pasid);
+
+	spin_unlock(&iommu->lock);
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+
+	return ret;
+}
+
+static void intel_iommu_detach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+
+	if (pasid != iommu_get_pasid_from_domain(info->dev, &info->domain->domain))
+		return;
+
+	spin_lock_irqsave(&iommu->lock, flags);
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+	spin_unlock_irqrestore(&iommu->lock, flags);
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
 	.domain_alloc		= intel_iommu_domain_alloc,
@@ -4932,6 +4998,8 @@ const struct iommu_ops intel_iommu_ops = {
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
 		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
+		.attach_dev_pasid	= intel_iommu_attach_dev_pasid,
+		.detach_dev_pasid	= intel_iommu_detach_dev_pasid,
 	}
 };
 
-- 
2.25.1

[PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

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

This patch add attach_dev_pasid functions to the default domain ops which
is used by DMA and identity domain types. It could be extended to support
other domain types whenever necessary.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1c2c92b657c7..75615c105fdf 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
 				    u64 addr, unsigned int mask)
 {
 	u16 sid, qdep;
+	ioasid_t pasid;
 
 	if (!info || !info->ats_enabled)
 		return;
 
 	sid = info->bus << 8 | info->devfn;
 	qdep = info->ats_qdep;
+	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);
+	if (pasid != INVALID_IOASID) {
+		qi_flush_dev_iotlb_pasid(info->iommu, sid, info->pfsid,
+					 pasid, qdep, addr, mask);
+	}
 	qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
 			   qdep, addr, mask);
 }
@@ -1591,6 +1597,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 	unsigned int mask = ilog2(aligned_pages);
 	uint64_t addr = (uint64_t)pfn << VTD_PAGE_SHIFT;
 	u16 did = domain->iommu_did[iommu->seq_id];
+	struct iommu_domain *iommu_domain = &domain->domain;
 
 	BUG_ON(pages == 0);
 
@@ -1599,6 +1606,9 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
 
 	if (domain_use_first_level(domain)) {
 		qi_flush_piotlb(iommu, did, PASID_RID2PASID, addr, pages, ih);
+		/* flush additional kernel DMA PASIDs attached */
+		if (iommu_domain->dma_pasid)
+			qi_flush_piotlb(iommu, did, iommu_domain->dma_pasid, addr, pages, ih);
 	} else {
 		unsigned long bitmask = aligned_pages - 1;
 
@@ -4255,6 +4265,7 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	struct dmar_domain *domain;
 	struct intel_iommu *iommu;
 	unsigned long flags;
+	ioasid_t pasid;
 
 	assert_spin_locked(&device_domain_lock);
 
@@ -4265,10 +4276,15 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	domain = info->domain;
 
 	if (info->dev && !dev_is_real_dma_subdevice(info->dev)) {
-		if (dev_is_pci(info->dev) && sm_supported(iommu))
+		if (dev_is_pci(info->dev) && sm_supported(iommu)) {
 			intel_pasid_tear_down_entry(iommu, info->dev,
 					PASID_RID2PASID, false);
-
+			pasid = iommu_get_pasid_from_domain(info->dev,
+							    &info->domain->domain);
+			if (pasid != INVALID_IOASID)
+				intel_pasid_tear_down_entry(iommu, info->dev,
+							    pasid, false);
+		}
 		iommu_disable_dev_iotlb(info);
 		domain_context_clear(info);
 		intel_pasid_free_table(info->dev);
@@ -4904,6 +4920,56 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,
 	}
 }
 
+static int intel_iommu_attach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	if (!sm_supported(iommu) || !info)
+		return -ENODEV;
+
+	if (WARN_ON(pasid == PASID_RID2PASID))
+		return -EINVAL;
+
+	spin_lock_irqsave(&device_domain_lock, flags);
+	spin_lock(&iommu->lock);
+	if (hw_pass_through && domain_type_is_si(dmar_domain))
+		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
+						     dev, pasid);
+	else if (domain_use_first_level(dmar_domain))
+		ret = domain_setup_first_level(iommu, dmar_domain,
+					       dev, pasid);
+	else
+		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
+						     dev, pasid);
+
+	spin_unlock(&iommu->lock);
+	spin_unlock_irqrestore(&device_domain_lock, flags);
+
+	return ret;
+}
+
+static void intel_iommu_detach_dev_pasid(struct iommu_domain *domain,
+					struct device *dev,
+					ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	unsigned long flags;
+
+	if (pasid != iommu_get_pasid_from_domain(info->dev, &info->domain->domain))
+		return;
+
+	spin_lock_irqsave(&iommu->lock, flags);
+	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
+	spin_unlock_irqrestore(&iommu->lock, flags);
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
 	.domain_alloc		= intel_iommu_domain_alloc,
@@ -4932,6 +4998,8 @@ const struct iommu_ops intel_iommu_ops = {
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
 		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
+		.attach_dev_pasid	= intel_iommu_attach_dev_pasid,
+		.detach_dev_pasid	= intel_iommu_detach_dev_pasid,
 	}
 };
 
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[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 <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/dma-iommu.c | 114 ++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |   3 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1ca85d37eeab..6ad7ba619ef0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 enum iommu_dma_cookie_type {
 	IOMMU_DMA_IOVA_COOKIE,
 	IOMMU_DMA_MSI_COOKIE,
@@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/* Protect iommu_domain DMA PASID data */
+static DEFINE_MUTEX(dma_pasid_lock);
+/**
+ * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the device's
+ * DMA domain.
+ * @dev: Device to be enabled
+ * @pasid: The returned kernel PASID to be used for DMA
+ *
+ * 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, the PASID will point to the same IOVA page table.
+ *
+ * @return err code or 0 on success
+ */
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret = 0;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+
+	/* Only support domain types that DMA API can be used */
+	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
+	    dom->type == IOMMU_DOMAIN_BLOCKED) {
+		dev_warn(dev, "Invalid domain type %d", dom->type);
+		return -EPERM;
+	}
+
+	mutex_lock(&dma_pasid_lock);
+	id = dom->dma_pasid;
+	if (!id) {
+		/*
+		 * First device to use PASID in its DMA domain, allocate
+		 * a single PASID per DMA domain is all we need, it is also
+		 * good for performance when it comes down to IOTLB flush.
+		 */
+		max = 1U << dev->iommu->pasid_bits;
+		if (!max) {
+			ret = -EINVAL;
+			goto done_unlock;
+		}
+
+		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+		if (id == INVALID_IOASID) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+
+		dom->dma_pasid = id;
+		atomic_set(&dom->dma_pasid_users, 1);
+	}
+
+	ret = iommu_attach_device_pasid(dom, dev, id);
+	if (!ret) {
+		*pasid = id;
+		atomic_inc(&dom->dma_pasid_users);
+		goto done_unlock;
+	}
+
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(id);
+		dom->dma_pasid = 0;
+	}
+done_unlock:
+	mutex_unlock(&dma_pasid_lock);
+	return ret;
+}
+EXPORT_SYMBOL(iommu_attach_dma_pasid);
+
+/**
+ * iommu_detach_dma_pasid --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.
+ *
+ */
+void iommu_detach_dma_pasid(struct device *dev)
+{
+	struct iommu_domain *dom;
+	ioasid_t pasid;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
+		return;
+
+	/* Only support DMA API managed domain type */
+	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
+		    dom->type == IOMMU_DOMAIN_BLOCKED))
+		return;
+
+	mutex_lock(&dma_pasid_lock);
+	pasid = iommu_get_pasid_from_domain(dev, dom);
+	if (!pasid || pasid == INVALID_IOASID) {
+		dev_err(dev, "No valid DMA PASID attached\n");
+		mutex_unlock(&dma_pasid_lock);
+		return;
+	}
+	iommu_detach_device_pasid(dom, dev, pasid);
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(pasid);
+		dom->dma_pasid = 0;
+	}
+	mutex_unlock(&dma_pasid_lock);
+}
+EXPORT_SYMBOL(iommu_detach_dma_pasid);
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 24607dc3c2ac..538650b9cb75 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -18,6 +18,9 @@ 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);
 
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
+void iommu_detach_dma_pasid(struct device *dev);
+
 /* 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);
-- 
2.25.1

[PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[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 <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/dma-iommu.c | 114 ++++++++++++++++++++++++++++++++++++++
 include/linux/dma-iommu.h |   3 +
 2 files changed, 117 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 1ca85d37eeab..6ad7ba619ef0 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
 	phys_addr_t		phys;
 };
 
+static DECLARE_IOASID_SET(iommu_dma_pasid);
+
 enum iommu_dma_cookie_type {
 	IOMMU_DMA_IOVA_COOKIE,
 	IOMMU_DMA_MSI_COOKIE,
@@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct iommu_domain *domain)
 	domain->iova_cookie = NULL;
 }
 
+/* Protect iommu_domain DMA PASID data */
+static DEFINE_MUTEX(dma_pasid_lock);
+/**
+ * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the device's
+ * DMA domain.
+ * @dev: Device to be enabled
+ * @pasid: The returned kernel PASID to be used for DMA
+ *
+ * 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, the PASID will point to the same IOVA page table.
+ *
+ * @return err code or 0 on success
+ */
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)
+{
+	struct iommu_domain *dom;
+	ioasid_t id, max;
+	int ret = 0;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
+		return -ENODEV;
+
+	/* Only support domain types that DMA API can be used */
+	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
+	    dom->type == IOMMU_DOMAIN_BLOCKED) {
+		dev_warn(dev, "Invalid domain type %d", dom->type);
+		return -EPERM;
+	}
+
+	mutex_lock(&dma_pasid_lock);
+	id = dom->dma_pasid;
+	if (!id) {
+		/*
+		 * First device to use PASID in its DMA domain, allocate
+		 * a single PASID per DMA domain is all we need, it is also
+		 * good for performance when it comes down to IOTLB flush.
+		 */
+		max = 1U << dev->iommu->pasid_bits;
+		if (!max) {
+			ret = -EINVAL;
+			goto done_unlock;
+		}
+
+		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
+		if (id == INVALID_IOASID) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+
+		dom->dma_pasid = id;
+		atomic_set(&dom->dma_pasid_users, 1);
+	}
+
+	ret = iommu_attach_device_pasid(dom, dev, id);
+	if (!ret) {
+		*pasid = id;
+		atomic_inc(&dom->dma_pasid_users);
+		goto done_unlock;
+	}
+
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(id);
+		dom->dma_pasid = 0;
+	}
+done_unlock:
+	mutex_unlock(&dma_pasid_lock);
+	return ret;
+}
+EXPORT_SYMBOL(iommu_attach_dma_pasid);
+
+/**
+ * iommu_detach_dma_pasid --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.
+ *
+ */
+void iommu_detach_dma_pasid(struct device *dev)
+{
+	struct iommu_domain *dom;
+	ioasid_t pasid;
+
+	dom = iommu_get_domain_for_dev(dev);
+	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
+		return;
+
+	/* Only support DMA API managed domain type */
+	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
+		    dom->type == IOMMU_DOMAIN_BLOCKED))
+		return;
+
+	mutex_lock(&dma_pasid_lock);
+	pasid = iommu_get_pasid_from_domain(dev, dom);
+	if (!pasid || pasid == INVALID_IOASID) {
+		dev_err(dev, "No valid DMA PASID attached\n");
+		mutex_unlock(&dma_pasid_lock);
+		return;
+	}
+	iommu_detach_device_pasid(dom, dev, pasid);
+	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
+		ioasid_free(pasid);
+		dom->dma_pasid = 0;
+	}
+	mutex_unlock(&dma_pasid_lock);
+}
+EXPORT_SYMBOL(iommu_detach_dma_pasid);
+
 /**
  * iommu_dma_get_resv_regions - Reserved region driver helper
  * @dev: Device from iommu_get_resv_regions()
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 24607dc3c2ac..538650b9cb75 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -18,6 +18,9 @@ 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);
 
+int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
+void iommu_detach_dma_pasid(struct device *dev);
+
 /* 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);
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 5/6] dmaengine: idxd: Use DMA API for in-kernel DMA with PASID

[Jacob Pan]

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/20210511194726.GP1002214@nvidia.com/
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 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(-)

diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index ccbefd0be617..190b08bd7c08 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -277,7 +277,6 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
 	unsigned int pasid;
 
 	int num_groups;
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index e1b5d1e4a949..e2e1c0eae6d6 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -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
 
 static int idxd_enable_system_pasid(struct idxd_device *idxd)
 {
-	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_attach_dma_pasid(&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_detach_dma_pasid(&idxd->pdev->dev);
 }
 
 static int idxd_probe(struct idxd_device *idxd)
@@ -527,10 +514,7 @@ static int idxd_probe(struct idxd_device *idxd)
 			else
 				set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
 		}
-	} else if (!sva) {
-		dev_warn(dev, "User forced SVA off via module param.\n");
 	}
-
 	idxd_read_caps(idxd);
 	idxd_read_table_offsets(idxd);
 
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index dfd549685c46..a48928973bd4 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -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

[PATCH v4 5/6] dmaengine: idxd: Use DMA API for in-kernel DMA with PASID

[Jacob Pan]

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/20210511194726.GP1002214@nvidia.com/
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 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(-)

diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
index ccbefd0be617..190b08bd7c08 100644
--- a/drivers/dma/idxd/idxd.h
+++ b/drivers/dma/idxd/idxd.h
@@ -277,7 +277,6 @@ struct idxd_device {
 	struct idxd_wq **wqs;
 	struct idxd_engine **engines;
 
-	struct iommu_sva *sva;
 	unsigned int pasid;
 
 	int num_groups;
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index e1b5d1e4a949..e2e1c0eae6d6 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -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
 
 static int idxd_enable_system_pasid(struct idxd_device *idxd)
 {
-	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_attach_dma_pasid(&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_detach_dma_pasid(&idxd->pdev->dev);
 }
 
 static int idxd_probe(struct idxd_device *idxd)
@@ -527,10 +514,7 @@ static int idxd_probe(struct idxd_device *idxd)
 			else
 				set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
 		}
-	} else if (!sva) {
-		dev_warn(dev, "User forced SVA off via module param.\n");
 	}
-
 	idxd_read_caps(idxd);
 	idxd_read_table_offsets(idxd);
 
diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
index dfd549685c46..a48928973bd4 100644
--- a/drivers/dma/idxd/sysfs.c
+++ b/drivers/dma/idxd/sysfs.c
@@ -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

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 6/6] iommu/vt-d: Delete unused SVM flag

[Jacob Pan]

Supervisor PASID for SVA/SVM is no longer supported, delete the unused
flag.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c |  2 +-
 include/linux/intel-svm.h | 13 -------------
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 44331db060e4..5b220d464218 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -750,7 +750,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			 * to unbind the mm while any page faults are outstanding.
 			 */
 			svm = pasid_private_find(req->pasid);
-			if (IS_ERR_OR_NULL(svm) || (svm->flags & SVM_FLAG_SUPERVISOR_MODE))
+			if (IS_ERR_OR_NULL(svm))
 				goto bad_req;
 		}
 
diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
index b3b125b332aa..6835a665c195 100644
--- a/include/linux/intel-svm.h
+++ b/include/linux/intel-svm.h
@@ -13,17 +13,4 @@
 #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
 #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
 
-/*
- * The SVM_FLAG_SUPERVISOR_MODE flag requests a PASID which can be used only
- * for access to kernel addresses. No IOTLB flushes are automatically done
- * for kernel mappings; it is valid only for access to the kernel's static
- * 1:1 mapping of physical memory — not to vmalloc or even module mappings.
- * A future API addition may permit the use of such ranges, by means of an
- * explicit IOTLB flush call (akin to the DMA API's unmap method).
- *
- * It is unlikely that we will ever hook into flush_tlb_kernel_range() to
- * do such IOTLB flushes automatically.
- */
-#define SVM_FLAG_SUPERVISOR_MODE	BIT(0)
-
 #endif /* __INTEL_SVM_H__ */
-- 
2.25.1

[PATCH v4 6/6] iommu/vt-d: Delete unused SVM flag

[Jacob Pan]

Supervisor PASID for SVA/SVM is no longer supported, delete the unused
flag.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/svm.c |  2 +-
 include/linux/intel-svm.h | 13 -------------
 2 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 44331db060e4..5b220d464218 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -750,7 +750,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			 * to unbind the mm while any page faults are outstanding.
 			 */
 			svm = pasid_private_find(req->pasid);
-			if (IS_ERR_OR_NULL(svm) || (svm->flags & SVM_FLAG_SUPERVISOR_MODE))
+			if (IS_ERR_OR_NULL(svm))
 				goto bad_req;
 		}
 
diff --git a/include/linux/intel-svm.h b/include/linux/intel-svm.h
index b3b125b332aa..6835a665c195 100644
--- a/include/linux/intel-svm.h
+++ b/include/linux/intel-svm.h
@@ -13,17 +13,4 @@
 #define PRQ_RING_MASK	((0x1000 << PRQ_ORDER) - 0x20)
 #define PRQ_DEPTH	((0x1000 << PRQ_ORDER) >> 5)
 
-/*
- * The SVM_FLAG_SUPERVISOR_MODE flag requests a PASID which can be used only
- * for access to kernel addresses. No IOTLB flushes are automatically done
- * for kernel mappings; it is valid only for access to the kernel's static
- * 1:1 mapping of physical memory — not to vmalloc or even module mappings.
- * A future API addition may permit the use of such ranges, by means of an
- * explicit IOTLB flush call (akin to the DMA API's unmap method).
- *
- * It is unlikely that we will ever hook into flush_tlb_kernel_range() to
- * do such IOTLB flushes automatically.
- */
-#define SVM_FLAG_SUPERVISOR_MODE	BIT(0)
-
 #endif /* __INTEL_SVM_H__ */
-- 
2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Baolu Lu]

On 2022/5/19 02:21, Jacob Pan wrote:
> IOMMU group maintains a PASID array which stores the associated IOMMU
> domains. This patch introduces a helper function to do domain to PASID
> look up. It will be used by TLB flush and device-PASID attach verification.

Do you really need this?

The IOMMU driver has maintained a device tracking list for each domain.
It has been used for cache invalidation when unmap() is called against
dma domain.

Best regards,
baolu

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
>   include/linux/iommu.h |  6 +++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00d0262a1fe9..22f44833db64 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
>   
>   	return domain;
>   }
> +
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
> +{
> +	struct iommu_domain *tdomain;
> +	struct iommu_group *group;
> +	unsigned long index;
> +	ioasid_t pasid = INVALID_IOASID;
> +
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return pasid;
> +
> +	xa_for_each(&group->pasid_array, index, tdomain) {
> +		if (domain == tdomain) {
> +			pasid = index;
> +			break;
> +		}
> +	}
> +	iommu_group_put(group);
> +
> +	return pasid;
> +}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36ad007084cc..c0440a4be699 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
>   			       struct device *dev, ioasid_t pasid);
>   struct iommu_domain *
>   iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
> -
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
>   #else /* CONFIG_IOMMU_API */
>   
>   struct iommu_ops {};
> @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
>   {
>   	return NULL;
>   }
> +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
> +{
> +	return INVALID_IOASID;
> +}
>   #endif /* CONFIG_IOMMU_API */
>   
>   #ifdef CONFIG_IOMMU_SVA

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Baolu Lu]

On 2022/5/19 02:21, Jacob Pan wrote:
> IOMMU group maintains a PASID array which stores the associated IOMMU
> domains. This patch introduces a helper function to do domain to PASID
> look up. It will be used by TLB flush and device-PASID attach verification.

Do you really need this?

The IOMMU driver has maintained a device tracking list for each domain.
It has been used for cache invalidation when unmap() is called against
dma domain.

Best regards,
baolu

> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
>   include/linux/iommu.h |  6 +++++-
>   2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00d0262a1fe9..22f44833db64 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3199,3 +3199,25 @@ struct iommu_domain *iommu_get_domain_for_iopf(struct device *dev,
>   
>   	return domain;
>   }
> +
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
> +{
> +	struct iommu_domain *tdomain;
> +	struct iommu_group *group;
> +	unsigned long index;
> +	ioasid_t pasid = INVALID_IOASID;
> +
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return pasid;
> +
> +	xa_for_each(&group->pasid_array, index, tdomain) {
> +		if (domain == tdomain) {
> +			pasid = index;
> +			break;
> +		}
> +	}
> +	iommu_group_put(group);
> +
> +	return pasid;
> +}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36ad007084cc..c0440a4be699 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct iommu_domain *domain,
>   			       struct device *dev, ioasid_t pasid);
>   struct iommu_domain *
>   iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
> -
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain);
>   #else /* CONFIG_IOMMU_API */
>   
>   struct iommu_ops {};
> @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid)
>   {
>   	return NULL;
>   }
> +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)
> +{
> +	return INVALID_IOASID;
> +}
>   #endif /* CONFIG_IOMMU_API */
>   
>   #ifdef CONFIG_IOMMU_SVA

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Christoph Hellwig]

On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote:
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)

Overly long line here.

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Christoph Hellwig]

On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote:
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct iommu_domain *domain)

Overly long line here.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/5/19 02:21, Jacob Pan wrote:
> DMA requests tagged with PASID can target individual IOMMU domains.
> Introduce a domain-wide PASID for DMA API, it will be used on the same
> mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> identity domain.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   include/linux/iommu.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9405034e3013..36ad007084cc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -106,6 +106,8 @@ struct iommu_domain {
>   	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
>   						      void *data);
>   	void *fault_data;
> +	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */

This looks more suitable for iommu_dma_cookie?

> +	atomic_t dma_pasid_users;
>   };
>   
>   static inline bool iommu_is_dma_domain(struct iommu_domain *domain)

Best regards,
baolu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/5/19 02:21, Jacob Pan wrote:
> DMA requests tagged with PASID can target individual IOMMU domains.
> Introduce a domain-wide PASID for DMA API, it will be used on the same
> mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> identity domain.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   include/linux/iommu.h | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9405034e3013..36ad007084cc 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -106,6 +106,8 @@ struct iommu_domain {
>   	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
>   						      void *data);
>   	void *fault_data;
> +	ioasid_t dma_pasid;		/* Used for DMA requests with PASID */

This looks more suitable for iommu_dma_cookie?

> +	atomic_t dma_pasid_users;
>   };
>   
>   static inline bool iommu_is_dma_domain(struct iommu_domain *domain)

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Baolu,

On Thu, 19 May 2022 14:41:06 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> > IOMMU group maintains a PASID array which stores the associated IOMMU
> > domains. This patch introduces a helper function to do domain to PASID
> > look up. It will be used by TLB flush and device-PASID attach
> > verification.  
> 
> Do you really need this?
> 
> The IOMMU driver has maintained a device tracking list for each domain.
> It has been used for cache invalidation when unmap() is called against
> dma domain.
Yes, I am aware of the device list. In v3, I stored DMA API PASID in device
list of device_domain_info. Since we already have a pasid_array, Jason
suggested to share the storage with the code. This helper is needed to
reverse look up the DMA PASID based on the domain attached.
Discussions here:
https://lore.kernel.org/lkml/20220511170025.GF49344@nvidia.com/t/#mf7cb7d54d89e6e732a020dc22435260da0a49580

Thanks,

Jacob

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Baolu,

On Thu, 19 May 2022 14:41:06 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> > IOMMU group maintains a PASID array which stores the associated IOMMU
> > domains. This patch introduces a helper function to do domain to PASID
> > look up. It will be used by TLB flush and device-PASID attach
> > verification.  
> 
> Do you really need this?
> 
> The IOMMU driver has maintained a device tracking list for each domain.
> It has been used for cache invalidation when unmap() is called against
> dma domain.
Yes, I am aware of the device list. In v3, I stored DMA API PASID in device
list of device_domain_info. Since we already have a pasid_array, Jason
suggested to share the storage with the code. This helper is needed to
reverse look up the DMA PASID based on the domain attached.
Discussions here:
https://lore.kernel.org/lkml/20220511170025.GF49344@nvidia.com/t/#mf7cb7d54d89e6e732a020dc22435260da0a49580

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Christoph,

On Wed, 18 May 2022 23:48:44 -0700, Christoph Hellwig <hch@infradead.org>
wrote:

> On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote:
> > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > iommu_domain *domain)  
> 
> Overly long line here.
will fix,

Thanks,

Jacob

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Christoph,

On Wed, 18 May 2022 23:48:44 -0700, Christoph Hellwig <hch@infradead.org>
wrote:

> On Wed, May 18, 2022 at 11:21:16AM -0700, Jacob Pan wrote:
> > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > iommu_domain *domain)  
> 
> Overly long line here.
will fix,

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, May 19, 2022 2:21 AM
> 
> IOMMU group maintains a PASID array which stores the associated IOMMU
> domains. This patch introduces a helper function to do domain to PASID
> look up. It will be used by TLB flush and device-PASID attach verification.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
>  include/linux/iommu.h |  6 +++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00d0262a1fe9..22f44833db64 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3199,3 +3199,25 @@ struct iommu_domain
> *iommu_get_domain_for_iopf(struct device *dev,
> 
>  	return domain;
>  }
> +
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain)
> +{
> +	struct iommu_domain *tdomain;
> +	struct iommu_group *group;
> +	unsigned long index;
> +	ioasid_t pasid = INVALID_IOASID;
> +
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return pasid;
> +
> +	xa_for_each(&group->pasid_array, index, tdomain) {
> +		if (domain == tdomain) {
> +			pasid = index;
> +			break;
> +		}
> +	}

Don't we need to acquire the group lock here?

Btw the intention of this function is a bit confusing. Patch01 already
stores the pasid under domain hence it's redundant to get it 
indirectly from xarray index. You could simply introduce a flag bit
(e.g. dma_pasid_enabled) in device_domain_info and then directly
use domain->dma_pasid once the flag is true.

> +	iommu_group_put(group);
> +
> +	return pasid;
> +}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36ad007084cc..c0440a4be699 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct
> iommu_domain *domain,
>  			       struct device *dev, ioasid_t pasid);
>  struct iommu_domain *
>  iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
> -
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain);
>  #else /* CONFIG_IOMMU_API */
> 
>  struct iommu_ops {};
> @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev,
> ioasid_t pasid)
>  {
>  	return NULL;
>  }
> +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain)
> +{
> +	return INVALID_IOASID;
> +}
>  #endif /* CONFIG_IOMMU_API */
> 
>  #ifdef CONFIG_IOMMU_SVA
> --
> 2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, May 19, 2022 2:21 AM
> 
> IOMMU group maintains a PASID array which stores the associated IOMMU
> domains. This patch introduces a helper function to do domain to PASID
> look up. It will be used by TLB flush and device-PASID attach verification.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 22 ++++++++++++++++++++++
>  include/linux/iommu.h |  6 +++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00d0262a1fe9..22f44833db64 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3199,3 +3199,25 @@ struct iommu_domain
> *iommu_get_domain_for_iopf(struct device *dev,
> 
>  	return domain;
>  }
> +
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain)
> +{
> +	struct iommu_domain *tdomain;
> +	struct iommu_group *group;
> +	unsigned long index;
> +	ioasid_t pasid = INVALID_IOASID;
> +
> +	group = iommu_group_get(dev);
> +	if (!group)
> +		return pasid;
> +
> +	xa_for_each(&group->pasid_array, index, tdomain) {
> +		if (domain == tdomain) {
> +			pasid = index;
> +			break;
> +		}
> +	}

Don't we need to acquire the group lock here?

Btw the intention of this function is a bit confusing. Patch01 already
stores the pasid under domain hence it's redundant to get it 
indirectly from xarray index. You could simply introduce a flag bit
(e.g. dma_pasid_enabled) in device_domain_info and then directly
use domain->dma_pasid once the flag is true.

> +	iommu_group_put(group);
> +
> +	return pasid;
> +}
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 36ad007084cc..c0440a4be699 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -694,7 +694,7 @@ void iommu_detach_device_pasid(struct
> iommu_domain *domain,
>  			       struct device *dev, ioasid_t pasid);
>  struct iommu_domain *
>  iommu_get_domain_for_iopf(struct device *dev, ioasid_t pasid);
> -
> +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain);
>  #else /* CONFIG_IOMMU_API */
> 
>  struct iommu_ops {};
> @@ -1070,6 +1070,10 @@ iommu_get_domain_for_iopf(struct device *dev,
> ioasid_t pasid)
>  {
>  	return NULL;
>  }
> +static ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> iommu_domain *domain)
> +{
> +	return INVALID_IOASID;
> +}
>  #endif /* CONFIG_IOMMU_API */
> 
>  #ifdef CONFIG_IOMMU_SVA
> --
> 2.25.1

RE: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, May 19, 2022 2:21 AM
> 
> 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 <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/dma-iommu.c | 114
> ++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-iommu.h |   3 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1ca85d37eeab..6ad7ba619ef0 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
>  	phys_addr_t		phys;
>  };
> 
> +static DECLARE_IOASID_SET(iommu_dma_pasid);
> +
>  enum iommu_dma_cookie_type {
>  	IOMMU_DMA_IOVA_COOKIE,
>  	IOMMU_DMA_MSI_COOKIE,
> @@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
> iommu_domain *domain)
>  	domain->iova_cookie = NULL;
>  }
> 
> +/* Protect iommu_domain DMA PASID data */
> +static DEFINE_MUTEX(dma_pasid_lock);
> +/**
> + * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the
> device's
> + * DMA domain.
> + * @dev: Device to be enabled
> + * @pasid: The returned kernel PASID to be used for DMA
> + *
> + * 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, the PASID will point to the same IOVA page table.
> + *
> + * @return err code or 0 on success
> + */
> +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)

iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from
a API p.o.v.

> +{
> +	struct iommu_domain *dom;
> +	ioasid_t id, max;
> +	int ret = 0;
> +
> +	dom = iommu_get_domain_for_dev(dev);
> +	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
> +		return -ENODEV;
> +
> +	/* Only support domain types that DMA API can be used */
> +	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
> +	    dom->type == IOMMU_DOMAIN_BLOCKED) {
> +		dev_warn(dev, "Invalid domain type %d", dom->type);
> +		return -EPERM;
> +	}

WARN_ON.

and probably we can just check whether domain is default domain here.

> +
> +	mutex_lock(&dma_pasid_lock);
> +	id = dom->dma_pasid;
> +	if (!id) {
> +		/*
> +		 * First device to use PASID in its DMA domain, allocate
> +		 * a single PASID per DMA domain is all we need, it is also
> +		 * good for performance when it comes down to IOTLB flush.
> +		 */
> +		max = 1U << dev->iommu->pasid_bits;
> +		if (!max) {
> +			ret = -EINVAL;
> +			goto done_unlock;
> +		}
> +
> +		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
> +		if (id == INVALID_IOASID) {
> +			ret = -ENOMEM;
> +			goto done_unlock;
> +		}
> +
> +		dom->dma_pasid = id;
> +		atomic_set(&dom->dma_pasid_users, 1);

this is always accessed with lock held hence no need to be atomic.

> +	}
> +
> +	ret = iommu_attach_device_pasid(dom, dev, id);
> +	if (!ret) {
> +		*pasid = id;
> +		atomic_inc(&dom->dma_pasid_users);
> +		goto done_unlock;
> +	}
> +
> +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> +		ioasid_free(id);
> +		dom->dma_pasid = 0;
> +	}
> +done_unlock:
> +	mutex_unlock(&dma_pasid_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(iommu_attach_dma_pasid);
> +
> +/**
> + * iommu_detach_dma_pasid --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.
> + *
> + */
> +void iommu_detach_dma_pasid(struct device *dev)
> +{
> +	struct iommu_domain *dom;
> +	ioasid_t pasid;
> +
> +	dom = iommu_get_domain_for_dev(dev);
> +	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
> +		return;
> +
> +	/* Only support DMA API managed domain type */
> +	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
> +		    dom->type == IOMMU_DOMAIN_BLOCKED))
> +		return;
> +
> +	mutex_lock(&dma_pasid_lock);
> +	pasid = iommu_get_pasid_from_domain(dev, dom);
> +	if (!pasid || pasid == INVALID_IOASID) {
> +		dev_err(dev, "No valid DMA PASID attached\n");
> +		mutex_unlock(&dma_pasid_lock);
> +		return;
> +	}

here just use dom->dma_pasid and let iommu driver to figure out
underlying whether this device has been attached to the domain
with the said pasid.

> +	iommu_detach_device_pasid(dom, dev, pasid);
> +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> +		ioasid_free(pasid);
> +		dom->dma_pasid = 0;
> +	}
> +	mutex_unlock(&dma_pasid_lock);
> +}
> +EXPORT_SYMBOL(iommu_detach_dma_pasid);
> +
>  /**
>   * iommu_dma_get_resv_regions - Reserved region driver helper
>   * @dev: Device from iommu_get_resv_regions()
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 24607dc3c2ac..538650b9cb75 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -18,6 +18,9 @@ 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);
> 
> +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
> +void iommu_detach_dma_pasid(struct device *dev);
> +
>  /* 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);
> --
> 2.25.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, May 19, 2022 2:21 AM
> 
> 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 <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/dma-iommu.c | 114
> ++++++++++++++++++++++++++++++++++++++
>  include/linux/dma-iommu.h |   3 +
>  2 files changed, 117 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 1ca85d37eeab..6ad7ba619ef0 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
>  	phys_addr_t		phys;
>  };
> 
> +static DECLARE_IOASID_SET(iommu_dma_pasid);
> +
>  enum iommu_dma_cookie_type {
>  	IOMMU_DMA_IOVA_COOKIE,
>  	IOMMU_DMA_MSI_COOKIE,
> @@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
> iommu_domain *domain)
>  	domain->iova_cookie = NULL;
>  }
> 
> +/* Protect iommu_domain DMA PASID data */
> +static DEFINE_MUTEX(dma_pasid_lock);
> +/**
> + * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the
> device's
> + * DMA domain.
> + * @dev: Device to be enabled
> + * @pasid: The returned kernel PASID to be used for DMA
> + *
> + * 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, the PASID will point to the same IOVA page table.
> + *
> + * @return err code or 0 on success
> + */
> +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)

iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from
a API p.o.v.

> +{
> +	struct iommu_domain *dom;
> +	ioasid_t id, max;
> +	int ret = 0;
> +
> +	dom = iommu_get_domain_for_dev(dev);
> +	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
> +		return -ENODEV;
> +
> +	/* Only support domain types that DMA API can be used */
> +	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
> +	    dom->type == IOMMU_DOMAIN_BLOCKED) {
> +		dev_warn(dev, "Invalid domain type %d", dom->type);
> +		return -EPERM;
> +	}

WARN_ON.

and probably we can just check whether domain is default domain here.

> +
> +	mutex_lock(&dma_pasid_lock);
> +	id = dom->dma_pasid;
> +	if (!id) {
> +		/*
> +		 * First device to use PASID in its DMA domain, allocate
> +		 * a single PASID per DMA domain is all we need, it is also
> +		 * good for performance when it comes down to IOTLB flush.
> +		 */
> +		max = 1U << dev->iommu->pasid_bits;
> +		if (!max) {
> +			ret = -EINVAL;
> +			goto done_unlock;
> +		}
> +
> +		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
> +		if (id == INVALID_IOASID) {
> +			ret = -ENOMEM;
> +			goto done_unlock;
> +		}
> +
> +		dom->dma_pasid = id;
> +		atomic_set(&dom->dma_pasid_users, 1);

this is always accessed with lock held hence no need to be atomic.

> +	}
> +
> +	ret = iommu_attach_device_pasid(dom, dev, id);
> +	if (!ret) {
> +		*pasid = id;
> +		atomic_inc(&dom->dma_pasid_users);
> +		goto done_unlock;
> +	}
> +
> +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> +		ioasid_free(id);
> +		dom->dma_pasid = 0;
> +	}
> +done_unlock:
> +	mutex_unlock(&dma_pasid_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(iommu_attach_dma_pasid);
> +
> +/**
> + * iommu_detach_dma_pasid --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.
> + *
> + */
> +void iommu_detach_dma_pasid(struct device *dev)
> +{
> +	struct iommu_domain *dom;
> +	ioasid_t pasid;
> +
> +	dom = iommu_get_domain_for_dev(dev);
> +	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
> +		return;
> +
> +	/* Only support DMA API managed domain type */
> +	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
> +		    dom->type == IOMMU_DOMAIN_BLOCKED))
> +		return;
> +
> +	mutex_lock(&dma_pasid_lock);
> +	pasid = iommu_get_pasid_from_domain(dev, dom);
> +	if (!pasid || pasid == INVALID_IOASID) {
> +		dev_err(dev, "No valid DMA PASID attached\n");
> +		mutex_unlock(&dma_pasid_lock);
> +		return;
> +	}

here just use dom->dma_pasid and let iommu driver to figure out
underlying whether this device has been attached to the domain
with the said pasid.

> +	iommu_detach_device_pasid(dom, dev, pasid);
> +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> +		ioasid_free(pasid);
> +		dom->dma_pasid = 0;
> +	}
> +	mutex_unlock(&dma_pasid_lock);
> +}
> +EXPORT_SYMBOL(iommu_detach_dma_pasid);
> +
>  /**
>   * iommu_dma_get_resv_regions - Reserved region driver helper
>   * @dev: Device from iommu_get_resv_regions()
> diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
> index 24607dc3c2ac..538650b9cb75 100644
> --- a/include/linux/dma-iommu.h
> +++ b/include/linux/dma-iommu.h
> @@ -18,6 +18,9 @@ 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);
> 
> +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid);
> +void iommu_detach_dma_pasid(struct device *dev);
> +
>  /* 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);
> --
> 2.25.1

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Monday, May 23, 2022 3:55 PM
> 
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > iommu_domain *domain)
> > +{
> > +	struct iommu_domain *tdomain;
> > +	struct iommu_group *group;
> > +	unsigned long index;
> > +	ioasid_t pasid = INVALID_IOASID;
> > +
> > +	group = iommu_group_get(dev);
> > +	if (!group)
> > +		return pasid;
> > +
> > +	xa_for_each(&group->pasid_array, index, tdomain) {
> > +		if (domain == tdomain) {
> > +			pasid = index;
> > +			break;
> > +		}
> > +	}
> 
> Don't we need to acquire the group lock here?
> 
> Btw the intention of this function is a bit confusing. Patch01 already
> stores the pasid under domain hence it's redundant to get it
> indirectly from xarray index. You could simply introduce a flag bit
> (e.g. dma_pasid_enabled) in device_domain_info and then directly
> use domain->dma_pasid once the flag is true.
> 

Just saw your discussion with Jason about v3. While it makes sense
to not specialize DMA domain in iommu driver, the use of this function
should only be that when the call chain doesn't pass down a pasid
value e.g. when doing cache invalidation for domain map/unmap. If
the upper interface already carries a pasid e.g. in detach_dev_pasid()
iommu driver can simply verify that the corresponding pasid xarray 
entry points to the specified domain instead of using this function to
loop xarray and then verify the returned pasid (as done in patch03/04).

Thanks
Kevin

RE: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Tian, Kevin]

> From: Tian, Kevin
> Sent: Monday, May 23, 2022 3:55 PM
> 
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > iommu_domain *domain)
> > +{
> > +	struct iommu_domain *tdomain;
> > +	struct iommu_group *group;
> > +	unsigned long index;
> > +	ioasid_t pasid = INVALID_IOASID;
> > +
> > +	group = iommu_group_get(dev);
> > +	if (!group)
> > +		return pasid;
> > +
> > +	xa_for_each(&group->pasid_array, index, tdomain) {
> > +		if (domain == tdomain) {
> > +			pasid = index;
> > +			break;
> > +		}
> > +	}
> 
> Don't we need to acquire the group lock here?
> 
> Btw the intention of this function is a bit confusing. Patch01 already
> stores the pasid under domain hence it's redundant to get it
> indirectly from xarray index. You could simply introduce a flag bit
> (e.g. dma_pasid_enabled) in device_domain_info and then directly
> use domain->dma_pasid once the flag is true.
> 

Just saw your discussion with Jason about v3. While it makes sense
to not specialize DMA domain in iommu driver, the use of this function
should only be that when the call chain doesn't pass down a pasid
value e.g. when doing cache invalidation for domain map/unmap. If
the upper interface already carries a pasid e.g. in detach_dev_pasid()
iommu driver can simply verify that the corresponding pasid xarray 
entry points to the specified domain instead of using this function to
loop xarray and then verify the returned pasid (as done in patch03/04).

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[Jacob Pan]

Hi Kevin,

On Mon, 23 May 2022 08:25:33 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Thursday, May 19, 2022 2:21 AM
> > 
> > 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 <jacob.jun.pan@linux.intel.com>
> > ---
> >  drivers/iommu/dma-iommu.c | 114
> > ++++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-iommu.h |   3 +
> >  2 files changed, 117 insertions(+)
> > 
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 1ca85d37eeab..6ad7ba619ef0 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
> >  	phys_addr_t		phys;
> >  };
> > 
> > +static DECLARE_IOASID_SET(iommu_dma_pasid);
> > +
> >  enum iommu_dma_cookie_type {
> >  	IOMMU_DMA_IOVA_COOKIE,
> >  	IOMMU_DMA_MSI_COOKIE,
> > @@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
> > iommu_domain *domain)
> >  	domain->iova_cookie = NULL;
> >  }
> > 
> > +/* Protect iommu_domain DMA PASID data */
> > +static DEFINE_MUTEX(dma_pasid_lock);
> > +/**
> > + * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the
> > device's
> > + * DMA domain.
> > + * @dev: Device to be enabled
> > + * @pasid: The returned kernel PASID to be used for DMA
> > + *
> > + * 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, the PASID will point to the same IOVA page table.
> > + *
> > + * @return err code or 0 on success
> > + */
> > +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)  
> 
> iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from
> a API p.o.v.
> 
I agree dma_pasid is too broad, technically it is dma_api_pasid but seems
too long.
My concern with dma_domain_pasid is that the pasid can also be used for
identity domain.

> > +{
> > +	struct iommu_domain *dom;
> > +	ioasid_t id, max;
> > +	int ret = 0;
> > +
> > +	dom = iommu_get_domain_for_dev(dev);
> > +	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
> > +		return -ENODEV;
> > +
> > +	/* Only support domain types that DMA API can be used */
> > +	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
> > +	    dom->type == IOMMU_DOMAIN_BLOCKED) {
> > +		dev_warn(dev, "Invalid domain type %d", dom->type);
> > +		return -EPERM;
> > +	}  
> 
> WARN_ON.
> 
> and probably we can just check whether domain is default domain here.
> 
good point, I will just use
struct iommu_domain *def_domain = iommu_get_dma_domain(dev);

> > +
> > +	mutex_lock(&dma_pasid_lock);
> > +	id = dom->dma_pasid;
> > +	if (!id) {
> > +		/*
> > +		 * First device to use PASID in its DMA domain,
> > allocate
> > +		 * a single PASID per DMA domain is all we need, it is
> > also
> > +		 * good for performance when it comes down to IOTLB
> > flush.
> > +		 */
> > +		max = 1U << dev->iommu->pasid_bits;
> > +		if (!max) {
> > +			ret = -EINVAL;
> > +			goto done_unlock;
> > +		}
> > +
> > +		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
> > +		if (id == INVALID_IOASID) {
> > +			ret = -ENOMEM;
> > +			goto done_unlock;
> > +		}
> > +
> > +		dom->dma_pasid = id;
> > +		atomic_set(&dom->dma_pasid_users, 1);  
> 
> this is always accessed with lock held hence no need to be atomic.
> 
good catch, will fix

> > +	}
> > +
> > +	ret = iommu_attach_device_pasid(dom, dev, id);
> > +	if (!ret) {
> > +		*pasid = id;
> > +		atomic_inc(&dom->dma_pasid_users);
> > +		goto done_unlock;
> > +	}
> > +
> > +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> > +		ioasid_free(id);
> > +		dom->dma_pasid = 0;
> > +	}
> > +done_unlock:
> > +	mutex_unlock(&dma_pasid_lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(iommu_attach_dma_pasid);
> > +
> > +/**
> > + * iommu_detach_dma_pasid --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.
> > + *
> > + */
> > +void iommu_detach_dma_pasid(struct device *dev)
> > +{
> > +	struct iommu_domain *dom;
> > +	ioasid_t pasid;
> > +
> > +	dom = iommu_get_domain_for_dev(dev);
> > +	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
> > +		return;
> > +
> > +	/* Only support DMA API managed domain type */
> > +	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
> > +		    dom->type == IOMMU_DOMAIN_BLOCKED))
> > +		return;
> > +
> > +	mutex_lock(&dma_pasid_lock);
> > +	pasid = iommu_get_pasid_from_domain(dev, dom);
> > +	if (!pasid || pasid == INVALID_IOASID) {
> > +		dev_err(dev, "No valid DMA PASID attached\n");
> > +		mutex_unlock(&dma_pasid_lock);
> > +		return;
> > +	}  
> 
> here just use dom->dma_pasid and let iommu driver to figure out
> underlying whether this device has been attached to the domain
> with the said pasid.
> 
Yeah, I am checking the pasid matching in the iommu driver. My thinking is
that here is a quick sanity check in the common code to rule out invalid value.


Thanks a lot!


Jacob

Re: [PATCH v4 4/6] iommu: Add PASID support for DMA mapping API users

[Jacob Pan]

Hi Kevin,

On Mon, 23 May 2022 08:25:33 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Thursday, May 19, 2022 2:21 AM
> > 
> > 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 <jacob.jun.pan@linux.intel.com>
> > ---
> >  drivers/iommu/dma-iommu.c | 114
> > ++++++++++++++++++++++++++++++++++++++
> >  include/linux/dma-iommu.h |   3 +
> >  2 files changed, 117 insertions(+)
> > 
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 1ca85d37eeab..6ad7ba619ef0 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -34,6 +34,8 @@ struct iommu_dma_msi_page {
> >  	phys_addr_t		phys;
> >  };
> > 
> > +static DECLARE_IOASID_SET(iommu_dma_pasid);
> > +
> >  enum iommu_dma_cookie_type {
> >  	IOMMU_DMA_IOVA_COOKIE,
> >  	IOMMU_DMA_MSI_COOKIE,
> > @@ -370,6 +372,118 @@ void iommu_put_dma_cookie(struct
> > iommu_domain *domain)
> >  	domain->iova_cookie = NULL;
> >  }
> > 
> > +/* Protect iommu_domain DMA PASID data */
> > +static DEFINE_MUTEX(dma_pasid_lock);
> > +/**
> > + * iommu_attach_dma_pasid --Attach a PASID for in-kernel DMA. Use the
> > device's
> > + * DMA domain.
> > + * @dev: Device to be enabled
> > + * @pasid: The returned kernel PASID to be used for DMA
> > + *
> > + * 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, the PASID will point to the same IOVA page table.
> > + *
> > + * @return err code or 0 on success
> > + */
> > +int iommu_attach_dma_pasid(struct device *dev, ioasid_t *pasid)  
> 
> iommu_attach_dma_domain_pasid? 'dma_pasid' is too broad from
> a API p.o.v.
> 
I agree dma_pasid is too broad, technically it is dma_api_pasid but seems
too long.
My concern with dma_domain_pasid is that the pasid can also be used for
identity domain.

> > +{
> > +	struct iommu_domain *dom;
> > +	ioasid_t id, max;
> > +	int ret = 0;
> > +
> > +	dom = iommu_get_domain_for_dev(dev);
> > +	if (!dom || !dom->ops || !dom->ops->attach_dev_pasid)
> > +		return -ENODEV;
> > +
> > +	/* Only support domain types that DMA API can be used */
> > +	if (dom->type == IOMMU_DOMAIN_UNMANAGED ||
> > +	    dom->type == IOMMU_DOMAIN_BLOCKED) {
> > +		dev_warn(dev, "Invalid domain type %d", dom->type);
> > +		return -EPERM;
> > +	}  
> 
> WARN_ON.
> 
> and probably we can just check whether domain is default domain here.
> 
good point, I will just use
struct iommu_domain *def_domain = iommu_get_dma_domain(dev);

> > +
> > +	mutex_lock(&dma_pasid_lock);
> > +	id = dom->dma_pasid;
> > +	if (!id) {
> > +		/*
> > +		 * First device to use PASID in its DMA domain,
> > allocate
> > +		 * a single PASID per DMA domain is all we need, it is
> > also
> > +		 * good for performance when it comes down to IOTLB
> > flush.
> > +		 */
> > +		max = 1U << dev->iommu->pasid_bits;
> > +		if (!max) {
> > +			ret = -EINVAL;
> > +			goto done_unlock;
> > +		}
> > +
> > +		id = ioasid_alloc(&iommu_dma_pasid, 1, max, dev);
> > +		if (id == INVALID_IOASID) {
> > +			ret = -ENOMEM;
> > +			goto done_unlock;
> > +		}
> > +
> > +		dom->dma_pasid = id;
> > +		atomic_set(&dom->dma_pasid_users, 1);  
> 
> this is always accessed with lock held hence no need to be atomic.
> 
good catch, will fix

> > +	}
> > +
> > +	ret = iommu_attach_device_pasid(dom, dev, id);
> > +	if (!ret) {
> > +		*pasid = id;
> > +		atomic_inc(&dom->dma_pasid_users);
> > +		goto done_unlock;
> > +	}
> > +
> > +	if (atomic_dec_and_test(&dom->dma_pasid_users)) {
> > +		ioasid_free(id);
> > +		dom->dma_pasid = 0;
> > +	}
> > +done_unlock:
> > +	mutex_unlock(&dma_pasid_lock);
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(iommu_attach_dma_pasid);
> > +
> > +/**
> > + * iommu_detach_dma_pasid --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.
> > + *
> > + */
> > +void iommu_detach_dma_pasid(struct device *dev)
> > +{
> > +	struct iommu_domain *dom;
> > +	ioasid_t pasid;
> > +
> > +	dom = iommu_get_domain_for_dev(dev);
> > +	if (WARN_ON(!dom || !dom->ops || !dom->ops->detach_dev_pasid))
> > +		return;
> > +
> > +	/* Only support DMA API managed domain type */
> > +	if (WARN_ON(dom->type == IOMMU_DOMAIN_UNMANAGED ||
> > +		    dom->type == IOMMU_DOMAIN_BLOCKED))
> > +		return;
> > +
> > +	mutex_lock(&dma_pasid_lock);
> > +	pasid = iommu_get_pasid_from_domain(dev, dom);
> > +	if (!pasid || pasid == INVALID_IOASID) {
> > +		dev_err(dev, "No valid DMA PASID attached\n");
> > +		mutex_unlock(&dma_pasid_lock);
> > +		return;
> > +	}  
> 
> here just use dom->dma_pasid and let iommu driver to figure out
> underlying whether this device has been attached to the domain
> with the said pasid.
> 
Yeah, I am checking the pasid matching in the iommu driver. My thinking is
that here is a quick sanity check in the common code to rule out invalid value.


Thanks a lot!


Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Kevin,

On Mon, 23 May 2022 09:14:04 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Tian, Kevin
> > Sent: Monday, May 23, 2022 3:55 PM
> >   
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > > iommu_domain *domain)
> > > +{
> > > +	struct iommu_domain *tdomain;
> > > +	struct iommu_group *group;
> > > +	unsigned long index;
> > > +	ioasid_t pasid = INVALID_IOASID;
> > > +
> > > +	group = iommu_group_get(dev);
> > > +	if (!group)
> > > +		return pasid;
> > > +
> > > +	xa_for_each(&group->pasid_array, index, tdomain) {
> > > +		if (domain == tdomain) {
> > > +			pasid = index;
> > > +			break;
> > > +		}
> > > +	}  
> > 
> > Don't we need to acquire the group lock here?
> > 
pasid_array is under RCU read lock so it is protected though may have stale
data. It also used in atomic context for TLB flush, cannot take the
group mutex. If the caller does detach_dev_pasid while doing TLB flush, it
could result in extra flush but harmless.

> > Btw the intention of this function is a bit confusing. Patch01 already
> > stores the pasid under domain hence it's redundant to get it
> > indirectly from xarray index. You could simply introduce a flag bit
> > (e.g. dma_pasid_enabled) in device_domain_info and then directly
> > use domain->dma_pasid once the flag is true.
> >   
> 
> Just saw your discussion with Jason about v3. While it makes sense
> to not specialize DMA domain in iommu driver, the use of this function
> should only be that when the call chain doesn't pass down a pasid
> value e.g. when doing cache invalidation for domain map/unmap. If
> the upper interface already carries a pasid e.g. in detach_dev_pasid()
> iommu driver can simply verify that the corresponding pasid xarray 
> entry points to the specified domain instead of using this function to
> loop xarray and then verify the returned pasid (as done in patch03/04).
Excellent point, I could just use xa_load(pasid) to compare the domain
instead of loop through xa.
I will add another helper.

bool iommu_is_pasid_domain_attached(struct device *dev, struct iommu_domain *domain, ioasid_t pasid)
{
	struct iommu_group *group;
	bool ret = false;

	group = iommu_group_get(dev);
	if (WARN_ON(!group))
		return false;

	if (domain == xa_load(&group->pasid_array, pasid))
		ret = true;

	iommu_group_put(group);

	return ret;
}

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/6] iommu: Add a helper to do PASID lookup from domain

[Jacob Pan]

Hi Kevin,

On Mon, 23 May 2022 09:14:04 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Tian, Kevin
> > Sent: Monday, May 23, 2022 3:55 PM
> >   
> > > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > +ioasid_t iommu_get_pasid_from_domain(struct device *dev, struct
> > > iommu_domain *domain)
> > > +{
> > > +	struct iommu_domain *tdomain;
> > > +	struct iommu_group *group;
> > > +	unsigned long index;
> > > +	ioasid_t pasid = INVALID_IOASID;
> > > +
> > > +	group = iommu_group_get(dev);
> > > +	if (!group)
> > > +		return pasid;
> > > +
> > > +	xa_for_each(&group->pasid_array, index, tdomain) {
> > > +		if (domain == tdomain) {
> > > +			pasid = index;
> > > +			break;
> > > +		}
> > > +	}  
> > 
> > Don't we need to acquire the group lock here?
> > 
pasid_array is under RCU read lock so it is protected though may have stale
data. It also used in atomic context for TLB flush, cannot take the
group mutex. If the caller does detach_dev_pasid while doing TLB flush, it
could result in extra flush but harmless.

> > Btw the intention of this function is a bit confusing. Patch01 already
> > stores the pasid under domain hence it's redundant to get it
> > indirectly from xarray index. You could simply introduce a flag bit
> > (e.g. dma_pasid_enabled) in device_domain_info and then directly
> > use domain->dma_pasid once the flag is true.
> >   
> 
> Just saw your discussion with Jason about v3. While it makes sense
> to not specialize DMA domain in iommu driver, the use of this function
> should only be that when the call chain doesn't pass down a pasid
> value e.g. when doing cache invalidation for domain map/unmap. If
> the upper interface already carries a pasid e.g. in detach_dev_pasid()
> iommu driver can simply verify that the corresponding pasid xarray 
> entry points to the specified domain instead of using this function to
> loop xarray and then verify the returned pasid (as done in patch03/04).
Excellent point, I could just use xa_load(pasid) to compare the domain
instead of loop through xa.
I will add another helper.

bool iommu_is_pasid_domain_attached(struct device *dev, struct iommu_domain *domain, ioasid_t pasid)
{
	struct iommu_group *group;
	bool ret = false;

	group = iommu_group_get(dev);
	if (WARN_ON(!group))
		return false;

	if (domain == xa_load(&group->pasid_array, pasid))
		ret = true;

	iommu_group_put(group);

	return ret;
}

Thanks,

Jacob

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe]

On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> DMA requests tagged with PASID can target individual IOMMU domains.
> Introduce a domain-wide PASID for DMA API, it will be used on the same
> mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> identity domain.

Huh? I can't understand what this is trying to say or why this patch
makes sense.

We really should not have pasid's like this attached to the domains..

Jason

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe via iommu]

On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> DMA requests tagged with PASID can target individual IOMMU domains.
> Introduce a domain-wide PASID for DMA API, it will be used on the same
> mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> identity domain.

Huh? I can't understand what this is trying to say or why this patch
makes sense.

We really should not have pasid's like this attached to the domains..

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe]

On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> On VT-d platforms with scalable mode enabled, devices issue DMA requests
> with PASID need to attach PASIDs to given IOMMU domains. The attach
> operation involves the following:
> - Programming the PASID into the device's PASID table
> - Tracking device domain and the PASID relationship
> - Managing IOTLB and device TLB invalidations
> 
> This patch add attach_dev_pasid functions to the default domain ops which
> is used by DMA and identity domain types. It could be extended to support
> other domain types whenever necessary.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 1c2c92b657c7..75615c105fdf 100644
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
>  				    u64 addr, unsigned int mask)
>  {
>  	u16 sid, qdep;
> +	ioasid_t pasid;
>  
>  	if (!info || !info->ats_enabled)
>  		return;
>  
>  	sid = info->bus << 8 | info->devfn;
>  	qdep = info->ats_qdep;
> +	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to
be flushed.

This whole API isn't suitable.

Jason

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe via iommu]

On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> On VT-d platforms with scalable mode enabled, devices issue DMA requests
> with PASID need to attach PASIDs to given IOMMU domains. The attach
> operation involves the following:
> - Programming the PASID into the device's PASID table
> - Tracking device domain and the PASID relationship
> - Managing IOTLB and device TLB invalidations
> 
> This patch add attach_dev_pasid functions to the default domain ops which
> is used by DMA and identity domain types. It could be extended to support
> other domain types whenever necessary.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 70 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 1c2c92b657c7..75615c105fdf 100644
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
>  				    u64 addr, unsigned int mask)
>  {
>  	u16 sid, qdep;
> +	ioasid_t pasid;
>  
>  	if (!info || !info->ats_enabled)
>  		return;
>  
>  	sid = info->bus << 8 | info->devfn;
>  	qdep = info->ats_qdep;
> +	pasid = iommu_get_pasid_from_domain(info->dev, &info->domain->domain);

No, a simgple domain can be attached to multiple pasids, all need to
be flushed.

This whole API isn't suitable.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > DMA requests tagged with PASID can target individual IOMMU domains.
> > Introduce a domain-wide PASID for DMA API, it will be used on the same
> > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > identity domain.  
> 
> Huh? I can't understand what this is trying to say or why this patch
> makes sense.
> 
> We really should not have pasid's like this attached to the domains..
> 
This is the same "DMA API global PASID" you reviewed in v3, I just
singled it out as a standalone patch and renamed it. Here is your previous
review comment.

> +++ b/include/linux/iommu.h
> @@ -105,6 +105,8 @@ struct iommu_domain {
>  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
>  						      void *data);
>  	void *fault_data;
> +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> +	atomic_t pasid_users;  

These are poorly named, this is really the DMA API global PASID and
shouldn't be used for other things.



Perhaps I misunderstood, do you mind explaining more?


Thanks,

Jacob

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > DMA requests tagged with PASID can target individual IOMMU domains.
> > Introduce a domain-wide PASID for DMA API, it will be used on the same
> > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > identity domain.  
> 
> Huh? I can't understand what this is trying to say or why this patch
> makes sense.
> 
> We really should not have pasid's like this attached to the domains..
> 
This is the same "DMA API global PASID" you reviewed in v3, I just
singled it out as a standalone patch and renamed it. Here is your previous
review comment.

> +++ b/include/linux/iommu.h
> @@ -105,6 +105,8 @@ struct iommu_domain {
>  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
>  						      void *data);
>  	void *fault_data;
> +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> +	atomic_t pasid_users;  

These are poorly named, this is really the DMA API global PASID and
shouldn't be used for other things.



Perhaps I misunderstood, do you mind explaining more?


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> > On VT-d platforms with scalable mode enabled, devices issue DMA requests
> > with PASID need to attach PASIDs to given IOMMU domains. The attach
> > operation involves the following:
> > - Programming the PASID into the device's PASID table
> > - Tracking device domain and the PASID relationship
> > - Managing IOTLB and device TLB invalidations
> > 
> > This patch add attach_dev_pasid functions to the default domain ops
> > which is used by DMA and identity domain types. It could be extended to
> > support other domain types whenever necessary.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 1c2c92b657c7..75615c105fdf 100644
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > device_domain_info *info, u64 addr, unsigned int mask)
> >  {
> >  	u16 sid, qdep;
> > +	ioasid_t pasid;
> >  
> >  	if (!info || !info->ats_enabled)
> >  		return;
> >  
> >  	sid = info->bus << 8 | info->devfn;
> >  	qdep = info->ats_qdep;
> > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > &info->domain->domain);  
> 
> No, a simgple domain can be attached to multiple pasids, all need to
> be flushed.
> 
Here is device TLB flush, why would I want to flush PASIDs other than my
own device attached?

At one level up, we do have a list of device to be flushed.
	list_for_each_entry(info, &domain->devices, link)
		__iommu_flush_dev_iotlb(info, addr, mask);

	
Note that RID2PASID is not in the pasid_array, its DEVTLB flush also needs
special handling in that the device is doing DMA w/o PASID, thus not aware
of RID2PASID.


> This whole API isn't suitable.
> 
> Jason


Thanks,

Jacob

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> > On VT-d platforms with scalable mode enabled, devices issue DMA requests
> > with PASID need to attach PASIDs to given IOMMU domains. The attach
> > operation involves the following:
> > - Programming the PASID into the device's PASID table
> > - Tracking device domain and the PASID relationship
> > - Managing IOTLB and device TLB invalidations
> > 
> > This patch add attach_dev_pasid functions to the default domain ops
> > which is used by DMA and identity domain types. It could be extended to
> > support other domain types whenever necessary.
> > 
> > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> >  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 1c2c92b657c7..75615c105fdf 100644
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > device_domain_info *info, u64 addr, unsigned int mask)
> >  {
> >  	u16 sid, qdep;
> > +	ioasid_t pasid;
> >  
> >  	if (!info || !info->ats_enabled)
> >  		return;
> >  
> >  	sid = info->bus << 8 | info->devfn;
> >  	qdep = info->ats_qdep;
> > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > &info->domain->domain);  
> 
> No, a simgple domain can be attached to multiple pasids, all need to
> be flushed.
> 
Here is device TLB flush, why would I want to flush PASIDs other than my
own device attached?

At one level up, we do have a list of device to be flushed.
	list_for_each_entry(info, &domain->devices, link)
		__iommu_flush_dev_iotlb(info, addr, mask);

	
Note that RID2PASID is not in the pasid_array, its DEVTLB flush also needs
special handling in that the device is doing DMA w/o PASID, thus not aware
of RID2PASID.


> This whole API isn't suitable.
> 
> Jason


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe]

On Tue, May 24, 2022 at 09:12:35AM -0700, Jacob Pan wrote:
> Hi Jason,
> 
> On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> > > On VT-d platforms with scalable mode enabled, devices issue DMA requests
> > > with PASID need to attach PASIDs to given IOMMU domains. The attach
> > > operation involves the following:
> > > - Programming the PASID into the device's PASID table
> > > - Tracking device domain and the PASID relationship
> > > - Managing IOTLB and device TLB invalidations
> > > 
> > > This patch add attach_dev_pasid functions to the default domain ops
> > > which is used by DMA and identity domain types. It could be extended to
> > > support other domain types whenever necessary.
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > >  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 70 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index 1c2c92b657c7..75615c105fdf 100644
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > > device_domain_info *info, u64 addr, unsigned int mask)
> > >  {
> > >  	u16 sid, qdep;
> > > +	ioasid_t pasid;
> > >  
> > >  	if (!info || !info->ats_enabled)
> > >  		return;
> > >  
> > >  	sid = info->bus << 8 | info->devfn;
> > >  	qdep = info->ats_qdep;
> > > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > > &info->domain->domain);  
> > 
> > No, a simgple domain can be attached to multiple pasids, all need to
> > be flushed.
> > 
> Here is device TLB flush, why would I want to flush PASIDs other than my
> own device attached?

Again, a domain can be attached to multiple PASID's *on the same
device*

The idea that there is only one PASID per domain per device is not
right.

Jason

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe via iommu]

On Tue, May 24, 2022 at 09:12:35AM -0700, Jacob Pan wrote:
> Hi Jason,
> 
> On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:
> > > On VT-d platforms with scalable mode enabled, devices issue DMA requests
> > > with PASID need to attach PASIDs to given IOMMU domains. The attach
> > > operation involves the following:
> > > - Programming the PASID into the device's PASID table
> > > - Tracking device domain and the PASID relationship
> > > - Managing IOTLB and device TLB invalidations
> > > 
> > > This patch add attach_dev_pasid functions to the default domain ops
> > > which is used by DMA and identity domain types. It could be extended to
> > > support other domain types whenever necessary.
> > > 
> > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > >  drivers/iommu/intel/iommu.c | 72 +++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 70 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > > index 1c2c92b657c7..75615c105fdf 100644
> > > +++ b/drivers/iommu/intel/iommu.c
> > > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > > device_domain_info *info, u64 addr, unsigned int mask)
> > >  {
> > >  	u16 sid, qdep;
> > > +	ioasid_t pasid;
> > >  
> > >  	if (!info || !info->ats_enabled)
> > >  		return;
> > >  
> > >  	sid = info->bus << 8 | info->devfn;
> > >  	qdep = info->ats_qdep;
> > > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > > &info->domain->domain);  
> > 
> > No, a simgple domain can be attached to multiple pasids, all need to
> > be flushed.
> > 
> Here is device TLB flush, why would I want to flush PASIDs other than my
> own device attached?

Again, a domain can be attached to multiple PASID's *on the same
device*

The idea that there is only one PASID per domain per device is not
right.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 15:02:41 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, May 24, 2022 at 09:12:35AM -0700, Jacob Pan wrote:
> > Hi Jason,
> > 
> > On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com>
> > wrote: 
> > > On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:  
> > > > On VT-d platforms with scalable mode enabled, devices issue DMA
> > > > requests with PASID need to attach PASIDs to given IOMMU domains.
> > > > The attach operation involves the following:
> > > > - Programming the PASID into the device's PASID table
> > > > - Tracking device domain and the PASID relationship
> > > > - Managing IOTLB and device TLB invalidations
> > > > 
> > > > This patch add attach_dev_pasid functions to the default domain ops
> > > > which is used by DMA and identity domain types. It could be
> > > > extended to support other domain types whenever necessary.
> > > > 
> > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > >  drivers/iommu/intel/iommu.c | 72
> > > > +++++++++++++++++++++++++++++++++++-- 1 file changed, 70
> > > > insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/intel/iommu.c
> > > > b/drivers/iommu/intel/iommu.c index 1c2c92b657c7..75615c105fdf
> > > > 100644 +++ b/drivers/iommu/intel/iommu.c
> > > > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > > > device_domain_info *info, u64 addr, unsigned int mask)
> > > >  {
> > > >  	u16 sid, qdep;
> > > > +	ioasid_t pasid;
> > > >  
> > > >  	if (!info || !info->ats_enabled)
> > > >  		return;
> > > >  
> > > >  	sid = info->bus << 8 | info->devfn;
> > > >  	qdep = info->ats_qdep;
> > > > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > > > &info->domain->domain);    
> > > 
> > > No, a simgple domain can be attached to multiple pasids, all need to
> > > be flushed.
> > >   
> > Here is device TLB flush, why would I want to flush PASIDs other than my
> > own device attached?  
> 
> Again, a domain can be attached to multiple PASID's *on the same
> device*
> 
> The idea that there is only one PASID per domain per device is not
> right.
> 
Got you, I was under the impression that there is no use case yet for
multiple PASIDs per device-domain based on our early discussion.
https://lore.kernel.org/lkml/20220315142216.GV11336@nvidia.com/

Perhaps I misunderstood. I will make the API more future proof and search
through the pasid_array xa for *all* domain-device matches. Like you
suggested earlier, may need to retrieve the xa in the first place and use
xas_for_each to get a faster search.

Thanks,

Jacob

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jacob Pan]

Hi Jason,

On Tue, 24 May 2022 15:02:41 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, May 24, 2022 at 09:12:35AM -0700, Jacob Pan wrote:
> > Hi Jason,
> > 
> > On Tue, 24 May 2022 10:51:35 -0300, Jason Gunthorpe <jgg@nvidia.com>
> > wrote: 
> > > On Wed, May 18, 2022 at 11:21:17AM -0700, Jacob Pan wrote:  
> > > > On VT-d platforms with scalable mode enabled, devices issue DMA
> > > > requests with PASID need to attach PASIDs to given IOMMU domains.
> > > > The attach operation involves the following:
> > > > - Programming the PASID into the device's PASID table
> > > > - Tracking device domain and the PASID relationship
> > > > - Managing IOTLB and device TLB invalidations
> > > > 
> > > > This patch add attach_dev_pasid functions to the default domain ops
> > > > which is used by DMA and identity domain types. It could be
> > > > extended to support other domain types whenever necessary.
> > > > 
> > > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> > > > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > > >  drivers/iommu/intel/iommu.c | 72
> > > > +++++++++++++++++++++++++++++++++++-- 1 file changed, 70
> > > > insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/intel/iommu.c
> > > > b/drivers/iommu/intel/iommu.c index 1c2c92b657c7..75615c105fdf
> > > > 100644 +++ b/drivers/iommu/intel/iommu.c
> > > > @@ -1556,12 +1556,18 @@ static void __iommu_flush_dev_iotlb(struct
> > > > device_domain_info *info, u64 addr, unsigned int mask)
> > > >  {
> > > >  	u16 sid, qdep;
> > > > +	ioasid_t pasid;
> > > >  
> > > >  	if (!info || !info->ats_enabled)
> > > >  		return;
> > > >  
> > > >  	sid = info->bus << 8 | info->devfn;
> > > >  	qdep = info->ats_qdep;
> > > > +	pasid = iommu_get_pasid_from_domain(info->dev,
> > > > &info->domain->domain);    
> > > 
> > > No, a simgple domain can be attached to multiple pasids, all need to
> > > be flushed.
> > >   
> > Here is device TLB flush, why would I want to flush PASIDs other than my
> > own device attached?  
> 
> Again, a domain can be attached to multiple PASID's *on the same
> device*
> 
> The idea that there is only one PASID per domain per device is not
> right.
> 
Got you, I was under the impression that there is no use case yet for
multiple PASIDs per device-domain based on our early discussion.
https://lore.kernel.org/lkml/20220315142216.GV11336@nvidia.com/

Perhaps I misunderstood. I will make the API more future proof and search
through the pasid_array xa for *all* domain-device matches. Like you
suggested earlier, may need to retrieve the xa in the first place and use
xas_for_each to get a faster search.

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe]

On Tue, May 24, 2022 at 01:45:26PM -0700, Jacob Pan wrote:

> > The idea that there is only one PASID per domain per device is not
> > right.
> > 
> Got you, I was under the impression that there is no use case yet for
> multiple PASIDs per device-domain based on our early discussion.

The key word there is "in-kernel" and "DMA API" - iommufd userspace
will do whatever it likes.

I wish you guys would organize your work so adding generic PASID
support to IOMMU was its own series with its own purpose so everything
stops becoming confused with SVA and DMA API ideas that are not
general.

Jason

Re: [PATCH v4 3/6] iommu/vt-d: Implement domain ops for attach_dev_pasid

[Jason Gunthorpe via iommu]

On Tue, May 24, 2022 at 01:45:26PM -0700, Jacob Pan wrote:

> > The idea that there is only one PASID per domain per device is not
> > right.
> > 
> Got you, I was under the impression that there is no use case yet for
> multiple PASIDs per device-domain based on our early discussion.

The key word there is "in-kernel" and "DMA API" - iommufd userspace
will do whatever it likes.

I wish you guys would organize your work so adding generic PASID
support to IOMMU was its own series with its own purpose so everything
stops becoming confused with SVA and DMA API ideas that are not
general.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe]

On Tue, May 24, 2022 at 08:17:27AM -0700, Jacob Pan wrote:
> Hi Jason,
> 
> On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > > DMA requests tagged with PASID can target individual IOMMU domains.
> > > Introduce a domain-wide PASID for DMA API, it will be used on the same
> > > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > > identity domain.  
> > 
> > Huh? I can't understand what this is trying to say or why this patch
> > makes sense.
> > 
> > We really should not have pasid's like this attached to the domains..
> > 
> This is the same "DMA API global PASID" you reviewed in v3, I just
> singled it out as a standalone patch and renamed it. Here is your previous
> review comment.
> 
> > +++ b/include/linux/iommu.h
> > @@ -105,6 +105,8 @@ struct iommu_domain {
> >  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
> >  						      void *data);
> >  	void *fault_data;
> > +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> > +	atomic_t pasid_users;  
> 
> These are poorly named, this is really the DMA API global PASID and
> shouldn't be used for other things.
> 
> 
> 
> Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch,
maybe it just needs a better commit message, or maybe something is
wrong.

I keep saying the DMA API usage is not special, so why do we need to
create a new global pasid and refcount? Realistically this is only
going to be used by IDXD, why can't we just allocate a PASID and
return it to the driver every time a driver asks for DMA API on PASI
mode? Why does the core need to do anything special?

Jason

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe via iommu]

On Tue, May 24, 2022 at 08:17:27AM -0700, Jacob Pan wrote:
> Hi Jason,
> 
> On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > > DMA requests tagged with PASID can target individual IOMMU domains.
> > > Introduce a domain-wide PASID for DMA API, it will be used on the same
> > > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > > identity domain.  
> > 
> > Huh? I can't understand what this is trying to say or why this patch
> > makes sense.
> > 
> > We really should not have pasid's like this attached to the domains..
> > 
> This is the same "DMA API global PASID" you reviewed in v3, I just
> singled it out as a standalone patch and renamed it. Here is your previous
> review comment.
> 
> > +++ b/include/linux/iommu.h
> > @@ -105,6 +105,8 @@ struct iommu_domain {
> >  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
> >  						      void *data);
> >  	void *fault_data;
> > +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> > +	atomic_t pasid_users;  
> 
> These are poorly named, this is really the DMA API global PASID and
> shouldn't be used for other things.
> 
> 
> 
> Perhaps I misunderstood, do you mind explaining more?

You still haven't really explained what this is for in this patch,
maybe it just needs a better commit message, or maybe something is
wrong.

I keep saying the DMA API usage is not special, so why do we need to
create a new global pasid and refcount? Realistically this is only
going to be used by IDXD, why can't we just allocate a PASID and
return it to the driver every time a driver asks for DMA API on PASI
mode? Why does the core need to do anything special?

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, May 30, 2022 8:23 PM
> 
> On Tue, May 24, 2022 at 08:17:27AM -0700, Jacob Pan wrote:
> > Hi Jason,
> >
> > On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com>
> wrote:
> >
> > > On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > > > DMA requests tagged with PASID can target individual IOMMU domains.
> > > > Introduce a domain-wide PASID for DMA API, it will be used on the
> same
> > > > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > > > identity domain.
> > >
> > > Huh? I can't understand what this is trying to say or why this patch
> > > makes sense.
> > >
> > > We really should not have pasid's like this attached to the domains..
> > >
> > This is the same "DMA API global PASID" you reviewed in v3, I just
> > singled it out as a standalone patch and renamed it. Here is your previous
> > review comment.
> >
> > > +++ b/include/linux/iommu.h
> > > @@ -105,6 +105,8 @@ struct iommu_domain {
> > >  	enum iommu_page_response_code (*iopf_handler)(struct
> iommu_fault *fault,
> > >  						      void *data);
> > >  	void *fault_data;
> > > +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> > > +	atomic_t pasid_users;
> >
> > These are poorly named, this is really the DMA API global PASID and
> > shouldn't be used for other things.
> >
> >
> >
> > Perhaps I misunderstood, do you mind explaining more?
> 
> You still haven't really explained what this is for in this patch,
> maybe it just needs a better commit message, or maybe something is
> wrong.
> 
> I keep saying the DMA API usage is not special, so why do we need to
> create a new global pasid and refcount? Realistically this is only
> going to be used by IDXD, why can't we just allocate a PASID and
> return it to the driver every time a driver asks for DMA API on PASI
> mode? Why does the core need to do anything special?
> 

Agree. I guess it was a mistake caused by treating ENQCMD as the
only user although the actual semantics of the invented interfaces
have already evolved to be quite general.

This is very similar to what we have been discussing for iommufd.
a PASID is just an additional routing info when attaching a device
to an I/O address space (DMA API in this context) and by default
it should be a per-device resource except when ENQCMD is
explicitly opt in.

Hence it's right time for us to develop common facility working
for both this DMA API usage and iommufd, i.e.:

for normal PASID attach to a domain, driver:

	allocates a local pasid from device local space;
	attaches the local pasid to a domain;

for PASID attach in particular for ENQCMD, driver:

	allocates a global pasid in system-wide;
	attaches the global pasid to a domain;
	set the global pasid in PASID_MSR;

In both cases the pasid is stored in the attach data instead of the
domain.

DMA API pasid is no special from above except it needs to allow
one device attached to the same domain twice (one with RID
and the other with RID+PASID).

for iommufd those operations are initiated by userspace via
iommufd uAPI.

Thanks
Kevin

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Monday, May 30, 2022 8:23 PM
> 
> On Tue, May 24, 2022 at 08:17:27AM -0700, Jacob Pan wrote:
> > Hi Jason,
> >
> > On Tue, 24 May 2022 10:50:34 -0300, Jason Gunthorpe <jgg@nvidia.com>
> wrote:
> >
> > > On Wed, May 18, 2022 at 11:21:15AM -0700, Jacob Pan wrote:
> > > > DMA requests tagged with PASID can target individual IOMMU domains.
> > > > Introduce a domain-wide PASID for DMA API, it will be used on the
> same
> > > > mapping as legacy DMA without PASID. Let it be IOVA or PA in case of
> > > > identity domain.
> > >
> > > Huh? I can't understand what this is trying to say or why this patch
> > > makes sense.
> > >
> > > We really should not have pasid's like this attached to the domains..
> > >
> > This is the same "DMA API global PASID" you reviewed in v3, I just
> > singled it out as a standalone patch and renamed it. Here is your previous
> > review comment.
> >
> > > +++ b/include/linux/iommu.h
> > > @@ -105,6 +105,8 @@ struct iommu_domain {
> > >  	enum iommu_page_response_code (*iopf_handler)(struct
> iommu_fault *fault,
> > >  						      void *data);
> > >  	void *fault_data;
> > > +	ioasid_t pasid;		/* Used for DMA requests with PASID */
> > > +	atomic_t pasid_users;
> >
> > These are poorly named, this is really the DMA API global PASID and
> > shouldn't be used for other things.
> >
> >
> >
> > Perhaps I misunderstood, do you mind explaining more?
> 
> You still haven't really explained what this is for in this patch,
> maybe it just needs a better commit message, or maybe something is
> wrong.
> 
> I keep saying the DMA API usage is not special, so why do we need to
> create a new global pasid and refcount? Realistically this is only
> going to be used by IDXD, why can't we just allocate a PASID and
> return it to the driver every time a driver asks for DMA API on PASI
> mode? Why does the core need to do anything special?
> 

Agree. I guess it was a mistake caused by treating ENQCMD as the
only user although the actual semantics of the invented interfaces
have already evolved to be quite general.

This is very similar to what we have been discussing for iommufd.
a PASID is just an additional routing info when attaching a device
to an I/O address space (DMA API in this context) and by default
it should be a per-device resource except when ENQCMD is
explicitly opt in.

Hence it's right time for us to develop common facility working
for both this DMA API usage and iommufd, i.e.:

for normal PASID attach to a domain, driver:

	allocates a local pasid from device local space;
	attaches the local pasid to a domain;

for PASID attach in particular for ENQCMD, driver:

	allocates a global pasid in system-wide;
	attaches the global pasid to a domain;
	set the global pasid in PASID_MSR;

In both cases the pasid is stored in the attach data instead of the
domain.

DMA API pasid is no special from above except it needs to allow
one device attached to the same domain twice (one with RID
and the other with RID+PASID).

for iommufd those operations are initiated by userspace via
iommufd uAPI.

Thanks
Kevin
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/5/31 18:12, Tian, Kevin wrote:
>>>> +++ b/include/linux/iommu.h
>>>> @@ -105,6 +105,8 @@ struct iommu_domain {
>>>>   	enum iommu_page_response_code (*iopf_handler)(struct
>> iommu_fault *fault,
>>>>   						      void *data);
>>>>   	void *fault_data;
>>>> +	ioasid_t pasid;		/* Used for DMA requests with PASID */
>>>> +	atomic_t pasid_users;
>>> These are poorly named, this is really the DMA API global PASID and
>>> shouldn't be used for other things.
>>>
>>>
>>>
>>> Perhaps I misunderstood, do you mind explaining more?
>> You still haven't really explained what this is for in this patch,
>> maybe it just needs a better commit message, or maybe something is
>> wrong.
>>
>> I keep saying the DMA API usage is not special, so why do we need to
>> create a new global pasid and refcount? Realistically this is only
>> going to be used by IDXD, why can't we just allocate a PASID and
>> return it to the driver every time a driver asks for DMA API on PASI
>> mode? Why does the core need to do anything special?
>>
> Agree. I guess it was a mistake caused by treating ENQCMD as the
> only user although the actual semantics of the invented interfaces
> have already evolved to be quite general.
> 
> This is very similar to what we have been discussing for iommufd.
> a PASID is just an additional routing info when attaching a device
> to an I/O address space (DMA API in this context) and by default
> it should be a per-device resource except when ENQCMD is
> explicitly opt in.
> 
> Hence it's right time for us to develop common facility working
> for both this DMA API usage and iommufd, i.e.:
> 
> for normal PASID attach to a domain, driver:
> 
> 	allocates a local pasid from device local space;
> 	attaches the local pasid to a domain;
> 
> for PASID attach in particular for ENQCMD, driver:
> 
> 	allocates a global pasid in system-wide;
> 	attaches the global pasid to a domain;
> 	set the global pasid in PASID_MSR;
> 
> In both cases the pasid is stored in the attach data instead of the
> domain.
> 
> DMA API pasid is no special from above except it needs to allow
> one device attached to the same domain twice (one with RID
> and the other with RID+PASID).
> 
> for iommufd those operations are initiated by userspace via
> iommufd uAPI.

My understanding is that device driver owns its PASID policy. If ENQCMD
is supported on the device, the PASIDs should be allocated through
ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
driver.

For kernel DMA w/ PASID, after the driver has a PASID for this purpose,
it can just set the default domain to the PASID on device. There's no
need for enable/disable() interfaces.

Best regards,
baolu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/5/31 18:12, Tian, Kevin wrote:
>>>> +++ b/include/linux/iommu.h
>>>> @@ -105,6 +105,8 @@ struct iommu_domain {
>>>>   	enum iommu_page_response_code (*iopf_handler)(struct
>> iommu_fault *fault,
>>>>   						      void *data);
>>>>   	void *fault_data;
>>>> +	ioasid_t pasid;		/* Used for DMA requests with PASID */
>>>> +	atomic_t pasid_users;
>>> These are poorly named, this is really the DMA API global PASID and
>>> shouldn't be used for other things.
>>>
>>>
>>>
>>> Perhaps I misunderstood, do you mind explaining more?
>> You still haven't really explained what this is for in this patch,
>> maybe it just needs a better commit message, or maybe something is
>> wrong.
>>
>> I keep saying the DMA API usage is not special, so why do we need to
>> create a new global pasid and refcount? Realistically this is only
>> going to be used by IDXD, why can't we just allocate a PASID and
>> return it to the driver every time a driver asks for DMA API on PASI
>> mode? Why does the core need to do anything special?
>>
> Agree. I guess it was a mistake caused by treating ENQCMD as the
> only user although the actual semantics of the invented interfaces
> have already evolved to be quite general.
> 
> This is very similar to what we have been discussing for iommufd.
> a PASID is just an additional routing info when attaching a device
> to an I/O address space (DMA API in this context) and by default
> it should be a per-device resource except when ENQCMD is
> explicitly opt in.
> 
> Hence it's right time for us to develop common facility working
> for both this DMA API usage and iommufd, i.e.:
> 
> for normal PASID attach to a domain, driver:
> 
> 	allocates a local pasid from device local space;
> 	attaches the local pasid to a domain;
> 
> for PASID attach in particular for ENQCMD, driver:
> 
> 	allocates a global pasid in system-wide;
> 	attaches the global pasid to a domain;
> 	set the global pasid in PASID_MSR;
> 
> In both cases the pasid is stored in the attach data instead of the
> domain.
> 
> DMA API pasid is no special from above except it needs to allow
> one device attached to the same domain twice (one with RID
> and the other with RID+PASID).
> 
> for iommufd those operations are initiated by userspace via
> iommufd uAPI.

My understanding is that device driver owns its PASID policy. If ENQCMD
is supported on the device, the PASIDs should be allocated through
ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
driver.

For kernel DMA w/ PASID, after the driver has a PASID for this purpose,
it can just set the default domain to the PASID on device. There's no
need for enable/disable() interfaces.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe]

On Tue, May 31, 2022 at 08:45:28PM +0800, Baolu Lu wrote:

> My understanding is that device driver owns its PASID policy.

I'm not sure this is actually useful, the core code should provide the
allocator as I can't think of any device that actually needs a special
allocator.

> If ENQCMD is supported on the device, the PASIDs should be allocated
> through ioasid_alloc(). Otherwise, the whole PASID pool is managed
> by the device driver.

This is OK only if we know in-advance that the device needs a global
PASID. ENQCMD is one reason, maybe there are others down the road.

Better to make this core code policy.

Jason

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe via iommu]

On Tue, May 31, 2022 at 08:45:28PM +0800, Baolu Lu wrote:

> My understanding is that device driver owns its PASID policy.

I'm not sure this is actually useful, the core code should provide the
allocator as I can't think of any device that actually needs a special
allocator.

> If ENQCMD is supported on the device, the PASIDs should be allocated
> through ioasid_alloc(). Otherwise, the whole PASID pool is managed
> by the device driver.

This is OK only if we know in-advance that the device needs a global
PASID. ENQCMD is one reason, maybe there are others down the road.

Better to make this core code policy.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Baolu,

On Tue, 31 May 2022 20:45:28 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2022/5/31 18:12, Tian, Kevin wrote:
> >>>> +++ b/include/linux/iommu.h
> >>>> @@ -105,6 +105,8 @@ struct iommu_domain {
> >>>>   	enum iommu_page_response_code (*iopf_handler)(struct  
> >> iommu_fault *fault,  
> >>>>   						      void *data);
> >>>>   	void *fault_data;
> >>>> +	ioasid_t pasid;		/* Used for DMA requests
> >>>> with PASID */
> >>>> +	atomic_t pasid_users;  
> >>> These are poorly named, this is really the DMA API global PASID and
> >>> shouldn't be used for other things.
> >>>
> >>>
> >>>
> >>> Perhaps I misunderstood, do you mind explaining more?  
> >> You still haven't really explained what this is for in this patch,
> >> maybe it just needs a better commit message, or maybe something is
> >> wrong.
> >>
> >> I keep saying the DMA API usage is not special, so why do we need to
> >> create a new global pasid and refcount? Realistically this is only
> >> going to be used by IDXD, why can't we just allocate a PASID and
> >> return it to the driver every time a driver asks for DMA API on PASI
> >> mode? Why does the core need to do anything special?
> >>  
The reason why I store PASID at IOMMU domain is for IOTLB flush within the
domain. Device driver is not aware of domain level IOTLB flush. We also
have iova_cookie for each domain which essentially is for RIDPASID.

> > Agree. I guess it was a mistake caused by treating ENQCMD as the
> > only user although the actual semantics of the invented interfaces
> > have already evolved to be quite general.
> > 
> > This is very similar to what we have been discussing for iommufd.
> > a PASID is just an additional routing info when attaching a device
> > to an I/O address space (DMA API in this context) and by default
> > it should be a per-device resource except when ENQCMD is
> > explicitly opt in.
> > 
> > Hence it's right time for us to develop common facility working
> > for both this DMA API usage and iommufd, i.e.:
> > 
> > for normal PASID attach to a domain, driver:
> > 
> > 	allocates a local pasid from device local space;
> > 	attaches the local pasid to a domain;
> > 
> > for PASID attach in particular for ENQCMD, driver:
> > 
> > 	allocates a global pasid in system-wide;
> > 	attaches the global pasid to a domain;
> > 	set the global pasid in PASID_MSR;
> > 
> > In both cases the pasid is stored in the attach data instead of the
> > domain.
> > 
So during IOTLB flush for the domain, do we loop through the attach data?

> > DMA API pasid is no special from above except it needs to allow
> > one device attached to the same domain twice (one with RID
> > and the other with RID+PASID).
> > 
> > for iommufd those operations are initiated by userspace via
> > iommufd uAPI.  
> 
> My understanding is that device driver owns its PASID policy. If ENQCMD
> is supported on the device, the PASIDs should be allocated through
> ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
> driver.
> 
It seems the changes we want for this patchset are:
1. move ioasid_alloc() from the core to device (allocation scope will be
based on whether ENQCMD is intended or not)
2. store pasid in the attach data
3. use the same iommufd api to attach/set pasid on its default domain
Am I summarizing correctly?

> For kernel DMA w/ PASID, after the driver has a PASID for this purpose,
> it can just set the default domain to the PASID on device. There's no
> need for enable/disable() interfaces.
> 
> Best regards,
> baolu


Thanks,

Jacob

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Baolu,

On Tue, 31 May 2022 20:45:28 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2022/5/31 18:12, Tian, Kevin wrote:
> >>>> +++ b/include/linux/iommu.h
> >>>> @@ -105,6 +105,8 @@ struct iommu_domain {
> >>>>   	enum iommu_page_response_code (*iopf_handler)(struct  
> >> iommu_fault *fault,  
> >>>>   						      void *data);
> >>>>   	void *fault_data;
> >>>> +	ioasid_t pasid;		/* Used for DMA requests
> >>>> with PASID */
> >>>> +	atomic_t pasid_users;  
> >>> These are poorly named, this is really the DMA API global PASID and
> >>> shouldn't be used for other things.
> >>>
> >>>
> >>>
> >>> Perhaps I misunderstood, do you mind explaining more?  
> >> You still haven't really explained what this is for in this patch,
> >> maybe it just needs a better commit message, or maybe something is
> >> wrong.
> >>
> >> I keep saying the DMA API usage is not special, so why do we need to
> >> create a new global pasid and refcount? Realistically this is only
> >> going to be used by IDXD, why can't we just allocate a PASID and
> >> return it to the driver every time a driver asks for DMA API on PASI
> >> mode? Why does the core need to do anything special?
> >>  
The reason why I store PASID at IOMMU domain is for IOTLB flush within the
domain. Device driver is not aware of domain level IOTLB flush. We also
have iova_cookie for each domain which essentially is for RIDPASID.

> > Agree. I guess it was a mistake caused by treating ENQCMD as the
> > only user although the actual semantics of the invented interfaces
> > have already evolved to be quite general.
> > 
> > This is very similar to what we have been discussing for iommufd.
> > a PASID is just an additional routing info when attaching a device
> > to an I/O address space (DMA API in this context) and by default
> > it should be a per-device resource except when ENQCMD is
> > explicitly opt in.
> > 
> > Hence it's right time for us to develop common facility working
> > for both this DMA API usage and iommufd, i.e.:
> > 
> > for normal PASID attach to a domain, driver:
> > 
> > 	allocates a local pasid from device local space;
> > 	attaches the local pasid to a domain;
> > 
> > for PASID attach in particular for ENQCMD, driver:
> > 
> > 	allocates a global pasid in system-wide;
> > 	attaches the global pasid to a domain;
> > 	set the global pasid in PASID_MSR;
> > 
> > In both cases the pasid is stored in the attach data instead of the
> > domain.
> > 
So during IOTLB flush for the domain, do we loop through the attach data?

> > DMA API pasid is no special from above except it needs to allow
> > one device attached to the same domain twice (one with RID
> > and the other with RID+PASID).
> > 
> > for iommufd those operations are initiated by userspace via
> > iommufd uAPI.  
> 
> My understanding is that device driver owns its PASID policy. If ENQCMD
> is supported on the device, the PASIDs should be allocated through
> ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
> driver.
> 
It seems the changes we want for this patchset are:
1. move ioasid_alloc() from the core to device (allocation scope will be
based on whether ENQCMD is intended or not)
2. store pasid in the attach data
3. use the same iommufd api to attach/set pasid on its default domain
Am I summarizing correctly?

> For kernel DMA w/ PASID, after the driver has a PASID for this purpose,
> it can just set the default domain to the PASID on device. There's no
> need for enable/disable() interfaces.
> 
> Best regards,
> baolu


Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe]

On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:

> The reason why I store PASID at IOMMU domain is for IOTLB flush within the
> domain. Device driver is not aware of domain level IOTLB flush. We also
> have iova_cookie for each domain which essentially is for RIDPASID.

You need to make the PASID stuff work generically.

The domain needs to hold a list of all the places it needs to flush
and that list needs to be maintained during attach/detach.

A single PASID on the domain is obviously never going to work
generically.

Jason

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jason Gunthorpe via iommu]

On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:

> The reason why I store PASID at IOMMU domain is for IOTLB flush within the
> domain. Device driver is not aware of domain level IOTLB flush. We also
> have iova_cookie for each domain which essentially is for RIDPASID.

You need to make the PASID stuff work generically.

The domain needs to hold a list of all the places it needs to flush
and that list needs to be maintained during attach/detach.

A single PASID on the domain is obviously never going to work
generically.

Jason
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Jason,

On Tue, 31 May 2022 16:05:50 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:
> 
> > The reason why I store PASID at IOMMU domain is for IOTLB flush within
> > the domain. Device driver is not aware of domain level IOTLB flush. We
> > also have iova_cookie for each domain which essentially is for
> > RIDPASID.  
> 
> You need to make the PASID stuff work generically.
> 
> The domain needs to hold a list of all the places it needs to flush
> and that list needs to be maintained during attach/detach.
> 
> A single PASID on the domain is obviously never going to work
> generically.
> 
I agree, I did it this way really meant to be part of iommu_domain's
iommu_dma_cookie, not meant to be global. But for the lack of common
storage between identity domain and dma domain, I put it here as global.

Then should we also extract RIDPASID to become part of the generic API?
i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's
pasid_array today.

Thanks,

Jacob

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Jacob Pan]

Hi Jason,

On Tue, 31 May 2022 16:05:50 -0300, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:
> 
> > The reason why I store PASID at IOMMU domain is for IOTLB flush within
> > the domain. Device driver is not aware of domain level IOTLB flush. We
> > also have iova_cookie for each domain which essentially is for
> > RIDPASID.  
> 
> You need to make the PASID stuff work generically.
> 
> The domain needs to hold a list of all the places it needs to flush
> and that list needs to be maintained during attach/detach.
> 
> A single PASID on the domain is obviously never going to work
> generically.
> 
I agree, I did it this way really meant to be part of iommu_domain's
iommu_dma_cookie, not meant to be global. But for the lack of common
storage between identity domain and dma domain, I put it here as global.

Then should we also extract RIDPASID to become part of the generic API?
i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's
pasid_array today.

Thanks,

Jacob
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, June 1, 2022 1:30 AM
> > >
> > > In both cases the pasid is stored in the attach data instead of the
> > > domain.
> > >
> So during IOTLB flush for the domain, do we loop through the attach data?

Yes and it's required.

> 
> > > DMA API pasid is no special from above except it needs to allow
> > > one device attached to the same domain twice (one with RID
> > > and the other with RID+PASID).
> > >
> > > for iommufd those operations are initiated by userspace via
> > > iommufd uAPI.
> >
> > My understanding is that device driver owns its PASID policy. If ENQCMD
> > is supported on the device, the PASIDs should be allocated through
> > ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
> > driver.
> >
> It seems the changes we want for this patchset are:
> 1. move ioasid_alloc() from the core to device (allocation scope will be
> based on whether ENQCMD is intended or not)

yes, and the driver can specify whether the allocation is system-wide
or per-device.

> 2. store pasid in the attach data
> 3. use the same iommufd api to attach/set pasid on its default domain

s/iommufd/iommu/

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, June 1, 2022 1:30 AM
> > >
> > > In both cases the pasid is stored in the attach data instead of the
> > > domain.
> > >
> So during IOTLB flush for the domain, do we loop through the attach data?

Yes and it's required.

> 
> > > DMA API pasid is no special from above except it needs to allow
> > > one device attached to the same domain twice (one with RID
> > > and the other with RID+PASID).
> > >
> > > for iommufd those operations are initiated by userspace via
> > > iommufd uAPI.
> >
> > My understanding is that device driver owns its PASID policy. If ENQCMD
> > is supported on the device, the PASIDs should be allocated through
> > ioasid_alloc(). Otherwise, the whole PASID pool is managed by the device
> > driver.
> >
> It seems the changes we want for this patchset are:
> 1. move ioasid_alloc() from the core to device (allocation scope will be
> based on whether ENQCMD is intended or not)

yes, and the driver can specify whether the allocation is system-wide
or per-device.

> 2. store pasid in the attach data
> 3. use the same iommufd api to attach/set pasid on its default domain

s/iommufd/iommu/
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, June 1, 2022 4:44 AM
> 
> Hi Jason,
> 
> On Tue, 31 May 2022 16:05:50 -0300, Jason Gunthorpe <jgg@nvidia.com>
> wrote:
> 
> > On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:
> >
> > > The reason why I store PASID at IOMMU domain is for IOTLB flush within
> > > the domain. Device driver is not aware of domain level IOTLB flush. We
> > > also have iova_cookie for each domain which essentially is for
> > > RIDPASID.
> >
> > You need to make the PASID stuff work generically.
> >
> > The domain needs to hold a list of all the places it needs to flush
> > and that list needs to be maintained during attach/detach.
> >
> > A single PASID on the domain is obviously never going to work
> > generically.
> >
> I agree, I did it this way really meant to be part of iommu_domain's
> iommu_dma_cookie, not meant to be global. But for the lack of common
> storage between identity domain and dma domain, I put it here as global.
> 
> Then should we also extract RIDPASID to become part of the generic API?
> i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's
> pasid_array today.
> 

RIDPASID is just an alias to RID in the PASID table (similar to pasid#0
on other platforms). it's reserved and not exposed outside the 
intel-iommu driver. So I don't think it should be managed via the
generic iommu API. But probably you can generalize it with other
pasids within intel-iommu driver if doing so can simplify the logic
there.

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Wednesday, June 1, 2022 4:44 AM
> 
> Hi Jason,
> 
> On Tue, 31 May 2022 16:05:50 -0300, Jason Gunthorpe <jgg@nvidia.com>
> wrote:
> 
> > On Tue, May 31, 2022 at 10:29:55AM -0700, Jacob Pan wrote:
> >
> > > The reason why I store PASID at IOMMU domain is for IOTLB flush within
> > > the domain. Device driver is not aware of domain level IOTLB flush. We
> > > also have iova_cookie for each domain which essentially is for
> > > RIDPASID.
> >
> > You need to make the PASID stuff work generically.
> >
> > The domain needs to hold a list of all the places it needs to flush
> > and that list needs to be maintained during attach/detach.
> >
> > A single PASID on the domain is obviously never going to work
> > generically.
> >
> I agree, I did it this way really meant to be part of iommu_domain's
> iommu_dma_cookie, not meant to be global. But for the lack of common
> storage between identity domain and dma domain, I put it here as global.
> 
> Then should we also extract RIDPASID to become part of the generic API?
> i.e. set pasid, flush IOTLB etc. Right? RIDPASID is not in group's
> pasid_array today.
> 

RIDPASID is just an alias to RID in the PASID table (similar to pasid#0
on other platforms). it's reserved and not exposed outside the 
intel-iommu driver. So I don't think it should be managed via the
generic iommu API. But probably you can generalize it with other
pasids within intel-iommu driver if doing so can simplify the logic
there.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/6/1 09:43, Tian, Kevin wrote:
>> From: Jacob Pan<jacob.jun.pan@linux.intel.com>
>> Sent: Wednesday, June 1, 2022 1:30 AM
>>>> In both cases the pasid is stored in the attach data instead of the
>>>> domain.
>>>>
>> So during IOTLB flush for the domain, do we loop through the attach data?
> Yes and it's required.

What does the attach data mean here? Do you mean group->pasid_array?

Why not tracking the {device, pasid} info in the iommu driver when
setting domain to {device, pasid}? We have tracked device in a list when
setting a domain to device.

Best regards,
baolu

Re: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Baolu Lu]

On 2022/6/1 09:43, Tian, Kevin wrote:
>> From: Jacob Pan<jacob.jun.pan@linux.intel.com>
>> Sent: Wednesday, June 1, 2022 1:30 AM
>>>> In both cases the pasid is stored in the attach data instead of the
>>>> domain.
>>>>
>> So during IOTLB flush for the domain, do we loop through the attach data?
> Yes and it's required.

What does the attach data mean here? Do you mean group->pasid_array?

Why not tracking the {device, pasid} info in the iommu driver when
setting domain to {device, pasid}? We have tracked device in a list when
setting a domain to device.

Best regards,
baolu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, June 1, 2022 5:37 PM
> 
> On 2022/6/1 09:43, Tian, Kevin wrote:
> >> From: Jacob Pan<jacob.jun.pan@linux.intel.com>
> >> Sent: Wednesday, June 1, 2022 1:30 AM
> >>>> In both cases the pasid is stored in the attach data instead of the
> >>>> domain.
> >>>>
> >> So during IOTLB flush for the domain, do we loop through the attach data?
> > Yes and it's required.
> 
> What does the attach data mean here? Do you mean group->pasid_array?

any structure describing the attaching relationship from {device, pasid} to
a domain

> 
> Why not tracking the {device, pasid} info in the iommu driver when
> setting domain to {device, pasid}? We have tracked device in a list when
> setting a domain to device.
> 

Yes, that tracking structure is the attach data. 😊

RE: [PATCH v4 1/6] iommu: Add a per domain PASID for DMA API

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, June 1, 2022 5:37 PM
> 
> On 2022/6/1 09:43, Tian, Kevin wrote:
> >> From: Jacob Pan<jacob.jun.pan@linux.intel.com>
> >> Sent: Wednesday, June 1, 2022 1:30 AM
> >>>> In both cases the pasid is stored in the attach data instead of the
> >>>> domain.
> >>>>
> >> So during IOTLB flush for the domain, do we loop through the attach data?
> > Yes and it's required.
> 
> What does the attach data mean here? Do you mean group->pasid_array?

any structure describing the attaching relationship from {device, pasid} to
a domain

> 
> Why not tracking the {device, pasid} info in the iommu driver when
> setting domain to {device, pasid}? We have tracked device in a list when
> setting a domain to device.
> 

Yes, that tracking structure is the attach data. 😊
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu