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

[Sagi Shahar <sagis@google.com>]

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.

> A side topic is when developers should provide comments for the uses of
> "__packed". Is it always preferred a comment for it or a developer can save
> the comment for obvious cases, e.g, the structure is defined according to
> some hardware layout?
>
> >
> >> +    uint32_t cr0;
> >> +    uint32_t cr3;
> >> +    uint32_t cr4;
> >> +    struct td_boot_parameters_dtr gdtr;
> >> +    struct td_boot_parameters_dtr idtr;
> >> +    struct td_per_vcpu_parameters per_vcpu[];
> >> +};
> >> +
> ...
>
> >> diff --git a/tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S b/tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S
> >> new file mode 100644
> >> index 000000000000..c8cbe214bba9
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/kvm/lib/x86/tdx/td_boot.S
> >> @@ -0,0 +1,100 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +
> >> +#include "tdx/td_boot_asm.h"
> >> +
> >> +/* Offsets for reading struct td_boot_parameters. */
> >> +#define TD_BOOT_PARAMETERS_CR0         0
> >> +#define TD_BOOT_PARAMETERS_CR3         4
> >> +#define TD_BOOT_PARAMETERS_CR4         8
> >> +#define TD_BOOT_PARAMETERS_GDT         12
> >> +#define TD_BOOT_PARAMETERS_IDT         18
> >> +#define TD_BOOT_PARAMETERS_PER_VCPU    24
> >> +
> >> +/* Offsets for reading struct td_per_vcpu_parameters. */
> >> +#define TD_PER_VCPU_PARAMETERS_ESP_GVA     0
> >> +#define TD_PER_VCPU_PARAMETERS_LJMP_TARGET 4
> >> +
> >> +#define SIZEOF_TD_PER_VCPU_PARAMETERS      10
> > Please figure out how to replicate the functionality of the kernel's OFFSET()
> > macro from  include/linux/kbuild.h, I have zero desire to maintain open coded
> > offset values.
> >