Re-enable IDXD kernel workqueue under DMA API

Supervisor Request Enable (SRE) bit in a PASID entry is for permission checking on DMA requests. When SRE = 0, DMA with supervisor privilege will be blocked. However, for in-kernel DMA this is not necessary in that we are targeting kernel memory anyway. There's no need to differentiate user and kernel for in-kernel DMA.

Let's use non-privileged (user) permission for all PASIDs used in kernel, it will be consistent with DMA without PASID (RID_PASID) as well.

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

There's no more usage, remove PASID supervisor support.

Suggested-by Lu Baolu<[email protected]>

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

Devices that use Intel ENQCMD to submit work must use global PASIDs in that the PASID are stored in a per CPU MSR. When such device need to submit work for in-kernel DMA with PASID, it must allocate PASIDs from the same global number space to avoid conflict.

This patch introduces IOMMU SVA APIs to reserve and release global PASIDs. It is expected that device drivers will use the allocated PASIDs to attach to appropriate IOMMU domains for use.

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

---
  drivers/iommu/iommu-sva.c | 33 +++++++++++++++++++++++++++++++++
  include/linux/iommu.h     | 14 ++++++++++++++
  2 files changed, 47 insertions(+)

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

index c434b95dc8eb..84b9de84b3e0 100644
@@ -148,6 +148,39 @@ u32 iommu_sva_get_pasid(struct iommu_sva *handle)
  }
  EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
 
+/**
+ * @brief
+ *	Reserve a PASID from the SVA global number space.
+ *

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

+		return;
+
+	ida_free(&iommu_global_pasid_ida, pasid);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_release_pasid);
+
  /*
   * I/O page fault handler for SVA
   */

include/linux/iommu.h +7 -0

index 54f535ff9868..0471089dc1d0 100644
@@ -1187,6 +1187,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
  					struct mm_struct *mm);
  void iommu_sva_unbind_device(struct iommu_sva *handle);
  u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max);
+void iommu_sva_release_pasid(ioasid_t pasid);
+
  #else
  static inline struct iommu_sva *
  iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)

@@ -1202,6 +1205,17 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
  {
  	return IOMMU_PASID_INVALID;
  }
+
+static inline ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max)
+{
+	return IOMMU_PASID_INVALID;

Quoted 32 lines · Jacob Pan

Devices that use Intel ENQCMD to submit work must use global PASIDs in that the PASID are stored in a per CPU MSR. When such device need to submit work for in-kernel DMA with PASID, it must allocate PASIDs from the same global number space to avoid conflict.

This patch introduces IOMMU SVA APIs to reserve and release global PASIDs. It is expected that device drivers will use the allocated PASIDs to attach to appropriate IOMMU domains for use.

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

---
  drivers/iommu/iommu-sva.c | 33 +++++++++++++++++++++++++++++++++
  include/linux/iommu.h     | 14 ++++++++++++++
  2 files changed, 47 insertions(+)

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

index c434b95dc8eb..84b9de84b3e0 100644
@@ -148,6 +148,39 @@ u32 iommu_sva_get_pasid(struct iommu_sva *handle)
  }
  EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
 
+/**
+ * @brief
+ *	Reserve a PASID from the SVA global number space.
+ *
+ * @param min starting range, inclusive
+ * @param max ending range, inclusive
+ * @return The reserved PASID on success or IOMMU_PASID_INVALID on

failure.
+ */

drivers/iommu/iommu-sva.c u32 iommu_sva_get_pasid(struct iommu_sva *handle) · line 164

Quoted 6 lines

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

+ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max)
+{
+	int ret;
+
+	if (!pasid_valid(min) || !pasid_valid(max) ||
+	    min == 0 || max < min)  

I still think we should make "min == 0" a valid case. The ARM/AMD/Intel drivers should reserve PASID 0 for special usage with this interface.

Probably we should also make "min == max" a valid case. Both @min and @max are inclusive.

Quoted 5 lines

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

+		return IOMMU_PASID_INVALID;
+
+	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,

GFP_KERNEL);

drivers/iommu/iommu-sva.c u32 iommu_sva_get_pasid(struct iommu_sva *handle) · line 177

Quoted 10 lines

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

+	if (ret < 0)
+		return IOMMU_PASID_INVALID;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_reserve_pasid);
+
+void iommu_sva_release_pasid(ioasid_t pasid)
+{
+	if (!pasid_valid(pasid))  

The caller should never release an invalid pasid. So perhaps,

	if (WARN_ON(!pasid_valid(pasid)))
		return;

to discover bugs during development.

Quoted 36 lines

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

+		return;
+
+	ida_free(&iommu_global_pasid_ida, pasid);
+}
+EXPORT_SYMBOL_GPL(iommu_sva_release_pasid);
+
  /*
   * I/O page fault handler for SVA
   */

include/linux/iommu.h +11 -0

index 54f535ff9868..0471089dc1d0 100644
@@ -1187,6 +1187,9 @@ struct iommu_sva *iommu_sva_bind_device(struct
 device *dev, struct mm_struct *mm);
  void iommu_sva_unbind_device(struct iommu_sva *handle);
  u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max);
+void iommu_sva_release_pasid(ioasid_t pasid);
+
  #else
  static inline struct iommu_sva *
  iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)

@@ -1202,6 +1205,17 @@ static inline u32 iommu_sva_get_pasid(struct
 iommu_sva *handle) {
  	return IOMMU_PASID_INVALID;
  }
+
+static inline ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t
 max) +{
+	return IOMMU_PASID_INVALID;
+}
+
+static inline void iommu_sva_release_pasid(ioasid_t pasid)
+{
+

include/linux/iommu.h static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle) · line 1221

Quoted 5 lines

include/linux/iommu.h +2 -0

+}
+
  static inline void mm_pasid_init(struct mm_struct *mm) {}
  static inline void mm_pasid_drop(struct mm_struct *mm) {}
  #endif /* CONFIG_IOMMU_SVA */  

Best regards,
baolu

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

+/**
+ * @brief
+ * Reserve a PASID from the SVA global number space.
+ *
+ * @param min starting range, inclusive
+ * @param max ending range, inclusive
+ * @return The reserved PASID on success or IOMMU_PASID_INVALID on failure.
+ */

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

+ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max)
+{
+	int ret;
+
+	if (!pasid_valid(min) || !pasid_valid(max) ||
+	    min == 0 || max < min)
+		return IOMMU_PASID_INVALID;
+
+	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,

GFP_KERNEL);

Quoted 22 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

+/**
+ * @brief
+ * Reserve a PASID from the SVA global number space.
+ *
+ * @param min starting range, inclusive
+ * @param max ending range, inclusive
+ * @return The reserved PASID on success or IOMMU_PASID_INVALID on failure.
+ */

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

+ioasid_t iommu_sva_reserve_pasid(ioasid_t min, ioasid_t max)
+{
+	int ret;
+
+	if (!pasid_valid(min) || !pasid_valid(max) ||
+	    min == 0 || max < min)
+		return IOMMU_PASID_INVALID;
+
+	ret = ida_alloc_range(&iommu_global_pasid_ida, min, max,

GFP_KERNEL);

drivers/iommu/iommu-sva.c u32 iommu_sva_get_pasid(struct iommu_sva *handle) · line 174

Quoted 7 lines

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

+	if (ret < 0)
+		return IOMMU_PASID_INVALID;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_sva_reserve_pasid);
+  

Look at this function. There is no single word about sva except in the function name.

sva is just one user of global pasids.

when a driver supports sva it has to always use global pasids even for non-sva usages like dma pasid.

but this doesn't mean that we should build the API around sva.

it's really about global pasids.

let's just call it clearly as iommu_alloc_global_pasid(min, max).

Then we can define a wrapper iommu_reserve_global_pasid(pasid) as iommu_alloc_global_pasid(pasid, pasid).

for PASID#0 driver calls iommu_reserve_global_pasid(0).

for dma pasid driver calls iommu_alloc_global_pasid() to get a random one instead of reserving pasid#1.

this would be future proof when the same driver starts to allocate more pasids for other usages e..g siov.

Quoted 2 lines

this would be future proof when the same driver starts to allocate more pasids for other usages e..g siov.

I don't have strong preference here. Jason and others?

Quoted 3 lines · Jacob Pan

Quoted 2 lines

this would be future proof when the same driver starts to allocate more pasids for other usages e..g siov.

I don't have strong preference here. Jason and others?

Oh I'm all for getting the word SVA out of the PASID infrastructure.

SVA has nothing to do with PASID other than it requires it to work.

On VT-d platforms, RID_PASID is used for DMA request without PASID. We should not treat RID_PASID special instead let it be allocated from the global SVA PASID number space.

Consequently, for devices also do DMA with PASID, drivers will not worry about conflicts when it comes to allocating PASIDs for in-kernel DMA.

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

---
  drivers/iommu/intel/iommu.c | 4 ++++
  1 file changed, 4 insertions(+)

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

index 9f737ef55463..61c06f7ad8f7 100644

Quoted 3 lines · Jacob Pan

On VT-d platforms, RID_PASID is used for DMA request without PASID. We should not treat RID_PASID special instead let it be allocated from the global SVA PASID number space.

It's same to AMD and ARM SMMUv3, right? They also need an explicit reservation of PASID 0.

Quoted 15 lines

Consequently, for devices also do DMA with PASID, drivers will not worry about conflicts when it comes to allocating PASIDs for in-kernel DMA.

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

---
  drivers/iommu/intel/iommu.c | 4 ++++
  1 file changed, 4 insertions(+)

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

index 9f737ef55463..61c06f7ad8f7 100644
@@ -3956,6 +3956,10 @@ int __init intel_iommu_init(void)
 

drivers/iommu/intel/iommu.c int __init intel_iommu_init(void) · line 3959

Quoted 3 lines

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

  	intel_iommu_enabled = 1;
 
+#ifdef CONFIG_INTEL_IOMMU_SVM  

Do we really need this #ifdef? IOMMU_SVA is selected by INTEL_IOMMU_SVM, right? So if CONFIG_INTEL_IOMMU_SVM is not set, iommu_sva_reserve_pasid() is just a dumb.

Quoted 7 lines

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

+	/* Reserved RID_PASID from the global namespace for legacy DMA
 */
+	iommu_sva_reserve_pasid(PASID_RID2PASID, PASID_RID2PASID);
+#endif
  	return 0;
 

out_free_dmar:

Best regards,
baolu

Quoted 7 lines

Quoted 3 lines

On VT-d platforms, RID_PASID is used for DMA request without PASID. We should not treat RID_PASID special instead let it be allocated from the global SVA PASID number space.

It's same to AMD and ARM SMMUv3, right? They also need an explicit reservation of PASID 0.

yes, all IOMMU drivers need to do that. I will give it a try but might need help to place the call.

Quoted 6 lines · Jacob Pan

Quoted 6 lines

Quoted 3 lines

On VT-d platforms, RID_PASID is used for DMA request without PASID. We should not treat RID_PASID special instead let it be allocated from the global SVA PASID number space.

It's same to AMD and ARM SMMUv3, right? They also need an explicit reservation of PASID 0.

Quoted 2 lines

yes, all IOMMU drivers need to do that. I will give it a try but might need help to place the call.

It might be simpler to just let SVA code allocate from 1 up instead of 0 (as is in the current code). Global PASID allocator would still allow the full range from 0 to max. Then there is no change to architectures that don't support non-zero RID_PASID. For VT-d, it would still work in the future when we have nonzero RID_PASID. is that reasonable?

Quoted 3 lines · Jacob Pan

On VT-d platforms, RID_PASID is used for DMA request without PASID. We should not treat RID_PASID special instead let it be allocated from the global SVA PASID number space.

It's same to AMD and ARM SMMUv3, right? They also need an explicit reservation of PASID 0.

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

index 65b15be72878..b6c26f25d1ba 100644
@@ -595,6 +595,7 @@ struct dmar_domain {
 
  	spinlock_t lock;		/* Protect device tracking lists */
  	struct list_head devices;	/* all devices' list */
+	struct list_head dev_pasids;	/* all attached pasids */
 
  	struct dma_pte	*pgd;		/* virtual address */
  	int		gaw;		/* max guest address width */

@@ -708,6 +709,7 @@ struct device_domain_info {
  	u8 ats_supported:1;
  	u8 ats_enabled:1;
  	u8 dtlb_extra_inval:1;	/* Quirk for devices need extra flush */
+	u8 dev_attached:1;	/* Device context activated */
  	u8 ats_qdep;
  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
  	struct intel_iommu *iommu; /* IOMMU used by this device */

From: Baolu Lu <[email protected]>
Sent: Tuesday, March 28, 2023 1:49 PM

Quoted 21 lines · Jacob Pan

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

index 65b15be72878..b6c26f25d1ba 100644
@@ -595,6 +595,7 @@ struct dmar_domain {
 
  	spinlock_t lock;		/* Protect device tracking lists */
  	struct list_head devices;	/* all devices' list */
+	struct list_head dev_pasids;	/* all attached pasids */
 
  	struct dma_pte	*pgd;		/* virtual address */
  	int		gaw;		/* max guest address width */

@@ -708,6 +709,7 @@ struct device_domain_info {
  	u8 ats_supported:1;
  	u8 ats_enabled:1;
  	u8 dtlb_extra_inval:1;	/* Quirk for devices need extra flush */
+	u8 dev_attached:1;	/* Device context activated */
  	u8 ats_qdep;
  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
  	struct intel_iommu *iommu; /* IOMMU used by this device */

drivers/iommu/intel/iommu.h struct device_domain_info { · line 724

Quoted 8 lines

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

@@ -715,6 +717,12 @@ struct device_domain_info {
  	struct pasid_table *pasid_table; /* pasid table */
  };
 
+struct device_pasid_info {
+	struct list_head link_domain;	/* link to domain siblings */
+	struct device *dev;		/* physical device derived from */
+	ioasid_t pasid;			/* PASID on physical device */
+};

The dev_pasids list seems to be duplicate with iommu_group::pasid_array.

The pasid_array is de facto per-device as the PCI subsystem requires ACS to be enabled on the upstream path to the root port.

pci_enable_pasid():
385 if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
386 return -EINVAL;

For such PCI topology, pci_device_group() always assigns an exclusive iommu group (a.k.a. singleton group).

So, how about moving the pasid_array from struct iommu_group to struct dev_iommu? With this refactoring, the individual iommu driver has no need to create their own pasid array or list.

Instead of using iommu_group::mutex, perhaps the pasid_array needs its own lock in struct dev_iommu after moving.

Quoted 49 lines

From: Baolu Lu <[email protected]>
Sent: Tuesday, March 28, 2023 1:49 PM

Quoted 31 lines · Jacob Pan

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

index 65b15be72878..b6c26f25d1ba 100644
@@ -595,6 +595,7 @@ struct dmar_domain {
 
  	spinlock_t lock;		/* Protect device tracking

lists */ struct list_head devices; /* all devices' list */

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

+	struct list_head dev_pasids;	/* all attached pasids */
 
  	struct dma_pte	*pgd;		/* virtual

address */ int gaw; /* max guest
address width */ @@ -708,6 +709,7 @@ struct device_domain_info {
u8 ats_supported:1;
u8 ats_enabled:1;
u8 dtlb_extra_inval:1; /* Quirk for devices need
extra flush */

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

+	u8 dev_attached:1;	/* Device context activated */
  	u8 ats_qdep;
  	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
  	struct intel_iommu *iommu; /* IOMMU used by this device */

@@ -715,6 +717,12 @@ struct device_domain_info {
  	struct pasid_table *pasid_table; /* pasid table */
  };
 
+struct device_pasid_info {
+	struct list_head link_domain;	/* link to domain

siblings */

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

+	struct device *dev;		/* physical device

derived from */

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

+	ioasid_t pasid;			/* PASID on physical

Quoted 1 line

device */ +};

The dev_pasids list seems to be duplicate with iommu_group::pasid_array.

The pasid_array is de facto per-device as the PCI subsystem requires ACS to be enabled on the upstream path to the root port.

pci_enable_pasid():
385 if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR |
PCI_ACS_UF)) 386 return -EINVAL;

For such PCI topology, pci_device_group() always assigns an exclusive iommu group (a.k.a. singleton group).

Quoted 7 lines

So, how about moving the pasid_array from struct iommu_group to struct dev_iommu? With this refactoring, the individual iommu driver has no need to create their own pasid array or list.

Instead of using iommu_group::mutex, perhaps the pasid_array needs its own lock in struct dev_iommu after moving.

What you suggested is a right thing and more friendly to pasid attach in iommufd [1].

but dev_pasids list here is a different thing. It tracks which [device, pasid] is attached to the domain. w/o this information you'll have to walk the pasid_array of every attached device under the domain and search for every pasid entry pointing to the said domain. It's very inefficient.

of course if this can be done more generally it'd be nice.😊

[1] https://lore.kernel.org/linux-iommu/[email protected]/

Quoted 46 lines

From: Baolu Lu <[email protected]>
Sent: Tuesday, March 28, 2023 1:49 PM

Quoted 21 lines · Jacob Pan

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

index 65b15be72878..b6c26f25d1ba 100644
@@ -595,6 +595,7 @@ struct dmar_domain {
 
   	spinlock_t lock;		/* Protect device tracking lists */
   	struct list_head devices;	/* all devices' list */
+	struct list_head dev_pasids;	/* all attached pasids */
 
   	struct dma_pte	*pgd;		/* virtual address */
   	int		gaw;		/* max guest address width */

@@ -708,6 +709,7 @@ struct device_domain_info {
   	u8 ats_supported:1;
   	u8 ats_enabled:1;
   	u8 dtlb_extra_inval:1;	/* Quirk for devices need extra flush */
+	u8 dev_attached:1;	/* Device context activated */
   	u8 ats_qdep;
   	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
   	struct intel_iommu *iommu; /* IOMMU used by this device */

drivers/iommu/intel/iommu.h struct device_domain_info { · line 724

Quoted 8 lines

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

@@ -715,6 +717,12 @@ struct device_domain_info {
   	struct pasid_table *pasid_table; /* pasid table */
   };
 
+struct device_pasid_info {
+	struct list_head link_domain;	/* link to domain siblings */
+	struct device *dev;		/* physical device derived from */
+	ioasid_t pasid;			/* PASID on physical device */
+};

The dev_pasids list seems to be duplicate with iommu_group::pasid_array.

The pasid_array is de facto per-device as the PCI subsystem requires ACS to be enabled on the upstream path to the root port.

pci_enable_pasid():
385 if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
386 return -EINVAL;

For such PCI topology, pci_device_group() always assigns an exclusive iommu group (a.k.a. singleton group).

Quoted 7 lines

So, how about moving the pasid_array from struct iommu_group to struct dev_iommu? With this refactoring, the individual iommu driver has no need to create their own pasid array or list.

Instead of using iommu_group::mutex, perhaps the pasid_array needs its own lock in struct dev_iommu after moving.

What you suggested is a right thing and more friendly to pasid attach in iommufd [1].

but dev_pasids list here is a different thing. It tracks which [device, pasid] is attached to the domain. w/o this information you'll have to walk the pasid_array of every attached device under the domain and search for every pasid entry pointing to the said domain. It's very inefficient.

of course if this can be done more generally it'd be nice.😊

[1] https://lore.kernel.org/linux-iommu/[email protected]/

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

Devices that use ENQCMDS to submit work on buffers mapped by DMA API must attach a PASID to the default domain of the device. In preparation for this use case, this patch implements set_dev_pasid() for the default_domain_ops. Besides PASID attachment, device will also be attached to the domain as the result of this call if the device has not been attached before.

Quoted 10 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

Devices that use ENQCMDS to submit work on buffers mapped by DMA API must attach a PASID to the default domain of the device. In preparation for this use case, this patch implements set_dev_pasid() for the default_domain_ops. Besides PASID attachment, device will also be attached to the domain as the result of this call if the device has not been attached before.

I didn't get the last point. PASID attach should only have the scope for the pasid. RID of the device might be attached to another domain which shouldn't be changed by this call.

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 11:40 PM

Hi Kevin,

Quoted 4 lines · Kevin

Quoted 4 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

Devices that use ENQCMDS to submit work on buffers mapped by DMA

API

Quoted 10 lines

Quoted 6 lines

must attach a PASID to the default domain of the device. In preparation for this use case, this patch implements set_dev_pasid() for the default_domain_ops. Besides PASID attachment, device will also be attached to the domain as the result of this call if the device has not been attached before.

I didn't get the last point. PASID attach should only have the scope for the pasid. RID of the device might be attached to another domain which shouldn't be changed by this call.

I meant if the RID context has not been set up before attaching this PASID, this call will also set up the context, PASID dir etc. In the end, we eliminated ordering requirement of attaching device, RID_PASID first, then other PASIDs.
How about:
"If the device context has not been set up prior to this call, this will set up the device context in addition to PASID attachment."

Hi Kevin,

Quoted 11 lines · Kevin

Quoted 10 lines

From: Jacob Pan <[email protected]>
Sent: Tuesday, March 28, 2023 7:22 AM

Devices that use ENQCMDS to submit work on buffers mapped by DMA API must attach a PASID to the default domain of the device. In preparation for this use case, this patch implements set_dev_pasid() for the default_domain_ops. Besides PASID attachment, device will also be attached to the domain as the result of this call if the device has not been attached before.

Quoted 3 lines

I didn't get the last point. PASID attach should only have the scope for the pasid. RID of the device might be attached to another domain which shouldn't be changed by this call.

I meant if the RID context has not been set up before attaching this PASID, this call will also set up the context, PASID dir etc. In the end, we eliminated ordering requirement of attaching device, RID_PASID first, then other PASIDs.

How about:
"If the device context has not been set up prior to this call, this will set up the device context in addition to PASID attachment."

Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API.

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

---
  drivers/iommu/iommu.c | 1 +
  include/linux/iommu.h | 5 +++++
  2 files changed, 6 insertions(+)

drivers/iommu/iommu.c +0 -0

index 10db680acaed..c51d343a75d2 100644

From: Baolu Lu <[email protected]>
Sent: Tuesday, March 28, 2023 2:04 PM

drivers/iommu/iommu.c struct iommu_domain

Quoted 15 lines · Jacob Pan

Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API.

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

---
  drivers/iommu/iommu.c | 1 +
  include/linux/iommu.h | 5 +++++
  2 files changed, 6 insertions(+)

drivers/iommu/iommu.c +0 -0

index 10db680acaed..c51d343a75d2 100644
@@ -2118,6 +2118,7 @@ struct iommu_domain

*iommu_get_dma_domain(struct device *dev)

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2121

Quoted 4 lines

drivers/iommu/iommu.c +1 -0

  {
  	return dev->iommu_group->default_domain;
  }
+EXPORT_SYMBOL_GPL(iommu_get_dma_domain);

Directly exporting this function for external use seems unsafe. If the caller is the kernel driver for this device, it's fine because default domain remains unchanged during the life cycle of the driver. Otherwise, using this function may cause UAF. Keep in mind that group's default domain could be changed through sysfs.

However, iommu_get_domain_for_dev() has already done so and has been exported. Maybe I'm worried too much. :-)

Quoted 31 lines

From: Baolu Lu <[email protected]>
Sent: Tuesday, March 28, 2023 2:04 PM

drivers/iommu/iommu.c struct iommu_domain

Quoted 15 lines · Jacob Pan

Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API.

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

---
  drivers/iommu/iommu.c | 1 +
  include/linux/iommu.h | 5 +++++
  2 files changed, 6 insertions(+)

drivers/iommu/iommu.c +0 -0

index 10db680acaed..c51d343a75d2 100644
@@ -2118,6 +2118,7 @@ struct iommu_domain  

*iommu_get_dma_domain(struct device *dev)

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct device *dev) · line 2121

Quoted 4 lines

drivers/iommu/iommu.c +1 -0

  {
  	return dev->iommu_group->default_domain;
  }
+EXPORT_SYMBOL_GPL(iommu_get_dma_domain);  

Directly exporting this function for external use seems unsafe. If the caller is the kernel driver for this device, it's fine because default domain remains unchanged during the life cycle of the driver. Otherwise, using this function may cause UAF. Keep in mind that group's default domain could be changed through sysfs.

Quoted 3 lines

However, iommu_get_domain_for_dev() has already done so and has been exported. Maybe I'm worried too much. :-)

Agree. The kernel driver managing the device wants to get the current domain of the device then iommu_get_domain_for_dev() is the right interface. It knows the domain is the dma domain.

Quoted 3 lines

Agree. The kernel driver managing the device wants to get the current domain of the device then iommu_get_domain_for_dev() is the right interface. It knows the domain is the dma domain.

This goes back to v1 then :)

I thought the comments from v1 is that we don't want to check the domain type is DMA domain returned by iommu_get_domain_for_dev()

If the driver "knows" the domain is dma domain, why can't it use iommu_get_dma_domain()? seems paradoxical.

Quoted 10 lines · Jacob Pan

Quoted 3 lines

Agree. The kernel driver managing the device wants to get the current domain of the device then iommu_get_domain_for_dev() is the right interface. It knows the domain is the dma domain.

This goes back to v1 then :)

I thought the comments from v1 is that we don't want to check the domain type is DMA domain returned by iommu_get_domain_for_dev()

If the driver "knows" the domain is dma domain, why can't it use iommu_get_dma_domain()? seems paradoxical.

Huh?

The DMA API operates on the current domain of the device, be it IDENTITY or UNMANAGED.

You have to copy whatever the current domain is to the PASID and you should definately not check if it is DMA or something.

Quoted 10 lines · Jacob Pan

Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API.

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

---
  drivers/iommu/iommu.c | 1 +
  include/linux/iommu.h | 5 +++++
  2 files changed, 6 insertions(+)

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct · line 2121

Quoted 9 lines

drivers/iommu/iommu.c +1 -0

index 10db680acaed..c51d343a75d2 100644
@@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct
 device *dev) {
  	return dev->iommu_group->default_domain;
  }
+EXPORT_SYMBOL_GPL(iommu_get_dma_domain);  

Directly exporting this function for external use seems unsafe. If the caller is the kernel driver for this device, it's fine because default domain remains unchanged during the life cycle of the driver. Otherwise, using this function may cause UAF. Keep in mind that group's default domain could be changed through sysfs.

However, iommu_get_domain_for_dev() has already done so and has been exported. Maybe I'm worried too much. :-)

Best regards,
baolu

Quoted 11 lines · Baolu Lu

Quoted 10 lines · Jacob Pan

Devices that use ENQCMDS to submit work needs to retrieve its DMA domain. It can then attach PASID to the DMA domain for shared mapping (with RID) established by DMA API.

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

---
   drivers/iommu/iommu.c | 1 +
   include/linux/iommu.h | 5 +++++
   2 files changed, 6 insertions(+)

Quoted 14 lines

drivers/iommu/iommu.c struct iommu_domain *iommu_get_dma_domain(struct · line 2121

Quoted 9 lines

drivers/iommu/iommu.c +1 -0

index 10db680acaed..c51d343a75d2 100644
@@ -2118,6 +2118,7 @@ struct iommu_domain *iommu_get_dma_domain(struct
 device *dev) {
   	return dev->iommu_group->default_domain;
   }
+EXPORT_SYMBOL_GPL(iommu_get_dma_domain);

Directly exporting this function for external use seems unsafe. If the caller is the kernel driver for this device, it's fine because default domain remains unchanged during the life cycle of the driver. Otherwise, using this function may cause UAF. Keep in mind that group's default domain could be changed through sysfs.

don't you have to unload the driver?

Kernel workqueues were disabled due to flawed use of kernel VA and SVA API. Now That we have the support for attaching PASID to the device's

default domain and the ability to reserve global PASIDs from SVA APIs, we can re-enable the kernel work queues and use them under DMA API.

We also use non-privileged access for in-kernel DMA to be consistent with the IOMMU settings. Consequently, interrupt for user privilege is enabled for work completion IRQs.

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Reviewed-by Dave Jiang <[email protected]>

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

---
  drivers/dma/idxd/device.c | 30 ++++-------------------
  drivers/dma/idxd/init.c   | 51 ++++++++++++++++++++++++++++++++++++---
  drivers/dma/idxd/sysfs.c  |  7 ------
  3 files changed, 52 insertions(+), 36 deletions(-)

drivers/dma/idxd/device.c +5 -25

index 6fca8fa8d3a8..f6b133d61a04 100644
@@ -299,21 +299,6 @@ void idxd_wqs_unmap_portal(struct idxd_device *idxd)
  	}
  }
 
-static void __idxd_wq_set_priv_locked(struct idxd_wq *wq, int priv)
-{
-	struct idxd_device *idxd = wq->idxd;
-	union wqcfg wqcfg;
-	unsigned int offset;
-
-	offset = WQCFG_OFFSET(idxd, wq->id, WQCFG_PRIVL_IDX);
-	spin_lock(&idxd->dev_lock);
-	wqcfg.bits[WQCFG_PRIVL_IDX] = ioread32(idxd->reg_base + offset);
-	wqcfg.priv = priv;
-	wq->wqcfg->bits[WQCFG_PRIVL_IDX] = wqcfg.bits[WQCFG_PRIVL_IDX];
-	iowrite32(wqcfg.bits[WQCFG_PRIVL_IDX], idxd->reg_base + offset);
-	spin_unlock(&idxd->dev_lock);
-}
-
  static void __idxd_wq_set_pasid_locked(struct idxd_wq *wq, int pasid)
  {
  	struct idxd_device *idxd = wq->idxd;

@@ -1324,15 +1309,14 @@ int drv_enable_wq(struct idxd_wq *wq)
  	}
 
  	/*
-	 * In the event that the WQ is configurable for pasid and priv bits.
-	 * For kernel wq, the driver should setup the pasid, pasid_en, and priv bit.
-	 * However, for non-kernel wq, the driver should only set the pasid_en bit for
-	 * shared wq. A dedicated wq that is not 'kernel' type will configure pasid and
+	 * In the event that the WQ is configurable for pasid, the driver
+	 * should setup the pasid, pasid_en bit. This is true for both kernel
+	 * and user shared workqueues. There is no need to setup priv bit in
+	 * that in-kernel DMA will also do user privileged requests.
+	 * A dedicated wq that is not 'kernel' type will configure pasid and
  	 * pasid_en later on so there is no need to setup.
  	 */
  	if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags)) {
-		int priv = 0;
-
  		if (wq_pasid_enabled(wq)) {
  			if (is_idxd_wq_kernel(wq) || wq_shared(wq)) {
  				u32 pasid = wq_dedicated(wq) ? idxd->pasid : 0;

@@ -1340,10 +1324,6 @@ int drv_enable_wq(struct idxd_wq *wq)
  				__idxd_wq_set_pasid_locked(wq, pasid);
  			}
  		}
-
-		if (is_idxd_wq_kernel(wq))
-			priv = 1;
-		__idxd_wq_set_priv_locked(wq, priv);
  	}
 
  	rc = 0;

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

index e6ee267da0ff..a3396e1b38f1 100644
@@ -506,14 +506,56 @@ static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d
 
  static int idxd_enable_system_pasid(struct idxd_device *idxd)
  {
-	return -EOPNOTSUPP;
+	struct pci_dev *pdev = idxd->pdev;
+	struct device *dev = &pdev->dev;
+	struct iommu_domain *domain;
+	union gencfg_reg gencfg;
+	ioasid_t pasid;
+	int ret;
+
+	/*
+	 * Attach a global PASID to the DMA domain so that we can use ENQCMDS
+	 * to submit work on buffers mapped by DMA API.
+	 */
+	domain = iommu_get_dma_domain(dev);
+	if (!domain)
+		return -EPERM;
+
+	pasid = iommu_sva_reserve_pasid(1, dev->iommu->max_pasids);
+	if (pasid == IOMMU_PASID_INVALID)
+		return -ENOSPC;
+
+	ret = iommu_attach_device_pasid(domain, dev, pasid);
+	if (ret) {
+		dev_err(dev, "failed to attach device pasid %d, domain type %d",
+			pasid, domain->type);
+		iommu_sva_release_pasid(pasid);
+		return ret;
+	}
+
+	/* Since we set user privilege for kernel DMA, enable completion IRQ */
+	gencfg.bits = ioread32(idxd->reg_base + IDXD_GENCFG_OFFSET);
+	gencfg.user_int_en = 1;
+	iowrite32(gencfg.bits, idxd->reg_base + IDXD_GENCFG_OFFSET);
+	idxd->pasid = pasid;
+
+	return ret;
  }
 
  static void idxd_disable_system_pasid(struct idxd_device *idxd)
  {
+	struct pci_dev *pdev = idxd->pdev;

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

  	idxd->sva = NULL;
+	idxd->pasid = IOMMU_PASID_INVALID;
  }
 
  static int idxd_probe(struct idxd_device *idxd)

@@ -535,8 +577,9 @@ static int idxd_probe(struct idxd_device *idxd)
  		} else {
  			set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
 
-			if (idxd_enable_system_pasid(idxd))
-				dev_warn(dev, "No in-kernel DMA with PASID.\n");
+			rc = idxd_enable_system_pasid(idxd);
+			if (rc)
+				dev_warn(dev, "No in-kernel DMA with PASID. %d\n", rc);
  			else
  				set_bit(IDXD_FLAG_PASID_ENABLED, &idxd->flags);
  		}

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

index 18cd8151dee0..c5561c00a503 100644
@@ -944,13 +944,6 @@ static ssize_t wq_name_store(struct device *dev,
  	if (strlen(buf) > WQ_NAME_SIZE || strlen(buf) == 0)
  		return -EINVAL;
 

Hi, Jacob,

Quoted 2 lines · Jacob Pan

Kernel workqueues were disabled due to flawed use of kernel VA and SVA API. Now That we have the support for attaching PASID to the device's

s/That/that/

Quoted 142 lines

default domain and the ability to reserve global PASIDs from SVA APIs, we can re-enable the kernel work queues and use them under DMA API.

We also use non-privileged access for in-kernel DMA to be consistent with the IOMMU settings. Consequently, interrupt for user privilege is enabled for work completion IRQs.

Link https://lore.kernel.org/linux-iommu/[email protected]/ open here

Reviewed-by Dave Jiang <[email protected]>

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

---
  drivers/dma/idxd/device.c | 30 ++++-------------------
  drivers/dma/idxd/init.c   | 51 ++++++++++++++++++++++++++++++++++++---
  drivers/dma/idxd/sysfs.c  |  7 ------
  3 files changed, 52 insertions(+), 36 deletions(-)

drivers/dma/idxd/device.c +0 -15

index 6fca8fa8d3a8..f6b133d61a04 100644
@@ -299,21 +299,6 @@ void idxd_wqs_unmap_portal(struct idxd_device
 *idxd) }
  }
 
-static void __idxd_wq_set_priv_locked(struct idxd_wq *wq, int priv)
-{
-	struct idxd_device *idxd = wq->idxd;
-	union wqcfg wqcfg;
-	unsigned int offset;
-
-	offset = WQCFG_OFFSET(idxd, wq->id, WQCFG_PRIVL_IDX);
-	spin_lock(&idxd->dev_lock);
-	wqcfg.bits[WQCFG_PRIVL_IDX] = ioread32(idxd->reg_base +
 offset);
-	wqcfg.priv = priv;
-	wq->wqcfg->bits[WQCFG_PRIVL_IDX] = wqcfg.bits[WQCFG_PRIVL_IDX];
-	iowrite32(wqcfg.bits[WQCFG_PRIVL_IDX], idxd->reg_base +
 offset);
-	spin_unlock(&idxd->dev_lock);
-}
-
  static void __idxd_wq_set_pasid_locked(struct idxd_wq *wq, int pasid)

  {
  	struct idxd_device *idxd = wq->idxd;

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

@@ -1324,15 +1309,14 @@ int drv_enable_wq(struct idxd_wq *wq)
  	}
 
  	/*
-	 * In the event that the WQ is configurable for pasid and priv

bits.

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

-	 * For kernel wq, the driver should setup the pasid, pasid_en,

and priv bit.

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

-	 * However, for non-kernel wq, the driver should only set the

pasid_en bit for

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

-	 * shared wq. A dedicated wq that is not 'kernel' type will

configure pasid and

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

+	 * In the event that the WQ is configurable for pasid, the

driver

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

+	 * should setup the pasid, pasid_en bit. This is true for both

kernel

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

+	 * and user shared workqueues. There is no need to setup priv

bit in

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

+	 * that in-kernel DMA will also do user privileged requests.
+	 * A dedicated wq that is not 'kernel' type will configure

pasid and
* pasid_en later on so there is no need to setup. */
if (test_bit(IDXD_FLAG_CONFIGURABLE, &idxd->flags)) {

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

-		int priv = 0;
-
  		if (wq_pasid_enabled(wq)) {
  			if (is_idxd_wq_kernel(wq) || wq_shared(wq)) {
  				u32 pasid = wq_dedicated(wq) ?

idxd->pasid : 0; @@ -1340,10 +1324,6 @@ int drv_enable_wq(struct idxd_wq *wq) __idxd_wq_set_pasid_locked(wq, pasid);
}
}

drivers/dma/idxd/device.c +0 -4

-
-		if (is_idxd_wq_kernel(wq))
-			priv = 1;
-		__idxd_wq_set_priv_locked(wq, priv);
  	}
 
  	rc = 0;

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

index e6ee267da0ff..a3396e1b38f1 100644
@@ -506,14 +506,56 @@ static struct idxd_device *idxd_alloc(struct

pci_dev *pdev, struct idxd_driver_d
static int idxd_enable_system_pasid(struct idxd_device *idxd) {

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

-	return -EOPNOTSUPP;
+	struct pci_dev *pdev = idxd->pdev;
+	struct device *dev = &pdev->dev;
+	struct iommu_domain *domain;
+	union gencfg_reg gencfg;
+	ioasid_t pasid;
+	int ret;
+
+	/*
+	 * Attach a global PASID to the DMA domain so that we can use

ENQCMDS

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

+	 * to submit work on buffers mapped by DMA API.
+	 */
+	domain = iommu_get_dma_domain(dev);
+	if (!domain)
+		return -EPERM;
+
+	pasid = iommu_sva_reserve_pasid(1, dev->iommu->max_pasids);
+	if (pasid == IOMMU_PASID_INVALID)
+		return -ENOSPC;
+
+	ret = iommu_attach_device_pasid(domain, dev, pasid);
+	if (ret) {
+		dev_err(dev, "failed to attach device pasid %d, domain

type %d",

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

+			pasid, domain->type);
+		iommu_sva_release_pasid(pasid);
+		return ret;
+	}
+
+	/* Since we set user privilege for kernel DMA, enable

completion IRQ */

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

+	gencfg.bits = ioread32(idxd->reg_base + IDXD_GENCFG_OFFSET);
+	gencfg.user_int_en = 1;
+	iowrite32(gencfg.bits, idxd->reg_base + IDXD_GENCFG_OFFSET);
+	idxd->pasid = pasid;
+
+	return ret;
  }
 
  static void idxd_disable_system_pasid(struct idxd_device *idxd)
  {
+	struct pci_dev *pdev = idxd->pdev;
+	struct device *dev = &pdev->dev;
+	struct iommu_domain *domain;
+
+	domain = iommu_get_domain_for_dev(dev);
+	if (!domain || domain->type == IOMMU_DOMAIN_BLOCKED)
+		return;

drivers/dma/idxd/init.c static struct idxd_device *idxd_alloc(struct pci_dev *pdev, struct idxd_driver_d · line 556

Quoted 3 lines

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

-	iommu_sva_unbind_device(idxd->sva);
+	iommu_detach_device_pasid(domain, dev, idxd->pasid);
+	iommu_sva_release_pasid(idxd->pasid);  

May need gencfg.user_int_en = 0 here.