* [PATCH 0/2] Expose TDX Module version @ 2025-10-01 2:22 Chao Gao 2025-10-01 2:22 ` [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version Chao Gao 2025-10-01 2:22 ` [PATCH 2/2] coco/tdx-host: Expose " Chao Gao 0 siblings, 2 replies; 15+ messages in thread From: Chao Gao @ 2025-10-01 2:22 UTC (permalink / raw) To: linux-coco, linux-kernel, x86 Cc: Chao Gao, Borislav Petkov, Dan Williams, Dave Hansen, H. Peter Anvin, Ingo Molnar, Kai Huang, Kirill A. Shutemov, Paolo Bonzini, Thomas Gleixner, Xu Yilun Currently, there is no user interface to get the TDX Module version. However, in bug reporting or analysis scenarios, the first question normally asked is which TDX Module version is on your system, to determine if this is a known issue or a new regression. To address this issue, this series exposes the TDX Module version as sysfs attributes of the tdx_host device [*]. This series is built on the VMX always-on series, which enables VMXON during CPU bringup, and the "tdx_host" device, which creates a virtual device to serve as the user interface for all TDX host features, e.g., TDX Connect. The base-commit is: https://git.kernel.org/pub/scm/linux/kernel/git/devsec/tsm.git/commit/?h=tdx&id=9332e088937f [*]: https://lore.kernel.org/linux-coco/20250919142237.418648-2-dan.j.williams@intel.com/ Chao Gao (2): x86/virt/tdx: Retrieve TDX module version coco/tdx-host: Expose TDX module version .../ABI/testing/sysfs-devices-faux-tdx-host | 6 +++++ MAINTAINERS | 1 + arch/x86/include/asm/tdx_global_metadata.h | 7 ++++++ arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 16 ++++++++++++ drivers/virt/coco/tdx-host/tdx-host.c | 25 ++++++++++++++++++- 5 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-devices-faux-tdx-host -- 2.47.3 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-01 2:22 [PATCH 0/2] Expose TDX Module version Chao Gao @ 2025-10-01 2:22 ` Chao Gao 2025-10-01 15:15 ` Dave Hansen 2025-10-24 14:20 ` Vishal Annapurve 2025-10-01 2:22 ` [PATCH 2/2] coco/tdx-host: Expose " Chao Gao 1 sibling, 2 replies; 15+ messages in thread From: Chao Gao @ 2025-10-01 2:22 UTC (permalink / raw) To: linux-coco, linux-kernel, x86 Cc: Chao Gao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kirill A. Shutemov, Kai Huang, Paolo Bonzini, Dan Williams Each TDX module is associated with a version in the x.y.z format, where x represents the major version, y the minor version, and z the update version. Knowing the running TDX module version is valuable for bug reporting and debugging. Retrieve the TDX module version using the existing metadata reading interface, in preparation for exposing it to userspace via sysfs. Note changes to tdx_global_metadata.{hc} are auto-generated by the following command. $ python tdx.py global_metadata.json tdx_global_metadata.h \ tdx_global_metadata.c The 'tdx.py' can be fetched from [1]. The 'global_metadata.json' can be fetched from [2]. And 'tdx.py' has "BUILD_DATE", "BUILD_NUM" and "INTERNAL_VERSION" in TDX_STRUCTS; they are removed before running the above command as they are not needed for now. Signed-off-by: Chao Gao <chao.gao@intel.com> Link: https://lore.kernel.org/kvm/0853b155ec9aac09c594caa60914ed6ea4dc0a71.camel@intel.com/ # [1] Link: https://cdrdv2.intel.com/v1/dl/getContent/795381 # [2] --- arch/x86/include/asm/tdx_global_metadata.h | 7 +++++++ arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 16 ++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/arch/x86/include/asm/tdx_global_metadata.h b/arch/x86/include/asm/tdx_global_metadata.h index 060a2ad744bf..40689c8dc67e 100644 --- a/arch/x86/include/asm/tdx_global_metadata.h +++ b/arch/x86/include/asm/tdx_global_metadata.h @@ -5,6 +5,12 @@ #include <linux/types.h> +struct tdx_sys_info_version { + u16 minor_version; + u16 major_version; + u16 update_version; +}; + struct tdx_sys_info_features { u64 tdx_features0; }; @@ -35,6 +41,7 @@ struct tdx_sys_info_td_conf { }; struct tdx_sys_info { + struct tdx_sys_info_version version; struct tdx_sys_info_features features; struct tdx_sys_info_tdmr tdmr; struct tdx_sys_info_td_ctrl td_ctrl; diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c index 13ad2663488b..0454124803f3 100644 --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c @@ -7,6 +7,21 @@ * Include this file to other C file instead. */ +static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version) +{ + int ret = 0; + u64 val; + + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) + sysinfo_version->minor_version = val; + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) + sysinfo_version->major_version = val; + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) + sysinfo_version->update_version = val; + + return ret; +} + static int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinfo_features) { int ret = 0; @@ -89,6 +104,7 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo) { int ret = 0; + ret = ret ?: get_tdx_sys_info_version(&sysinfo->version); ret = ret ?: get_tdx_sys_info_features(&sysinfo->features); ret = ret ?: get_tdx_sys_info_tdmr(&sysinfo->tdmr); ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl); -- 2.47.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-01 2:22 ` [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version Chao Gao @ 2025-10-01 15:15 ` Dave Hansen 2025-10-22 7:54 ` Chao Gao 2025-10-24 14:20 ` Vishal Annapurve 1 sibling, 1 reply; 15+ messages in thread From: Dave Hansen @ 2025-10-01 15:15 UTC (permalink / raw) To: Chao Gao, linux-coco, linux-kernel, x86 Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kirill A. Shutemov, Kai Huang, Paolo Bonzini, Dan Williams On 9/30/25 19:22, Chao Gao wrote: > + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) > + sysinfo_version->minor_version = val; > + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) > + sysinfo_version->major_version = val; > + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) > + sysinfo_version->update_version = val; Heh, how long does this take in practice to get 6 bytes of data out of the module? When is the point that we move over to TDH.SYS.RDALL? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-01 15:15 ` Dave Hansen @ 2025-10-22 7:54 ` Chao Gao 2025-10-22 10:26 ` Kiryl Shutsemau 2025-11-13 1:24 ` Chao Gao 0 siblings, 2 replies; 15+ messages in thread From: Chao Gao @ 2025-10-22 7:54 UTC (permalink / raw) To: Dave Hansen Cc: linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kirill A. Shutemov, Kai Huang, Paolo Bonzini, Dan Williams On Wed, Oct 01, 2025 at 08:15:46AM -0700, Dave Hansen wrote: >On 9/30/25 19:22, Chao Gao wrote: >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) >> + sysinfo_version->minor_version = val; >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) >> + sysinfo_version->major_version = val; >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) >> + sysinfo_version->update_version = val; > >Heh, how long does this take in practice to get 6 bytes of data out of >the module? ~8us. And the whole metadata reading process (i.e., get_tdx_sys_info()) takes ~113us. >When is the point that we move over to TDH.SYS.RDALL? TDH.SYS.RDALL takes ~16us. I'm uncertain whether the saved CPU time of ~100us justifies implementing TDH.SYS.RDALL. TDH.SYS.RDALL returns all metadata as a list of arrays, requiring the kernel to parse this structure and iterate through all fields. One advantage of TDH.SYS.RDALL is that it eliminates the need to check field existence before reading, since it simply dumps all available fields rather than targeting specific ones. For example, TDH.SYS.RDALL removes the need for the tdx_supports_ABC() check: in the DPAMT series: + if (!ret && tdx_supports_dynamic_pamt(&tdx_sysinfo) && + !(ret = read_sys_metadata_field(0x9100000100000013, &val))) + sysinfo_tdmr->pamt_page_bitmap_entry_bits = val; in the TDX Module update series: + if (tdx_supports_runtime_update(&tdx_sysinfo) && + !(ret = read_sys_metadata_field(0x8900000100000000, &val))) + sysinfo_handoff->module_hv = val; While iterating through the array list adds some complexity, this is a one-time cost. Once the loop structure is in place, adding new fields only requires inserting a "switch-case" clause within the loop. Please see the draft code below. If TDH.SYS.RDALL is the right direction, I can refine the code and submit a formal patch series. diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c index 128e6ffba736..fa9bb6d47a87 100644 --- a/arch/x86/virt/vmx/tdx/tdx.c +++ b/arch/x86/virt/vmx/tdx/tdx.c @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list) return ret; } -static int read_sys_metadata_field(u64 field_id, u64 *data) +static int read_sys_metadata_field(u64 *field_id, void *ptr) { struct tdx_module_args args = {}; int ret; /* - * TDH.SYS.RD -- reads one global metadata field - * - RDX (in): the field to read - * - R8 (out): the field data + * TDH.SYS.RDALL -- reads all global metadata fields + * - RDX (in): the physical address of the buffer to store + * - R8 (in/out): the initial field ID to read (in) and + * the next field ID to read (out). */ - args.rdx = field_id; - ret = seamcall_prerr_ret(TDH_SYS_RD, &args); + args.rdx = __pa(ptr); + args.r8 = *field_id; + ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args); if (ret) return ret; - - *data = args.r8; + *field_id = args.r8; return 0; } diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h index 1965adb63f1f..44d92047073e 100644 --- a/arch/x86/virt/vmx/tdx/tdx.h +++ b/arch/x86/virt/vmx/tdx/tdx.h @@ -42,6 +42,7 @@ #define TDH_SYS_RD 34 #define TDH_SYS_LP_INIT 35 #define TDH_SYS_TDMR_INIT 36 +#define TDH_SYS_RDALL 37 #define TDH_MEM_TRACK 38 #define TDH_PHYMEM_CACHE_WB 40 #define TDH_PHYMEM_PAGE_WBINVD 41 diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c index 3fdd5cbc21d8..f4b16367ef2f 100644 --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c @@ -7,121 +7,177 @@ * Include this file to other C file instead. */ -static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version) +struct md_field_id { - int ret = 0; - u64 val; - - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) - sysinfo_version->minor_version = val; - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) - sysinfo_version->major_version = val; - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) - sysinfo_version->update_version = val; - - return ret; -} - -static int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinfo_features) -{ - int ret = 0; - u64 val; - - if (!ret && !(ret = read_sys_metadata_field(0x0A00000300000008, &val))) - sysinfo_features->tdx_features0 = val; - - return ret; -} - -static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) + union { + struct { + u32 field_code; // Bits 31:0 + u32 element_size_code : 2; // Bits 33:32 + u32 last_element_in_field : 4; // Bits 37:34 + u32 last_field_in_sequence : 9; // Bits 46:38 + u32 reserved_1 : 3; // Bits 49:47 + u32 inc_size : 1; // Bit 50 + u32 write_mask_valid : 1; // Bit 51 + u32 context_code : 3; // Bits 54:52 + u32 reserved_2 : 1; // Bit 55 + u32 class_code : 6; // Bits 61:56 + u32 reserved_3 : 1; // Bit 62 + u32 non_arch : 1; // Bit 63 + }; + u64 raw; + }; +}; +struct md_list_header { - int ret = 0; - u64 val; - - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000008, &val))) - sysinfo_tdmr->max_tdmrs = val; - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000009, &val))) - sysinfo_tdmr->max_reserved_per_tdmr = val; - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000010, &val))) - sysinfo_tdmr->pamt_4k_entry_size = val; - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000011, &val))) - sysinfo_tdmr->pamt_2m_entry_size = val; - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000012, &val))) - sysinfo_tdmr->pamt_1g_entry_size = val; - - return ret; -} + u16 list_buff_size; + u16 num_sequences; + u32 reserved; +}; -static int get_tdx_sys_info_td_ctrl(struct tdx_sys_info_td_ctrl *sysinfo_td_ctrl) +struct md_sequence { - int ret = 0; - u64 val; + struct md_field_id sequence_header; + u64 element[0]; +}; + +#define TDX_MD_ID_MINOR_VERSION 0x0800000100000003 +#define TDX_MD_ID_MAJOR_VERSION 0x0800000100000004 +#define TDX_MD_ID_UPDATE_VERSION 0x0800000100000005 +#define TDX_MD_ID_MODULE_HV 0x8900000100000000 +#define TDX_MD_ID_TDX_FEATURES0 0x0A00000300000008 +#define TDX_MD_ID_MAX_TDMRS 0x9100000100000008 +#define TDX_MD_ID_MAX_RESERVED_PER_TDMR 0x9100000100000009 +#define TDX_MD_ID_PAMT_4K_ENTRY_SIZE 0x9100000100000010 +#define TDX_MD_ID_PAMT_2M_ENTRY_SIZE 0x9100000100000011 +#define TDX_MD_ID_PAMT_1G_ENTRY_SIZE 0x9100000100000012 +#define TDX_MD_ID_TDR_BASE_SIZE 0x9800000100000000 +#define TDX_MD_ID_TDCS_BASE_SIZE 0x9800000100000100 +#define TDX_MD_ID_TDVPS_BASE_SIZE 0x9800000100000200 +#define TDX_MD_ID_ATTRIBUTES_FIXED0 0x1900000300000000 +#define TDX_MD_ID_ATTRIBUTES_FIXED1 0x1900000300000001 +#define TDX_MD_ID_XFAM_FIXED0 0x1900000300000002 +#define TDX_MD_ID_XFAM_FIXED1 0x1900000300000003 +#define TDX_MD_ID_NUM_CPUID_CONFIG 0x9900000100000004 +#define TDX_MD_ID_MAX_VCPUS_PER_TD 0x9900000100000008 +#define TDX_MD_ID_CPUID_CONFIG_LEAVES 0x9900000300000400 +#define TDX_MD_ID_CPUID_CONFIG_VALUES 0x9900000300000500 - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000000, &val))) - sysinfo_td_ctrl->tdr_base_size = val; - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000100, &val))) - sysinfo_td_ctrl->tdcs_base_size = val; - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000200, &val))) - sysinfo_td_ctrl->tdvps_base_size = val; - - return ret; -} +/* + * Extract CLASS_CODE (bits 61:56), CONTEXT_CODE(bits 54:52), + * FIELD_CODE(bits 23:0) from metadata IDs. Other bits in metadata IDs + * cannot be used for comparison. + */ +#define TDX_MD_ID_MASK 0x3f70000000ffffff -static int get_tdx_sys_info_td_conf(struct tdx_sys_info_td_conf *sysinfo_td_conf) +static void parse_md_sequence(struct md_sequence *sequence, struct tdx_sys_info *sysinfo) { - int ret = 0; - u64 val; + u32 num_elements = (sequence->sequence_header.last_element_in_field + 1) * + (sequence->sequence_header.last_field_in_sequence + 1); + u64 base_id = sequence->sequence_header.raw & TDX_MD_ID_MASK; + u64 field_id; + u64 *data; int i, j; - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000000, &val))) - sysinfo_td_conf->attributes_fixed0 = val; - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000001, &val))) - sysinfo_td_conf->attributes_fixed1 = val; - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000002, &val))) - sysinfo_td_conf->xfam_fixed0 = val; - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000003, &val))) - sysinfo_td_conf->xfam_fixed1 = val; - if (!ret && !(ret = read_sys_metadata_field(0x9900000100000004, &val))) - sysinfo_td_conf->num_cpuid_config = val; - if (!ret && !(ret = read_sys_metadata_field(0x9900000100000008, &val))) - sysinfo_td_conf->max_vcpus_per_td = val; - if (sysinfo_td_conf->num_cpuid_config > ARRAY_SIZE(sysinfo_td_conf->cpuid_config_leaves)) - return -EINVAL; - for (i = 0; i < sysinfo_td_conf->num_cpuid_config; i++) - if (!ret && !(ret = read_sys_metadata_field(0x9900000300000400 + i, &val))) - sysinfo_td_conf->cpuid_config_leaves[i] = val; - if (sysinfo_td_conf->num_cpuid_config > ARRAY_SIZE(sysinfo_td_conf->cpuid_config_values)) - return -EINVAL; - for (i = 0; i < sysinfo_td_conf->num_cpuid_config; i++) - for (j = 0; j < 2; j++) - if (!ret && !(ret = read_sys_metadata_field(0x9900000300000500 + i * 2 + j, &val))) - sysinfo_td_conf->cpuid_config_values[i][j] = val; - - return ret; + pr_info("header 0x%016llx id 0x%016llx elements_in_field %d field %d\n", sequence->sequence_header.raw, base_id, + sequence->sequence_header.last_element_in_field + 1, + sequence->sequence_header.last_field_in_sequence + 1); + + /* Iterate over all elements in the sequence */ + for (i = 0; i < num_elements; i++) { + field_id = base_id + i; + data = sequence->element + i; + + switch (field_id) { + #define READ_TDX_MD(name, field) \ + case (TDX_MD_ID_##name & TDX_MD_ID_MASK): \ + field = *data; \ + break; + + READ_TDX_MD(MINOR_VERSION, sysinfo->version.minor_version); + READ_TDX_MD(MAJOR_VERSION, sysinfo->version.major_version); + READ_TDX_MD(UPDATE_VERSION, sysinfo->version.update_version); + + READ_TDX_MD(MODULE_HV, sysinfo->handoff.module_hv); + + READ_TDX_MD(TDX_FEATURES0, sysinfo->features.tdx_features0); + + READ_TDX_MD(MAX_TDMRS, sysinfo->tdmr.max_tdmrs); + READ_TDX_MD(MAX_RESERVED_PER_TDMR, sysinfo->tdmr.max_reserved_per_tdmr); + READ_TDX_MD(PAMT_4K_ENTRY_SIZE, sysinfo->tdmr.pamt_4k_entry_size); + READ_TDX_MD(PAMT_2M_ENTRY_SIZE, sysinfo->tdmr.pamt_2m_entry_size); + READ_TDX_MD(PAMT_1G_ENTRY_SIZE, sysinfo->tdmr.pamt_1g_entry_size); + + READ_TDX_MD(TDR_BASE_SIZE, sysinfo->td_ctrl.tdr_base_size); + READ_TDX_MD(TDCS_BASE_SIZE, sysinfo->td_ctrl.tdcs_base_size); + READ_TDX_MD(TDVPS_BASE_SIZE, sysinfo->td_ctrl.tdvps_base_size); + + READ_TDX_MD(ATTRIBUTES_FIXED0, sysinfo->td_conf.attributes_fixed0); + READ_TDX_MD(ATTRIBUTES_FIXED1, sysinfo->td_conf.attributes_fixed1); + READ_TDX_MD(XFAM_FIXED0, sysinfo->td_conf.xfam_fixed0); + READ_TDX_MD(XFAM_FIXED1, sysinfo->td_conf.xfam_fixed1); + READ_TDX_MD(NUM_CPUID_CONFIG, sysinfo->td_conf.num_cpuid_config); + READ_TDX_MD(MAX_VCPUS_PER_TD, sysinfo->td_conf.max_vcpus_per_td); + #undef READ_TDX_MD + + case TDX_MD_ID_CPUID_CONFIG_LEAVES & TDX_MD_ID_MASK: + for (j = 0; j < sysinfo->td_conf.num_cpuid_config; j++) + sysinfo->td_conf.cpuid_config_leaves[j] = data[j]; + /* return as all elements in this sequence are handled */ + return; + case TDX_MD_ID_CPUID_CONFIG_VALUES & TDX_MD_ID_MASK: + for (j = 0; j < sysinfo->td_conf.num_cpuid_config; j++) { + sysinfo->td_conf.cpuid_config_values[j][0] = data[2*j]; + sysinfo->td_conf.cpuid_config_values[j][1] = data[2*j+1]; + } + /* return as all elements in this sequence are handled */ + return; + default: + break; + } + } } -static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff) +static void parse_md_list(struct md_list_header *list_header, struct tdx_sys_info *sysinfo) { - int ret = 0; - u64 val; - - if (tdx_supports_runtime_update(&tdx_sysinfo) && - !(ret = read_sys_metadata_field(0x8900000100000000, &val))) - sysinfo_handoff->module_hv = val; - - return ret; + struct md_sequence *sequence; + u32 num_elements; + int i = 0; + + pr_info("header: buff_size %u, num_sequences %u\n", + list_header->list_buff_size, + list_header->num_sequences); + + sequence = (struct md_sequence *)(list_header + 1); + + while (i < list_header->num_sequences) { + num_elements = (sequence->sequence_header.last_element_in_field + 1) * + (sequence->sequence_header.last_field_in_sequence + 1); + parse_md_sequence(sequence, sysinfo); + sequence = (struct md_sequence *)(&sequence->element[num_elements]); + i++; + } } static int get_tdx_sys_info(struct tdx_sys_info *sysinfo) { + unsigned long buffer; + u64 field_id = -1; int ret = 0; - ret = ret ?: get_tdx_sys_info_version(&sysinfo->version); - ret = ret ?: get_tdx_sys_info_features(&sysinfo->features); - ret = ret ?: get_tdx_sys_info_tdmr(&sysinfo->tdmr); - ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl); - ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf); - ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff); + buffer = __get_free_page(GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + do { + ret = read_sys_metadata_field(&field_id, (void *)buffer); + if (ret) + break; + parse_md_list((void *)buffer, sysinfo); + } while (field_id != -1); + + free_page(buffer); + if (ret) + pr_info("TDX sysinfo version read all failed: %d\n", ret); return ret; } ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-22 7:54 ` Chao Gao @ 2025-10-22 10:26 ` Kiryl Shutsemau 2025-10-24 6:33 ` Chao Gao 2025-11-13 1:24 ` Chao Gao 1 sibling, 1 reply; 15+ messages in thread From: Kiryl Shutsemau @ 2025-10-22 10:26 UTC (permalink / raw) To: Chao Gao Cc: Dave Hansen, linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kai Huang, Paolo Bonzini, Dan Williams On Wed, Oct 22, 2025 at 03:54:51PM +0800, Chao Gao wrote: > On Wed, Oct 01, 2025 at 08:15:46AM -0700, Dave Hansen wrote: > >On 9/30/25 19:22, Chao Gao wrote: > >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) > >> + sysinfo_version->minor_version = val; > >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) > >> + sysinfo_version->major_version = val; > >> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) > >> + sysinfo_version->update_version = val; > > > >Heh, how long does this take in practice to get 6 bytes of data out of > >the module? > > ~8us. And the whole metadata reading process (i.e., get_tdx_sys_info()) takes > ~113us. > > >When is the point that we move over to TDH.SYS.RDALL? > > TDH.SYS.RDALL takes ~16us. > > I'm uncertain whether the saved CPU time of ~100us justifies implementing > TDH.SYS.RDALL. > > TDH.SYS.RDALL returns all metadata as a list of arrays, requiring the kernel to > parse this structure and iterate through all fields. > > One advantage of TDH.SYS.RDALL is that it eliminates the need to check field > existence before reading, since it simply dumps all available fields rather > than targeting specific ones. For example, TDH.SYS.RDALL removes the need for > the tdx_supports_ABC() check: > > in the DPAMT series: > > + if (!ret && tdx_supports_dynamic_pamt(&tdx_sysinfo) && > + !(ret = read_sys_metadata_field(0x9100000100000013, &val))) > + sysinfo_tdmr->pamt_page_bitmap_entry_bits = val; > > in the TDX Module update series: > > + if (tdx_supports_runtime_update(&tdx_sysinfo) && > + !(ret = read_sys_metadata_field(0x8900000100000000, &val))) > + sysinfo_handoff->module_hv = val; > > While iterating through the array list adds some complexity, this is a one-time > cost. Once the loop structure is in place, adding new fields only requires > inserting a "switch-case" clause within the loop. > > Please see the draft code below. If TDH.SYS.RDALL is the right direction, I can > refine the code and submit a formal patch series. I don't hate it. Seems more scalable than current approach. See some comments below. > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > index 128e6ffba736..fa9bb6d47a87 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.c > +++ b/arch/x86/virt/vmx/tdx/tdx.c > @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list) > return ret; > } > > -static int read_sys_metadata_field(u64 field_id, u64 *data) > +static int read_sys_metadata_field(u64 *field_id, void *ptr) Keeping the same name for completely different functionality? > { > struct tdx_module_args args = {}; > int ret; > > /* > - * TDH.SYS.RD -- reads one global metadata field > - * - RDX (in): the field to read > - * - R8 (out): the field data > + * TDH.SYS.RDALL -- reads all global metadata fields > + * - RDX (in): the physical address of the buffer to store > + * - R8 (in/out): the initial field ID to read (in) and > + * the next field ID to read (out). > */ > - args.rdx = field_id; > - ret = seamcall_prerr_ret(TDH_SYS_RD, &args); > + args.rdx = __pa(ptr); Maybe take virtual address (unsigned long) of the buffer as an argument to the function. And use virt_to_phys() here. This way there's no need in cast on caller side. > + args.r8 = *field_id; > + ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args); > if (ret) > return ret; > - > - *data = args.r8; > + *field_id = args.r8; > > return 0; Hm. Isn't it buggy? Caller expects to see field_id == -1 to exit loop, but you never set it in case of an error. It will result in endless loop if error happens not on the first iteration. Drop the branch and always return ret. > } > diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h > index 1965adb63f1f..44d92047073e 100644 > --- a/arch/x86/virt/vmx/tdx/tdx.h > +++ b/arch/x86/virt/vmx/tdx/tdx.h > @@ -42,6 +42,7 @@ > #define TDH_SYS_RD 34 > #define TDH_SYS_LP_INIT 35 > #define TDH_SYS_TDMR_INIT 36 > +#define TDH_SYS_RDALL 37 > #define TDH_MEM_TRACK 38 > #define TDH_PHYMEM_CACHE_WB 40 > #define TDH_PHYMEM_PAGE_WBINVD 41 > diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c > index 3fdd5cbc21d8..f4b16367ef2f 100644 > --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c > +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c > @@ -7,121 +7,177 @@ > * Include this file to other C file instead. > */ > > -static int get_tdx_sys_info_version(struct tdx_sys_info_version *sysinfo_version) > +struct md_field_id > { > - int ret = 0; > - u64 val; > - > - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) > - sysinfo_version->minor_version = val; > - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) > - sysinfo_version->major_version = val; > - if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) > - sysinfo_version->update_version = val; > - > - return ret; > -} > - > -static int get_tdx_sys_info_features(struct tdx_sys_info_features *sysinfo_features) > -{ > - int ret = 0; > - u64 val; > - > - if (!ret && !(ret = read_sys_metadata_field(0x0A00000300000008, &val))) > - sysinfo_features->tdx_features0 = val; > - > - return ret; > -} > - > -static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr) > + union { > + struct { > + u32 field_code; // Bits 31:0 > + u32 element_size_code : 2; // Bits 33:32 > + u32 last_element_in_field : 4; // Bits 37:34 > + u32 last_field_in_sequence : 9; // Bits 46:38 > + u32 reserved_1 : 3; // Bits 49:47 > + u32 inc_size : 1; // Bit 50 > + u32 write_mask_valid : 1; // Bit 51 > + u32 context_code : 3; // Bits 54:52 > + u32 reserved_2 : 1; // Bit 55 > + u32 class_code : 6; // Bits 61:56 > + u32 reserved_3 : 1; // Bit 62 > + u32 non_arch : 1; // Bit 63 > + }; > + u64 raw; > + }; > +}; > +struct md_list_header > { > - int ret = 0; > - u64 val; > - > - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000008, &val))) > - sysinfo_tdmr->max_tdmrs = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000009, &val))) > - sysinfo_tdmr->max_reserved_per_tdmr = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000010, &val))) > - sysinfo_tdmr->pamt_4k_entry_size = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000011, &val))) > - sysinfo_tdmr->pamt_2m_entry_size = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9100000100000012, &val))) > - sysinfo_tdmr->pamt_1g_entry_size = val; > - > - return ret; > -} > + u16 list_buff_size; > + u16 num_sequences; > + u32 reserved; > +}; > > -static int get_tdx_sys_info_td_ctrl(struct tdx_sys_info_td_ctrl *sysinfo_td_ctrl) > +struct md_sequence > { > - int ret = 0; > - u64 val; > + struct md_field_id sequence_header; > + u64 element[0]; > +}; > + > +#define TDX_MD_ID_MINOR_VERSION 0x0800000100000003 > +#define TDX_MD_ID_MAJOR_VERSION 0x0800000100000004 > +#define TDX_MD_ID_UPDATE_VERSION 0x0800000100000005 > +#define TDX_MD_ID_MODULE_HV 0x8900000100000000 > +#define TDX_MD_ID_TDX_FEATURES0 0x0A00000300000008 > +#define TDX_MD_ID_MAX_TDMRS 0x9100000100000008 > +#define TDX_MD_ID_MAX_RESERVED_PER_TDMR 0x9100000100000009 > +#define TDX_MD_ID_PAMT_4K_ENTRY_SIZE 0x9100000100000010 > +#define TDX_MD_ID_PAMT_2M_ENTRY_SIZE 0x9100000100000011 > +#define TDX_MD_ID_PAMT_1G_ENTRY_SIZE 0x9100000100000012 > +#define TDX_MD_ID_TDR_BASE_SIZE 0x9800000100000000 > +#define TDX_MD_ID_TDCS_BASE_SIZE 0x9800000100000100 > +#define TDX_MD_ID_TDVPS_BASE_SIZE 0x9800000100000200 > +#define TDX_MD_ID_ATTRIBUTES_FIXED0 0x1900000300000000 > +#define TDX_MD_ID_ATTRIBUTES_FIXED1 0x1900000300000001 > +#define TDX_MD_ID_XFAM_FIXED0 0x1900000300000002 > +#define TDX_MD_ID_XFAM_FIXED1 0x1900000300000003 > +#define TDX_MD_ID_NUM_CPUID_CONFIG 0x9900000100000004 > +#define TDX_MD_ID_MAX_VCPUS_PER_TD 0x9900000100000008 > +#define TDX_MD_ID_CPUID_CONFIG_LEAVES 0x9900000300000400 > +#define TDX_MD_ID_CPUID_CONFIG_VALUES 0x9900000300000500 > > - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000000, &val))) > - sysinfo_td_ctrl->tdr_base_size = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000100, &val))) > - sysinfo_td_ctrl->tdcs_base_size = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9800000100000200, &val))) > - sysinfo_td_ctrl->tdvps_base_size = val; > - > - return ret; > -} > +/* > + * Extract CLASS_CODE (bits 61:56), CONTEXT_CODE(bits 54:52), > + * FIELD_CODE(bits 23:0) from metadata IDs. Other bits in metadata IDs > + * cannot be used for comparison. > + */ > +#define TDX_MD_ID_MASK 0x3f70000000ffffff > > -static int get_tdx_sys_info_td_conf(struct tdx_sys_info_td_conf *sysinfo_td_conf) > +static void parse_md_sequence(struct md_sequence *sequence, struct tdx_sys_info *sysinfo) > { > - int ret = 0; > - u64 val; > + u32 num_elements = (sequence->sequence_header.last_element_in_field + 1) * > + (sequence->sequence_header.last_field_in_sequence + 1); > + u64 base_id = sequence->sequence_header.raw & TDX_MD_ID_MASK; > + u64 field_id; > + u64 *data; > int i, j; > > - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000000, &val))) > - sysinfo_td_conf->attributes_fixed0 = val; > - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000001, &val))) > - sysinfo_td_conf->attributes_fixed1 = val; > - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000002, &val))) > - sysinfo_td_conf->xfam_fixed0 = val; > - if (!ret && !(ret = read_sys_metadata_field(0x1900000300000003, &val))) > - sysinfo_td_conf->xfam_fixed1 = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9900000100000004, &val))) > - sysinfo_td_conf->num_cpuid_config = val; > - if (!ret && !(ret = read_sys_metadata_field(0x9900000100000008, &val))) > - sysinfo_td_conf->max_vcpus_per_td = val; > - if (sysinfo_td_conf->num_cpuid_config > ARRAY_SIZE(sysinfo_td_conf->cpuid_config_leaves)) > - return -EINVAL; > - for (i = 0; i < sysinfo_td_conf->num_cpuid_config; i++) > - if (!ret && !(ret = read_sys_metadata_field(0x9900000300000400 + i, &val))) > - sysinfo_td_conf->cpuid_config_leaves[i] = val; > - if (sysinfo_td_conf->num_cpuid_config > ARRAY_SIZE(sysinfo_td_conf->cpuid_config_values)) > - return -EINVAL; > - for (i = 0; i < sysinfo_td_conf->num_cpuid_config; i++) > - for (j = 0; j < 2; j++) > - if (!ret && !(ret = read_sys_metadata_field(0x9900000300000500 + i * 2 + j, &val))) > - sysinfo_td_conf->cpuid_config_values[i][j] = val; > - > - return ret; > + pr_info("header 0x%016llx id 0x%016llx elements_in_field %d field %d\n", sequence->sequence_header.raw, base_id, > + sequence->sequence_header.last_element_in_field + 1, > + sequence->sequence_header.last_field_in_sequence + 1); > + > + /* Iterate over all elements in the sequence */ > + for (i = 0; i < num_elements; i++) { > + field_id = base_id + i; > + data = sequence->element + i; > + > + switch (field_id) { > + #define READ_TDX_MD(name, field) \ > + case (TDX_MD_ID_##name & TDX_MD_ID_MASK): \ > + field = *data; \ > + break; > + > + READ_TDX_MD(MINOR_VERSION, sysinfo->version.minor_version); > + READ_TDX_MD(MAJOR_VERSION, sysinfo->version.major_version); > + READ_TDX_MD(UPDATE_VERSION, sysinfo->version.update_version); > + > + READ_TDX_MD(MODULE_HV, sysinfo->handoff.module_hv); > + > + READ_TDX_MD(TDX_FEATURES0, sysinfo->features.tdx_features0); > + > + READ_TDX_MD(MAX_TDMRS, sysinfo->tdmr.max_tdmrs); > + READ_TDX_MD(MAX_RESERVED_PER_TDMR, sysinfo->tdmr.max_reserved_per_tdmr); > + READ_TDX_MD(PAMT_4K_ENTRY_SIZE, sysinfo->tdmr.pamt_4k_entry_size); > + READ_TDX_MD(PAMT_2M_ENTRY_SIZE, sysinfo->tdmr.pamt_2m_entry_size); > + READ_TDX_MD(PAMT_1G_ENTRY_SIZE, sysinfo->tdmr.pamt_1g_entry_size); > + > + READ_TDX_MD(TDR_BASE_SIZE, sysinfo->td_ctrl.tdr_base_size); > + READ_TDX_MD(TDCS_BASE_SIZE, sysinfo->td_ctrl.tdcs_base_size); > + READ_TDX_MD(TDVPS_BASE_SIZE, sysinfo->td_ctrl.tdvps_base_size); > + > + READ_TDX_MD(ATTRIBUTES_FIXED0, sysinfo->td_conf.attributes_fixed0); > + READ_TDX_MD(ATTRIBUTES_FIXED1, sysinfo->td_conf.attributes_fixed1); > + READ_TDX_MD(XFAM_FIXED0, sysinfo->td_conf.xfam_fixed0); > + READ_TDX_MD(XFAM_FIXED1, sysinfo->td_conf.xfam_fixed1); > + READ_TDX_MD(NUM_CPUID_CONFIG, sysinfo->td_conf.num_cpuid_config); > + READ_TDX_MD(MAX_VCPUS_PER_TD, sysinfo->td_conf.max_vcpus_per_td); > + #undef READ_TDX_MD > + > + case TDX_MD_ID_CPUID_CONFIG_LEAVES & TDX_MD_ID_MASK: > + for (j = 0; j < sysinfo->td_conf.num_cpuid_config; j++) > + sysinfo->td_conf.cpuid_config_leaves[j] = data[j]; > + /* return as all elements in this sequence are handled */ > + return; > + case TDX_MD_ID_CPUID_CONFIG_VALUES & TDX_MD_ID_MASK: > + for (j = 0; j < sysinfo->td_conf.num_cpuid_config; j++) { > + sysinfo->td_conf.cpuid_config_values[j][0] = data[2*j]; > + sysinfo->td_conf.cpuid_config_values[j][1] = data[2*j+1]; > + } > + /* return as all elements in this sequence are handled */ > + return; > + default: > + break; > + } > + } > } > > -static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff) > +static void parse_md_list(struct md_list_header *list_header, struct tdx_sys_info *sysinfo) > { > - int ret = 0; > - u64 val; > - > - if (tdx_supports_runtime_update(&tdx_sysinfo) && > - !(ret = read_sys_metadata_field(0x8900000100000000, &val))) > - sysinfo_handoff->module_hv = val; > - > - return ret; > + struct md_sequence *sequence; > + u32 num_elements; > + int i = 0; > + > + pr_info("header: buff_size %u, num_sequences %u\n", > + list_header->list_buff_size, > + list_header->num_sequences); > + > + sequence = (struct md_sequence *)(list_header + 1); > + > + while (i < list_header->num_sequences) { > + num_elements = (sequence->sequence_header.last_element_in_field + 1) * > + (sequence->sequence_header.last_field_in_sequence + 1); > + parse_md_sequence(sequence, sysinfo); > + sequence = (struct md_sequence *)(&sequence->element[num_elements]); > + i++; > + } > } > > static int get_tdx_sys_info(struct tdx_sys_info *sysinfo) > { > + unsigned long buffer; > + u64 field_id = -1; > int ret = 0; > > - ret = ret ?: get_tdx_sys_info_version(&sysinfo->version); > - ret = ret ?: get_tdx_sys_info_features(&sysinfo->features); > - ret = ret ?: get_tdx_sys_info_tdmr(&sysinfo->tdmr); > - ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl); > - ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf); > - ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff); > + buffer = __get_free_page(GFP_KERNEL); > + if (!buffer) > + return -ENOMEM; > + > + do { > + ret = read_sys_metadata_field(&field_id, (void *)buffer); > + if (ret) > + break; > + parse_md_list((void *)buffer, sysinfo); > + } while (field_id != -1); > + > + free_page(buffer); > + if (ret) > + pr_info("TDX sysinfo version read all failed: %d\n", ret); > > return ret; > } > -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-22 10:26 ` Kiryl Shutsemau @ 2025-10-24 6:33 ` Chao Gao 2025-10-24 9:35 ` Kiryl Shutsemau 0 siblings, 1 reply; 15+ messages in thread From: Chao Gao @ 2025-10-24 6:33 UTC (permalink / raw) To: Kiryl Shutsemau Cc: Dave Hansen, linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kai Huang, Paolo Bonzini, Dan Williams >I don't hate it. Seems more scalable than current approach. > >See some comments below. > >> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c >> index 128e6ffba736..fa9bb6d47a87 100644 >> --- a/arch/x86/virt/vmx/tdx/tdx.c >> +++ b/arch/x86/virt/vmx/tdx/tdx.c >> @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list) >> return ret; >> } >> >> -static int read_sys_metadata_field(u64 field_id, u64 *data) >> +static int read_sys_metadata_field(u64 *field_id, void *ptr) > >Keeping the same name for completely different functionality? how about read_sys_metadata_fields() or read_sys_metadata_all()? > >> { >> struct tdx_module_args args = {}; >> int ret; >> >> /* >> - * TDH.SYS.RD -- reads one global metadata field >> - * - RDX (in): the field to read >> - * - R8 (out): the field data >> + * TDH.SYS.RDALL -- reads all global metadata fields >> + * - RDX (in): the physical address of the buffer to store >> + * - R8 (in/out): the initial field ID to read (in) and >> + * the next field ID to read (out). >> */ >> - args.rdx = field_id; >> - ret = seamcall_prerr_ret(TDH_SYS_RD, &args); >> + args.rdx = __pa(ptr); > >Maybe take virtual address (unsigned long) of the buffer as an argument >to the function. And use virt_to_phys() here. > >This way there's no need in cast on caller side. Sure. Will do. > >> + args.r8 = *field_id; >> + ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args); >> if (ret) >> return ret; >> - >> - *data = args.r8; >> + *field_id = args.r8; >> >> return 0; > >Hm. Isn't it buggy? > >Caller expects to see field_id == -1 to exit loop, but you never set it >in case of an error. It will result in endless loop if error happens not on >the first iteration. The caller checks the return value and bails out if there was an error. > >Drop the branch and always return ret. Setting field_id to -1 on error appears unnecessary since callers must check the return value anyway. And, even if args.r8 were copied to field_id on error, this wouldn't guarantee that field_id would be set to -1, as SEAMCALLs may encounter #GP/#UD exceptions where r8 remains unchanged. Given this, I prefer to leave field_id as an undefined value on error, and callers should not read/use it when an error occurs. What do you think? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-24 6:33 ` Chao Gao @ 2025-10-24 9:35 ` Kiryl Shutsemau 2025-10-24 10:04 ` Chao Gao 0 siblings, 1 reply; 15+ messages in thread From: Kiryl Shutsemau @ 2025-10-24 9:35 UTC (permalink / raw) To: Chao Gao Cc: Dave Hansen, linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kai Huang, Paolo Bonzini, Dan Williams On Fri, Oct 24, 2025 at 02:33:23PM +0800, Chao Gao wrote: > >I don't hate it. Seems more scalable than current approach. > > > >See some comments below. > > > >> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c > >> index 128e6ffba736..fa9bb6d47a87 100644 > >> --- a/arch/x86/virt/vmx/tdx/tdx.c > >> +++ b/arch/x86/virt/vmx/tdx/tdx.c > >> @@ -226,22 +226,23 @@ static int build_tdx_memlist(struct list_head *tmb_list) > >> return ret; > >> } > >> > >> -static int read_sys_metadata_field(u64 field_id, u64 *data) > >> +static int read_sys_metadata_field(u64 *field_id, void *ptr) > > > >Keeping the same name for completely different functionality? > > how about read_sys_metadata_fields() or read_sys_metadata_all()? Looks good. > > > > >> { > >> struct tdx_module_args args = {}; > >> int ret; > >> > >> /* > >> - * TDH.SYS.RD -- reads one global metadata field > >> - * - RDX (in): the field to read > >> - * - R8 (out): the field data > >> + * TDH.SYS.RDALL -- reads all global metadata fields > >> + * - RDX (in): the physical address of the buffer to store > >> + * - R8 (in/out): the initial field ID to read (in) and > >> + * the next field ID to read (out). > >> */ > >> - args.rdx = field_id; > >> - ret = seamcall_prerr_ret(TDH_SYS_RD, &args); > >> + args.rdx = __pa(ptr); > > > >Maybe take virtual address (unsigned long) of the buffer as an argument > >to the function. And use virt_to_phys() here. > > > >This way there's no need in cast on caller side. > > Sure. Will do. > > > > >> + args.r8 = *field_id; > >> + ret = seamcall_prerr_ret(TDH_SYS_RDALL, &args); > >> if (ret) > >> return ret; > >> - > >> - *data = args.r8; > >> + *field_id = args.r8; > >> > >> return 0; > > > >Hm. Isn't it buggy? > > > >Caller expects to see field_id == -1 to exit loop, but you never set it > >in case of an error. It will result in endless loop if error happens not on > >the first iteration. > > The caller checks the return value and bails out if there was an error. I misread it. Missed the break. > > > >Drop the branch and always return ret. > > Setting field_id to -1 on error appears unnecessary since callers must check > the return value anyway. And, even if args.r8 were copied to field_id > on error, this wouldn't guarantee that field_id would be set to -1, as > SEAMCALLs may encounter #GP/#UD exceptions where r8 remains unchanged. > > Given this, I prefer to leave field_id as an undefined value on error, and > callers should not read/use it when an error occurs. It is not undefined. TDX module sets R8 to -1 in case of error. > > What do you think? -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-24 9:35 ` Kiryl Shutsemau @ 2025-10-24 10:04 ` Chao Gao 0 siblings, 0 replies; 15+ messages in thread From: Chao Gao @ 2025-10-24 10:04 UTC (permalink / raw) To: Kiryl Shutsemau Cc: Dave Hansen, linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kai Huang, Paolo Bonzini, Dan Williams >> >Hm. Isn't it buggy? >> > >> >Caller expects to see field_id == -1 to exit loop, but you never set it >> >in case of an error. It will result in endless loop if error happens not on >> >the first iteration. >> >> The caller checks the return value and bails out if there was an error. > >I misread it. Missed the break. > >> > >> >Drop the branch and always return ret. >> >> Setting field_id to -1 on error appears unnecessary since callers must check >> the return value anyway. And, even if args.r8 were copied to field_id >> on error, this wouldn't guarantee that field_id would be set to -1, as >> SEAMCALLs may encounter #GP/#UD exceptions where r8 remains unchanged. >> >> Given this, I prefer to leave field_id as an undefined value on error, and >> callers should not read/use it when an error occurs. > >It is not undefined. TDX module sets R8 to -1 in case of error. Yes, I saw it. That's TDX module ABI. It doesn't necessarily have to be the semantics of this kernel API. So, we have two choices: 1. Follow the TDX module, i.e., set field_id to -1 on error. Then we should do: if (ret) *field_id = -1; else *field_id = args.r8; to cover #GP/#UD cases. 2. Leave field_id undefined on error, as in this patch. I don't see the value of setting field_id to -1 on error if callers are expected to check the return value. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-22 7:54 ` Chao Gao 2025-10-22 10:26 ` Kiryl Shutsemau @ 2025-11-13 1:24 ` Chao Gao 1 sibling, 0 replies; 15+ messages in thread From: Chao Gao @ 2025-11-13 1:24 UTC (permalink / raw) To: Dave Hansen Cc: linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kirill A. Shutemov, Kai Huang, Paolo Bonzini, Dan Williams On Wed, Oct 22, 2025 at 03:55:01PM +0800, Chao Gao wrote: >On Wed, Oct 01, 2025 at 08:15:46AM -0700, Dave Hansen wrote: >>On 9/30/25 19:22, Chao Gao wrote: >>> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000003, &val))) >>> + sysinfo_version->minor_version = val; >>> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000004, &val))) >>> + sysinfo_version->major_version = val; >>> + if (!ret && !(ret = read_sys_metadata_field(0x0800000100000005, &val))) >>> + sysinfo_version->update_version = val; >> >>Heh, how long does this take in practice to get 6 bytes of data out of >>the module? > >~8us. And the whole metadata reading process (i.e., get_tdx_sys_info()) takes >~113us. > >>When is the point that we move over to TDH.SYS.RDALL? > >TDH.SYS.RDALL takes ~16us. > >I'm uncertain whether the saved CPU time of ~100us justifies implementing >TDH.SYS.RDALL. I chatted with Dave and Yilun offline. We think that saving 100us for this one-off operation isn't worth the code churn. For the record, the idea of moving over to TDH.SYS.RDALL is: 1. Call TDH.SYS.RDALL to dump all metadata into a data structure 2. Add a tdx_get_one() helper to look up the data structure and get the metadata for a given field ID 3. Replace read_sys_metadata_field() with tdx_get_one() ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version 2025-10-01 2:22 ` [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version Chao Gao 2025-10-01 15:15 ` Dave Hansen @ 2025-10-24 14:20 ` Vishal Annapurve 1 sibling, 0 replies; 15+ messages in thread From: Vishal Annapurve @ 2025-10-24 14:20 UTC (permalink / raw) To: Chao Gao Cc: linux-coco, linux-kernel, x86, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Kirill A. Shutemov, Kai Huang, Paolo Bonzini, Dan Williams On Tue, Sep 30, 2025 at 7:24 PM Chao Gao <chao.gao@intel.com> wrote: > > Each TDX module is associated with a version in the x.y.z format, where x > represents the major version, y the minor version, and z the update > version. Knowing the running TDX module version is valuable for bug > reporting and debugging. > > Retrieve the TDX module version using the existing metadata reading > interface, in preparation for exposing it to userspace via sysfs. > In addition to exposing the TDX module version via sysfs, it would be helpful to have the TDX module version captured in dmesg logs/console. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/2] coco/tdx-host: Expose TDX module version 2025-10-01 2:22 [PATCH 0/2] Expose TDX Module version Chao Gao 2025-10-01 2:22 ` [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version Chao Gao @ 2025-10-01 2:22 ` Chao Gao 2025-10-01 4:12 ` Huang, Kai 2025-10-01 11:30 ` Kiryl Shutsemau 1 sibling, 2 replies; 15+ messages in thread From: Chao Gao @ 2025-10-01 2:22 UTC (permalink / raw) To: linux-coco, linux-kernel, x86 Cc: Chao Gao, Kirill A. Shutemov, Dave Hansen, Dan Williams, Xu Yilun Currently these is no way to know the TDX module version from the userspace. such information is helpful for bug reporting or debugging. With the tdx-host device in place, expose the TDX module version as a device attribute via sysfs. Signed-off-by: Chao Gao <chao.gao@intel.com> --- .../ABI/testing/sysfs-devices-faux-tdx-host | 6 +++++ MAINTAINERS | 1 + drivers/virt/coco/tdx-host/tdx-host.c | 25 ++++++++++++++++++- 3 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 Documentation/ABI/testing/sysfs-devices-faux-tdx-host diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host new file mode 100644 index 000000000000..18d4a4a71b80 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host @@ -0,0 +1,6 @@ +What: /sys/devices/faux/tdx_host/version +Contact: linux-coco@lists.linux.dev +Description: (RO) Report the version of the loaded TDX module. The TDX module + version is formatted as x.y.z, where "x" is the major version, + "y" is the minor version and "z" is the update version. Versions + are used for bug reporting, TD-Preserving updates and etc. diff --git a/MAINTAINERS b/MAINTAINERS index c1ad1294560c..7560dcf8a53d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -27333,6 +27333,7 @@ L: x86@kernel.org L: linux-coco@lists.linux.dev S: Supported T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx +F: Documentation/ABI/testing/sysfs-devices-faux-tdx-host F: Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest F: arch/x86/boot/compressed/tdx* F: arch/x86/coco/tdx/ diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c index 49c205913ef6..968a19f4e01a 100644 --- a/drivers/virt/coco/tdx-host/tdx-host.c +++ b/drivers/virt/coco/tdx-host/tdx-host.c @@ -22,6 +22,29 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_host_ids); static struct faux_device *fdev; +static ssize_t version_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo(); + const struct tdx_sys_info_version *ver; + + if (!tdx_sysinfo) + return -ENXIO; + + ver = &tdx_sysinfo->version; + + return sysfs_emit(buf, "%u.%u.%02u\n", ver->major_version, + ver->minor_version, + ver->update_version); +} +static DEVICE_ATTR_RO(version); + +static struct attribute *tdx_host_attrs[] = { + &dev_attr_version.attr, + NULL, +}; +ATTRIBUTE_GROUPS(tdx_host); + static int __init tdx_host_init(void) { int r; @@ -34,7 +57,7 @@ static int __init tdx_host_init(void) if (r) return r; - fdev = faux_device_create(KBUILD_MODNAME, NULL, NULL); + fdev = faux_device_create_with_groups(KBUILD_MODNAME, NULL, NULL, tdx_host_groups); if (!fdev) return -ENODEV; -- 2.47.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] coco/tdx-host: Expose TDX module version 2025-10-01 2:22 ` [PATCH 2/2] coco/tdx-host: Expose " Chao Gao @ 2025-10-01 4:12 ` Huang, Kai 2025-10-01 11:27 ` Kiryl Shutsemau 2025-10-01 11:30 ` Kiryl Shutsemau 1 sibling, 1 reply; 15+ messages in thread From: Huang, Kai @ 2025-10-01 4:12 UTC (permalink / raw) To: linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org Cc: Williams, Dan J, kas@kernel.org, yilun.xu@linux.intel.com, dave.hansen@linux.intel.com On Tue, 2025-09-30 at 19:22 -0700, Chao Gao wrote: > Currently these is no way to know the TDX module version from the ^ there is > userspace. such information is helpful for bug reporting or ^ Such > debugging. > > [...] > +static ssize_t version_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo(); > + const struct tdx_sys_info_version *ver; > + > + if (!tdx_sysinfo) > + return -ENXIO; > + > + ver = &tdx_sysinfo->version; > + > + return sysfs_emit(buf, "%u.%u.%02u\n", ver->major_version, > + ver->minor_version, > + ver->update_version); Nit: not sure whether the "%u.%u.%02u" needs a comment, e.g., why the %02u is used for the update_version? ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] coco/tdx-host: Expose TDX module version 2025-10-01 4:12 ` Huang, Kai @ 2025-10-01 11:27 ` Kiryl Shutsemau 2025-10-01 21:41 ` Huang, Kai 0 siblings, 1 reply; 15+ messages in thread From: Kiryl Shutsemau @ 2025-10-01 11:27 UTC (permalink / raw) To: Huang, Kai Cc: linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org, Williams, Dan J, yilun.xu@linux.intel.com, dave.hansen@linux.intel.com On Wed, Oct 01, 2025 at 04:12:10AM +0000, Huang, Kai wrote: > On Tue, 2025-09-30 at 19:22 -0700, Chao Gao wrote: > > Currently these is no way to know the TDX module version from the > ^ > there is > > > userspace. such information is helpful for bug reporting or > ^ > Such > > > debugging. > > > > > > [...] > > > +static ssize_t version_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo(); > > + const struct tdx_sys_info_version *ver; > > + > > + if (!tdx_sysinfo) > > + return -ENXIO; > > + > > + ver = &tdx_sysinfo->version; > > + > > + return sysfs_emit(buf, "%u.%u.%02u\n", ver->major_version, > > + ver->minor_version, > > + ver->update_version); > > Nit: not sure whether the "%u.%u.%02u" needs a comment, e.g., why the %02u > is used for the update_version? That's how TDX module version formatted: https://github.com/intel/tdx-module/tags I think it is good idea to match it. -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] coco/tdx-host: Expose TDX module version 2025-10-01 11:27 ` Kiryl Shutsemau @ 2025-10-01 21:41 ` Huang, Kai 0 siblings, 0 replies; 15+ messages in thread From: Huang, Kai @ 2025-10-01 21:41 UTC (permalink / raw) To: kas@kernel.org Cc: Williams, Dan J, linux-coco@lists.linux.dev, yilun.xu@linux.intel.com, linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org, dave.hansen@linux.intel.com On Wed, 2025-10-01 at 12:27 +0100, Kiryl Shutsemau wrote: > On Wed, Oct 01, 2025 at 04:12:10AM +0000, Huang, Kai wrote: > > On Tue, 2025-09-30 at 19:22 -0700, Chao Gao wrote: > > > Currently these is no way to know the TDX module version from the > > ^ > > there is > > > > > userspace. such information is helpful for bug reporting or > > ^ > > Such > > > > > debugging. > > > > > > > > > > [...] > > > > > +static ssize_t version_show(struct device *dev, struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo(); > > > + const struct tdx_sys_info_version *ver; > > > + > > > + if (!tdx_sysinfo) > > > + return -ENXIO; > > > + > > > + ver = &tdx_sysinfo->version; > > > + > > > + return sysfs_emit(buf, "%u.%u.%02u\n", ver->major_version, > > > + ver->minor_version, > > > + ver->update_version); > > > > Nit: not sure whether the "%u.%u.%02u" needs a comment, e.g., why the %02u > > is used for the update_version? > > That's how TDX module version formatted: > > https://github.com/intel/tdx-module/tags > > I think it is good idea to match it. Yeah agreed! ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] coco/tdx-host: Expose TDX module version 2025-10-01 2:22 ` [PATCH 2/2] coco/tdx-host: Expose " Chao Gao 2025-10-01 4:12 ` Huang, Kai @ 2025-10-01 11:30 ` Kiryl Shutsemau 1 sibling, 0 replies; 15+ messages in thread From: Kiryl Shutsemau @ 2025-10-01 11:30 UTC (permalink / raw) To: Chao Gao; +Cc: linux-coco, linux-kernel, x86, Dave Hansen, Dan Williams, Xu Yilun On Tue, Sep 30, 2025 at 07:22:45PM -0700, Chao Gao wrote: > diff --git a/MAINTAINERS b/MAINTAINERS > index c1ad1294560c..7560dcf8a53d 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -27333,6 +27333,7 @@ L: x86@kernel.org > L: linux-coco@lists.linux.dev > S: Supported > T: git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git x86/tdx > +F: Documentation/ABI/testing/sysfs-devices-faux-tdx-host > F: Documentation/ABI/testing/sysfs-devices-virtual-misc-tdx_guest > F: arch/x86/boot/compressed/tdx* > F: arch/x86/coco/tdx/ The entry was updated recently: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/tree/MAINTAINERS?h=x86/tdx#n27325 I think you need to rebase onto current tip tree. -- Kiryl Shutsemau / Kirill A. Shutemov ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2025-11-13 1:25 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-01 2:22 [PATCH 0/2] Expose TDX Module version Chao Gao 2025-10-01 2:22 ` [PATCH 1/2] x86/virt/tdx: Retrieve TDX module version Chao Gao 2025-10-01 15:15 ` Dave Hansen 2025-10-22 7:54 ` Chao Gao 2025-10-22 10:26 ` Kiryl Shutsemau 2025-10-24 6:33 ` Chao Gao 2025-10-24 9:35 ` Kiryl Shutsemau 2025-10-24 10:04 ` Chao Gao 2025-11-13 1:24 ` Chao Gao 2025-10-24 14:20 ` Vishal Annapurve 2025-10-01 2:22 ` [PATCH 2/2] coco/tdx-host: Expose " Chao Gao 2025-10-01 4:12 ` Huang, Kai 2025-10-01 11:27 ` Kiryl Shutsemau 2025-10-01 21:41 ` Huang, Kai 2025-10-01 11:30 ` Kiryl Shutsemau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).