Received-SPF: pass (google.com: domain of linux-kernel+bounces-104623-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 15 Mar 2024 09:22:55 -0700
From: Isaku Yamahata <isaku.yamahata@intel.com>
To: "Huang, Kai" <kai.huang@intel.com>
Cc: isaku.yamahata@intel.com, kvm@vger.kernel.org,
	linux-kernel@vger.kernel.org, isaku.yamahata@gmail.com,
	Paolo Bonzini <pbonzini@redhat.com>, erdemaktas@google.com,
	Sean Christopherson <seanjc@google.com>,
	Sagi Shahar <sagis@google.com>, chen.bo@intel.com,
	hang.yuan@intel.com, tina.zhang@intel.com,
	isaku.yamahata@linux.intel.com
Subject: Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX
 module on initialization
Message-ID: <20240315162255.GG1258280@ls.amr.corp.intel.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <eaa2c1e23971f058e5921681b0b84d7ea7d38dc1.1708933498.git.isaku.yamahata@intel.com>
 <e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com>

On Fri, Mar 15, 2024 at 12:09:29PM +1300,
"Huang, Kai" <kai.huang@intel.com> 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.
-- 
Isaku Yamahata <isaku.yamahata@intel.com>