Hi Eric, I showed the planed change inline, let me know if I misunderstood.
Thanks Zhenzhong >-----Original Message----- >From: Eric Auger <[email protected]> >Subject: Re: [PATCH v7 12/23] intel_iommu: Add some macros and inline >functions > >Hi Zhenzhong, > >On 10/24/25 10:43 AM, Zhenzhong Duan wrote: >> Add some macros and inline functions that will be used by following >> patch. >> >> This patch also make a cleanup to change macro >VTD_SM_PASID_ENTRY_DID >> and VTD_SM_PASID_ENTRY_FSPM to use extract64() just like what smmu >does, >> because they are either used in following patches or used indirectly by >> new introduced inline functions. But we doesn't aim to change the huge >> amount of bit mask style macro definitions in this patch, that should be >> in a separate patch. >> >> Suggested-by: Eric Auger <[email protected]> >> Signed-off-by: Zhenzhong Duan <[email protected]> >> Reviewed-by: Yi Liu <[email protected]> >> --- >> hw/i386/intel_iommu_internal.h | 8 +++++-- >> hw/i386/intel_iommu.c | 38 >+++++++++++++++++++++++++++------- >> 2 files changed, 37 insertions(+), 9 deletions(-) >> >> diff --git a/hw/i386/intel_iommu_internal.h >b/hw/i386/intel_iommu_internal.h >> index 09edba81e2..df80af839d 100644 >> --- a/hw/i386/intel_iommu_internal.h >> +++ b/hw/i386/intel_iommu_internal.h >> @@ -642,10 +642,14 @@ typedef struct VTDPASIDCacheInfo { >> #define VTD_SM_PASID_ENTRY_PT (4ULL << 6) >> >> #define VTD_SM_PASID_ENTRY_AW 7ULL /* Adjusted >guest-address-width */ >> -#define VTD_SM_PASID_ENTRY_DID(val) ((val) & >VTD_DOMAIN_ID_MASK) >> +#define VTD_SM_PASID_ENTRY_DID(x) extract64((x)->val[1], 0, 16) >> >> -#define VTD_SM_PASID_ENTRY_FSPM 3ULL >> #define VTD_SM_PASID_ENTRY_FSPTPTR (~0xfffULL) >> +#define VTD_SM_PASID_ENTRY_SRE_BIT(x) extract64((x)->val[2], 0, 1) >> +/* 00: 4-level paging, 01: 5-level paging, 10-11: Reserved */ >the above comment is not that useful along with bitmask definition. I >would rather move it along with VTD_PE_GET_FS_LEVEL(pe) OK, will move it like below: --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -56,6 +56,7 @@ /* pe operations */ #define VTD_PE_GET_TYPE(pe) ((pe)->val[0] & VTD_SM_PASID_ENTRY_PGTT) +/* 4: 4-level paging, 5: 5-level paging */ #define VTD_PE_GET_FS_LEVEL(pe) (VTD_SM_PASID_ENTRY_FSPM(pe) + 4) #define VTD_PE_GET_SS_LEVEL(pe) \ (2 + (((pe)->val[0] >> 2) & VTD_SM_PASID_ENTRY_AW)) >> +#define VTD_SM_PASID_ENTRY_FSPM(x) extract64((x)->val[2], 2, >2) >> +#define VTD_SM_PASID_ENTRY_WPE_BIT(x) extract64((x)->val[2], 4, >1) >> +#define VTD_SM_PASID_ENTRY_EAFE_BIT(x) extract64((x)->val[2], 7, 1) >I would get rid of _BIT suffix to make it consistent with other fields. OK >> >> /* First Stage Paging Structure */ >> /* Masks for First Stage Paging Entry */ >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c >> index 56abbb991d..871e6aad19 100644 >> --- a/hw/i386/intel_iommu.c >> +++ b/hw/i386/intel_iommu.c >> @@ -52,8 +52,7 @@ >> >> /* pe operations */ >> #define VTD_PE_GET_TYPE(pe) ((pe)->val[0] & >VTD_SM_PASID_ENTRY_PGTT) >> -#define VTD_PE_GET_FS_LEVEL(pe) \ >> - (4 + (((pe)->val[2] >> 2) & VTD_SM_PASID_ENTRY_FSPM)) >> +#define VTD_PE_GET_FS_LEVEL(pe) (VTD_SM_PASID_ENTRY_FSPM(pe) + >4) >> #define VTD_PE_GET_SS_LEVEL(pe) \ >> (2 + (((pe)->val[0] >> 2) & VTD_SM_PASID_ENTRY_AW)) >> >> @@ -837,6 +836,31 @@ static inline bool >vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) >> } >> } >> >> +static inline dma_addr_t vtd_pe_get_fspt_base(VTDPASIDEntry *pe) >> +{ >> + return pe->val[2] & VTD_SM_PASID_ENTRY_FSPTPTR; > >also introduce a define for FSPPTR using extract64((x)->val[2] Will change like below: --- a/hw/i386/intel_iommu_internal.h +++ b/hw/i386/intel_iommu_internal.h @@ -650,7 +650,7 @@ typedef struct VTDPIOTLBInvInfo { #define VTD_SM_PASID_ENTRY_AW 7ULL /* Adjusted guest-address-width */ #define VTD_SM_PASID_ENTRY_DID(x) extract64((x)->val[1], 0, 16) -#define VTD_SM_PASID_ENTRY_FSPTPTR (~0xfffULL) +#define VTD_SM_PASID_ENTRY_FSPTPFN extract64((x)->val[2], 12, 52) #define VTD_SM_PASID_ENTRY_SRE_BIT(x) extract64((x)->val[2], 0, 1) /* 00: 4-level paging, 01: 5-level paging, 10-11: Reserved */ #define VTD_SM_PASID_ENTRY_FSPM(x) extract64((x)->val[2], 2, 2) diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index a3a4a8a72b..f801458649 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -850,7 +851,7 @@ static inline bool vtd_pe_type_check(IntelIOMMUState *s, VTDPASIDEntry *pe) static inline dma_addr_t vtd_pe_get_fspt_base(VTDPASIDEntry *pe) { - return pe->val[2] & VTD_SM_PASID_ENTRY_FSPTPTR; + return VTD_SM_PASID_ENTRY_FSPTPFN << 12; } > >> +} >> + >> +/* >> + * First stage IOVA address width: 48 bits for 4-level paging(FSPM=00) >> + * 57 bits for 5-level >paging(FSPM=01) >> + */ >> +static inline uint32_t vtd_pe_get_fs_aw(VTDPASIDEntry *pe) >> +{ >> + return 48 + VTD_SM_PASID_ENTRY_FSPM(pe) * 9; >can you add a comment refering to the spec here? OK >> +} >> + >> +static inline bool vtd_pe_pgtt_is_pt(VTDPASIDEntry *pe) >> +{ >> + return (VTD_PE_GET_TYPE(pe) == VTD_SM_PASID_ENTRY_PT); >> +} >> + >> +/* check if pgtt is first stage translation */ >> +static inline bool vtd_pe_pgtt_is_fst(VTDPASIDEntry *pe) >> +{ >> + return (VTD_PE_GET_TYPE(pe) == VTD_SM_PASID_ENTRY_FST); >> +} >> + >> static inline bool vtd_pdire_present(VTDPASIDDirEntry *pdire) >> { >> return pdire->val & 1; >> @@ -1625,7 +1649,7 @@ static uint16_t >vtd_get_domain_id(IntelIOMMUState *s, >> >> if (s->root_scalable) { >> vtd_ce_get_pasid_entry(s, ce, &pe, pasid); >> - return VTD_SM_PASID_ENTRY_DID(pe.val[1]); >> + return VTD_SM_PASID_ENTRY_DID(&pe); >> } >> >> return VTD_CONTEXT_ENTRY_DID(ce->hi); >> @@ -1707,7 +1731,7 @@ static bool >vtd_dev_pt_enabled(IntelIOMMUState *s, VTDContextEntry *ce, >> */ >> return false; >> } >> - return (VTD_PE_GET_TYPE(&pe) == VTD_SM_PASID_ENTRY_PT); >> + return vtd_pe_pgtt_is_pt(&pe); >> } >> >> return (vtd_ce_get_type(ce) == VTD_CONTEXT_TT_PASS_THROUGH); >> @@ -3146,9 +3170,9 @@ static void >vtd_pasid_cache_sync_locked(gpointer key, gpointer value, >> /* Fall through */ >> case VTD_INV_DESC_PASIDC_G_DSI: >> if (pc_entry->valid) { >> - did = >VTD_SM_PASID_ENTRY_DID(pc_entry->pasid_entry.val[1]); >> + did = VTD_SM_PASID_ENTRY_DID(&pc_entry->pasid_entry); >> } else { >> - did = VTD_SM_PASID_ENTRY_DID(pe.val[1]); >> + did = VTD_SM_PASID_ENTRY_DID(&pe); >> } >> if (pc_info->did != did) { >> return; >> @@ -5267,7 +5291,7 @@ static int >vtd_pri_perform_implicit_invalidation(VTDAddressSpace *vtd_as, >> return -EINVAL; >> } >> pgtt = VTD_PE_GET_TYPE(&pe); >> - domain_id = VTD_SM_PASID_ENTRY_DID(pe.val[1]); >> + domain_id = VTD_SM_PASID_ENTRY_DID(&pe); >> ret = 0; >> switch (pgtt) { >> case VTD_SM_PASID_ENTRY_FST: >Otherwise looks good to me > >Thanks > >Eric
