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

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

[Kai Huang]

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

Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4d0aa31..4525c0a7 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -8902,7 +8902,7 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
-	u32 secondary_exec_ctl = vmx_secondary_exec_control(vmx);
+	u32 secondary_exec_ctl = vmcs_read32(SECONDARY_VM_EXEC_CONTROL);
 
 	if (vmx_rdtscp_supported()) {
 		bool rdtscp_enabled = guest_cpuid_has_rdtscp(vcpu);
-- 
2.5.0

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

[Paolo Bonzini]

On 03/11/2015 06:49, Kai Huang wrote:
> 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 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

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

[Kai Huang]

On 11/03/2015 05:59 PM, Paolo Bonzini wrote:
>
> On 03/11/2015 06:49, Kai Huang wrote:
>> 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 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?
Thanks Paolo for your comments.

Sure I agree. I will send out the v2 patch by following what you suggested.

>
> (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).
I intend to rename vmx_enable{disable}_pml to 
vmx_create{destroy}_pml_buffer, as besides allocating buffer, we also 
need to write buffer address and PML index to VMCS.

Thanks,
-Kai
>
> Thanks for reporting the issue!
>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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