| Message ID | 1446529798-31895-1-git-send-email-kai.huang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 > -- 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
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);
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(-)