Re: [PATCH v3 06/16] KVM: selftests: Introduce struct kvm_mmu

[Yosry Ahmed]

On Tue, Dec 23, 2025 at 02:29:23PM -0800, Sean Christopherson wrote:
> On Thu, Nov 27, 2025, Yosry Ahmed wrote:
> > In preparation for generalizing the virt mapping functions to work with
> > TDP page tables, introduce struct kvm_mmu. This struct currently only
> > holds the root GPA and number of page table levels. Parameterize virt
> > mapping functions by the kvm_mmu, and use the root GPA and page table
> > levels instead of hardcoding vm->pgd and vm->pgtable_levels.
> > 
> > There's a subtle change here, instead of checking that the parent
> > pointer is the address of the vm->pgd, check if the value pointed at by
> > the parent pointer is the root GPA (i.e. the value of vm->pgd in this
> > case). No change in behavior expected.
> > 
> > Opportunistically, switch the ordering of the checks in the assertion in
> > virt_get_pte(), as it makes more sense to check if the parent PTE is the
> > root (in which case, not a PTE) before checking the present flag.
> > 
> > vm->arch.mmu is dynamically allocated to avoid a circular dependency
> > chain if kvm_util_arch.h includes processor.h for the struct definition:
> > kvm_util_arch.h -> processor.h -> kvm_util.h -> kvm_util_arch.h
> > 
> > No functional change intended.
> > 
> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@xxxxxxxxx>
> > ---
> >  .../selftests/kvm/include/x86/kvm_util_arch.h |  4 ++
> >  .../selftests/kvm/include/x86/processor.h     |  8 ++-
> >  .../testing/selftests/kvm/lib/x86/processor.c | 61 +++++++++++++------
> >  3 files changed, 53 insertions(+), 20 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/kvm/include/x86/kvm_util_arch.h b/tools/testing/selftests/kvm/include/x86/kvm_util_arch.h
> > index 972bb1c4ab4c..d8808fa33faa 100644
> > --- a/tools/testing/selftests/kvm/include/x86/kvm_util_arch.h
> > +++ b/tools/testing/selftests/kvm/include/x86/kvm_util_arch.h
> > @@ -10,6 +10,8 @@
> >  
> >  extern bool is_forced_emulation_enabled;
> >  
> > +struct kvm_mmu;
> > +
> >  struct kvm_vm_arch {
> >  	vm_vaddr_t gdt;
> >  	vm_vaddr_t tss;
> > @@ -19,6 +21,8 @@ struct kvm_vm_arch {
> >  	uint64_t s_bit;
> >  	int sev_fd;
> >  	bool is_pt_protected;
> > +
> > +	struct kvm_mmu *mmu;
> 
> No, put kvm_mmu in common code and create kvm_vm.mmu.  This makes the "mmu" object
> a weird copy of state that's already in kvm_vm (pgd, pgd_created, and pgtable_levels),
> and more importantly makes it _way_ to easy to botch the x86 MMU code (speaking
> from first hand experience), e.g. due to grabbing vm->pgtable_levels instead of
> the mmu's version.  I don't see an easy way to _completely_ guard against goofs
> like that, but it's easy-ish to audit code the code for instance of "vm->mmu.",
> and adding a common kvm_mmu avoids the weird duplicate code.

Do you mean move pgd, pgd_created, and pgtable_levels into kvm_mmu? If
yes, that makes sense to me and is obviously an improvement over what
it's in this patch.

I didn't immediately make the connection, but in hindsight it's obvious
that having some of the state in kvm_vm_arch and some in kvm_vm is
fragile.