Re: [RESEND PATCH ] KVM: VMX: Enable/disable PML when dirty logging gets enabled/disabled

[Sean Christopherson]

On Wed, Feb 10, 2021, Makarand Sonare wrote:
> Currently, if enable_pml=1 PML remains enabled for the entire lifetime
> of the VM irrespective of whether dirty logging is enable or disabled.
> When dirty logging is disabled, all the pages of the VM are manually
> marked dirty, so that PML is effectively non-operational. Clearing

s/clearing/setting

Clearing is also expensive, but that can't be optimized away with this change.

> the dirty bits is an expensive operation which can cause severe MMU
> lock contention in a performance sensitive path when dirty logging
> is disabled after a failed or canceled live migration. Also, this
> would break if some other code path clears the dirty bits in which
> case, PML will actually start logging dirty pages even when dirty
> logging is disabled incurring unnecessary vmexits when the PML buffer
> becomes full. In order to avoid this extra overhead, we should
> enable or disable PML in VMCS when dirty logging gets enabled
> or disabled instead of keeping it always enabled.


...

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 777177ea9a35e..eb6639f0ee7eb 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4276,7 +4276,7 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
>  	*/
>  	exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
>  
> -	if (!enable_pml)
> +	if (!enable_pml || !vcpu->kvm->arch.pml_enabled)
>  		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;

The checks are unnecessary if PML is dynamically toggled, i.e. this snippet can
unconditionally clear PML.  When setting SECONDARY_EXEC (below snippet), PML
will be preserved in the current controls, which is what we want.

>  	if (cpu_has_vmx_xsaves()) {
> @@ -7133,7 +7133,8 @@ static void vmcs_set_secondary_exec_control(struct vcpu_vmx *vmx)
>  		SECONDARY_EXEC_SHADOW_VMCS |
>  		SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE |
>  		SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES |
> -		SECONDARY_EXEC_DESC;
> +		SECONDARY_EXEC_DESC |
> +		SECONDARY_EXEC_ENABLE_PML;
>  
>  	u32 new_ctl = vmx->secondary_exec_control;
>  	u32 cur_ctl = secondary_exec_controls_get(vmx);
> @@ -7509,6 +7510,19 @@ static void vmx_sched_in(struct kvm_vcpu *vcpu, int cpu)
>  static void vmx_slot_enable_log_dirty(struct kvm *kvm,
>  				     struct kvm_memory_slot *slot)
>  {
> +	/*
> +	 * Check all slots and enable PML if dirty logging
> +	 * is being enabled for the 1st slot
> +	 *
> +	 */
> +	if (enable_pml &&
> +	    kvm->dirty_logging_enable_count == 1 &&
> +	    !kvm->arch.pml_enabled) {
> +		kvm->arch.pml_enabled = true;
> +		kvm_make_all_cpus_request(kvm,
> +			KVM_REQ_UPDATE_VCPU_DIRTY_LOGGING_STATE);
> +	}

This is flawed.  .slot_enable_log_dirty() and .slot_disable_log_dirty() are only
called when LOG_DIRTY_PAGE is toggled in an existing memslot _and_ only the
flags of the memslot are being changed.  This fails to enable PML if the first
memslot with LOG_DIRTY_PAGE is created or moved, and fails to disable PML if the
last memslot with LOG_DIRTY_PAGE is deleted.

> +
>  	if (!kvm_dirty_log_manual_protect_and_init_set(kvm))
>  		kvm_mmu_slot_leaf_clear_dirty(kvm, slot);
>  	kvm_mmu_slot_largepage_remove_write_access(kvm, slot);
> @@ -7517,9 +7531,39 @@ static void vmx_slot_enable_log_dirty(struct kvm *kvm,
>  static void vmx_slot_disable_log_dirty(struct kvm *kvm,
>  				       struct kvm_memory_slot *slot)
>  {
> +	/*
> +	 * Check all slots and disable PML if dirty logging
> +	 * is being disabled for the last slot
> +	 *
> +	 */
> +	if (enable_pml &&
> +	    kvm->dirty_logging_enable_count == 0 &&
> +	    kvm->arch.pml_enabled) {
> +		kvm->arch.pml_enabled = false;
> +		kvm_make_all_cpus_request(kvm,
> +			KVM_REQ_UPDATE_VCPU_DIRTY_LOGGING_STATE);
> +	}
> +
>  	kvm_mmu_slot_set_dirty(kvm, slot);
>  }

...

>  #define kvm_err(fmt, ...) \
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index ee4ac2618ec59..c6e5b026bbfe8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -307,6 +307,7 @@ bool kvm_make_all_cpus_request(struct kvm *kvm, unsigned int req)
>  {
>  	return kvm_make_all_cpus_request_except(kvm, req, NULL);
>  }
> +EXPORT_SYMBOL_GPL(kvm_make_all_cpus_request);
>  
>  #ifndef CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL
>  void kvm_flush_remote_tlbs(struct kvm *kvm)
> @@ -1366,15 +1367,24 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	}
>  
>  	/* Allocate/free page dirty bitmap as needed */
> -	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
> +	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>  		new.dirty_bitmap = NULL;
> -	else if (!new.dirty_bitmap && !kvm->dirty_ring_size) {
> +
> +		if (old.flags & KVM_MEM_LOG_DIRTY_PAGES) {
> +			WARN_ON(kvm->dirty_logging_enable_count == 0);
> +			--kvm->dirty_logging_enable_count;

The count will be corrupted if kvm_set_memslot() fails.

The easiest/cleanest way to fix both this and the refcounting bug is to handle
the count in kvm_mmu_slot_apply_flags().  That will also allow making the dirty
log count x86-only, and it can then be renamed to cpu_dirty_log_count to align
with the

We can always move/rename the count variable if additional motivation for
tracking dirty logging comes along.


> +		}
> +
> +	} else if (!new.dirty_bitmap && !kvm->dirty_ring_size) {
>  		r = kvm_alloc_dirty_bitmap(&new);
>  		if (r)
>  			return r;
>  
>  		if (kvm_dirty_log_manual_protect_and_init_set(kvm))
>  			bitmap_set(new.dirty_bitmap, 0, new.npages);
> +
> +		++kvm->dirty_logging_enable_count;
> +		WARN_ON(kvm->dirty_logging_enable_count == 0);
>  	}
>  
>  	r = kvm_set_memslot(kvm, mem, &old, &new, as_id, change);
> -- 
> 2.30.0.478.g8a0d178c01-goog