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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 10 Feb 2021 18:07:06 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Makarand Sonare <makarandsonare@...gle.com>
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        pbonzini@...hat.com, pshier@...gle.com, jmattson@...gle.com,
        Ben Gardon <bgardon@...gle.com>
Subject: Re: [RESEND PATCH ] KVM: VMX: Enable/disable PML when dirty logging
 gets enabled/disabled

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

Breaking this up into a few paragraphs would be helpful.

> Tested:
> 	kvm-unit-tests
> 	dirty_log_test
> 	dirty_log_perf_test

Eh, I get that we like these for internal tracking, but for upstream there's an
assumption that you did your due diligence.  If there's something noteworthy
about your testing (or lack thereof), throw it in the cover letter or in the
part that's not recorded in the final commit.

> Signed-off-by: Makarand Sonare <makarandsonare@...gle.com>
> Reviewed-by: Ben Gardon <bgardon@...gle.com>
> ---

...

> @@ -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);

The justification for dynamically toggling PML is that it means KVM can skip
setting all the dirty bits when logging is disabled, but that code is still here.
Is there a follow-up planned to reap the reward?

>  }