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

[Tian, Kevin]

> 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? 😊