Date: Fri, 15 Mar 2024 10:18:52 +0800
From: Xiaoyao Li <xiaoyao.li@...el.com>
To: "Huang, Kai" <kai.huang@...el.com>, isaku.yamahata@...el.com,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
 erdemaktas@...gle.com, Sean Christopherson <seanjc@...gle.com>,
 Sagi Shahar <sagis@...gle.com>, chen.bo@...el.com, hang.yuan@...el.com,
 tina.zhang@...el.com
Subject: Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX
 module on initialization

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.

> [*] 
> https://lore.kernel.org/lkml/bd61e29d-5842-4136-b30f-929b00bdf6f9@intel.com/T/#m2512e378c83bc44d3ca653f96f25c3fc85eb0e8a
> 
> 
> 
> 

Powered by blists - more mailing lists