[PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[Joel Granados via B4 Relay]

This series makes use of iommufd_hwpt_replace_device to execute
non-pasid/non-svm user space IOPFs. Our main motivation is to enable
user-space driver driven device verification without SVM/PASID.

What?
  * Enable IO page fault handling in user space for a non-pasid, non-svm
    and non-virtualised use case.
  * Move IOMMU_IOPF configuration from INTEL_IOMMU_SVM into INTEL_IOMMU.
  * Move all page request queue related logic to a new (prq.c) file.
  * Remove PASID checks from PRQ event handling as well as PRQ
    initialization.
  * Allow execution of IOMMU_HWPT_ALLOC with a valid fault id
    (IOMMU_HWPT_FAULT_ID_VALID)
  * Insert a zero handle into the PASID array in dev->iommu_group when
    replacing the old HWPT with an IOPF enabled HWPT.

Why?
  The PCI ATS Extended Capability allows peripheral devices to
  participate in the caching of translations when operating under an
  IOMMU. Further, the ATS Page Request Interface (PRI) Extension allows
  devices to handle missing mappings. Currently, PRI is mainly used in
  the context of Shared Virtual Addressing, requiring support for the
  Process Address Space Identifier (PASID) capability, but other use
  cases such as enabling user-space driver driven device verification
  and reducing memory pinning exists. This patchest sets out to enable
  these use cases.

Testing?
  The non-svm IOPF interface is exercised by first initializing an IOPF
  enabled IOAS and then reading the fault file descriptor. Pseudocode on
  the iopf initializing and handling is in [3] and [4] (using libvfn).

  Supplementary repositories supporting this patchset:
    1. A user space library libvfn [1] which is used for testing and
       verification (see examples/iopf.c), and
    2. Basic emulation of PCIe ATS/PRI and Intel VT-d PRQ in QEMU [2].

Changes in v2:
  - Remove "nesting" from wording. This wording is left over from initial
    versions that are now irrelevant.
  - Dropped "iommu: init pasid array while doing domain_replace and iopf
    is active" as the initialization of the pasid_array x-array happens
    automatically when an iopf capable domain is replaced on a device.
  - Corrected commit message in "iommu/vt-d: Separate page request queue
    from SVM"
  - Link to v1: https://lore.kernel.org/r/20240904-jag-iopfv8-v1-0-e3549920adf3@samsung.com

V1:
  - This is the first version of the series after initial feedback from
    the RFC [5].

Comments and feedback are greatly appreciated
Best

Joel

[1] https://github.com/SamsungDS/libvfn/tree/iommufd-fault-queue
[2] https://gitlab.com/birkelund/qemu/-/tree/pcie-ats-pri

[3] Initializing
```
  int iopf_init(struct iommu_ioas *ioas, const char *bdf)
  {
      // open vfio device from bdf
      int devfd = open('/dev/vfio/devices/VFIO_DEV', O_RDWR);

      struct vfio_device_bind_iommufd bind = {
          .argsz = sizeof(bind),
          .flags = 0,
          .iommufd = __iommufd,
      };
      ioctl(devfd, VFIO_DEVICE_BIND_IOMMUFD, &bind);

      struct iommu_ioas *ioas = ioas;
      struct vfio_device_attach_iommufd_pt attach_data = {
          .argsz = sizeof(attach_data),
          .flags = 0,
          .pt_id = ioas->id,
      };
      ioctl(devfd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data);

      struct iommu_fault_alloc fault = {
          .size = sizeof(fault),
          .flags = 0,
      };
      ioctl(__iommufd, IOMMU_FAULT_QUEUE_ALLOC, &fault);

      struct iommu_hwpt_alloc fault_cmd = {
          .size = sizeof(fault_cmd),
          .flags = IOMMU_HWPT_FAULT_ID_VALID,
          .dev_id = bind.out_devid,
          .pt_id = ioas->id,
          .data_len = 0,
          .data_uptr = (uint64_t)NULL,
          .fault_id = fault.out_fault_id,
          .__reserved = 0,
          };
      ioctl(__iommufd, IOMMU_HWPT_ALLOC, &fault_cmd);

      // This is a re-attach
      struct vfio_device_attach_iommufd_pt attach = {
          .argsz = sizeof(attach),
          .flags = 0,
          .pt_id = fault_cmd.out_hwpt_id
      };
      ioctl(dev_fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach);
  }
```

[4] Handling
```
  int handle_iopf(void *vaddr, int len, uint64_t iova) {
    exec_command(CMD)

    int iopf_fd = fault_cmd.fault_id;

    struct iommu_hwpt_pgfault pgfault = {0};
    if(read(iopf_fd, &pgfault, sizeof(pgfault)) == 0);
      return; // no page fault

    ret = iommu_map_vaddr(__iommmufd, vaddr, len, &iova)
    struct iommu_hwpt_page_response pgfault_response = {
      .cookie = pgfault.cookie,
      .code = ret ? IOMMUFD_PAGE_RESP_SUCCESS : IOMMUFD_PAGE_RESP_INVALID,
    };

    write(iopf_fd, &pgfault_response, sizeof(pgfault_response));

    return;
  }
```

[5] https://lore.kernel.org/20240826-iopf-for-all-v1-0-59174e6a7528@samsung.com

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
Joel Granados (3):
      iommu/vt-d: Separate page request queue from SVM
      iommu: kconfig: Move IOMMU_IOPF into INTEL_IOMMU
      iommufd: Enable PRI when doing the iommufd_hwpt_alloc

Klaus Jensen (2):
      iommu/vt-d: Remove the pasid present check in prq_event_thread
      iommu/vt-d: drop pasid requirement for prq initialization

 drivers/iommu/intel/Kconfig          |   2 +-
 drivers/iommu/intel/Makefile         |   2 +-
 drivers/iommu/intel/iommu.c          |  29 +--
 drivers/iommu/intel/iommu.h          |  14 +-
 drivers/iommu/intel/prq.c            | 404 +++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/svm.c            | 397 ----------------------------------
 drivers/iommu/iommufd/hw_pagetable.c |   3 +-
 7 files changed, 425 insertions(+), 426 deletions(-)
---
base-commit: 431c1646e1f86b949fa3685efc50b660a364c2b6
change-id: 20240904-jag-iopfv8-1577fd20422d

Best regards,
-- 
Joel Granados <j.granados@samsung.com>

[PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Joel Granados via B4 Relay]

From: Joel Granados <j.granados@samsung.com>

IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM. Move
all Page Request Queue (PRQ) functions that handle prq events to a new
file in drivers/iommu/intel/prq.c. The page_req_des struct is now
declared in drivers/iommu/intel/prq.c.

No functional changes are intended. This is a preparation patch to
enable the use of IO page faults outside the SVM/PASID use cases.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/iommu/intel/Makefile |   2 +-
 drivers/iommu/intel/iommu.c  |  18 +-
 drivers/iommu/intel/iommu.h  |  14 +-
 drivers/iommu/intel/prq.c    | 410 +++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel/svm.c    | 397 -----------------------------------------
 5 files changed, 423 insertions(+), 418 deletions(-)

diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
index c8beb0281559..d3bb0798092d 100644
--- a/drivers/iommu/intel/Makefile
+++ b/drivers/iommu/intel/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
-obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o nested.o cache.o
+obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o nested.o cache.o prq.o
 obj-$(CONFIG_DMAR_TABLE) += trace.o cap_audit.o
 obj-$(CONFIG_DMAR_PERF) += perf.o
 obj-$(CONFIG_INTEL_IOMMU_DEBUGFS) += debugfs.o
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 4aa070cf56e7..5acc52c62e8c 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1487,12 +1487,10 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
 	/* free context mapping */
 	free_context_table(iommu);
 
-#ifdef CONFIG_INTEL_IOMMU_SVM
 	if (pasid_supported(iommu)) {
 		if (ecap_prs(iommu->ecap))
-			intel_svm_finish_prq(iommu);
+			intel_finish_prq(iommu);
 	}
-#endif
 }
 
 /*
@@ -2482,19 +2480,18 @@ static int __init init_dmars(void)
 
 		iommu_flush_write_buffer(iommu);
 
-#ifdef CONFIG_INTEL_IOMMU_SVM
 		if (pasid_supported(iommu) && ecap_prs(iommu->ecap)) {
 			/*
 			 * Call dmar_alloc_hwirq() with dmar_global_lock held,
 			 * could cause possible lock race condition.
 			 */
 			up_write(&dmar_global_lock);
-			ret = intel_svm_enable_prq(iommu);
+			ret = intel_enable_prq(iommu);
 			down_write(&dmar_global_lock);
 			if (ret)
 				goto free_iommu;
 		}
-#endif
+
 		ret = dmar_set_interrupt(iommu);
 		if (ret)
 			goto free_iommu;
@@ -2924,13 +2921,12 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 	intel_iommu_init_qi(iommu);
 	iommu_flush_write_buffer(iommu);
 
-#ifdef CONFIG_INTEL_IOMMU_SVM
 	if (pasid_supported(iommu) && ecap_prs(iommu->ecap)) {
-		ret = intel_svm_enable_prq(iommu);
+		ret = intel_enable_prq(iommu);
 		if (ret)
 			goto disable_iommu;
 	}
-#endif
+
 	ret = dmar_set_interrupt(iommu);
 	if (ret)
 		goto disable_iommu;
@@ -4673,9 +4669,7 @@ const struct iommu_ops intel_iommu_ops = {
 	.def_domain_type	= device_def_domain_type,
 	.remove_dev_pasid	= intel_iommu_remove_dev_pasid,
 	.pgsize_bitmap		= SZ_4K,
-#ifdef CONFIG_INTEL_IOMMU_SVM
-	.page_response		= intel_svm_page_response,
-#endif
+	.page_response		= intel_page_response,
 	.default_domain_ops = &(const struct iommu_domain_ops) {
 		.attach_dev		= intel_iommu_attach_device,
 		.set_dev_pasid		= intel_iommu_set_dev_pasid,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index a969be2258b1..3bce514e1d88 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -719,12 +719,10 @@ struct intel_iommu {
 
 	struct iommu_flush flush;
 #endif
-#ifdef CONFIG_INTEL_IOMMU_SVM
 	struct page_req_dsc *prq;
 	unsigned char prq_name[16];    /* Name for PRQ interrupt */
 	unsigned long prq_seq_number;
 	struct completion prq_complete;
-#endif
 	struct iopf_queue *iopf_queue;
 	unsigned char iopfq_name[16];
 	/* Synchronization between fault report and iommu device release. */
@@ -1156,18 +1154,18 @@ void intel_context_flush_present(struct device_domain_info *info,
 				 struct context_entry *context,
 				 u16 did, bool affect_domains);
 
+int intel_enable_prq(struct intel_iommu *iommu);
+int intel_finish_prq(struct intel_iommu *iommu);
+void intel_page_response(struct device *dev, struct iopf_fault *evt,
+			struct iommu_page_response *msg);
+void intel_drain_pasid_prq(struct device *dev, u32 pasid);
+
 #ifdef CONFIG_INTEL_IOMMU_SVM
 void intel_svm_check(struct intel_iommu *iommu);
-int intel_svm_enable_prq(struct intel_iommu *iommu);
-int intel_svm_finish_prq(struct intel_iommu *iommu);
-void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
-			     struct iommu_page_response *msg);
 struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
 					    struct mm_struct *mm);
-void intel_drain_pasid_prq(struct device *dev, u32 pasid);
 #else
 static inline void intel_svm_check(struct intel_iommu *iommu) {}
-static inline void intel_drain_pasid_prq(struct device *dev, u32 pasid) {}
 static inline struct iommu_domain *intel_svm_domain_alloc(struct device *dev,
 							  struct mm_struct *mm)
 {
diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
new file mode 100644
index 000000000000..3376f60082b5
--- /dev/null
+++ b/drivers/iommu/intel/prq.c
@@ -0,0 +1,410 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright © 2015 Intel Corporation.
+ *
+ * Originally split from drivers/iommu/intel/svm.c
+ */
+
+#include <linux/pci.h>
+#include <linux/pci-ats.h>
+
+#include "iommu.h"
+#include "../iommu-pages.h"
+#include "trace.h"
+
+/* Page request queue descriptor */
+struct page_req_dsc {
+	union {
+		struct {
+			u64 type:8;
+			u64 pasid_present:1;
+			u64 rsvd:7;
+			u64 rid:16;
+			u64 pasid:20;
+			u64 exe_req:1;
+			u64 pm_req:1;
+			u64 rsvd2:10;
+		};
+		u64 qw_0;
+	};
+	union {
+		struct {
+			u64 rd_req:1;
+			u64 wr_req:1;
+			u64 lpig:1;
+			u64 prg_index:9;
+			u64 addr:52;
+		};
+		u64 qw_1;
+	};
+	u64 qw_2;
+	u64 qw_3;
+};
+
+/**
+ * intel_drain_pasid_prq - Drain page requests and responses for a pasid
+ * @dev: target device
+ * @pasid: pasid for draining
+ *
+ * Drain all pending page requests and responses related to @pasid in both
+ * software and hardware. This is supposed to be called after the device
+ * driver has stopped DMA, the pasid entry has been cleared, and both IOTLB
+ * and DevTLB have been invalidated.
+ *
+ * It waits until all pending page requests for @pasid in the page fault
+ * queue are completed by the prq handling thread. Then follow the steps
+ * described in VT-d spec CH7.10 to drain all page requests and page
+ * responses pending in the hardware.
+ */
+void intel_drain_pasid_prq(struct device *dev, u32 pasid)
+{
+	struct device_domain_info *info;
+	struct dmar_domain *domain;
+	struct intel_iommu *iommu;
+	struct qi_desc desc[3];
+	struct pci_dev *pdev;
+	int head, tail;
+	u16 sid, did;
+	int qdep;
+
+	info = dev_iommu_priv_get(dev);
+	if (WARN_ON(!info || !dev_is_pci(dev)))
+		return;
+
+	if (!info->pri_enabled)
+		return;
+
+	iommu = info->iommu;
+	domain = info->domain;
+	pdev = to_pci_dev(dev);
+	sid = PCI_DEVID(info->bus, info->devfn);
+	did = domain_id_iommu(domain, iommu);
+	qdep = pci_ats_queue_depth(pdev);
+
+	/*
+	 * Check and wait until all pending page requests in the queue are
+	 * handled by the prq handling thread.
+	 */
+prq_retry:
+	reinit_completion(&iommu->prq_complete);
+	tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
+	head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
+	while (head != tail) {
+		struct page_req_dsc *req;
+
+		req = &iommu->prq[head / sizeof(*req)];
+		if (!req->pasid_present || req->pasid != pasid) {
+			head = (head + sizeof(*req)) & PRQ_RING_MASK;
+			continue;
+		}
+
+		wait_for_completion(&iommu->prq_complete);
+		goto prq_retry;
+	}
+
+	iopf_queue_flush_dev(dev);
+
+	/*
+	 * Perform steps described in VT-d spec CH7.10 to drain page
+	 * requests and responses in hardware.
+	 */
+	memset(desc, 0, sizeof(desc));
+	desc[0].qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
+			QI_IWD_FENCE |
+			QI_IWD_TYPE;
+	desc[1].qw0 = QI_EIOTLB_PASID(pasid) |
+			QI_EIOTLB_DID(did) |
+			QI_EIOTLB_GRAN(QI_GRAN_NONG_PASID) |
+			QI_EIOTLB_TYPE;
+	desc[2].qw0 = QI_DEV_EIOTLB_PASID(pasid) |
+			QI_DEV_EIOTLB_SID(sid) |
+			QI_DEV_EIOTLB_QDEP(qdep) |
+			QI_DEIOTLB_TYPE |
+			QI_DEV_IOTLB_PFSID(info->pfsid);
+qi_retry:
+	reinit_completion(&iommu->prq_complete);
+	qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
+	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
+		wait_for_completion(&iommu->prq_complete);
+		goto qi_retry;
+	}
+}
+
+
+static bool is_canonical_address(u64 addr)
+{
+	int shift = 64 - (__VIRTUAL_MASK_SHIFT + 1);
+	long saddr = (long) addr;
+
+	return (((saddr << shift) >> shift) == saddr);
+}
+
+static void handle_bad_prq_event(struct intel_iommu *iommu,
+				 struct page_req_dsc *req, int result)
+{
+	struct qi_desc desc = { };
+
+	pr_err("%s: Invalid page request: %08llx %08llx\n",
+	       iommu->name, ((unsigned long long *)req)[0],
+	       ((unsigned long long *)req)[1]);
+
+	if (!req->lpig)
+		return;
+
+	desc.qw0 = QI_PGRP_PASID(req->pasid) |
+			QI_PGRP_DID(req->rid) |
+			QI_PGRP_PASID_P(req->pasid_present) |
+			QI_PGRP_RESP_CODE(result) |
+			QI_PGRP_RESP_TYPE;
+	desc.qw1 = QI_PGRP_IDX(req->prg_index) |
+			QI_PGRP_LPIG(req->lpig);
+
+	qi_submit_sync(iommu, &desc, 1, 0);
+}
+
+static int prq_to_iommu_prot(struct page_req_dsc *req)
+{
+	int prot = 0;
+
+	if (req->rd_req)
+		prot |= IOMMU_FAULT_PERM_READ;
+	if (req->wr_req)
+		prot |= IOMMU_FAULT_PERM_WRITE;
+	if (req->exe_req)
+		prot |= IOMMU_FAULT_PERM_EXEC;
+	if (req->pm_req)
+		prot |= IOMMU_FAULT_PERM_PRIV;
+
+	return prot;
+}
+
+static void intel_prq_report(struct intel_iommu *iommu, struct device *dev,
+				 struct page_req_dsc *desc)
+{
+	struct iopf_fault event = { };
+
+	/* Fill in event data for device specific processing */
+	event.fault.type = IOMMU_FAULT_PAGE_REQ;
+	event.fault.prm.addr = (u64)desc->addr << VTD_PAGE_SHIFT;
+	event.fault.prm.pasid = desc->pasid;
+	event.fault.prm.grpid = desc->prg_index;
+	event.fault.prm.perm = prq_to_iommu_prot(desc);
+
+	if (desc->lpig)
+		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
+	if (desc->pasid_present) {
+		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+		event.fault.prm.flags |= IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
+	}
+
+	iommu_report_device_fault(dev, &event);
+}
+
+static irqreturn_t prq_event_thread(int irq, void *d)
+{
+	struct intel_iommu *iommu = d;
+	struct page_req_dsc *req;
+	int head, tail, handled;
+	struct device *dev;
+	u64 address;
+
+	/*
+	 * Clear PPR bit before reading head/tail registers, to ensure that
+	 * we get a new interrupt if needed.
+	 */
+	writel(DMA_PRS_PPR, iommu->reg + DMAR_PRS_REG);
+
+	tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
+	head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
+	handled = (head != tail);
+	while (head != tail) {
+		req = &iommu->prq[head / sizeof(*req)];
+		address = (u64)req->addr << VTD_PAGE_SHIFT;
+
+		if (unlikely(!req->pasid_present)) {
+			pr_err("IOMMU: %s: Page request without PASID\n",
+			       iommu->name);
+bad_req:
+			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
+			goto prq_advance;
+		}
+
+		if (unlikely(!is_canonical_address(address))) {
+			pr_err("IOMMU: %s: Address is not canonical\n",
+			       iommu->name);
+			goto bad_req;
+		}
+
+		if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {
+			pr_err("IOMMU: %s: Page request in Privilege Mode\n",
+			       iommu->name);
+			goto bad_req;
+		}
+
+		if (unlikely(req->exe_req && req->rd_req)) {
+			pr_err("IOMMU: %s: Execution request not supported\n",
+			       iommu->name);
+			goto bad_req;
+		}
+
+		/* Drop Stop Marker message. No need for a response. */
+		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
+			goto prq_advance;
+
+		/*
+		 * If prq is to be handled outside iommu driver via receiver of
+		 * the fault notifiers, we skip the page response here.
+		 */
+		mutex_lock(&iommu->iopf_lock);
+		dev = device_rbtree_find(iommu, req->rid);
+		if (!dev) {
+			mutex_unlock(&iommu->iopf_lock);
+			goto bad_req;
+		}
+
+		intel_prq_report(iommu, dev, req);
+		trace_prq_report(iommu, dev, req->qw_0, req->qw_1,
+				 req->qw_2, req->qw_3,
+				 iommu->prq_seq_number++);
+		mutex_unlock(&iommu->iopf_lock);
+prq_advance:
+		head = (head + sizeof(*req)) & PRQ_RING_MASK;
+	}
+
+	dmar_writeq(iommu->reg + DMAR_PQH_REG, tail);
+
+	/*
+	 * Clear the page request overflow bit and wake up all threads that
+	 * are waiting for the completion of this handling.
+	 */
+	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
+		pr_info_ratelimited("IOMMU: %s: PRQ overflow detected\n",
+				    iommu->name);
+		head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
+		tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
+		if (head == tail) {
+			iopf_queue_discard_partial(iommu->iopf_queue);
+			writel(DMA_PRS_PRO, iommu->reg + DMAR_PRS_REG);
+			pr_info_ratelimited("IOMMU: %s: PRQ overflow cleared",
+					    iommu->name);
+		}
+	}
+
+	if (!completion_done(&iommu->prq_complete))
+		complete(&iommu->prq_complete);
+
+	return IRQ_RETVAL(handled);
+}
+
+int intel_enable_prq(struct intel_iommu *iommu)
+{
+	struct iopf_queue *iopfq;
+	int irq, ret;
+
+	iommu->prq = iommu_alloc_pages_node(iommu->node, GFP_KERNEL, PRQ_ORDER);
+	if (!iommu->prq) {
+		pr_warn("IOMMU: %s: Failed to allocate page request queue\n",
+			iommu->name);
+		return -ENOMEM;
+	}
+
+	irq = dmar_alloc_hwirq(IOMMU_IRQ_ID_OFFSET_PRQ + iommu->seq_id, iommu->node, iommu);
+	if (irq <= 0) {
+		pr_err("IOMMU: %s: Failed to create IRQ vector for page request queue\n",
+		       iommu->name);
+		ret = -EINVAL;
+		goto free_prq;
+	}
+	iommu->pr_irq = irq;
+
+	snprintf(iommu->iopfq_name, sizeof(iommu->iopfq_name),
+		 "dmar%d-iopfq", iommu->seq_id);
+	iopfq = iopf_queue_alloc(iommu->iopfq_name);
+	if (!iopfq) {
+		pr_err("IOMMU: %s: Failed to allocate iopf queue\n", iommu->name);
+		ret = -ENOMEM;
+		goto free_hwirq;
+	}
+	iommu->iopf_queue = iopfq;
+
+	snprintf(iommu->prq_name, sizeof(iommu->prq_name), "dmar%d-prq", iommu->seq_id);
+
+	ret = request_threaded_irq(irq, NULL, prq_event_thread, IRQF_ONESHOT,
+				   iommu->prq_name, iommu);
+	if (ret) {
+		pr_err("IOMMU: %s: Failed to request IRQ for page request queue\n",
+		       iommu->name);
+		goto free_iopfq;
+	}
+	dmar_writeq(iommu->reg + DMAR_PQH_REG, 0ULL);
+	dmar_writeq(iommu->reg + DMAR_PQT_REG, 0ULL);
+	dmar_writeq(iommu->reg + DMAR_PQA_REG, virt_to_phys(iommu->prq) | PRQ_ORDER);
+
+	init_completion(&iommu->prq_complete);
+
+	return 0;
+
+free_iopfq:
+	iopf_queue_free(iommu->iopf_queue);
+	iommu->iopf_queue = NULL;
+free_hwirq:
+	dmar_free_hwirq(irq);
+	iommu->pr_irq = 0;
+free_prq:
+	iommu_free_pages(iommu->prq, PRQ_ORDER);
+	iommu->prq = NULL;
+
+	return ret;
+}
+
+int intel_finish_prq(struct intel_iommu *iommu)
+{
+	dmar_writeq(iommu->reg + DMAR_PQH_REG, 0ULL);
+	dmar_writeq(iommu->reg + DMAR_PQT_REG, 0ULL);
+	dmar_writeq(iommu->reg + DMAR_PQA_REG, 0ULL);
+
+	if (iommu->pr_irq) {
+		free_irq(iommu->pr_irq, iommu);
+		dmar_free_hwirq(iommu->pr_irq);
+		iommu->pr_irq = 0;
+	}
+
+	if (iommu->iopf_queue) {
+		iopf_queue_free(iommu->iopf_queue);
+		iommu->iopf_queue = NULL;
+	}
+
+	iommu_free_pages(iommu->prq, PRQ_ORDER);
+	iommu->prq = NULL;
+
+	return 0;
+}
+
+void intel_page_response(struct device *dev, struct iopf_fault *evt,
+			 struct iommu_page_response *msg)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct intel_iommu *iommu = info->iommu;
+	u8 bus = info->bus, devfn = info->devfn;
+	struct iommu_fault_page_request *prm;
+	struct qi_desc desc;
+	bool pasid_present;
+	bool last_page;
+	u16 sid;
+
+	prm = &evt->fault.prm;
+	sid = PCI_DEVID(bus, devfn);
+	pasid_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+	last_page = prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
+
+	desc.qw0 = QI_PGRP_PASID(prm->pasid) | QI_PGRP_DID(sid) |
+			QI_PGRP_PASID_P(pasid_present) |
+			QI_PGRP_RESP_CODE(msg->code) |
+			QI_PGRP_RESP_TYPE;
+	desc.qw1 = QI_PGRP_IDX(prm->grpid) | QI_PGRP_LPIG(last_page);
+	desc.qw2 = 0;
+	desc.qw3 = 0;
+
+	qi_submit_sync(iommu, &desc, 1, 0);
+}
+
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 0e3a9b38bef2..6ab7d9d03d3d 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -25,92 +25,6 @@
 #include "../iommu-pages.h"
 #include "trace.h"
 
-static irqreturn_t prq_event_thread(int irq, void *d);
-
-int intel_svm_enable_prq(struct intel_iommu *iommu)
-{
-	struct iopf_queue *iopfq;
-	int irq, ret;
-
-	iommu->prq = iommu_alloc_pages_node(iommu->node, GFP_KERNEL, PRQ_ORDER);
-	if (!iommu->prq) {
-		pr_warn("IOMMU: %s: Failed to allocate page request queue\n",
-			iommu->name);
-		return -ENOMEM;
-	}
-
-	irq = dmar_alloc_hwirq(IOMMU_IRQ_ID_OFFSET_PRQ + iommu->seq_id, iommu->node, iommu);
-	if (irq <= 0) {
-		pr_err("IOMMU: %s: Failed to create IRQ vector for page request queue\n",
-		       iommu->name);
-		ret = -EINVAL;
-		goto free_prq;
-	}
-	iommu->pr_irq = irq;
-
-	snprintf(iommu->iopfq_name, sizeof(iommu->iopfq_name),
-		 "dmar%d-iopfq", iommu->seq_id);
-	iopfq = iopf_queue_alloc(iommu->iopfq_name);
-	if (!iopfq) {
-		pr_err("IOMMU: %s: Failed to allocate iopf queue\n", iommu->name);
-		ret = -ENOMEM;
-		goto free_hwirq;
-	}
-	iommu->iopf_queue = iopfq;
-
-	snprintf(iommu->prq_name, sizeof(iommu->prq_name), "dmar%d-prq", iommu->seq_id);
-
-	ret = request_threaded_irq(irq, NULL, prq_event_thread, IRQF_ONESHOT,
-				   iommu->prq_name, iommu);
-	if (ret) {
-		pr_err("IOMMU: %s: Failed to request IRQ for page request queue\n",
-		       iommu->name);
-		goto free_iopfq;
-	}
-	dmar_writeq(iommu->reg + DMAR_PQH_REG, 0ULL);
-	dmar_writeq(iommu->reg + DMAR_PQT_REG, 0ULL);
-	dmar_writeq(iommu->reg + DMAR_PQA_REG, virt_to_phys(iommu->prq) | PRQ_ORDER);
-
-	init_completion(&iommu->prq_complete);
-
-	return 0;
-
-free_iopfq:
-	iopf_queue_free(iommu->iopf_queue);
-	iommu->iopf_queue = NULL;
-free_hwirq:
-	dmar_free_hwirq(irq);
-	iommu->pr_irq = 0;
-free_prq:
-	iommu_free_pages(iommu->prq, PRQ_ORDER);
-	iommu->prq = NULL;
-
-	return ret;
-}
-
-int intel_svm_finish_prq(struct intel_iommu *iommu)
-{
-	dmar_writeq(iommu->reg + DMAR_PQH_REG, 0ULL);
-	dmar_writeq(iommu->reg + DMAR_PQT_REG, 0ULL);
-	dmar_writeq(iommu->reg + DMAR_PQA_REG, 0ULL);
-
-	if (iommu->pr_irq) {
-		free_irq(iommu->pr_irq, iommu);
-		dmar_free_hwirq(iommu->pr_irq);
-		iommu->pr_irq = 0;
-	}
-
-	if (iommu->iopf_queue) {
-		iopf_queue_free(iommu->iopf_queue);
-		iommu->iopf_queue = NULL;
-	}
-
-	iommu_free_pages(iommu->prq, PRQ_ORDER);
-	iommu->prq = NULL;
-
-	return 0;
-}
-
 void intel_svm_check(struct intel_iommu *iommu)
 {
 	if (!pasid_supported(iommu))
@@ -237,317 +151,6 @@ static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
 	return ret;
 }
 
-/* Page request queue descriptor */
-struct page_req_dsc {
-	union {
-		struct {
-			u64 type:8;
-			u64 pasid_present:1;
-			u64 rsvd:7;
-			u64 rid:16;
-			u64 pasid:20;
-			u64 exe_req:1;
-			u64 pm_req:1;
-			u64 rsvd2:10;
-		};
-		u64 qw_0;
-	};
-	union {
-		struct {
-			u64 rd_req:1;
-			u64 wr_req:1;
-			u64 lpig:1;
-			u64 prg_index:9;
-			u64 addr:52;
-		};
-		u64 qw_1;
-	};
-	u64 qw_2;
-	u64 qw_3;
-};
-
-static bool is_canonical_address(u64 addr)
-{
-	int shift = 64 - (__VIRTUAL_MASK_SHIFT + 1);
-	long saddr = (long) addr;
-
-	return (((saddr << shift) >> shift) == saddr);
-}
-
-/**
- * intel_drain_pasid_prq - Drain page requests and responses for a pasid
- * @dev: target device
- * @pasid: pasid for draining
- *
- * Drain all pending page requests and responses related to @pasid in both
- * software and hardware. This is supposed to be called after the device
- * driver has stopped DMA, the pasid entry has been cleared, and both IOTLB
- * and DevTLB have been invalidated.
- *
- * It waits until all pending page requests for @pasid in the page fault
- * queue are completed by the prq handling thread. Then follow the steps
- * described in VT-d spec CH7.10 to drain all page requests and page
- * responses pending in the hardware.
- */
-void intel_drain_pasid_prq(struct device *dev, u32 pasid)
-{
-	struct device_domain_info *info;
-	struct dmar_domain *domain;
-	struct intel_iommu *iommu;
-	struct qi_desc desc[3];
-	struct pci_dev *pdev;
-	int head, tail;
-	u16 sid, did;
-	int qdep;
-
-	info = dev_iommu_priv_get(dev);
-	if (WARN_ON(!info || !dev_is_pci(dev)))
-		return;
-
-	if (!info->pri_enabled)
-		return;
-
-	iommu = info->iommu;
-	domain = info->domain;
-	pdev = to_pci_dev(dev);
-	sid = PCI_DEVID(info->bus, info->devfn);
-	did = domain_id_iommu(domain, iommu);
-	qdep = pci_ats_queue_depth(pdev);
-
-	/*
-	 * Check and wait until all pending page requests in the queue are
-	 * handled by the prq handling thread.
-	 */
-prq_retry:
-	reinit_completion(&iommu->prq_complete);
-	tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
-	head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
-	while (head != tail) {
-		struct page_req_dsc *req;
-
-		req = &iommu->prq[head / sizeof(*req)];
-		if (!req->pasid_present || req->pasid != pasid) {
-			head = (head + sizeof(*req)) & PRQ_RING_MASK;
-			continue;
-		}
-
-		wait_for_completion(&iommu->prq_complete);
-		goto prq_retry;
-	}
-
-	iopf_queue_flush_dev(dev);
-
-	/*
-	 * Perform steps described in VT-d spec CH7.10 to drain page
-	 * requests and responses in hardware.
-	 */
-	memset(desc, 0, sizeof(desc));
-	desc[0].qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
-			QI_IWD_FENCE |
-			QI_IWD_TYPE;
-	desc[1].qw0 = QI_EIOTLB_PASID(pasid) |
-			QI_EIOTLB_DID(did) |
-			QI_EIOTLB_GRAN(QI_GRAN_NONG_PASID) |
-			QI_EIOTLB_TYPE;
-	desc[2].qw0 = QI_DEV_EIOTLB_PASID(pasid) |
-			QI_DEV_EIOTLB_SID(sid) |
-			QI_DEV_EIOTLB_QDEP(qdep) |
-			QI_DEIOTLB_TYPE |
-			QI_DEV_IOTLB_PFSID(info->pfsid);
-qi_retry:
-	reinit_completion(&iommu->prq_complete);
-	qi_submit_sync(iommu, desc, 3, QI_OPT_WAIT_DRAIN);
-	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
-		wait_for_completion(&iommu->prq_complete);
-		goto qi_retry;
-	}
-}
-
-static int prq_to_iommu_prot(struct page_req_dsc *req)
-{
-	int prot = 0;
-
-	if (req->rd_req)
-		prot |= IOMMU_FAULT_PERM_READ;
-	if (req->wr_req)
-		prot |= IOMMU_FAULT_PERM_WRITE;
-	if (req->exe_req)
-		prot |= IOMMU_FAULT_PERM_EXEC;
-	if (req->pm_req)
-		prot |= IOMMU_FAULT_PERM_PRIV;
-
-	return prot;
-}
-
-static void intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
-				 struct page_req_dsc *desc)
-{
-	struct iopf_fault event = { };
-
-	/* Fill in event data for device specific processing */
-	event.fault.type = IOMMU_FAULT_PAGE_REQ;
-	event.fault.prm.addr = (u64)desc->addr << VTD_PAGE_SHIFT;
-	event.fault.prm.pasid = desc->pasid;
-	event.fault.prm.grpid = desc->prg_index;
-	event.fault.prm.perm = prq_to_iommu_prot(desc);
-
-	if (desc->lpig)
-		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
-	if (desc->pasid_present) {
-		event.fault.prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
-		event.fault.prm.flags |= IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
-	}
-
-	iommu_report_device_fault(dev, &event);
-}
-
-static void handle_bad_prq_event(struct intel_iommu *iommu,
-				 struct page_req_dsc *req, int result)
-{
-	struct qi_desc desc = { };
-
-	pr_err("%s: Invalid page request: %08llx %08llx\n",
-	       iommu->name, ((unsigned long long *)req)[0],
-	       ((unsigned long long *)req)[1]);
-
-	if (!req->lpig)
-		return;
-
-	desc.qw0 = QI_PGRP_PASID(req->pasid) |
-			QI_PGRP_DID(req->rid) |
-			QI_PGRP_PASID_P(req->pasid_present) |
-			QI_PGRP_RESP_CODE(result) |
-			QI_PGRP_RESP_TYPE;
-	desc.qw1 = QI_PGRP_IDX(req->prg_index) |
-			QI_PGRP_LPIG(req->lpig);
-
-	qi_submit_sync(iommu, &desc, 1, 0);
-}
-
-static irqreturn_t prq_event_thread(int irq, void *d)
-{
-	struct intel_iommu *iommu = d;
-	struct page_req_dsc *req;
-	int head, tail, handled;
-	struct device *dev;
-	u64 address;
-
-	/*
-	 * Clear PPR bit before reading head/tail registers, to ensure that
-	 * we get a new interrupt if needed.
-	 */
-	writel(DMA_PRS_PPR, iommu->reg + DMAR_PRS_REG);
-
-	tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
-	head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
-	handled = (head != tail);
-	while (head != tail) {
-		req = &iommu->prq[head / sizeof(*req)];
-		address = (u64)req->addr << VTD_PAGE_SHIFT;
-
-		if (unlikely(!req->pasid_present)) {
-			pr_err("IOMMU: %s: Page request without PASID\n",
-			       iommu->name);
-bad_req:
-			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
-			goto prq_advance;
-		}
-
-		if (unlikely(!is_canonical_address(address))) {
-			pr_err("IOMMU: %s: Address is not canonical\n",
-			       iommu->name);
-			goto bad_req;
-		}
-
-		if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {
-			pr_err("IOMMU: %s: Page request in Privilege Mode\n",
-			       iommu->name);
-			goto bad_req;
-		}
-
-		if (unlikely(req->exe_req && req->rd_req)) {
-			pr_err("IOMMU: %s: Execution request not supported\n",
-			       iommu->name);
-			goto bad_req;
-		}
-
-		/* Drop Stop Marker message. No need for a response. */
-		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
-			goto prq_advance;
-
-		/*
-		 * If prq is to be handled outside iommu driver via receiver of
-		 * the fault notifiers, we skip the page response here.
-		 */
-		mutex_lock(&iommu->iopf_lock);
-		dev = device_rbtree_find(iommu, req->rid);
-		if (!dev) {
-			mutex_unlock(&iommu->iopf_lock);
-			goto bad_req;
-		}
-
-		intel_svm_prq_report(iommu, dev, req);
-		trace_prq_report(iommu, dev, req->qw_0, req->qw_1,
-				 req->qw_2, req->qw_3,
-				 iommu->prq_seq_number++);
-		mutex_unlock(&iommu->iopf_lock);
-prq_advance:
-		head = (head + sizeof(*req)) & PRQ_RING_MASK;
-	}
-
-	dmar_writeq(iommu->reg + DMAR_PQH_REG, tail);
-
-	/*
-	 * Clear the page request overflow bit and wake up all threads that
-	 * are waiting for the completion of this handling.
-	 */
-	if (readl(iommu->reg + DMAR_PRS_REG) & DMA_PRS_PRO) {
-		pr_info_ratelimited("IOMMU: %s: PRQ overflow detected\n",
-				    iommu->name);
-		head = dmar_readq(iommu->reg + DMAR_PQH_REG) & PRQ_RING_MASK;
-		tail = dmar_readq(iommu->reg + DMAR_PQT_REG) & PRQ_RING_MASK;
-		if (head == tail) {
-			iopf_queue_discard_partial(iommu->iopf_queue);
-			writel(DMA_PRS_PRO, iommu->reg + DMAR_PRS_REG);
-			pr_info_ratelimited("IOMMU: %s: PRQ overflow cleared",
-					    iommu->name);
-		}
-	}
-
-	if (!completion_done(&iommu->prq_complete))
-		complete(&iommu->prq_complete);
-
-	return IRQ_RETVAL(handled);
-}
-
-void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
-			     struct iommu_page_response *msg)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct intel_iommu *iommu = info->iommu;
-	u8 bus = info->bus, devfn = info->devfn;
-	struct iommu_fault_page_request *prm;
-	struct qi_desc desc;
-	bool pasid_present;
-	bool last_page;
-	u16 sid;
-
-	prm = &evt->fault.prm;
-	sid = PCI_DEVID(bus, devfn);
-	pasid_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
-	last_page = prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
-
-	desc.qw0 = QI_PGRP_PASID(prm->pasid) | QI_PGRP_DID(sid) |
-			QI_PGRP_PASID_P(pasid_present) |
-			QI_PGRP_RESP_CODE(msg->code) |
-			QI_PGRP_RESP_TYPE;
-	desc.qw1 = QI_PGRP_IDX(prm->grpid) | QI_PGRP_LPIG(last_page);
-	desc.qw2 = 0;
-	desc.qw3 = 0;
-
-	qi_submit_sync(iommu, &desc, 1, 0);
-}
-
 static void intel_svm_domain_free(struct iommu_domain *domain)
 {
 	struct dmar_domain *dmar_domain = to_dmar_domain(domain);

-- 
2.43.0

RE: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Tian, Kevin]

> From: Joel Granados via B4 Relay
> <devnull+j.granados.samsung.com@kernel.org>
> 
> From: Joel Granados <j.granados@samsung.com>
> 
> IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
> Move
> all Page Request Queue (PRQ) functions that handle prq events to a new
> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> declared in drivers/iommu/intel/prq.c.
> 
> No functional changes are intended. This is a preparation patch to
> enable the use of IO page faults outside the SVM/PASID use cases.

Do we want to guard it under a new config option e.g.
CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
for the majority usages which don't require IOPF.

Baolu?

> -#ifdef CONFIG_INTEL_IOMMU_SVM
>  	if (pasid_supported(iommu)) {
>  		if (ecap_prs(iommu->ecap))
> -			intel_svm_finish_prq(iommu);
> +			intel_finish_prq(iommu);
>  	}
> -#endif

either intel_iommu_finish_prq() or intel_prq_finish().

same for other helpers.

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Baolu Lu]

On 9/14/24 8:52 AM, Tian, Kevin wrote:
>> From: Joel Granados via B4 Relay
>> <devnull+j.granados.samsung.com@kernel.org>
>>
>> From: Joel Granados<j.granados@samsung.com>
>>
>> IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
>> Move
>> all Page Request Queue (PRQ) functions that handle prq events to a new
>> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
>> declared in drivers/iommu/intel/prq.c.
>>
>> No functional changes are intended. This is a preparation patch to
>> enable the use of IO page faults outside the SVM/PASID use cases.
> Do we want to guard it under a new config option e.g.
> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> for the majority usages which don't require IOPF.
> 
> Baolu?

The OS builder doesn't know if Linux will run on a platform with PRI-
capable devices. They'll probably always enable this option if we
provide it.

This option could be useful for embedded systems, but I'm not sure if
any embedded systems have VT-d hardware, which is mainly for high-end
PCs or cloud servers.

So, maybe we could leave it as is for now and add it later if we see a
real use case.

Thanks,
baolu

RE: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Saturday, September 14, 2024 9:18 AM
> 
> On 9/14/24 8:52 AM, Tian, Kevin wrote:
> >> From: Joel Granados via B4 Relay
> >> <devnull+j.granados.samsung.com@kernel.org>
> >>
> >> From: Joel Granados<j.granados@samsung.com>
> >>
> >> IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
> >> Move
> >> all Page Request Queue (PRQ) functions that handle prq events to a new
> >> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> >> declared in drivers/iommu/intel/prq.c.
> >>
> >> No functional changes are intended. This is a preparation patch to
> >> enable the use of IO page faults outside the SVM/PASID use cases.
> > Do we want to guard it under a new config option e.g.
> > CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> > for the majority usages which don't require IOPF.
> >
> > Baolu?
> 
> The OS builder doesn't know if Linux will run on a platform with PRI-
> capable devices. They'll probably always enable this option if we
> provide it.

hmm then why do we need a SVM option? In reality I haven't seen
a platform which supports IOPF but no pasid/SVM. so the reason
for whether to have an option should be same between IOPF/SVM.

IMHO the point of options is to allow reducing footprint of the kernel
image and many options are probably always enabled in distributions...

> 
> This option could be useful for embedded systems, but I'm not sure if
> any embedded systems have VT-d hardware, which is mainly for high-end
> PCs or cloud servers.
> 
> So, maybe we could leave it as is for now and add it later if we see a
> real use case.
> 

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Baolu Lu]

On 2024/9/14 10:53, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Saturday, September 14, 2024 9:18 AM
>>
>> On 9/14/24 8:52 AM, Tian, Kevin wrote:
>>>> From: Joel Granados via B4 Relay
>>>> <devnull+j.granados.samsung.com@kernel.org>
>>>>
>>>> From: Joel Granados<j.granados@samsung.com>
>>>>
>>>> IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
>>>> Move
>>>> all Page Request Queue (PRQ) functions that handle prq events to a new
>>>> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
>>>> declared in drivers/iommu/intel/prq.c.
>>>>
>>>> No functional changes are intended. This is a preparation patch to
>>>> enable the use of IO page faults outside the SVM/PASID use cases.
>>> Do we want to guard it under a new config option e.g.
>>> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
>>> for the majority usages which don't require IOPF.
>>>
>>> Baolu?
>> The OS builder doesn't know if Linux will run on a platform with PRI-
>> capable devices. They'll probably always enable this option if we
>> provide it.
> hmm then why do we need a SVM option? In reality I haven't seen
> a platform which supports IOPF but no pasid/SVM. so the reason
> for whether to have an option should be same between IOPF/SVM.
> 
> IMHO the point of options is to allow reducing footprint of the kernel
> image and many options are probably always enabled in distributions...

To be honest, I would hope to remove the SVM option some day. It's
nothing special except listening to an external notification and
synchronize the caches when the page table is updated. It's common to
all cases where a page table is shared between the IOMMU and another
component.

As for CONFIG_INTEL_IOMMU_IOPF, my suggestion is that we don't need to
add any unnecessary options unless we see a real need.

Thanks,
baolu

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Jason Gunthorpe]

On Sat, Sep 14, 2024 at 01:49:44PM +0800, Baolu Lu wrote:
> On 2024/9/14 10:53, Tian, Kevin wrote:
> > > From: Baolu Lu<baolu.lu@linux.intel.com>
> > > Sent: Saturday, September 14, 2024 9:18 AM
> > > 
> > > On 9/14/24 8:52 AM, Tian, Kevin wrote:
> > > > > From: Joel Granados via B4 Relay
> > > > > <devnull+j.granados.samsung.com@kernel.org>
> > > > > 
> > > > > From: Joel Granados<j.granados@samsung.com>
> > > > > 
> > > > > IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
> > > > > Move
> > > > > all Page Request Queue (PRQ) functions that handle prq events to a new
> > > > > file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> > > > > declared in drivers/iommu/intel/prq.c.
> > > > > 
> > > > > No functional changes are intended. This is a preparation patch to
> > > > > enable the use of IO page faults outside the SVM/PASID use cases.
> > > > Do we want to guard it under a new config option e.g.
> > > > CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> > > > for the majority usages which don't require IOPF.
> > > > 
> > > > Baolu?
> > > The OS builder doesn't know if Linux will run on a platform with PRI-
> > > capable devices. They'll probably always enable this option if we
> > > provide it.
> > hmm then why do we need a SVM option? In reality I haven't seen
> > a platform which supports IOPF but no pasid/SVM. so the reason
> > for whether to have an option should be same between IOPF/SVM.
> > 
> > IMHO the point of options is to allow reducing footprint of the kernel
> > image and many options are probably always enabled in distributions...
> 
> To be honest, I would hope to remove the SVM option some day. It's
> nothing special except listening to an external notification and
> synchronize the caches when the page table is updated. It's common to
> all cases where a page table is shared between the IOMMU and another
> component.
> 
> As for CONFIG_INTEL_IOMMU_IOPF, my suggestion is that we don't need to
> add any unnecessary options unless we see a real need.

You could possibly bundle the SVA and IOPF options together

I called the new option on the ARM side CONFIG_ARM_SMMU_V3_IOMMUFD
which seems like a reasonable cut point against embedded vs server.

Jason

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Joel Granados]

On Sun, Sep 15, 2024 at 10:49:28AM -0300, Jason Gunthorpe wrote:
> On Sat, Sep 14, 2024 at 01:49:44PM +0800, Baolu Lu wrote:
> > On 2024/9/14 10:53, Tian, Kevin wrote:
> > > > From: Baolu Lu<baolu.lu@linux.intel.com>
> > > > Sent: Saturday, September 14, 2024 9:18 AM
> > > > 
> > > > On 9/14/24 8:52 AM, Tian, Kevin wrote:
> > > > > > From: Joel Granados via B4 Relay
> > > > > > <devnull+j.granados.samsung.com@kernel.org>
> > > > > > 
> > > > > > From: Joel Granados<j.granados@samsung.com>
> > > > > > 
> > > > > > IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
> > > > > > Move
> > > > > > all Page Request Queue (PRQ) functions that handle prq events to a new
> > > > > > file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> > > > > > declared in drivers/iommu/intel/prq.c.
> > > > > > 
> > > > > > No functional changes are intended. This is a preparation patch to
> > > > > > enable the use of IO page faults outside the SVM/PASID use cases.
> > > > > Do we want to guard it under a new config option e.g.
> > > > > CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> > > > > for the majority usages which don't require IOPF.
> > > > > 
> > > > > Baolu?
> > > > The OS builder doesn't know if Linux will run on a platform with PRI-
> > > > capable devices. They'll probably always enable this option if we
> > > > provide it.
> > > hmm then why do we need a SVM option? In reality I haven't seen
> > > a platform which supports IOPF but no pasid/SVM. so the reason
> > > for whether to have an option should be same between IOPF/SVM.
> > > 
> > > IMHO the point of options is to allow reducing footprint of the kernel
> > > image and many options are probably always enabled in distributions...
> > 
> > To be honest, I would hope to remove the SVM option some day. It's
> > nothing special except listening to an external notification and
> > synchronize the caches when the page table is updated. It's common to
> > all cases where a page table is shared between the IOMMU and another
> > component.
> > 
> > As for CONFIG_INTEL_IOMMU_IOPF, my suggestion is that we don't need to
> > add any unnecessary options unless we see a real need.
> 
> You could possibly bundle the SVA and IOPF options together
> 
> I called the new option on the ARM side CONFIG_ARM_SMMU_V3_IOMMUFD
> which seems like a reasonable cut point against embedded vs server.

I'll go with Baolu's suggestion of leaving as is for my V3.

Thx for the review

-- 

Joel Granados

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Baolu Lu]

On 9/15/24 9:49 PM, Jason Gunthorpe wrote:
> On Sat, Sep 14, 2024 at 01:49:44PM +0800, Baolu Lu wrote:
>> On 2024/9/14 10:53, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Saturday, September 14, 2024 9:18 AM
>>>>
>>>> On 9/14/24 8:52 AM, Tian, Kevin wrote:
>>>>>> From: Joel Granados via B4 Relay
>>>>>> <devnull+j.granados.samsung.com@kernel.org>
>>>>>>
>>>>>> From: Joel Granados<j.granados@samsung.com>
>>>>>>
>>>>>> IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
>>>>>> Move
>>>>>> all Page Request Queue (PRQ) functions that handle prq events to a new
>>>>>> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
>>>>>> declared in drivers/iommu/intel/prq.c.
>>>>>>
>>>>>> No functional changes are intended. This is a preparation patch to
>>>>>> enable the use of IO page faults outside the SVM/PASID use cases.
>>>>> Do we want to guard it under a new config option e.g.
>>>>> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
>>>>> for the majority usages which don't require IOPF.
>>>>>
>>>>> Baolu?
>>>> The OS builder doesn't know if Linux will run on a platform with PRI-
>>>> capable devices. They'll probably always enable this option if we
>>>> provide it.
>>> hmm then why do we need a SVM option? In reality I haven't seen
>>> a platform which supports IOPF but no pasid/SVM. so the reason
>>> for whether to have an option should be same between IOPF/SVM.
>>>
>>> IMHO the point of options is to allow reducing footprint of the kernel
>>> image and many options are probably always enabled in distributions...
>> To be honest, I would hope to remove the SVM option some day. It's
>> nothing special except listening to an external notification and
>> synchronize the caches when the page table is updated. It's common to
>> all cases where a page table is shared between the IOMMU and another
>> component.
>>
>> As for CONFIG_INTEL_IOMMU_IOPF, my suggestion is that we don't need to
>> add any unnecessary options unless we see a real need.
> You could possibly bundle the SVA and IOPF options together
> 
> I called the new option on the ARM side CONFIG_ARM_SMMU_V3_IOMMUFD
> which seems like a reasonable cut point against embedded vs server.

Probably I will consider this after this series. This is not intel iommu
specific, hence it's better to make it consistent for all drivers.

Thanks,
baolu

RE: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Saturday, September 14, 2024 1:50 PM
> 
> On 2024/9/14 10:53, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@linux.intel.com>
> >> Sent: Saturday, September 14, 2024 9:18 AM
> >>
> >> On 9/14/24 8:52 AM, Tian, Kevin wrote:
> >>>> From: Joel Granados via B4 Relay
> >>>> <devnull+j.granados.samsung.com@kernel.org>
> >>>>
> >>>> From: Joel Granados<j.granados@samsung.com>
> >>>>
> >>>> IO page faults are no longer dependent on
> CONFIG_INTEL_IOMMU_SVM.
> >>>> Move
> >>>> all Page Request Queue (PRQ) functions that handle prq events to a
> new
> >>>> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> >>>> declared in drivers/iommu/intel/prq.c.
> >>>>
> >>>> No functional changes are intended. This is a preparation patch to
> >>>> enable the use of IO page faults outside the SVM/PASID use cases.
> >>> Do we want to guard it under a new config option e.g.
> >>> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> >>> for the majority usages which don't require IOPF.
> >>>
> >>> Baolu?
> >> The OS builder doesn't know if Linux will run on a platform with PRI-
> >> capable devices. They'll probably always enable this option if we
> >> provide it.
> > hmm then why do we need a SVM option? In reality I haven't seen
> > a platform which supports IOPF but no pasid/SVM. so the reason
> > for whether to have an option should be same between IOPF/SVM.
> >
> > IMHO the point of options is to allow reducing footprint of the kernel
> > image and many options are probably always enabled in distributions...
> 
> To be honest, I would hope to remove the SVM option some day. It's
> nothing special except listening to an external notification and
> synchronize the caches when the page table is updated. 

more than that... for each IOMMU the current code allocates 16 pages
and 1 hwirq. Those are unnecessary burdens in majority deployments
which don't support/require I/O page faults.

> It's common to
> all cases where a page table is shared between the IOMMU and another
> component.

but "all cases" with shared page table is actually a small group now.

> 
> As for CONFIG_INTEL_IOMMU_IOPF, my suggestion is that we don't need to
> add any unnecessary options unless we see a real need.
> 

so I was thinking the opposite, i.e. keeping the option until doing so
becomes a real burden.😊

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Baolu Lu]

On 2024/9/18 16:20, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Saturday, September 14, 2024 1:50 PM
>>
>> On 2024/9/14 10:53, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Saturday, September 14, 2024 9:18 AM
>>>>
>>>> On 9/14/24 8:52 AM, Tian, Kevin wrote:
>>>>>> From: Joel Granados via B4 Relay
>>>>>> <devnull+j.granados.samsung.com@kernel.org>
>>>>>>
>>>>>> From: Joel Granados<j.granados@samsung.com>
>>>>>>
>>>>>> IO page faults are no longer dependent on
>> CONFIG_INTEL_IOMMU_SVM.
>>>>>> Move
>>>>>> all Page Request Queue (PRQ) functions that handle prq events to a
>> new
>>>>>> file in drivers/iommu/intel/prq.c. The page_req_des struct is now
>>>>>> declared in drivers/iommu/intel/prq.c.
>>>>>>
>>>>>> No functional changes are intended. This is a preparation patch to
>>>>>> enable the use of IO page faults outside the SVM/PASID use cases.
>>>>> Do we want to guard it under a new config option e.g.
>>>>> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
>>>>> for the majority usages which don't require IOPF.
>>>>>
>>>>> Baolu?
>>>> The OS builder doesn't know if Linux will run on a platform with PRI-
>>>> capable devices. They'll probably always enable this option if we
>>>> provide it.
>>> hmm then why do we need a SVM option? In reality I haven't seen
>>> a platform which supports IOPF but no pasid/SVM. so the reason
>>> for whether to have an option should be same between IOPF/SVM.
>>>
>>> IMHO the point of options is to allow reducing footprint of the kernel
>>> image and many options are probably always enabled in distributions...
>> To be honest, I would hope to remove the SVM option some day. It's
>> nothing special except listening to an external notification and
>> synchronize the caches when the page table is updated.
> more than that... for each IOMMU the current code allocates 16 pages
> and 1 hwirq. Those are unnecessary burdens in majority deployments
> which don't support/require I/O page faults.

Yeah! I only focused on the kernel binary size but ignored these system
resources consumed by IOPF. Then, perhaps

diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index f52fb39c968e..847a5c43c9dc 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -97,4 +97,7 @@ config INTEL_IOMMU_PERF_EVENTS
           to aid performance tuning and debug. These are available on 
modern
           processors which support Intel VT-d 4.0 and later.

+config INTEL_IOMMU_IOPF
+       depends on IOMMUFD || INTEL_IOMMU_SVM
+
  endif # INTEL_IOMMU
diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
index c8beb0281559..c382307ae7aa 100644
--- a/drivers/iommu/intel/Makefile
+++ b/drivers/iommu/intel/Makefile
@@ -5,6 +5,7 @@ obj-$(CONFIG_DMAR_TABLE) += trace.o cap_audit.o
  obj-$(CONFIG_DMAR_PERF) += perf.o
  obj-$(CONFIG_INTEL_IOMMU_DEBUGFS) += debugfs.o
  obj-$(CONFIG_INTEL_IOMMU_SVM) += svm.o
+obj-$(CONFIG_INTEL_IOMMU_IOPF) += prq.o
  ifdef CONFIG_INTEL_IOMMU
  obj-$(CONFIG_IRQ_REMAP) += irq_remapping.o
  endif

?

Thanks,
baolu

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Jason Gunthorpe]

On Wed, Sep 18, 2024 at 07:17:32PM +0800, Baolu Lu wrote:
> > more than that... for each IOMMU the current code allocates 16 pages
> > and 1 hwirq. Those are unnecessary burdens in majority deployments
> > which don't support/require I/O page faults.
> 
> Yeah! I only focused on the kernel binary size but ignored these system
> resources consumed by IOPF. Then, perhaps

If you care about runtime overhead it should be delt with by
dynamically allocating the memory and enabling it, not via kconfig

We can dynmaically add IRQS in some cases now for instance

Jason

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Joel Granados]

On Fri, Sep 20, 2024 at 09:54:34AM -0300, Jason Gunthorpe wrote:
> On Wed, Sep 18, 2024 at 07:17:32PM +0800, Baolu Lu wrote:
> > > more than that... for each IOMMU the current code allocates 16 pages
> > > and 1 hwirq. Those are unnecessary burdens in majority deployments
> > > which don't support/require I/O page faults.
> > 
> > Yeah! I only focused on the kernel binary size but ignored these system
> > resources consumed by IOPF. Then, perhaps
> 
> If you care about runtime overhead it should be delt with by
> dynamically allocating the memory and enabling it, not via kconfig
> 
> We can dynmaically add IRQS in some cases now for instance
> 
> Jason

Summary (Please correct if inaccurate):
1. Kevin Tian & Baolu Lu have proposed a kconfig guard
   (INTEL_IOMMU_IOPF) to avoid unnecessary resource allocation (of 16
   pages and 1 hwirq). It can be keep it until it becomes a burden.
2. Jason Gunthorp: runtime overhead should be handled by dynamically
   allocating memory and enabling it. Not via Kconfig.

There was no real consensus reached here. I'll leave IOMMU_IOPF guarded
under INTEL_IOMMU (no changes from V2), two reasons for this IMO:
1. The reasoning being that any system that has the resources for
   INTEL_IOMMU has them for IOMMU_IOPF.
2. If the IOPF resources are a burden, they should be solved by changing
   the way we allocate memory instead of hiding them behind a kconfig.

Quick Note: I am adding my new email to the thread so I get the responses
routed to the correct inbox.

Best

-- 

Joel Granados

Re: [PATCH v2 1/5] iommu/vt-d: Separate page request queue from SVM

[Joel Granados]

On Sat, Sep 14, 2024 at 12:52:22AM +0000, Tian, Kevin wrote:
> > From: Joel Granados via B4 Relay
> > <devnull+j.granados.samsung.com@kernel.org>
> > 
> > From: Joel Granados <j.granados@samsung.com>
> > 
> > IO page faults are no longer dependent on CONFIG_INTEL_IOMMU_SVM.
> > Move
> > all Page Request Queue (PRQ) functions that handle prq events to a new
> > file in drivers/iommu/intel/prq.c. The page_req_des struct is now
> > declared in drivers/iommu/intel/prq.c.
> > 
> > No functional changes are intended. This is a preparation patch to
> > enable the use of IO page faults outside the SVM/PASID use cases.
> 
> Do we want to guard it under a new config option e.g.
> CONFIG_INTEL_IOMMU_IOPF? it's unnecessary to allocate resources
> for the majority usages which don't require IOPF.
> 
> Baolu?
> 
> > -#ifdef CONFIG_INTEL_IOMMU_SVM
> >  	if (pasid_supported(iommu)) {
> >  		if (ecap_prs(iommu->ecap))
> > -			intel_svm_finish_prq(iommu);
> > +			intel_finish_prq(iommu);
> >  	}
> > -#endif
> 
> either intel_iommu_finish_prq() or intel_prq_finish().

Thx; I see the pattern now! The first (Adding "_iommu_" to the name)
makes more sense to me as I see some intel_iommu_* function further down
in the iommu.h file.

> 
> same for other helpers.

Will change for the next version

Best

-- 

Joel Granados

[PATCH v2 2/5] iommu/vt-d: Remove the pasid present check in prq_event_thread

[Joel Granados via B4 Relay]

From: Klaus Jensen <k.jensen@samsung.com>

PASID is not strictly needed when handling a PRQ event; remove the check
for the pasid present bit in the request. This change was not included
in the creation of prq.c to emphasize the change in capability checks
when handing PRQ events.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/iommu/intel/prq.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/intel/prq.c b/drivers/iommu/intel/prq.c
index 3376f60082b5..5f2d01a9aa11 100644
--- a/drivers/iommu/intel/prq.c
+++ b/drivers/iommu/intel/prq.c
@@ -221,18 +221,12 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		req = &iommu->prq[head / sizeof(*req)];
 		address = (u64)req->addr << VTD_PAGE_SHIFT;
 
-		if (unlikely(!req->pasid_present)) {
-			pr_err("IOMMU: %s: Page request without PASID\n",
-			       iommu->name);
-bad_req:
-			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
-			goto prq_advance;
-		}
-
 		if (unlikely(!is_canonical_address(address))) {
 			pr_err("IOMMU: %s: Address is not canonical\n",
 			       iommu->name);
-			goto bad_req;
+bad_req:
+			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
+			goto prq_advance;
 		}
 
 		if (unlikely(req->pm_req && (req->rd_req | req->wr_req))) {

-- 
2.43.0

RE: [PATCH v2 2/5] iommu/vt-d: Remove the pasid present check in prq_event_thread

[Tian, Kevin]

> From: Joel Granados via B4 Relay
> <devnull+j.granados.samsung.com@kernel.org>
> 
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> PASID is not strictly needed when handling a PRQ event; remove the check
> for the pasid present bit in the request. This change was not included
> in the creation of prq.c to emphasize the change in capability checks
> when handing PRQ events.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Joel Granados <j.granados@samsung.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v2 3/5] iommu: kconfig: Move IOMMU_IOPF into INTEL_IOMMU

[Joel Granados via B4 Relay]

From: Joel Granados <j.granados@samsung.com>

Move IOMMU_IOPF from under INTEL_IOMMU_SVM into INTEL_IOMMU. This
certifies that the core intel iommu utilizes the IOPF library
functions, independent of the INTEL_IOMMU_SVM config.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/iommu/intel/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index f52fb39c968e..2888671c9278 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -15,6 +15,7 @@ config INTEL_IOMMU
 	select DMA_OPS
 	select IOMMU_API
 	select IOMMU_IOVA
+	select IOMMU_IOPF
 	select IOMMUFD_DRIVER if IOMMUFD
 	select NEED_DMA_MAP_STATE
 	select DMAR_TABLE
@@ -51,7 +52,6 @@ config INTEL_IOMMU_SVM
 	depends on X86_64
 	select MMU_NOTIFIER
 	select IOMMU_SVA
-	select IOMMU_IOPF
 	help
 	  Shared Virtual Memory (SVM) provides a facility for devices
 	  to access DMA resources through process address space by

-- 
2.43.0

[PATCH v2 4/5] iommufd: Enable PRI when doing the iommufd_hwpt_alloc

[Joel Granados via B4 Relay]

From: Joel Granados <j.granados@samsung.com>

Add IOMMU_HWPT_FAULT_ID_VALID as part of the valid flags when doing an
iommufd_hwpt_alloc allowing the use of an iommu fault allocation
(iommu_fault_alloc) with the IOMMU_HWPT_ALLOC ioctl.

Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/iommu/intel/iommu.c          | 3 ++-
 drivers/iommu/iommufd/hw_pagetable.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5acc52c62e8c..3d1c971eb9e5 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3718,7 +3718,8 @@ intel_iommu_domain_alloc_user(struct device *dev, u32 flags,
 	}
 
 	if (flags &
-	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)))
+	    (~(IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING
+	       | IOMMU_HWPT_FAULT_ID_VALID)))
 		return ERR_PTR(-EOPNOTSUPP);
 	if (nested_parent && !nested_supported(iommu))
 		return ERR_PTR(-EOPNOTSUPP);
diff --git a/drivers/iommu/iommufd/hw_pagetable.c b/drivers/iommu/iommufd/hw_pagetable.c
index aefde4443671..88074e459995 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -107,7 +107,8 @@ iommufd_hwpt_paging_alloc(struct iommufd_ctx *ictx, struct iommufd_ioas *ioas,
 			  const struct iommu_user_data *user_data)
 {
 	const u32 valid_flags = IOMMU_HWPT_ALLOC_NEST_PARENT |
-				IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+				IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+				IOMMU_HWPT_FAULT_ID_VALID;
 	const struct iommu_ops *ops = dev_iommu_ops(idev->dev);
 	struct iommufd_hwpt_paging *hwpt_paging;
 	struct iommufd_hw_pagetable *hwpt;

-- 
2.43.0

RE: [PATCH v2 4/5] iommufd: Enable PRI when doing the iommufd_hwpt_alloc

[Tian, Kevin]

> From: Joel Granados via B4 Relay
> <devnull+j.granados.samsung.com@kernel.org>
> 
> From: Joel Granados <j.granados@samsung.com>
> 
> Add IOMMU_HWPT_FAULT_ID_VALID as part of the valid flags when doing
> an
> iommufd_hwpt_alloc allowing the use of an iommu fault allocation
> (iommu_fault_alloc) with the IOMMU_HWPT_ALLOC ioctl.
> 
> Signed-off-by: Joel Granados <j.granados@samsung.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v2 5/5] iommu/vt-d: drop pasid requirement for prq initialization

[Joel Granados via B4 Relay]

From: Klaus Jensen <k.jensen@samsung.com>

PASID support within the IOMMU is not required to enable the Page
Request Queue, only the PRS capability.

Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
Signed-off-by: Joel Granados <j.granados@samsung.com>
---
 drivers/iommu/intel/iommu.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3d1c971eb9e5..9f3bbdbd6372 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1487,10 +1487,8 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
 	/* free context mapping */
 	free_context_table(iommu);
 
-	if (pasid_supported(iommu)) {
-		if (ecap_prs(iommu->ecap))
-			intel_finish_prq(iommu);
-	}
+	if (ecap_prs(iommu->ecap))
+		intel_finish_prq(iommu);
 }
 
 /*
@@ -2480,7 +2478,7 @@ static int __init init_dmars(void)
 
 		iommu_flush_write_buffer(iommu);
 
-		if (pasid_supported(iommu) && ecap_prs(iommu->ecap)) {
+		if (ecap_prs(iommu->ecap)) {
 			/*
 			 * Call dmar_alloc_hwirq() with dmar_global_lock held,
 			 * could cause possible lock race condition.
@@ -2921,7 +2919,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 	intel_iommu_init_qi(iommu);
 	iommu_flush_write_buffer(iommu);
 
-	if (pasid_supported(iommu) && ecap_prs(iommu->ecap)) {
+	if (ecap_prs(iommu->ecap)) {
 		ret = intel_enable_prq(iommu);
 		if (ret)
 			goto disable_iommu;

-- 
2.43.0

RE: [PATCH v2 5/5] iommu/vt-d: drop pasid requirement for prq initialization

[Tian, Kevin]

> From: Joel Granados via B4 Relay
> <devnull+j.granados.samsung.com@kernel.org>
> 
> From: Klaus Jensen <k.jensen@samsung.com>
> 
> PASID support within the IOMMU is not required to enable the Page
> Request Queue, only the PRS capability.
> 
> Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> Signed-off-by: Joel Granados <j.granados@samsung.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[Tian, Kevin]

> From: Joel Granados via B4 Relay
> <devnull+j.granados.samsung.com@kernel.org>
> 
> This series makes use of iommufd_hwpt_replace_device to execute
> non-pasid/non-svm user space IOPFs. Our main motivation is to enable
> user-space driver driven device verification without SVM/PASID.

can you elaborate why IOPFs are necessary to help verify such usage?

> 
> What?
>   * Enable IO page fault handling in user space for a non-pasid, non-svm
>     and non-virtualised use case.
>   * Move IOMMU_IOPF configuration from INTEL_IOMMU_SVM into
> INTEL_IOMMU.
>   * Move all page request queue related logic to a new (prq.c) file.
>   * Remove PASID checks from PRQ event handling as well as PRQ
>     initialization.
>   * Allow execution of IOMMU_HWPT_ALLOC with a valid fault id
>     (IOMMU_HWPT_FAULT_ID_VALID)
>   * Insert a zero handle into the PASID array in dev->iommu_group when
>     replacing the old HWPT with an IOPF enabled HWPT.

the last bullet is stale now.

btw a selftest is expected too.

Re: [PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[Joel Granados]

On Sat, Sep 14, 2024 at 12:48:31AM +0000, Tian, Kevin wrote:
> > From: Joel Granados via B4 Relay
> > <devnull+j.granados.samsung.com@kernel.org>
> > 
> > This series makes use of iommufd_hwpt_replace_device to execute
> > non-pasid/non-svm user space IOPFs. Our main motivation is to enable
> > user-space driver driven device verification without SVM/PASID.
> 
> can you elaborate why IOPFs are necessary to help verify such usage?

In retrospect "enable" might not be the best word to use here. We are not
"enabling" user-space driver driven device verification as it is already
enabled; you could already poke a device from user space. But the whole poke
space was not available, you could not test IOPF without having an SVM/PASID
capable IOMMU. Therefore a better wording would be "Our main motivation is to
expand or facilitate user-space driver driven device verification by enabling
IOPF without SMV/PASID".

Does this address your concern?

> 
> > 
> > What?
> >   * Enable IO page fault handling in user space for a non-pasid, non-svm
> >     and non-virtualised use case.
> >   * Move IOMMU_IOPF configuration from INTEL_IOMMU_SVM into
> > INTEL_IOMMU.
> >   * Move all page request queue related logic to a new (prq.c) file.
> >   * Remove PASID checks from PRQ event handling as well as PRQ
> >     initialization.
> >   * Allow execution of IOMMU_HWPT_ALLOC with a valid fault id
> >     (IOMMU_HWPT_FAULT_ID_VALID)
> >   * Insert a zero handle into the PASID array in dev->iommu_group when
> >     replacing the old HWPT with an IOPF enabled HWPT.
> 
> the last bullet is stale now.
oops. Missed that one; will correct in next version 

> 
> btw a selftest is expected too.
I'll figure this out for the next version.

Thx for the review

Best
-- 

Joel Granados

RE: [PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[Tian, Kevin]

> From: Joel Granados <j.granados@samsung.com>
> Sent: Monday, September 16, 2024 4:51 PM
> 
> On Sat, Sep 14, 2024 at 12:48:31AM +0000, Tian, Kevin wrote:
> > > From: Joel Granados via B4 Relay
> > > <devnull+j.granados.samsung.com@kernel.org>
> > >
> > > This series makes use of iommufd_hwpt_replace_device to execute
> > > non-pasid/non-svm user space IOPFs. Our main motivation is to enable
> > > user-space driver driven device verification without SVM/PASID.
> >
> > can you elaborate why IOPFs are necessary to help verify such usage?
> 
> In retrospect "enable" might not be the best word to use here. We are not
> "enabling" user-space driver driven device verification as it is already
> enabled; you could already poke a device from user space. But the whole
> poke
> space was not available, you could not test IOPF without having an
> SVM/PASID
> capable IOMMU. Therefore a better wording would be "Our main motivation
> is to
> expand or facilitate user-space driver driven device verification by enabling
> IOPF without SMV/PASID".
> 

hmm did you actually see a IOMMU which supports IOPF only but
not SVM/PASID? 

this series alone has its merit, e.g. postcopy migration might want
such notification. But not sure it helps solve a real problem in your side...

Re: [PATCH v2 0/5] iommu: Enable user space IOPFs in non-PASID and non-svm cases

[Joel Granados]

On Fri, Sep 20, 2024 at 06:57:04AM +0000, Tian, Kevin wrote:
> > From: Joel Granados <j.granados@samsung.com>
> > Sent: Monday, September 16, 2024 4:51 PM
> > 
> > On Sat, Sep 14, 2024 at 12:48:31AM +0000, Tian, Kevin wrote:
> > > > From: Joel Granados via B4 Relay
> > > > <devnull+j.granados.samsung.com@kernel.org>
> > > >
> > > > This series makes use of iommufd_hwpt_replace_device to execute
> > > > non-pasid/non-svm user space IOPFs. Our main motivation is to enable
> > > > user-space driver driven device verification without SVM/PASID.
> > >
> > > can you elaborate why IOPFs are necessary to help verify such usage?
> > 
> > In retrospect "enable" might not be the best word to use here. We are not
> > "enabling" user-space driver driven device verification as it is already
> > enabled; you could already poke a device from user space. But the whole
> > poke
> > space was not available, you could not test IOPF without having an
> > SVM/PASID
> > capable IOMMU. Therefore a better wording would be "Our main motivation
> > is to
> > expand or facilitate user-space driver driven device verification by enabling
> > IOPF without SMV/PASID".
> > 
> 
> hmm did you actually see a IOMMU which supports IOPF only but
> not SVM/PASID? 
> 
> this series alone has its merit, e.g. postcopy migration might want
> such notification. But not sure it helps solve a real problem in your side...

I understand that you want more information about what problem(s) are
solved by this patch set from my point of view. right?

One of the main motivations is to enable IOPF in use cases where PASID
is *not* an option, like NVMe devices. Therefore one of the examples for
enabling user-space driver driver device verification are NVMe without
PASID.

Quick Note: I am adding my new email to the thread so I get the
responses routed to the correct inbox.


-- 

Joel Granados