Received-SPF: pass (google.com: domain of linux-kernel+bounces-104064-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <10c41a88-d692-4ff5-a0c3-ae13a06a055c@intel.com>
Date: Fri, 15 Mar 2024 13:11:37 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX
 module on initialization
Content-Language: en-US
To: "Huang, Kai" <kai.huang@intel.com>,
 "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 "Yamahata, Isaku" <isaku.yamahata@intel.com>
Cc: "Zhang, Tina" <tina.zhang@intel.com>,
 "seanjc@google.com" <seanjc@google.com>, "Yuan, Hang" <hang.yuan@intel.com>,
 "Chen, Bo2" <chen.bo@intel.com>, "sagis@google.com" <sagis@google.com>,
 "isaku.yamahata@gmail.com" <isaku.yamahata@gmail.com>,
 "Aktas, Erdem" <erdemaktas@google.com>,
 "pbonzini@redhat.com" <pbonzini@redhat.com>
References: <cover.1708933498.git.isaku.yamahata@intel.com>
 <eaa2c1e23971f058e5921681b0b84d7ea7d38dc1.1708933498.git.isaku.yamahata@intel.com>
 <e88e5448-e354-4ec6-b7de-93dd8f7786b5@intel.com>
 <15a13c5d-df88-46cf-8d88-2c8b94ff41ff@intel.com>
 <aa1ed4c118177e3e341eebccecac3b07bf75a47d.camel@intel.com>
From: Xiaoyao Li <xiaoyao.li@intel.com>
In-Reply-To: <aa1ed4c118177e3e341eebccecac3b07bf75a47d.camel@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 3/15/2024 12:57 PM, Huang, Kai wrote:
> 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;

Dosen't num_cpuid_config serve as temporary variable in some sense?

For this case, it needs to be used for calculating the size of tdx_info. 
So we have to have it. But it's not the common case.

E.g., if we have another non-u64 field (e.g., field_x) in tdx_info, we 
cannot to read it via

	tdx_sys_metadata_field_read(FIELD_X_ID, &tdx_info->field_x);

we have to use a temporary u64 variable.

> 	...
> 
> (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.
> 
>