Re: [PATCH] KVM: x86/tdp_mmu: Trigger the callback only when an interesting change

[Yan Zhao]

On Tue, Oct 08, 2024 at 12:15:14PM -0700, Sean Christopherson wrote:
> On Fri, Sep 27, 2024, Sean Christopherson wrote:
> > On Fri, Sep 27, 2024, Sean Christopherson wrote:
> > > On Fri, Sep 27, 2024, Sean Christopherson wrote:
> 
> ...
> 
> > > > Oh, right, I forgot about that.  I'll tweak the changelog to call that out before
> > > > posting.  Hmm, and I'll drop the Cc: stable@ too, as commit b64d740ea7dd ("kvm:
> > > > x86: mmu: Always flush TLBs when enabling dirty logging") was a bug fix, i.e. if
> > > > anything should be backported it's that commit.
> > > 
> > > Actually, a better idea.  I think it makes sense to fully commit to not flushing
> > > when overwriting SPTEs, and instead rely on the dirty logging logic to do a remote
> > > TLB flush.
> > 
> > Oooh, but there's a bug.
> 
> Nope, there's not.
> 
> > KVM can tolerate/handle stale Dirty/Writable TLB entries when dirty logging,
> > but KVM cannot tolerate stale Writable TLB entries when write- protecting for
> > shadow paging.  The TDP MMU always flushes when clearing the MMU- writable
> > flag (modulo a bug that would cause KVM to make the SPTE !MMU-writable in the
> > page fault path), but the shadow MMU does not.
> > 
> > So I'm pretty sure we need the below, and then it may or may not make sense to have
> > a common "flush needed" helper (outside of the write-protecting flows, KVM probably
> > should WARN if MMU-writable is cleared).
> > 
> > ---
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index ce8323354d2d..7bd9c296f70e 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -514,9 +514,12 @@ static u64 mmu_spte_update_no_track(u64 *sptep, u64 new_spte)
> >  /* Rules for using mmu_spte_update:
> >   * Update the state bits, it means the mapped pfn is not changed.
> >   *
> > - * Whenever an MMU-writable SPTE is overwritten with a read-only SPTE, remote
> > - * TLBs must be flushed. Otherwise rmap_write_protect will find a read-only
> > - * spte, even though the writable spte might be cached on a CPU's TLB.
> > + * If the MMU-writable flag is cleared, i.e. the SPTE is write-protected for
> > + * write-tracking, remote TLBs must be flushed, even if the SPTE was read-only,
> > + * as KVM allows stale Writable TLB entries to exist.  When dirty logging, KVM
> > + * flushes TLBs based on whether or not dirty bitmap/ring entries were reaped,
> > + * not whether or not SPTEs were modified, i.e. only the write-protected case
> > + * needs to precisely flush when modifying SPTEs.
> >   *
> >   * Returns true if the TLB needs to be flushed
> >   */
> > @@ -533,8 +536,7 @@ static bool mmu_spte_update(u64 *sptep, u64 new_spte)
> >          * we always atomically update it, see the comments in
> >          * spte_has_volatile_bits().
> >          */
> > -       if (is_mmu_writable_spte(old_spte) &&
> > -             !is_writable_pte(new_spte))
> > +       if (is_mmu_writable_spte(old_spte) && !is_mmu_writable_spte(new_spte))
> 
> It took me forever and a day to realize this, but !is_writable_pte(new_spte) is
> correct, because the logic is checking if the new SPTE is !Writable, it's *not*
> checking to see if the Writable bit is _cleared_.  I.e. KVM will flush if the
> old SPTE is read-only but MMU-writable.
For read-only, host-writable is false, so MMU-writable can't be true?

Compared to "!is_writable_pte(new_spte)", "!is_mmu_writable_spte(new_spte)" just
skips the case "MMU-writalbe=1 + !Writable", which is for dirty logging.
> 
> That said, I'm still going to include this change, albet with a drastically
> different changelog.  Checking is_mmu_writable_spte() instead of is_writable_pte()
> is still desirable, as it avoids unnecessary TLB flushes in the rare case where
> KVM "refreshes" a !Writable SPTE.  Of course, with the other change to not clobber
> SPTEs when prefetching, that scenario becomes even more rare, but it's still worth
> doing, especially since IMO it makes it more obvious when KVM _does_ need to do a
> remote TLB flush (before dropping mmu_lock).
>