[PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

[PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

[Lu Baolu]

The existing IOPF handling code relies on the per-PASID SVA data
structures. It does not apply to scenarios other than SVA. This
decouples the I/O page fault reporting and responding code from
SVA related data structures so that the PRQ handling code could
become generic.

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

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 08618d94fbac..175f5f559e86 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -757,7 +757,6 @@ struct intel_svm_dev {
 	struct device *dev;
 	struct intel_iommu *iommu;
 	struct iommu_sva sva;
-	unsigned long prq_seq_number;
 	u32 pasid;
 	int users;
 	u16 did;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index b391e46e537e..23a62bead112 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -32,6 +32,7 @@ static void intel_svm_drain_prq(struct device *dev, u32 pasid);
 #define to_intel_svm_dev(handle) container_of(handle, struct intel_svm_dev, sva)
 
 static DEFINE_XARRAY_ALLOC(pasid_private_array);
+static unsigned long prq_seq_number;
 static int pasid_private_add(ioasid_t pasid, void *priv)
 {
 	return xa_alloc(&pasid_private_array, &pasid, priv,
@@ -48,23 +49,6 @@ static void *pasid_private_find(ioasid_t pasid)
 	return xa_load(&pasid_private_array, pasid);
 }
 
-static struct intel_svm_dev *
-svm_lookup_device_by_sid(struct intel_svm *svm, u16 sid)
-{
-	struct intel_svm_dev *sdev = NULL, *t;
-
-	rcu_read_lock();
-	list_for_each_entry_rcu(t, &svm->devs, list) {
-		if (t->sid == sid) {
-			sdev = t;
-			break;
-		}
-	}
-	rcu_read_unlock();
-
-	return sdev;
-}
-
 static struct intel_svm_dev *
 svm_lookup_device_by_dev(struct intel_svm *svm, struct device *dev)
 {
@@ -689,11 +673,10 @@ static void handle_bad_prq_event(struct intel_iommu *iommu,
 
 static irqreturn_t prq_event_thread(int irq, void *d)
 {
-	struct intel_svm_dev *sdev = NULL;
 	struct intel_iommu *iommu = d;
-	struct intel_svm *svm = NULL;
 	struct page_req_dsc *req;
 	int head, tail, handled;
+	struct pci_dev *pdev;
 	u64 address;
 
 	/*
@@ -713,8 +696,6 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 			pr_err("IOMMU: %s: Page request without PASID\n",
 			       iommu->name);
 bad_req:
-			svm = NULL;
-			sdev = NULL;
 			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
 			goto prq_advance;
 		}
@@ -741,34 +722,15 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 		if (unlikely(req->lpig && !req->rd_req && !req->wr_req))
 			goto prq_advance;
 
-		if (!svm || svm->pasid != req->pasid) {
-			/*
-			 * It can't go away, because the driver is not permitted
-			 * to unbind the mm while any page faults are outstanding.
-			 */
-			svm = pasid_private_find(req->pasid);
-			if (IS_ERR_OR_NULL(svm))
-				goto bad_req;
-		}
-
-		if (!sdev || sdev->sid != req->rid) {
-			sdev = svm_lookup_device_by_sid(svm, req->rid);
-			if (!sdev)
-				goto bad_req;
-		}
-
-		sdev->prq_seq_number++;
-
-		/*
-		 * If prq is to be handled outside iommu driver via receiver of
-		 * the fault notifiers, we skip the page response here.
-		 */
-		if (intel_svm_prq_report(iommu, sdev->dev, req))
+		pdev = pci_get_domain_bus_and_slot(iommu->segment,
+						   PCI_BUS_NUM(req->rid),
+						   req->rid & 0xff);
+		if (!pdev || intel_svm_prq_report(iommu, &pdev->dev, req))
 			handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
 
-		trace_prq_report(iommu, sdev->dev, req->qw_0, req->qw_1,
+		trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
 				 req->priv_data[0], req->priv_data[1],
-				 sdev->prq_seq_number);
+				 prq_seq_number++);
 prq_advance:
 		head = (head + sizeof(*req)) & PRQ_RING_MASK;
 	}
@@ -803,8 +765,6 @@ int intel_svm_page_response(struct device *dev,
 			    struct iommu_page_response *msg)
 {
 	struct iommu_fault_page_request *prm;
-	struct intel_svm_dev *sdev = NULL;
-	struct intel_svm *svm = NULL;
 	struct intel_iommu *iommu;
 	bool private_present;
 	bool pasid_present;
@@ -823,8 +783,6 @@ int intel_svm_page_response(struct device *dev,
 	if (!msg || !evt)
 		return -EINVAL;
 
-	mutex_lock(&pasid_mutex);
-
 	prm = &evt->fault.prm;
 	sid = PCI_DEVID(bus, devfn);
 	pasid_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
@@ -841,12 +799,6 @@ int intel_svm_page_response(struct device *dev,
 		goto out;
 	}
 
-	ret = pasid_to_svm_sdev(dev, prm->pasid, &svm, &sdev);
-	if (ret || !sdev) {
-		ret = -ENODEV;
-		goto out;
-	}
-
 	/*
 	 * Per VT-d spec. v3.0 ch7.7, system software must respond
 	 * with page group response if private data is present (PDP)
@@ -876,7 +828,6 @@ int intel_svm_page_response(struct device *dev,
 		qi_submit_sync(iommu, &desc, 1, 0);
 	}
 out:
-	mutex_unlock(&pasid_mutex);
 	return ret;
 }
 
-- 
2.25.1

RE: [PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Thursday, September 8, 2022 10:36 AM
> 
> The existing IOPF handling code relies on the per-PASID SVA data
> structures. It does not apply to scenarios other than SVA. This
> decouples the I/O page fault reporting and responding code from
> SVA related data structures so that the PRQ handling code could
> become generic.

I think the point is that it's unnecessary to access those SVA data
in the fault path. otherwise 'decouple' reads like an alternative
implementation is added instead of just removing the code.

Overall this is a nice cleanup, but a few nits here:

> -		/*
> -		 * If prq is to be handled outside iommu driver via receiver of
> -		 * the fault notifiers, we skip the page response here.
> -		 */

I didn't understand what this comment is trying to say. But just want
to confirm removing it is the desired thing given the original code below
it is still kept below...

> -		if (intel_svm_prq_report(iommu, sdev->dev, req))
> +		pdev = pci_get_domain_bus_and_slot(iommu->segment,
> +						   PCI_BUS_NUM(req->rid),
> +						   req->rid & 0xff);
> +		if (!pdev || intel_svm_prq_report(iommu, &pdev->dev, req))
>  			handle_bad_prq_event(iommu, req,
> QI_RESP_INVALID);
> 
> -		trace_prq_report(iommu, sdev->dev, req->qw_0, req->qw_1,
> +		trace_prq_report(iommu, &pdev->dev, req->qw_0, req-
> >qw_1,
>  				 req->priv_data[0], req->priv_data[1],
> -				 sdev->prq_seq_number);
> +				 prq_seq_number++);

Previously this is counted per device but now becomes global. Could it
be stored elsewhere in a per-device structure?

Re: [PATCH 1/1] iommu/vt-d: Make SVA and IOPF irrelevant

[Baolu Lu]

On 2022/9/9 16:33, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Thursday, September 8, 2022 10:36 AM
>>
>> The existing IOPF handling code relies on the per-PASID SVA data
>> structures. It does not apply to scenarios other than SVA. This
>> decouples the I/O page fault reporting and responding code from
>> SVA related data structures so that the PRQ handling code could
>> become generic.
> 
> I think the point is that it's unnecessary to access those SVA data
> in the fault path. otherwise 'decouple' reads like an alternative
> implementation is added instead of just removing the code.

Makes sense. I will rephrase the commit message like this:

iommu/vt-d: Remove unnecessary SVA data accesses in page fault path

The existing I/O page fault handling code accesses the per-PASID SVA data
structures. This is unnecessary and makes the fault handling code only
suitable for SVA scenarios. This removes the SVA data accesses from the
I/O page fault reporting and responding code, so that the fault handling
code could be generic.

> 
> Overall this is a nice cleanup, but a few nits here:
> 
>> -		/*
>> -		 * If prq is to be handled outside iommu driver via receiver of
>> -		 * the fault notifiers, we skip the page response here.
>> -		 */
> 
> I didn't understand what this comment is trying to say. But just want
> to confirm removing it is the desired thing given the original code below
> it is still kept below...

I carelessly removed this comment. Yes. It still makes sense. I will add
it back.

> 
>> -		if (intel_svm_prq_report(iommu, sdev->dev, req))
>> +		pdev = pci_get_domain_bus_and_slot(iommu->segment,
>> +						   PCI_BUS_NUM(req->rid),
>> +						   req->rid & 0xff);
>> +		if (!pdev || intel_svm_prq_report(iommu, &pdev->dev, req))
>>   			handle_bad_prq_event(iommu, req,
>> QI_RESP_INVALID);
>>
>> -		trace_prq_report(iommu, sdev->dev, req->qw_0, req->qw_1,
>> +		trace_prq_report(iommu, &pdev->dev, req->qw_0, req-
>>> qw_1,
>>   				 req->priv_data[0], req->priv_data[1],
>> -				 sdev->prq_seq_number);
>> +				 prq_seq_number++);
> 
> Previously this is counted per device but now becomes global. Could it
> be stored elsewhere in a per-device structure?

I think the per-IOMMU structure might be the right place to store it.
Those faults share a page fault queue.

Best regards,
baolu