Re: [PATCH RFC 02/10] KVM: Allow reading memslots while holding slots_arch_lock

[Sean Christopherson <seanjc@google.com>]

On Thu, Sep 10, 2026, Marco Elver wrote:
> On Thu, 10 Sept 2026 at 18:30, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Sep 10, 2026, Marco Elver wrote:
> > > kvm_swap_active_memslots() updates kvm->memslots[as_id] while holding both
> > > kvm->slots_lock and kvm->slots_arch_lock. Holding either lock guarantees
> > > that memslots cannot be concurrently modified.
> >
> > Sure, but that's irrelevant.  The goal of the srcu_dereference_check() is to
> > ensure that readers see a stable view of the VM's overall memory, not simply that
> > kvm->memslots can't be written.
> 
> Functionally, this is irrelevant for readers. But under lockdep it
> isn't for writers: srcu_dereference_check() (with lockdep) asserts
> that the srcu reader-lock is held, or the condition 'c' holds, which
> here is holding any of the writer locks.

No, the rules for writing kvm->memslots is that *both* are held.

> > > Allow reading memslots in __kvm_memslots() when kvm->slots_arch_lock is
> > > held.
> >
> > Why?
> 
> Holding any of the writer locks guarantees no concurrent modification;
> therefore, if any writer lock is held, it's not required that the srcu
> reader-lock is held. There are few places where only either slots_lock
> or slots_arch_lock is held, which is sufficient for reading.

Yes, but with caveats.  And more importantly, pure readers really shouldn't be
taking slots_arch_lock, because either it's overkill and will generate unnecessary
lock content, or the alleged reader is doing more than just reading.

Holding just slots_arch_lock *could* be fine, depending on the usage, but those
details matter, which is why I asked "why".  I want to know exactly why we should
relax the locking rules.

Ah, poking around the code, I suspect that the motivation is
kvm_enable_external_write_tracking()?  Which grabs __kvm_memslots() but only
holds slots_arch_lock, i.e. would get a lockdep splat if someone with KVMGT ran
with lockdep enabled.

That thing isn't a pure reader.  It only reads the actual kvm->memslots pointer,
but it writes to each of the slots metadata.

So, allowing __kvm_memslots() to be called with just slot_arch_lock is ok in
this situation, and if my guess is right, necessary to fix a false positive.  But
I'm on the fence as to whether or not we generally want to allow that, versus
taking kvm->srcu in kvm_enable_external_write_tracking() even though strictly
speaking it's unnecessary.