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

[Peter Fang <peter.fang@intel.com>]

On Tue, Sep 08, 2026 at 12:18:38PM -0700, Ackerley Tng wrote:
> 
> >> +	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");

Yep, looks good to me.

> 
> 
> >> +	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?

Hmm... In earlier versions this was:

        params->cr0 = sregs->cr0;
        params->cr4 = sregs->cr4;

        TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
        TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");

Which made more sense because perhaps @sregs could be sanity checked.
But now the assignments are all in the same function:

        cr4 = kvm_get_default_cr4(vm->mmu.pgtable_levels);
        cr4 |= X86_CR4_MCE;
        cr4 &= ~(X86_CR4_VMXE | X86_CR4_SMXE);

        params->cr0 = kvm_get_default_cr0();
        params->cr4 = cr4;

        TEST_ASSERT(params->cr0 != 0, "cr0 should not be 0");
        TEST_ASSERT(params->cr4 != 0, "cr4 should not be 0");

This makes much less sense to me. It looks like sanity checking
kvm_get_default_{cr0,cr4}().

So I'd remove the two asserts for cr0/cr4. Keeping the ones for
cr3/gdtr/idtr is probably fine.

>