>-----Original Message----- >From: Liu, Yi L <[email protected]> >Subject: Re: [PATCH v5 08/15] intel_iommu: Refactor PASID processing to use >IOMMU_NO_PASID internally > >On 5/20/26 19:00, Duan, Zhenzhong wrote: >>>>>> @@ -2121,10 +2112,6 @@ static bool >>>>> vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >>>>>> >>>>>> vtd_iommu_lock(s); >>>>>> >>>>>> - if (pasid == PCI_NO_PASID && s->root_scalable) { >>>>>> - pasid = IOMMU_NO_PASID; >>>>>> - } >>>>>> - >>>>>> /* Try to fetch pte from IOTLB */ >>>>>> iotlb_entry = vtd_lookup_iotlb(s, source_id, pasid, addr); >>>>>> if (iotlb_entry) { >>>>>> @@ -2235,7 +2222,7 @@ static bool >>>>> vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >>>>>> if (ret_fr) { >>>>>> if (!vtd_is_recoverable_fault(-ret_fr, iommu_idx)) { >>>>>> vtd_report_fault(s, -ret_fr, is_fpd_set, source_id, >>>>>> - addr, is_write, pasid != PCI_NO_PASID, >>>>>> pasid); >>>>>> + addr, is_write, s->root_scalable, pasid); >>>>> >>>>> a typo here? s->root_scalable should be "pasid != IOMMU_NO_PASID"? >>>> >>>> This is intentional. >>>> pasid = vtd_as->pasid = iommu_pasid, so it never equals PCI_NO_PASID now. >>> >>> not quite get. It can still possibly be IOMMU_NO_PASID, right? If not, >>> this looks to have introduced functional change, which should not be >>> expected by this patch. >> >> Before this patch, If pasid == PCI_NO_PASID && s->root_scalable is true, >> then we >pass is_pasid=true to vtd_report_fault(). >> If we change to use "pasid != IOMMU_NO_PASID" in this patch, then we pass >is_pasid=false to vtd_report_fault() for the same scenario because pasid is >IOMMU_NO_PASID now. >> >> But using s->root_scalable could keep functions unchanged, right? > >got your point. But this helper should cover both translation w/ and w/o >pasid, it should not reply on the s->root_scalable, but rely on if pasid >is included. So the logic does not look correct. > >checked the original commit. It seems recording the rid_pasid in the >fault reporting wrongly. And that's why you got the current logic here. >May need a fix. Below is the original commit, and the spec definition. > >- commit: 1b2b12376c8a513a0c7b5e3b8ea702038d3d7db5 > >- Spec says pasid is only valid when the fault request is request with > PASID. > > " > When Set, indicates the faulted request has a PASID TLP Prefix. The > value of the PASID field is reported in the PASID Value (PV) field. > "
I see, you are right, will use "pasid != IOMMU_NO_PASID" in this patch, let me know if you want a separate fix patch instead. Thanks Zhenzhong
