Re: [PATCH 08/12] KVM: selftests: Use 'leaf' instead of hugepage to describe EPT entries

Yosry Ahmed
2025-10-13T22:25:28Z

On Mon, Oct 13, 2025 at 02:41:56PM -0700, Sean Christopherson wrote:
> On Wed, Oct 01, 2025, Yosry Ahmed wrote:
> > From: Yosry Ahmed <yosryahmed@google.com>
> >
> > The assertions use 'hugepage' to describe a terminal EPT entry, but
> > 'leaf' is more accruate as a PG_LEVEL_4K EPT entry is a leaf but not a
> > hugepage.
>
> Yes, it's more accurate, but also less precise. I'm guessing the assert message
> and comment talked about hugepages because that's the type of mappings that
> caused problems at the time.

Given that it refers to PG_LEVEL_4K entries too, I wouldn't call it less
precise. All callers actually create 4K mappings so it is never actually
a hugepage in the current context :D

>
> Ah, actually, I bet the code was copy+pasted from virt_create_upper_pte(), in
> which case the assumptions about wanting to create a hupage are both accurate
> and precise.
>
> > The distincion will be useful in coming changes that will pass
> > the value around and 'leaf' is clearer than hugepage or page_size.
>
> What value?

'leaf'. The following changes will pass 'leaf' in as a boolean instead
of checking 'current_level == target_level' here. So passing in
'hugepage' would be inaccurate, and 'page_size' is not as clear (but
still works).

>
> > Leave the EPT bit named page_size to keep it conforming to the manual.
> >
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@linux.dev>
> > ---
> > tools/testing/selftests/kvm/lib/x86/vmx.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/lib/x86/vmx.c b/tools/testing/selftests/kvm/lib/x86/vmx.c
> > index 04c4b97bcd1e7..673756b27e903 100644
> > --- a/tools/testing/selftests/kvm/lib/x86/vmx.c
> > +++ b/tools/testing/selftests/kvm/lib/x86/vmx.c
> > @@ -380,15 +380,15 @@ static void nested_create_pte(struct kvm_vm *vm,
> > pte->address = vm_alloc_page_table(vm) >> vm->page_shift;
> > } else {
> > /*
> > - * Entry already present. Assert that the caller doesn't want
> > - * a hugepage at this level, and that there isn't a hugepage at
> > - * this level.
> > + * Entry already present. Assert that the caller doesn't want a
> > + * leaf entry at this level, and that there isn't a leaf entry
> > + * at this level.
> > */
> > TEST_ASSERT(current_level != target_level,
> > - "Cannot create hugepage at level: %u, nested_paddr: 0x%lx",
> > + "Cannot create leaf entry at level: %u, nested_paddr: 0x%lx",
> > current_level, nested_paddr);
> > TEST_ASSERT(!pte->page_size,
> > - "Cannot create page table at level: %u, nested_paddr: 0x%lx",
> > + "Leaf entry already exists at level: %u, nested_paddr: 0x%lx",
>
> This change is flat out wrong. The existing PRESENT PTE _might_ be a 4KiB leaf
> entry, but it might also be an existing non-leaf page table.

Hmm if pte->page_size is true then it has to be a leaf page table,
right?

If it's an existing non-leaf page table we shouldn't fail, the assertion
here is when we try to override a leaf page table IIUC.

>
> Instead of hacking on the nested code, can we instead tweak __virt_pg_map() to
> work with nested TDP? At a glance, it's already quite close, e.g. "just" needs
> to be taught about EPT RWX bits and allow the call to pass in the root pointer.

That would be ideal, I'll take a look. In case I don't have time for
that unification, can this be a follow-up change?

>
> > current_level, nested_paddr);
> > }
> > }
> > --
> > 2.51.0.618.g983fd99d29-goog
> >