lkml.org 
[lkml]   [2024]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX module on initialization
    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>

    \
     
     \ /
      Last update: 2024-05-27 15:50    [W:7.359 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site