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