On Fri, Aug 15, 2025, Sagi Shahar wrote:
On Tue, Aug 12, 2025 at 11:22 PM Binbin Wu binbin.wu@linux.intel.com wrote:
On 8/12/2025 4:13 AM, Sean Christopherson wrote:
On Thu, Aug 07, 2025, Sagi Shahar wrote:
[...]
+/*
- Boot parameters for the TD.
- Unlike a regular VM, KVM cannot set registers such as esp, eip, etc
- before boot, so to run selftests, these registers' values have to be
- initialized by the TD.
- This struct is loaded in TD private memory at TD_BOOT_PARAMETERS_GPA.
- The TD boot code will read off parameters from this struct and set up the
- vCPU for executing selftests.
- */
+struct __packed td_boot_parameters {
None of these comments explain why these structures are __packed, and I suspect _that_ is the most interesting/relevant information for unfamiliar readers.
I guess because the fields defined in this structure are accessed by hard-coded offsets in boot code. But as you suggested below, replicating the functionality of the kernel's OFFSET() could get rid of "__packed".
I agree, I think the reason for using __packed is because of the hard coded offsets. I tried using OFFSET() as Sean suggested but couldn't make it work.
I can't get the Kbuild scripts to work inside the kvm selftests Makefile. I tried adding the following rules based on a reference I found:
+include/x86/tdx/td_boot_offsets.h: lib/x86/tdx/td_boot_offsets.s
$(call filechk,offsets,__TDX_BOOT_OFFSETS_H__)+lib/x86/tdx/td_boot_offsets.s: lib/x86/tdx/td_boot_offsets.c
$(call if_changed_dep,cc_s_c)But I'm getting the following error when trying to generate the header:
/bin/sh: -c: line 1: syntax error near unexpected token `;' /bin/sh: -c: line 1: `set -e; ; printf '# cannot find fixdep (%s)\n'
lib/x86/tdx/.td_boot_offsets.s.cmd; printf '# using basic dep
data\n\n' >> lib/x86/tdx/.td_boot_offsets.s.cmd; cat lib/x86/tdx/.td_boot_offsets.s.d >> lib/x86/tdx/.td_boot_offsets.s.cmd; printf '\n%s\n' 'cmd_lib/x86/tdx/td_boot_offsets.s := ' >> lib/x86/tdx/.td_boot_offsets.s.cmd' make: *** [Makefile.kvm:44: lib/x86/tdx/td_boot_offsets.s] Error 2
For now I can add a comment on the __packed and add a TODO to replace it with OFFSET. I think that making OFFSET work inside the kvm selftests will require more expertise in the Kbuild system which I don't have.
No, I don't want to punt on this. I don't care about __packed, I care about the maintenance and review costs associated with hand coding struct offsets in .S files.
The problem is this line:
$(call if_changed_dep,cc_s_c)
IIUC, the kernel's "generic" command for generating a .s file from a .c file assumes various paths and flags, which doesn't play nice with KVM selftests' unusual setup.
We could fudge around that by defining a custom command, e.g.
cmd_kvm_cc_s_c = $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -S $< -o $@
but that just runs into more problems with the build system (variables not defined, more assumptions about the environment, etc).
AFAICT, there's no need to use if_changed_dep, i.e. fixdep. KVM selftests generate dependencies using standard mechanisms, and they appear to work as expected for this case, so just omit the if_change_dep and let the existing dependency stuff do its magic.
This could be tidied up, e.g. add kbuild.h to tool/s, and is obviously incomplete, but it works.
--- tools/testing/selftests/kvm/Makefile.kvm | 13 ++++++++++ .../kvm/lib/x86/tdx/td_boot_offsets.c | 25 +++++++++++++++++++ 2 files changed, 38 insertions(+) create mode 100644 tools/testing/selftests/kvm/lib/x86/tdx/td_boot_offsets.c
diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm index 438502e02a0f..1fec90da15a1 100644 --- a/tools/testing/selftests/kvm/Makefile.kvm +++ b/tools/testing/selftests/kvm/Makefile.kvm @@ -237,6 +237,9 @@ OVERRIDE_TARGETS = 1 include ../lib.mk include ../cgroup/lib/libcgroup.mk
+include $(top_srcdir)/scripts/Kbuild.include +include $(top_srcdir)/scripts/Makefile.lib + INSTALL_HDR_PATH = $(top_srcdir)/usr LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/ LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include @@ -326,18 +329,28 @@ $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c $(GEN_HDRS) $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S $(GEN_HDRS) $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
+$(OUTPUT)/lib/x86/tdx/td_boot.o: $(OUTPUT)/include/x86/tdx/td_boot_offsets.h + # Compile the string overrides as freestanding to prevent the compiler from # generating self-referential code, e.g. without "freestanding" the compiler may # "optimize" memcmp() by invoking memcmp(), thus causing infinite recursion. $(LIBKVM_STRING_OBJ): $(OUTPUT)/%.o: %.c $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -ffreestanding $< -o $@
+$(OUTPUT)/lib/x86/tdx/td_boot_offsets.s: lib/x86/tdx/td_boot_offsets.c + $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -S $< -o $@ + +$(OUTPUT)/include/x86/tdx/td_boot_offsets.h: $(OUTPUT)/lib/x86/tdx/td_boot_offsets.s FORCE + $(call filechk,offsets,__TDX_BOOT_OFFSETS_H__) + $(shell mkdir -p $(sort $(dir $(TEST_GEN_PROGS)))) $(SPLIT_TEST_GEN_OBJ): $(GEN_HDRS) $(TEST_GEN_PROGS): $(LIBKVM_OBJS) $(TEST_GEN_PROGS_EXTENDED): $(LIBKVM_OBJS) $(TEST_GEN_OBJ): $(GEN_HDRS)
+FORCE: + cscope: include_paths = $(LINUX_TOOL_INCLUDE) $(LINUX_HDR_PATH) include lib .. cscope: $(RM) cscope.* diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/td_boot_offsets.c b/tools/testing/selftests/kvm/lib/x86/tdx/td_boot_offsets.c new file mode 100644 index 000000000000..622f9a19ca30 --- /dev/null +++ b/tools/testing/selftests/kvm/lib/x86/tdx/td_boot_offsets.c @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: GPL-2.0 +#define COMPILE_OFFSETS + +#include <linux/kernel.h> + +#include "tdx/td_boot.h" + +#define DEFINE(sym, val) \ + asm volatile("\n.ascii "->" #sym " %0 " #val """ : : "i" (val)) + +#define BLANK() asm volatile("\n.ascii "->"" : : ) + +#define OFFSET(sym, str, mem) \ + DEFINE(sym, offsetof(struct str, mem)) + +#define COMMENT(x) \ + asm volatile("\n.ascii "->#" x """) + +static void __attribute__((used)) common(void) +{ + BLANK(); + OFFSET(TD_BOOT_cr0, td_boot_parameters, cr0); + OFFSET(TD_BOOT_cr3, td_boot_parameters, cr3); + OFFSET(TD_BOOT_cr4, td_boot_parameters, cr4); +}
base-commit: 04b916993b06d35ec96a5324e0ed93d1eb115dbd