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

[Sean Christopherson <seanjc@google.com>]

On Wed, Oct 09, 2024, Yan Zhao wrote:
> On Tue, Oct 08, 2024 at 12:15:14PM -0700, Sean Christopherson wrote:
> > On Fri, Sep 27, 2024, Sean Christopherson wrote:
> > > ---
> > > 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?

Read-only here refers to the SPTE itself, i.e. the !is_writable_pte() case.