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@xxxxxxxxxxxxxxx>
> > 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