On Thu, Oct 27, 2022 at 1:35 PM Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
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 :).
>
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 32f259fa5801..25ed8c1725ff 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -709,6 +709,8 @@ struct kvm {
> > struct kvm_memslots __memslots[KVM_ADDRESS_SPACE_NUM][2];
> > /* The current active memslot set for each address space */
> > struct kvm_memslots __rcu *memslots[KVM_ADDRESS_SPACE_NUM];
> > + /* The number of memslots with dirty logging enabled. */
> > + int nr_memslots_dirty_logging;
>
> I believe this can technically be a u16, as even with SMM KVM ensures the total
> number of memslots fits in a u16. A BUILD_BUG_ON() sanity check is probably a
> good idea regardless.
Will do.
>
> > struct xarray vcpu_array;
> >
> > /* Used to wait for completion of MMU notifiers. */
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index e30f1b4ecfa5..57e4406005cd 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1641,6 +1641,9 @@ static void kvm_commit_memory_region(struct kvm *kvm,
> > const struct kvm_memory_slot *new,
> > enum kvm_mr_change change)
> > {
> > + int old_flags = old ? old->flags : 0;
> > + int new_flags = new ? new->flags : 0;
>
> Not that it really matters, but kvm_memory_slot.flags is a u32.
Oops, thanks.
>
> > /*
> > * Update the total number of memslot pages before calling the arch
> > * hook so that architectures can consume the result directly.
> > @@ -1650,6 +1653,13 @@ static void kvm_commit_memory_region(struct kvm *kvm,
> > else if (change == KVM_MR_CREATE)
> > kvm->nr_memslot_pages += new->npages;
> >
> > + if ((old_flags ^ new_flags) & KVM_MEM_LOG_DIRTY_PAGES) {
> > + if (new_flags & KVM_MEM_LOG_DIRTY_PAGES)
> > + kvm->nr_memslots_dirty_logging++;
> > + else
> > + kvm->nr_memslots_dirty_logging--;
>
> A sanity check that KVM hasn't botched the count is probably a good idea. E.g.
> __kvm_set_memory_region() as a WARN_ON_ONCE() sanity check that KVM won't end up
> underflowing nr_memslot_pages.
Good idea, will do.
