Re-enable IDXD kernel workqueue under DMA API

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

PCIe Process address space ID (PASID) is used to tag DMA traffic, it provides finer grained isolation than requester ID (RID).

For each RID, 0 is as a special PASID for the legacy DMA (without PASID), thus RID_PASID. This is universal across all architectures, therefore warranted to be declared in the common header. Noting that VT-d could support none-zero RID_PASID, but currently not used.

By having a common RID_PASID, we can avoid conflicts between different use cases in the generic code. e.g. SVA and DMA API with PASIDs.

Quoted 12 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

PCIe Process address space ID (PASID) is used to tag DMA traffic, it provides finer grained isolation than requester ID (RID).

For each RID, 0 is as a special PASID for the legacy DMA (without PASID), thus RID_PASID. This is universal across all architectures, therefore warranted to be declared in the common header. Noting that VT-d could support none-zero RID_PASID, but currently not used.

Quoted 2 lines

By having a common RID_PASID, we can avoid conflicts between different use cases in the generic code. e.g. SVA and DMA API with PASIDs.

You intend it to be generic but in the end only vt-d driver is changed to use it in this series...

include/linux/iommu.h enum iommu_dev_features { · line 194

Quoted 6 lines

include/linux/iommu.h +1 -0

@@ -190,6 +190,7 @@ enum iommu_dev_features {
 	IOMMU_DEV_FEAT_IOPF,
 };
 
+#define IOMMU_DEF_RID_PASID	(0U) /* Reserved for DMA w/o PASID
 */  

Is RID a general team on other platform?

Would IOMMU_DEF_NO_PASID serve the purpose better?

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 11:56 PM

Hi Kevin,

Quoted 12 lines · Kevin

Quoted 12 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

PCIe Process address space ID (PASID) is used to tag DMA traffic, it provides finer grained isolation than requester ID (RID).

For each RID, 0 is as a special PASID for the legacy DMA (without PASID), thus RID_PASID. This is universal across all architectures, therefore warranted to be declared in the common header. Noting that VT-d could support none-zero RID_PASID, but currently not used.

Quoted 5 lines

Quoted 2 lines

By having a common RID_PASID, we can avoid conflicts between different use cases in the generic code. e.g. SVA and DMA API with PASIDs.

You intend it to be generic but in the end only vt-d driver is changed to use it in this series...

change for SVA is in the patch.

Quoted 9 lines

include/linux/iommu.h enum iommu_dev_features { · line 194

Quoted 7 lines

include/linux/iommu.h +1 -0

@@ -190,6 +190,7 @@ enum iommu_dev_features {
 	IOMMU_DEV_FEAT_IOPF,
 };
 
+#define IOMMU_DEF_RID_PASID	(0U) /* Reserved for DMA w/o PASID
 */

Is RID a general team on other platform?

RID, aka requester id is a PCI term. so I this it is common though on SMMU might be better called stream ID PASID?

Quoted 1 line

Would IOMMU_DEF_NO_PASID serve the purpose better?

not sure, it is still a PASID. For completeness, might be called IOMMU_DEF_PASID_FOR_DMA_NO_PASID :)

other suggestions?

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

By having a common RID_PASID, we can avoid conflicts between different use cases in the generic code. e.g. SVA and DMA API with PASIDs.

You intend it to be generic but in the end only vt-d driver is changed to use it in this series...

change for SVA is in the patch.

My point was that since it is common why there is no change in arm-smmu driver to use the common macro?

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

SVA PASID allocation is hardcoded to start from 1 because 0 is used for RID_PASID, let's make it explicit to avoid the potential conflicts.

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

---
 drivers/iommu/iommu-sva.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index c434b95dc8eb..ac7c93bacb5c 100644
@@ -66,7 +66,7 @@ struct iommu_sva *iommu_sva_bind_device(struct

device *dev, struct mm_struct *mm

drivers/iommu/iommu-sva.c +1 -1

 		return ERR_PTR(-EOPNOTSUPP);
 
 	/* Allocate mm->pasid if necessary. */
-	ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
+	ret = iommu_sva_alloc_pasid(mm, IOMMU_DEF_RID_PASID + 1,

max_pasids - 1);

Quoted 19 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

SVA PASID allocation is hardcoded to start from 1 because 0 is used for RID_PASID, let's make it explicit to avoid the potential conflicts.

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

---
 drivers/iommu/iommu-sva.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index c434b95dc8eb..ac7c93bacb5c 100644
@@ -66,7 +66,7 @@ struct iommu_sva *iommu_sva_bind_device(struct

device *dev, struct mm_struct *mm
return ERR_PTR(-EOPNOTSUPP);

Quoted 4 lines

drivers/iommu/iommu-sva.c +1 -1

 	/* Allocate mm->pasid if necessary. */
-	ret = iommu_sva_alloc_pasid(mm, 1, max_pasids - 1);
+	ret = iommu_sva_alloc_pasid(mm, IOMMU_DEF_RID_PASID + 1,

max_pasids - 1);

To be future proof it's probably cleaner to define a IOMMU_MAX_RSVD_PASID in case there may be more reserved pasids in future usages?

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

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.

/* Allocate a PASID for the mm within range (inclusive) */
-static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)

drivers/iommu/iommu-sva.c +1 -1

+static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev)
 {
 	int ret = 0;
 
-	if (!pasid_valid(min) || !pasid_valid(max) ||

Quoted 7 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

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.

well the device itself cannot submit work to itself. It's software to submit work to the device. 😊

Quoted 9 lines

that the PASID are stored in a per CPU MSR

/* Allocate a PASID for the mm within range (inclusive) */
-static int iommu_sva_alloc_pasid(struct mm_struct *mm, ioasid_t min, ioasid_t max)
+static int iommu_sva_alloc_pasid(struct mm_struct *mm, struct device *dev) {
int ret = 0;

drivers/iommu/iommu-sva.c line 21

Quoted 11 lines

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

-	if (!pasid_valid(min) || !pasid_valid(max) ||
-	    min == 0 || max < min)
-		return -EINVAL;
-
 	mutex_lock(&iommu_sva_lock);
 	/* Is a PASID already associated with this mm? */
-	if (pasid_valid(mm->pasid)) {
-		if (mm->pasid < min || mm->pasid > max)
-			ret = -EOVERFLOW;
+	if (pasid_valid(mm->pasid))
 		goto out;  

emmm here you still want to check whether mm->pasid exceeds the max pasid width of the bound device.

drivers/iommu/iommu.c struct iommu_domain *iommu_sva_domain_alloc(struct device *dev, · line 3460

Quoted 6 lines · Jacob Pan

drivers/iommu/iommu.c +6 -0

+ioasid_t iommu_alloc_global_pasid_dev(struct device *dev)
+{
+	int ret;
+	ioasid_t max;
+
+	max = dev_iommu_get_max_pasids(dev);  

Perhaps you can use dev->iommu->max_pasids. It's static, so no need to recalculate it.

Quoted 4 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

drivers/iommu/intel/iommu.c static int __init si_domain_init(int hw) · line 2440

Quoted 9 lines

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

+static int dmar_domain_attach_device_pasid(struct dmar_domain *domain,
+					   struct intel_iommu *iommu,
+					   struct device *dev, ioasid_t pasid)
+{
+	int ret;
+
+	/* PASID table is mandatory for a PCI device in scalable mode. */
+	if (!sm_supported(iommu) && dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;

"&&" should be "||"

Quoted 4 lines · Kevin

Quoted 4 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

Quoted 13 lines

drivers/iommu/intel/iommu.c static int __init si_domain_init(int hw) · line 2440

Quoted 10 lines

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

+static int dmar_domain_attach_device_pasid(struct dmar_domain *domain,
+					   struct intel_iommu *iommu,
+					   struct device *dev,

ioasid_t pasid) +{

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

+	int ret;
+
+	/* PASID table is mandatory for a PCI device in scalable

mode. */

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

+	if (!sm_supported(iommu) && dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;  

"&&" should be "||"

Also should return success instead if this is a RID case. Perhaps,

	if (!sm_supported(iommu) || dev_is_real_dma_subdevice(dev))
		return pasid == RID2PASID ? 0 : -EOPNOTSUPP;

Quoted 4 lines

From: Jacob Pan <[email protected]>
Sent: Friday, April 28, 2023 1:50 AM

drivers/iommu/intel/iommu.c static int __init si_domain_init(int hw) · line 2440

Quoted 10 lines

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

+static int dmar_domain_attach_device_pasid(struct dmar_domain *domain,
+					   struct intel_iommu *iommu,
+					   struct device *dev,

ioasid_t pasid) +{

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

+	int ret;
+
+	/* PASID table is mandatory for a PCI device in scalable mode.

*/

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

+	if (!sm_supported(iommu) && dev_is_real_dma_subdevice(dev))
+		return -EOPNOTSUPP;  

"&&" should be "||"

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

@@ -2433,12 +2477,17 @@ static int dmar_domain_attach_device_pasid(struct dmar_domain *domain,
  					   struct intel_iommu *iommu,
  					   struct device *dev, ioasid_t pasid)
  {
+	struct device_pasid_info *dev_pasid;
+	unsigned long flags;
  	int ret;
 
-	/* PASID table is mandatory for a PCI device in scalable mode. */
  	if (!sm_supported(iommu) && dev_is_real_dma_subdevice(dev))
  		return -EOPNOTSUPP;
 
+	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+	if (!dev_pasid)
+		return -ENOMEM;
+
  	if (hw_pass_through && domain_type_is_si(domain))
  		ret = intel_pasid_setup_pass_through(iommu, domain, dev, pasid);
  	else if (domain->use_first_level)

@@ -2446,6 +2495,17 @@ static int dmar_domain_attach_device_pasid(struct dmar_domain *domain,
  	else
  		ret = intel_pasid_setup_second_level(iommu, domain, dev, pasid);
 
+	if (ret) {
+		kfree(dev_pasid);
+		return ret;
+	}
+
+	dev_pasid->pasid = pasid;
+	dev_pasid->dev = dev;
+	spin_lock_irqsave(&domain->lock, flags);
+	list_add(&dev_pasid->link_domain, &domain->dev_pasids);
+	spin_unlock_irqrestore(&domain->lock, flags);
+
  	return 0;
  }
 

@@ -2467,16 +2527,13 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
  		return ret;
  	info->domain = domain;
  	spin_lock_irqsave(&domain->lock, flags);
+	if (info->dev_attached) {
+		spin_unlock_irqrestore(&domain->lock, flags);
+		return 0;
+	}
  	list_add(&info->link, &domain->devices);
  	spin_unlock_irqrestore(&domain->lock, flags);
 
-	ret = dmar_domain_attach_device_pasid(domain, iommu, dev,
-					      IOMMU_DEF_RID_PASID);
-	if (ret) {
-		dev_err(dev, "Setup RID2PASID failed\n");
-		device_block_translation(dev);
-	}
-
  	ret = domain_context_mapping(domain, dev);
  	if (ret) {
  		dev_err(dev, "Domain context map failed\n");

@@ -2485,8 +2542,9 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
  	}
 
  	iommu_enable_pci_caps(info);
+	info->dev_attached = 1;
 
-	return 0;
+	return ret;
  }
 
  static bool device_has_rmrr(struct device *dev)

@@ -4044,6 +4102,7 @@ static void device_block_translation(struct device *dev)
 
  	spin_lock_irqsave(&info->domain->lock, flags);
  	list_del(&info->link);
+	info->dev_attached = 0;
  	spin_unlock_irqrestore(&info->domain->lock, flags);
 
  	domain_detach_iommu(info->domain, iommu);

@@ -4175,8 +4234,15 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
  				     struct device *dev)
  {
  	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu;
+	u8 bus, devfn;
  	int ret;
 
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+
  	if (domain->type == IOMMU_DOMAIN_UNMANAGED &&
  	    device_is_rmrr_locked(dev)) {
  		dev_warn(dev, "Device is ineligible for IOMMU domain attach due to platform RMRR requirement.  Contact your platform vendor.\n");

@@ -4190,7 +4256,23 @@ static int intel_iommu_attach_device(struct iommu_domain *domain,
  	if (ret)
  		return ret;
 
-	return dmar_domain_attach_device(to_dmar_domain(domain), dev);
+	ret = dmar_domain_attach_device(to_dmar_domain(domain), dev);
+	if (ret) {
+		dev_err(dev, "Attach device failed\n");
+		return ret;
+	}
+
+	/* PASID table is mandatory for a PCI device in scalable mode. */
+	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
+		/* Setup the PASID entry for requests without PASID: */
+		ret = dmar_domain_attach_device_pasid(dmar_domain, iommu, dev,

Quoted 120 lines · Jacob Pan

(quoted diff) +0 -0

@@ -2433,12 +2477,17 @@ static int

dmar_domain_attach_device_pasid(struct dmar_domain *domain, struct intel_iommu *iommu, struct device *dev, ioasid_t pasid)
{

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

+	struct device_pasid_info *dev_pasid;
+	unsigned long flags;
  	int ret;
 
-	/* PASID table is mandatory for a PCI device in scalable mode.

*/ if (!sm_supported(iommu) && dev_is_real_dma_subdevice(dev))
return -EOPNOTSUPP;

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

+	dev_pasid = kzalloc(sizeof(*dev_pasid), GFP_KERNEL);
+	if (!dev_pasid)
+		return -ENOMEM;
+
  	if (hw_pass_through && domain_type_is_si(domain))
  		ret = intel_pasid_setup_pass_through(iommu, domain,

dev, pasid); else if (domain->use_first_level)

(quoted diff) +0 -0

@@ -2446,6 +2495,17 @@ static int

dmar_domain_attach_device_pasid(struct dmar_domain *domain, else
ret = intel_pasid_setup_second_level(iommu, domain,
dev, pasid);

drivers/iommu/intel/iommu.c +25 -8

+	if (ret) {
+		kfree(dev_pasid);
+		return ret;
+	}
+
+	dev_pasid->pasid = pasid;
+	dev_pasid->dev = dev;
+	spin_lock_irqsave(&domain->lock, flags);
+	list_add(&dev_pasid->link_domain, &domain->dev_pasids);
+	spin_unlock_irqrestore(&domain->lock, flags);
+
  	return 0;
  }

@@ -2467,16 +2527,13 @@ static int dmar_domain_attach_device(struct
 dmar_domain *domain, return ret;
  	info->domain = domain;
  	spin_lock_irqsave(&domain->lock, flags);
+	if (info->dev_attached) {
+		spin_unlock_irqrestore(&domain->lock, flags);
+		return 0;
+	}
  	list_add(&info->link, &domain->devices);
  	spin_unlock_irqrestore(&domain->lock, flags);
 
-	ret = dmar_domain_attach_device_pasid(domain, iommu, dev,
-					      IOMMU_DEF_RID_PASID);
-	if (ret) {
-		dev_err(dev, "Setup RID2PASID failed\n");
-		device_block_translation(dev);
-	}
-
  	ret = domain_context_mapping(domain, dev);
  	if (ret) {
  		dev_err(dev, "Domain context map failed\n");

@@ -2485,8 +2542,9 @@ static int dmar_domain_attach_device(struct
 dmar_domain *domain, }
 
  	iommu_enable_pci_caps(info);
+	info->dev_attached = 1;
 
-	return 0;
+	return ret;
  }
 
  static bool device_has_rmrr(struct device *dev)

@@ -4044,6 +4102,7 @@ static void device_block_translation(struct
 device *dev) 
  	spin_lock_irqsave(&info->domain->lock, flags);
  	list_del(&info->link);
+	info->dev_attached = 0;
  	spin_unlock_irqrestore(&info->domain->lock, flags);
 
  	domain_detach_iommu(info->domain, iommu);

@@ -4175,8 +4234,15 @@ static int intel_iommu_attach_device(struct
 iommu_domain *domain, struct device *dev)
  {
  	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu;
+	u8 bus, devfn;
  	int ret;
 
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+
  	if (domain->type == IOMMU_DOMAIN_UNMANAGED &&
  	    device_is_rmrr_locked(dev)) {
  		dev_warn(dev, "Device is ineligible for IOMMU domain

attach due to platform RMRR requirement. Contact your platform vendor.\n"); @@ -4190,7 +4256,23 @@ static int intel_iommu_attach_device(struct iommu_domain *domain, if (ret) return ret;

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

-	return dmar_domain_attach_device(to_dmar_domain(domain), dev);
+	ret = dmar_domain_attach_device(to_dmar_domain(domain), dev);
+	if (ret) {
+		dev_err(dev, "Attach device failed\n");
+		return ret;
+	}
+
+	/* PASID table is mandatory for a PCI device in scalable mode.

*/

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

+	if (sm_supported(iommu) && !dev_is_real_dma_subdevice(dev)) {
+		/* Setup the PASID entry for requests without PASID: */
+		ret = dmar_domain_attach_device_pasid(dmar_domain,

iommu, dev,
+
IOMMU_DEF_RID_PASID);

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

+		if (ret) {
+			dev_err(dev, "Setup RID2PASID failed\n");
+			device_block_translation(dev);
+		}
+	}
+	return ret;
  }

Quoted 1 line

static int intel_iommu_map(struct iommu_domain *domain,

I am not following why do you need to change the attach_device path in this patch. Perhaps you want to make sure that context entry for the device is configured before attach_device_pasid?

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.

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.

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

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

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

index 388453a7415e..f9d6c31cdc8e 100644
@@ -278,6 +278,8 @@ static LIST_HEAD(dmar_satc_units);
  	list_for_each_entry(rmrr, &dmar_rmrr_units, list)
 
  static void device_block_translation(struct device *dev);
+static void intel_iommu_detach_device_pasid(struct iommu_domain *domain,
+					    struct device *dev, ioasid_t pasid);
  static void intel_iommu_domain_free(struct iommu_domain *domain);
 
  int dmar_disabled = !IS_ENABLED(CONFIG_INTEL_IOMMU_DEFAULT_ON);

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

+static int intel_iommu_attach_device_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;
+	int ret;
+
+	if (!pasid_supported(iommu))

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

+	if (ret) {
+		dev_err(dev, "Attach device failed\n");
+		return ret;
+	}
+	return dmar_domain_attach_device_pasid(dmar_domain, iommu, dev, pasid);
+}
+
+
+
  const struct iommu_ops intel_iommu_ops = {
  	.capable		= intel_iommu_capable,
  	.domain_alloc		= intel_iommu_domain_alloc,

Quoted 28 lines · Jacob Pan

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.

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.

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

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

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

index 388453a7415e..f9d6c31cdc8e 100644
@@ -278,6 +278,8 @@ static LIST_HEAD(dmar_satc_units);
  	list_for_each_entry(rmrr, &dmar_rmrr_units, list)
 
  static void device_block_translation(struct device *dev);
+static void intel_iommu_detach_device_pasid(struct iommu_domain
 *domain,
+					    struct device *dev,

ioasid_t pasid); static void intel_iommu_domain_free(struct iommu_domain *domain);
int dmar_disabled = !IS_ENABLED(CONFIG_INTEL_IOMMU_DEFAULT_ON);

drivers/iommu/intel/iommu.c static void device_block_translation(struct · line 4099

Quoted 9 lines

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

@@ -4091,8 +4093,7 @@ static void device_block_translation(struct
 device *dev) iommu_disable_pci_caps(info);
  	if (!dev_is_real_dma_subdevice(dev)) {
  		if (sm_supported(iommu))
-			intel_pasid_tear_down_entry(iommu, dev,
-
 IOMMU_DEF_RID_PASID, false);
+
 intel_iommu_detach_device_pasid(&info->domain->domain, dev,
 IOMMU_DEF_RID_PASID);  

device_block_translation() is called when switch RID's domain or release the device. I assume that we don't need to touch this path when we add the attach_dev_pasid support.

Blocking DMA translation through RID/PASID should be done in remove_dev_pasid path.

Or, I overlooked anything?

[...]

Quoted 17 lines

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

+static int intel_iommu_attach_device_pasid(struct iommu_domain *domain,
+					   struct device *dev,

ioasid_t pasid) +{

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

+	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;
+	int ret;
+
+	if (!pasid_supported(iommu))
+		return -ENODEV;
+
+	ret = prepare_domain_attach_device(domain, dev);
+	if (ret)
+		return ret;
+
+	/*

drivers/iommu/intel/iommu.c

Quoted 5 lines

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

+	 * Most likely the device context has already been set up,

will only

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

+	 * take a domain ID reference. Otherwise, device context will

be set

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

+	 * up here.  

The "otherwise" case is only default domain deferred attaching case, right?

When the device driver starts to call attach_dev_pasid api, it means that the bus and device DMA configuration have been done. We could do the deferred default domain attaching now. So, perhaps we should add below code in the core:

drivers/iommu/iommu.c +0 -0

index f1dcfa3f1a1b..633b5ca53606 100644

drivers/iommu/iommu.c +0 -0

         mutex_lock(&group->mutex);
         curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, 

GFP_KERNEL);
         if (curr) {

Perhaps need to call iommu_deferred_attach() inside the group->mutex critical region?

drivers/iommu/intel/iommu.c

Quoted 5 lines

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

+	 * The upper layer APIs make no assumption about the ordering

between

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

+	 * device attachment and the PASID attachment.
+	 */
+	ret = dmar_domain_attach_device(to_dmar_domain(domain), dev);  

Calling attach_device on the attach_dev_pasid path is not right.

Quoted 17 lines

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

+	if (ret) {
+		dev_err(dev, "Attach device failed\n");
+		return ret;
+	}
+	return dmar_domain_attach_device_pasid(dmar_domain, iommu,

dev, pasid); +}

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

+
+
+
  const struct iommu_ops intel_iommu_ops = {
  	.capable		= intel_iommu_capable,
  	.domain_alloc		= intel_iommu_domain_alloc,

@@ -4802,6 +4861,7 @@ const struct iommu_ops intel_iommu_ops = {
  #endif
  	.default_domain_ops = &(const struct iommu_domain_ops) {
  		.attach_dev		=
 intel_iommu_attach_device,

Quoted 5 lines

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

+		.set_dev_pasid          =

intel_iommu_attach_device_pasid, .map_pages =
intel_iommu_map_pages, .unmap_pages =
intel_iommu_unmap_pages, .iotlb_sync_map =
intel_iommu_iotlb_sync_map,

Best regards,
baolu

Quoted 15 lines

Quoted 15 lines

(quoted diff) +2 -0

+static int intel_iommu_attach_device_pasid(struct iommu_domain *domain,
+					   struct device *dev,

ioasid_t pasid) +{

(quoted diff) +12 -0

+	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;
+	int ret;
+
+	if (!pasid_supported(iommu))
+		return -ENODEV;
+
+	ret = prepare_domain_attach_device(domain, dev);
+	if (ret)
+		return ret;
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +2 -0

+	/*
+	 * Most likely the device context has already been set up,

will only

(quoted diff) +1 -0

+	 * take a domain ID reference. Otherwise, device context will

be set

(quoted diff) +1 -0

+	 * up here.

The "otherwise" case is only default domain deferred attaching case, right?

it might be the only case so far, but my intention is to be general. i.e. no ordering requirements. I believe it is more future proof in case device_attach_pasid called before device_attach.

Quoted 24 lines · Jacob Pan

Quoted 16 lines

Quoted 16 lines

+static int intel_iommu_attach_device_pasid(struct iommu_domain *domain,

(quoted diff) +1 -0

+					   struct device *dev,

ioasid_t pasid) +{

(quoted diff) +12 -0

+	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;
+	int ret;
+
+	if (!pasid_supported(iommu))
+		return -ENODEV;
+
+	ret = prepare_domain_attach_device(domain, dev);
+	if (ret)
+		return ret;
+

Quoted 8 lines

Review

Quoted 6 lines

(quoted diff) +2 -0

+	/*
+	 * Most likely the device context has already been set up,

will only

(quoted diff) +1 -0

+	 * take a domain ID reference. Otherwise, device context will

be set

(quoted diff) +1 -0

+	 * up here.  

The "otherwise" case is only default domain deferred attaching case, right?

Quoted 3 lines

it might be the only case so far, but my intention is to be general. i.e. no ordering requirements. I believe it is more future proof in case device_attach_pasid called before device_attach.

Let's put aside deferred attach and talk about it later.

The device's context entry is configured when the default domain is being attached to the device. And, the default domain attaching is in the iommu probe_device path. It always happens before set_device_pasid which is designed to be called by the device driver. So the real situation is that when the device driver calls set_device_pasid, the context entry should already have been configured.

Then let's pick up the deferred attach case. It is a workaround for kdump (Documentation/admin-guide/kdump/kdump.rst). I don't think PASID feature is functionally required by any kdump capture kernel as its main purpose is to dump the memory of a panic kernel.

In summary, it seems to be reasonable for the vt-d driver to return -EBUSY when a device's context was copied. The idxd driver should continue to work without kernel DMA with PASID support.

	if (context_copied(iommu, bus, devfn))
		return -EBUSY;

Make things general is always good, but this doesn't mean that we need to make the code complex to support cases that do not exist or are not used. Thoughts?