Message-ID: <aQN0Qg24tMQ9ckUT@google.com>
Date: Thu, 30 Oct 2025 07:20:50 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Reinette Chatre <reinette.chatre@...el.com>
Cc: Sagi Shahar <sagis@...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>, Binbin Wu <binbin.wu@...ux.intel.com>, 
	Oliver Upton <oliver.upton@...ux.dev>, "Pratik R. Sampat" <pratikrajesh.sampat@....com>, 
	Ira Weiny <ira.weiny@...el.com>, Chao Gao <chao.gao@...el.com>, 
	Chenyi Qiang <chenyi.qiang@...el.com>, linux-kernel@...r.kernel.org, kvm@...r.kernel.org
Subject: Re: [PATCH v12 08/23] KVM: selftests: Define structs to pass
 parameters to TDX boot code

On Wed, Oct 29, 2025, Reinette Chatre wrote:
> Hi Sagi,
> 
> On 10/28/25 2:20 PM, Sagi Shahar wrote:
> > TDX registers are inaccessible to KVM. Therefore we need a different
> > mechanism to load boot parameters for TDX code. TDX boot code will read
> > the registers values from memory and set the registers manually.
> > 
> > This patch defines the data structures used to communicate between c
> > code and the TDX assembly boot code which will be added in a later
> > patch.
> > 
> 
> (sidenote: I do not know what the bar for this work is so I'll defer
> comments related to local customs like using "we" and "this patch" in
> changelog)

The same as KVM x86, which follows the same rules as the tip tree, with a few
intentional differences.  By all means, call out those things, it'll save me the
effort :-)

Documentation/process/maintainer-kvm-x86.rst

> 
> > Use kbuild.h to expose the offsets into the structs from c code to
> > assembly code.
> > 
> 
> 
> > diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> > index 148d427ff24b..5e809064ff1c 100644
> > --- a/tools/testing/selftests/kvm/Makefile.kvm
> > +++ b/tools/testing/selftests/kvm/Makefile.kvm
> 
> ...
> 
> > @@ -328,18 +336,28 @@ $(LIBKVM_C_OBJ): $(OUTPUT)/%.o: %.c $(GEN_HDRS)
> >  $(LIBKVM_S_OBJ): $(OUTPUT)/%.o: %.S $(GEN_HDRS)
> >  	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
> >  
> > +$(LIBKVM_ASM_DEFS_OBJ): $(OUTPUT)/%.s: %.c FORCE
> > +	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -S $< -o $@
> > +
> >  # 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)/include/x86/tdx/td_boot_offsets.h: $(OUTPUT)/lib/x86/tdx/td_boot_offsets.s FORCE
> > +	$(call filechk,offsets,__TDX_BOOT_OFFSETS_H__)

Presumably this needs to be guarded so that it's x86-only.  I can't tell for sure
as there are other problems in this series of a similar nature that prevent me from
getting far enough to see.  Please build test on at least one other architecture
before sending the next version.

lib/kvm_util.c:7:10: fatal error: tdx/tdx_util.h: No such file or directory
    7 | #include "tdx/tdx_util.h"
      |          ^~~~~~~~~~~~~~~~
compilation terminated.


If possible, I would also really like to see these programatically defined, e.g.
something like (I have no idea if this is remotely valid syntax):

  $(OUTPUT)/$(TEST_GEN_HEADERS): $(OUTPUT)/%.s FORCE
     $(call filechk,offsets,__%_h__)


> Some folks prefer to keep build output separate and may build tests using a command
> line like:
> 	make O=<output dir> TARGETS=kvm -C tools/testing/selftests

Ya, I exclusively build that way.

> This is a valid usage and will result in td_boot_offsets.h placed in <output dir> that
> is not covered by current include path. A build with above command line thus fails:
> 
> lib/x86/tdx/td_boot.S:4:10: fatal error: tdx/td_boot_offsets.h: No such file or directory
>     4 | #include "tdx/td_boot_offsets.h"
>       |          ^~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> 
> 
> Something like below may be needed to add the output directory to the include path:
> 
> diff --git a/tools/testing/selftests/kvm/Makefile.kvm b/tools/testing/selftests/kvm/Makefile.kvm
> index 2f49c8965df9..98bc40a7f069 100644
> --- a/tools/testing/selftests/kvm/Makefile.kvm
> +++ b/tools/testing/selftests/kvm/Makefile.kvm
> @@ -262,7 +262,7 @@ CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \
>  	-fno-stack-protector -fno-PIE -fno-strict-aliasing \
>  	-I$(LINUX_TOOL_INCLUDE) -I$(LINUX_TOOL_ARCH_INCLUDE) \
>  	-I$(LINUX_HDR_PATH) -Iinclude -I$(<D) -Iinclude/$(ARCH) \
> -	-I ../rseq -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES)
> +	-I ../rseq -I.. -I$(OUTPUT)/include/$(ARCH) $(EXTRA_CFLAGS) $(KHDR_INCLUDES)

Hrm, ya, though I assume we want to define e.g. KVM_GEN_HDRS so that they can be
added to the csope.  Note, ARM already has some generated header stuff, but the
generated code comes from outside of KVM selftests, so we'll want to make sure to
avoid a collision with GEN_HDRS, thus the KVM_ prefix.

	ifeq ($(ARCH),arm64)
	tools_dir := $(top_srcdir)/tools
	arm64_tools_dir := $(tools_dir)/arch/arm64/tools/

	ifneq ($(abs_objdir),)
	arm64_hdr_outdir := $(abs_objdir)/tools/
	else
	arm64_hdr_outdir := $(tools_dir)/
	endif

	GEN_HDRS := $(arm64_hdr_outdir)arch/arm64/include/generated/
	CFLAGS += -I$(GEN_HDRS)

	$(GEN_HDRS): $(wildcard $(arm64_tools_dir)/*)
		$(MAKE) -C $(arm64_tools_dir) OUTPUT=$(arm64_hdr_outdir)
	endif

Powered by blists - more mailing lists