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

[Binbin Wu]

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

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.