Re: [PATCH v19 034/130] KVM: TDX: Get system-wide info about TDX module on initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx/T/#m2512e378c83bc44d3ca653f96f25c3fc85eb0e8a









[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux