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
