2023-10-27 04:55:22

by Yi Sun

[permalink] [raw]
Subject: [PATCH v7] x86/tdx: Dump TDX Version During TD Bootup

Different versions of TDX have significant differences, as stated in the
"IntelĀ® TDX Module Incompatibilities between v1.0 and v1.5" reference.

It would be useful for TD users to be aware of the vendor and version of
the current TDX in use. Users could expect different results when checking
CPIUD or reading MSR in the user space, depending on the TDX version.
Additionally, refer to the TDX version when reporting issues.

Reviewed-by: Kirill A. Shutemov <[email protected]>
Reviewed-by: Nikolay Borisov <[email protected]>
Co-developed-by: Dongcheng Yan <[email protected]>
Signed-off-by: Dongcheng Yan <[email protected]>
Signed-off-by: Yi Sun <[email protected]>

---
V6 -> V7:
- Reduce unnecessary variable initializations and improve the dump
information. (Kuppuswamy Sathyanarayanan)

V5 -> V6:
- Remove random warnings at every step. Print the error in common
code. (Dave Hansen)

- Remove useless assignment and cast. Re-zeroed the input args
between tdcalls. Refine the comments. (Dave Hansen)

V4 -> V5:
- Print the version info inside the function detect_tdx_version, but
not tdx_early_init(). Remove the structure tdg_sys_info, but have 3
local variables instead. (Huang, Kai)

V3 -> V4:
- Rebase the patch on top of the latest tip tree. (Huang, Kai)
- Change the return value of function tdg_get_sysinfo as void, and
zero out tdg_sys_info when error occurs. (Kuppuswamy Sathyanarayanan)

V2 -> V3:
- Move the allocation of struct tdg_sys_info on stack inside
tdx_early_init() and pass down to tdg_get_sysinfo() to fill.
(Kirill Shutemov)

V1 -> V2:
- Move the defination of field IDs and the struct tdg_sys_info to tdx.c.
(Kuppuswamy Sathyanarayanan)


diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 2f27ae1e2c6b..66e020e3bb48 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -37,6 +37,15 @@

#define TDREPORT_SUBTYPE_0 0

+/*
+ * TDX metadata base field id, used by TDCALL TDG.SYS.RD
+ * See TDX ABI Spec Global Metadata Fields
+ */
+#define TDX_SYS_VENDOR_ID_FID 0x0800000200000000ULL
+#define TDX_SYS_MINOR_FID 0x0800000100000003ULL
+#define TDX_SYS_MAJOR_FID 0x0800000100000004ULL
+#define TDX_VENDOR_INTEL 0x8086
+
/* Called from __tdx_hypercall() for unrecoverable failure */
noinstr void __noreturn __tdx_hypercall_failed(void)
{
@@ -800,6 +809,55 @@ static bool tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
return true;
}

+/*
+ * Detect TDX Module version info from TDG.SYS.RD TDCALL
+ */
+static void detect_tdx_version(void)
+{
+ struct tdx_module_args args = {};
+ u16 major_version, minor_version;
+ u32 vendor_id;
+ u64 ret;
+
+ args.rdx = TDX_SYS_VENDOR_ID_FID;
+ ret = __tdcall_ret(TDG_SYS_RD, &args);
+ if (ret)
+ goto err_out;
+
+ vendor_id = args.r8;
+
+ memset(&args, 0, sizeof(args));
+ args.rdx = TDX_SYS_MAJOR_FID;
+ ret = __tdcall_ret(TDG_SYS_RD, &args);
+ if (ret)
+ goto err_out;
+
+ major_version = args.r8;
+
+ memset(&args, 0, sizeof(args));
+ args.rdx = TDX_SYS_MINOR_FID;
+ ret = __tdcall_ret(TDG_SYS_RD, &args);
+ if (ret)
+ goto err_out;
+
+ minor_version = args.r8;
+
+ pr_info("Guest detected. version:%u.%u VendorID:%x\n",
+ major_version, minor_version, vendor_id);
+
+ return;
+
+err_out:
+ if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
+ pr_info("TDG.SYS.RD not available\n");
+ else
+ pr_info("TDG.SYS.RD unknown error (%llu), reading field %llu\n",
+ ret, args.rdx);
+
+ pr_info("Assuming TDX version:1.x (x<5) VendorID:%x\n",
+ TDX_VENDOR_INTEL);
+}
+
void __init tdx_early_init(void)
{
struct tdx_module_args args = {
@@ -870,5 +928,5 @@ void __init tdx_early_init(void)
*/
x86_cpuinit.parallel_bringup = false;

- pr_info("Guest detected\n");
+ detect_tdx_version();
}
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index f74695dea217..10b6c61e835e 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -17,6 +17,8 @@
#define TDG_MR_REPORT 4
#define TDG_MEM_PAGE_ACCEPT 6
#define TDG_VM_WR 8
+/* The TDCALL TDG.SYS.RD originates from TDX version 1.5 */
+#define TDG_SYS_RD 11

/* TDCS fields. To be used by TDG.VM.WR and TDG.VM.RD module calls */
#define TDCS_NOTIFY_ENABLES 0x9100000000000010
--
2.34.1


2023-10-27 08:57:24

by Kai Huang

[permalink] [raw]
Subject: Re: [PATCH v7] x86/tdx: Dump TDX Version During TD Bootup


> +/*
> + * Detect TDX Module version info from TDG.SYS.RD TDCALL
> + */
> +static void detect_tdx_version(void)
> +{
> + struct tdx_module_args args = {};
> + u16 major_version, minor_version;
> + u32 vendor_id;
> + u64 ret;
> +
> + args.rdx = TDX_SYS_VENDOR_ID_FID;
> + ret = __tdcall_ret(TDG_SYS_RD, &args);
> + if (ret)
> + goto err_out;

I am not sure we need to detect vendor ID? I believe TDX must be from Intel?

Also, I think either "err" or "out" is sufficient?

> +
> + vendor_id = args.r8;
> +
> + memset(&args, 0, sizeof(args));
> + args.rdx = TDX_SYS_MAJOR_FID;
> + ret = __tdcall_ret(TDG_SYS_RD, &args);
> + if (ret)
> + goto err_out;
> +
> + major_version = args.r8;
> +
> + memset(&args, 0, sizeof(args));
> + args.rdx = TDX_SYS_MINOR_FID;
> + ret = __tdcall_ret(TDG_SYS_RD, &args);
> + if (ret)
> + goto err_out;
> +
> + minor_version = args.r8;
> +
> + pr_info("Guest detected. version:%u.%u VendorID:%x\n",
> + major_version, minor_version, vendor_id);
> +
> + return;
> +
> +err_out:
> + if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> + pr_info("TDG.SYS.RD not available\n");
> + else
> + pr_info("TDG.SYS.RD unknown error (%llu), reading field %llu\n",
> + ret, args.rdx);
> +
> + pr_info("Assuming TDX version:1.x (x<5) VendorID:%x\n",
> + TDX_VENDOR_INTEL);

You removed "Guest detected" in tdx_early_init(), but didn't add it here, so
unfortunately the "Guest detected" will get lost here.

Also, I suppose for the case TDG.SYS.RD isn't supported there's still some value
to print "Assume module version: 1.x (x<5)", but when TDG.SYS.RD failed
unexpectedly I don't think we can make that assumption, and printing "assuming
..." could give user misinformation which is worse than not printing IMHO.

So, I think you can keep the printing of "Guest detected" in tdx_early_init(),
and somehow refine the code to print something like below:

For module that does TDG.SYS.RD successfully:

tdx: Guest detected
tdx: Module version: %u.%u

For module that doesn't support TDG.SYS.RD:

tdx: Guest detected
tdx: Failed to get module version: TDG.SYS.RD unsupported. Assume module
version: 1.x (x < 5).

For module that TDG.SYS.RD failed unexpectedly:

tdx: Guest detected
tdx: Failed to get module version: TDG.SYS.RD failed to read 0x%llu: 0x%llu

2023-10-27 15:26:04

by Dave Hansen

[permalink] [raw]
Subject: Re: [PATCH v7] x86/tdx: Dump TDX Version During TD Bootup

On 10/26/23 21:52, Yi Sun wrote:
> Different versions of TDX have significant differences, as stated in the
> "IntelĀ® TDX Module Incompatibilities between v1.0 and v1.5" reference.
> It would be useful for TD users to be aware of the vendor and version of
> the current TDX in use. Users could expect different results when
> checking CPIUD or reading MSR in the user space, depending on the TDX
> version. Additionally, refer to the TDX version when reporting issues.

Hi Folks,

I tried to get some of this across in my last message. I think I was
being too subtle. Please stop sending this patch.

dmesg is not the right place for this information. dmesg is a ring
buffer. All data there goes away eventually.

If users *need* this -- and your changelog seems to be making the case
that they do -- it can't be from a source that goes away.

On another note, there used to be a "TDX versions in sysfs" patch
somewhere. Maybe it was just on the host side, I don't remember. But,
either way, could you please go have a chat with Kai and see what he's
doing around TDX module version exporting? *Whatever* gets done, it
needs to be _consistent_ across host and guest, if not have the exact
same user<->kernel ABI.

Then, someone is going to want to update the TDX module, just like
firmware. When that happens, this information in dmesg (if even still
there) is going to be outright wrong. So those folks that *need* it are
going to be getting stale info.