Re: [PATCH v3 07/10] x86/virt/tdx: Convert the td_conf metadata reader

[Nikolay Borisov <nik.borisov@suse.com>]

On 2.10.26 г. 15:28 ч., Chao Gao wrote:
> On Fri, Oct 02, 2026 at 04:28:54AM +0800, Edgecombe, Rick P wrote:
>> On Thu, 2026-10-01 at 19:34 +0300, Nikolay Borisov wrote:
>>>> The comments are not strictly required, but probably fit into turning this
>>>> thing
>>>> into "hand coding". I think it's a nice addition. I wonder if we should make
>>>> the
>>>> "i * 2 + j" a bit clearer though?
>>>>
>>>> I'm not against leaving it as is for now. It helps that it doesn't morph too
>>>> much from the code getting moved. But I think the above falls a bit short of
>>>> hand coded quality.
>>>
>>> Perhaps this code warrants open coding? The code will end up looking
>>> like config_values[i][0] ... config_values[i][1] ....
>>
>> Yea that would be a good way to improve it.
> 
> Yes. With one less level of indentation, all three reads get the same
> shape. I ended up with:
> 
> 	/*
> 	 * cpuid_config_leaves[i] holds a CPUID leaf and sub-leaf number,
> 	 * and the two u64s of cpuid_config_values[i] mark which bits of
> 	 * that leaf's EAX/EBX/ECX/EDX the host can set to 0 or 1 for a TD.
> 	 *
> 	 * TDX_FIELD_CPUID_CONFIG_LEAVES and TDX_FIELD_CPUID_CONFIG_VALUES
> 	 * give the field ID of each array's first element, and the rest
> 	 * follow in order.
> 	 */
> 	for (i = 0; i < td_conf->num_cpuid_config; i++) {
> 		ret = read_sys_metadata_field(TDX_FIELD_CPUID_CONFIG_LEAVES + i,
> 					      &td_conf->cpuid_config_leaves[i]);
> 		if (ret)
> 			return ret;
> 
> 		ret = read_sys_metadata_field(TDX_FIELD_CPUID_CONFIG_VALUES + i * 2,
> 					      &td_conf->cpuid_config_values[i][0]);
> 		if (ret)
> 			return ret;

nit: Is this more readable:

	int cpuid_config_val = TDX_FIELD_CPUID_CONFIG_VALUES + i * 2;
	read_sys_metadata_field(cpuid_config_val...)
	read_sys_metadata_field(cpuid_config_val+1...)

> 
> 		ret = read_sys_metadata_field(TDX_FIELD_CPUID_CONFIG_VALUES + i * 2 + 1,
> 					      &td_conf->cpuid_config_values[i][1]);
> 		if (ret)
> 			return ret;
> 	}