Re: [PATCH v4 21/21] KVM: selftests: Test READ=>WRITE dirty logging behavior for shadow MMU

Yosry Ahmed
2026-01-08T20:24:27Z

On Thu, Jan 08, 2026 at 10:31:22AM -0800, Sean Christopherson wrote:
> On Thu, Jan 08, 2026, Yosry Ahmed wrote:
> > On Thu, Jan 08, 2026 at 08:32:44AM -0800, Sean Christopherson wrote:
> > > On Fri, Jan 02, 2026, Yosry Ahmed wrote:
> > > diff --git a/tools/testing/selftests/kvm/lib/x86/processor.c b/tools/testing/selftests/kvm/lib/x86/processor.c
> > > index ab869a98bbdc..fab18e9be66c 100644
> > > --- a/tools/testing/selftests/kvm/lib/x86/processor.c
> > > +++ b/tools/testing/selftests/kvm/lib/x86/processor.c
> > > @@ -390,6 +390,13 @@ static uint64_t *__vm_get_page_table_entry(struct kvm_vm *vm,
> > > return virt_get_pte(vm, mmu, pte, vaddr, PG_LEVEL_4K);
> > > }
> > >
> > > +uint64_t *tdp_get_pte(struct kvm_vm *vm, uint64_t l2_gpa)
> >
> > nested_paddr is the name used by tdp_map(), maybe use that here as well
> > (and in the header)?
>
> Oh hell no :-) nested_paddr is a terrible name (I was *very* tempted to change
> it on the fly, but restrained myself). "nested" is far too ambigous, e.g. without
> nested virtualization, "nested_paddr" arguably refers to _L1_ physical addresses
> (SVM called 'em Nested Page Tables after all).

That's fair, I generally like consistency to a fault :)

>
> > > + int level = PG_LEVEL_4K;
> > > +
> > > + return __vm_get_page_table_entry(vm, &vm->stage2_mmu, l2_gpa, &level);
> > > +}
> > > +
> > > uint64_t *vm_get_pte(struct kvm_vm *vm, uint64_t vaddr)
> > > {
> > > int level = PG_LEVEL_4K;
> > [..]
> > > @@ -133,35 +220,50 @@ static void test_dirty_log(bool nested_tdp)
> > >
> > > /* Add an extra memory slot for testing dirty logging */
> > > vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS,
> > > - GUEST_TEST_MEM,
> > > + TEST_MEM_BASE,
> > > TEST_MEM_SLOT_INDEX,
> > > TEST_MEM_PAGES,
> > > KVM_MEM_LOG_DIRTY_PAGES);
> > >
> > > /*
> > > - * Add an identity map for GVA range [0xc0000000, 0xc0002000). This
> > > + * Add an identity map for GVA range [0xc0000000, 0xc0004000). This
> > > * affects both L1 and L2. However...
> > > */
> > > - virt_map(vm, GUEST_TEST_MEM, GUEST_TEST_MEM, TEST_MEM_PAGES);
> > > + virt_map(vm, TEST_MEM_BASE, TEST_MEM_BASE, TEST_MEM_PAGES);
> > >
> > > /*
> > > - * ... pages in the L2 GPA range [0xc0001000, 0xc0003000) will map to
> > > - * 0xc0000000.
> > > + * ... pages in the L2 GPA ranges [0xc0001000, 0xc0002000) and
> > > + * [0xc0003000, 0xc0004000) will map to 0xc0000000 and 0xc0001000
> > > + * respectively.
> >
> > Are these ranges correct? I thought L2 GPA range [0xc0002000,
> > 0xc0004000) will map to [0xc0000000, 0xc0002000).
>
> Gah, no. I looked at the comments after changing things around, but my eyes had
> glazed over by that point.
>
> > Also, perhaps it's better to express those in terms of the macros?
> >
> > L2 GPA range [TEST_MEM_ALIAS_BASE, TEST_MEM_ALIAS_BASE + 2*PAGE_SIZE)
> > will map to [TEST_MEM_BASE, TEST_MEM_BASE + 2*PAGE_SIZE)?
>
> Hmm, no, at some point we need to concretely state the addresses, so that people
> debugging this know what to expect, i.e. don't have to manually compute the
> addresses from the macros in order to debug.

I was trying to avoid a situation where the comment gets out of sync
with the macros in a way that gets confusing. Maybe reference both if
it's not too verbose?

/*
* ... pages in the L2 GPA range [0xc0002000, 0xc0004000) at
* TEST_MEM_ALIAS_BASE will map to [[0xc0000000, 0xc0002000) at
* TEST_MEM_BASE.
*/

>
> > > *
> > > * When TDP is disabled, the L2 guest code will still access the same L1
> > > * GPAs as the TDP enabled case.
> > > + *
> > > + * Set the Dirty bit in the PTEs used by L2 so that KVM will create
> > > + * writable SPTEs when handling read faults (if the Dirty bit isn't
> > > + * set, KVM must intercept the next write to emulate the Dirty bit
> > > + * update).
> > > */
> > > if (nested_tdp) {
> > > + vm_vaddr_t gva0 = TEST_GUEST_ADDR(TEST_MEM_ALIAS_BASE, 0);
> > > + vm_vaddr_t gva1 = TEST_GUEST_ADDR(TEST_MEM_ALIAS_BASE, 1);
> >
> > Why are these gvas? Should these be L2 GPAs?
>
> Pure oversight.
>
> > Maybe 'uint64_t l2_gpa0' or 'uint64_t nested_paddr0'?
>
> For better of worse, vm_paddr_t is the typedef in selftests. Hmm, if/when we go
> with David M's proposal to switch to u64 (from e.g. uint64_t), it'd probably be
> a good time to switch to KVM's gva_t and gpa_t as well.

vm_paddr_t is fine too, I am just against using vm_vaddr_t. tdp_map()
takes in uint64_t for the GPAs, which is why I suggested uint64_t here.

>
> > Also maybe add TEST_ALIAS_GPA() macro to keep things consistent?
>
> Ya, then the line lengths are short enough to omit the local variables. How's
> this look?

Looks good, thanks!

>
> /*
> * ... pages in the L2 GPA address range [0xc0002000, 0xc0004000) will
> * map to [0xc0000000, 0xc0002000) when TDP is enabled (for L2).
> *
> * When TDP is disabled, the L2 guest code will still access the same L1
> * GPAs as the TDP enabled case.
> *
> * Set the Dirty bit in the PTEs used by L2 so that KVM will create
> * writable SPTEs when handling read faults (if the Dirty bit isn't
> * set, KVM must intercept the next write to emulate the Dirty bit
> * update).
> */
> if (nested_tdp) {
> tdp_identity_map_default_memslots(vm);
> tdp_map(vm, TEST_ALIAS_GPA(0), TEST_GPA(0), PAGE_SIZE);
> tdp_map(vm, TEST_ALIAS_GPA(1), TEST_GPA(1), PAGE_SIZE);
>
> *tdp_get_pte(vm, TEST_ALIAS_GPA(0)) |= PTE_DIRTY_MASK(&vm->stage2_mmu);
> *tdp_get_pte(vm, TEST_ALIAS_GPA(1)) |= PTE_DIRTY_MASK(&vm->stage2_mmu);
> } else {
> *vm_get_pte(vm, TEST_GVA(0)) |= PTE_DIRTY_MASK(&vm->mmu);
> *vm_get_pte(vm, TEST_GVA(1)) |= PTE_DIRTY_MASK(&vm->mmu);
> }