Hi Eric, >-----Original Message----- >From: Eric Auger <[email protected]> >Subject: Re: [PATCH v6 02/22] intel_iommu: Delete RPS capability related >supporting code > >Hi Zhenzhong, > >On 9/18/25 10:57 AM, Zhenzhong Duan wrote: >> RID-PASID Support(RPS) is not set in vIOMMU ECAP register, the supporting >> code is there but never take effect. >takes
Will do >> >> Meanwhile, according to VTD spec section 3.4.3: >> "Implementations not supporting RID_PASID capability (ECAP_REG.RPS is >0b), >> use a PASID value of 0 to perform address translation for requests without >> PASID." >> >> We should delete the supporting code which fetches RID_PASID field from >> scalable context entry and use 0 as RID_PASID directly, because RID_PASID >> field is ignored if no RPS support according to spec. >> >> This simplify the code and doesn't bring any penalty. >simplifies Will do >> >> Opportunistically, s/rid2pasid/rid_pasid and s/RID2PASID/RID_PASID as >> VTD spec uses RID_PASID terminology. >> >> Suggested-by: Yi Liu <[email protected]> >> Signed-off-by: Zhenzhong Duan <[email protected]> >> --- >> hw/i386/intel_iommu_internal.h | 1 - >> hw/i386/intel_iommu.c | 49 +++++++++++++--------------------- >> 2 files changed, 19 insertions(+), 31 deletions(-) >> >> diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> index 360e937989..6abe76556a 100644 >> --- a/hw/i386/intel_iommu_internal.h >> +++ b/hw/i386/intel_iommu_internal.h >> @@ -547,7 +547,6 @@ typedef struct VTDRootEntry VTDRootEntry; >> #define VTD_CTX_ENTRY_LEGACY_SIZE 16 >> #define VTD_CTX_ENTRY_SCALABLE_SIZE 32 >> >> -#define VTD_SM_CONTEXT_ENTRY_RID2PASID_MASK 0xfffff >> #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL0(aw) (0x1e0ULL | >~VTD_HAW_MASK(aw)) >> #define VTD_SM_CONTEXT_ENTRY_RSVD_VAL1 >0xffffffffffe00000ULL >> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 71b70b795d..b976b251bc 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -41,8 +41,7 @@ >> #include "trace.h" >> >> /* context entry operations */ >> -#define VTD_CE_GET_RID2PASID(ce) \ >> - ((ce)->val[1] & VTD_SM_CONTEXT_ENTRY_RID2PASID_MASK) >> +#define RID_PASID 0 >I would call that RID_PASID_0 to make it more explicit in the code >or even it is a PASID to PASID_0 would do the job too. OK, will use PASID_0 >> #define VTD_CE_GET_PASID_DIR_TABLE(ce) \ >> ((ce)->val[0] & VTD_PASID_DIR_BASE_ADDR_MASK) >> >> @@ -951,7 +950,7 @@ static int vtd_ce_get_pasid_entry(IntelIOMMUState >*s, VTDContextEntry *ce, >> int ret = 0; >> >> if (pasid == PCI_NO_PASID) { >> - pasid = VTD_CE_GET_RID2PASID(ce); >> + pasid = RID_PASID; >> } >> pasid_dir_base = VTD_CE_GET_PASID_DIR_TABLE(ce); >> ret = vtd_get_pe_from_pasid_table(s, pasid_dir_base, pasid, pe); >> @@ -970,7 +969,7 @@ static int vtd_ce_get_pasid_fpd(IntelIOMMUState >*s, >> VTDPASIDEntry pe; >> >> if (pasid == PCI_NO_PASID) { >> - pasid = VTD_CE_GET_RID2PASID(ce); >> + pasid = RID_PASID; >> } >> pasid_dir_base = VTD_CE_GET_PASID_DIR_TABLE(ce); >> >> @@ -1510,15 +1509,14 @@ static inline int >vtd_context_entry_rsvd_bits_check(IntelIOMMUState *s, >> return 0; >> } >> >> -static int vtd_ce_rid2pasid_check(IntelIOMMUState *s, >> +static int vtd_ce_rid_pasid_check(IntelIOMMUState *s, >> VTDContextEntry *ce) >> { >> VTDPASIDEntry pe; >> >> /* >> * Make sure in Scalable Mode, a present context entry >> - * has valid rid2pasid setting, which includes valid >> - * rid2pasid field and corresponding pasid entry setting >> + * has valid pasid entry setting at RID_PASID(0). >s/at RID_PASID(0) /for PASID_0? Sure >> */ >> return vtd_ce_get_pasid_entry(s, ce, &pe, PCI_NO_PASID); >> } >> @@ -1581,12 +1579,11 @@ static int >vtd_dev_to_context_entry(IntelIOMMUState *s, uint8_t bus_num, >> } >> } else { >> /* >> - * Check if the programming of context-entry.rid2pasid >> - * and corresponding pasid setting is valid, and thus >> - * avoids to check pasid entry fetching result in future >> - * helper function calling. >> + * Check if the programming of pasid setting at RID_PASID(0) >of pasid 0? OK >> + * is valid, and thus avoids to check pasid entry fetching >> + * result in future helper function calling. >> */ >> - ret_fr = vtd_ce_rid2pasid_check(s, ce); >> + ret_fr = vtd_ce_rid_pasid_check(s, ce); >> if (ret_fr) { >> return ret_fr; >> } >> @@ -2097,7 +2094,7 @@ static bool >vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >> bool reads = true; >> bool writes = true; >> uint8_t access_flags, pgtt; >> - bool rid2pasid = (pasid == PCI_NO_PASID) && s->root_scalable; >> + bool rid_pasid = (pasid == PCI_NO_PASID) && s->root_scalable; >I am not keen of the rid_pasid name. It does not tell what is the >semantic of the variable. rid_pasid is an actual field in the CE. >does that check whether we face a request without pasid in scalable >mode. If so I would call that request_wo_pasid_sm or somethink alike OK >> VTDIOTLBEntry *iotlb_entry; >> uint64_t xlat, size; >> >> @@ -2111,8 +2108,8 @@ static bool >vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >> >> cc_entry = &vtd_as->context_cache_entry; >> >> - /* Try to fetch pte from IOTLB, we don't need RID2PASID logic */ >> - if (!rid2pasid) { >> + /* Try to fetch pte from IOTLB, we don't need RID_PASID(0) logic */ >It is unclear what the "RID_PASID(0) logic" is. All the more so we now >just have to set the pasid to PASID_0. You have keen insight, yes, this piece of code could be further simplified. We don't need to check rid2_pasid anymore, just index iotlb cache even for PASID_0. >> + if (!rid_pasid) { >> iotlb_entry = vtd_lookup_iotlb(s, source_id, pasid, addr); >> if (iotlb_entry) { >> trace_vtd_iotlb_page_hit(source_id, addr, >iotlb_entry->pte, >> @@ -2160,8 +2157,8 @@ static bool >vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >> cc_entry->context_cache_gen = s->context_cache_gen; >> } >> >> - if (rid2pasid) { >> - pasid = VTD_CE_GET_RID2PASID(&ce); >> + if (rid_pasid) { >> + pasid = RID_PASID; >> } >> >> /* >> @@ -2189,8 +2186,8 @@ static bool >vtd_do_iommu_translate(VTDAddressSpace *vtd_as, PCIBus *bus, >> return true; >> } >> >> - /* Try to fetch pte from IOTLB for RID2PASID slow path */ >> - if (rid2pasid) { >> + /* Try to fetch pte from IOTLB for RID_PASID(0) slow path */ >PASID_0? With simplification as above, this code is useless and will be deleted. >> + if (rid_pasid) { >> iotlb_entry = vtd_lookup_iotlb(s, source_id, pasid, addr); >> if (iotlb_entry) { >> trace_vtd_iotlb_page_hit(source_id, addr, >iotlb_entry->pte, >> @@ -2464,20 +2461,14 @@ static void >vtd_iotlb_page_invalidate_notify(IntelIOMMUState *s, >> ret = vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), >> vtd_as->devfn, &ce); >> if (!ret && domain_id == vtd_get_domain_id(s, &ce, >vtd_as->pasid)) { >> - uint32_t rid2pasid = PCI_NO_PASID; >> - >> - if (s->root_scalable) { >> - rid2pasid = VTD_CE_GET_RID2PASID(&ce); >> - } >> - >> /* >> * In legacy mode, vtd_as->pasid == pasid is always true. >> * In scalable mode, for vtd address space backing a PCI >> * device without pasid, needs to compare pasid with >> - * rid2pasid of this device. >> + * RID_PASID(0) of this device. >> */ >> if (!(vtd_as->pasid == pasid || >> - (vtd_as->pasid == PCI_NO_PASID && pasid == >rid2pasid))) { >> + (vtd_as->pasid == PCI_NO_PASID && pasid == >RID_PASID))) { >would strongly suggest using PASID_0 instead Sure. Thanks Zhenzhong >> continue; >> } >> >> @@ -2976,9 +2967,7 @@ static void >vtd_piotlb_pasid_invalidate(IntelIOMMUState *s, >> if (!vtd_dev_to_context_entry(s, pci_bus_num(vtd_as->bus), >> vtd_as->devfn, &ce) && >> domain_id == vtd_get_domain_id(s, &ce, vtd_as->pasid)) { >> - uint32_t rid2pasid = VTD_CE_GET_RID2PASID(&ce); >> - >> - if ((vtd_as->pasid != PCI_NO_PASID || pasid != rid2pasid) && >> + if ((vtd_as->pasid != PCI_NO_PASID || pasid != RID_PASID) >&& >> vtd_as->pasid != pasid) { >> continue; >> } >Thanks > >Eric
