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

[Baolu Lu <baolu.lu@linux.intel.com>]

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