From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH] KVM: VMX: Fix commit which broke PML Date: Tue, 3 Nov 2015 10:59:32 +0100 Message-ID: <56388584.3080501@redhat.com> References: <1446529798-31895-1-git-send-email-kai.huang@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit To: Kai Huang , guangrong.xiao@linux.intel.com, kvm@vger.kernel.org Return-path: Received: from mx1.redhat.com ([209.132.183.28]:57878 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752729AbbKCJ7g (ORCPT ); Tue, 3 Nov 2015 04:59:36 -0500 In-Reply-To: <1446529798-31895-1-git-send-email-kai.huang@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 03/11/2015 06:49, Kai Huang wrote: > I found PML was broken since below commit: > > commit feda805fe7c4ed9cf78158e73b1218752e3b4314 > Author: Xiao Guangrong > Date: Wed Sep 9 14:05:55 2015 +0800 > > KVM: VMX: unify SECONDARY_VM_EXEC_CONTROL update > > Unify the update in vmx_cpuid_update() > > Signed-off-by: Xiao Guangrong > [Rewrite to use vmcs_set_secondary_exec_control. - Paolo] > Signed-off-by: Paolo Bonzini > > The reason is PML after above commit vmx_cpuid_update calls > vmx_secondary_exec_control, in which PML is disabled unconditionally, as PML is > enabled in creating vcpu. Therefore if vcpu_cpuid_update is called after vcpu is > created, PML will be disabled unexpectedly while log-dirty code still think PML > is used. Actually looks calling vmx_secondary_exec_control in vmx_cpuid_update > is likely to break any VMX features that is enabled/disabled on demand by > updating SECONDARY_VM_EXEC_CONTROL, if vmx_cpuid_update is called between the > feature is enabled and disabled. > > Fix this by calling vmcs_read32 to read out SECONDARY_VM_EXEC_CONTROL directly. vmx_cpuid_update() is meant to be mostly idempotent; the parts that depend on the current VMCS configuration are hidden in vmcs_set_secondary_control. So a better fix would be to add SECONDARY_EXEC_ENABLE_PML to vmcs_set_secondary_exec_control's "mask" variable. However, you can see from the comment: /* * These bits in the secondary execution controls field * are dynamic, the others are mostly based on the hypervisor * architecture and the guest's CPUID. Do not touch the * dynamic bits. */ that even this is not the optimal fix. SECONDARY_EXEC_ENABLE_PML is either always set or always clear, so it shouldn't be in "mask". Instead, it should be in vmcs_config.cpu_based_2nd_exec_ctrl. It isn't because my review didn't notice this remnant of your original implementation, which dynamically enabled/disabled PML. In fact, cpu_has_vmx_pml() expects SECONDARY_EXEC_ENABLE_PML to be set in vmcs_config.cpu_based_2nd_exec_ctrl, so it is a bit confusing to remove the bit unconditionally in vmx_secondary_exec_control! So I think SECONDARY_EXEC_ENABLE_PML should not be removed unconditionally from exec_control in vmx_secondary_exec_control; the removal should be conditional on !enable_pml, like we do for e.g. EPT or VPID. If you do this, vmx_enable_pml and vmx_disable_pml need not touch SECONDARY_VM_EXEC_CONTROL anymore. Do you agree? If so, can you prepare a patch along these lines? (Since you are at it, perhaps you can rename vmx_enable_pml and vmx_disable_pml, since they will only allocate and free the PML page). Thanks for reporting the issue! Paolo