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:
> 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.

>  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.

>  	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.

>  	/*
>  	 * 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.

> +	}
> +
>  	kvm_arch_commit_memory_region(kvm, old, new, change);
>  
>  	switch (change) {
> -- 
> 2.38.1.273.g43a17bfeac-goog
>