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

[Reinette Chatre <reinette.chatre@intel.com>]

On 8/15/25 5:22 PM, Sean Christopherson wrote:
> 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.
> 

Thank you very much Sean. So much cleaner than what I came up with. I need to
investigate why those includes did not seem to work for me.

Reinette