Re: [PATCH 1/2] KVM: Keep track of the number of memslots with dirty logging enabled

[Sean Christopherson <seanjc@google.com>]

On Thu, Oct 27, 2022, David Matlack wrote:
> On Thu, Oct 27, 2022 at 1:35 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Oct 27, 2022, David Matlack wrote:
> > > Add a new field to struct kvm that keeps track of the number of memslots
> > > with dirty logging enabled. This will be used in a future commit to
> > > cheaply check if any memslot is doing dirty logging.
> > >
> > > Signed-off-by: David Matlack <dmatlack@google.com>
> > > ---
> > >  include/linux/kvm_host.h |  2 ++
> > >  virt/kvm/kvm_main.c      | 10 ++++++++++
> >
> > Why put this in common code?  I'm having a hard time coming up with a second use
> > case since the count isn't stable, i.e. it can't be used for anything except
> > scenarios like x86's NX huge page mitigation where a false negative/positive is benign.
> 
> I agree, but what is the downside of putting it in common code?

The potential for misuse, e.g. outside of slots_lock.

> The downside of putting it in architecture-specific code is if any other
> architecture needs it (or something similar) in the future they are unlikely
> to look through x86 code to see if it already exists. i.e.  we're more likely
> to end up with duplicate code.
>
> And while the count is not stable outside of slots_lock, it could
> still theoretically be used under slots_lock to coordinate something
> that depends on dirty logging being enabled in any slot. In our
> internal kernel, for example, we use it to decide when to
> create/destroy the KVM dirty log worker threads (although I doubt that
> specific usecase will ever see the light of day upstream :).

Yeah, I'm definitely not dead set against putting it in common code.  I suspect
I'm a little overly sensitive at the moment to x86 pushing a bunch of x86-centric
logic into kvm_main.c and making life miserable for everyone.  Been spending far
too much time unwinding the mess that is kvm_init()...