On Fri, Sep 27, 2024, Sean Christopherson wrote:
> On Thu, Sep 26, 2024, Yan Zhao wrote:
> > On Thu, Sep 12, 2024 at 05:07:57PM -0700, Sean Christopherson wrote:
> > > On Thu, Sep 12, 2024, Isaku Yamahata wrote:
> > > Right now, the fixes for make_spte() are sitting toward the end of the massive
> > > kvm_follow_pfn() rework (80+ patches and counting), but despite the size, I am
> > > fairly confident that series can land in 6.13 (lots and lots of small patches).
> > >
> > > ---
> > > Author: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > AuthorDate: Thu Sep 12 16:23:21 2024 -0700
> > > Commit: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > CommitDate: Thu Sep 12 16:35:06 2024 -0700
> > >
> > > KVM: x86/mmu: Flush TLBs if resolving a TDP MMU fault clears W or D bits
> > >
> > > Do a remote TLB flush if installing a leaf SPTE overwrites an existing
> > > leaf SPTE (with the same target pfn) and clears the Writable bit or the
> > > Dirty bit. KVM isn't _supposed_ to clear Writable or Dirty bits in such
> > > a scenario, but make_spte() has a flaw where it will fail to set the Dirty
> > > if the existing SPTE is writable.
> > >
> > > E.g. if two vCPUs race to handle faults, the KVM will install a W=1,D=1
> > > SPTE for the first vCPU, and then overwrite it with a W=1,D=0 SPTE for the
> > > second vCPU. If the first vCPU (or another vCPU) accesses memory using
> > > the W=1,D=1 SPTE, i.e. creates a writable, dirty TLB entry, and that is
> > > the only SPTE that is dirty at the time of the next relevant clearing of
> > > the dirty logs, then clear_dirty_gfn_range() will not modify any SPTEs
> > > because it sees the D=0 SPTE, and thus will complete the clearing of the
> > > dirty logs without performing a TLB flush.
> > But it looks that kvm_flush_remote_tlbs_memslot() will always be invoked no
> > matter clear_dirty_gfn_range() finds a D bit or not.
>
> 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.
E.g. on top of this change in the mega-series is a cleanup to unify the TDP MMU
and shadow MMU logic for clearing Writable and Dirty bits, with this comment
(which is a massaged version of an existing comment for mmu_spte_update()):
/*
* Whenever an MMU-writable SPTE is overwritten with a read-only SPTE, remote
* TLBs must be flushed. Otherwise write-protecting the gfn may find a read-
* only SPTE, even though the writable SPTE might be cached in a CPU's TLB.
*
* Remote TLBs also need to be flushed if the Dirty bit is cleared, as false
* negatives are not acceptable, e.g. if KVM is using D-bit based PML on VMX.
*
* Don't flush if the Accessed bit is cleared, as access tracking tolerates
* false negatives, and the one path that does care about TLB flushes,
* kvm_mmu_notifier_clear_flush_young(), uses mmu_spte_update_no_track().
*
* Note, this logic only applies to leaf SPTEs. The caller is responsible for
* determining whether or not a TLB flush is required when modifying a shadow-
* present non-leaf SPTE.
*/
But that comment is was made stale by commit b64d740ea7dd. And looking through
the dirty logging logic, KVM (luckily? thankfully?) flushes based on whether or
not dirty bitmap/ring entries are reaped, not based on whether or not SPTEs were
modified.
