[v19,034/130] KVM: TDX: Get system-wide info about TDX module on initialization

Commit Message

Isaku Yamahata Feb. 26, 2024, 8:25 a.m. UTC

From: Isaku Yamahata <[e-mail]>

TDX KVM needs system-wide information about the TDX module, store it in
struct tdx_info.

Signed-off-by: Isaku Yamahata <[e-mail]>
---
v19:
- Added features0
- Use tdx_sys_metadata_read()
- Fix error recovery path by Yuan

Change v18:
- Newly Added

Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/include/uapi/asm/kvm.h | 11 +++++
 arch/x86/kvm/vmx/main.c         |  9 +++-
 arch/x86/kvm/vmx/tdx.c          | 80 ++++++++++++++++++++++++++++++++-
 arch/x86/kvm/vmx/x86_ops.h      |  2 +
 4 files changed, 100 insertions(+), 2 deletions(-)

Comments

Binbin Wu March 14, 2024, 3:06 p.m. UTC | #1

On 2/26/2024 4:25 PM, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
>
> TDX KVM needs system-wide information about the TDX module, store it in
> struct tdx_info.

Nit: Maybe you can add some description about hardware_unsetup()?

Reviewed-by: Binbin Wu <[e-mail]>

> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
> v19:
> - Added features0
> - Use tdx_sys_metadata_read()
> - Fix error recovery path by Yuan
>
> Change v18:
> - Newly Added
>
> Signed-off-by: Isaku Yamahata <[e-mail]>
> ---
>   arch/x86/include/uapi/asm/kvm.h | 11 +++++
>   arch/x86/kvm/vmx/main.c         |  9 +++-
>   arch/x86/kvm/vmx/tdx.c          | 80 ++++++++++++++++++++++++++++++++-
>   arch/x86/kvm/vmx/x86_ops.h      |  2 +
>   4 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> index aa7a56a47564..45b2c2304491 100644
> --- a/arch/x86/include/uapi/asm/kvm.h
> +++ b/arch/x86/include/uapi/asm/kvm.h
> @@ -567,4 +567,15 @@ struct kvm_pmu_event_filter {
>   #define KVM_X86_TDX_VM		2
>   #define KVM_X86_SNP_VM		3
>   
> +#define KVM_TDX_CPUID_NO_SUBLEAF	((__u32)-1)
> +
> +struct kvm_tdx_cpuid_config {
> +	__u32 leaf;
> +	__u32 sub_leaf;
> +	__u32 eax;
> +	__u32 ebx;
> +	__u32 ecx;
> +	__u32 edx;
> +};
> +
>   #endif /* _ASM_X86_KVM_H */
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index fa19682b366c..a948a6959ac7 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -32,6 +32,13 @@ static __init int vt_hardware_setup(void)
>   	return 0;
>   }
>   
> +static void vt_hardware_unsetup(void)
> +{
> +	if (enable_tdx)
> +		tdx_hardware_unsetup();
> +	vmx_hardware_unsetup();
> +}
> +
>   static int vt_vm_init(struct kvm *kvm)
>   {
>   	if (is_td(kvm))
> @@ -54,7 +61,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>   
>   	.check_processor_compatibility = vmx_check_processor_compat,
>   
> -	.hardware_unsetup = vmx_hardware_unsetup,
> +	.hardware_unsetup = vt_hardware_unsetup,
>   
>   	/* TDX cpu enablement is done by tdx_hardware_setup(). */
>   	.hardware_enable = vmx_hardware_enable,
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index dce21f675155..5edfb99abb89 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -40,6 +40,21 @@ static void __used tdx_guest_keyid_free(int keyid)
>   	ida_free(&tdx_guest_keyid_pool, keyid);
>   }
>   
> +struct tdx_info {
> +	u64 features0;
> +	u64 attributes_fixed0;
> +	u64 attributes_fixed1;
> +	u64 xfam_fixed0;
> +	u64 xfam_fixed1;
> +
> +	u16 num_cpuid_config;
> +	/* This must the last member. */
> +	DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
> +};
> +
> +/* Info about the TDX module. */
> +static struct tdx_info *tdx_info;
> +
>   #define TDX_MD_MAP(_fid, _ptr)			\
>   	{ .fid = MD_FIELD_ID_##_fid,		\
>   	  .ptr = (_ptr), }
> @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
>   	}
>   }
>   
> -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>   {
>   	struct tdx_md_map *m;
>   	int ret, i;
> @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>   	return 0;
>   }
>   
> +#define TDX_INFO_MAP(_field_id, _member)			\
> +	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
> +
>   static int __init tdx_module_setup(void)
>   {
> +	u16 num_cpuid_config;
>   	int ret;
> +	u32 i;
> +
> +	struct tdx_md_map mds[] = {
> +		TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
> +	};
> +
> +	struct tdx_metadata_field_mapping fields[] = {
> +		TDX_INFO_MAP(FEATURES0, features0),
> +		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> +		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> +		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> +		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> +	};
>   
>   	ret = tdx_enable();
>   	if (ret) {
> @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
>   		return ret;
>   	}
>   
> +	ret = tdx_md_read(mds, ARRAY_SIZE(mds));
> +	if (ret)
> +		return ret;
> +
> +	tdx_info = kzalloc(sizeof(*tdx_info) +
> +			   sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
> +			   GFP_KERNEL);
> +	if (!tdx_info)
> +		return -ENOMEM;
> +	tdx_info->num_cpuid_config = num_cpuid_config;
> +
> +	ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
> +	if (ret)
> +		goto error_out;
> +
> +	for (i = 0; i < num_cpuid_config; i++) {
> +		struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> +		u64 leaf, eax_ebx, ecx_edx;
> +		struct tdx_md_map cpuids[] = {
> +			TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
> +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
> +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
> +		};
> +
> +		ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
> +		if (ret)
> +			goto error_out;
> +
> +		c->leaf = (u32)leaf;
> +		c->sub_leaf = leaf >> 32;
> +		c->eax = (u32)eax_ebx;
> +		c->ebx = eax_ebx >> 32;
> +		c->ecx = (u32)ecx_edx;
> +		c->edx = ecx_edx >> 32;
> +	}
> +
>   	return 0;
> +
> +error_out:
> +	/* kfree() accepts NULL. */
> +	kfree(tdx_info);
> +	return ret;
>   }
>   
>   bool tdx_is_vm_type_supported(unsigned long type)
> @@ -162,3 +235,8 @@ int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
>   out:
>   	return r;
>   }
> +
> +void tdx_hardware_unsetup(void)
> +{
> +	kfree(tdx_info);
> +}
> diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
> index f4da88a228d0..e8cb4ae81cf1 100644
> --- a/arch/x86/kvm/vmx/x86_ops.h
> +++ b/arch/x86/kvm/vmx/x86_ops.h
> @@ -136,9 +136,11 @@ void vmx_setup_mce(struct kvm_vcpu *vcpu);
>   
>   #ifdef CONFIG_INTEL_TDX_HOST
>   int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops);
> +void tdx_hardware_unsetup(void);
>   bool tdx_is_vm_type_supported(unsigned long type);
>   #else
>   static inline int tdx_hardware_setup(struct kvm_x86_ops *x86_ops) { return -EOPNOTSUPP; }
> +static inline void tdx_hardware_unsetup(void) {}
>   static inline bool tdx_is_vm_type_supported(unsigned long type) { return false; }
>   #endif
>

Huang, Kai March 14, 2024, 11:09 p.m. UTC | #2

> +struct tdx_info {
> +	u64 features0;
> +	u64 attributes_fixed0;
> +	u64 attributes_fixed1;
> +	u64 xfam_fixed0;
> +	u64 xfam_fixed1;
> +
> +	u16 num_cpuid_config;
> +	/* This must the last member. */
> +	DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
> +};
> +
> +/* Info about the TDX module. */
> +static struct tdx_info *tdx_info;
> +
>   #define TDX_MD_MAP(_fid, _ptr)			\
>   	{ .fid = MD_FIELD_ID_##_fid,		\
>   	  .ptr = (_ptr), }
> @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
>   	}
>   }
>   
> -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>   {
>   	struct tdx_md_map *m;
>   	int ret, i;
> @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>   	return 0;
>   }
>   
> +#define TDX_INFO_MAP(_field_id, _member)			\
> +	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
> +
>   static int __init tdx_module_setup(void)
>   {
> +	u16 num_cpuid_config;
>   	int ret;
> +	u32 i;
> +
> +	struct tdx_md_map mds[] = {
> +		TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
> +	};
> +
> +	struct tdx_metadata_field_mapping fields[] = {
> +		TDX_INFO_MAP(FEATURES0, features0),
> +		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> +		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> +		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> +		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> +	};
>   
>   	ret = tdx_enable();
>   	if (ret) {
> @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
>   		return ret;
>   	}
>   
> +	ret = tdx_md_read(mds, ARRAY_SIZE(mds));
> +	if (ret)
> +		return ret;
> +
> +	tdx_info = kzalloc(sizeof(*tdx_info) +
> +			   sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
> +			   GFP_KERNEL);
> +	if (!tdx_info)
> +		return -ENOMEM;
> +	tdx_info->num_cpuid_config = num_cpuid_config;
> +
> +	ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
> +	if (ret)
> +		goto error_out;
> +
> +	for (i = 0; i < num_cpuid_config; i++) {
> +		struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> +		u64 leaf, eax_ebx, ecx_edx;
> +		struct tdx_md_map cpuids[] = {
> +			TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
> +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
> +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
> +		};
> +
> +		ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
> +		if (ret)
> +			goto error_out;
> +
> +		c->leaf = (u32)leaf;
> +		c->sub_leaf = leaf >> 32;
> +		c->eax = (u32)eax_ebx;
> +		c->ebx = eax_ebx >> 32;
> +		c->ecx = (u32)ecx_edx;
> +		c->edx = ecx_edx >> 32;

OK I can see why you don't want to use ...

	struct tdx_metadata_field_mapping fields[] = {
		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
	};

... to read num_cpuid_config first, because the memory to hold @tdx_info 
hasn't been allocated, because its size depends on the num_cpuid_config.

And I confess it's because the tdx_sys_metadata_field_read() that got 
exposed in patch ("x86/virt/tdx: Export global metadata read 
infrastructure") only returns 'u64' for all metadata field, and you 
didn't want to use something like this:

	u64 num_cpuid_config;
	
	tdx_sys_metadata_field_read(..., &num_cpuid_config);

	...

	tdx_info->num_cpuid_config = num_cpuid_config;

Or you can explicitly cast:

	tdx_info->num_cpuid_config = (u16)num_cpuid_config;

(I know people may don't like the assigning 'u64' to 'u16', but it seems 
nothing wrong to me, because the way done in (1) below effectively has 
the same result comparing to type case).

But there are other (better) ways to do:

1) you can introduce a helper as suggested by Xiaoyao in [*]:


	int tdx_sys_metadata_read_single(u64 field_id,
					int bytes,  void *buf)
	{
		return stbuf_read_sys_metadata_field(field_id, 0,
						bytes, buf);
	}

And do:

	tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
		sizeof(num_cpuid_config), &num_cpuid_config);

That's _much_ cleaner than the 'struct tdx_md_map', which only confuses 
people.

But I don't think we need to do this as mentioned above -- we just do 
type cast.

2) You can just preallocate enough memory.  It cannot be larger than 
1024B, right?  You can even just allocate one page.  It's just 4K, no 
one cares.

Then you can do:

	struct tdx_metadata_field_mapping tdx_info_fields = {
		...
		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
	};

	tdx_sys_metadata_read(tdx_info_fields,
			ARRAY_SIZE(tdx_info_fields, tdx_info);

And then you read the CPUID_CONFIG array one by one using the same 
'struct tdx_metadata_field_mapping' and tdx_sys_metadata_read():


	for (i = 0; i < tdx_info->num_cpuid_config; i++) {
		struct tdx_metadata_field_mapping cpuid_fields = {
			TDX_CPUID_CONFIG_MAP(CPUID_CONFIG_LEAVES + i,
						leaf),
			...
		};
		struct kvm_tdx_cpuid_config *c =
				&tdx_info->cpuid_configs[i];

		tdx_sys_metadata_read(cpuid_fields,
				ARRAY_SIZE(cpuid_fields), c);

		....
	}

So stopping having the duplicated 'struct tdx_md_map' and related staff, 
as they are absolutely unnecessary and only confuses people.

Btw, I am hesitated to do the change suggested by Xiaoyao in [*], as to 
me there's nothing wrong to do the type cast.  I'll response in that thread.

[*] 
https://lore.kernel.org/lkml/[e-mail]/T/#m2512e378c83bc44d3ca653f96f25c3fc85eb0e8a

Xiaoyao Li March 15, 2024, 2:18 a.m. UTC | #3

On 3/15/2024 7:09 AM, Huang, Kai wrote:
> 
>> +struct tdx_info {
>> +    u64 features0;
>> +    u64 attributes_fixed0;
>> +    u64 attributes_fixed1;
>> +    u64 xfam_fixed0;
>> +    u64 xfam_fixed1;
>> +
>> +    u16 num_cpuid_config;
>> +    /* This must the last member. */
>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>> +};
>> +
>> +/* Info about the TDX module. */
>> +static struct tdx_info *tdx_info;
>> +
>>   #define TDX_MD_MAP(_fid, _ptr)            \
>>       { .fid = MD_FIELD_ID_##_fid,        \
>>         .ptr = (_ptr), }
>> @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
>>       }
>>   }
>> -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>> +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>>   {
>>       struct tdx_md_map *m;
>>       int ret, i;
>> @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map 
>> *maps, int nr_maps)
>>       return 0;
>>   }
>> +#define TDX_INFO_MAP(_field_id, _member)            \
>> +    TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
>> +
>>   static int __init tdx_module_setup(void)
>>   {
>> +    u16 num_cpuid_config;
>>       int ret;
>> +    u32 i;
>> +
>> +    struct tdx_md_map mds[] = {
>> +        TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
>> +    };
>> +
>> +    struct tdx_metadata_field_mapping fields[] = {
>> +        TDX_INFO_MAP(FEATURES0, features0),
>> +        TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
>> +        TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
>> +        TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
>> +        TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
>> +    };
>>       ret = tdx_enable();
>>       if (ret) {
>> @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
>>           return ret;
>>       }
>> +    ret = tdx_md_read(mds, ARRAY_SIZE(mds));
>> +    if (ret)
>> +        return ret;
>> +
>> +    tdx_info = kzalloc(sizeof(*tdx_info) +
>> +               sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
>> +               GFP_KERNEL);
>> +    if (!tdx_info)
>> +        return -ENOMEM;
>> +    tdx_info->num_cpuid_config = num_cpuid_config;
>> +
>> +    ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
>> +    if (ret)
>> +        goto error_out;
>> +
>> +    for (i = 0; i < num_cpuid_config; i++) {
>> +        struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
>> +        u64 leaf, eax_ebx, ecx_edx;
>> +        struct tdx_md_map cpuids[] = {
>> +            TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
>> +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
>> +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
>> +        };
>> +
>> +        ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
>> +        if (ret)
>> +            goto error_out;
>> +
>> +        c->leaf = (u32)leaf;
>> +        c->sub_leaf = leaf >> 32;
>> +        c->eax = (u32)eax_ebx;
>> +        c->ebx = eax_ebx >> 32;
>> +        c->ecx = (u32)ecx_edx;
>> +        c->edx = ecx_edx >> 32;
> 
> OK I can see why you don't want to use ...
> 
>      struct tdx_metadata_field_mapping fields[] = {
>          TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
>      };
> 
> ... to read num_cpuid_config first, because the memory to hold @tdx_info 
> hasn't been allocated, because its size depends on the num_cpuid_config.
> 
> And I confess it's because the tdx_sys_metadata_field_read() that got 
> exposed in patch ("x86/virt/tdx: Export global metadata read 
> infrastructure") only returns 'u64' for all metadata field, and you 
> didn't want to use something like this:
> 
>      u64 num_cpuid_config;
> 
>      tdx_sys_metadata_field_read(..., &num_cpuid_config);
> 
>      ...
> 
>      tdx_info->num_cpuid_config = num_cpuid_config;
> 
> Or you can explicitly cast:
> 
>      tdx_info->num_cpuid_config = (u16)num_cpuid_config;
> 
> (I know people may don't like the assigning 'u64' to 'u16', but it seems 
> nothing wrong to me, because the way done in (1) below effectively has 
> the same result comparing to type case).
> 
> But there are other (better) ways to do:
> 
> 1) you can introduce a helper as suggested by Xiaoyao in [*]:
> 
> 
>      int tdx_sys_metadata_read_single(u64 field_id,
>                      int bytes,  void *buf)
>      {
>          return stbuf_read_sys_metadata_field(field_id, 0,
>                          bytes, buf);
>      }
> 
> And do:
> 
>      tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
>          sizeof(num_cpuid_config), &num_cpuid_config);
> 
> That's _much_ cleaner than the 'struct tdx_md_map', which only confuses 
> people.
> 
> But I don't think we need to do this as mentioned above -- we just do 
> type cast.

type cast needs another tmp variable to hold the output of u64.

The reason I want to introduce tdx_sys_metadata_read_single() is to 
provide a simple and unified interface for other codes to read one 
metadata field, instead of letting the caller to use temporary u64 
variable and handle the cast or memcpy itself.

> [*] 
> https://lore.kernel.org/lkml/[e-mail]/T/#m2512e378c83bc44d3ca653f96f25c3fc85eb0e8a
> 
> 
> 
>

Huang, Kai March 15, 2024, 4:57 a.m. UTC | #4

On Fri, 2024-03-15 at 10:18 +0800, Li, Xiaoyao wrote:
> On 3/15/2024 7:09 AM, Huang, Kai wrote:
> > 
> > > +struct tdx_info {
> > > +    u64 features0;
> > > +    u64 attributes_fixed0;
> > > +    u64 attributes_fixed1;
> > > +    u64 xfam_fixed0;
> > > +    u64 xfam_fixed1;
> > > +
> > > +    u16 num_cpuid_config;
> > > +    /* This must the last member. */
> > > +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
> > > +};
> > > +
> > > +/* Info about the TDX module. */
> > > +static struct tdx_info *tdx_info;
> > > +
> > >   #define TDX_MD_MAP(_fid, _ptr)            \
> > >       { .fid = MD_FIELD_ID_##_fid,        \
> > >         .ptr = (_ptr), }
> > > @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
> > >       }
> > >   }
> > > -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> > > +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> > >   {
> > >       struct tdx_md_map *m;
> > >       int ret, i;
> > > @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map 
> > > *maps, int nr_maps)
> > >       return 0;
> > >   }
> > > +#define TDX_INFO_MAP(_field_id, _member)            \
> > > +    TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
> > > +
> > >   static int __init tdx_module_setup(void)
> > >   {
> > > +    u16 num_cpuid_config;
> > >       int ret;
> > > +    u32 i;
> > > +
> > > +    struct tdx_md_map mds[] = {
> > > +        TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
> > > +    };
> > > +
> > > +    struct tdx_metadata_field_mapping fields[] = {
> > > +        TDX_INFO_MAP(FEATURES0, features0),
> > > +        TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> > > +        TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> > > +        TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> > > +        TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> > > +    };
> > >       ret = tdx_enable();
> > >       if (ret) {
> > > @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
> > >           return ret;
> > >       }
> > > +    ret = tdx_md_read(mds, ARRAY_SIZE(mds));
> > > +    if (ret)
> > > +        return ret;
> > > +
> > > +    tdx_info = kzalloc(sizeof(*tdx_info) +
> > > +               sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
> > > +               GFP_KERNEL);
> > > +    if (!tdx_info)
> > > +        return -ENOMEM;
> > > +    tdx_info->num_cpuid_config = num_cpuid_config;
> > > +
> > > +    ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
> > > +    if (ret)
> > > +        goto error_out;
> > > +
> > > +    for (i = 0; i < num_cpuid_config; i++) {
> > > +        struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> > > +        u64 leaf, eax_ebx, ecx_edx;
> > > +        struct tdx_md_map cpuids[] = {
> > > +            TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
> > > +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
> > > +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
> > > +        };
> > > +
> > > +        ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
> > > +        if (ret)
> > > +            goto error_out;
> > > +
> > > +        c->leaf = (u32)leaf;
> > > +        c->sub_leaf = leaf >> 32;
> > > +        c->eax = (u32)eax_ebx;
> > > +        c->ebx = eax_ebx >> 32;
> > > +        c->ecx = (u32)ecx_edx;
> > > +        c->edx = ecx_edx >> 32;
> > 
> > OK I can see why you don't want to use ...
> > 
> >      struct tdx_metadata_field_mapping fields[] = {
> >          TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
> >      };
> > 
> > ... to read num_cpuid_config first, because the memory to hold @tdx_info 
> > hasn't been allocated, because its size depends on the num_cpuid_config.
> > 
> > And I confess it's because the tdx_sys_metadata_field_read() that got 
> > exposed in patch ("x86/virt/tdx: Export global metadata read 
> > infrastructure") only returns 'u64' for all metadata field, and you 
> > didn't want to use something like this:
> > 
> >      u64 num_cpuid_config;
> > 
> >      tdx_sys_metadata_field_read(..., &num_cpuid_config);
> > 
> >      ...
> > 
> >      tdx_info->num_cpuid_config = num_cpuid_config;
> > 
> > Or you can explicitly cast:
> > 
> >      tdx_info->num_cpuid_config = (u16)num_cpuid_config;
> > 
> > (I know people may don't like the assigning 'u64' to 'u16', but it seems 
> > nothing wrong to me, because the way done in (1) below effectively has 
> > the same result comparing to type case).
> > 
> > But there are other (better) ways to do:
> > 
> > 1) you can introduce a helper as suggested by Xiaoyao in [*]:
> > 
> > 
> >      int tdx_sys_metadata_read_single(u64 field_id,
> >                      int bytes,  void *buf)
> >      {
> >          return stbuf_read_sys_metadata_field(field_id, 0,
> >                          bytes, buf);
> >      }
> > 
> > And do:
> > 
> >      tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
> >          sizeof(num_cpuid_config), &num_cpuid_config);
> > 
> > That's _much_ cleaner than the 'struct tdx_md_map', which only confuses 
> > people.
> > 
> > But I don't think we need to do this as mentioned above -- we just do 
> > type cast.
> 
> type cast needs another tmp variable to hold the output of u64.
> 
> The reason I want to introduce tdx_sys_metadata_read_single() is to 
> provide a simple and unified interface for other codes to read one 
> metadata field, instead of letting the caller to use temporary u64 
> variable and handle the cast or memcpy itself.
> 

You can always use u64 to hold u16 metadata field AFAICT, so it doesn't have to
be temporary.

Here is what Isaku can do using the current API:

	u64 num_cpuid_config;
	

	...

	tdx_sys_metadata_field_read(NUM_CPUID_CONFIG, &num_cpuid_config);

	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);

	tdx_info->num_cpuid_config = num_cpuid_config;

	...

(you can do explicit (u16)num_cpuid_config type cast above if you want.)

With your suggestion, here is what Isaku can do:

	u16 num_cpuid_config;

	...

	tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
sizeof(num_cpuid_config),
				&num_cpuid_config);

	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);

	tdx_info->num_cpuid_config = num_cpuid_config;

	...

I don't see big difference?

One example that the current tdx_sys_metadata_field_read() doesn't quite fit is
you have something like this:

	struct {
		u16 whatever;
		...
	} st;

	tdx_sys_metadata_field_read(FIELD_ID_WHATEVER, &st.whatever);

But for this use case you are not supposed to use tdx_sys_metadata_field_read(),
but use tdx_sys_metadata_read() which has a mapping provided anyway.

So, while I don't quite object your proposal, I don't see it being quite
necessary.

I'll let other people to have a say.

Xiaoyao Li March 15, 2024, 5:11 a.m. UTC | #5

On 3/15/2024 12:57 PM, Huang, Kai wrote:
> On Fri, 2024-03-15 at 10:18 +0800, Li, Xiaoyao wrote:
>> On 3/15/2024 7:09 AM, Huang, Kai wrote:
>>>
>>>> +struct tdx_info {
>>>> +    u64 features0;
>>>> +    u64 attributes_fixed0;
>>>> +    u64 attributes_fixed1;
>>>> +    u64 xfam_fixed0;
>>>> +    u64 xfam_fixed1;
>>>> +
>>>> +    u16 num_cpuid_config;
>>>> +    /* This must the last member. */
>>>> +    DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
>>>> +};
>>>> +
>>>> +/* Info about the TDX module. */
>>>> +static struct tdx_info *tdx_info;
>>>> +
>>>>    #define TDX_MD_MAP(_fid, _ptr)            \
>>>>        { .fid = MD_FIELD_ID_##_fid,        \
>>>>          .ptr = (_ptr), }
>>>> @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
>>>>        }
>>>>    }
>>>> -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>>>> +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
>>>>    {
>>>>        struct tdx_md_map *m;
>>>>        int ret, i;
>>>> @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map
>>>> *maps, int nr_maps)
>>>>        return 0;
>>>>    }
>>>> +#define TDX_INFO_MAP(_field_id, _member)            \
>>>> +    TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
>>>> +
>>>>    static int __init tdx_module_setup(void)
>>>>    {
>>>> +    u16 num_cpuid_config;
>>>>        int ret;
>>>> +    u32 i;
>>>> +
>>>> +    struct tdx_md_map mds[] = {
>>>> +        TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
>>>> +    };
>>>> +
>>>> +    struct tdx_metadata_field_mapping fields[] = {
>>>> +        TDX_INFO_MAP(FEATURES0, features0),
>>>> +        TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
>>>> +        TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
>>>> +        TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
>>>> +        TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
>>>> +    };
>>>>        ret = tdx_enable();
>>>>        if (ret) {
>>>> @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
>>>>            return ret;
>>>>        }
>>>> +    ret = tdx_md_read(mds, ARRAY_SIZE(mds));
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    tdx_info = kzalloc(sizeof(*tdx_info) +
>>>> +               sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
>>>> +               GFP_KERNEL);
>>>> +    if (!tdx_info)
>>>> +        return -ENOMEM;
>>>> +    tdx_info->num_cpuid_config = num_cpuid_config;
>>>> +
>>>> +    ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
>>>> +    if (ret)
>>>> +        goto error_out;
>>>> +
>>>> +    for (i = 0; i < num_cpuid_config; i++) {
>>>> +        struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
>>>> +        u64 leaf, eax_ebx, ecx_edx;
>>>> +        struct tdx_md_map cpuids[] = {
>>>> +            TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
>>>> +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
>>>> +            TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
>>>> +        };
>>>> +
>>>> +        ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
>>>> +        if (ret)
>>>> +            goto error_out;
>>>> +
>>>> +        c->leaf = (u32)leaf;
>>>> +        c->sub_leaf = leaf >> 32;
>>>> +        c->eax = (u32)eax_ebx;
>>>> +        c->ebx = eax_ebx >> 32;
>>>> +        c->ecx = (u32)ecx_edx;
>>>> +        c->edx = ecx_edx >> 32;
>>>
>>> OK I can see why you don't want to use ...
>>>
>>>       struct tdx_metadata_field_mapping fields[] = {
>>>           TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
>>>       };
>>>
>>> ... to read num_cpuid_config first, because the memory to hold @tdx_info
>>> hasn't been allocated, because its size depends on the num_cpuid_config.
>>>
>>> And I confess it's because the tdx_sys_metadata_field_read() that got
>>> exposed in patch ("x86/virt/tdx: Export global metadata read
>>> infrastructure") only returns 'u64' for all metadata field, and you
>>> didn't want to use something like this:
>>>
>>>       u64 num_cpuid_config;
>>>
>>>       tdx_sys_metadata_field_read(..., &num_cpuid_config);
>>>
>>>       ...
>>>
>>>       tdx_info->num_cpuid_config = num_cpuid_config;
>>>
>>> Or you can explicitly cast:
>>>
>>>       tdx_info->num_cpuid_config = (u16)num_cpuid_config;
>>>
>>> (I know people may don't like the assigning 'u64' to 'u16', but it seems
>>> nothing wrong to me, because the way done in (1) below effectively has
>>> the same result comparing to type case).
>>>
>>> But there are other (better) ways to do:
>>>
>>> 1) you can introduce a helper as suggested by Xiaoyao in [*]:
>>>
>>>
>>>       int tdx_sys_metadata_read_single(u64 field_id,
>>>                       int bytes,  void *buf)
>>>       {
>>>           return stbuf_read_sys_metadata_field(field_id, 0,
>>>                           bytes, buf);
>>>       }
>>>
>>> And do:
>>>
>>>       tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
>>>           sizeof(num_cpuid_config), &num_cpuid_config);
>>>
>>> That's _much_ cleaner than the 'struct tdx_md_map', which only confuses
>>> people.
>>>
>>> But I don't think we need to do this as mentioned above -- we just do
>>> type cast.
>>
>> type cast needs another tmp variable to hold the output of u64.
>>
>> The reason I want to introduce tdx_sys_metadata_read_single() is to
>> provide a simple and unified interface for other codes to read one
>> metadata field, instead of letting the caller to use temporary u64
>> variable and handle the cast or memcpy itself.
>>
> 
> You can always use u64 to hold u16 metadata field AFAICT, so it doesn't have to
> be temporary.
> 
> Here is what Isaku can do using the current API:
> 
> 	u64 num_cpuid_config;
 >
> 
> 	...
> 
> 	tdx_sys_metadata_field_read(NUM_CPUID_CONFIG, &num_cpuid_config);
> 
> 	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);
> 
> 	tdx_info->num_cpuid_config = num_cpuid_config;

Dosen't num_cpuid_config serve as temporary variable in some sense?

For this case, it needs to be used for calculating the size of tdx_info. 
So we have to have it. But it's not the common case.

E.g., if we have another non-u64 field (e.g., field_x) in tdx_info, we 
cannot to read it via

	tdx_sys_metadata_field_read(FIELD_X_ID, &tdx_info->field_x);

we have to use a temporary u64 variable.

> 	...
> 
> (you can do explicit (u16)num_cpuid_config type cast above if you want.)
> 
> With your suggestion, here is what Isaku can do:
> 
> 	u16 num_cpuid_config;
> 
> 	...
> 
> 	tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
> sizeof(num_cpuid_config),
> 				&num_cpuid_config);
> 
> 	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);
> 
> 	tdx_info->num_cpuid_config = num_cpuid_config;
> 
> 	...
> 
> I don't see big difference?
> 
> One example that the current tdx_sys_metadata_field_read() doesn't quite fit is
> you have something like this:
> 
> 	struct {
> 		u16 whatever;
> 		...
> 	} st;
> 
> 	tdx_sys_metadata_field_read(FIELD_ID_WHATEVER, &st.whatever);
> 
> But for this use case you are not supposed to use tdx_sys_metadata_field_read(),
> but use tdx_sys_metadata_read() which has a mapping provided anyway.
> 
> So, while I don't quite object your proposal, I don't see it being quite
> necessary.
> 
> I'll let other people to have a say.
> 
>

Huang, Kai March 15, 2024, 5:39 a.m. UTC | #6

On Fri, 2024-03-15 at 13:11 +0800, Li, Xiaoyao wrote:
> > Here is what Isaku can do using the current API:
> > 
> >  	u64 num_cpuid_config;
>  >
> > 
> >  	...
> > 
> >  	tdx_sys_metadata_field_read(NUM_CPUID_CONFIG, &num_cpuid_config);
> > 
> >  	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);
> > 
> >  	tdx_info->num_cpuid_config = num_cpuid_config;
> 
> Dosen't num_cpuid_config serve as temporary variable in some sense?

You need it, regardless whether it is u64 or u16.

> 
> For this case, it needs to be used for calculating the size of tdx_info. 
> So we have to have it. But it's not the common case.
> 
> E.g., if we have another non-u64 field (e.g., field_x) in tdx_info, we 
> cannot to read it via
> 
> 	tdx_sys_metadata_field_read(FIELD_X_ID, &tdx_info->field_x);
> 
> we have to use a temporary u64 variable.

Let me repeat below in my _previous_ reply:

"
One example that the current tdx_sys_metadata_field_read() doesn't quite fit is
you have something like this:

	struct {
		u16 whatever;
		...
	} st;

	tdx_sys_metadata_field_read(FIELD_ID_WHATEVER, &st.whatever);

But for this use case you are not supposed to use tdx_sys_metadata_field_read(),
but use tdx_sys_metadata_read() which has a mapping provided anyway.
"

So sorry I am not seeing a real example from you.

Xiaoyao Li March 15, 2024, 5:50 a.m. UTC | #7

On 3/15/2024 1:39 PM, Huang, Kai wrote:
> On Fri, 2024-03-15 at 13:11 +0800, Li, Xiaoyao wrote:
>>> Here is what Isaku can do using the current API:
>>>
>>>   	u64 num_cpuid_config;
>>   >
>>>
>>>   	...
>>>
>>>   	tdx_sys_metadata_field_read(NUM_CPUID_CONFIG, &num_cpuid_config);
>>>
>>>   	tdx_info = kzalloc(calculate_tdx_info_size(num_cpuid_config), ...);
>>>
>>>   	tdx_info->num_cpuid_config = num_cpuid_config;
>>
>> Dosen't num_cpuid_config serve as temporary variable in some sense?
> 
> You need it, regardless whether it is u64 or u16.
> 
>>
>> For this case, it needs to be used for calculating the size of tdx_info.
>> So we have to have it. But it's not the common case.
>>
>> E.g., if we have another non-u64 field (e.g., field_x) in tdx_info, we
>> cannot to read it via
>>
>> 	tdx_sys_metadata_field_read(FIELD_X_ID, &tdx_info->field_x);
>>
>> we have to use a temporary u64 variable.
> 
> Let me repeat below in my _previous_ reply:
> 
> "
> One example that the current tdx_sys_metadata_field_read() doesn't quite fit is
> you have something like this:
> 
> 	struct {
> 		u16 whatever;
> 		...
> 	} st;
> 
> 	tdx_sys_metadata_field_read(FIELD_ID_WHATEVER, &st.whatever);
> 
> But for this use case you are not supposed to use tdx_sys_metadata_field_read(),
> but use tdx_sys_metadata_read() which has a mapping provided anyway.
> "

tdx_sys_metadata_read() is too complicated for just reading one field.

Caller needs to prepare a one-item size array of "struct 
tdx_metadata_field_mapping" and pass the correct offset.

> So sorry I am not seeing a real example from you.
>

Isaku Yamahata March 15, 2024, 4:22 p.m. UTC | #8

On Fri, Mar 15, 2024 at 12:09:29PM +1300,
"Huang, Kai" <[e-mail]> wrote:

> 
> > +struct tdx_info {
> > +	u64 features0;
> > +	u64 attributes_fixed0;
> > +	u64 attributes_fixed1;
> > +	u64 xfam_fixed0;
> > +	u64 xfam_fixed1;
> > +
> > +	u16 num_cpuid_config;
> > +	/* This must the last member. */
> > +	DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
> > +};
> > +
> > +/* Info about the TDX module. */
> > +static struct tdx_info *tdx_info;
> > +
> >   #define TDX_MD_MAP(_fid, _ptr)			\
> >   	{ .fid = MD_FIELD_ID_##_fid,		\
> >   	  .ptr = (_ptr), }
> > @@ -66,7 +81,7 @@ static size_t tdx_md_element_size(u64 fid)
> >   	}
> >   }
> > -static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> > +static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> >   {
> >   	struct tdx_md_map *m;
> >   	int ret, i;
> > @@ -84,9 +99,26 @@ static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
> >   	return 0;
> >   }
> > +#define TDX_INFO_MAP(_field_id, _member)			\
> > +	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
> > +
> >   static int __init tdx_module_setup(void)
> >   {
> > +	u16 num_cpuid_config;
> >   	int ret;
> > +	u32 i;
> > +
> > +	struct tdx_md_map mds[] = {
> > +		TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
> > +	};
> > +
> > +	struct tdx_metadata_field_mapping fields[] = {
> > +		TDX_INFO_MAP(FEATURES0, features0),
> > +		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
> > +		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
> > +		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
> > +		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
> > +	};
> >   	ret = tdx_enable();
> >   	if (ret) {
> > @@ -94,7 +126,48 @@ static int __init tdx_module_setup(void)
> >   		return ret;
> >   	}
> > +	ret = tdx_md_read(mds, ARRAY_SIZE(mds));
> > +	if (ret)
> > +		return ret;
> > +
> > +	tdx_info = kzalloc(sizeof(*tdx_info) +
> > +			   sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
> > +			   GFP_KERNEL);
> > +	if (!tdx_info)
> > +		return -ENOMEM;
> > +	tdx_info->num_cpuid_config = num_cpuid_config;
> > +
> > +	ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
> > +	if (ret)
> > +		goto error_out;
> > +
> > +	for (i = 0; i < num_cpuid_config; i++) {
> > +		struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
> > +		u64 leaf, eax_ebx, ecx_edx;
> > +		struct tdx_md_map cpuids[] = {
> > +			TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
> > +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
> > +			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
> > +		};
> > +
> > +		ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
> > +		if (ret)
> > +			goto error_out;
> > +
> > +		c->leaf = (u32)leaf;
> > +		c->sub_leaf = leaf >> 32;
> > +		c->eax = (u32)eax_ebx;
> > +		c->ebx = eax_ebx >> 32;
> > +		c->ecx = (u32)ecx_edx;
> > +		c->edx = ecx_edx >> 32;
> 
> OK I can see why you don't want to use ...
> 
> 	struct tdx_metadata_field_mapping fields[] = {
> 		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
> 	};
> 
> ... to read num_cpuid_config first, because the memory to hold @tdx_info
> hasn't been allocated, because its size depends on the num_cpuid_config.
> 
> And I confess it's because the tdx_sys_metadata_field_read() that got
> exposed in patch ("x86/virt/tdx: Export global metadata read
> infrastructure") only returns 'u64' for all metadata field, and you didn't
> want to use something like this:
> 
> 	u64 num_cpuid_config;
> 	
> 	tdx_sys_metadata_field_read(..., &num_cpuid_config);
> 
> 	...
> 
> 	tdx_info->num_cpuid_config = num_cpuid_config;
> 
> Or you can explicitly cast:
> 
> 	tdx_info->num_cpuid_config = (u16)num_cpuid_config;
> 
> (I know people may don't like the assigning 'u64' to 'u16', but it seems
> nothing wrong to me, because the way done in (1) below effectively has the
> same result comparing to type case).
> 
> But there are other (better) ways to do:
> 
> 1) you can introduce a helper as suggested by Xiaoyao in [*]:
> 
> 
> 	int tdx_sys_metadata_read_single(u64 field_id,
> 					int bytes,  void *buf)
> 	{
> 		return stbuf_read_sys_metadata_field(field_id, 0,
> 						bytes, buf);
> 	}
> 
> And do:
> 
> 	tdx_sys_metadata_read_single(NUM_CPUID_CONFIG,
> 		sizeof(num_cpuid_config), &num_cpuid_config);
> 
> That's _much_ cleaner than the 'struct tdx_md_map', which only confuses
> people.
> 
> But I don't think we need to do this as mentioned above -- we just do type
> cast.
> 
> 2) You can just preallocate enough memory.  It cannot be larger than 1024B,
> right?  You can even just allocate one page.  It's just 4K, no one cares.
> 
> Then you can do:
> 
> 	struct tdx_metadata_field_mapping tdx_info_fields = {
> 		...
> 		TDX_INFO_MAP(NUM_CPUID_CONFIG, num_cpuid_config),
> 	};
> 
> 	tdx_sys_metadata_read(tdx_info_fields,
> 			ARRAY_SIZE(tdx_info_fields, tdx_info);
> 
> And then you read the CPUID_CONFIG array one by one using the same 'struct
> tdx_metadata_field_mapping' and tdx_sys_metadata_read():
> 
> 
> 	for (i = 0; i < tdx_info->num_cpuid_config; i++) {
> 		struct tdx_metadata_field_mapping cpuid_fields = {
> 			TDX_CPUID_CONFIG_MAP(CPUID_CONFIG_LEAVES + i,
> 						leaf),
> 			...
> 		};
> 		struct kvm_tdx_cpuid_config *c =
> 				&tdx_info->cpuid_configs[i];
> 
> 		tdx_sys_metadata_read(cpuid_fields,
> 				ARRAY_SIZE(cpuid_fields), c);
> 
> 		....
> 	}
> 
> So stopping having the duplicated 'struct tdx_md_map' and related staff, as
> they are absolutely unnecessary and only confuses people.


Ok, I'll rewrite the code to use tdx_sys_metadata_read() by introducng
tentative struct in a function scope.

Patch

diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
index aa7a56a47564..45b2c2304491 100644
--- a/arch/x86/include/uapi/asm/kvm.h
+++ b/arch/x86/include/uapi/asm/kvm.h
@@ -567,4 +567,15 @@  struct kvm_pmu_event_filter {
 #define KVM_X86_TDX_VM		2
 #define KVM_X86_SNP_VM		3
 
+#define KVM_TDX_CPUID_NO_SUBLEAF	((__u32)-1)
+
+struct kvm_tdx_cpuid_config {
+	__u32 leaf;
+	__u32 sub_leaf;
+	__u32 eax;
+	__u32 ebx;
+	__u32 ecx;
+	__u32 edx;
+};
+
 #endif /* _ASM_X86_KVM_H */
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index fa19682b366c..a948a6959ac7 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -32,6 +32,13 @@  static __init int vt_hardware_setup(void)
 	return 0;
 }
 
+static void vt_hardware_unsetup(void)
+{
+	if (enable_tdx)
+		tdx_hardware_unsetup();
+	vmx_hardware_unsetup();
+}
+
 static int vt_vm_init(struct kvm *kvm)
 {
 	if (is_td(kvm))
@@ -54,7 +61,7 @@  struct kvm_x86_ops vt_x86_ops __initdata = {
 
 	.check_processor_compatibility = vmx_check_processor_compat,
 
-	.hardware_unsetup = vmx_hardware_unsetup,
+	.hardware_unsetup = vt_hardware_unsetup,
 
 	/* TDX cpu enablement is done by tdx_hardware_setup(). */
 	.hardware_enable = vmx_hardware_enable,
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index dce21f675155..5edfb99abb89 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -40,6 +40,21 @@  static void __used tdx_guest_keyid_free(int keyid)
 	ida_free(&tdx_guest_keyid_pool, keyid);
 }
 
+struct tdx_info {
+	u64 features0;
+	u64 attributes_fixed0;
+	u64 attributes_fixed1;
+	u64 xfam_fixed0;
+	u64 xfam_fixed1;
+
+	u16 num_cpuid_config;
+	/* This must the last member. */
+	DECLARE_FLEX_ARRAY(struct kvm_tdx_cpuid_config, cpuid_configs);
+};
+
+/* Info about the TDX module. */
+static struct tdx_info *tdx_info;
+
 #define TDX_MD_MAP(_fid, _ptr)			\
 	{ .fid = MD_FIELD_ID_##_fid,		\
 	  .ptr = (_ptr), }
@@ -66,7 +81,7 @@  static size_t tdx_md_element_size(u64 fid)
 	}
 }
 
-static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
+static int tdx_md_read(struct tdx_md_map *maps, int nr_maps)
 {
 	struct tdx_md_map *m;
 	int ret, i;
@@ -84,9 +99,26 @@  static int __used tdx_md_read(struct tdx_md_map *maps, int nr_maps)
 	return 0;
 }
 
+#define TDX_INFO_MAP(_field_id, _member)			\
+	TD_SYSINFO_MAP(_field_id, struct tdx_info, _member)
+
 static int __init tdx_module_setup(void)
 {
+	u16 num_cpuid_config;
 	int ret;
+	u32 i;
+
+	struct tdx_md_map mds[] = {
+		TDX_MD_MAP(NUM_CPUID_CONFIG, &num_cpuid_config),
+	};
+
+	struct tdx_metadata_field_mapping fields[] = {
+		TDX_INFO_MAP(FEATURES0, features0),
+		TDX_INFO_MAP(ATTRS_FIXED0, attributes_fixed0),
+		TDX_INFO_MAP(ATTRS_FIXED1, attributes_fixed1),
+		TDX_INFO_MAP(XFAM_FIXED0, xfam_fixed0),
+		TDX_INFO_MAP(XFAM_FIXED1, xfam_fixed1),
+	};
 
 	ret = tdx_enable();
 	if (ret) {
@@ -94,7 +126,48 @@  static int __init tdx_module_setup(void)
 		return ret;
 	}
 
+	ret = tdx_md_read(mds, ARRAY_SIZE(mds));
+	if (ret)
+		return ret;
+
+	tdx_info = kzalloc(sizeof(*tdx_info) +
+			   sizeof(*tdx_info->cpuid_configs) * num_cpuid_config,
+			   GFP_KERNEL);
+	if (!tdx_info)
+		return -ENOMEM;
+	tdx_info->num_cpuid_config = num_cpuid_config;
+
+	ret = tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdx_info);
+	if (ret)
+		goto error_out;
+
+	for (i = 0; i < num_cpuid_config; i++) {
+		struct kvm_tdx_cpuid_config *c = &tdx_info->cpuid_configs[i];
+		u64 leaf, eax_ebx, ecx_edx;
+		struct tdx_md_map cpuids[] = {
+			TDX_MD_MAP(CPUID_CONFIG_LEAVES + i, &leaf),
+			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2, &eax_ebx),
+			TDX_MD_MAP(CPUID_CONFIG_VALUES + i * 2 + 1, &ecx_edx),
+		};
+
+		ret = tdx_md_read(cpuids, ARRAY_SIZE(cpuids));
+		if (ret)
+			goto error_out;
+
+		c->leaf = (u32)leaf;
+		c->sub_leaf = leaf >> 32;
+		c->eax = (u32)eax_ebx;
+		c->ebx = eax_ebx >> 32;
+		c->ecx = (u32)ecx_edx;
+		c->edx = ecx_edx >> 32;
+	}
+
 	return 0;
+
+error_out:
+	/* kfree() accepts NULL. */
+	kfree(tdx_info);
+	return ret;
 }
 
 bool tdx_is_vm_type_supported(unsigned long type)
@@ -162,3 +235,8 @@  int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops)
 out:
 	return r;
 }
+
+void tdx_hardware_unsetup(void)
+{
+	kfree(tdx_info);
+}
diff --git a/arch/x86/kvm/vmx/x86_ops.h b/arch/x86/kvm/vmx/x86_ops.h
index f4da88a228d0..e8cb4ae81cf1 100644
--- a/arch/x86/kvm/vmx/x86_ops.h
+++ b/arch/x86/kvm/vmx/x86_ops.h
@@ -136,9 +136,11 @@  void vmx_setup_mce(struct kvm_vcpu *vcpu);
 
 #ifdef CONFIG_INTEL_TDX_HOST
 int __init tdx_hardware_setup(struct kvm_x86_ops *x86_ops);
+void tdx_hardware_unsetup(void);
 bool tdx_is_vm_type_supported(unsigned long type);
 #else
 static inline int tdx_hardware_setup(struct kvm_x86_ops *x86_ops) { return -EOPNOTSUPP; }
+static inline void tdx_hardware_unsetup(void) {}
 static inline bool tdx_is_vm_type_supported(unsigned long type) { return false; }
 #endif