[PATCH v2] KVM: vmx: update sec exec controls for UMIP iff emulating UMIP

[PATCH v2] KVM: vmx: update sec exec controls for UMIP iff emulating UMIP

[Sean Christopherson]

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;
-- 
2.17.0

Re: [PATCH v2] KVM: vmx: update sec exec controls for UMIP iff emulating UMIP

[Paolo Bonzini]

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