[PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Lu Baolu]

The new method for driver fault reporting support relies on the domain
to specify a iopf_handler. The driver should detect this and setup the
HW when fault capable domains are attached.

Move SMMUv3 to use this method and have VT-D validate support during
attach so that all three fault capable drivers have a no-op FEAT_SVA and
_IOPF. Then remove them.

This was initiated by Jason. I'm following up to remove FEAT_IOPF and
further clean up.

The whole series is also available at github:
https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Jason Gunthorpe (3):
  iommu/arm-smmu-v3: Put iopf enablement in the domain attach path
  iommu/vt-d: Check if SVA is supported when attaching the SVA domain
  iommu: Remove IOMMU_DEV_FEAT_SVA

Lu Baolu (9):
  iommu/vt-d: Move scalable mode ATS enablement to probe path
  iommu/vt-d: Move PRI enablement in probe path
  iommu/vt-d: Cleanup intel_context_flush_present()
  iommu/vt-d: Put iopf enablement in domain attach path
  iommufd/selftest: Put iopf enablement in domain attach path
  dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF
  uacce: Remove unnecessary IOMMU_DEV_FEAT_IOPF
  iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF
  iommu: Remove iommu_dev_enable/disable_feature()

 drivers/accel/amdxdna/aie2_pci.c              |  13 +-
 drivers/dma/idxd/init.c                       |  43 +--
 drivers/iommu/amd/iommu.c                     |  34 --
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  86 +----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 130 ++++----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  34 +-
 drivers/iommu/intel/iommu.c                   | 301 ++++++------------
 drivers/iommu/intel/iommu.h                   |  50 ++-
 drivers/iommu/intel/nested.c                  |  16 +-
 drivers/iommu/intel/pasid.c                   |  41 +--
 drivers/iommu/intel/prq.c                     |   2 +-
 drivers/iommu/intel/svm.c                     |  52 ++-
 drivers/iommu/iommu-sva.c                     |   3 -
 drivers/iommu/iommu.c                         |  32 --
 drivers/iommu/iommufd/device.c                |   1 -
 drivers/iommu/iommufd/fault.c                 | 111 ++-----
 drivers/iommu/iommufd/iommufd_private.h       |   3 -
 drivers/iommu/iommufd/selftest.c              |  52 ++-
 drivers/misc/uacce/uacce.c                    |  40 ---
 include/linux/iommu.h                         |  35 --
 20 files changed, 380 insertions(+), 699 deletions(-)

-- 
2.43.0

[PATCH 01/12] iommu/arm-smmu-v3: Put iopf enablement in the domain attach path

[Lu Baolu]

From: Jason Gunthorpe <jgg@nvidia.com>

SMMUv3 co-mingles FEAT_IOPF and FEAT_SVA behaviors so that fault reporting
doesn't work unless both are enabled. This is not correct and causes
problems for iommufd which does not enable FEAT_SVA for it's fault capable
domains.

These APIs are both obsolete, update SMMUv3 to use the new method like AMD
implements.

A driver should enable iopf support when a domain with an iopf_handler is
attached, and disable iopf support when the domain is removed.

Move the fault support logic to sva domain allocation and to domain
attach, refusing to create or attach fault capable domains if the HW
doesn't support it.

Move all the logic for controlling the iopf queue under
arm_smmu_attach_prepare(). Keep track of the number of domains on the
master (over all the SSIDs) that require iopf. When the first domain
requiring iopf is attached create the iopf queue, when the last domain is
detached destroy it.

Turn FEAT_IOPF and FEAT_SVA into no ops.

Remove the sva_lock, this is all protected by the group mutex.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 86 ++--------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 98 ++++++++++++++-----
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   | 34 +------
 3 files changed, 81 insertions(+), 137 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 9ba596430e7c..605d1dd0e1cc 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -13,8 +13,6 @@
 #include "arm-smmu-v3.h"
 #include "../../io-pgtable-arm.h"
 
-static DEFINE_MUTEX(sva_lock);
-
 static void __maybe_unused
 arm_smmu_update_s1_domain_cd_entry(struct arm_smmu_domain *smmu_domain)
 {
@@ -257,84 +255,6 @@ bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
 	return true;
 }
 
-bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master)
-{
-	/* We're not keeping track of SIDs in fault events */
-	if (master->num_streams != 1)
-		return false;
-
-	return master->stall_enabled;
-}
-
-bool arm_smmu_master_sva_supported(struct arm_smmu_master *master)
-{
-	if (!(master->smmu->features & ARM_SMMU_FEAT_SVA))
-		return false;
-
-	/* SSID support is mandatory for the moment */
-	return master->ssid_bits;
-}
-
-bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)
-{
-	bool enabled;
-
-	mutex_lock(&sva_lock);
-	enabled = master->sva_enabled;
-	mutex_unlock(&sva_lock);
-	return enabled;
-}
-
-static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
-{
-	struct device *dev = master->dev;
-
-	/*
-	 * 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;
-
-	return iopf_queue_add_device(master->smmu->evtq.iopf, dev);
-}
-
-static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
-{
-	struct device *dev = master->dev;
-
-	if (!master->iopf_enabled)
-		return;
-
-	iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
-}
-
-int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
-{
-	int ret;
-
-	mutex_lock(&sva_lock);
-	ret = arm_smmu_master_sva_enable_iopf(master);
-	if (!ret)
-		master->sva_enabled = true;
-	mutex_unlock(&sva_lock);
-
-	return ret;
-}
-
-int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
-{
-	mutex_lock(&sva_lock);
-	arm_smmu_master_sva_disable_iopf(master);
-	master->sva_enabled = false;
-	mutex_unlock(&sva_lock);
-
-	return 0;
-}
-
 void arm_smmu_sva_notifier_synchronize(void)
 {
 	/*
@@ -353,6 +273,9 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	struct arm_smmu_cd target;
 	int ret;
 
+	if (!(master->smmu->features & ARM_SMMU_FEAT_SVA))
+		return -EOPNOTSUPP;
+
 	/* Prevent arm_smmu_mm_release from being called while we are attaching */
 	if (!mmget_not_zero(domain->mm))
 		return -EINVAL;
@@ -406,6 +329,9 @@ struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,
 	u32 asid;
 	int ret;
 
+	if (!(master->smmu->features & ARM_SMMU_FEAT_SVA))
+		return ERR_PTR(-EOPNOTSUPP);
+
 	smmu_domain = arm_smmu_domain_alloc();
 	if (IS_ERR(smmu_domain))
 		return ERR_CAST(smmu_domain);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 358072b4e293..75b2f7c609ca 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2748,6 +2748,54 @@ to_smmu_domain_devices(struct iommu_domain *domain)
 	return NULL;
 }
 
+static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
+				struct arm_smmu_master_domain *master_domain)
+{
+	int ret;
+
+	iommu_group_mutex_assert(master->dev);
+
+	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
+		return -EOPNOTSUPP;
+
+	/*
+	 * Drivers for devices supporting PRI or stall require iopf others have
+	 * device-specific fault handlers and don't need IOPF, so this is not a
+	 * failure.
+	 */
+	if (!master->stall_enabled)
+		return 0;
+
+	/* We're not keeping track of SIDs in fault events */
+	if (master->num_streams != 1)
+		return -EOPNOTSUPP;
+
+	if (master->iopf_refcount) {
+		master->iopf_refcount++;
+		master_domain->using_iopf = true;
+		return 0;
+	}
+
+	ret = iopf_queue_add_device(master->smmu->evtq.iopf, master->dev);
+	if (ret)
+		return ret;
+	master->iopf_refcount = 1;
+	master_domain->using_iopf = true;
+	return 0;
+}
+
+static void arm_smmu_disable_iopf(struct arm_smmu_master *master)
+{
+	iommu_group_mutex_assert(master->dev);
+
+	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
+		return;
+
+	master->iopf_refcount--;
+	if (master->iopf_refcount == 0)
+		iopf_queue_remove_device(master->smmu->evtq.iopf, master->dev);
+}
+
 static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 					  struct iommu_domain *domain,
 					  ioasid_t ssid)
@@ -2768,11 +2816,16 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 						    nested_ats_flush);
 	if (master_domain) {
 		list_del(&master_domain->devices_elm);
-		kfree(master_domain);
 		if (master->ats_enabled)
 			atomic_dec(&smmu_domain->nr_ats_masters);
 	}
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
+
+	if (master_domain) {
+		if (master_domain->using_iopf)
+			arm_smmu_disable_iopf(master);
+		kfree(master_domain);
+	}
 }
 
 /*
@@ -2803,6 +2856,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 	struct arm_smmu_domain *smmu_domain =
 		to_smmu_domain_devices(new_domain);
 	unsigned long flags;
+	int ret;
 
 	/*
 	 * arm_smmu_share_asid() must not see two domains pointing to the same
@@ -2841,6 +2895,12 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 			master_domain->nested_ats_flush =
 				to_smmu_nested_domain(new_domain)->enable_ats;
 
+		if (new_domain->iopf_handler) {
+			ret = arm_smmu_enable_iopf(master, master_domain);
+			if (ret)
+				goto err_free_master_domain;
+		}
+
 		/*
 		 * During prepare we want the current smmu_domain and new
 		 * smmu_domain to be in the devices list before we change any
@@ -2860,8 +2920,8 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 		    !arm_smmu_master_canwbs(master)) {
 			spin_unlock_irqrestore(&smmu_domain->devices_lock,
 					       flags);
-			kfree(master_domain);
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_iopf;
 		}
 
 		if (state->ats_enabled)
@@ -2880,6 +2940,13 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 		wmb();
 	}
 	return 0;
+
+err_iopf:
+	if (master_domain && master_domain->using_iopf)
+		arm_smmu_disable_iopf(master);
+err_free_master_domain:
+	kfree(master_domain);
+	return ret;
 }
 
 /*
@@ -3475,8 +3542,7 @@ static void arm_smmu_release_device(struct device *dev)
 {
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
-	if (WARN_ON(arm_smmu_master_sva_enabled(master)))
-		iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
+	WARN_ON(master->iopf_refcount);
 
 	/* Put the STE back to what arm_smmu_init_strtab() sets */
 	if (dev->iommu->require_direct)
@@ -3561,18 +3627,8 @@ static int arm_smmu_dev_enable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-		if (!arm_smmu_master_iopf_supported(master))
-			return -EINVAL;
-		if (master->iopf_enabled)
-			return -EBUSY;
-		master->iopf_enabled = true;
-		return 0;
 	case IOMMU_DEV_FEAT_SVA:
-		if (!arm_smmu_master_sva_supported(master))
-			return -EINVAL;
-		if (arm_smmu_master_sva_enabled(master))
-			return -EBUSY;
-		return arm_smmu_master_enable_sva(master);
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -3588,16 +3644,8 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-		if (!master->iopf_enabled)
-			return -EINVAL;
-		if (master->sva_enabled)
-			return -EBUSY;
-		master->iopf_enabled = false;
-		return 0;
 	case IOMMU_DEV_FEAT_SVA:
-		if (!arm_smmu_master_sva_enabled(master))
-			return -EINVAL;
-		return arm_smmu_master_disable_sva(master);
+		return 0;
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index bd9d7c85576a..5653d7417db7 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -830,9 +830,8 @@ struct arm_smmu_master {
 	bool				ats_enabled : 1;
 	bool				ste_ats_enabled : 1;
 	bool				stall_enabled;
-	bool				sva_enabled;
-	bool				iopf_enabled;
 	unsigned int			ssid_bits;
+	unsigned int			iopf_refcount;
 };
 
 /* SMMU private data for an IOMMU domain */
@@ -910,6 +909,7 @@ struct arm_smmu_master_domain {
 	struct arm_smmu_master *master;
 	ioasid_t ssid;
 	bool nested_ats_flush : 1;
+	bool using_iopf : 1;
 };
 
 static inline struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
@@ -987,11 +987,6 @@ int arm_smmu_cmdq_issue_cmdlist(struct arm_smmu_device *smmu,
 
 #ifdef CONFIG_ARM_SMMU_V3_SVA
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu);
-bool arm_smmu_master_sva_supported(struct arm_smmu_master *master);
-bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master);
-int arm_smmu_master_enable_sva(struct arm_smmu_master *master);
-int arm_smmu_master_disable_sva(struct arm_smmu_master *master);
-bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master);
 void arm_smmu_sva_notifier_synchronize(void);
 struct iommu_domain *arm_smmu_sva_domain_alloc(struct device *dev,
 					       struct mm_struct *mm);
@@ -1001,31 +996,6 @@ static inline bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
 	return false;
 }
 
-static inline bool arm_smmu_master_sva_supported(struct arm_smmu_master *master)
-{
-	return false;
-}
-
-static inline bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)
-{
-	return false;
-}
-
-static inline int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
-{
-	return -ENODEV;
-}
-
-static inline int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
-{
-	return -ENODEV;
-}
-
-static inline bool arm_smmu_master_iopf_supported(struct arm_smmu_master *master)
-{
-	return false;
-}
-
 static inline void arm_smmu_sva_notifier_synchronize(void) {}
 
 #define arm_smmu_sva_domain_alloc NULL
-- 
2.43.0

[PATCH 02/12] iommu/vt-d: Check if SVA is supported when attaching the SVA domain

[Lu Baolu]

From: Jason Gunthorpe <jgg@nvidia.com>

Attach of a SVA domain should fail if SVA is not supported, move the check
for SVA support out of IOMMU_DEV_FEAT_SVA and into attach.

Also check when allocating a SVA domain to match other drivers.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 37 +------------------------------
 drivers/iommu/intel/svm.c   | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index cc46098f875b..a4048de66378 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3851,41 +3851,6 @@ static struct iommu_group *intel_iommu_device_group(struct device *dev)
 	return generic_device_group(dev);
 }
 
-static int intel_iommu_enable_sva(struct device *dev)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu;
-
-	if (!info || dmar_disabled)
-		return -EINVAL;
-
-	iommu = info->iommu;
-	if (!iommu)
-		return -EINVAL;
-
-	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
-		return -ENODEV;
-
-	if (!info->pasid_enabled || !info->ats_enabled)
-		return -EINVAL;
-
-	/*
-	 * Devices having device-specific I/O fault handling should not
-	 * support PCI/PRI. The IOMMU side has no means to check the
-	 * capability of device-specific IOPF.  Therefore, IOMMU can only
-	 * default that if the device driver enables SVA on a non-PRI
-	 * device, it will handle IOPF in its own way.
-	 */
-	if (!info->pri_supported)
-		return 0;
-
-	/* Devices supporting PRI should have it enabled. */
-	if (!info->pri_enabled)
-		return -EINVAL;
-
-	return 0;
-}
-
 static int context_flip_pri(struct device_domain_info *info, bool enable)
 {
 	struct intel_iommu *iommu = info->iommu;
@@ -4006,7 +3971,7 @@ intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
 		return intel_iommu_enable_iopf(dev);
 
 	case IOMMU_DEV_FEAT_SVA:
-		return intel_iommu_enable_sva(dev);
+		return 0;
 
 	default:
 		return -ENODEV;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index f5569347591f..ba93123cb4eb 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -110,6 +110,41 @@ static const struct mmu_notifier_ops intel_mmuops = {
 	.free_notifier = intel_mm_free_notifier,
 };
 
+static int intel_iommu_sva_supported(struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu;
+
+	if (!info || dmar_disabled)
+		return -EINVAL;
+
+	iommu = info->iommu;
+	if (!iommu)
+		return -EINVAL;
+
+	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
+		return -ENODEV;
+
+	if (!info->pasid_enabled || !info->ats_enabled)
+		return -EINVAL;
+
+	/*
+	 * Devices having device-specific I/O fault handling should not
+	 * support PCI/PRI. The IOMMU side has no means to check the
+	 * capability of device-specific IOPF.  Therefore, IOMMU can only
+	 * default that if the device driver enables SVA on a non-PRI
+	 * device, it will handle IOPF in its own way.
+	 */
+	if (!info->pri_supported)
+		return 0;
+
+	/* Devices supporting PRI should have it enabled. */
+	if (!info->pri_enabled)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t pasid,
 				   struct iommu_domain *old)
@@ -121,6 +156,10 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	unsigned long sflags;
 	int ret = 0;
 
+	ret = intel_iommu_sva_supported(dev);
+	if (ret)
+		return ret;
+
 	dev_pasid = domain_add_dev_pasid(domain, dev, pasid);
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
@@ -161,6 +200,10 @@ struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
 	struct dmar_domain *domain;
 	int ret;
 
+	ret = intel_iommu_sva_supported(dev);
+	if (ret)
+		return ERR_PTR(ret);
+
 	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
 	if (!domain)
 		return ERR_PTR(-ENOMEM);
-- 
2.43.0

[PATCH 03/12] iommu: Remove IOMMU_DEV_FEAT_SVA

[Lu Baolu]

From: Jason Gunthorpe <jgg@nvidia.com>

None of the drivers implement anything here anymore, remove the dead code.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/accel/amdxdna/aie2_pci.c            | 13 ++-----------
 drivers/dma/idxd/init.c                     |  8 +-------
 drivers/iommu/amd/iommu.c                   |  2 --
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c |  2 --
 drivers/iommu/intel/iommu.c                 |  6 ------
 drivers/iommu/iommu-sva.c                   |  3 ---
 drivers/misc/uacce/uacce.c                  |  9 ---------
 include/linux/iommu.h                       |  9 +--------
 8 files changed, 4 insertions(+), 48 deletions(-)

diff --git a/drivers/accel/amdxdna/aie2_pci.c b/drivers/accel/amdxdna/aie2_pci.c
index 5a058e565b01..c6cf7068d23c 100644
--- a/drivers/accel/amdxdna/aie2_pci.c
+++ b/drivers/accel/amdxdna/aie2_pci.c
@@ -512,12 +512,6 @@ static int aie2_init(struct amdxdna_dev *xdna)
 		goto release_fw;
 	}
 
-	ret = iommu_dev_enable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
-	if (ret) {
-		XDNA_ERR(xdna, "Enable PASID failed, ret %d", ret);
-		goto free_irq;
-	}
-
 	psp_conf.fw_size = fw->size;
 	psp_conf.fw_buf = fw->data;
 	for (i = 0; i < PSP_MAX_REGS; i++)
@@ -526,14 +520,14 @@ static int aie2_init(struct amdxdna_dev *xdna)
 	if (!ndev->psp_hdl) {
 		XDNA_ERR(xdna, "failed to create psp");
 		ret = -ENOMEM;
-		goto disable_sva;
+		goto free_irq;
 	}
 	xdna->dev_handle = ndev;
 
 	ret = aie2_hw_start(xdna);
 	if (ret) {
 		XDNA_ERR(xdna, "start npu failed, ret %d", ret);
-		goto disable_sva;
+		goto free_irq;
 	}
 
 	ret = aie2_mgmt_fw_query(ndev);
@@ -584,8 +578,6 @@ static int aie2_init(struct amdxdna_dev *xdna)
 	aie2_error_async_events_free(ndev);
 stop_hw:
 	aie2_hw_stop(xdna);
-disable_sva:
-	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
 free_irq:
 	pci_free_irq_vectors(pdev);
 release_fw:
@@ -601,7 +593,6 @@ static void aie2_fini(struct amdxdna_dev *xdna)
 
 	aie2_hw_stop(xdna);
 	aie2_error_async_events_free(ndev);
-	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_SVA);
 	pci_free_irq_vectors(pdev);
 }
 
diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index b946f78f85e1..1e5038cca22c 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -633,17 +633,11 @@ static int idxd_enable_sva(struct pci_dev *pdev)
 	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;
+	return 0;
 }
 
 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);
 }
 
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index b48a72bd7b23..e3653bdfcd7d 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2990,7 +2990,6 @@ static int amd_iommu_dev_enable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-	case IOMMU_DEV_FEAT_SVA:
 		break;
 	default:
 		ret = -EINVAL;
@@ -3006,7 +3005,6 @@ static int amd_iommu_dev_disable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-	case IOMMU_DEV_FEAT_SVA:
 		break;
 	default:
 		ret = -EINVAL;
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 75b2f7c609ca..ee945a9db641 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3627,7 +3627,6 @@ static int arm_smmu_dev_enable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-	case IOMMU_DEV_FEAT_SVA:
 		return 0;
 	default:
 		return -EINVAL;
@@ -3644,7 +3643,6 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
 
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-	case IOMMU_DEV_FEAT_SVA:
 		return 0;
 	default:
 		return -EINVAL;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a4048de66378..16dd8f0de76d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3970,9 +3970,6 @@ intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
 	case IOMMU_DEV_FEAT_IOPF:
 		return intel_iommu_enable_iopf(dev);
 
-	case IOMMU_DEV_FEAT_SVA:
-		return 0;
-
 	default:
 		return -ENODEV;
 	}
@@ -3985,9 +3982,6 @@ intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
 	case IOMMU_DEV_FEAT_IOPF:
 		return intel_iommu_disable_iopf(dev);
 
-	case IOMMU_DEV_FEAT_SVA:
-		return 0;
-
 	default:
 		return -ENODEV;
 	}
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 503c5d23c1ea..331be2761a75 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -63,9 +63,6 @@ static struct iommu_mm_data *iommu_alloc_mm_data(struct mm_struct *mm, struct de
  * reference is taken. Caller must call iommu_sva_unbind_device()
  * to release each reference.
  *
- * iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) must be called first, to
- * initialize the required SVA features.
- *
  * On error, returns an ERR_PTR value.
  */
 struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index bdc2e6fda782..2a1db2abeeca 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -479,14 +479,6 @@ static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
 		dev_err(parent, "failed to enable IOPF feature! ret = %pe\n", ERR_PTR(ret));
 		return flags;
 	}
-
-	ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_SVA);
-	if (ret) {
-		dev_err(parent, "failed to enable SVA feature! ret = %pe\n", ERR_PTR(ret));
-		iommu_dev_disable_feature(parent, IOMMU_DEV_FEAT_IOPF);
-		return flags;
-	}
-
 	return flags | UACCE_DEV_SVA;
 }
 
@@ -495,7 +487,6 @@ static void uacce_disable_sva(struct uacce_device *uacce)
 	if (!(uacce->flags & UACCE_DEV_SVA))
 		return;
 
-	iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_SVA);
 	iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
 }
 
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 38c65e92ecd0..1d0dde49168d 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -303,18 +303,11 @@ struct iommu_iort_rmr_data {
 
 /**
  * enum iommu_dev_features - Per device IOMMU features
- * @IOMMU_DEV_FEAT_SVA: Shared Virtual Addresses
- * @IOMMU_DEV_FEAT_IOPF: I/O Page Faults such as PRI or Stall. 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. When
- *			 supported, this feature must be enabled before and
- *			 disabled after %IOMMU_DEV_FEAT_SVA.
+ * @IOMMU_DEV_FEAT_IOPF: I/O Page Faults such as PRI or Stall.
  *
  * Device drivers enable a feature using iommu_dev_enable_feature().
  */
 enum iommu_dev_features {
-	IOMMU_DEV_FEAT_SVA,
 	IOMMU_DEV_FEAT_IOPF,
 };
 
-- 
2.43.0

[PATCH 04/12] iommu/vt-d: Move scalable mode ATS enablement to probe path

[Lu Baolu]

Device ATS is currently enabled when a domain is attached to the device
and disabled when the domain is detached. This creates a limitation:
when the IOMMU is operating in scalable mode and IOPF is enabled, the
device's domain cannot be changed.

Remove this limitation by moving ATS enablement to the device probe path.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 78 ++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 16dd8f0de76d..f52602bde742 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1172,34 +1172,6 @@ static bool dev_needs_extra_dtlb_flush(struct pci_dev *pdev)
 	return true;
 }
 
-static void iommu_enable_pci_caps(struct device_domain_info *info)
-{
-	struct pci_dev *pdev;
-
-	if (!dev_is_pci(info->dev))
-		return;
-
-	pdev = to_pci_dev(info->dev);
-	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
-	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT))
-		info->ats_enabled = 1;
-}
-
-static void iommu_disable_pci_caps(struct device_domain_info *info)
-{
-	struct pci_dev *pdev;
-
-	if (!dev_is_pci(info->dev))
-		return;
-
-	pdev = to_pci_dev(info->dev);
-
-	if (info->ats_enabled) {
-		pci_disable_ats(pdev);
-		info->ats_enabled = 0;
-	}
-}
-
 static void intel_flush_iotlb_all(struct iommu_domain *domain)
 {
 	cache_tag_flush_all(to_dmar_domain(domain));
@@ -1556,12 +1528,22 @@ domain_context_mapping(struct dmar_domain *domain, struct device *dev)
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 	u8 bus = info->bus, devfn = info->devfn;
+	struct pci_dev *pdev;
+	int ret;
 
 	if (!dev_is_pci(dev))
 		return domain_context_mapping_one(domain, iommu, bus, devfn);
 
-	return pci_for_each_dma_alias(to_pci_dev(dev),
-				      domain_context_mapping_cb, domain);
+	pdev = to_pci_dev(dev);
+	ret = pci_for_each_dma_alias(pdev, domain_context_mapping_cb, domain);
+	if (ret)
+		return ret;
+
+	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
+	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT))
+		info->ats_enabled = 1;
+
+	return 0;
 }
 
 /* Return largest possible superpage level for a given mapping */
@@ -1843,8 +1825,6 @@ static int dmar_domain_attach_device(struct dmar_domain *domain,
 	if (ret)
 		goto out_block_translation;
 
-	iommu_enable_pci_caps(info);
-
 	ret = cache_tag_assign_domain(domain, dev, IOMMU_NO_PASID);
 	if (ret)
 		goto out_block_translation;
@@ -3191,13 +3171,20 @@ static int domain_context_clear_one_cb(struct pci_dev *pdev, u16 alias, void *op
  */
 static void domain_context_clear(struct device_domain_info *info)
 {
+	struct pci_dev *pdev;
+
 	if (!dev_is_pci(info->dev)) {
 		domain_context_clear_one(info, info->bus, info->devfn);
 		return;
 	}
 
-	pci_for_each_dma_alias(to_pci_dev(info->dev),
-			       &domain_context_clear_one_cb, info);
+	pdev = to_pci_dev(info->dev);
+	pci_for_each_dma_alias(pdev, &domain_context_clear_one_cb, info);
+
+	if (info->ats_enabled) {
+		pci_disable_ats(pdev);
+		info->ats_enabled = 0;
+	}
 }
 
 /*
@@ -3214,7 +3201,6 @@ void device_block_translation(struct device *dev)
 	if (info->domain)
 		cache_tag_unassign_domain(info->domain, dev, IOMMU_NO_PASID);
 
-	iommu_disable_pci_caps(info);
 	if (!dev_is_real_dma_subdevice(dev)) {
 		if (sm_supported(iommu))
 			intel_pasid_tear_down_entry(iommu, dev,
@@ -3749,6 +3735,16 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 	    !pci_enable_pasid(pdev, info->pasid_supported & ~1))
 		info->pasid_enabled = 1;
 
+	if (sm_supported(iommu)) {
+		if (info->ats_supported && pci_ats_page_aligned(pdev)) {
+			ret = pci_enable_ats(pdev, VTD_PAGE_SHIFT);
+			if (ret)
+				pci_info(pdev, "Failed to enable ATS on device\n");
+			else
+				info->ats_enabled = 1;
+		}
+	}
+
 	return &iommu->iommu;
 free_table:
 	intel_pasid_free_table(dev);
@@ -3765,6 +3761,11 @@ static void intel_iommu_release_device(struct device *dev)
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 
+	if (info->ats_enabled) {
+		pci_disable_ats(to_pci_dev(dev));
+		info->ats_enabled = 0;
+	}
+
 	if (info->pasid_enabled) {
 		pci_disable_pasid(to_pci_dev(dev));
 		info->pasid_enabled = 0;
@@ -4365,13 +4366,10 @@ static int identity_domain_attach_dev(struct iommu_domain *domain, struct device
 	if (dev_is_real_dma_subdevice(dev))
 		return 0;
 
-	if (sm_supported(iommu)) {
+	if (sm_supported(iommu))
 		ret = intel_pasid_setup_pass_through(iommu, dev, IOMMU_NO_PASID);
-		if (!ret)
-			iommu_enable_pci_caps(info);
-	} else {
+	else
 		ret = device_setup_pass_through(dev);
-	}
 
 	return ret;
 }
-- 
2.43.0

[PATCH 05/12] iommu/vt-d: Move PRI enablement in probe path

[Lu Baolu]

Update PRI enablement to use the new method, similar to the amd iommu
driver. Enable PRI in the device probe path and disable it when the device
is released. PRI is enabled throughout the device's iommu lifecycle. The
infrastructure for the iommu subsystem to handle iopf requests is created
during iopf enablement and released during iopf disablement.  All invalid
page requests from the device are automatically handled by the iommu
subsystem if iopf is not enabled. Add iopf_refcount to track the iopf
enablement.

Convert the return type of intel_iommu_disable_iopf() to void, as there
is no way to handle a failure when disabling this feature.  Make
intel_iommu_enable/disable_iopf() helpers global, as they will be used
beyond the current file in the subsequent patch.

The iopf_refcount is not protected by any lock. This is acceptable, as
there is no concurrent access to it in the current code. The following
patch will address this by moving it to the domain attach/detach paths,
which are protected by the iommu group mutex.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 145 +++++++++++-------------------------
 drivers/iommu/intel/iommu.h |   4 +
 drivers/iommu/intel/pasid.c |   2 +
 drivers/iommu/intel/prq.c   |   2 +-
 4 files changed, 51 insertions(+), 102 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index f52602bde742..91d49e2cea34 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3743,6 +3743,16 @@ static struct iommu_device *intel_iommu_probe_device(struct device *dev)
 			else
 				info->ats_enabled = 1;
 		}
+
+		if (info->ats_enabled && info->pri_supported) {
+			/* PASID is required in PRG Response Message. */
+			if (info->pasid_enabled || !pci_prg_resp_pasid_required(pdev)) {
+				if (!pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
+					info->pri_enabled = 1;
+				else
+					pci_info(pdev, "Failed to enable PRI on device\n");
+			}
+		}
 	}
 
 	return &iommu->iommu;
@@ -3761,6 +3771,13 @@ static void intel_iommu_release_device(struct device *dev)
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 	struct intel_iommu *iommu = info->iommu;
 
+	WARN_ON(info->iopf_refcount);
+
+	if (info->pri_enabled) {
+		pci_disable_pri(to_pci_dev(dev));
+		info->pri_enabled = 0;
+	}
+
 	if (info->ats_enabled) {
 		pci_disable_ats(to_pci_dev(dev));
 		info->ats_enabled = 0;
@@ -3852,118 +3869,43 @@ static struct iommu_group *intel_iommu_device_group(struct device *dev)
 	return generic_device_group(dev);
 }
 
-static int context_flip_pri(struct device_domain_info *info, bool enable)
+int intel_iommu_enable_iopf(struct device *dev)
 {
-	struct intel_iommu *iommu = info->iommu;
-	u8 bus = info->bus, devfn = info->devfn;
-	struct context_entry *context;
-	u16 did;
-
-	spin_lock(&iommu->lock);
-	if (context_copied(iommu, bus, devfn)) {
-		spin_unlock(&iommu->lock);
-		return -EINVAL;
-	}
-
-	context = iommu_context_addr(iommu, bus, devfn, false);
-	if (!context || !context_present(context)) {
-		spin_unlock(&iommu->lock);
-		return -ENODEV;
-	}
-	did = context_domain_id(context);
-
-	if (enable)
-		context_set_sm_pre(context);
-	else
-		context_clear_sm_pre(context);
-
-	if (!ecap_coherent(iommu->ecap))
-		clflush_cache_range(context, sizeof(*context));
-	intel_context_flush_present(info, context, did, true);
-	spin_unlock(&iommu->lock);
-
-	return 0;
-}
-
-static int intel_iommu_enable_iopf(struct device *dev)
-{
-	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu;
+	struct intel_iommu *iommu = info->iommu;
 	int ret;
 
-	if (!pdev || !info || !info->ats_enabled || !info->pri_supported)
-		return -ENODEV;
-
-	if (info->pri_enabled)
-		return -EBUSY;
-
-	iommu = info->iommu;
-	if (!iommu)
-		return -EINVAL;
-
-	/* PASID is required in PRG Response Message. */
-	if (info->pasid_enabled && !pci_prg_resp_pasid_required(pdev))
-		return -EINVAL;
-
-	ret = pci_reset_pri(pdev);
-	if (ret)
-		return ret;
-
-	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
-	if (ret)
-		return ret;
-
-	ret = context_flip_pri(info, true);
-	if (ret)
-		goto err_remove_device;
-
-	ret = pci_enable_pri(pdev, PRQ_DEPTH);
-	if (ret)
-		goto err_clear_pri;
-
-	info->pri_enabled = 1;
-
-	return 0;
-err_clear_pri:
-	context_flip_pri(info, false);
-err_remove_device:
-	iopf_queue_remove_device(iommu->iopf_queue, dev);
-
-	return ret;
-}
-
-static int intel_iommu_disable_iopf(struct device *dev)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu = info->iommu;
-
 	if (!info->pri_enabled)
-		return -EINVAL;
+		return -ENODEV;
 
-	/* Disable new PRI reception: */
-	context_flip_pri(info, false);
+	if (info->iopf_refcount) {
+		info->iopf_refcount++;
+		return 0;
+	}
 
-	/*
-	 * Remove device from fault queue and acknowledge all outstanding
-	 * PRQs to the device:
-	 */
-	iopf_queue_remove_device(iommu->iopf_queue, dev);
+	ret = iopf_queue_add_device(iommu->iopf_queue, dev);
+	if (ret)
+		return ret;
 
-	/*
-	 * PCIe spec states that by clearing PRI enable bit, the Page
-	 * Request Interface will not issue new page requests, but has
-	 * outstanding page requests that have been transmitted or are
-	 * queued for transmission. This is supposed to be called after
-	 * the device driver has stopped DMA, all PASIDs have been
-	 * unbound and the outstanding PRQs have been drained.
-	 */
-	pci_disable_pri(to_pci_dev(dev));
-	info->pri_enabled = 0;
+	info->iopf_refcount = 1;
 
 	return 0;
 }
 
+void intel_iommu_disable_iopf(struct device *dev)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+
+	if (WARN_ON(!info->pri_enabled))
+		return;
+
+	if (--info->iopf_refcount)
+		return;
+
+	iopf_queue_remove_device(iommu->iopf_queue, dev);
+}
+
 static int
 intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
 {
@@ -3981,7 +3923,8 @@ intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
 {
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-		return intel_iommu_disable_iopf(dev);
+		intel_iommu_disable_iopf(dev);
+		return 0;
 
 	default:
 		return -ENODEV;
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 6ea7bbe26b19..f7d78cf0778c 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -774,6 +774,7 @@ struct device_domain_info {
 	u8 ats_enabled:1;
 	u8 dtlb_extra_inval:1;	/* Quirk for devices need extra flush */
 	u8 ats_qdep;
+	unsigned int iopf_refcount;
 	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
 	struct intel_iommu *iommu; /* IOMMU used by this device */
 	struct dmar_domain *domain; /* pointer to domain */
@@ -1314,6 +1315,9 @@ void intel_iommu_page_response(struct device *dev, struct iopf_fault *evt,
 			       struct iommu_page_response *msg);
 void intel_iommu_drain_pasid_prq(struct device *dev, u32 pasid);
 
+int intel_iommu_enable_iopf(struct device *dev);
+void intel_iommu_disable_iopf(struct device *dev);
+
 #ifdef CONFIG_INTEL_IOMMU_SVM
 void intel_svm_check(struct intel_iommu *iommu);
 struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index fb59a7d35958..c2742e256552 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -992,6 +992,8 @@ static int context_entry_set_pasid_table(struct context_entry *context,
 		context_set_sm_dte(context);
 	if (info->pasid_supported)
 		context_set_pasid(context);
+	if (info->pri_supported)
+		context_set_sm_pre(context);
 
 	context_set_fault_enable(context);
 	context_set_present(context);
diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
index c2d792db52c3..2bdaf9293ab4 100644
--- a/drivers/iommu/intel/prq.c
+++ b/drivers/iommu/intel/prq.c
@@ -67,7 +67,7 @@ void intel_iommu_drain_pasid_prq(struct device *dev, u32 pasid)
 	u16 sid, did;
 
 	info = dev_iommu_priv_get(dev);
-	if (!info->pri_enabled)
+	if (!info->iopf_refcount)
 		return;
 
 	iommu = info->iommu;
-- 
2.43.0

[PATCH 06/12] iommu/vt-d: Cleanup intel_context_flush_present()

[Lu Baolu]

The intel_context_flush_present() is called in places where either the
scalable mode is disabled, or scalable mode is enabled but all PASID
entries are known to be non-present. In these cases, the flush_domains
path within intel_context_flush_present() will never execute. This dead
code is therefore removed.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c |  2 +-
 drivers/iommu/intel/iommu.h |  3 +--
 drivers/iommu/intel/pasid.c | 39 ++++++-------------------------------
 3 files changed, 8 insertions(+), 36 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 91d49e2cea34..1d564240c977 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1730,7 +1730,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 	context_clear_entry(context);
 	__iommu_flush_cache(iommu, context, sizeof(*context));
 	spin_unlock(&iommu->lock);
-	intel_context_flush_present(info, context, did, true);
+	intel_context_flush_present(info, context, did);
 }
 
 int __domain_setup_first_level(struct intel_iommu *iommu,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index f7d78cf0778c..754f6d7ade26 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1306,8 +1306,7 @@ void cache_tag_flush_range_np(struct dmar_domain *domain, unsigned long start,
 			      unsigned long end);
 
 void intel_context_flush_present(struct device_domain_info *info,
-				 struct context_entry *context,
-				 u16 did, bool affect_domains);
+				 struct context_entry *context, u16 did);
 
 int intel_iommu_enable_prq(struct intel_iommu *iommu);
 int intel_iommu_finish_prq(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c2742e256552..a2c6be624dbf 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -932,7 +932,7 @@ static void device_pasid_table_teardown(struct device *dev, u8 bus, u8 devfn)
 	context_clear_entry(context);
 	__iommu_flush_cache(iommu, context, sizeof(*context));
 	spin_unlock(&iommu->lock);
-	intel_context_flush_present(info, context, did, false);
+	intel_context_flush_present(info, context, did);
 }
 
 static int pci_pasid_table_teardown(struct pci_dev *pdev, u16 alias, void *data)
@@ -1119,17 +1119,15 @@ static void __context_flush_dev_iotlb(struct device_domain_info *info)
 
 /*
  * Cache invalidations after change in a context table entry that was present
- * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations). If
- * IOMMU is in scalable mode and all PASID table entries of the device were
- * non-present, set flush_domains to false. Otherwise, true.
+ * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations).
+ * This helper can only be used when IOMMU is working in the legacy mode or
+ * IOMMU is in scalable mode but all PASID table entries of the device are
+ * non-present.
  */
 void intel_context_flush_present(struct device_domain_info *info,
-				 struct context_entry *context,
-				 u16 did, bool flush_domains)
+				 struct context_entry *context, u16 did)
 {
 	struct intel_iommu *iommu = info->iommu;
-	struct pasid_entry *pte;
-	int i;
 
 	/*
 	 * Device-selective context-cache invalidation. The Domain-ID field
@@ -1152,30 +1150,5 @@ void intel_context_flush_present(struct device_domain_info *info,
 		return;
 	}
 
-	/*
-	 * For scalable mode:
-	 * - Domain-selective PASID-cache invalidation to affected domains
-	 * - Domain-selective IOTLB invalidation to affected domains
-	 * - Global Device-TLB invalidation to affected functions
-	 */
-	if (flush_domains) {
-		/*
-		 * If the IOMMU is running in scalable mode and there might
-		 * be potential PASID translations, the caller should hold
-		 * the lock to ensure that context changes and cache flushes
-		 * are atomic.
-		 */
-		assert_spin_locked(&iommu->lock);
-		for (i = 0; i < info->pasid_table->max_pasid; i++) {
-			pte = intel_pasid_get_entry(info->dev, i);
-			if (!pte || !pasid_pte_is_present(pte))
-				continue;
-
-			did = pasid_get_domain_id(pte);
-			qi_flush_pasid_cache(iommu, did, QI_PC_ALL_PASIDS, 0);
-			iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
-		}
-	}
-
 	__context_flush_dev_iotlb(info);
 }
-- 
2.43.0

[PATCH 07/12] iommu/vt-d: Put iopf enablement in domain attach path

[Lu Baolu]

Update iopf enablement in the driver to use the new method, similar to
the arm-smmu-v3 driver. Enable iopf support when any domain with an
iopf_handler is attached, and disable it when the domain is removed.

Place all the logic for controlling the PRI and iopf queue in the domain
set/remove/replace paths. Keep track of the number of domains set to the
device and PASIDs that require iopf. When the first domain requiring iopf
is attached, add the device to the iopf queue and enable PRI. When the
last domain is removed, remove it from the iopf queue and disable PRI.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c  | 42 ++++++++++++++++++++++++++++++-----
 drivers/iommu/intel/iommu.h  | 43 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/nested.c | 16 ++++++++++++--
 drivers/iommu/intel/svm.c    |  9 ++++++--
 4 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1d564240c977..20d07f5fea85 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3223,6 +3223,9 @@ void device_block_translation(struct device *dev)
 static int blocking_domain_attach_dev(struct iommu_domain *domain,
 				      struct device *dev)
 {
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+
+	iopf_for_domain_remove(info->domain ? &info->domain->domain : NULL, dev);
 	device_block_translation(dev);
 	return 0;
 }
@@ -3432,7 +3435,15 @@ 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 = iopf_for_domain_set(domain, dev);
+	if (ret)
+		return ret;
+
+	ret = dmar_domain_attach_device(to_dmar_domain(domain), dev);
+	if (ret)
+		iopf_for_domain_remove(domain, dev);
+
+	return ret;
 }
 
 static int intel_iommu_map(struct iommu_domain *domain,
@@ -3878,6 +3889,8 @@ int intel_iommu_enable_iopf(struct device *dev)
 	if (!info->pri_enabled)
 		return -ENODEV;
 
+	/* pri_enabled is protected by the group mutex. */
+	iommu_group_mutex_assert(dev);
 	if (info->iopf_refcount) {
 		info->iopf_refcount++;
 		return 0;
@@ -3900,6 +3913,7 @@ void intel_iommu_disable_iopf(struct device *dev)
 	if (WARN_ON(!info->pri_enabled))
 		return;
 
+	iommu_group_mutex_assert(dev);
 	if (--info->iopf_refcount)
 		return;
 
@@ -3911,8 +3925,7 @@ intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
 {
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-		return intel_iommu_enable_iopf(dev);
-
+		return 0;
 	default:
 		return -ENODEV;
 	}
@@ -3923,7 +3936,6 @@ intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
 {
 	switch (feat) {
 	case IOMMU_DEV_FEAT_IOPF:
-		intel_iommu_disable_iopf(dev);
 		return 0;
 
 	default:
@@ -4004,6 +4016,7 @@ static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 
+	iopf_for_domain_remove(old, dev);
 	intel_pasid_tear_down_entry(info->iommu, dev, pasid, false);
 	domain_remove_dev_pasid(old, dev, pasid);
 
@@ -4077,6 +4090,10 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
 
+	ret = iopf_for_domain_replace(domain, old, dev);
+	if (ret)
+		goto out_remove_dev_pasid;
+
 	if (dmar_domain->use_first_level)
 		ret = domain_setup_first_level(iommu, dmar_domain,
 					       dev, pasid, old);
@@ -4084,7 +4101,7 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 		ret = domain_setup_second_level(iommu, dmar_domain,
 						dev, pasid, old);
 	if (ret)
-		goto out_remove_dev_pasid;
+		goto out_unwind_iopf;
 
 	domain_remove_dev_pasid(old, dev, pasid);
 
@@ -4092,6 +4109,8 @@ static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
 
 	return 0;
 
+out_unwind_iopf:
+	iopf_for_domain_replace(old, domain, dev);
 out_remove_dev_pasid:
 	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
@@ -4309,6 +4328,11 @@ static int identity_domain_attach_dev(struct iommu_domain *domain, struct device
 	if (dev_is_real_dma_subdevice(dev))
 		return 0;
 
+	/*
+	 * No PRI support with the global identity domain. No need to enable or
+	 * disable PRI in this path as the iommu has been put in the blocking
+	 * state.
+	 */
 	if (sm_supported(iommu))
 		ret = intel_pasid_setup_pass_through(iommu, dev, IOMMU_NO_PASID);
 	else
@@ -4328,9 +4352,15 @@ static int identity_domain_set_dev_pasid(struct iommu_domain *domain,
 	if (!pasid_supported(iommu) || dev_is_real_dma_subdevice(dev))
 		return -EOPNOTSUPP;
 
+	ret = iopf_for_domain_replace(domain, old, dev);
+	if (ret)
+		return ret;
+
 	ret = domain_setup_passthrough(iommu, dev, pasid, old);
-	if (ret)
+	if (ret) {
+		iopf_for_domain_replace(old, domain, dev);
 		return ret;
+	}
 
 	domain_remove_dev_pasid(old, dev, pasid);
 	return 0;
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 754f6d7ade26..dfb0628fabf8 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1317,6 +1317,49 @@ void intel_iommu_drain_pasid_prq(struct device *dev, u32 pasid);
 int intel_iommu_enable_iopf(struct device *dev);
 void intel_iommu_disable_iopf(struct device *dev);
 
+static inline int iopf_for_domain_set(struct iommu_domain *domain,
+				      struct device *dev)
+{
+	if (!domain || !domain->iopf_handler)
+		return 0;
+
+	return intel_iommu_enable_iopf(dev);
+}
+
+static inline void iopf_for_domain_remove(struct iommu_domain *domain,
+					  struct device *dev)
+{
+	if (!domain || !domain->iopf_handler)
+		return;
+
+	intel_iommu_disable_iopf(dev);
+}
+
+static inline int iopf_for_domain_replace(struct iommu_domain *new,
+					  struct iommu_domain *old,
+					  struct device *dev)
+{
+	if (!old)
+		return iopf_for_domain_set(new, dev);
+
+	if (!new) {
+		iopf_for_domain_remove(old, dev);
+		return 0;
+	}
+
+	/*
+	 * Replace a non-iopf-capable domain with an iopf-capable one requires
+	 * to enable PRI on the device. On the contrary, disable it.
+	 */
+	if (new->iopf_handler && !old->iopf_handler)
+		return intel_iommu_enable_iopf(dev);
+
+	if (!new->iopf_handler && old->iopf_handler)
+		intel_iommu_disable_iopf(dev);
+
+	return 0;
+}
+
 #ifdef CONFIG_INTEL_IOMMU_SVM
 void intel_svm_check(struct intel_iommu *iommu);
 struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
diff --git a/drivers/iommu/intel/nested.c b/drivers/iommu/intel/nested.c
index aba92c00b427..ad307248bcae 100644
--- a/drivers/iommu/intel/nested.c
+++ b/drivers/iommu/intel/nested.c
@@ -56,10 +56,14 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
 	if (ret)
 		goto detach_iommu;
 
+	ret = iopf_for_domain_set(domain, dev);
+	if (ret)
+		goto unassign_tag;
+
 	ret = intel_pasid_setup_nested(iommu, dev,
 				       IOMMU_NO_PASID, dmar_domain);
 	if (ret)
-		goto unassign_tag;
+		goto disable_iopf;
 
 	info->domain = dmar_domain;
 	spin_lock_irqsave(&dmar_domain->lock, flags);
@@ -67,6 +71,8 @@ static int intel_nested_attach_dev(struct iommu_domain *domain,
 	spin_unlock_irqrestore(&dmar_domain->lock, flags);
 
 	return 0;
+disable_iopf:
+	iopf_for_domain_remove(domain, dev);
 unassign_tag:
 	cache_tag_unassign_domain(dmar_domain, dev, IOMMU_NO_PASID);
 detach_iommu:
@@ -166,14 +172,20 @@ static int intel_nested_set_dev_pasid(struct iommu_domain *domain,
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
 
-	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
+	ret = iopf_for_domain_replace(domain, old, dev);
 	if (ret)
 		goto out_remove_dev_pasid;
 
+	ret = domain_setup_nested(iommu, dmar_domain, dev, pasid, old);
+	if (ret)
+		goto out_unwind_iopf;
+
 	domain_remove_dev_pasid(old, dev, pasid);
 
 	return 0;
 
+out_unwind_iopf:
+	iopf_for_domain_replace(old, domain, dev);
 out_remove_dev_pasid:
 	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index ba93123cb4eb..f3da596410b5 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -164,18 +164,23 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	if (IS_ERR(dev_pasid))
 		return PTR_ERR(dev_pasid);
 
+	ret = iopf_for_domain_replace(domain, old, dev);
+	if (ret)
+		goto out_remove_dev_pasid;
+
 	/* Setup the pasid table: */
 	sflags = cpu_feature_enabled(X86_FEATURE_LA57) ? PASID_FLAG_FL5LP : 0;
 	ret = __domain_setup_first_level(iommu, dev, pasid,
 					 FLPT_DEFAULT_DID, mm->pgd,
 					 sflags, old);
 	if (ret)
-		goto out_remove_dev_pasid;
+		goto out_unwind_iopf;
 
 	domain_remove_dev_pasid(old, dev, pasid);
 
 	return 0;
-
+out_unwind_iopf:
+	iopf_for_domain_replace(old, domain, dev);
 out_remove_dev_pasid:
 	domain_remove_dev_pasid(domain, dev, pasid);
 	return ret;
-- 
2.43.0

[PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Lu Baolu]

Update iopf enablement in the iommufd mock device driver to use the new
method, similar to the arm-smmu-v3 driver. Enable iopf support when any
domain with an iopf_handler is attached, and disable it when the domain
is removed.

Add a refcount in the mock device state structure to keep track of the
number of domains set to the device and PASIDs that require iopf.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommufd/selftest.c | 52 +++++++++++++++++++++++++-------
 1 file changed, 41 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
index d40deb0a4f06..a6b12cee7b00 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -58,6 +58,9 @@ enum {
 	MOCK_PFN_HUGE_IOVA = _MOCK_PFN_START << 2,
 };
 
+static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain);
+static void mock_dev_disable_iopf(struct device *dev, struct iommu_domain *domain);
+
 /*
  * Syzkaller has trouble randomizing the correct iova to use since it is linked
  * to the map ioctl's output, and it has no ide about that. So, simplify things.
@@ -164,6 +167,7 @@ struct mock_dev {
 	unsigned long flags;
 	int id;
 	u32 cache[MOCK_DEV_CACHE_NUM];
+	unsigned int iopf_refcount;
 };
 
 static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
 	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
 		return -EINVAL;
 
+	return mock_dev_enable_iopf(dev, domain);
+}
+
+static int mock_domain_blocking_attach(struct iommu_domain *domain,
+				       struct device *dev)
+{
+	mock_dev_disable_iopf(dev, domain);
+
 	return 0;
 }
 
 static const struct iommu_domain_ops mock_blocking_ops = {
-	.attach_dev = mock_domain_nop_attach,
+	.attach_dev = mock_domain_blocking_attach,
 };
 
 static struct iommu_domain mock_blocking_domain = {
@@ -549,22 +561,42 @@ static void mock_domain_page_response(struct device *dev, struct iopf_fault *evt
 {
 }
 
-static int mock_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
+static int mock_dev_enable_iopf(struct device *dev, struct iommu_domain *domain)
 {
-	if (feat != IOMMU_DEV_FEAT_IOPF || !mock_iommu_iopf_queue)
+	struct mock_dev *mdev = to_mock_dev(dev);
+	int ret;
+
+	if (!domain->iopf_handler)
+		return 0;
+
+	if (!mock_iommu_iopf_queue)
 		return -ENODEV;
 
-	return iopf_queue_add_device(mock_iommu_iopf_queue, dev);
+	if (mdev->iopf_refcount) {
+		mdev->iopf_refcount++;
+		return 0;
+	}
+
+	ret = iopf_queue_add_device(mock_iommu_iopf_queue, dev);
+	if (ret)
+		return ret;
+
+	mdev->iopf_refcount = 1;
+
+	return 0;
 }
 
-static int mock_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
+static void mock_dev_disable_iopf(struct device *dev, struct iommu_domain *domain)
 {
-	if (feat != IOMMU_DEV_FEAT_IOPF || !mock_iommu_iopf_queue)
-		return -ENODEV;
+	struct mock_dev *mdev = to_mock_dev(dev);
+
+	if (!domain->iopf_handler)
+		return;
+
+	if (--mdev->iopf_refcount)
+		return;
 
 	iopf_queue_remove_device(mock_iommu_iopf_queue, dev);
-
-	return 0;
 }
 
 static void mock_viommu_destroy(struct iommufd_viommu *viommu)
@@ -709,8 +741,6 @@ static const struct iommu_ops mock_ops = {
 	.device_group = generic_device_group,
 	.probe_device = mock_probe_device,
 	.page_response = mock_domain_page_response,
-	.dev_enable_feat = mock_dev_enable_feat,
-	.dev_disable_feat = mock_dev_disable_feat,
 	.user_pasid_table = true,
 	.viommu_alloc = mock_viommu_alloc,
 	.default_domain_ops =
-- 
2.43.0

[PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Lu Baolu]

The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a no-op.
It will also be removed from the iommu driver in the subsequent patch.
Remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/dma/idxd/init.c | 37 ++++++-------------------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
index 1e5038cca22c..d44944195807 100644
--- a/drivers/dma/idxd/init.c
+++ b/drivers/dma/idxd/init.c
@@ -626,21 +626,6 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
 	idxd->pasid = IOMMU_PASID_INVALID;
 }
 
-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;
-	return 0;
-}
-
-static void idxd_disable_sva(struct pci_dev *pdev)
-{
-	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
-}
-
 static int idxd_probe(struct idxd_device *idxd)
 {
 	struct pci_dev *pdev = idxd->pdev;
@@ -655,17 +640,13 @@ static int idxd_probe(struct idxd_device *idxd)
 	dev_dbg(dev, "IDXD reset complete\n");
 
 	if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && 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);
+		set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
 
-			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);
-		}
+		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);
 	} else if (!sva) {
 		dev_warn(dev, "User forced SVA off via module param.\n");
 	}
@@ -703,8 +684,6 @@ static int idxd_probe(struct idxd_device *idxd)
  err:
 	if (device_pasid_enabled(idxd))
 		idxd_disable_system_pasid(idxd);
-	if (device_user_pasid_enabled(idxd))
-		idxd_disable_sva(pdev);
 	return rc;
 }
 
@@ -715,8 +694,6 @@ static void idxd_cleanup(struct idxd_device *idxd)
 	idxd_cleanup_internals(idxd);
 	if (device_pasid_enabled(idxd))
 		idxd_disable_system_pasid(idxd);
-	if (device_user_pasid_enabled(idxd))
-		idxd_disable_sva(idxd->pdev);
 }
 
 /*
@@ -1248,8 +1225,6 @@ static void idxd_remove(struct pci_dev *pdev)
 	free_irq(irq_entry->vector, irq_entry);
 	pci_free_irq_vectors(pdev);
 	pci_iounmap(pdev, idxd->reg_base);
-	if (device_user_pasid_enabled(idxd))
-		idxd_disable_sva(pdev);
 	pci_disable_device(pdev);
 	destroy_workqueue(idxd->wq);
 	perfmon_pmu_remove(idxd);
-- 
2.43.0

[PATCH 10/12] uacce: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Lu Baolu]

None of the drivers implement anything for IOMMU_DEV_FEAT_IOPF anymore,
remove it to avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/misc/uacce/uacce.c | 31 -------------------------------
 1 file changed, 31 deletions(-)

diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c
index 2a1db2abeeca..42e7d2a2a90c 100644
--- a/drivers/misc/uacce/uacce.c
+++ b/drivers/misc/uacce/uacce.c
@@ -465,31 +465,6 @@ static void uacce_release(struct device *dev)
 	kfree(uacce);
 }
 
-static unsigned int uacce_enable_sva(struct device *parent, unsigned int flags)
-{
-	int ret;
-
-	if (!(flags & UACCE_DEV_SVA))
-		return flags;
-
-	flags &= ~UACCE_DEV_SVA;
-
-	ret = iommu_dev_enable_feature(parent, IOMMU_DEV_FEAT_IOPF);
-	if (ret) {
-		dev_err(parent, "failed to enable IOPF feature! ret = %pe\n", ERR_PTR(ret));
-		return flags;
-	}
-	return flags | UACCE_DEV_SVA;
-}
-
-static void uacce_disable_sva(struct uacce_device *uacce)
-{
-	if (!(uacce->flags & UACCE_DEV_SVA))
-		return;
-
-	iommu_dev_disable_feature(uacce->parent, IOMMU_DEV_FEAT_IOPF);
-}
-
 /**
  * uacce_alloc() - alloc an accelerator
  * @parent: pointer of uacce parent device
@@ -509,8 +484,6 @@ struct uacce_device *uacce_alloc(struct device *parent,
 	if (!uacce)
 		return ERR_PTR(-ENOMEM);
 
-	flags = uacce_enable_sva(parent, flags);
-
 	uacce->parent = parent;
 	uacce->flags = flags;
 	uacce->ops = interface->ops;
@@ -533,7 +506,6 @@ struct uacce_device *uacce_alloc(struct device *parent,
 	return uacce;
 
 err_with_uacce:
-	uacce_disable_sva(uacce);
 	kfree(uacce);
 	return ERR_PTR(ret);
 }
@@ -596,9 +568,6 @@ void uacce_remove(struct uacce_device *uacce)
 		unmap_mapping_range(q->mapping, 0, 0, 1);
 	}
 
-	/* disable sva now since no opened queues */
-	uacce_disable_sva(uacce);
-
 	if (uacce->cdev)
 		cdev_device_del(uacce->cdev, &uacce->dev);
 	xa_erase(&uacce_xa, uacce->dev_id);
-- 
2.43.0

[PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Lu Baolu]

The iopf enablement has been moved to the iommu drivers. It is unnecessary
for iommufd to handle iopf enablement. Remove the iopf enablement logic to
avoid duplication.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommufd/device.c          |   1 -
 drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
 drivers/iommu/iommufd/iommufd_private.h |   3 -
 3 files changed, 28 insertions(+), 87 deletions(-)

diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index dfd0898fb6c1..47e36456b438 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -215,7 +215,6 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
 	refcount_inc(&idev->obj.users);
 	/* igroup refcount moves into iommufd_device */
 	idev->igroup = igroup;
-	mutex_init(&idev->iopf_lock);
 
 	/*
 	 * If the caller fails after this success it must call
diff --git a/drivers/iommu/iommufd/fault.c b/drivers/iommu/iommufd/fault.c
index d9a937450e55..4776c632cff2 100644
--- a/drivers/iommu/iommufd/fault.c
+++ b/drivers/iommu/iommufd/fault.c
@@ -17,49 +17,6 @@
 #include "../iommu-priv.h"
 #include "iommufd_private.h"
 
-static int iommufd_fault_iopf_enable(struct iommufd_device *idev)
-{
-	struct device *dev = idev->dev;
-	int ret;
-
-	/*
-	 * Once we turn on PCI/PRI support for VF, the response failure code
-	 * should not be forwarded to the hardware due to PRI being a shared
-	 * resource between PF and VFs. There is no coordination for this
-	 * shared capability. This waits for a vPRI reset to recover.
-	 */
-	if (dev_is_pci(dev)) {
-		struct pci_dev *pdev = to_pci_dev(dev);
-
-		if (pdev->is_virtfn && pci_pri_supported(pdev))
-			return -EINVAL;
-	}
-
-	mutex_lock(&idev->iopf_lock);
-	/* Device iopf has already been on. */
-	if (++idev->iopf_enabled > 1) {
-		mutex_unlock(&idev->iopf_lock);
-		return 0;
-	}
-
-	ret = iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_IOPF);
-	if (ret)
-		--idev->iopf_enabled;
-	mutex_unlock(&idev->iopf_lock);
-
-	return ret;
-}
-
-static void iommufd_fault_iopf_disable(struct iommufd_device *idev)
-{
-	mutex_lock(&idev->iopf_lock);
-	if (!WARN_ON(idev->iopf_enabled == 0)) {
-		if (--idev->iopf_enabled == 0)
-			iommu_dev_disable_feature(idev->dev, IOMMU_DEV_FEAT_IOPF);
-	}
-	mutex_unlock(&idev->iopf_lock);
-}
-
 static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
 				     struct iommufd_device *idev)
 {
@@ -82,20 +39,23 @@ static int __fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
 int iommufd_fault_domain_attach_dev(struct iommufd_hw_pagetable *hwpt,
 				    struct iommufd_device *idev)
 {
-	int ret;
-
 	if (!hwpt->fault)
 		return -EINVAL;
 
-	ret = iommufd_fault_iopf_enable(idev);
-	if (ret)
-		return ret;
+	/*
+	 * Once we turn on PCI/PRI support for VF, the response failure code
+	 * should not be forwarded to the hardware due to PRI being a shared
+	 * resource between PF and VFs. There is no coordination for this
+	 * shared capability. This waits for a vPRI reset to recover.
+	 */
+	if (dev_is_pci(idev->dev)) {
+		struct pci_dev *pdev = to_pci_dev(idev->dev);
 
-	ret = __fault_domain_attach_dev(hwpt, idev);
-	if (ret)
-		iommufd_fault_iopf_disable(idev);
+		if (pdev->is_virtfn && pci_pri_supported(pdev))
+			return -EINVAL;
+	}
 
-	return ret;
+	return __fault_domain_attach_dev(hwpt, idev);
 }
 
 static void iommufd_auto_response_faults(struct iommufd_hw_pagetable *hwpt,
@@ -155,13 +115,12 @@ void iommufd_fault_domain_detach_dev(struct iommufd_hw_pagetable *hwpt,
 	handle = iommufd_device_get_attach_handle(idev);
 	iommu_detach_group_handle(hwpt->domain, idev->igroup->group);
 	iommufd_auto_response_faults(hwpt, handle);
-	iommufd_fault_iopf_disable(idev);
 	kfree(handle);
 }
 
-static int __fault_domain_replace_dev(struct iommufd_device *idev,
-				      struct iommufd_hw_pagetable *hwpt,
-				      struct iommufd_hw_pagetable *old)
+int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
+				     struct iommufd_hw_pagetable *hwpt,
+				     struct iommufd_hw_pagetable *old)
 {
 	struct iommufd_attach_handle *handle, *curr = NULL;
 	int ret;
@@ -170,6 +129,19 @@ static int __fault_domain_replace_dev(struct iommufd_device *idev,
 		curr = iommufd_device_get_attach_handle(idev);
 
 	if (hwpt->fault) {
+		/*
+		 * Once we turn on PCI/PRI support for VF, the response failure code
+		 * should not be forwarded to the hardware due to PRI being a shared
+		 * resource between PF and VFs. There is no coordination for this
+		 * shared capability. This waits for a vPRI reset to recover.
+		 */
+		if (dev_is_pci(idev->dev)) {
+			struct pci_dev *pdev = to_pci_dev(idev->dev);
+
+			if (pdev->is_virtfn && pci_pri_supported(pdev))
+				return -EINVAL;
+		}
+
 		handle = kzalloc(sizeof(*handle), GFP_KERNEL);
 		if (!handle)
 			return -ENOMEM;
@@ -190,33 +162,6 @@ static int __fault_domain_replace_dev(struct iommufd_device *idev,
 	return ret;
 }
 
-int iommufd_fault_domain_replace_dev(struct iommufd_device *idev,
-				     struct iommufd_hw_pagetable *hwpt,
-				     struct iommufd_hw_pagetable *old)
-{
-	bool iopf_off = !hwpt->fault && old->fault;
-	bool iopf_on = hwpt->fault && !old->fault;
-	int ret;
-
-	if (iopf_on) {
-		ret = iommufd_fault_iopf_enable(idev);
-		if (ret)
-			return ret;
-	}
-
-	ret = __fault_domain_replace_dev(idev, hwpt, old);
-	if (ret) {
-		if (iopf_on)
-			iommufd_fault_iopf_disable(idev);
-		return ret;
-	}
-
-	if (iopf_off)
-		iommufd_fault_iopf_disable(idev);
-
-	return 0;
-}
-
 void iommufd_fault_destroy(struct iommufd_object *obj)
 {
 	struct iommufd_fault *fault = container_of(obj, struct iommufd_fault, obj);
diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
index 0b1bafc7fd99..0eb3779db156 100644
--- a/drivers/iommu/iommufd/iommufd_private.h
+++ b/drivers/iommu/iommufd/iommufd_private.h
@@ -399,9 +399,6 @@ struct iommufd_device {
 	/* always the physical device */
 	struct device *dev;
 	bool enforce_cache_coherency;
-	/* protect iopf_enabled counter */
-	struct mutex iopf_lock;
-	unsigned int iopf_enabled;
 };
 
 static inline struct iommufd_device *
-- 
2.43.0

[PATCH 12/12] iommu: Remove iommu_dev_enable/disable_feature()

[Lu Baolu]

No external drivers use these interfaces anymore. Furthermore, no existing
iommu drivers implement anything in the callbacks. Remove them to avoid
dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/amd/iommu.c                   | 32 -------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 34 ---------------------
 drivers/iommu/intel/iommu.c                 | 25 ---------------
 drivers/iommu/iommu.c                       | 32 -------------------
 include/linux/iommu.h                       | 28 -----------------
 5 files changed, 151 deletions(-)

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index e3653bdfcd7d..8d74afa552fb 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2983,36 +2983,6 @@ static const struct iommu_dirty_ops amd_dirty_ops = {
 	.read_and_clear_dirty = amd_iommu_read_and_clear_dirty,
 };
 
-static int amd_iommu_dev_enable_feature(struct device *dev,
-					enum iommu_dev_features feat)
-{
-	int ret = 0;
-
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
-	return ret;
-}
-
-static int amd_iommu_dev_disable_feature(struct device *dev,
-					 enum iommu_dev_features feat)
-{
-	int ret = 0;
-
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		break;
-	default:
-		ret = -EINVAL;
-		break;
-	}
-	return ret;
-}
-
 const struct iommu_ops amd_iommu_ops = {
 	.capable = amd_iommu_capable,
 	.blocked_domain = &blocked_domain,
@@ -3026,8 +2996,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.get_resv_regions = amd_iommu_get_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
 	.def_domain_type = amd_iommu_def_domain_type,
-	.dev_enable_feat = amd_iommu_dev_enable_feature,
-	.dev_disable_feat = amd_iommu_dev_disable_feature,
 	.page_response = amd_iommu_page_response,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev	= amd_iommu_attach_device,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index ee945a9db641..28e67a9e3861 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3617,38 +3617,6 @@ static void arm_smmu_get_resv_regions(struct device *dev,
 	iommu_dma_get_resv_regions(dev, head);
 }
 
-static int arm_smmu_dev_enable_feature(struct device *dev,
-				       enum iommu_dev_features feat)
-{
-	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-
-	if (!master)
-		return -ENODEV;
-
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		return 0;
-	default:
-		return -EINVAL;
-	}
-}
-
-static int arm_smmu_dev_disable_feature(struct device *dev,
-					enum iommu_dev_features feat)
-{
-	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-
-	if (!master)
-		return -EINVAL;
-
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		return 0;
-	default:
-		return -EINVAL;
-	}
-}
-
 /*
  * HiSilicon PCIe tune and trace device can be used to trace TLP headers on the
  * PCIe link and save the data to memory by DMA. The hardware is restricted to
@@ -3681,8 +3649,6 @@ static struct iommu_ops arm_smmu_ops = {
 	.device_group		= arm_smmu_device_group,
 	.of_xlate		= arm_smmu_of_xlate,
 	.get_resv_regions	= arm_smmu_get_resv_regions,
-	.dev_enable_feat	= arm_smmu_dev_enable_feature,
-	.dev_disable_feat	= arm_smmu_dev_disable_feature,
 	.page_response		= arm_smmu_page_response,
 	.def_domain_type	= arm_smmu_def_domain_type,
 	.viommu_alloc		= arm_vsmmu_alloc,
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 20d07f5fea85..aad11e76ac40 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3920,29 +3920,6 @@ void intel_iommu_disable_iopf(struct device *dev)
 	iopf_queue_remove_device(iommu->iopf_queue, dev);
 }
 
-static int
-intel_iommu_dev_enable_feat(struct device *dev, enum iommu_dev_features feat)
-{
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		return 0;
-	default:
-		return -ENODEV;
-	}
-}
-
-static int
-intel_iommu_dev_disable_feat(struct device *dev, enum iommu_dev_features feat)
-{
-	switch (feat) {
-	case IOMMU_DEV_FEAT_IOPF:
-		return 0;
-
-	default:
-		return -ENODEV;
-	}
-}
-
 static bool intel_iommu_is_attach_deferred(struct device *dev)
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
@@ -4387,8 +4364,6 @@ const struct iommu_ops intel_iommu_ops = {
 	.release_device		= intel_iommu_release_device,
 	.get_resv_regions	= intel_iommu_get_resv_regions,
 	.device_group		= intel_iommu_device_group,
-	.dev_enable_feat	= intel_iommu_dev_enable_feat,
-	.dev_disable_feat	= intel_iommu_dev_disable_feat,
 	.is_attach_deferred	= intel_iommu_is_attach_deferred,
 	.def_domain_type	= device_def_domain_type,
 	.pgsize_bitmap		= SZ_4K,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 870c3cdbd0f6..c86ab80dce4d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -2877,38 +2877,6 @@ int iommu_fwspec_add_ids(struct device *dev, const u32 *ids, int num_ids)
 }
 EXPORT_SYMBOL_GPL(iommu_fwspec_add_ids);
 
-/*
- * Per device IOMMU features.
- */
-int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
-{
-	if (dev_has_iommu(dev)) {
-		const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-		if (ops->dev_enable_feat)
-			return ops->dev_enable_feat(dev, feat);
-	}
-
-	return -ENODEV;
-}
-EXPORT_SYMBOL_GPL(iommu_dev_enable_feature);
-
-/*
- * The device drivers should do the necessary cleanups before calling this.
- */
-int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
-{
-	if (dev_has_iommu(dev)) {
-		const struct iommu_ops *ops = dev_iommu_ops(dev);
-
-		if (ops->dev_disable_feat)
-			return ops->dev_disable_feat(dev, feat);
-	}
-
-	return -EBUSY;
-}
-EXPORT_SYMBOL_GPL(iommu_dev_disable_feature);
-
 /**
  * iommu_setup_default_domain - Set the default_domain for the group
  * @group: Group to change
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1d0dde49168d..127f99acab5f 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -301,16 +301,6 @@ struct iommu_iort_rmr_data {
 	u32 num_sids;
 };
 
-/**
- * enum iommu_dev_features - Per device IOMMU features
- * @IOMMU_DEV_FEAT_IOPF: I/O Page Faults such as PRI or Stall.
- *
- * Device drivers enable a feature using iommu_dev_enable_feature().
- */
-enum iommu_dev_features {
-	IOMMU_DEV_FEAT_IOPF,
-};
-
 #define IOMMU_NO_PASID	(0U) /* Reserved for DMA w/o PASID */
 #define IOMMU_FIRST_GLOBAL_PASID	(1U) /*starting range for allocation */
 #define IOMMU_PASID_INVALID	(-1U)
@@ -630,9 +620,6 @@ struct iommu_ops {
 	bool (*is_attach_deferred)(struct device *dev);
 
 	/* Per device IOMMU features */
-	int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
-	int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
-
 	void (*page_response)(struct device *dev, struct iopf_fault *evt,
 			      struct iommu_page_response *msg);
 
@@ -1102,9 +1089,6 @@ void dev_iommu_priv_set(struct device *dev, void *priv);
 extern struct mutex iommu_probe_device_lock;
 int iommu_probe_device(struct device *dev);
 
-int iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features f);
-int iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features f);
-
 int iommu_device_use_default_domain(struct device *dev);
 void iommu_device_unuse_default_domain(struct device *dev);
 
@@ -1393,18 +1377,6 @@ static inline int iommu_fwspec_add_ids(struct device *dev, u32 *ids,
 	return -ENODEV;
 }
 
-static inline int
-iommu_dev_enable_feature(struct device *dev, enum iommu_dev_features feat)
-{
-	return -ENODEV;
-}
-
-static inline int
-iommu_dev_disable_feature(struct device *dev, enum iommu_dev_features feat)
-{
-	return -ENODEV;
-}
-
 static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
 {
 	return NULL;
-- 
2.43.0

Re: [PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Nicolin Chen]

On Fri, Feb 14, 2025 at 02:11:03PM +0800, Lu Baolu wrote:
> The iopf enablement has been moved to the iommu drivers. It is unnecessary
> for iommufd to handle iopf enablement. Remove the iopf enablement logic to
> avoid duplication.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommufd/device.c          |   1 -
>  drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
>  drivers/iommu/iommufd/iommufd_private.h |   3 -
>  3 files changed, 28 insertions(+), 87 deletions(-)

This is in conflict with my fault patches that Jason just took
a couple days ago:
https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?h=for-next

I think it needs a rebase, perhaps on the branch mentioned here:
https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/

Thanks
Nicolin

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

Hi, Baolu

On Fri, 14 Feb 2025 at 14:11, Lu Baolu <baolu.lu@linux.intel.com> wrote:
>
> The new method for driver fault reporting support relies on the domain
> to specify a iopf_handler. The driver should detect this and setup the
> HW when fault capable domains are attached.
>
> Move SMMUv3 to use this method and have VT-D validate support during
> attach so that all three fault capable drivers have a no-op FEAT_SVA and
> _IOPF. Then remove them.
>
> This was initiated by Jason. I'm following up to remove FEAT_IOPF and
> further clean up.
>
> The whole series is also available at github:
> https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1

I quickly test this branch

1. host test is OK

2. qemu boot one device, test ok,
though reports this when guest bootup.
vfio-pci xxx: resetting
vfio-pci xxx: reset done

3. qemu boot multi device,  test fails, host kernel reports [Hardware Error]
qemu can boot no problem
Test fails.

Will do more check without these patches.

Thanks

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

On Fri, 14 Feb 2025 at 16:43, Zhangfei Gao <zhangfei.gao@linaro.org> wrote:
>
> Hi, Baolu
>
> On Fri, 14 Feb 2025 at 14:11, Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >
> > The new method for driver fault reporting support relies on the domain
> > to specify a iopf_handler. The driver should detect this and setup the
> > HW when fault capable domains are attached.
> >
> > Move SMMUv3 to use this method and have VT-D validate support during
> > attach so that all three fault capable drivers have a no-op FEAT_SVA and
> > _IOPF. Then remove them.
> >
> > This was initiated by Jason. I'm following up to remove FEAT_IOPF and
> > further clean up.
> >
> > The whole series is also available at github:
> > https://github.com/LuBaolu/intel-iommu/commits/iommu_no_feat-v1
>
> I quickly test this branch
>
> 1. host test is OK
>
> 2. qemu boot one device, test ok,
> though reports this when guest bootup.
> vfio-pci xxx: resetting
> vfio-pci xxx: reset done
>
> 3. qemu boot multi device,  test fails, host kernel reports [Hardware Error]
> qemu can boot no problem
> Test fails.
>
> Will do more checks without these patches.

Test on 6.14-rc2 without this patch set

1. qemu boot multi device,  test OK

2. log "vfio-pci xxx: resetting/reset done" also exists.

Thanks

Re: [PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Vinod Koul]

On 14-02-25, 14:11, Lu Baolu wrote:
> The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a no-op.
> It will also be removed from the iommu driver in the subsequent patch.
> Remove it to avoid dead code.

Acked-by: Vinod Koul <vkoul@kernel.org>


-- 
~Vinod

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 04:43:12PM +0800, Zhangfei Gao wrote:
> 3. qemu boot multi device,  test fails, host kernel reports
> [Hardware Error]

More details? Can you bisect?

Thanks,
Jason

Re: [PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Dave Jiang]

On 2/13/25 11:11 PM, Lu Baolu wrote:
> The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a no-op.
> It will also be removed from the iommu driver in the subsequent patch.
> Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  drivers/dma/idxd/init.c | 37 ++++++-------------------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 1e5038cca22c..d44944195807 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -626,21 +626,6 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
>  	idxd->pasid = IOMMU_PASID_INVALID;
>  }
>  
> -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;
> -	return 0;
> -}
> -
> -static void idxd_disable_sva(struct pci_dev *pdev)
> -{
> -	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
> -}
> -
>  static int idxd_probe(struct idxd_device *idxd)
>  {
>  	struct pci_dev *pdev = idxd->pdev;
> @@ -655,17 +640,13 @@ static int idxd_probe(struct idxd_device *idxd)
>  	dev_dbg(dev, "IDXD reset complete\n");
>  
>  	if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && 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);
> +		set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
>  
> -			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);
> -		}
> +		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);
>  	} else if (!sva) {
>  		dev_warn(dev, "User forced SVA off via module param.\n");
>  	}
> @@ -703,8 +684,6 @@ static int idxd_probe(struct idxd_device *idxd)
>   err:
>  	if (device_pasid_enabled(idxd))
>  		idxd_disable_system_pasid(idxd);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(pdev);
>  	return rc;
>  }
>  
> @@ -715,8 +694,6 @@ static void idxd_cleanup(struct idxd_device *idxd)
>  	idxd_cleanup_internals(idxd);
>  	if (device_pasid_enabled(idxd))
>  		idxd_disable_system_pasid(idxd);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(idxd->pdev);
>  }
>  
>  /*
> @@ -1248,8 +1225,6 @@ static void idxd_remove(struct pci_dev *pdev)
>  	free_irq(irq_entry->vector, irq_entry);
>  	pci_free_irq_vectors(pdev);
>  	pci_iounmap(pdev, idxd->reg_base);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(pdev);
>  	pci_disable_device(pdev);
>  	destroy_workqueue(idxd->wq);
>  	perfmon_pmu_remove(idxd);

Re: [PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Baolu Lu]

On 2/14/25 15:06, Nicolin Chen wrote:
> On Fri, Feb 14, 2025 at 02:11:03PM +0800, Lu Baolu wrote:
>> The iopf enablement has been moved to the iommu drivers. It is unnecessary
>> for iommufd to handle iopf enablement. Remove the iopf enablement logic to
>> avoid duplication.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommufd/device.c          |   1 -
>>   drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
>>   drivers/iommu/iommufd/iommufd_private.h |   3 -
>>   3 files changed, 28 insertions(+), 87 deletions(-)
> This is in conflict with my fault patches that Jason just took
> a couple days ago:
> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/? 
> h=for-next
> 
> I think it needs a rebase, perhaps on the branch mentioned here:
> https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/

Yes, sure. I will rebase it in the next version to avoid the conflict.

Thanks,
baolu

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

Hi, Jason

On Fri, 14 Feb 2025 at 20:56, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Fri, Feb 14, 2025 at 04:43:12PM +0800, Zhangfei Gao wrote:
> > 3. qemu boot multi device,  test fails, host kernel reports
> > [Hardware Error]
>
> More details? Can you bisect?

It does not relate to multi devices, one device also happens when user
page fault triggers.

iopf_queue_remove_device is called.
rcu_assign_pointer(param->fault_param, NULL);

call trace
[  304.961312] Call trace:
[  304.961314]  show_stack+0x20/0x38 (C)
[  304.961319]  dump_stack_lvl+0xc0/0xd0
[  304.961324]  dump_stack+0x18/0x28
[  304.961327]  iopf_queue_remove_device+0xb0/0x1f0
[  304.961331]  arm_smmu_remove_master_domain+0x204/0x250
[  304.961336]  arm_smmu_attach_commit+0x64/0x100
[  304.961338]  arm_smmu_attach_dev_nested+0x104/0x1a8
[  304.961340]  __iommu_attach_device+0x2c/0x110
[  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
[  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
[  304.961347]  iommu_replace_group_handle+0x9c/0x150
[  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
[  304.961353]  iommufd_device_do_replace+0x190/0x3c0
[  304.961355]  iommufd_device_change_pt+0x270/0x688
[  304.961357]  iommufd_device_replace+0x20/0x38
[  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
[  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
[  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990


When page fault triggers:

[ 1016.383578] ------------[ cut here ]-----------
[ 1016.388184] WARNING: CPU: 35 PID: 717 at
drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
[ 1016.398057] Modules linked in:
[ 1016.401102] CPU: 35 UID: 0 PID: 717 Comm: irq/31-arm-smmu Not
tainted 6.14.0-rc2-g4384d0f9bd24-dirty #19
[ 1016.410538] Hardware name: Huawei TaiShan 200 (Model
2280)/BC82AMDD, BIOS 2280-V2 CS V5.B133.01 03/25/2021
[ 1016.420147] pstate: a0400009 (NzCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 1016.427077] pc : iommu_report_device_fault+0x2c8/0x470
[ 1016.432194] lr : iommu_report_device_fault+0x2c8/0x470
[ 1016.437309] sp : ffff80008c7fbb20
[ 1016.440609] x29: ffff80008c7fbb20 x28: ffff4f25c95aa6ac x27: ffffb5d5fd356818
[ 1016.447714] x26: ffffb5d600a0b8a0 x25: 0000000000000000 x24: ffff6f454371d0a0
[ 1016.454819] x23: ffffb5d5ff9544e0 x22: ffff4f4545796040 x21: ffff6f25466d80c8
[ 1016.461923] x20: ffff80008c7fbc88 x19: ffff6f25631a2780 x18: 000000000000ffff
[ 1016.469028] x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001
[ 1016.476132] x14: 0000000000000001 x13: 0000000000000001 x12: ffff0001fffedf00
[ 1016.483236] x11: 0000000000000000 x10: 0000000000000c80 x9 : ffffb5d5fde985f0
[ 1016.490340] x8 : ffff4f4545796d20 x7 : 0000aaaada7af000 x6 : 0000000000000001
[ 1016.497444] x5 : ffffb5d600a0b000 x4 : ffffb5d600a0be70 x3 : 0000000000000000
[ 1016.504548] x2 : ffff4f4545796040 x1 : 0000000000000000 x0 : 0000000000000000
[ 1016.511652] Call trace:
[ 1016.514088]  iommu_report_device_fault+0x2c8/0x470 (P)
[ 1016.519205]  arm_smmu_handle_event+0x100/0x170
[ 1016.523633]  arm_smmu_evtq_thread+0x1e4/0x4a0
[ 1016.527973]  irq_thread_fn+0x34/0xb8
[ 1016.531534]  irq_thread+0x160/0x310
[ 1016.535008]  kthread+0x124/0x220
[ 1016.538225]  ret_from_fork+0x10/0x20
[ 1016.541786] ---[ end trace 0000000000000000 ]---
[ 1016.546403] arm-smmu-v3 arm-smmu-v3.3.auto: event 0x10 received:
[ 1016.552474] arm-smmu-v3 arm-smmu-v3.3.auto:  0x0000750100001810
[ 1016.558453] arm-smmu-v3 arm-smmu-v3.3.auto:  0x0000120080000176
[ 1016.564430] arm-smmu-v3 arm-smmu-v3.3.auto:  0x0000aaaada7af000
[ 1016.570406] arm-smmu-v3 arm-smmu-v3.3.auto:  0x0000aaaada7af000
[ 1016.576380] arm-smmu-v3 arm-smmu-v3.3.auto: event: F_TRANSLATION
client: 0000:75:00.1 sid: 0x7501 ssid: 0x1 iova: 0xaaaada7af000 ipa:
0xaaaada7af000
[ 1016.589700] arm-smmu-v3 arm-smmu-v3.3.auto: unpriv data write s1
"Input address caused fault" stall stag: 0x176


Thanks

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Baolu Lu]

On 2/15/25 16:11, Zhangfei Gao wrote:
> It does not relate to multi devices, one device also happens when user
> page fault triggers.
> 
> iopf_queue_remove_device is called.
> rcu_assign_pointer(param->fault_param, NULL);
> 
> call trace
> [  304.961312] Call trace:
> [  304.961314]  show_stack+0x20/0x38 (C)
> [  304.961319]  dump_stack_lvl+0xc0/0xd0
> [  304.961324]  dump_stack+0x18/0x28
> [  304.961327]  iopf_queue_remove_device+0xb0/0x1f0
> [  304.961331]  arm_smmu_remove_master_domain+0x204/0x250
> [  304.961336]  arm_smmu_attach_commit+0x64/0x100
> [  304.961338]  arm_smmu_attach_dev_nested+0x104/0x1a8
> [  304.961340]  __iommu_attach_device+0x2c/0x110
> [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
> [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
> [  304.961347]  iommu_replace_group_handle+0x9c/0x150
> [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
> [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
> [  304.961355]  iommufd_device_change_pt+0x270/0x688
> [  304.961357]  iommufd_device_replace+0x20/0x38
> [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
> [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
> [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
> 
> 
> When page fault triggers:
> 
> [ 1016.383578] ------------[ cut here ]-----------
> [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470

It's likely that iopf_queue_add_device() was not called for this device.

Thanks,
baolu

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

On Sat, 15 Feb 2025 at 18:09, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2/15/25 16:11, Zhangfei Gao wrote:
> > It does not relate to multi devices, one device also happens when user
> > page fault triggers.
> >
> > iopf_queue_remove_device is called.
> > rcu_assign_pointer(param->fault_param, NULL);
> >
> > call trace
> > [  304.961312] Call trace:
> > [  304.961314]  show_stack+0x20/0x38 (C)
> > [  304.961319]  dump_stack_lvl+0xc0/0xd0
> > [  304.961324]  dump_stack+0x18/0x28
> > [  304.961327]  iopf_queue_remove_device+0xb0/0x1f0
> > [  304.961331]  arm_smmu_remove_master_domain+0x204/0x250
> > [  304.961336]  arm_smmu_attach_commit+0x64/0x100
> > [  304.961338]  arm_smmu_attach_dev_nested+0x104/0x1a8
> > [  304.961340]  __iommu_attach_device+0x2c/0x110
> > [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
> > [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
> > [  304.961347]  iommu_replace_group_handle+0x9c/0x150
> > [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
> > [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
> > [  304.961355]  iommufd_device_change_pt+0x270/0x688
> > [  304.961357]  iommufd_device_replace+0x20/0x38
> > [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
> > [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
> > [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
> >
> >
> > When page fault triggers:
> >
> > [ 1016.383578] ------------[ cut here ]-----------
> > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
>
> It's likely that iopf_queue_add_device() was not called for this device.

iopf_queue_add_device is called, but quickly iopf_queue_remove_device
is called during guest bootup.
Then fault_param is set to NULL.

arm_smmu_attach_commit
arm_smmu_remove_master_domain
// newly added in the first patch
       if (master_domain) {
                  if (master_domain->using_iopf)
                          arm_smmu_disable_iopf(master); ->
iopf_queue_remove_device
                  kfree(master_domain);
          }

As a comparison, without this patchset, only iopf_queue_add_device is
called, not call iopf_queue_remove_device

Thanks

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Baolu Lu]

On 2/15/25 19:35, Zhangfei Gao wrote:
> On Sat, 15 Feb 2025 at 18:09, Baolu Lu<baolu.lu@linux.intel.com> wrote:
>> On 2/15/25 16:11, Zhangfei Gao wrote:
>>> It does not relate to multi devices, one device also happens when user
>>> page fault triggers.
>>>
>>> iopf_queue_remove_device is called.
>>> rcu_assign_pointer(param->fault_param, NULL);
>>>
>>> call trace
>>> [  304.961312] Call trace:
>>> [  304.961314]  show_stack+0x20/0x38 (C)
>>> [  304.961319]  dump_stack_lvl+0xc0/0xd0
>>> [  304.961324]  dump_stack+0x18/0x28
>>> [  304.961327]  iopf_queue_remove_device+0xb0/0x1f0
>>> [  304.961331]  arm_smmu_remove_master_domain+0x204/0x250
>>> [  304.961336]  arm_smmu_attach_commit+0x64/0x100
>>> [  304.961338]  arm_smmu_attach_dev_nested+0x104/0x1a8
>>> [  304.961340]  __iommu_attach_device+0x2c/0x110
>>> [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
>>> [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
>>> [  304.961347]  iommu_replace_group_handle+0x9c/0x150
>>> [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
>>> [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
>>> [  304.961355]  iommufd_device_change_pt+0x270/0x688
>>> [  304.961357]  iommufd_device_replace+0x20/0x38
>>> [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
>>> [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
>>> [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
>>>
>>>
>>> When page fault triggers:
>>>
>>> [ 1016.383578] ------------[ cut here ]-----------
>>> [ 1016.388184] WARNING: CPU: 35 PID: 717 at
>>> drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
>> It's likely that iopf_queue_add_device() was not called for this device.
> iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> is called during guest bootup.
> Then fault_param is set to NULL.
> 
> arm_smmu_attach_commit
> arm_smmu_remove_master_domain
> // newly added in the first patch
>         if (master_domain) {
>                    if (master_domain->using_iopf)

It seems the above check is incorrect. We only need to disable iopf when
an iopf-capable domain is about to be removed. Will the following
additional change make any difference?

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 28e67a9e3861..9b9ef738d070 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2822,7 +2822,7 @@ static void arm_smmu_remove_master_domain(struct 
arm_smmu_master *master,
         spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

         if (master_domain) {
-               if (master_domain->using_iopf)
+               if (domain->iopf_handler)
                         arm_smmu_disable_iopf(master);
                 kfree(master_domain);
         }

>                            arm_smmu_disable_iopf(master); ->
> iopf_queue_remove_device
>                    kfree(master_domain);
>            }
> 
> As a comparison, without this patchset, only iopf_queue_add_device is
> called, not call iopf_queue_remove_device

Thanks,
baolu

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

On Tue, 18 Feb 2025 at 11:00, Baolu Lu <baolu.lu@linux.intel.com> wrote:
>
> On 2/15/25 19:35, Zhangfei Gao wrote:
> > On Sat, 15 Feb 2025 at 18:09, Baolu Lu<baolu.lu@linux.intel.com> wrote:
> >> On 2/15/25 16:11, Zhangfei Gao wrote:
> >>> It does not relate to multi devices, one device also happens when user
> >>> page fault triggers.
> >>>
> >>> iopf_queue_remove_device is called.
> >>> rcu_assign_pointer(param->fault_param, NULL);
> >>>
> >>> call trace
> >>> [  304.961312] Call trace:
> >>> [  304.961314]  show_stack+0x20/0x38 (C)
> >>> [  304.961319]  dump_stack_lvl+0xc0/0xd0
> >>> [  304.961324]  dump_stack+0x18/0x28
> >>> [  304.961327]  iopf_queue_remove_device+0xb0/0x1f0
> >>> [  304.961331]  arm_smmu_remove_master_domain+0x204/0x250
> >>> [  304.961336]  arm_smmu_attach_commit+0x64/0x100
> >>> [  304.961338]  arm_smmu_attach_dev_nested+0x104/0x1a8
> >>> [  304.961340]  __iommu_attach_device+0x2c/0x110
> >>> [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
> >>> [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
> >>> [  304.961347]  iommu_replace_group_handle+0x9c/0x150
> >>> [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
> >>> [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
> >>> [  304.961355]  iommufd_device_change_pt+0x270/0x688
> >>> [  304.961357]  iommufd_device_replace+0x20/0x38
> >>> [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
> >>> [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
> >>> [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
> >>>
> >>>
> >>> When page fault triggers:
> >>>
> >>> [ 1016.383578] ------------[ cut here ]-----------
> >>> [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> >>> drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> >> It's likely that iopf_queue_add_device() was not called for this device.
> > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > is called during guest bootup.
> > Then fault_param is set to NULL.
> >
> > arm_smmu_attach_commit
> > arm_smmu_remove_master_domain
> > // newly added in the first patch
> >         if (master_domain) {
> >                    if (master_domain->using_iopf)
>
> It seems the above check is incorrect. We only need to disable iopf when
> an iopf-capable domain is about to be removed. Will the following
> additional change make any difference?
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 28e67a9e3861..9b9ef738d070 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2822,7 +2822,7 @@ static void arm_smmu_remove_master_domain(struct
> arm_smmu_master *master,
>          spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>
>          if (master_domain) {
> -               if (master_domain->using_iopf)
> +               if (domain->iopf_handler)
>                          arm_smmu_disable_iopf(master);
>                  kfree(master_domain);
>          }

Yes, good idea, using this can solve the issue.

Thanks

Re: [PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Jason Gunthorpe]

On Sat, Feb 15, 2025 at 02:32:32PM +0800, Baolu Lu wrote:
> On 2/14/25 15:06, Nicolin Chen wrote:
> > On Fri, Feb 14, 2025 at 02:11:03PM +0800, Lu Baolu wrote:
> > > The iopf enablement has been moved to the iommu drivers. It is unnecessary
> > > for iommufd to handle iopf enablement. Remove the iopf enablement logic to
> > > avoid duplication.
> > > 
> > > Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> > > ---
> > >   drivers/iommu/iommufd/device.c          |   1 -
> > >   drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
> > >   drivers/iommu/iommufd/iommufd_private.h |   3 -
> > >   3 files changed, 28 insertions(+), 87 deletions(-)
> > This is in conflict with my fault patches that Jason just took
> > a couple days ago:
> > https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?
> > h=for-next
> > 
> > I think it needs a rebase, perhaps on the branch mentioned here:
> > https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/
> 
> Yes, sure. I will rebase it in the next version to avoid the conflict.

That's troublesome, I think just leave it so Joerg can pick it up. We
can figure out what to do with the conflict later.

Jason

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Jason Gunthorpe]

On Tue, Feb 18, 2025 at 10:57:30AM +0800, Baolu Lu wrote:
> > > > [  304.961340]  __iommu_attach_device+0x2c/0x110
> > > > [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
> > > > [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
> > > > [  304.961347]  iommu_replace_group_handle+0x9c/0x150
> > > > [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
> > > > [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
> > > > [  304.961355]  iommufd_device_change_pt+0x270/0x688
> > > > [  304.961357]  iommufd_device_replace+0x20/0x38
> > > > [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
> > > > [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
> > > > [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
> > > > 
> > > > 
> > > > When page fault triggers:
> > > > 
> > > > [ 1016.383578] ------------[ cut here ]-----------
> > > > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > > > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> > > It's likely that iopf_queue_add_device() was not called for this device.
> > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > is called during guest bootup.
> > Then fault_param is set to NULL.
> > 
> > arm_smmu_attach_commit
> > arm_smmu_remove_master_domain
> > // newly added in the first patch
> >         if (master_domain) {
> >                    if (master_domain->using_iopf)
> 
> It seems the above check is incorrect. We only need to disable iopf when
> an iopf-capable domain is about to be removed. Will the following
> additional change make any difference?

The check looks right, it should only disable if it was enabled? The
refcounting is what keep track of the 'about to be removed' and it
should  be that using_iopf and domain->iopf_handler are mostly the
same.

Hmm, I think the issue is related to nested

to_smmu_domain_devices() returns the S2 parent for the nesting domain
always

Which means the smmu_domain->devices list (on the s2) will end up with
two entries for the same SID during the replace operation at VM boot,
one with faulting and one without.

I think that arm_smmu_remove_master_domain() will end up removing the
wrong master_domain because arm_smmu_find_master_domain() can't tell
the two apart.

When I wrote this there was no nested and the list devices list was
unique to each domain, so everything inside was the same.

Like below?

Jason

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index b14f1d0ee7076b..dc8708b414468e 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2710,10 +2710,9 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 	pci_disable_pasid(pdev);
 }
 
-static struct arm_smmu_master_domain *
-arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
-			    struct arm_smmu_master *master,
-			    ioasid_t ssid, bool nested_ats_flush)
+static struct arm_smmu_master_domain *arm_smmu_find_master_domain(
+	struct arm_smmu_domain *smmu_domain, struct iommu_domain *domain,
+	struct arm_smmu_master *master, ioasid_t ssid, bool nested_ats_flush)
 {
 	struct arm_smmu_master_domain *master_domain;
 
@@ -2722,6 +2721,7 @@ arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
 	list_for_each_entry(master_domain, &smmu_domain->devices,
 			    devices_elm) {
 		if (master_domain->master == master &&
+		    master_domain->domain == domain &&
 		    master_domain->ssid == ssid &&
 		    master_domain->nested_ats_flush == nested_ats_flush)
 			return master_domain;
@@ -2812,8 +2812,8 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
 		nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	master_domain = arm_smmu_find_master_domain(smmu_domain, master, ssid,
-						    nested_ats_flush);
+	master_domain = arm_smmu_find_master_domain(smmu_domain, domain, master,
+						    ssid, nested_ats_flush);
 	if (master_domain) {
 		list_del(&master_domain->devices_elm);
 		if (master->ats_enabled)
@@ -2889,6 +2889,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
 		master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
 		if (!master_domain)
 			return -ENOMEM;
+		master_domain->domain = new_domain;
 		master_domain->master = master;
 		master_domain->ssid = state->ssid;
 		if (new_domain->type == IOMMU_DOMAIN_NESTED)
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 5653d7417db7d9..fe6b88affa4a60 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -907,6 +907,11 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
 struct arm_smmu_master_domain {
 	struct list_head devices_elm;
 	struct arm_smmu_master *master;
+	/*
+	 * For nested domains the master_domain is threaded onto the S2 parent,
+	 * this points to the IOMMU_DOMAIN_NESTED to disambiguate the masters.
+	 */
+	struct iommu_domain *domain;
 	ioasid_t ssid;
 	bool nested_ats_flush : 1;
 	bool using_iopf : 1;

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Zhangfei Gao]

Hi, Jason

On Tue, 18 Feb 2025 at 21:57, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Feb 18, 2025 at 10:57:30AM +0800, Baolu Lu wrote:
> > > > > [  304.961340]  __iommu_attach_device+0x2c/0x110
> > > > > [  304.961343]  __iommu_device_set_domain.isra.0+0x78/0xe0
> > > > > [  304.961345]  __iommu_group_set_domain_internal+0x78/0x160
> > > > > [  304.961347]  iommu_replace_group_handle+0x9c/0x150
> > > > > [  304.961350]  iommufd_fault_domain_replace_dev+0x88/0x120
> > > > > [  304.961353]  iommufd_device_do_replace+0x190/0x3c0
> > > > > [  304.961355]  iommufd_device_change_pt+0x270/0x688
> > > > > [  304.961357]  iommufd_device_replace+0x20/0x38
> > > > > [  304.961359]  vfio_iommufd_physical_attach_ioas+0x30/0x78
> > > > > [  304.961363]  vfio_df_ioctl_attach_pt+0xa8/0x188
> > > > > [  304.961366]  vfio_device_fops_unl_ioctl+0x310/0x990
> > > > >
> > > > >
> > > > > When page fault triggers:
> > > > >
> > > > > [ 1016.383578] ------------[ cut here ]-----------
> > > > > [ 1016.388184] WARNING: CPU: 35 PID: 717 at
> > > > > drivers/iommu/io-pgfault.c:231 iommu_report_device_fault+0x2c8/0x470
> > > > It's likely that iopf_queue_add_device() was not called for this device.
> > > iopf_queue_add_device is called, but quickly iopf_queue_remove_device
> > > is called during guest bootup.
> > > Then fault_param is set to NULL.
> > >
> > > arm_smmu_attach_commit
> > > arm_smmu_remove_master_domain
> > > // newly added in the first patch
> > >         if (master_domain) {
> > >                    if (master_domain->using_iopf)
> >
> > It seems the above check is incorrect. We only need to disable iopf when
> > an iopf-capable domain is about to be removed. Will the following
> > additional change make any difference?
>
> The check looks right, it should only disable if it was enabled? The
> refcounting is what keep track of the 'about to be removed' and it
> should  be that using_iopf and domain->iopf_handler are mostly the
> same.
>
> Hmm, I think the issue is related to nested
>
> to_smmu_domain_devices() returns the S2 parent for the nesting domain
> always
>
> Which means the smmu_domain->devices list (on the s2) will end up with
> two entries for the same SID during the replace operation at VM boot,
> one with faulting and one without.
>
> I think that arm_smmu_remove_master_domain() will end up removing the
> wrong master_domain because arm_smmu_find_master_domain() can't tell
> the two apart.
>
> When I wrote this there was no nested and the list devices list was
> unique to each domain, so everything inside was the same.
>
> Like below?
>
> Jason
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index b14f1d0ee7076b..dc8708b414468e 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2710,10 +2710,9 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>         pci_disable_pasid(pdev);
>  }
>
> -static struct arm_smmu_master_domain *
> -arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
> -                           struct arm_smmu_master *master,
> -                           ioasid_t ssid, bool nested_ats_flush)
> +static struct arm_smmu_master_domain *arm_smmu_find_master_domain(
> +       struct arm_smmu_domain *smmu_domain, struct iommu_domain *domain,
> +       struct arm_smmu_master *master, ioasid_t ssid, bool nested_ats_flush)
>  {
>         struct arm_smmu_master_domain *master_domain;
>
> @@ -2722,6 +2721,7 @@ arm_smmu_find_master_domain(struct arm_smmu_domain *smmu_domain,
>         list_for_each_entry(master_domain, &smmu_domain->devices,
>                             devices_elm) {
>                 if (master_domain->master == master &&
> +                   master_domain->domain == domain &&
>                     master_domain->ssid == ssid &&
>                     master_domain->nested_ats_flush == nested_ats_flush)
>                         return master_domain;
> @@ -2812,8 +2812,8 @@ static void arm_smmu_remove_master_domain(struct arm_smmu_master *master,
>                 nested_ats_flush = to_smmu_nested_domain(domain)->enable_ats;
>
>         spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -       master_domain = arm_smmu_find_master_domain(smmu_domain, master, ssid,
> -                                                   nested_ats_flush);
> +       master_domain = arm_smmu_find_master_domain(smmu_domain, domain, master,
> +                                                   ssid, nested_ats_flush);
>         if (master_domain) {
>                 list_del(&master_domain->devices_elm);
>                 if (master->ats_enabled)
> @@ -2889,6 +2889,7 @@ int arm_smmu_attach_prepare(struct arm_smmu_attach_state *state,
>                 master_domain = kzalloc(sizeof(*master_domain), GFP_KERNEL);
>                 if (!master_domain)
>                         return -ENOMEM;
> +               master_domain->domain = new_domain;
>                 master_domain->master = master;
>                 master_domain->ssid = state->ssid;
>                 if (new_domain->type == IOMMU_DOMAIN_NESTED)
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 5653d7417db7d9..fe6b88affa4a60 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -907,6 +907,11 @@ void arm_smmu_make_sva_cd(struct arm_smmu_cd *target,
>  struct arm_smmu_master_domain {
>         struct list_head devices_elm;
>         struct arm_smmu_master *master;
> +       /*
> +        * For nested domains the master_domain is threaded onto the S2 parent,
> +        * this points to the IOMMU_DOMAIN_NESTED to disambiguate the masters.
> +        */
> +       struct iommu_domain *domain;
>         ioasid_t ssid;
>         bool nested_ats_flush : 1;
>         bool using_iopf : 1;

I have tested it, and it solved the issue.

Thanks

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Jason Gunthorpe]

On Tue, Feb 18, 2025 at 11:25:59PM +0800, Zhangfei Gao wrote:

> I have tested it, and it solved the issue.

Great, thanks, Baolu can you updated the patch?

Thanks
Jason

Re: [PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Fenghua Yu]

On 2/13/25 22:11, Lu Baolu wrote:
> The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a no-op.
> It will also be removed from the iommu driver in the subsequent patch.
> Remove it to avoid dead code.
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>
> ---
>   drivers/dma/idxd/init.c | 37 ++++++-------------------------------
>   1 file changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
> index 1e5038cca22c..d44944195807 100644
> --- a/drivers/dma/idxd/init.c
> +++ b/drivers/dma/idxd/init.c
> @@ -626,21 +626,6 @@ static void idxd_disable_system_pasid(struct idxd_device *idxd)
>   	idxd->pasid = IOMMU_PASID_INVALID;
>   }
>   
> -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;
> -	return 0;
> -}
> -
> -static void idxd_disable_sva(struct pci_dev *pdev)
> -{
> -	iommu_dev_disable_feature(&pdev->dev, IOMMU_DEV_FEAT_IOPF);
> -}
> -
>   static int idxd_probe(struct idxd_device *idxd)
>   {
>   	struct pci_dev *pdev = idxd->pdev;
> @@ -655,17 +640,13 @@ static int idxd_probe(struct idxd_device *idxd)
>   	dev_dbg(dev, "IDXD reset complete\n");
>   
>   	if (IS_ENABLED(CONFIG_INTEL_IDXD_SVM) && 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);
> +		set_bit(IDXD_FLAG_USER_PASID_ENABLED, &idxd->flags);
>   
> -			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);
> -		}
> +		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);
>   	} else if (!sva) {
>   		dev_warn(dev, "User forced SVA off via module param.\n");
>   	}
> @@ -703,8 +684,6 @@ static int idxd_probe(struct idxd_device *idxd)
>    err:
>   	if (device_pasid_enabled(idxd))
>   		idxd_disable_system_pasid(idxd);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(pdev);
>   	return rc;
>   }
>   
> @@ -715,8 +694,6 @@ static void idxd_cleanup(struct idxd_device *idxd)
>   	idxd_cleanup_internals(idxd);
>   	if (device_pasid_enabled(idxd))
>   		idxd_disable_system_pasid(idxd);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(idxd->pdev);
>   }
>   
>   /*
> @@ -1248,8 +1225,6 @@ static void idxd_remove(struct pci_dev *pdev)
>   	free_irq(irq_entry->vector, irq_entry);
>   	pci_free_irq_vectors(pdev);
>   	pci_iounmap(pdev, idxd->reg_base);
> -	if (device_user_pasid_enabled(idxd))
> -		idxd_disable_sva(pdev);
>   	pci_disable_device(pdev);
>   	destroy_workqueue(idxd->wq);
>   	perfmon_pmu_remove(idxd);


Thanks.


-Fenghua

Re: [PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Baolu Lu]

On 2025/2/18 21:06, Jason Gunthorpe wrote:
> On Sat, Feb 15, 2025 at 02:32:32PM +0800, Baolu Lu wrote:
>> On 2/14/25 15:06, Nicolin Chen wrote:
>>> On Fri, Feb 14, 2025 at 02:11:03PM +0800, Lu Baolu wrote:
>>>> The iopf enablement has been moved to the iommu drivers. It is unnecessary
>>>> for iommufd to handle iopf enablement. Remove the iopf enablement logic to
>>>> avoid duplication.
>>>>
>>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommufd/device.c          |   1 -
>>>>    drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
>>>>    drivers/iommu/iommufd/iommufd_private.h |   3 -
>>>>    3 files changed, 28 insertions(+), 87 deletions(-)
>>> This is in conflict with my fault patches that Jason just took
>>> a couple days ago:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/jgg/iommufd.git/log/?
>>> h=for-next
>>>
>>> I think it needs a rebase, perhaps on the branch mentioned here:
>>> https://lore.kernel.org/linux-iommu/20250213150836.GC3754072@nvidia.com/
>> Yes, sure. I will rebase it in the next version to avoid the conflict.
> That's troublesome, I think just leave it so Joerg can pick it up. We
> can figure out what to do with the conflict later.

Okay, sure.

Re: [PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Baolu Lu]

On 2025/2/19 6:55, Fenghua Yu wrote:
> On 2/13/25 22:11, Lu Baolu wrote:
>> The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a 
>> no-op.
>> It will also be removed from the iommu driver in the subsequent patch.
>> Remove it to avoid dead code.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Fenghua Yu <fenghuay@nvidia.com>

Thank you, Fenghua.

Can you please update your contact email in MAINTAINERS file.

Thanks,
baolu

Re: [PATCH 00/12] iommu: Remove IOMMU_DEV_FEAT_SVA/_IOPF

[Baolu Lu]

On 2025/2/19 0:53, Jason Gunthorpe wrote:
> On Tue, Feb 18, 2025 at 11:25:59PM +0800, Zhangfei Gao wrote:
> 
>> I have tested it, and it solved the issue.
> Great, thanks, Baolu can you updated the patch?

Yes, sure. Will be included in the next version.

Thanks,
baolu

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>  	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>  		return -EINVAL;
>  
> +	return mock_dev_enable_iopf(dev, domain);
> +}

This isn't going to work for a replace type operation? Maybe like:

if (old_domain->iopf_handler && !domain->iopf_handler)
	return mock_dev_disable_iopf(dev, domain);
if (old_domain->iopf_handler && domain->iopf_handler)
        return 0;
return mock_dev_enable_iopf(dev, domain);

?

Jason

Re: [PATCH 09/12] dmaengine: idxd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 02:11:01PM +0800, Lu Baolu wrote:
> The IOMMU_DEV_FEAT_IOPF implementation in the iommu driver is just a no-op.
> It will also be removed from the iommu driver in the subsequent patch.
> Remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/dma/idxd/init.c | 37 ++++++-------------------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 10/12] uacce: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 02:11:02PM +0800, Lu Baolu wrote:
> None of the drivers implement anything for IOMMU_DEV_FEAT_IOPF anymore,
> remove it to avoid dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/misc/uacce/uacce.c | 31 -------------------------------
>  1 file changed, 31 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 11/12] iommufd: Remove unnecessary IOMMU_DEV_FEAT_IOPF

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 02:11:03PM +0800, Lu Baolu wrote:
> The iopf enablement has been moved to the iommu drivers. It is unnecessary
> for iommufd to handle iopf enablement. Remove the iopf enablement logic to
> avoid duplication.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommufd/device.c          |   1 -
>  drivers/iommu/iommufd/fault.c           | 111 ++++++------------------
>  drivers/iommu/iommufd/iommufd_private.h |   3 -
>  3 files changed, 28 insertions(+), 87 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 12/12] iommu: Remove iommu_dev_enable/disable_feature()

[Jason Gunthorpe]

On Fri, Feb 14, 2025 at 02:11:04PM +0800, Lu Baolu wrote:
> No external drivers use these interfaces anymore. Furthermore, no existing
> iommu drivers implement anything in the callbacks. Remove them to avoid
> dead code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/amd/iommu.c                   | 32 -------------------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 34 ---------------------
>  drivers/iommu/intel/iommu.c                 | 25 ---------------
>  drivers/iommu/iommu.c                       | 32 -------------------
>  include/linux/iommu.h                       | 28 -----------------
>  5 files changed, 151 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Baolu Lu]

On 2/20/25 09:02, Jason Gunthorpe wrote:
> On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
>> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>>   	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>>   		return -EINVAL;
>>   
>> +	return mock_dev_enable_iopf(dev, domain);
>> +}
> 
> This isn't going to work for a replace type operation? Maybe like:
> 
> if (old_domain->iopf_handler && !domain->iopf_handler)
> 	return mock_dev_disable_iopf(dev, domain);
> if (old_domain->iopf_handler && domain->iopf_handler)
>          return 0;
> return mock_dev_enable_iopf(dev, domain);
> 
> ?

The iommufd mock device driver appears not to support replacement. The
replacement operation on this driver is likely handled as follows:

- attach domain_a
- attach blocking_domain
- attach domain_b

The mock_dev_disable_iopf() is called in attach_dev of the blocking
domain.

There seems to be a bug in this patch. The existing domain should be
passed to mock_dev_disable_iopf().  It requires something similar to the
following:

diff --git a/drivers/iommu/iommufd/selftest.c 
b/drivers/iommu/iommufd/selftest.c
index a6b12cee7b00..54a6f0851758 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -168,6 +168,7 @@ struct mock_dev {
         int id;
         u32 cache[MOCK_DEV_CACHE_NUM];
         unsigned int iopf_refcount;
+       struct iommu_domain *domain;
  };

  static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,17 +198,24 @@ static int mock_domain_nop_attach(struct 
iommu_domain *domain,
                                   struct device *dev)
  {
         struct mock_dev *mdev = to_mock_dev(dev);
+       int ret;

         if (domain->dirty_ops && (mdev->flags & 
MOCK_FLAGS_DEVICE_NO_DIRTY))
                 return -EINVAL;

-       return mock_dev_enable_iopf(dev, domain);
+       ret = mock_dev_enable_iopf(dev, domain);
+       if (ret)
+               return ret;
+
+       mdev->domain = domain;
  }

  static int mock_domain_blocking_attach(struct iommu_domain *domain,
                                        struct device *dev)
  {
-       mock_dev_disable_iopf(dev, domain);
+       struct mock_dev *mdev = to_mock_dev(dev);
+
+       mock_dev_disable_iopf(dev, mdev->domain);

         return 0;
  }

Thanks,
baolu

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Jason Gunthorpe]

On Thu, Feb 20, 2025 at 03:03:21PM +0800, Baolu Lu wrote:
> On 2/20/25 09:02, Jason Gunthorpe wrote:
> > On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
> > > @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
> > >   	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
> > >   		return -EINVAL;
> > > +	return mock_dev_enable_iopf(dev, domain);
> > > +}
> > 
> > This isn't going to work for a replace type operation? Maybe like:
> > 
> > if (old_domain->iopf_handler && !domain->iopf_handler)
> > 	return mock_dev_disable_iopf(dev, domain);
> > if (old_domain->iopf_handler && domain->iopf_handler)
> >          return 0;
> > return mock_dev_enable_iopf(dev, domain);
> > 
> > ?
> 
> The iommufd mock device driver appears not to support replacement.

That's not technically a choice the driver gets to have ..

> The
> replacement operation on this driver is likely handled as follows:
> 
> - attach domain_a attach blocking_domain attach domain_b

Nothing actually does that though?

Jason

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Baolu Lu]

On 2/21/25 02:00, Jason Gunthorpe wrote:
> On Thu, Feb 20, 2025 at 03:03:21PM +0800, Baolu Lu wrote:
>> On 2/20/25 09:02, Jason Gunthorpe wrote:
>>> On Fri, Feb 14, 2025 at 02:11:00PM +0800, Lu Baolu wrote:
>>>> @@ -197,11 +201,19 @@ static int mock_domain_nop_attach(struct iommu_domain *domain,
>>>>    	if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>>>>    		return -EINVAL;
>>>> +	return mock_dev_enable_iopf(dev, domain);
>>>> +}
>>> This isn't going to work for a replace type operation? Maybe like:
>>>
>>> if (old_domain->iopf_handler && !domain->iopf_handler)
>>> 	return mock_dev_disable_iopf(dev, domain);
>>> if (old_domain->iopf_handler && domain->iopf_handler)
>>>           return 0;
>>> return mock_dev_enable_iopf(dev, domain);
>>>
>>> ?
>> The iommufd mock device driver appears not to support replacement.
> That's not technically a choice the driver gets to have ..

Yes.

> 
>> The
>> replacement operation on this driver is likely handled as follows:
>>
>> - attach domain_a attach blocking_domain attach domain_b
> Nothing actually does that though?

Ah! you are right.

This driver allows a new domain to be set even if a domain already
exists. This is different from what vt-d driver does, which removes the
old domain first. So perhaps we need to enhance it in two ways:

- Store the existing domain (a.k.a. old domain).
- Handle iopf enablement, taking the existing domain into account.

How about below change?

diff --git a/drivers/iommu/iommufd/selftest.c 
b/drivers/iommu/iommufd/selftest.c
index a6b12cee7b00..5ffbd4e3f372 100644
--- a/drivers/iommu/iommufd/selftest.c
+++ b/drivers/iommu/iommufd/selftest.c
@@ -168,6 +168,7 @@ struct mock_dev {
         int id;
         u32 cache[MOCK_DEV_CACHE_NUM];
         unsigned int iopf_refcount;
+       struct iommu_domain *domain;
  };

  static inline struct mock_dev *to_mock_dev(struct device *dev)
@@ -197,17 +198,28 @@ static int mock_domain_nop_attach(struct 
iommu_domain *domain,
                                   struct device *dev)
  {
         struct mock_dev *mdev = to_mock_dev(dev);
+       int ret;

         if (domain->dirty_ops && (mdev->flags & 
MOCK_FLAGS_DEVICE_NO_DIRTY))
                 return -EINVAL;

-       return mock_dev_enable_iopf(dev, domain);
+       if (mdev->domain)
+               mock_dev_disable_iopf(dev, mdev->domain);
+
+       ret = mock_dev_enable_iopf(dev, domain);
+       if (ret)
+               return ret;
+
+       mdev->domain = domain;
+       return 0;
  }

Thanks,
baolu

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Jason Gunthorpe]

On Fri, Feb 21, 2025 at 09:31:48AM +0800, Baolu Lu wrote:

> How about below change?

Sure

> -       return mock_dev_enable_iopf(dev, domain);
> +       if (mdev->domain)
> +               mock_dev_disable_iopf(dev, mdev->domain);
> +
> +       ret = mock_dev_enable_iopf(dev, domain);
> +       if (ret)

Though here the domain is disabled but not removed from mdev->domain,
is it OK?

Jason

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Baolu Lu]

On 2/21/25 23:04, Jason Gunthorpe wrote:
>> -       return mock_dev_enable_iopf(dev, domain);
>> +       if (mdev->domain)
>> +               mock_dev_disable_iopf(dev, mdev->domain);
>> +
>> +       ret = mock_dev_enable_iopf(dev, domain);
>> +       if (ret)
> Though here the domain is disabled but not removed from mdev->domain,
> is it OK?

That's not okay. I can make it like below:

static int mock_domain_nop_attach(struct iommu_domain *domain,
                                   struct device *dev)
{
         struct mock_dev *mdev = to_mock_dev(dev);
         int ret;

         if (domain->dirty_ops && (mdev->flags & 
MOCK_FLAGS_DEVICE_NO_DIRTY))
                 return -EINVAL;

         ret = mock_dev_enable_iopf(dev, domain);
         if (ret)
                 return ret;

         mock_dev_disable_iopf(dev, mdev->domain);
         mdev->domain = domain;

         return 0;
}

Both mock_dev_enable/disable_iopf() will be a no-op if domain or
domain's iopf handler is empty:

         if (!domain || !domain->iopf_handler)
                 return;

Does it work for you?

Thanks,
baolu

Re: [PATCH 08/12] iommufd/selftest: Put iopf enablement in domain attach path

[Jason Gunthorpe]

On Sat, Feb 22, 2025 at 03:25:43PM +0800, Baolu Lu wrote:
> On 2/21/25 23:04, Jason Gunthorpe wrote:
> > > -       return mock_dev_enable_iopf(dev, domain);
> > > +       if (mdev->domain)
> > > +               mock_dev_disable_iopf(dev, mdev->domain);
> > > +
> > > +       ret = mock_dev_enable_iopf(dev, domain);
> > > +       if (ret)
> > Though here the domain is disabled but not removed from mdev->domain,
> > is it OK?
> 
> That's not okay. I can make it like below:
> 
> static int mock_domain_nop_attach(struct iommu_domain *domain,
>                                   struct device *dev)
> {
>         struct mock_dev *mdev = to_mock_dev(dev);
>         int ret;
> 
>         if (domain->dirty_ops && (mdev->flags & MOCK_FLAGS_DEVICE_NO_DIRTY))
>                 return -EINVAL;
> 
>         ret = mock_dev_enable_iopf(dev, domain);
>         if (ret)
>                 return ret;
> 
>         mock_dev_disable_iopf(dev, mdev->domain);
>         mdev->domain = domain;
> 
>         return 0;
> }
> 
> Both mock_dev_enable/disable_iopf() will be a no-op if domain or
> domain's iopf handler is empty:
> 
>         if (!domain || !domain->iopf_handler)
>                 return;
> 
> Does it work for you?

Yeah that looks Ok

Jason