Re: [PATCH 17/22] KVM: x86/mmu: pull struct kvm_pagewalk out of struct kvm_mmu

[Yosry Ahmed]

On Tue, May 19, 2026 at 12:25:29PM +0200, Paolo Bonzini wrote:
> Il mer 13 mag 2026, 23:36 Yosry Ahmed <yosry@xxxxxxxxxx> ha scritto:
> >
> > However, I can't immediately tell what vcpu->arch.cpu_walk is doing
> > either (compared to vcpu->arch.tdp_walk), so maybe the names can be
> > improved? If these walks are tied to these MMUs, maybe name them as
> > such (e.g. root_pg_walk and guest_pg_walk)?
> 
> No, cpu_walk is always GVA->(n)GPA and tdp_walk is the optional
> nGPA->GPA stage. While there is a 1:1 mapping from a struct kvm_mmu to
> kvm_pagewalk when doing shadow paging, for *emulation* purposes
> cpu_walk is used for both L1 and L2 and It replaces the
> vcpu->arch.walk_mmu pointer from the old code (which led to either
> root_mmu or nested_mmu). In fact the main change in the series is the
> removal of walk_mmu, with cpu_walk always representing the CR0/CR3/CR4
> page tables.
> 
> I could call them gva_walk and ngpa_walk, but I think the current name
> are also self-explanatory (especially once you understand that
> walk_mmu is no more and cpu_walk can be used for both L1 and L2). The
> confusion comes more if you look at the walkers from the POV of struct
> kvm_mmu. Which leads to the other half...

I think the source of my confusion is that when TDP is enabled
'root_mmu' is used for tracking the TDP for L1 (GPA->HPA), as well as
walking L1's own page tables (GVA->GPA).

I guess the main point of the series (in the context of root_mmu) is to
stop doing that and separate the former (building GPA->HPA) from the
latter (GVA->GPA).

So after this series:
- root_mmu only tracks building TDP for L1, or shadowing L1's page
  tables without TDP.
- guest_mmu is only used to shadow L1's TDP for L2.
- cpu_walk is used to walk L1's or L2's CR3-based page tables (i.e.
  replaces part of old root_mmu and nested_mmu).
- tdp_walk is used to walk L1's TDPs for L2 (i.e. replaces part of
  old guest_mmu). I guess tdp_walk is always tied to guest_mmu?

Is my understanding correct? I feel like I missed something.

That being said, I still don't like cpu_walk tbh. Maybe cr3_walk as
suggested by Kishen, or more explicitly gva_walk or sth. The CPU walks
both CR3-based page tables as well as TDP page tables, so I am not sure
why use 'cpu' here?

> 
> > I also think root_mmu and guest_mmu could still use some improvement
> > but that's probably outside the scope of this series. These are
> > essentially L1 MMU and L2 MMU, right? Maybe just mmu and nested_mmu
> > could work? But I am not sure if we can reclaim the nested_mmu name,
> > it's gonna screw with anyone doing backports :/
> 
> And even more important vcpu->arch.mmu is the pointer to either
> root_mmu or guest_mmu. I wouldn't reclaim either mmu or nested_mmu.
> 
> guest_mmu is not L2 MMU if L1 does not use two-dimensional paging, so
> l1_mmu and l2_mmu does not cut it entirely. And root_mmu can be either
> GVA->HPA or GPA->HPA, therefore applying the idea above (e.g., gpa_mmu
> and ngpa_mmu) would not work well.
> 
> I suppose guest_mmu could be ngpa_mmu or shadow_tdp_mmu, but another

I was going to suggest shadow_tdp_mmu as I was reading this, I like that
name. Still can't think of a good name for root_mmu.

I thought about a union of two names, shadow_mmu and tdp_mmu, but I
suspect most code paths would apply equally to both?

> possibility/refactoring would be to adjust the code and call the two
> MMUs direct_mmu and shadow_mmu. I haven't looked into what this means
> for the code but it would definitely make for the clearest naming.

IIUC direct_mmu would only be used for L1 TDP, and shadow_mmu would for
either shadowing L1 page tables (with TDP=0) or shadowing L1's TDP for
L2?

The naming sounds nice, but as you say below, I don't know how that
would work code-wise.

> Having direct_mmu->w be NULL would be nice, so it seems promising but
> not something I was going to do soon (the *real* reason to submit this
> patch now is to get rid of is_executable_pte() half-assed support for
> MBEC/GMET, and that's where I stopped).
> 
> Even with the small complication that CR0.PG=0 is a direct mapping but
> would use shadow_mmu, it's still a GVA->HPA mapping and thus pretty
> understandable.
> 
> tl;dr: naming is hard so I tried to change as little as possible in
> this respect...

Makes sense. Thanks for bearing with me.