Re: [PATCH v4 09/10] iommu: Make iommu_queue_iopf() more generic

[Baolu Lu]

On 9/13/23 10:25 AM, Tian, Kevin wrote:
> > From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Monday, September 11, 2023 8:27 PM
> >
> > > Out of curiosity. Is it a valid configuration which has
> > REQUEST_PASID_VALID
> > > set but RESP_PASID_VALID cleared? I'm unclear why another response
> > > flag is required beyond what the request flag has told...
> >
> > This seems to have uncovered a bug in VT-d driver.
> >
> > The PCIe spec (Section 10.4.2.2) states:
> >
> > "
> > If a Page Request has a PASID, the corresponding PRG Response Message
> > may optionally contain one as well.
> >
> > If the PRG Response PASID Required bit is Clear, PRG Response Messages
> > do not have a PASID. If the PRG Response PASID Required bit is Set, PRG
> > Response Messages have a PASID if the Page Request also had one. The
> > Function is permitted to use the PASID value from the prefix in
> > conjunction with the PRG Index to match requests and responses.
> > "
> >
> > The "PRG Response PASID Required bit" is a read-only field in the PCI
> > page request status register. It is represented by
> > "pdev->pasid_required".
> >
> > So below code in VT-d driver is not correct:
> >
> > 542 static int intel_svm_prq_report(struct intel_iommu *iommu, struct
> > device *dev,
> > 543                                 struct page_req_dsc *desc)
> > 544 {
> >
> > [...]
> >
> > 556
> > 557         if (desc->lpig)
> > 558                 event.fault.prm.flags |=
> > IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
> > 559         if (desc->pasid_present) {
> > 560                 event.fault.prm.flags |=
> > IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
> > 561                 event.fault.prm.flags |=
> > IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
> > 562         }
> > [...]
> >
> > The right logic should be
> >
> > 	if (pdev->pasid_required)
> > 		event.fault.prm.flags |=
> > IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
> >
> > Thoughts?
>
> yes, it's the right fix. We haven't seen any bug report probably because
> all SVM-capable devices have pasid_required set? 😊

More precisely, the idxd devices have pasid_required set. :-)

Anyway, I will post a formal fix for this.

Best regards,
baolu