Date: Fri, 15 Mar 2024 12:09:29 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: <isaku.yamahata@...el.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <isaku.yamahata@...il.com>, Paolo Bonzini <pbonzini@...hat.com>,
	<erdemaktas@...gle.com>, Sean Christopherson <seanjc@...gle.com>, Sagi Shahar
	<sagis@...gle.com>, <chen.bo@...el.com>, <hang.yuan@...el.com>,
	<tina.zhang@...el.com>
Subject: Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX
 module on initialization


> +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/bd61e29d-5842-4136-b30f-929b00bdf6f9@intel.com/T/#m2512e378c83bc44d3ca653f96f25c3fc85eb0e8a

Powered by blists - more mailing lists