dmaengine: idxd: Add enable/disable device IOPF feature

From: Lu Baolu <[email protected]>
Sent: Friday, February 3, 2023 4:45 PM

Generally enabling IOMMU_DEV_FEAT_SVA requires IOMMU_DEV_FEAT_IOPF, but
some devices manage I/O Page Faults themselves instead of relying on the IOMMU. Move IOPF related code from SVA to IOPF enabling path to make the
driver work for devices that manage IOPF themselves.

For the device drivers that relies on the IOMMU for IOPF through PCI/PRI, IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after IOMMU_DEV_FEAT_SVA.

Quoted 10 lines

From: Lu Baolu <[email protected]>
Sent: Friday, February 3, 2023 4:45 PM

Generally enabling IOMMU_DEV_FEAT_SVA requires IOMMU_DEV_FEAT_IOPF, but
some devices manage I/O Page Faults themselves instead of relying on the IOMMU. Move IOPF related code from SVA to IOPF enabling path to make the
driver work for devices that manage IOPF themselves.

Quoted 3 lines

For the device drivers that relies on the IOMMU for IOPF through PCI/PRI, IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after IOMMU_DEV_FEAT_SVA.

ARM still handles this differently:

arm_smmu_master_enable_sva()
arm_smmu_master_sva_enable_iopf():

{
	/*
	 * Drivers for devices supporting PRI or stall should enable IOPF first.
	 * Others have device-specific fault handlers and don't need IOPF.
	 */
	if (!arm_smmu_master_iopf_supported(master))
		return 0;

	if (!master->iopf_enabled)
		return -EINVAL;
}

i.e. device specific IOPF is allowed only when PRI or stall is not supported.

it's different from what this patch does to allow device specific IOPF even when PRI is supported.

should we make them consistent given SVA/IOPF capabilities are general iommu definitions or fine to leave each iommu driver with different restriction?

drivers/iommu/intel/iommu.c static int intel_iommu_enable_sva(struct device *dev) · line 4650

Quoted 10 lines

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

-	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
-	if (!ret)
-		ret = iommu_register_device_fault_handler(dev,

iommu_queue_iopf, dev);

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

-
-	return ret;
+	return 0;
  }

here and below...

drivers/iommu/intel/iommu.c static int intel_iommu_disable_sva(struct device *dev) · line 4682

Quoted 11 lines

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

+	ret = iopf_queue_add_device(info->iommu->iopf_queue, dev);
+	if (ret)
+		return ret;
+
+	ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,

dev);

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

+	if (ret)
+		iopf_queue_remove_device(info->iommu->iopf_queue, dev);
+
+	return ret;
  }

...indicate a bug fix on error handling. better to have the fix as a separate patch and then move code.

The iommu subsystem requires IOMMU_DEV_FEAT_IOPF must be enabled before and disabled after IOMMU_DEV_FEAT_SVA, if device's I/O page faults rely on the IOMMU. Add explicit IOMMU_DEV_FEAT_IOPF enabling/disabling in this driver.

At present, missing IOPF enabling/disabling doesn't cause any real issue, because the IOMMU driver places the IOPF enabling/disabling in the path of SVA feature handling. But this may change.

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

---
  drivers/dma/idxd/init.c | 31 +++++++++++++++++++++++++------
  1 file changed, 25 insertions(+), 6 deletions(-)

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

index 529ea09c9094..d5a709a842a8 100644
@@ -511,6 +511,27 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
  	idxd->sva = NULL;
  }
 
+static int idxd_enable_sva(struct pci_dev *pdev)
+{
+	int ret;
+
+	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
+	if (ret)
+		return ret;
+
+	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
+	if (ret)
+		iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
+
+	return ret;
+}
+
+static void idxd_disable_sva(struct pci_dev *pdev)
+{
+	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
+	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
+}
+
  static int idxd_probe(struct idxd_device *idxd)
  {
  	struct pci_dev *pdev = idxd->pdev;

@@ -525,7 +546,7 @@ static int idxd_probe(struct idxd_device *idxd)
  	dev_dbg(dev, "IDXD reset complete\n");
 
  	if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && sva) {
-		if (iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA)) {
+		if (idxd_enable_sva(pdev)) {
  			dev_warn(dev, "Unable to turn on user SVA feature.\n");
  		} else {
  			set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);

@@ -573,21 +594,19 @@ static int idxd_probe(struct idxd_device *idxd)
  	if (device_pasid_enabled(idxd))
  		idxd_disable_system_pasid(idxd);
  	if (device_user_pasid_enabled(idxd))
-		iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+		idxd_disable_sva(pdev);
  	return rc;
  }
 
  static void idxd_cleanup(struct idxd_device *idxd)
  {
-	struct device *dev = &idxd->pdev->dev;
-
  	perfmon_pmu_remove(idxd);
  	idxd_cleanup_interrupts(idxd);
  	idxd_cleanup_internals(idxd);
  	if (device_pasid_enabled(idxd))
  		idxd_disable_system_pasid(idxd);
  	if (device_user_pasid_enabled(idxd))
-		iommu_dev_disable_feature(dev, IOMMU_DEV_FEAT_SVA);
+		idxd_disable_sva(idxd->pdev);
  }
 
  static int idxd_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)

@@ -705,7 +724,7 @@ static void idxd_remove(struct pci_dev *pdev)
  	pci_free_irq_vectors(pdev);
  	pci_iounmap(pdev, idxd->reg_base);
  	if (device_user_pasid_enabled(idxd))
-		iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
+		idxd_disable_sva(pdev);
  	pci_disable_device(pdev);
  	destroy_workqueue(idxd->wq);
  	perfmon_pmu_remove(idxd);