Re: [PATCH v14 11/22] KVM: selftests: Set up TDX boot parameters region

[Ackerley Tng]

Peter Fang <peter.fang@xxxxxxxxx> writes:

> On Wed, Jul 22, 2026 at 11:13:16PM +0000, Lisa Wang wrote:
>> From: Sagi Shahar <sagis@xxxxxxxxxx>
>>
>> Allocate memory for TDX boot parameters and define the utility functions
>> necessary to fill this memory with the boot parameters.
>>
>> Co-developed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>> Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
>> Signed-off-by: Sagi Shahar <sagis@xxxxxxxxxx>
>> Signed-off-by: Lisa Wang <wyihan@xxxxxxxxxx>
>> ---
>>  .../selftests/kvm/include/x86/tdx/tdx_util.h       |  2 +
>>  tools/testing/selftests/kvm/lib/x86/processor.c    |  2 +
>>  tools/testing/selftests/kvm/lib/x86/tdx/tdx_util.c | 51 ++++++++++++++++++++++
>>  3 files changed, 55 insertions(+)
>>
>
> [ ... ]
>
>> +
>> +void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm)
>> +{
>> +	struct td_boot_parameters *params =
>> +		addr_gpa2hva(vm, TD_BOOT_PARAMETERS_GPA);
>> +	u32 cr4;
>> +
>> +	cr4 = kvm_get_default_cr4(vm->mmu.pgtable_levels);
>> +
>> +	/* TDX spec 11.6.2: CR4 bit MCE is fixed to 1 */
>> +	cr4 |= X86_CR4_MCE;
>> +
>> +	/* TDX spec 11.6.2: CR4 bit VMXE and SMXE are fixed to 0 */
>
>         This is now out of date (11.9.2 as of today)...
>         I wonder if it's better to just say something like: "Per the TDX
>         spec, ..."
>

Makes sense, thanks!

>> +	cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);
>> +
>> +	/* Set parameters! */
>> +	params->cr0 = kvm_get_default_cr0();
>> +	TEST_ASSERT(vm->mmu.pgd < (1ULL << 32),
>> +		    "PGD must be within 32-bit address space for 32-bit boot code");
>> +	params->cr3 = vm->mmu.pgd;
>> +	params->cr4 = cr4;
>> +	params->idtr.base = vm->arch.idt;
>> +	params->idtr.limit = kvm_get_default_idt_limit();
>> +	params->gdtr.base = vm->arch.gdt;
>
>         vm->arch.{idt,gdt} must also be 32-bit addressable here, just
>         like vm->mmu.pgd.
>

Something like this?

    TEST_ASSERT(params->gdtr.base < (1ULL << 32),
            "gdt must be within 32-bit address space for 32-bit boot code");


>> +	params->gdtr.limit = kvm_get_default_gdt_limit();
>> +
>> +	TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
>> +	TEST_ASSERT(params->cr3 != 0, "cr3 should not be 0");
>> +	TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");
>> +	TEST_ASSERT(params->gdtr.base != 0, "gdt base address should not be 0");
>> +	TEST_ASSERT(params->idtr.base != 0, "idt base address should not be 0");
>
>         Making sure that cr0/cr4 are simply non-zero feels kind of odd
>         to me. And cr3/gdtr/idtr all came from memory allocators with
>         non-zero minimum addresses.
>

I guess I intended these to be sanity checks that the caller didn't
leave these unset (defaulting to 0), for an early warning to the
selftest writer if something went wrong. I'd prefer to keep something
that would retain this early warning for the writer.

What would you replace this with, to provide the early warning, or would
you rather just remove it?

>> +}
>> +
>>  static struct kvm_tdx_capabilities *tdx_read_capabilities(struct kvm_vm *vm)
>>  {
>>  	static struct kvm_tdx_capabilities *tdx_cap;
>>
>> --
>> 2.55.0.229.g6434b31f56-goog
>>
>>