Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX module on initialization
From: Huang, Kai
Date: Fri Mar 15 2024 - 00:58:21 EST
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.