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

[Sean Christopherson <seanjc@google.com>]

On Thu, Jul 23, 2026, Yosry Ahmed wrote:
> On Wed, Jul 22, 2026 at 05:56:19PM -0700, Sean Christopherson wrote:
> > On Wed, Jul 22, 2026, Sean Christopherson wrote:
> > > On Tue, Jun 16, 2026, Yosry Ahmed wrote:
> > > > Refactor helpers out of kvm_mmu_invalidate_addr() and kvm_mmu_invlpg()
> > > > that take in an extra argument to skip the GVA flush.
> > > > 
> > > > This will be used when invalidating GVAs in a different context than the
> > > > correct one (i.e.  invalidating an L2 GVA from L1), so flushing the
> > > > current context would flush the wrong TLB entries.
> > > > 
> > > > No functional change intended.
> > > > 
> > > > Signed-off-by: Yosry Ahmed <yosry@kernel.org>
> > > > ---
> > > >  arch/x86/kvm/mmu/mmu.c | 23 +++++++++++++++++------
> > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > 
> > > > 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.

Hmm, and arguably, the way invlpga_interception() handles the ASID is flat out
wrong.  KVM doesn't need to flush *all* roots, rather it needs to flush L1 roots
for ASID=0, and L2 roots for ASID!=0.  If we can figure out an elegant way to
express and handle that, it should naturally handle the "flush GVA" aspect.

Actually, isn't there a pre-existing over-flush when handling kvm_mmu_invpcid_gva()?
Oof, and a missed flush?

To fix the over-flush, I think we want this?

diff --git arch/x86/kvm/mmu/mmu.c arch/x86/kvm/mmu/mmu.c
index 6c13da942bfc..7f3e0eb33b29 100644
--- arch/x86/kvm/mmu/mmu.c
+++ arch/x86/kvm/mmu/mmu.c
@@ -6672,7 +6672,8 @@ void kvm_mmu_invalidate_addr(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
                if (is_noncanonical_invlpg_address(addr, vcpu))
                        return;
 
-               kvm_x86_call(flush_tlb_gva)(vcpu, addr);
+               if (roots & KVM_MMU_ROOT_CURRENT)
+                       kvm_x86_call(flush_tlb_gva)(vcpu, addr);
 
                if (tdp_enabled)
                        return;


And that highlights the missed flush: if the PCID isn't the current PCID, then
flush_tlb_gva() neglects to flush the hardware TLB for the target PCID, which
could leave a stale entry in the TLB if the guest switches to the new PCID with
MOV CR3 + X86_CR3_PCID_NOFLUSH.