Re: [PATCH v8 06/30] KVM: selftests: Add helper functions to create TDX VMs

[Sean Christopherson <seanjc@google.com>]

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
--