[PATCH] KVM: VMX: Fix commit which broke PML

[PATCH] KVM: VMX: Fix commit which broke PML

[Paolo Bonzini]

From: Kai Huang <kai.huang@linux.intel.com>

[ upstream commit feda805fe7c4ed9cf78158e73b1218752e3b4314, for 4.1.y only.
  The symptoms are not as bad as they were when the upstream patch went
  in, but it still fixes an ugly call trace on VM shutdown and prepares
  for the next patch. ]

I found PML was broken since below commit:

	commit feda805fe7c4ed9cf78158e73b1218752e3b4314
	Author: Xiao Guangrong <guangrong.xiao@linux.intel.com>
	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 <guangrong.xiao@linux.intel.com>
	[Rewrite to use vmcs_set_secondary_exec_control. - Paolo]
	Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

The reason is in above commit vmx_cpuid_update calls vmx_secondary_exec_control,
in which currently SECONDARY_EXEC_ENABLE_PML bit is cleared 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
thinks PML is used.

Fix this by clearing SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control
only when PML is not supported or not enabled (!enable_pml). This is more
reasonable as PML is currently either always enabled or disabled. With this
explicit updating SECONDARY_EXEC_ENABLE_PML in vmx_enable{disable}_pml is not
needed so also rename vmx_enable{disable}_pml to vmx_create{destroy}_pml_buffer.

Fixes: feda805fe7c4ed9cf78158e73b1218752e3b4314
Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
[While at it, change a wrong ASSERT to an "if".  The condition can happen
 if creating the VCPU fails with ENOMEM. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/vmx.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 08f9d9230b94..c7b131a900fc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4593,8 +4593,9 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
 	   a current VMCS12
 	*/
 	exec_control &= ~SECONDARY_EXEC_SHADOW_VMCS;
-	/* PML is enabled/disabled in creating/destorying vcpu */
-	exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
+
+	if (!enable_pml)
+		exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
 
 	return exec_control;
 }
@@ -7632,10 +7633,9 @@ static void vmx_get_exit_info(struct kvm_vcpu *vcpu, u64 *info1, u64 *info2)
 	*info2 = vmcs_read32(VM_EXIT_INTR_INFO);
 }
 
-static int vmx_enable_pml(struct vcpu_vmx *vmx)
+static int vmx_create_pml_buffer(struct vcpu_vmx *vmx)
 {
 	struct page *pml_pg;
-	u32 exec_control;
 
 	pml_pg = alloc_page(GFP_KERNEL | __GFP_ZERO);
 	if (!pml_pg)
@@ -7646,24 +7646,15 @@ static int vmx_enable_pml(struct vcpu_vmx *vmx)
 	vmcs_write64(PML_ADDRESS, page_to_phys(vmx->pml_pg));
 	vmcs_write16(GUEST_PML_INDEX, PML_ENTITY_NUM - 1);
 
-	exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-	exec_control |= SECONDARY_EXEC_ENABLE_PML;
-	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
-
 	return 0;
 }
 
-static void vmx_disable_pml(struct vcpu_vmx *vmx)
+static void vmx_destroy_pml_buffer(struct vcpu_vmx *vmx)
 {
-	u32 exec_control;
-
-	ASSERT(vmx->pml_pg);
-	__free_page(vmx->pml_pg);
-	vmx->pml_pg = NULL;
-
-	exec_control = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
-	exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
-	vmcs_write32(SECONDARY_VM_EXEC_CONTROL, exec_control);
+	if (vmx->pml_pg) {
+		__free_page(vmx->pml_pg);
+		vmx->pml_pg = NULL;
+	}
 }
 
 static void vmx_flush_pml_buffer(struct vcpu_vmx *vmx)
@@ -8399,7 +8390,7 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	if (enable_pml)
-		vmx_disable_pml(vmx);
+		vmx_destroy_pml_buffer(vmx);
 	free_vpid(vmx);
 	leave_guest_mode(vcpu);
 	vmx_free_vcpu_nested(vcpu);
@@ -8480,7 +8471,7 @@ static struct kvm_vcpu *vmx_create_vcpu(struct kvm *kvm, unsigned int id)
 	 * for the guest, etc.
 	 */
 	if (enable_pml) {
-		err = vmx_enable_pml(vmx);
+		err = vmx_create_pml_buffer(vmx);
 		if (err)
 			goto free_vmcs;
 	}
-- 
2.9.3

Re: [PATCH] KVM: VMX: Fix commit which broke PML

[Greg Edwards]

On Tue, Nov 22, 2016 at 04:43:51PM +0100, Paolo Bonzini wrote:
> From: Kai Huang <kai.huang@linux.intel.com>
>
> [ upstream commit feda805fe7c4ed9cf78158e73b1218752e3b4314, for 4.1.y only.
>   The symptoms are not as bad as they were when the upstream patch went
>   in, but it still fixes an ugly call trace on VM shutdown and prepares
>   for the next patch. ]
>
> I found PML was broken since below commit:
>
> 	commit feda805fe7c4ed9cf78158e73b1218752e3b4314
> 	Author: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> 	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 <guangrong.xiao@linux.intel.com>
> 	[Rewrite to use vmcs_set_secondary_exec_control. - Paolo]
> 	Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> The reason is in above commit vmx_cpuid_update calls vmx_secondary_exec_control,
> in which currently SECONDARY_EXEC_ENABLE_PML bit is cleared 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
> thinks PML is used.
>
> Fix this by clearing SECONDARY_EXEC_ENABLE_PML in vmx_secondary_exec_control
> only when PML is not supported or not enabled (!enable_pml). This is more
> reasonable as PML is currently either always enabled or disabled. With this
> explicit updating SECONDARY_EXEC_ENABLE_PML in vmx_enable{disable}_pml is not
> needed so also rename vmx_enable{disable}_pml to vmx_create{destroy}_pml_buffer.
>
> Fixes: feda805fe7c4ed9cf78158e73b1218752e3b4314
> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
> [While at it, change a wrong ASSERT to an "if".  The condition can happen
>  if creating the VCPU fails with ENOMEM. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Tested-by: Greg Edwards <gedwards@ddn.com>