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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZwaBt0BzzG6Z0UGN@google.com>
Date: Wed, 9 Oct 2024 06:14:31 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Yan Zhao <yan.y.zhao@...el.com>
Cc: Isaku Yamahata <isaku.yamahata@...el.com>, kvm@...r.kernel.org, sagis@...gle.com, 
	chao.gao@...el.com, pbonzini@...hat.com, rick.p.edgecombe@...el.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: x86/tdp_mmu: Trigger the callback only when an
 interesting change

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.