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)
> +#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.
>
> /* 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]
> +}
> +
> +/*
> + * 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?
> +}
> +
> +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
