Re: [PATCH v6 02/11] x86/virt/tdx: Allocate page bitmap for Dynamic PAMT

[Sohil Mehta]

On 5/25/2026 7:35 PM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> 
> The TDX Physical Address Metadata Table (PAMT) holds data about the
> physical memory used by TDX, and must be allocated by the kernel during
> TDX module initialization.
> 
> The exact size of the required PAMT memory is determined by the TDX module
> and may vary between TDX module versions. Currently it is approximately
> 0.4% of the system memory. This is a significant commitment, especially if
> it is not known upfront whether the machine will run any TDX guests.
> 
> Each memory region that the TDX module might use needs three separate PAMT
> allocations. One for each supported page size (1GB, 2MB, 4KB). The
> TDX module supports a new feature designed to reduce PAMT overhead called
> Dynamic PAMT. At a high level, Dynamic PAMT still has the 1GB and 2MB
> levels allocated on TDX module initialization, but the 4KB level is
> allocated dynamically during runtime.

The last statement is slightly confusing to me. Is it trying to say that
the "dynamic" part is only applicable to 4KB allocations?


> 
> However, in the details, Dynamic PAMT still needs some smaller per 4KB
> page scoped data (currently it is 1 bit per page). The TDX module exposes
> the number of bits as a separate piece of metadata than the 4KB static
> allocation for regular PAMT. Although the size is enumerated differently,
> it is handed to the TDX module in the same way the 4KB page size PAMT
> allocation is for regular, non-dynamic PAMT.
> 
> Begin to implement Dynamic PAMT in the kernel by reading the bits-per-page
> needed for Dynamic PAMT. Calculate the size needed for the bitmap,
> and use it instead of the 4KB size determined for normal PAMT, in the case
> of Dynamic PAMT.
> 
> Unlike the existing metadata reading code, this code is not generated by a
> script. 


It might be useful to say that this file was auto-generated in the past
but going forward it is going to be manually updated.

> So adjust the comment to be more generic. Also, start to adopt a
> more normal kernel code style without the tenary statements and if

s/a more/
s/tenary/ternary


> conditionals assignments that the auto generated code has.
> 
> Assisted-by: Sashiko:claude-opus-4-6
> Reviewed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>

The review tags goes after the SOBs.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> ---

> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 503f9a3f46d61..82dc27aecf297 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -149,6 +149,11 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>  const char *tdx_dump_mce_info(struct mce *m);
>  const struct tdx_sys_info *tdx_get_sysinfo(void);
>  
> +static inline bool tdx_supports_dynamic_pamt(const struct tdx_sys_info *sysinfo)
> +{
> +	return false; /* To be enabled when kernel is ready */

I would avoid the tail comment even if it is temporary.

> +}
> +
>  int tdx_guest_keyid_alloc(void);
>  u32 tdx_get_nr_guest_keyids(void);
>  void tdx_guest_keyid_free(unsigned int keyid);



> @@ -33,6 +33,18 @@ static __init int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinf
>  	return ret;
>  }
>  
> +static __init int get_tdx_sys_info_tdmr_dpamt(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> +{
> +	int ret;
> +	u64 val;
> +
> +	ret = read_sys_metadata_field(0x9100000100000013, &val);

Should this be a #define now that the file is being manually updated? Or
is the plan to do it all together? A #define would make it easier to
read this patch.

> +	if (!ret)
> +		sysinfo_tdmr->pamt_page_bitmap_entry_bits = val;
> +
> +	return ret;
> +}
> +
>  static __init int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
>  {
>  	int ret = 0;
> @@ -116,5 +128,12 @@ static __init int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
>  	ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl);
>  	ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf);
>  
> +	/*
> +	 * Don't treat a module that doesn't support Dynamic PAMT
> +	 * as a failure. Only read the metadata optionally.
> +	 */
> +	if (!ret && tdx_supports_dynamic_pamt(sysinfo))
> +		ret = get_tdx_sys_info_tdmr_dpamt(&sysinfo->tdmr);

There is a need for the comment because it combines two checks:

1) Did any of the previous stages fail?
2) Does the TDX module support Dynamic PAMT?

Should these be separated for readability and to follow the typical
kernel style?

	if (ret)
		return ret;

	if (tdx_supports_dynamic_pamt(sysinfo))
		ret = get_tdx_sys_info_tdmr_dpamt(&sysinfo->tdmr);

	return ret;

I think you can avoid the comment altogether in that case.

> +
>  	return ret;
>  }