Message-ID: <d8425202-733e-4b29-990d-56d9efed44f9@intel.com>
Date: Fri, 15 Aug 2025 17:28:39 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Sagi Shahar <sagis@...gle.com>, Binbin Wu <binbin.wu@...ux.intel.com>
CC: Sean Christopherson <seanjc@...gle.com>,
	<linux-kselftest@...r.kernel.org>, Paolo Bonzini <pbonzini@...hat.com>,
	"Shuah Khan" <shuah@...nel.org>, Ackerley Tng <ackerleytng@...gle.com>, Ryan
 Afranji <afranji@...gle.com>, Andrew Jones <ajones@...tanamicro.com>, Isaku
 Yamahata <isaku.yamahata@...el.com>, Erdem Aktas <erdemaktas@...gle.com>,
	"Rick Edgecombe" <rick.p.edgecombe@...el.com>, Roger Wang
	<runanwang@...gle.com>, Oliver Upton <oliver.upton@...ux.dev>, "Pratik R.
 Sampat" <pratikrajesh.sampat@....com>, Ira Weiny <ira.weiny@...el.com>,
	<linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v8 06/30] KVM: selftests: Add helper functions to create
 TDX VMs

Hi Sagi,

On 8/14/25 10:20 PM, Sagi Shahar wrote:
> On Tue, Aug 12, 2025 at 11:22 PM Binbin Wu <binbin.wu@...ux.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
> 

I do not believe that the selftests can directly use the Kbuild
infrastructure. From what I understand, when similar things are needed they
are duplicated. Take for example tools/build/Build.include that is included
in Makefile.kvm and contains the needed things duplicated from
scripts/Kbuild.include.

I see two options here:
a) Selftests relies on the kernel to generate the header file. This unfortunately
   will create dependency on kernel being built first and I expect a no-go.
b) - Duplicate the filechk (that depends on tmp-target), filechk_offsets, and 
     sed-offsets defines in from scripts/Makefile.lib to tools/build/Build.include
   - Duplicate include/linux/kbuild.h in tools/include/linux

(b) will allow your rule duplicated below to work:
	include/x86/tdx/td_boot_offsets.h: lib/x86/tdx/td_boot_offsets.s
	       $(call filechk,offsets,__TDX_BOOT_OFFSETS_H__)

tools/build/Build.include already contains if_changed_dep and the command
it calls may just be locally defined to make the other rule you are trying work.

I played around with this a bit and found changes below to work. I certainly
admit that it is crude but "it works" as a proof-of-concept. I also look forward to
learn from other folks on how to to this right.

If you think something like this is useful then please consider below only as a 
proof-of-concept since it needs to be split appropriately, missing cleanups
(handling "make clean"), may not need to be TDX specific, etc.

---8<---
 tools/build/Build.include                     | 56 +++++++++++++++++++
 tools/include/linux/kbuild.h                  | 16 ++++++
 tools/testing/selftests/kvm/Makefile.kvm      | 11 ++++
 .../selftests/kvm/lib/x86/asm-tdx-offsets.c   | 28 ++++++++++
 .../selftests/kvm/lib/x86/tdx/tdcall.S        | 16 +-----
 5 files changed, 113 insertions(+), 14 deletions(-)
 create mode 100644 tools/include/linux/kbuild.h
 create mode 100644 tools/testing/selftests/kvm/lib/x86/asm-tdx-offsets.c

diff --git a/tools/build/Build.include b/tools/build/Build.include
index e45b2eb0d24a..330892a1ba15 100644
--- a/tools/build/Build.include
+++ b/tools/build/Build.include
@@ -20,6 +20,10 @@ space   := $(empty) $(empty)
 # Name of target with a '.' as filename prefix. foo/bar.o => foo/.bar.o
 dot-target = $(dir $@).$(notdir $@)
 
+###
+# Name of target with a '.tmp_' as filename prefix. foo/bar.o => foo/.tmp_bar.o
+tmp-target = $(dir $@).tmp_$(notdir $@)
+
 ###
 # filename of target with directory and extension stripped
 basetarget = $(basename $(notdir $@))
@@ -43,6 +47,58 @@ escsq = $(subst $(squote),'\$(squote)',$1)
 echo-cmd = $(if $($(quiet)cmd_$(1)),\
            echo '  $(call escsq,$($(quiet)cmd_$(1)))';)
 
+# Default sed regexp - multiline due to syntax constraints
+#
+# Use [:space:] because LLVM's integrated assembler inserts <tab> around
+# the .ascii directive whereas GCC keeps the <space> as-is.
+define sed-offsets
+	's:^[[:space:]]*\.ascii[[:space:]]*"\(.*\)".*:\1:; \
+	/^->/{s:->#\(.*\):/* \1 */:; \
+	s:^->\([^ ]*\) [\$$#]*\([^ ]*\) \(.*\):#define \1 \2 /* \3 */:; \
+	s:->::; p;}'
+endef
+
+# Use filechk to avoid rebuilds when a header changes, but the resulting file
+# does not
+define filechk_offsets
+	 echo "#ifndef $2"; \
+	 echo "#define $2"; \
+	 echo "/*"; \
+	 echo " * DO NOT MODIFY."; \
+	 echo " *"; \
+	 echo " * This file was generated by Kbuild"; \
+	 echo " */"; \
+	 echo ""; \
+	 sed -ne $(sed-offsets) < $<; \
+	 echo ""; \
+	 echo "#endif"
+endef
+
+###
+# filechk is used to check if the content of a generated file is updated.
+# Sample usage:
+#
+# filechk_sample = echo $(KERNELRELEASE)
+# version.h: FORCE
+#	$(call filechk,sample)
+#
+# The rule defined shall write to stdout the content of the new file.
+# The existing file will be compared with the new one.
+# - If no file exist it is created
+# - If the content differ the new file is used
+# - If they are equal no change, and no timestamp update
+define filechk
+	echo $(tmp-target);					\
+	$(Q)set -e;						\
+	mkdir -p $(dir $@);					\
+	trap "rm -f $(tmp-target)" EXIT;			\
+	{ $(filechk_$(1)); } > $(tmp-target);			\
+	if [ ! -r $@ ] || ! cmp -s $@ $(tmp-target); then	\
+		$(echo-cmd) '  UPD     $@';			\
+		mv -f $(tmp-target) $@;				\
+	fi
+endef
+
 ###
 # Replace >$< with >$$< to preserve $ when reloading the .cmd file
 # (needed for make)
diff --git a/tools/include/linux/kbuild.h b/tools/include/linux/kbuild.h
new file mode 100644
index 000000000000..e7be517aaaf6
--- /dev/null
+++ b/tools/include/linux/kbuild.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __LINUX_KBUILD_H
+#define __LINUX_KBUILD_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 "\"")
+
+#endif
diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
index ef2b1a47719c..e7bd835a56db 100644
--- a/tools/testing/selftests/kvm/Makefile.kvm
+++ b/tools/testing/selftests/kvm/Makefile.kvm
@@ -241,12 +241,14 @@ INSTALL_HDR_PATH = $(top_srcdir)/usr
 LINUX_HDR_PATH = $(INSTALL_HDR_PATH)/include/
 LINUX_TOOL_INCLUDE = $(top_srcdir)/tools/include
 LINUX_TOOL_ARCH_INCLUDE = $(top_srcdir)/tools/arch/$(ARCH)/include
+LINUX_TOOL_OUTPUT_ARCH_INCLUDE = $(OUTPUT)/include/$(ARCH)
 CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \
 	-Wno-gnu-variable-sized-type-not-at-end -MD -MP -DCONFIG_64BIT \
 	-fno-builtin-memcmp -fno-builtin-memcpy \
 	-fno-builtin-memset -fno-builtin-strnlen \
 	-fno-stack-protector -fno-PIE -fno-strict-aliasing \
 	-I$(LINUX_TOOL_INCLUDE) -I$(LINUX_TOOL_ARCH_INCLUDE) \
+	-I$(LINUX_TOOL_OUTPUT_ARCH_INCLUDE) \
 	-I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(ARCH) \
 	-I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
 ifeq ($(ARCH),s390)
@@ -256,6 +258,15 @@ ifeq ($(ARCH),x86)
 ifeq ($(shell echo "void foo(void) { }" | $(CC) -march=x86-64-v2 -x c - -c -o /dev/null 2>/dev/null; echo "$$?"),0)
 	CFLAGS += -march=x86-64-v2
 endif
+
+GEN_HDRS := $(OUTPUT)/include/$(ARCH)/generated/asm-tdx-offsets.h
+
+$(OUTPUT)/lib/$(ARCH)/asm-tdx-offsets.s: lib/$(ARCH)/asm-tdx-offsets.c
+	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -S $< -o $@
+
+$(GEN_HDRS): $(OUTPUT)/lib/$(ARCH)/asm-tdx-offsets.s
+	$(call filechk,offsets,__ASM_KVM_SELFTESTS_OFFSETS_H__)
+
 endif
 ifeq ($(ARCH),arm64)
 tools_dir := $(top_srcdir)/tools
diff --git a/tools/testing/selftests/kvm/lib/x86/asm-tdx-offsets.c b/tools/testing/selftests/kvm/lib/x86/asm-tdx-offsets.c
new file mode 100644
index 000000000000..ad56d99b56be
--- /dev/null
+++ b/tools/testing/selftests/kvm/lib/x86/asm-tdx-offsets.c
@@ -0,0 +1,28 @@
+#include <linux/kbuild.h>
+
+#include "tdx/tdcall.h"
+
+static inline void common(void)
+{
+	OFFSET(TDX_HYPERCALL_r10, tdx_hypercall_args, r10);
+	OFFSET(TDX_HYPERCALL_r11, tdx_hypercall_args, r11);
+	OFFSET(TDX_HYPERCALL_r12, tdx_hypercall_args, r12);
+	OFFSET(TDX_HYPERCALL_r13, tdx_hypercall_args, r13);
+	OFFSET(TDX_HYPERCALL_r14, tdx_hypercall_args, r14);
+	OFFSET(TDX_HYPERCALL_r15, tdx_hypercall_args, r15);
+
+	BLANK();
+	OFFSET(TDX_MODULE_rcx, tdx_module_output, rcx);
+	OFFSET(TDX_MODULE_rdx, tdx_module_output, rdx);
+	OFFSET(TDX_MODULE_r8, tdx_module_output, r8);
+	OFFSET(TDX_MODULE_r9, tdx_module_output, r9);
+	OFFSET(TDX_MODULE_r10, tdx_module_output, r10);
+	OFFSET(TDX_MODULE_r11, tdx_module_output, r11);
+}
+
+int main(void)
+{
+	common();
+
+	return 0;
+}
diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/tdcall.S b/tools/testing/selftests/kvm/lib/x86/tdx/tdcall.S
index c393a0fb35be..e98aa5178db9 100644
--- a/tools/testing/selftests/kvm/lib/x86/tdx/tdcall.S
+++ b/tools/testing/selftests/kvm/lib/x86/tdx/tdcall.S
@@ -1,18 +1,13 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /* Adapted from arch/x86/coco/tdx/tdcall.S */
 
+#include "generated/asm-tdx-offsets.h"
+
 /*
  * TDCALL is supported in Binutils >= 2.36, add it for older version.
  */
 #define tdcall		.byte 0x66,0x0f,0x01,0xcc
 
-#define TDX_HYPERCALL_r10 0 /* offsetof(struct tdx_hypercall_args, r10) */
-#define TDX_HYPERCALL_r11 8 /* offsetof(struct tdx_hypercall_args, r11) */
-#define TDX_HYPERCALL_r12 16 /* offsetof(struct tdx_hypercall_args, r12) */
-#define TDX_HYPERCALL_r13 24 /* offsetof(struct tdx_hypercall_args, r13) */
-#define TDX_HYPERCALL_r14 32 /* offsetof(struct tdx_hypercall_args, r14) */
-#define TDX_HYPERCALL_r15 40 /* offsetof(struct tdx_hypercall_args, r15) */
-
 /*
  * Bitmasks of exposed registers (with VMM).
  */
@@ -91,13 +86,6 @@ __tdx_hypercall:
 	pop %rbp
 	ret
 
-#define TDX_MODULE_rcx 0 /* offsetof(struct tdx_module_output, rcx) */
-#define TDX_MODULE_rdx 8 /* offsetof(struct tdx_module_output, rdx) */
-#define TDX_MODULE_r8 16 /* offsetof(struct tdx_module_output, r8) */
-#define TDX_MODULE_r9 24 /* offsetof(struct tdx_module_output, r9) */
-#define TDX_MODULE_r10 32 /* offsetof(struct tdx_module_output, r10) */
-#define TDX_MODULE_r11 40 /* offsetof(struct tdx_module_output, r11) */
-
 .globl __tdx_module_call
 .type __tdx_module_call, @function
 __tdx_module_call:
---8<---

Powered by blists - more mailing lists