Re: [RFC PATCH v2 22/25] KVM: x86/mmu: Refactor kvm_mmu_invlpg() to allow skipping the gva flush

[Yosry Ahmed]

On Mon, Jul 27, 2026 at 8:38 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Fri, Jul 24, 2026, Yosry Ahmed wrote:
> > > > > > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > > > > > index 65c35ed8f4a01..3feb75732f7b4 100644
> > > > > > > --- a/arch/x86/kvm/mmu/mmu.c
> > > > > > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > > > > > @@ -6615,15 +6615,15 @@ static void kvm_mmu_invalidate_addr_in_root(struct kvm_vcpu *vcpu,
> > > > > > >         write_unlock(&vcpu->kvm->mmu_lock);
> > > > > > >  }
> > > > > > >
> > > > > > > -void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > > > -                            u64 addr, unsigned long roots)
> > > > > > > +static void __kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > > > +                                     u64 addr, unsigned long roots, bool flush_gva)
> > > > > > >  {
> > > > > > >         int i;
> > > > > > >
> > > > > > >         WARN_ON_ONCE(roots & ~KVM_MMU_ROOTS_ALL);
> > > > > > >
> > > > > > >         /* It's actually a GPA for vcpu->arch.guest_mmu.  */
> > > > > > > -       if (mmu != &vcpu->arch.guest_mmu) {
> > > > > > > +       if (flush_gva && mmu != &vcpu->arch.guest_mmu) {
> > > > > > >                 /* INVLPG on a non-canonical address is a NOP according to the SDM.  */
> > > > > > >                 if (is_noncanonical_invlpg_address(addr, vcpu))
> > > > > > >                         return;
> > > > > > > @@ -6642,9 +6642,15 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > > >                         kvm_mmu_invalidate_addr_in_root(vcpu, mmu, addr, mmu->prev_roots[i].hpa);
> > > > > > >         }
> > > > > > >  }
> > > > > > > +
> > > > > > > +void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
> > > > > > > +                              u64 addr, unsigned long roots)
> > > > > >
> > > > > > Rather than kvm_mmu_invalidate_addr() for the wrapper, what if we call this
> > > > > > kvm_mmu_invalidate_gva()?  And then kvm_mmu_invlpg_gva().  Then we don't need
> > > > > > to have the "in_root" version to a quad-underscores helper, and IMO it's more
> > > > > > obvious what's different between the one-line wrappers and the inner helpers.
> > > > >
> > > > > Hrm, or maybe I'm not understanding what "flush_gva" means.  At first glance, I
> > > > > was assuming you were using it to differentiate between GVA and GPA, but IIUC,
> > > > > it's literally skipping the flush for the current context, which just so happens
> > > > > to be done only for GVAs.  I'd still like to avoid the "in_root" helper, if at
> > > > > all possible.
> > > >
> > > > Yes, it's skipping the TLB flush that is only needed for GVAs. The
> > > > alternative I had in mind (but thought was worse) was to refactor the
> > > > TLB flush part out of kvm_mmu_invalidate_addr() (or
> > > > __kvm_mmu_invalidate_addr()) in this patch instead of adding a boolean,
> > > > but this still requires adding a wrapper and the possibility of a
> > > > quad-underscore helper.
> > > >
> > > > What's the main objection to kvm_mmu_invalidate_addr_in_root()?
> > >
> > > I don't love the __kvm_mmu_invalidate_addr() => kvm_mmu_invalidate_addr_in_root()
> > > callchain.  It's not at all obvious that the in_root() helper shouldn't be called
> > > directly.  I don't hate it, but I do think we need better clarity on what all this
> > > is doing.
> > >
> > > E.g. when looking at __kvm_inject_emulated_page_fault(), since it hardcodes a
> > > single root, it's a bit headscratching to use kvm_mmu_invalidate_addr() instead
> > > of kvm_mmu_invalidate_addr_in_root.
> >
> > Coming back to this, I agree it's confusing. I think this can be fixed
> > with a better name though. Looking at
> > kvm_mmu_invalidate_addr_in_root() (or __kvm_mmu_invalidate_addr() in
> > current code), seems like what it does is find SPTEs for that address,
> > sync them, and flush the TLB if needed.
> >
> > So maybe mmu_sync_addr_sptes() or mmu_sync_and_flush_addr_sptes()?
>
> kvm_mmu_sync_addr()?  The "sptes" part is implied in things like kvm_sync_page()
> and mmu_sync_children().

Sounds good. I assume you're okay with keeping everything else the
same way? I assume you didn't like any of the alternatives?