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

[Nikolay Borisov]

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;
> 	}