| Message ID | 20180430170106.26836-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/04/2018 19:01, Sean Christopherson wrote: > Update SECONDARY_EXEC_DESC for UMIP emulation if and only UMIP > is actually being emulated. Skipping the VMCS update eliminates > unnecessary VMREAD/VMWRITE when UMIP is supported in hardware, > and on platforms that don't have SECONDARY_VM_EXEC_CONTROL. The > latter case resolves a bug where KVM would fill the kernel log > with warnings due to failed VMWRITEs on older platforms. > > Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP") > Cc: stable@vger.kernel.org #4.16 > Reported-by: Paolo Zeppegno <pzeppegno@gmail.com> > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Suggested-by: Radim Krčmář <rkrcmar@redhat.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > v2: Fix the bug simply by skipping VMCS updates when UMIP is not > being emulated, as suggested by Paolo and again by Radim. > The approach of updating the VMCS only when CR4.UMIP changed > was not guaranteed to work in all cases. Optimizing away > VMREAD/VMWRITE will be tackled in a separate series. > > arch/x86/kvm/vmx.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index aafcc9881e88..53d85439e5e5 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -1494,6 +1494,12 @@ static inline bool cpu_has_vmx_vmfunc(void) > SECONDARY_EXEC_ENABLE_VMFUNC; > } > > +static bool vmx_umip_emulated(void) > +{ > + return vmcs_config.cpu_based_2nd_exec_ctrl & > + SECONDARY_EXEC_DESC; > +} > + > static inline bool report_flexpriority(void) > { > return flexpriority_enabled; > @@ -4776,14 +4782,16 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) > else > hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON; > > - if ((cr4 & X86_CR4_UMIP) && !boot_cpu_has(X86_FEATURE_UMIP)) { > - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, > - SECONDARY_EXEC_DESC); > - hw_cr4 &= ~X86_CR4_UMIP; > - } else if (!is_guest_mode(vcpu) || > - !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) > - vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL, > + if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) { > + if (cr4 & X86_CR4_UMIP) { > + vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, > SECONDARY_EXEC_DESC); > + hw_cr4 &= ~X86_CR4_UMIP; > + } else if (!is_guest_mode(vcpu) || > + !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) > + vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL, > + SECONDARY_EXEC_DESC); > + } > > if (cr4 & X86_CR4_VMXE) { > /* > @@ -9512,12 +9520,6 @@ static bool vmx_xsaves_supported(void) > SECONDARY_EXEC_XSAVES; > } > > -static bool vmx_umip_emulated(void) > -{ > - return vmcs_config.cpu_based_2nd_exec_ctrl & > - SECONDARY_EXEC_DESC; > -} > - > static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx) > { > u32 exit_intr_info; > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index aafcc9881e88..53d85439e5e5 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -1494,6 +1494,12 @@ static inline bool cpu_has_vmx_vmfunc(void) SECONDARY_EXEC_ENABLE_VMFUNC; } +static bool vmx_umip_emulated(void) +{ + return vmcs_config.cpu_based_2nd_exec_ctrl & + SECONDARY_EXEC_DESC; +} + static inline bool report_flexpriority(void) { return flexpriority_enabled; @@ -4776,14 +4782,16 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4) else hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON; - if ((cr4 & X86_CR4_UMIP) && !boot_cpu_has(X86_FEATURE_UMIP)) { - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, - SECONDARY_EXEC_DESC); - hw_cr4 &= ~X86_CR4_UMIP; - } else if (!is_guest_mode(vcpu) || - !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) - vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL, + if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) { + if (cr4 & X86_CR4_UMIP) { + vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL, SECONDARY_EXEC_DESC); + hw_cr4 &= ~X86_CR4_UMIP; + } else if (!is_guest_mode(vcpu) || + !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC)) + vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL, + SECONDARY_EXEC_DESC); + } if (cr4 & X86_CR4_VMXE) { /* @@ -9512,12 +9520,6 @@ static bool vmx_xsaves_supported(void) SECONDARY_EXEC_XSAVES; } -static bool vmx_umip_emulated(void) -{ - return vmcs_config.cpu_based_2nd_exec_ctrl & - SECONDARY_EXEC_DESC; -} - static void vmx_recover_nmi_blocking(struct vcpu_vmx *vmx) { u32 exit_intr_info;
Update SECONDARY_EXEC_DESC for UMIP emulation if and only UMIP is actually being emulated. Skipping the VMCS update eliminates unnecessary VMREAD/VMWRITE when UMIP is supported in hardware, and on platforms that don't have SECONDARY_VM_EXEC_CONTROL. The latter case resolves a bug where KVM would fill the kernel log with warnings due to failed VMWRITEs on older platforms. Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP") Cc: stable@vger.kernel.org #4.16 Reported-by: Paolo Zeppegno <pzeppegno@gmail.com> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Suggested-by: Radim Krčmář <rkrcmar@redhat.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- v2: Fix the bug simply by skipping VMCS updates when UMIP is not being emulated, as suggested by Paolo and again by Radim. The approach of updating the VMCS only when CR4.UMIP changed was not guaranteed to work in all cases. Optimizing away VMREAD/VMWRITE will be tackled in a separate series. arch/x86/kvm/vmx.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-)