Re: [PATCH v7 12/23] intel_iommu: Add some macros and inline functions

[Eric Auger <eric.auger@redhat.com>]

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 <eric.auger@redhat.com>
>> 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 <eric.auger@redhat.com>
>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
>>> ---
>>>  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