TDX KVM selftests

From: Sean Christopherson <[email protected]>

Add VM_TYPE() and __VM_SHAPE() macros to create a vm_shape structure given a type (and mode), and use the macros to define VM_SHAPE_{SEV,SEV_ES,SNP} shapes for x86's SEV family of VM shapes. Providing common infrastructure will avoid having to copy+paste vm_sev_create_with_one_vcpu() for TDX.

Use the new SEV+ shapes and drop vm_sev_create_with_one_vcpu().

Opportunistically move the existing VM_SHAPE() (now __VM_SHAPE()) macro below the definitions of VM_MODE_DEFAULT so that all of the SHAPE/TYPE macros are bundled together.

No functional change intended.

Reviewed-by Binbin Wu <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

---
 tools/testing/selftests/kvm/include/kvm_util.h     | 27 ++++++++-------
 .../testing/selftests/kvm/include/x86/processor.h  |  4 +++
 tools/testing/selftests/kvm/include/x86/sev.h      |  2 --
 tools/testing/selftests/kvm/lib/x86/sev.c          | 16 ---------
 tools/testing/selftests/kvm/x86/sev_smoke_test.c   | 40 +++++++++++-----------
 5 files changed, 39 insertions(+), 50 deletions(-)

From: Sean Christopherson <[email protected]>

Add VM_TYPE() and __VM_SHAPE() macros to create a vm_shape structure given a type (and mode), and use the macros to define VM_SHAPE_{SEV,SEV_ES,SNP} shapes for x86's SEV family of VM shapes. Providing common infrastructure will avoid having to copy+paste vm_sev_create_with_one_vcpu() for TDX.

Use the new SEV+ shapes and drop vm_sev_create_with_one_vcpu().

Opportunistically move the existing VM_SHAPE() (now __VM_SHAPE()) macro below the definitions of VM_MODE_DEFAULT so that all of the SHAPE/TYPE macros are bundled together.

No functional change intended.

Reviewed-by Binbin Wu <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

Signed-off-by Sean Christopherson <[email protected]>

[...snip...]

From: Isaku Yamahata <[email protected]>

Initialize the TDX S-bit and the GPA tag mask in kvm_init_vm_address_properties() for TDX VMs, similar to how the C-bit is initialized for SEV VMs.

The TDX S-bit is used to distinguish between shared and private guest physical addresses. Its position is determined by the guest physical address width, which is either 48 or 52 bits for current TDX implementations.

Reviewed-by Binbin Wu <[email protected]>

Co-developed-by Adrian Hunter <[email protected]>

Signed-off-by Adrian Hunter <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Co-developed-by Sagi Shahar <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

---

Quoted 6 lines · Lisa Wang

From: Isaku Yamahata <[email protected]>

Initialize the TDX S-bit and the GPA tag mask in kvm_init_vm_address_properties() for TDX VMs, similar to how the C-bit is initialized for SEV VMs.

Quoted 4 lines

The TDX S-bit is used to distinguish between shared and private guest physical addresses. Its position is determined by the guest physical address width, which is either 48 or 52 bits for current TDX implementations.

Since S-bit=1 means shared and C-bit=1 means private, we can't have a single bit. I'd justify why a second field is needed.

For "untagging" GPAs we could
have a single field, but there are other usages?

Quoted 11 lines

Reviewed-by Binbin Wu <[email protected]>

Co-developed-by Adrian Hunter <[email protected]>

Signed-off-by Adrian Hunter <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Co-developed-by Sagi Shahar <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

---

Nit: these are not ordered correctly. I think KVM prefers the order in:
Documentation/process/maintainer-tip.rst

But I think at least the RBs can be grouped together. Also... can't really point fingers here, but that is a fair amount of of patch history.

From: Sagi Shahar <[email protected]>

Add tdx_init_vm() to handle the mandatory VM-level initialization sequence required for Intel TDX.

For TDX, the guest's CPUID configuration must be "sealed" during KVM_TDX_INIT_VM before any vCPUs are created. This is necessary because the TDX hardware directly virtualizes CPUID and includes the configuration in the guest's initial security measurement.

The helper calculates the required CPUID values by filtering the host- supported bits (kvm_get_supported_cpuid) against the "directly configurable" bits reported by KVM_TDX_CAPABILITIES, ensuring compliance with the strict requirements of the TDH.MNG.INIT SEAMCALL.

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +4 -0

+({									\
+	u64 r;								\
+									\
+	union {								\

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +11 -0

+
+void tdx_init_vm(struct kvm_vm *vm, u64 attributes)
+{
+	struct kvm_tdx_init_vm *init_vm;
+	const struct kvm_cpuid2 *tmp;
+	struct kvm_cpuid2 *cpuid;
+
+	tmp = kvm_get_supported_cpuid();
+
+	cpuid = allocate_kvm_cpuid2(tmp->nent);
+	memcpy(cpuid, tmp, kvm_cpuid2_size(tmp->nent));

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +5 -0

+
+	tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);
+
+	free(init_vm);
+}

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 115

Quoted 1 line

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +1 -0

+	init_vm->attributes = attributes;

Besides CPUID, it only allows attributes to be configure but leave XFAM as 0. I think the changelog needs to explain why we need to configure attributes.

The rest of the patch looks good to me.

Quoted 7 lines · Xiaoyao Li

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 115

Quoted 1 line

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +1 -0

+	init_vm->attributes = attributes;

Besides CPUID, it only allows attributes to be configure but leave XFAM as 0. I think the changelog needs to explain why we need to configure attributes.

The rest of the patch looks good to me.

How about directly setting xfam to zero for clarity?

Quoted 2 lines

+ */
+#define __tdx_vm_ioctl(vm, cmd, _flags, arg) \

sev uses the name __vm_sev_ioctl, I think we need to keep them consistent.

Quoted 4 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +4 -0

+({									\
+	u64 r;								\
+									\
+	union {								\

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 32

Quoted 10 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +10 -0

+		struct kvm_tdx_cmd c;					\
+		unsigned long raw;					\
+	} tdx_cmd = { .c = {						\
+		.id = (cmd),						\
+		.flags = (u32)(_flags),					\
+		.data = (u64)(arg),					\
+	} };								\
+									\
+	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw);	\
+	r ?: tdx_cmd.c.hw_error;					\

I know it takes the same handling from __vm_sev_ioctl(). But I think the handling for hw_error is not correct, at least for TDX (I didn't check for SEV).

the hw_error is the additional info, to tell the SEAMCALL return code, when the IOCTL fails. KVM requires hw_error to be in the input, and KVM puts the SEAMCALL return code into hw_error when the IOCTL fails due to SEAMCALL failure. That means, when r == 0, the hw_error is always 0.

I think we need to provide hw_error along with r to the caller so that caller can print them together.

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 43

Quoted 11 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +11 -0

+})
+
+#define tdx_vm_ioctl(vm, cmd, flags, arg)				\
+({									\
+	u64 ret = __tdx_vm_ioctl(vm, cmd, flags, arg);			\
+									\
+	if (ret) {							\
+		TEST_ASSERT(!ret,					\
+			    "%s failed, rc: 0x%llx errno: %i (%s)",	\
+			    #cmd, (unsigned long long)ret,		\
+			    errno, strerror(errno));			\

The if() looks silly. Why add it? And why change it from __TEST_ASSERT_VM_VCPU_IOCTL() in the v13?

Considering the suggestion of hw_error above, I think we need to introduce the TEST_ASSERT_TDX_VM_VCPU_IOCTL() which accepts additional hw_error?

<snip>

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 6

Quoted 12 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +6 -0

new file mode 100644
index 000000000000..e1ffb67a106c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "processor.h"
+#include "tdx/tdx_util.h"
+
+static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)

make it const, is better.

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 115

Quoted 1 line

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +1 -0

+	init_vm->attributes = attributes;

Besides CPUID, it only allows attributes to be configure but leave XFAM as
0. I think the changelog needs to explain why we need to configure attributes.

The rest of the patch looks good to me.

Quoted 4 lines · Xiaoyao Li

Quoted 2 lines

+ */
+#define __tdx_vm_ioctl(vm, cmd, _flags, arg) \

sev uses the name __vm_sev_ioctl, I think we need to keep them consistent.

While __vm_tdx_ioctl matches SEV, the TDX selftest follows a tdx_<scope>_* naming convention (1. TDX prefix, 2. Scope: VM or vCPU). I named it __tdx_vm_ioctl to keep the TDX codebase internally consistent.[1]

Do you think we should align with SEV's naming convention instead of sticking with the internal TDX pattern?

[1]: https://lore.kernel.org/kvm/[email protected]/

Quoted 19 lines

Quoted 4 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +4 -0

+({									\
+	u64 r;								\
+									\
+	union {								\

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 32

Quoted 10 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +10 -0

+		struct kvm_tdx_cmd c;					\
+		unsigned long raw;					\
+	} tdx_cmd = { .c = {						\
+		.id = (cmd),						\
+		.flags = (u32)(_flags),					\
+		.data = (u64)(arg),					\
+	} };								\
+									\
+	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw);	\
+	r ?: tdx_cmd.c.hw_error;					\

I know it takes the same handling from __vm_sev_ioctl(). But I think the handling for hw_error is not correct, at least for TDX (I didn't check for SEV).

Quoted 7 lines

the hw_error is the additional info, to tell the SEAMCALL return code, when the IOCTL fails. KVM requires hw_error to be in the input, and KVM puts the SEAMCALL return code into hw_error when the IOCTL fails due to SEAMCALL failure. That means, when r == 0, the hw_error is always 0.

I think we need to provide hw_error along with r to the caller so that caller can print them together.

I think the value of r is not important, because the ioctl failure is already captured in errno.

We only need to fix the return values for SEV and TDX and have TEST_ASSERT_* print formatted error logs with errno and hw_error.

- r ?: {tdx, sev}_cmd.c.hw_error;
+ r ? {tdx, sev}_cmd.c.hw_error : 0;

Quoted 15 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 43

Quoted 11 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +11 -0

+})
+
+#define tdx_vm_ioctl(vm, cmd, flags, arg)				\
+({									\
+	u64 ret = __tdx_vm_ioctl(vm, cmd, flags, arg);			\
+									\
+	if (ret) {							\
+		TEST_ASSERT(!ret,					\
+			    "%s failed, rc: 0x%llx errno: %i (%s)",	\
+			    #cmd, (unsigned long long)ret,		\
+			    errno, strerror(errno));			\

The if() looks silly. Why add it? And why change it from __TEST_ASSERT_VM_VCPU_IOCTL() in the v13?

Quoted 2 lines

Considering the suggestion of hw_error above, I think we need to introduce the TEST_ASSERT_TDX_VM_VCPU_IOCTL() which accepts additional hw_error?

The reason we could not use __TEST_ASSERT_VM_VCPU_IOCTL() directly[2] is because it formats ther return value as %i (32-bit), whereas __tdx_vm_ioctl might return a u64 hardware error code.

I agree with your suggestion to introduce a new TEST_ASSERT_TDX_VM_VCPU_IOCTL() macro to print out u64 hardware error code properly.

[2]: https://lore.kernel.org/all/[email protected]/

Quoted 15 lines

<snip>

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 6

Quoted 12 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +6 -0

new file mode 100644
index 000000000000..e1ffb67a106c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "processor.h"
+#include "tdx/tdx_util.h"
+
+static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)

make it const, is better.

Thanks, noted.

Quoted 5 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c line 115

Quoted 1 line

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +1 -0

+	init_vm->attributes = attributes;

Besides CPUID, it only allows attributes to be configure but leave XFAM as
0. I think the changelog needs to explain why we need to configure attributes.

Thanks, noted.

Quoted 1 line

The rest of the patch looks good to me.

Quoted 6 lines · Lisa Wang

Quoted 4 lines · Xiaoyao Li

Quoted 2 lines

+ */
+#define __tdx_vm_ioctl(vm, cmd, _flags, arg) \

sev uses the name __vm_sev_ioctl, I think we need to keep them consistent.

Quoted 9 lines

While __vm_tdx_ioctl matches SEV, the TDX selftest follows a tdx_<scope>_* naming convention (1. TDX prefix, 2. Scope: VM or vCPU). I named it __tdx_vm_ioctl to keep the TDX codebase internally consistent.[1]

Do you think we should align with SEV's naming convention instead of sticking with the internal TDX pattern?

[1]: https://lore.kernel.org/kvm/[email protected]/

Given that __vm_sev_ioctl and __tdx_vm_ioctl aren't likely to appear next to each other, I think it's better to have the tdx prefix earlier to keep the TDX code consistent. To move things along, I think we could continue as-is and not swap to align with sev.

(The sev functions actually look kind of inconsistent in sev.h, but that's a discussion for another series.)

Quoted 30 lines

Quoted 19 lines

Quoted 4 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +4 -0

+({									\
+	u64 r;								\
+									\
+	union {								\

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 32

Quoted 10 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +10 -0

+		struct kvm_tdx_cmd c;					\
+		unsigned long raw;					\
+	} tdx_cmd = { .c = {						\
+		.id = (cmd),						\
+		.flags = (u32)(_flags),					\
+		.data = (u64)(arg),					\
+	} };								\
+									\
+	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw);	\
+	r ?: tdx_cmd.c.hw_error;					\

I know it takes the same handling from __vm_sev_ioctl(). But I think the handling for hw_error is not correct, at least for TDX (I didn't check for SEV).

Quoted 7 lines

the hw_error is the additional info, to tell the SEAMCALL return code, when the IOCTL fails. KVM requires hw_error to be in the input, and KVM puts the SEAMCALL return code into hw_error when the IOCTL fails due to SEAMCALL failure. That means, when r == 0, the hw_error is always 0.

I think we need to provide hw_error along with r to the caller so that caller can print them together.

I think the value of r is not important, because the ioctl failure is already captured in errno.

Quoted 6 lines

We only need to fix the return values for SEV and TDX and have TEST_ASSERT_* print formatted error logs with errno and hw_error.

- r ?: {tdx, sev}_cmd.c.hw_error;
+ r ? {tdx, sev}_cmd.c.hw_error : 0;

I didn't look in detail, perhaps Lisa could look into these:

+ What are the possible values of r? Is it always going to be 1 on error?
+ Is r always positive or negative?
+ Is tdx_cmd.c.hw_error always positive or negative? It's probably not one of the standard Linux errors, right?

Perhaps squashing hw_error together with a retval conflates the two.

In the lower-level __tdx_vm_ioctl() we could pass a hw_error and have the macro set hw_error? That will allow the caller to print both.

Quoted 15 lines

Quoted 15 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h static inline bool is_tdx_vm(struct kvm_vm *vm) · line 43

Quoted 11 lines

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +11 -0

+})
+
+#define tdx_vm_ioctl(vm, cmd, flags, arg)				\
+({									\
+	u64 ret = __tdx_vm_ioctl(vm, cmd, flags, arg);			\
+									\
+	if (ret) {							\
+		TEST_ASSERT(!ret,					\
+			    "%s failed, rc: 0x%llx errno: %i (%s)",	\
+			    #cmd, (unsigned long long)ret,		\
+			    errno, strerror(errno));			\

The if() looks silly. Why add it? And why change it from __TEST_ASSERT_VM_VCPU_IOCTL() in the v13?

Quoted 7 lines

Quoted 2 lines

Considering the suggestion of hw_error above, I think we need to introduce the TEST_ASSERT_TDX_VM_VCPU_IOCTL() which accepts additional hw_error?

The reason we could not use __TEST_ASSERT_VM_VCPU_IOCTL() directly[2] is because it formats ther return value as %i (32-bit), whereas __tdx_vm_ioctl might return a u64 hardware error code.

Quoted 3 lines

I agree with your suggestion to introduce a new TEST_ASSERT_TDX_VM_VCPU_IOCTL() macro to print out u64 hardware error code properly.

Quoted 2 lines

[2]: https://lore.kernel.org/all/[email protected]/

Is tdx_vm_ioctl() the only place where TEST_ASSERT_TDX_VM_VCPU_IOCTL() is going to be used though? If so, maybe we should defer introducing TEST_ASSERT_TDX_VM_VCPU_IOCTL() till later.

I think the issue with if (ret) is just that TEST_ASSERT(!ret) already does that same check, and so we can drop the if (ret) part.

Quoted 4 lines

Quoted 4 lines

The reason we could not use __TEST_ASSERT_VM_VCPU_IOCTL() directly[2] is because it formats ther return value as %i (32-bit), whereas __tdx_vm_ioctl might return a u64 hardware error code.

Since we cannot simply use hw_error to replace ret, there will be not 32bit vs 64bit issue. But ...

Quoted 3 lines

Quoted 3 lines

I agree with your suggestion to introduce a new TEST_ASSERT_TDX_VM_VCPU_IOCTL() macro to print out u64 hardware error code properly.

... if we want to print hw_error as well, we still need a new macro.

Quoted 6 lines

Quoted 2 lines

[2]: https://lore.kernel.org/all/[email protected]/

Is tdx_vm_ioctl() the only place where TEST_ASSERT_TDX_VM_VCPU_IOCTL() is going to be used though? If so, maybe we should defer introducing TEST_ASSERT_TDX_VM_VCPU_IOCTL() till later.

I think tdx_vcpu_ioctl() will use it as well?

Quoted 3 lines

I think the issue with if (ret) is just that TEST_ASSERT(!ret) already does that same check, and so we can drop the if (ret) part.

From: Sagi Shahar <[email protected]>

Add tdx_init_vm() to handle the mandatory VM-level initialization sequence required for Intel TDX.

For TDX, the guest's CPUID configuration must be "sealed" during KVM_TDX_INIT_VM before any vCPUs are created. This is necessary because the TDX hardware directly virtualizes CPUID and includes the configuration in the guest's initial security measurement.

The helper calculates the required CPUID values by filtering the host- supported bits (kvm_get_supported_cpuid) against the "directly configurable" bits reported by KVM_TDX_CAPABILITIES, ensuring compliance with the strict requirements of the TDH.MNG.INIT SEAMCALL.

Co-developed-by Isaku Yamahata <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Co-developed-by Rick Edgecombe <[email protected]>

Signed-off-by Rick Edgecombe <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
 tools/testing/selftests/kvm/Makefile.kvm           |   1 +
 .../testing/selftests/kvm/include/x86/processor.h  |   2 +
 .../selftests/kvm/include/x86/tdx/tdx_util.h       |  35 ++++++
 tools/testing/selftests/kvm/lib/x86/processor.c    |  21 +++-
 tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 120 +++++++++++++++++++++
 5 files changed, 175 insertions(+), 4 deletions(-)

tools/testing/selftests/kvm/Makefile.kvm +1 -0

index e5769268936a..3f98d1c6488c 100644
@@ -27,6 +27,7 @@ LIBKVM_x86 += lib/x86/pmu.c
 LIBKVM_x86 += lib/x86/processor.c
 LIBKVM_x86 += lib/x86/sev.c
 LIBKVM_x86 += lib/x86/svm.c
+LIBKVM_x86 += lib/x86/tdx/tdx_util.c
 LIBKVM_x86 += lib/x86/ucall.c
 LIBKVM_x86 += lib/x86/vmx.c
 

tools/testing/selftests/kvm/include/x86/processor.h +2 -0

index 0aa6eecfcbde..76180dfaffea 100644
@@ -956,6 +956,8 @@ static inline void vcpu_xcrs_set(struct kvm_vcpu *vcpu, struct kvm_xcrs *xcrs)
 	vcpu_ioctl(vcpu, KVM_SET_XCRS, xcrs);
 }
 
+const struct kvm_cpuid_entry2 *__get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
+						 u32 function, u32 index);
 const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
 					       u32 function, u32 index);
 const struct kvm_cpuid2 *kvm_get_supported_cpuid(void);

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +35 -0

index f647e6ca6b34..eb8602dce0bc 100644
@@ -11,4 +11,39 @@ static inline bool is_tdx_vm(struct kvm_vm *vm)
 	return vm->type == KVM_X86_TDX_VM;
 }
 
+/*
+ * TDX ioctls
+ * Use underscores to avoid collisions with struct member names.
+ */
+#define __tdx_vm_ioctl(vm, cmd, _flags, arg)				\
+({									\
+	u64 r;								\
+									\
+	union {								\
+		struct kvm_tdx_cmd c;					\
+		unsigned long raw;					\
+	} tdx_cmd = { .c = {						\
+		.id = (cmd),						\
+		.flags = (u32)(_flags),					\
+		.data = (u64)(arg),					\
+	} };								\
+									\
+	r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw);	\
+	r ?: tdx_cmd.c.hw_error;					\
+})
+
+#define tdx_vm_ioctl(vm, cmd, flags, arg)				\
+({									\
+	u64 ret = __tdx_vm_ioctl(vm, cmd, flags, arg);			\
+									\
+	if (ret) {							\
+		TEST_ASSERT(!ret,					\
+			    "%s failed, rc: 0x%llx errno: %i (%s)",	\
+			    #cmd, (unsigned long long)ret,		\
+			    errno, strerror(errno));			\
+	}								\
+})
+
+void tdx_init_vm(struct kvm_vm *vm, u64 attributes);
+
 #endif /* SELFTESTS_TDX_TDX_UTIL_H */

tools/testing/selftests/kvm/lib/x86/processor.c +0 -0

index b68ad1dc7e02..7d23344854cc 100644

tools/testing/selftests/kvm/lib/x86/processor.c +15 -4

+
 	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);
 	TEST_ASSERT(r > 0, "KVM_GET_TSC_KHZ did not provide a valid TSC frequency.");
 	guest_tsc_khz = r;

@@ -1328,8 +1331,8 @@ void kvm_init_vm_address_properties(struct kvm_vm *vm)
 	}
 }
 
-const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
-					       u32 function, u32 index)
+const struct kvm_cpuid_entry2 *__get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
+						 u32 function, u32 index)
 {
 	int i;
 

@@ -1339,11 +1342,21 @@ const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
 			return &cpuid->entries[i];
 	}
 
-	TEST_FAIL("CPUID function 0x%x index 0x%x not found ", function, index);
-
 	return NULL;
 }
 
+const struct kvm_cpuid_entry2 *get_cpuid_entry(const struct kvm_cpuid2 *cpuid,
+					       u32 function, u32 index)
+{
+	const struct kvm_cpuid_entry2 *entry;
+
+	entry = __get_cpuid_entry(cpuid, function, index);
+	if (!entry)
+		TEST_FAIL("CPUID function 0x%x index 0x%x not found ", function, index);
+
+	return entry;
+}
+
 #define X86_HYPERCALL(inputs...)					\
 ({									\
 	u64 r;							\

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +120 -0

new file mode 100644
index 000000000000..e1ffb67a106c
@@ -0,0 +1,120 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include "processor.h"
+#include "tdx/tdx_util.h"
+
+static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
+{
+	static struct kvm_tdx_capabilities *tdx_cap;
+	int nr_cpuid_configs = 4;
+	int rc = -1;
+	int i;
+
+	if (tdx_cap)
+		return tdx_cap;
+
+	do {
+		nr_cpuid_configs *= 2;
+
+		tdx_cap = realloc(tdx_cap, sizeof(*tdx_cap) +
+					   (sizeof(struct kvm_cpuid_entry2) * nr_cpuid_configs));
+		TEST_ASSERT(tdx_cap,
+			    "Could not allocate memory for tdx capability nr_cpuid_configs %d\n",
+			    nr_cpuid_configs);
+
+		tdx_cap->cpuid.nent = nr_cpuid_configs;
+		rc = __tdx_vm_ioctl(vm, KVM_TDX_CAPABILITIES, 0, tdx_cap);
+	} while (rc < 0 && errno == E2BIG);
+
+	TEST_ASSERT(rc == 0, "KVM_TDX_CAPABILITIES failed: %d %d",
+		    rc, errno);
+
+	pr_debug("tdx_cap: supported_attrs: 0x%016llx\n"
+		 "tdx_cap: supported_xfam 0x%016llx\n",
+		 tdx_cap->supported_attrs, tdx_cap->supported_xfam);
+
+	for (i = 0; i < tdx_cap->cpuid.nent; i++) {
+		const struct kvm_cpuid_entry2 *config = &tdx_cap->cpuid.entries[i];
+
+		pr_debug("cpuid config[%d]: leaf 0x%x sub_leaf 0x%x eax 0x%08x ebx 0x%08x ecx 0x%08x edx 0x%08x\n",
+			 i, config->function, config->index,
+			 config->eax, config->ebx, config->ecx, config->edx);
+	}
+
+	return tdx_cap;
+}
+
+/*
+ * Filter CPUID based on TDX supported capabilities
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   cpuid_data - CPUID fields to filter
+ *
+ * Output Args: None
+ *
+ * Return: None
+ *
+ * For each CPUID leaf, filter out unsupported bits based on the capabilities
+ * reported by the TDX module
+ */
+static void tdx_filter_cpuid(struct kvm_vm *vm,
+			     struct kvm_cpuid2 *cpuid_data)
+{
+	struct kvm_tdx_capabilities *tdx_cap;
+	const struct kvm_cpuid_entry2 *config;
+	struct kvm_cpuid_entry2 *e;
+	int i;
+
+	tdx_cap = tdx_read_capabilities(vm);
+
+	i = 0;
+	while (i < cpuid_data->nent) {
+		e = cpuid_data->entries + i;
+		config = __get_cpuid_entry(&tdx_cap->cpuid, e->function, e->index);
+
+		if (!config) {
+			int left = cpuid_data->nent - i - 1;
+
+			if (left > 0)
+				memmove(cpuid_data->entries + i,
+					cpuid_data->entries + i + 1,
+					sizeof(*cpuid_data->entries) * left);
+			cpuid_data->nent--;
+			continue;
+		}
+
+		e->eax &= config->eax;
+		e->ebx &= config->ebx;
+		e->ecx &= config->ecx;
+		e->edx &= config->edx;
+
+		i++;
+	}
+}
+
+void tdx_init_vm(struct kvm_vm *vm, u64 attributes)
+{
+	struct kvm_tdx_init_vm *init_vm;
+	const struct kvm_cpuid2 *tmp;
+	struct kvm_cpuid2 *cpuid;
+
+	tmp = kvm_get_supported_cpuid();
+
+	cpuid = allocate_kvm_cpuid2(tmp->nent);
+	memcpy(cpuid, tmp, kvm_cpuid2_size(tmp->nent));
+	tdx_filter_cpuid(vm, cpuid);
+
+	init_vm = calloc(1, sizeof(*init_vm) +
+			 sizeof(init_vm->cpuid.entries[0]) * cpuid->nent);
+	TEST_ASSERT(init_vm, "init_vm allocation failed");
+
+	memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));
+	free(cpuid);
+
+	init_vm->attributes = attributes;
+
+	tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);
+
+	free(init_vm);
+}

Quoted 9 lines · Lisa Wang

From: Sagi Shahar <[email protected]>

Add tdx_init_vm() to handle the mandatory VM-level initialization sequence required for Intel TDX.

For TDX, the guest's CPUID configuration must be "sealed" during KVM_TDX_INIT_VM before any vCPUs are created. This is necessary because the TDX hardware directly virtualizes CPUID and includes the configuration in the guest's initial security measurement.

"TDX hardware" should be TDX module. But I'm not sure what it is trying to say about directly virtualizes.

Also, it is not accurate to say that the CPUID configuration is included in the measurement? Is that right Peter? It's not in the report, but is it in the measurement?

Or maybe it doesn't really need to be in the log anyway.

Quoted 1 line

From: Sagi Shahar <[email protected]>

Add tdx_init_vm() to handle the mandatory VM-level initialization sequence required for Intel TDX.

For TDX, the guest's CPUID configuration must be "sealed" during KVM_TDX_INIT_VM before any vCPUs are created. This is necessary because the TDX hardware directly virtualizes CPUID and includes the configuration in the guest's initial security measurement.

The helper calculates the required CPUID values by filtering the host- supported bits (kvm_get_supported_cpuid) against the "directly configurable" bits reported by KVM_TDX_CAPABILITIES, ensuring compliance with the strict requirements of the TDH.MNG.INIT SEAMCALL.

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +10 -0

+
+/*
+ * Filter CPUID based on TDX supported capabilities
+ *
+ * Input Args:
+ *   vm - Virtual Machine
+ *   cpuid_data - CPUID fields to filter
+ *
+ * Output Args: None
+ *

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c new file +3 -0

+	struct kvm_cpuid_entry2 *e;
+	int i;
+

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

+}
+

 void tdx_init_vm(struct kvm_vm *vm, u64 attributes) {
  struct kvm_tdx_init_vm *init_vm;

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +0 -0

@@ -112,6 +122,8 @@ void tdx_init_vm(struct kvm_vm *vm, u64 attributes)
  	memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));
  	free(cpuid);

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

+	tdx_check_attributes(vm, attributes);
+

  init_vm->attributes = attributes;

  tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c static void tdx_filter_cpuid(struct kvm_vm *vm, · line 103

Quoted 8 lines · Lisa Wang

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +8 -0

+static void tdx_check_attributes(struct kvm_vm *vm, u64 attributes)
+{
+	struct kvm_tdx_capabilities *tdx_cap;
+
+	tdx_cap = tdx_read_capabilities(vm);
+
+	/* Make sure all the attributes are reported as supported */
+	TEST_ASSERT_EQ(attributes & tdx_cap->supported_attrs, attributes);

I think this patch actually does nothing because attributes is passed in as 0? So 0 & supported == 0. It does exercise KVM_TDX_CAPABILITIES as claimed, but so does tdx_filter_cpuid(). So for a minimal support, we could probably drop this patch.

Quoted 14 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

+}
+

 void tdx_init_vm(struct kvm_vm *vm, u64 attributes) {
  struct kvm_tdx_init_vm *init_vm;

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

@@ -112,6 +122,8 @@ void tdx_init_vm(struct kvm_vm *vm, u64 attributes)
  	memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));
  	free(cpuid);
 
+	tdx_check_attributes(vm, attributes);
+
  	init_vm->attributes = attributes;
 
  	tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);

From: Sagi Shahar <[email protected]>

Move kernel segment definitions to a separate file which can be included from assembly files.

Reviewed-by Ira Weiny <[email protected]>

Reviewed-by Binbin Wu <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

From: Sagi Shahar <[email protected]>

Move kernel segment definitions to a separate file which can be included from assembly files.

Reviewed-by Ira Weiny <[email protected]>

Reviewed-by Binbin Wu <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

Reviewed-by Xiaoyao Li <[email protected]>

---
 tools/testing/selftests/kvm/include/x86/processor_asm.h | 12 ++++++++++++
 tools/testing/selftests/kvm/lib/x86/processor.c         |  5 +----
 2 files changed, 13 insertions(+), 4 deletions(-)

tools/testing/selftests/kvm/include/x86/processor_asm.h new file +12 -0

new file mode 100644
index 000000000000..713b6bc0aeb7
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Used for storing defines used by both c and assembly code.
+ */
+#ifndef SELFTEST_KVM_PROCESSOR_ASM_H
+#define SELFTEST_KVM_PROCESSOR_ASM_H
+
+#define KERNEL_CS	0x8
+#define KERNEL_DS	0x10
+#define KERNEL_TSS	0x18
+
+#endif  /* SELFTEST_KVM_PROCESSOR_ASM_H */

tools/testing/selftests/kvm/lib/x86/processor.c +1 -5

index 7d23344854cc..97acc5aaf7b7 100644
@@ -8,6 +8,7 @@
 #include "kvm_util.h"
 #include "pmu.h"
 #include "processor.h"
+#include "processor_asm.h"
 #include "smm.h"
 #include "svm_util.h"
 #include "sev.h"

@@ -18,10 +19,6 @@
 #define NUM_INTERRUPTS 256
 #endif
 
-#define KERNEL_CS	0x8
-#define KERNEL_DS	0x10
-#define KERNEL_TSS	0x18
-
 gva_t exception_handlers;
 bool host_cpu_is_amd;
 bool host_cpu_is_intel;
 
--

2.55.0.229.g6434b31f56-goog

From: Sagi Shahar <[email protected]>

Add the Kbuild macros needed to enable the filechk_offsets mechanism to generate C header files containing structure member offset information.

Tools depending on assembly code that operate on structures have to hardcode the offsets of structure members. The Kbuild infrastructure can instead generate C header files with these offsets automatically, allowing them to be included in assembly code as symbolic constants.

For example, the TDX guest boot code requires access to parameters passed in the C structure (struct td_boot_parameters). This header provides the macros needed to extract these offsets from C code and expose them to assembly, ensuring the two remain synchronized.

Signed-off-by Sagi Shahar <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

Reviewed-by Binbin Wu <[email protected]>

---
 tools/include/linux/kbuild.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

tools/include/linux/kbuild.h new file +11 -1

new file mode 100644
index 000000000000..957fd55cd159
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __TOOLS_LINUX_KBUILD_H
+#define __TOOLS_LINUX_KBUILD_H
+
+#define DEFINE(sym, val) \
+	asm volatile("\n.ascii \"->" #sym " %0 " #val "\"" : : "i" (val))
+
+#define OFFSET(sym, str, mem) \
+	DEFINE(sym, __builtin_offsetof(struct str, mem))
+
+#endif /* __TOOLS_LINUX_KBUILD_H */
 
--

2.55.0.229.g6434b31f56-goog

From: Erdem Aktas <[email protected]>

Add code to boot a TDX test VM. Since TDX registers are inaccessible to KVM, the boot code loads the relevant values from memory into the registers before jumping to the guest code.

Reviewed-by Binbin Wu <[email protected]>

Signed-off-by Erdem Aktas <[email protected]>

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Sagi Shahar <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
  tools/testing/selftests/kvm/Makefile.kvm           |  1 +
  .../selftests/kvm/include/x86/tdx/td_boot.h        | 22 ++++++--
  tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S  | 61 ++++++++++++++++++++++
  3 files changed, 81 insertions(+), 3 deletions(-)

tools/testing/selftests/kvm/Makefile.kvm +1 -0

index 397afebbb34b..645d9aac61db 100644
@@ -32,6 +32,7 @@ LIBKVM_x86 += lib/x86/svm.c
  LIBKVM_x86 += lib/x86/tdx/tdx_util.c
  LIBKVM_x86 += lib/x86/ucall.c
  LIBKVM_x86 += lib/x86/vmx.c
+LIBKVM_x86 += lib/x86/tdx/td_boot.S
 
  LIBKVM_arm64 += lib/arm64/gic.c
  LIBKVM_arm64 += lib/arm64/gic_v3.c

tools/testing/selftests/kvm/include/x86/tdx/td_boot.h +19 -3

index bf2282931d49..89cf6c3485be 100644
@@ -2,9 +2,6 @@
  #ifndef SELFTEST_TDX_TD_BOOT_H
  #define SELFTEST_TDX_TD_BOOT_H
 
-#include <linux/compiler.h>
-#include <linux/types.h>
-
  /*
   * Layout for boot section (not to scale)
   *

@@ -24,7 +21,19 @@
   * |                           |
   * |                           |
   * |___________________________|____ 0x0_ffff_0000: TD_BOOT_PARAMETERS_GPA
+ *
+ * TD_BOOT_PARAMETERS_GPA is arbitrarily chosen to
+ *
+ * + be within the 4GB address space
+ * + provide enough contiguous memory for the struct td_boot_parameters such
+ *   that there is one struct td_per_vcpu_parameters for KVM_MAX_VCPUS
   */
+#define TD_BOOT_PARAMETERS_GPA 0xffff0000
+
+#if !defined(__ASSEMBLY__) && !defined(__ASSEMBLER__)
+
+#include <linux/compiler.h>
+#include <linux/types.h>
 
  /*
   * The exact memory layout for LGDT or LIDT instructions.

@@ -63,4 +72,11 @@ struct td_boot_parameters {
  	struct td_per_vcpu_parameters per_vcpu[];
  };
 
+void td_boot(void);
+void td_boot_code_end(void);
+
+#define TD_BOOT_CODE_SIZE (td_boot_code_end - td_boot)
+
+#endif /* !defined(__ASSEMBLY__) && !defined(__ASSEMBLER__) */
+
  #endif /* SELFTEST_TDX_TD_BOOT_H */

tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S new file +61 -0

new file mode 100644
index 000000000000..bca9a4c3d8f8
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#include "tdx/td_boot.h"
+#include "tdx/td_boot_offsets.h"
+#include "processor_asm.h"
+
+.code32
+
+.globl td_boot
+td_boot:
+	/*
+	 * In this procedure, edi is used as a temporary register.
+	 * Paging is turned off.
+	 */
+	cli
+
+	movl $TD_BOOT_PARAMETERS_GPA, %ebx
+
+	/*
+	 * Find the address of struct td_per_vcpu_parameters for this
+	 * vCPU based on esi (TDX spec: initialized with vCPU id). Put
+	 * struct address into register for indirect addressing.
+	 */
+	movl $SIZEOF_TD_PER_VCPU_PARAMETERS, %eax
+	mul %esi
+	leal TD_BOOT_PARAMETERS_PER_VCPU(%ebx), %edi
+	addl %edi, %eax
+
+	/* Setup stack. */
+	movl TD_PER_VCPU_PARAMETERS_ESP_GVA(%eax), %esp
+
+	/* Setup GDT. */
+	leal TD_BOOT_PARAMETERS_GDT(%ebx), %edi
+	lgdt (%edi)
+
+	/* Setup IDT. */
+	leal TD_BOOT_PARAMETERS_IDT(%ebx), %edi
+	lidt (%edi)
+
+	/*
+	 * Set up control registers (There are no instructions to mov from
+	 * memory to control registers, hence use edi as a scratch register).
+	 */
+	movl TD_BOOT_PARAMETERS_CR4(%ebx), %edi
+	movl %edi, %cr4
+	movl TD_BOOT_PARAMETERS_CR3(%ebx), %edi
+	movl %edi, %cr3
+	movl TD_BOOT_PARAMETERS_CR0(%ebx), %edi
+	movl %edi, %cr0
+
+	/* Switching to 64bit mode after ljmp and then jump to guest code */
+	ljmp $(KERNEL_CS),$1f
+1:
+	jmp *TD_PER_VCPU_PARAMETERS_GUEST_CODE(%eax)
+
+/* Leave marker so size of td_boot code can be computed. */
+.globl td_boot_code_end
+td_boot_code_end:
+
+/* Disable executable stack. */
+.section .note.GNU-stack,"",%progbits

From: Erdem Aktas <[email protected]>

Add code to boot a TDX test VM. Since TDX registers are inaccessible to KVM, the boot code loads the relevant values from memory into the registers before jumping to the guest code.

Reviewed-by Binbin Wu <[email protected]>

Signed-off-by Erdem Aktas <[email protected]>

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Co-developed-by Sagi Shahar <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---

tools/testing/selftests/kvm/include/x86/tdx/td_boot.h +0 -3

index bf2282931d49..89cf6c3485be 100644
@@ -2,9 +2,6 @@
 #ifndef SELFTEST_TDX_TD_BOOT_H
 #define SELFTEST_TDX_TD_BOOT_H
 
-#include <linux/compiler.h>
-#include <linux/types.h>
-
 /*
  * Layout for boot section (not to scale)
  *

@@ -24,7 +21,19 @@
  * |                           |
  * |                           |
  * |___________________________|____ 0x0_ffff_0000: TD_BOOT_PARAMETERS_GPA

tools/testing/selftests/kvm/include/x86/tdx/td_boot.h +5 -0

+
+#define TD_BOOT_CODE_SIZE (td_boot_code_end - td_boot)
+
+#endif /* !defined(__ASSEMBLY__) && !defined(__ASSEMBLER__) */
+
 #endif /* SELFTEST_TDX_TD_BOOT_H */

tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S new file +2 -0

new file mode 100644
index 000000000000..bca9a4c3d8f8
@@ -0,0 +1,61 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+

tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S new file +6 -0

+	 */
+	movl $SIZEOF_TD_PER_VCPU_PARAMETERS, %eax
+	mul %esi
+	leal TD_BOOT_PARAMETERS_PER_VCPU(%ebx), %edi
+	addl %edi, %eax
+

From: Sagi Shahar <[email protected]>

TDX can't set sregs values directly using KVM_SET_SREGS. Expose the default values of certain sregs used by TDX VMs so they can be set manually.

Reviewed-by Binbin Wu <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Reviewed-by Ira Weiny <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
  .../testing/selftests/kvm/include/x86/processor.h  | 35 ++++++++++++++++++++++
  tools/testing/selftests/kvm/lib/x86/processor.c    | 20 ++++---------
  2 files changed, 41 insertions(+), 14 deletions(-)

tools/testing/selftests/kvm/include/x86/processor.h +35 -0

index 76180dfaffea..7e00c9414595 100644
@@ -29,6 +29,10 @@ extern u64 guest_tsc_khz;
  #define MAX_NR_CPUID_ENTRIES 100
  #endif
 
+#ifndef NUM_INTERRUPTS
+#define NUM_INTERRUPTS 256
+#endif
+
  #define NONCANONICAL 0xaaaaaaaaaaaaaaaaull
 
  /* Forced emulation prefix, used to invoke the emulator unconditionally. */

@@ -1564,4 +1568,35 @@ u64 *tdp_get_pte(struct kvm_vm *vm, u64 l2_gpa);
 
  bool sys_clocksource_is_based_on_tsc(void);
 
+static inline u16 kvm_get_default_idt_limit(void)
+{
+	return NUM_INTERRUPTS * sizeof(struct idt_entry) - 1;
+}
+
+static inline u16 kvm_get_default_gdt_limit(void)
+{
+	return getpagesize() - 1;
+}
+
+static inline u64 kvm_get_default_cr0(void)
+{
+	return X86_CR0_PE | X86_CR0_NE | X86_CR0_PG;
+}
+
+static inline u64 kvm_get_default_cr4(int pgtable_levels)
+{
+	u64 cr4 = X86_CR4_PAE | X86_CR4_OSFXSR;
+
+	if (kvm_cpu_has(X86_FEATURE_XSAVE))
+		cr4 |= X86_CR4_OSXSAVE;
+	if (pgtable_levels == 5)
+		cr4 |= X86_CR4_LA57;
+	return cr4;
+}
+
+static inline u64 kvm_get_default_efer(void)
+{
+	return EFER_LME | EFER_LMA | EFER_NX;
+}
+
  #endif /* SELFTEST_KVM_PROCESSOR_H */

tools/testing/selftests/kvm/lib/x86/processor.c +6 -14

index 97acc5aaf7b7..ad75e19bb3a2 100644
@@ -15,10 +15,6 @@
  #include "tdx/tdx_util.h"
  #include "vmx.h"
 
-#ifndef NUM_INTERRUPTS
-#define NUM_INTERRUPTS 256
-#endif
-
  gva_t exception_handlers;
  bool host_cpu_is_amd;
  bool host_cpu_is_intel;

@@ -647,17 +643,13 @@ static void vcpu_init_sregs(struct kvm_vm *vm, struct kvm_vcpu *vcpu)
  	vcpu_sregs_get(vcpu, &sregs);
 
  	sregs.idt.base = vm->arch.idt;
-	sregs.idt.limit = NUM_INTERRUPTS * sizeof(struct idt_entry) - 1;
+	sregs.idt.limit = kvm_get_default_idt_limit();
  	sregs.gdt.base = vm->arch.gdt;
-	sregs.gdt.limit = getpagesize() - 1;
-
-	sregs.cr0 = X86_CR0_PE | X86_CR0_NE | X86_CR0_PG;
-	sregs.cr4 |= X86_CR4_PAE | X86_CR4_OSFXSR;
-	if (kvm_cpu_has(X86_FEATURE_XSAVE))
-		sregs.cr4 |= X86_CR4_OSXSAVE;
-	if (vm->mmu.pgtable_levels == 5)
-		sregs.cr4 |= X86_CR4_LA57;
-	sregs.efer |= (EFER_LME | EFER_LMA | EFER_NX);
+	sregs.gdt.limit = kvm_get_default_gdt_limit();
+
+	sregs.cr0 = kvm_get_default_cr0();
+	sregs.cr4 |= kvm_get_default_cr4(vm->mmu.pgtable_levels);
+	sregs.efer |= kvm_get_default_efer();
 
  	kvm_seg_set_unusable(&sregs.ldt);
  	kvm_seg_set_kernel_code_64bit(&sregs.cs);

tools/testing/selftests/kvm/include/x86/processor.h extern u64 guest_tsc_khz; · line 35

Quoted 5 lines · Lisa Wang

tools/testing/selftests/kvm/include/x86/processor.h +4 -0

+#ifndef NUM_INTERRUPTS
+#define NUM_INTERRUPTS 256
+#endif
+

Is this necessary for this patch? I see why to extract kvm_get_default_idt_limit() because now it's used twice. But why extract this define too?

Quoted 4 lines

Is this necessary for this patch? I see why to extract kvm_get_default_idt_limit() because now it's used twice. But why extract this
define too?

kvm_get_default_idt_limit() uses NUM_INTERRUPTS, and kvm_get_default_idt_limit() is extracted as a helper from .c file to .h file. Shouldn't the define of NUM_INTERRUPTS be moved together?

From: Sagi Shahar <[email protected]>

Add memory for TDX boot code in a separate memslot.

Use virt_map() to get identity map in this memory region to allow for seamless transition from paging disabled to paging enabled code.

Copy the boot code into the memory region and set up the reset vector at this point. While it's possible to separate the memory allocation and boot code initialization into separate functions, having all the calculations for memory size and offsets in one place simplifies the code and avoids duplications.

Handcode the reset vector as suggested by Sean Christopherson.

Reviewed-by Binbin Wu <[email protected]>

Suggested-by Sean Christopherson <[email protected]>

Co-developed-by Erdem Aktas <[email protected]>

Signed-off-by Erdem Aktas <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

From: Sagi Shahar <[email protected]>

Add memory for TDX boot code in a separate memslot.

Use virt_map() to get identity map in this memory region to allow for seamless transition from paging disabled to paging enabled code.

Copy the boot code into the memory region and set up the reset vector at this point. While it's possible to separate the memory allocation and boot code initialization into separate functions, having all the calculations for memory size and offsets in one place simplifies the code and avoids duplications.

Handcode the reset vector as suggested by Sean Christopherson.

Reviewed-by Binbin Wu <[email protected]>

Suggested-by Sean Christopherson <[email protected]>

Co-developed-by Erdem Aktas <[email protected]>

Signed-off-by Erdem Aktas <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
 .../selftests/kvm/include/x86/tdx/tdx_util.h       |  1 +
 tools/testing/selftests/kvm/lib/x86/processor.c    |  4 +-
 tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 46 ++++++++++++++++++++++
 3 files changed, 50 insertions(+), 1 deletion(-)

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +28 -0

+
+void tdx_vm_setup_boot_code_region(struct kvm_vm *vm)
+{
+	size_t total_code_size = TD_BOOT_CODE_SIZE + X86_RESET_VECTOR_SIZE;
+	gpa_t boot_code_gpa = X86_RESET_VECTOR - TD_BOOT_CODE_SIZE;
+	gpa_t alloc_gpa = round_down(boot_code_gpa, PAGE_SIZE);
+	size_t nr_pages = DIV_ROUND_UP(total_code_size, PAGE_SIZE);
+	const u64 gmem_flags = 0;
+	gpa_t gpa;
+	u8 *hva;
+
+	vm_mem_add(vm, VM_MEM_SRC_SHMEM, alloc_gpa, TD_BOOT_CODE_SLOT,
+		   nr_pages, KVM_MEM_GUEST_MEMFD, -1, 0, gmem_flags);
+
+	gpa = vm_phy_pages_alloc(vm, nr_pages, alloc_gpa, TD_BOOT_CODE_SLOT);
+	TEST_ASSERT(gpa == alloc_gpa, "Failed vm_phy_pages_alloc\n");
+
+	virt_map(vm, alloc_gpa, alloc_gpa, nr_pages);
+	hva = addr_gpa2hva(vm, boot_code_gpa);
+	memcpy(hva, td_boot, TD_BOOT_CODE_SIZE);
+
+	hva += TD_BOOT_CODE_SIZE;
+	TEST_ASSERT(hva == addr_gpa2hva(vm, X86_RESET_VECTOR),
+		    "Expected RESET vector at hva 0x%lx, got %lx",
+		    (unsigned long)addr_gpa2hva(vm, X86_RESET_VECTOR), (unsigned long)hva);
+
+	/*
+	 * Handcode "JMP rel8" at the RESET vector to jump back to the TD boot

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

+}
+
 static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
 {
 	static struct kvm_tdx_capabilities *tdx_cap;

Signature · 2 lines

-- 
2.55.0.229.g6434b31f56-goog

Quoted 27 lines · Lisa Wang

(quoted diff) +27 -0

+void tdx_vm_setup_boot_code_region(struct kvm_vm *vm)
+{
+	size_t total_code_size = TD_BOOT_CODE_SIZE + X86_RESET_VECTOR_SIZE;
+	gpa_t boot_code_gpa = X86_RESET_VECTOR - TD_BOOT_CODE_SIZE;
+	gpa_t alloc_gpa = round_down(boot_code_gpa, PAGE_SIZE);
+	size_t nr_pages = DIV_ROUND_UP(total_code_size, PAGE_SIZE);
+	const u64 gmem_flags = 0;
+	gpa_t gpa;
+	u8 *hva;
+
+	vm_mem_add(vm, VM_MEM_SRC_SHMEM, alloc_gpa, TD_BOOT_CODE_SLOT,
+		   nr_pages, KVM_MEM_GUEST_MEMFD, -1, 0, gmem_flags);
+
+	gpa = vm_phy_pages_alloc(vm, nr_pages, alloc_gpa, TD_BOOT_CODE_SLOT);
+	TEST_ASSERT(gpa == alloc_gpa, "Failed vm_phy_pages_alloc\n");
+
+	virt_map(vm, alloc_gpa, alloc_gpa, nr_pages);
+	hva = addr_gpa2hva(vm, boot_code_gpa);
+	memcpy(hva, td_boot, TD_BOOT_CODE_SIZE);
+
+	hva += TD_BOOT_CODE_SIZE;
+	TEST_ASSERT(hva == addr_gpa2hva(vm, X86_RESET_VECTOR),
+		    "Expected RESET vector at hva 0x%lx, got %lx",
+		    (unsigned long)addr_gpa2hva(vm, X86_RESET_VECTOR), (unsigned long)hva);
+
+	/*
+	 * Handcode "JMP rel8" at the RESET vector to jump back to the TD boot

Review

Quoted 10 lines

(quoted diff) +10 -0

+	 * code, as there are only 16 bytes at the RESET vector before RIP will
+	 * wrap back to zero. Insert a trailing int3 so that the vCPU crashes in
+	 * case the JMP somehow falls through. Note! The target address is
+	 * relative to the end of the instruction!
+	 */
+	TEST_ASSERT(TD_BOOT_CODE_SIZE + 2 <= 128,
+		    "TD boot code not addressable by 'JMP rel8'");
+	hva[0] = 0xeb;
+	hva[1] = 256 - 2 - TD_BOOT_CODE_SIZE;
+	hva[2] = 0xcc;

This handcoding has persisted for several versions (since v9) so I only want to comment gently... The current code looks correct but I think this could be simpler. But feel free to keep the current implementation.

I can see handcoding "JMP rel8" generates a jump instruction predictably. But having to calculate and sanity check the boot code offset by hand seems quite painful. And the math is quite tricky. I was thinking the assembler could do a lot more heavy lifting here.

A nice property of this boot code is the fact that it's executed in 32-bit mode from the get go. So there's no need for 16-bit programming and "JMP rel32" is readily available. I.e. a "jmp td_boot" in td_boot.S should suffice and the assembler screams if td_boot isn't reachable. And the int3's after the jump can probably go away as well since the jump is now very, very likely to succeed.

There shouldn't be a need to "pad reset_vector to its full size of 16 bytes" as stated in v8 [1]. The exported "reset_vector" symbol in v8, plus the boot code start & end markers, should be enough to help tdx_vm_setup_boot_code_region() put this boot blob in the right place.

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +0 -0

index 831b0e5160df..170354dcdb66 100644
@@ -15,10 +15,11 @@
 

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +0 -0

 	u64 gmem_flags = 0;
 	gpa_t gpa;
 	u8 *hva;

Quoted 5 lines

There shouldn't be a need to "pad reset_vector to its full size of 16 bytes" as stated in v8 [1]. The exported "reset_vector" symbol in v8, plus the boot code start & end markers, should be enough to help tdx_vm_setup_boot_code_region() put this boot blob in the right place.

Ya, looking at this with fresh eyes, AFAICT there's no reason to handcode anything, it's just basic arithmetic.

Side topic, this series doesn't compile for me, so the below isn't even properly compile-tested (I hacked in arbitrary literals to get past the undefined references).

/usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:26:(.text+0xf): undefined reference to TD_BOOT_PARAMETERS_PER_VCPU' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:30:(.text+0x17): undefined reference to TD_PER_VCPU_PARAMETERS_ESP_GVA' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:33:(.text+0x1d): undefined reference to TD_BOOT_PARAMETERS_GDT' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:37:(.text+0x26): undefined reference to TD_BOOT_PARAMETERS_IDT' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:44:(.text+0x2f): undefined reference to TD_BOOT_PARAMETERS_CR4' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:46:(.text+0x38): undefined reference to TD_BOOT_PARAMETERS_CR3' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:48:(.text+0x41): undefined reference to TD_BOOT_PARAMETERS_CR0' /usr/bin/x86_64-linux-gnu-ld.bfd: tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S:54:(.text+0x51): undefined reference to TD_PER_VCPU_PARAMETERS_GUEST_CODE'

From: Sagi Shahar <[email protected]>

Allocate memory for TDX boot parameters and define the utility functions necessary to fill this memory with the boot parameters.

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
  .../selftests/kvm/include/x86/tdx/tdx_util.h       |  2 +
  tools/testing/selftests/kvm/lib/x86/processor.c    |  2 +
  tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 51 ++++++++++++++++++++++
  3 files changed, 55 insertions(+)

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +2 -0

index 642ffa010da3..f5003cbaa106 100644
@@ -46,5 +46,7 @@ static inline bool is_tdx_vm(struct kvm_vm *vm)
 
  void tdx_init_vm(struct kvm_vm *vm, u64 attributes);
  void tdx_vm_setup_boot_code_region(struct kvm_vm *vm);
+void tdx_vm_setup_boot_parameters_region(struct kvm_vm *vm, u32 nr_runnable_vcpus);
+void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm);
 
  #endif /* SELFTESTS_TDX_TDX_UTIL_H */

tools/testing/selftests/kvm/lib/x86/processor.c +2 -0

index f93953777ca1..748c6041f8bf 100644
@@ -794,6 +794,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm, unsigned int nr_vcpus)
  	if (is_tdx_vm(vm)) {
  		tdx_init_vm(vm, 0);
  		tdx_vm_setup_boot_code_region(vm);
+		tdx_vm_setup_boot_parameters_region(vm, nr_vcpus);
+		tdx_vm_load_common_boot_parameters(vm);
  	}
 
  	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +1 -0

index d64d153d19e2..2ce8c297f7ed 100644
@@ -6,6 +6,7 @@
 
  /* Arbitrarily selected to avoid overlaps with anything else */
  #define TD_BOOT_CODE_SLOT	20
+#define TD_BOOT_PARAMETERS_SLOT	21
 
  #define X86_RESET_VECTOR	0xfffffff0ul
  #define X86_RESET_VECTOR_SIZE	16

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +7 -0

+	int npages = DIV_ROUND_UP(boot_params_size, PAGE_SIZE);
+	const u64 gmem_flags = 0;
+	gpa_t gpa;
+
+	vm_mem_add(vm, VM_MEM_SRC_SHMEM, TD_BOOT_PARAMETERS_GPA,
+		   TD_BOOT_PARAMETERS_SLOT, npages,
+		   KVM_MEM_GUEST_MEMFD, -1, 0, gmem_flags);

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +28 -0

+	u32 cr4;
+
+	cr4 = kvm_get_default_cr4(vm->mmu.pgtable_levels);
+
+	/* TDX spec 11.6.2: CR4 bit MCE is fixed to 1 */
+	cr4 |= X86_CR4_MCE;
+
+	/* TDX spec 11.6.2: CR4 bit VMXE and SMXE are fixed to 0 */
+	cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);
+
+	/* Set parameters! */
+	params->cr0 = kvm_get_default_cr0();
+	TEST_ASSERT(vm->mmu.pgd < (1ULL << 32),
+		    "PGD must be within 32-bit address space for 32-bit boot code");
+	params->cr3 = vm->mmu.pgd;
+	params->cr4 = cr4;
+	params->idtr.base = vm->arch.idt;
+	params->idtr.limit = kvm_get_default_idt_limit();
+	params->gdtr.base = vm->arch.gdt;
+	params->gdtr.limit = kvm_get_default_gdt_limit();
+
+	TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
+	TEST_ASSERT(params->cr3 != 0, "cr3 should not be 0");
+	TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");
+	TEST_ASSERT(params->gdtr.base != 0, "gdt base address should not be 0");
+	TEST_ASSERT(params->idtr.base != 0, "idt base address should not be 0");
+}
+
  static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
  {
  	static struct kvm_tdx_capabilities *tdx_cap;

From: Sagi Shahar <[email protected]>

Allocate memory for TDX boot parameters and define the utility functions necessary to fill this memory with the boot parameters.

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
 .../selftests/kvm/include/x86/tdx/tdx_util.h       |  2 +
 tools/testing/selftests/kvm/lib/x86/processor.c    |  2 +
 tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 51 ++++++++++++++++++++++
 3 files changed, 55 insertions(+)

tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h +2 -0

index 642ffa010da3..f5003cbaa106 100644
@@ -46,5 +46,7 @@ static inline bool is_tdx_vm(struct kvm_vm *vm)
 
 void tdx_init_vm(struct kvm_vm *vm, u64 attributes);
 void tdx_vm_setup_boot_code_region(struct kvm_vm *vm);
+void tdx_vm_setup_boot_parameters_region(struct kvm_vm *vm, u32 nr_runnable_vcpus);
+void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm);
 
 #endif /* SELFTESTS_TDX_TDX_UTIL_H */

tools/testing/selftests/kvm/lib/x86/processor.c +2 -0

index f93953777ca1..748c6041f8bf 100644
@@ -794,6 +794,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm, unsigned int nr_vcpus)
 	if (is_tdx_vm(vm)) {
 		tdx_init_vm(vm, 0);
 		tdx_vm_setup_boot_code_region(vm);
+		tdx_vm_setup_boot_parameters_region(vm, nr_vcpus);
+		tdx_vm_load_common_boot_parameters(vm);
 	}
 
 	r = __vm_ioctl(vm, KVM_GET_TSC_KHZ, NULL);

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +51 -0

index d64d153d19e2..2ce8c297f7ed 100644
@@ -6,6 +6,7 @@
 
 /* Arbitrarily selected to avoid overlaps with anything else */
 #define TD_BOOT_CODE_SLOT	20
+#define TD_BOOT_PARAMETERS_SLOT	21
 
 #define X86_RESET_VECTOR	0xfffffff0ul
 #define X86_RESET_VECTOR_SIZE	16

@@ -49,6 +50,56 @@ void tdx_vm_setup_boot_code_region(struct kvm_vm *vm)
 	hva[2] = 0xcc;
 }
 
+void tdx_vm_setup_boot_parameters_region(struct kvm_vm *vm, u32 nr_runnable_vcpus)
+{
+	size_t boot_params_size =
+		sizeof(struct td_boot_parameters) +
+		nr_runnable_vcpus * sizeof(struct td_per_vcpu_parameters);
+	int npages = DIV_ROUND_UP(boot_params_size, PAGE_SIZE);
+	const u64 gmem_flags = 0;
+	gpa_t gpa;
+
+	vm_mem_add(vm, VM_MEM_SRC_SHMEM, TD_BOOT_PARAMETERS_GPA,
+		   TD_BOOT_PARAMETERS_SLOT, npages,
+		   KVM_MEM_GUEST_MEMFD, -1, 0, gmem_flags);
+	gpa = vm_phy_pages_alloc(vm, npages, TD_BOOT_PARAMETERS_GPA, TD_BOOT_PARAMETERS_SLOT);
+	TEST_ASSERT(gpa == TD_BOOT_PARAMETERS_GPA, "Failed vm_phy_pages_alloc\n");
+
+	virt_map(vm, TD_BOOT_PARAMETERS_GPA, TD_BOOT_PARAMETERS_GPA, npages);
+}
+
+void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm)
+{
+	struct td_boot_parameters *params =
+		addr_gpa2hva(vm, TD_BOOT_PARAMETERS_GPA);
+	u32 cr4;
+
+	cr4 = kvm_get_default_cr4(vm->mmu.pgtable_levels);
+
+	/* TDX spec 11.6.2: CR4 bit MCE is fixed to 1 */
+	cr4 |= X86_CR4_MCE;
+
+	/* TDX spec 11.6.2: CR4 bit VMXE and SMXE are fixed to 0 */
+	cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);
+
+	/* Set parameters! */
+	params->cr0 = kvm_get_default_cr0();
+	TEST_ASSERT(vm->mmu.pgd < (1ULL << 32),
+		    "PGD must be within 32-bit address space for 32-bit boot code");
+	params->cr3 = vm->mmu.pgd;
+	params->cr4 = cr4;
+	params->idtr.base = vm->arch.idt;
+	params->idtr.limit = kvm_get_default_idt_limit();
+	params->gdtr.base = vm->arch.gdt;
+	params->gdtr.limit = kvm_get_default_gdt_limit();
+
+	TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
+	TEST_ASSERT(params->cr3 != 0, "cr3 should not be 0");
+	TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");
+	TEST_ASSERT(params->gdtr.base != 0, "gdt base address should not be 0");
+	TEST_ASSERT(params->idtr.base != 0, "idt base address should not be 0");
+}
+
 static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
 {
 	static struct kvm_tdx_capabilities *tdx_cap;

From: Sagi Shahar <[email protected]>

Allocate memory for TDX boot parameters and define the utility functions necessary to fill this memory with the boot parameters.

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
 .../selftests/kvm/include/x86/tdx/tdx_util.h       |  2 +
 tools/testing/selftests/kvm/lib/x86/processor.c    |  2 +
 tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 51 ++++++++++++++++++++++
 3 files changed, 55 insertions(+)

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -0

+}
+
 static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
 {
 	static struct kvm_tdx_capabilities *tdx_cap;

Signature · 2 lines

-- 
2.55.0.229.g6434b31f56-goog

Quoted 15 lines · Lisa Wang

From: Sagi Shahar <[email protected]>

Allocate memory for TDX boot parameters and define the utility functions necessary to fill this memory with the boot parameters.

Co-developed-by Ackerley Tng <[email protected]>

Signed-off-by Ackerley Tng <[email protected]>

Signed-off-by Sagi Shahar <[email protected]>

Signed-off-by Lisa Wang <[email protected]>

---
 .../selftests/kvm/include/x86/tdx/tdx_util.h       |  2 +
 tools/testing/selftests/kvm/lib/x86/processor.c    |  2 +
 tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 51 ++++++++++++++++++++++
 3 files changed, 55 insertions(+)

[ ... ]

Quoted 9 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +9 -0

+
+void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm)
+{
+	struct td_boot_parameters *params =
+		addr_gpa2hva(vm, TD_BOOT_PARAMETERS_GPA);
+	u32 cr4;
+
+	cr4 = kvm_get_default_cr4(vm->mmu.pgtable_levels);
+

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c void tdx_vm_setup_boot_code_region(struct kvm_vm *vm) · line 82

Quoted 4 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +4 -0

+	/* TDX spec 11.6.2: CR4 bit MCE is fixed to 1 */
+	cr4 |= X86_CR4_MCE;
+
+	/* TDX spec 11.6.2: CR4 bit VMXE and SMXE are fixed to 0 */

This is now out of date (11.9.2 as of today)...
I wonder if it's better to just say something like: "Per the TDX spec, ..."

Quoted 6 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +6 -0

+	cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);
+
+	/* Set parameters! */
+	params->cr0 = kvm_get_default_cr0();
+	TEST_ASSERT(vm->mmu.pgd < (1ULL << 32),
+		    "PGD must be within 32-bit address space for 32-bit boot code");

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c void tdx_vm_setup_boot_code_region(struct kvm_vm *vm) · line 93

Quoted 5 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +5 -0

+	params->cr3 = vm->mmu.pgd;
+	params->cr4 = cr4;
+	params->idtr.base = vm->arch.idt;
+	params->idtr.limit = kvm_get_default_idt_limit();
+	params->gdtr.base = vm->arch.gdt;

        vm->arch.{idt,gdt} must also be 32-bit addressable here, just
        like vm->mmu.pgd.

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c void tdx_vm_setup_boot_code_region(struct kvm_vm *vm) · line 100

Quoted 7 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +7 -0

+	params->gdtr.limit = kvm_get_default_gdt_limit();
+
+	TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
+	TEST_ASSERT(params->cr3 != 0, "cr3 should not be 0");
+	TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");
+	TEST_ASSERT(params->gdtr.base != 0, "gdt base address should not be 0");
+	TEST_ASSERT(params->idtr.base != 0, "idt base address should not be 0");

Making sure that cr0/cr4 are simply non-zero feels kind of odd to me. And cr3/gdtr/idtr all came from memory allocators with non-zero minimum addresses.

Quoted 10 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +2 -1

+}
+
 static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
 {
 	static struct kvm_tdx_capabilities *tdx_cap;
 
--

2.55.0.229.g6434b31f56-goog

Quoted 16 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c void tdx_vm_setup_boot_code_region(struct kvm_vm *vm) · line 93

Quoted 12 lines

tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c +11 -0

+	cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);
+
+	/* Set parameters! */
+	params->cr0 = kvm_get_default_cr0();
+	TEST_ASSERT(vm->mmu.pgd < (1ULL << 32),
+		    "PGD must be within 32-bit address space for 32-bit boot code");
+	params->cr3 = vm->mmu.pgd;
+	params->cr4 = cr4;
+	params->idtr.base = vm->arch.idt;
+	params->idtr.limit = kvm_get_default_idt_limit();
+	params->gdtr.base = vm->arch.gdt;

        vm->arch.{idt,gdt} must also be 32-bit addressable here, just
        like vm->mmu.pgd.

Something like this?

    TEST_ASSERT(params->gdtr.base < (1ULL << 32),
            "gdt must be within 32-bit address space for 32-bit boot code");