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