Re: [PATCH v1 2/8] x86/virt/tdx: Name the TDX module global metadata field IDs

On 8/4/26 04:29, Chao Gao wrote:
> From: Dave Hansen <[email protected]>
> 
> tdx_global_metadata.c currently identifies each field by a bare 64-bit hex
> literal, extracted from the JSON file by an out-of-tree script:
> 
> 	if (!ret && !(ret = read_sys_metadata_field(0x9100000100000008, &val)))
> 		sysinfo_tdmr->max_tdmrs = val;
> 
> That is unreviewable on its own. Verifying any one line requires the
> reviewer to cross-reference the JSON file.

I'm looking at "global_metadata.pdf" from this[1]. I see a line with
"MAX_TDMRS" and "0x9100000100000008". That matches the lines above. That
seems *FAR* from unreviewable. There's also no JSON in sight.

What am I missing?

Listen, I don't like how this turned out. I'm asking for it to be
changed. But let's not justify it with things that just aren't true.

> The hex literals in tdx_global_metadata.c are left alone because that file
> will be replaced by a table-driven reader in the following patches.

This part of the changelog is good.

> No functional change intended.

This is a bit much. :)

> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index bdfd0e1e337a..5f567cb6c07a 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -58,6 +58,57 @@
>   */
>  #define TDX_VERSION_SHIFT		16
>  
> +/*
> + * Global Scope Metadata field IDs.
> + *
> + * See "Global-Scope (TDX Module) Metadata" in the Intel TDX Module ABI
> + * spec.
> + *
> + * A field ID is a 64-bit value that encodes the metadata "Class"
> + * (which Linux mirrors in 'struct tdx_sys_info' sub-structures),
> + * the element size, and a per-class field index. Each ID below
> + * is paired with the C member that holds its value.
> + */

This is a bit verbose for my taste.

> +/* Class "TDX Module Version" */
> +#define MD_FIELD_ID_MINOR_VERSION		0x0800000100000003ULL
> +#define MD_FIELD_ID_MAJOR_VERSION		0x0800000100000004ULL
> +#define MD_FIELD_ID_UPDATE_VERSION		0x0800000100000005ULL
> +
> +/* Class "TDX Features" */
> +#define MD_FIELD_ID_TDX_FEATURES0		0x0A00000300000008ULL
> +
> +/* Class "TDMR Info" */
> +#define MD_FIELD_ID_MAX_TDMRS			0x9100000100000008ULL
> +#define MD_FIELD_ID_MAX_RESERVED_PER_TDMR	0x9100000100000009ULL
> +#define MD_FIELD_ID_PAMT_4K_ENTRY_SIZE		0x9100000100000010ULL
> +#define MD_FIELD_ID_PAMT_2M_ENTRY_SIZE		0x9100000100000011ULL
> +#define MD_FIELD_ID_PAMT_1G_ENTRY_SIZE		0x9100000100000012ULL
> +
> +/* Class "TD Control Structures" */
> +#define MD_FIELD_ID_TDR_BASE_SIZE		0x9800000100000000ULL
> +#define MD_FIELD_ID_TDCS_BASE_SIZE		0x9800000100000100ULL
> +#define MD_FIELD_ID_TDVPS_BASE_SIZE		0x9800000100000200ULL
> +
> +/* Class "TD Configuration" */
> +#define MD_FIELD_ID_ATTRIBUTES_FIXED0		0x1900000300000000ULL
> +#define MD_FIELD_ID_ATTRIBUTES_FIXED1		0x1900000300000001ULL
> +#define MD_FIELD_ID_XFAM_FIXED0			0x1900000300000002ULL
> +#define MD_FIELD_ID_XFAM_FIXED1			0x1900000300000003ULL
> +#define MD_FIELD_ID_NUM_CPUID_CONFIG		0x9900000100000004ULL
> +#define MD_FIELD_ID_MAX_VCPUS_PER_TD		0x9900000100000008ULL
> +
> +/* Class "TDX Module Handoff" */
> +#define MD_FIELD_ID_MODULE_HV			0x8900000100000000ULL

These are fine on their own.

> +/*
> + * Base IDs for the configurable-CPUID arrays.  The field ID of leaf
> + * index @i is BASE + i; for the values array, sub-entry @j of index
> + * @i is BASE + i*2 + j.
> + */
> +#define MD_FIELD_ID_CPUID_CONFIG_LEAVES		0x9900000300000400ULL
> +#define MD_FIELD_ID_CPUID_CONFIG_VALUES		0x9900000300000500ULL

This is complete jibberish without more context.

1,
https://www.intel.com/content/www/us/en/content-details/865803/abi-definitions-for-intel-tdx.html