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.
