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

[Yosry Ahmed]

> > > > > 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()?

I think this would make it obvious it is not a direct alternative to
kvm_mmu_invalidate_addr() that just operates on a single 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.

I wouldn't say it's wrong. It syncs more roots than needed, yes, and I
did document that in the comment. I didn't want to spend time
optimizing this because it only matters if NPT=0 in the host (and
probably L1 will only use INVLPGA if it also has NPT=0).

I think KVM generally over-syncs in multiple cases. For example,
emulating INVVPID on a single address flushes the entire root (see
handle_invvpid()), and I don't think anyone cared enough to fix that.
Also, emulating INVLPG and INVPCID syncs all roots, arguably it should
only sync the ones' matching the vCPUs current guest_mode?

> If we can figure out an elegant way to
> express and handle that, it should naturally handle the "flush GVA" aspect.

Hmm how? I think the way to properly do this is to filter roots based
on role.guest_mode, like kvm_mmu_free_guest_mode_roots(). I'd probably
refactor that logic out of kvm_mmu_free_guest_mode_roots() and reuse
it if we go this route. But this doesn't really help with the "flush
GVA" aspect.

One way to actually tackle both is to pass "guest_mode" to
kvm_invalidate_mmu_addr() as a boolean (yuck). Then, it can do the
root filtering inside kvm_invalidate_mmu_addr(). We can then plump
guest_mode into flush_tlb_gva, and handle flushing L1's ASID vs L2's
ASID completely within flush_tlb_gva.

But then we'll have to figure out what's the right value of that
boolean to pass from all callers of kvm_invalidate_mmu_addr() and
there's more things we can get wrong.