Hi Eric, All clear, thanks. btw: I'm taking vacation this week, I'll reply your comments for other patches a little later.
BRs, 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 11/3/25 4:44 AM, Duan, Zhenzhong wrote: >> 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 */ >I think I would rather put quote the spec >paging mode for first-stage translation. • 00: 4-level paging • 01: >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) > >VTD_SM_PASID_ENTRY_FSPTPFN(x)? > >> #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; >ditto >> } >> >> >>>> +} >>>> + >>>> +/* >>>> + * 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
