Message-ID: <0b1f3c07-a1e9-4008-8de5-52b1fea7ad7b@redhat.com>
Date: Mon, 28 Oct 2024 19:35:22 +0100
From: Paolo Bonzini <pbonzini@...hat.com>
To: Kai Huang <kai.huang@...el.com>, dave.hansen@...el.com,
 kirill.shutemov@...ux.intel.com, tglx@...utronix.de, bp@...en8.de,
 peterz@...radead.org, mingo@...hat.com, hpa@...or.com,
 dan.j.williams@...el.com, seanjc@...gle.com
Cc: x86@...nel.org, linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
 rick.p.edgecombe@...el.com, isaku.yamahata@...el.com,
 adrian.hunter@...el.com, nik.borisov@...e.com, Klaus Kiwi <kkiwi@...hat.com>
Subject: Re: [PATCH v6 00/10] TDX host: metadata reading tweaks, bug fix and
 info dump

On 10/28/24 13:41, Kai Huang wrote:
> This series does necessary tweaks to TDX host "global metadata" reading
> code to fix some immediate issues in the TDX module initialization code,
> with intention to also provide a flexible code base to support sharing
> global metadata to KVM (and other kernel components) for future needs.

Kai/Dave/Rick,

the v6 of this series messes up the TDX patches for KVM, which do not 
apply anymore. I can work on a rebase myself for the sake of putting 
this series in kvm-coco-queue; but please help me a little bit by 
including in the generated data all the fields that KVM needs.

Are you able to send quickly a v7 that includes these fields, and that 
also checks in the script that generates the files?

Emphasis on "quickly".  No internal review processes of any kind, please.

Thanks,

Paolo

> This series, and additional patches to initialize TDX when loading KVM
> module and read essential metadata fields for KVM TDX can be found at
> [1].
> 
> Hi Dave (and maintainers),
> 
> This series targets x86 tip.  Also add Dan, KVM maintainers and KVM list
> so people can also review and comment.
> 
> This is a pre-work of the "quite near future" KVM TDX support.  I
> appreciate if you can review, comment and take this series if the
> patches look good to you.
> 
> History:
> 
> v5 -> v6:
>   - Change to use a script [*] to auto-generate metadata reading code.
> 
>    - https://lore.kernel.org/kvm/f25673ea-08c5-474b-a841-095656820b67@intel.com/
>    - https://lore.kernel.org/kvm/CABgObfYXUxqQV_FoxKjC8U3t5DnyM45nz5DpTxYZv2x_uFK_Kw@mail.gmail.com/
> 
>     Per Dave, this patchset doesn't contain a patch to add the script
>     to the kernel tree but append it in this cover letter in order to
>     minimize the review effort.
> 
>   - Change to use auto-generated code to read TDX module version,
>     supported features and CMRs in one patch, and made that from and
>     signed by Paolo.
>   - Couple of new patches due to using the auto-generated code
>   - Remove the "reading metadata" part (due to they are auto-generated
>     in one patch now) from the consumer patches.
> 
> Pervious versions and more background please see:
> 
>   - https://lore.kernel.org/kvm/9a06e2cf469cbca2777ac2c4ef70579e6bb934d5.camel@intel.com/T/
> 
> [1]: https://github.com/intel/tdx/tree/kvm-tdxinit-host-metadata-v6
> 
> [*] The script used to generate the patch 3:
> 
> #! /usr/bin/env python3
> import json
> import sys
> 
> # Note: this script does not run as part of the build process.
> # It is used to generate structs from the TDX global_metadata.json
> # file, and functions to fill in said structs.  Rerun it if
> # you need more fields.
> 
> TDX_STRUCTS = {
>      "version": [
>          "BUILD_DATE",
>          "BUILD_NUM",
>          "MINOR_VERSION",
>          "MAJOR_VERSION",
>          "UPDATE_VERSION",
>          "INTERNAL_VERSION",
>      ],
>      "features": [
>          "TDX_FEATURES0"
>      ],
>      "tdmr": [
>          "MAX_TDMRS",
>          "MAX_RESERVED_PER_TDMR",
>          "PAMT_4K_ENTRY_SIZE",
>          "PAMT_2M_ENTRY_SIZE",
>          "PAMT_1G_ENTRY_SIZE",
>      ],
>      "cmr": [
>          "NUM_CMRS", "CMR_BASE", "CMR_SIZE"
>      ],
> #   "td_ctrl": [
> #        "TDR_BASE_SIZE",
> #        "TDCS_BASE_SIZE",
> #        "TDVPS_BASE_SIZE",
> #    ],
> #    "td_conf": [
> #        "ATTRIBUTES_FIXED0",
> #        "ATTRIBUTES_FIXED1",
> #        "XFAM_FIXED0",
> #        "XFAM_FIXED1",
> #        "NUM_CPUID_CONFIG",
> #        "MAX_VCPUS_PER_TD",
> #        "CPUID_CONFIG_LEAVES",
> #        "CPUID_CONFIG_VALUES",
> #    ],
> }
> 
> def print_class_struct_field(field_name, element_bytes, num_fields, num_elements, file):
>      element_type = "u%s" % (element_bytes * 8)
>      element_array = ""
>      if num_fields > 1:
>          element_array += "[%d]" % (num_fields)
>      if num_elements > 1:
>          element_array += "[%d]" % (num_elements)
>      print("\t%s %s%s;" % (element_type, field_name, element_array), file=file)
> 
> def print_class_struct(class_name, fields, file):
>      struct_name = "tdx_sys_info_%s" % (class_name)
>      print("struct %s {" % (struct_name), file=file)
>      for f in fields:
>          print_class_struct_field(
>              f["Field Name"].lower(),
>              int(f["Element Size (Bytes)"]),
>              int(f["Num Fields"]),
>              int(f["Num Elements"]),
>              file=file)
>      print("};", file=file)
> 
> def print_read_field(field_id, struct_var, struct_member, indent, file):
>      print(
>          "%sif (!ret && !(ret = read_sys_metadata_field(%s, &val)))\n%s\t%s->%s = val;"
>          % (indent, field_id, indent, struct_var, struct_member),
>          file=file,
>      )
> 
> def print_class_function(class_name, fields, file):
>      func_name = "get_tdx_sys_info_%s" % (class_name)
>      struct_name = "tdx_sys_info_%s" % (class_name)
>      struct_var = "sysinfo_%s" % (class_name)
> 
>      print("static int %s(struct %s *%s)" % (func_name, struct_name, struct_var), file=file)
>      print("{", file=file)
>      print("\tint ret = 0;", file=file)
>      print("\tu64 val;", file=file)
> 
>      has_i = 0
>      has_j = 0
>      for f in fields:
>          num_fields = int(f["Num Fields"])
>          num_elements = int(f["Num Elements"])
>          if num_fields > 1:
>              has_i = 1
>          if num_elements > 1:
>              has_j = 1
> 
>      if has_i == 1 and has_j == 1:
>          print("\tint i, j;", file=file)
>      elif has_i == 1:
>          print("\tint i;", file=file)
> 
>      print(file=file)
>      for f in fields:
>          fname = f["Field Name"]
>          field_id = f["Base FIELD_ID (Hex)"]
>          num_fields = int(f["Num Fields"])
>          num_elements = int(f["Num Elements"])
>          struct_member = fname.lower()
>          indent = "\t"
>          if num_fields > 1:
>              if fname == "CMR_BASE" or fname == "CMR_SIZE":
>                  limit = "sysinfo_cmr->num_cmrs"
>              elif fname == "CPUID_CONFIG_LEAVES" or fname == "CPUID_CONFIG_VALUES":
>                  limit = "sysinfo_td_conf->num_cpuid_config"
>              else:
>                  limit = "%d" %(num_fields)
>              print("%sfor (i = 0; i < %s; i++)" % (indent, limit), file=file)
>              indent += "\t"
>              field_id += " + i"
>              struct_member += "[i]"
>          if num_elements > 1:
>              print("%sfor (j = 0; j < %d; j++)" % (indent, num_elements), file=file)
>              indent += "\t"
>              field_id += " * 2 + j"
>              struct_member += "[j]"
> 
>          print_read_field(
>              field_id,
>              struct_var,
>              struct_member,
>              indent,
>              file=file,
>          )
> 
>      print(file=file)
>      print("\treturn ret;", file=file)
>      print("}", file=file)
> 
> def print_main_struct(file):
>      print("struct tdx_sys_info {", file=file)
>      for class_name, field_names in TDX_STRUCTS.items():
>          struct_name = "tdx_sys_info_%s" % (class_name)
>          struct_var = class_name
>          print("\tstruct %s %s;" % (struct_name, struct_var), file=file)
>      print("};", file=file)
> 
> def print_main_function(file):
>      print("static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)", file=file)
>      print("{", file=file)
>      print("\tint ret = 0;", file=file)
>      print(file=file)
>      for class_name, field_names in TDX_STRUCTS.items():
>          func_name = "get_tdx_sys_info_" + class_name
>          struct_var = class_name
>          print("\tret = ret ?: %s(&sysinfo->%s);" % (func_name, struct_var), file=file)
>      print(file=file)
>      print("\treturn ret;", file=file)
>      print("}", file=file)
> 
> jsonfile = sys.argv[1]
> hfile = sys.argv[2]
> cfile = sys.argv[3]
> hfileifdef = hfile.replace(".", "_")
> 
> with open(jsonfile, "r") as f:
>      json_in = json.load(f)
>      fields = {x["Field Name"]: x for x in json_in["Fields"]}
> 
> with open(hfile, "w") as f:
>      print("/* SPDX-License-Identifier: GPL-2.0 */", file=f)
>      print("/* Automatically generated TDX global metadata structures. */", file=f)
>      print("#ifndef _X86_VIRT_TDX_AUTO_GENERATED_" + hfileifdef.upper(), file=f)
>      print("#define _X86_VIRT_TDX_AUTO_GENERATED_" + hfileifdef.upper(), file=f)
>      print(file=f)
>      print("#include <linux/types.h>", file=f)
>      print(file=f)
>      for class_name, field_names in TDX_STRUCTS.items():
>          print_class_struct(class_name, [fields[x] for x in field_names], file=f)
>          print(file=f)
>      print_main_struct(file=f)
>      print(file=f)
>      print("#endif", file=f)
> 
> with open(cfile, "w") as f:
>      print("// SPDX-License-Identifier: GPL-2.0", file=f)
>      print("/*", file=f)
>      print(" * Automatically generated functions to read TDX global metadata.", file=f)
>      print(" *", file=f)
>      print(" * This file doesn't compile on its own as it lacks of inclusion", file=f)
>      print(" * of SEAMCALL wrapper primitive which reads global metadata.", file=f)
>      print(" * Include this file to other C file instead.", file=f)
>      print(" */", file=f)
>      for class_name, field_names in TDX_STRUCTS.items():
>          print(file=f)
>          print_class_function(class_name, [fields[x] for x in field_names], file=f)
>      print(file=f)
>      print_main_function(file=f)
> 
> 
> 
> 
> Kai Huang (9):
>    x86/virt/tdx: Rename 'struct tdx_tdmr_sysinfo' to reflect the spec
>      better
>    x86/virt/tdx: Start to track all global metadata in one structure
>    x86/virt/tdx: Use dedicated struct members for PAMT entry sizes
>    x86/virt/tdx: Add missing header file inclusion to local tdx.h
>    x86/virt/tdx: Switch to use auto-generated global metadata reading
>      code
>    x86/virt/tdx: Trim away tail null CMRs
>    x86/virt/tdx: Reduce TDMR's reserved areas by using CMRs to find
>      memory holes
>    x86/virt/tdx: Require the module to assert it has the NO_RBP_MOD
>      mitigation
>    x86/virt/tdx: Print TDX module version
> 
> Paolo Bonzini (1):
>    x86/virt/tdx: Use auto-generated code to read global metadata
> 
>   arch/x86/virt/vmx/tdx/tdx.c                 | 178 ++++++++++++--------
>   arch/x86/virt/vmx/tdx/tdx.h                 |  43 +----
>   arch/x86/virt/vmx/tdx/tdx_global_metadata.c |  89 ++++++++++
>   arch/x86/virt/vmx/tdx/tdx_global_metadata.h |  42 +++++
>   4 files changed, 247 insertions(+), 105 deletions(-)
>   create mode 100644 arch/x86/virt/vmx/tdx/tdx_global_metadata.c
>   create mode 100644 arch/x86/virt/vmx/tdx/tdx_global_metadata.h
> 
> 
> base-commit: 21f0d4005e7eb71b95cf6b55041fd525bdb11c1f

Powered by blists - more mailing lists