[PATCH] KVM: vmx: update SECONDARY_EXEC_DESC only if CR4.UMIP changes

[PATCH] KVM: vmx: update SECONDARY_EXEC_DESC only if CR4.UMIP changes

[Sean Christopherson]

Update SECONDARY_EXEC_DESC in SECONDARY_VM_EXEC_CONTROL for UMIP
emulation if and only if CR4.UMIP is being modified and UMIP is
not supported by hardware, i.e. we're emulating UMIP.  If CR4.UMIP
is not being changed then it's safe to assume that the previous
invocation of vmx_set_cr4() correctly set SECONDARY_EXEC_DESC,
i.e. the desired value is already the current value.  This avoids
unnecessary VMREAD/VMWRITE to SECONDARY_VM_EXEC_CONTROL, which
is critical as not all processors support SECONDARY_VM_EXEC_CONTROL.

WARN once and signal a fault if CR4.UMIP is changing and UMIP can't
be emulated, i.e. SECONDARY_EXEC_DESC can't be set.  Prior checks
should prevent setting UMIP if it can't be emulated, i.e. UMIP
shouldn't have been advertised to the guest if it can't be emulated,
regardless of whether or not UMIP is supported in bare metal.

Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
Cc: stable@vger.kernel.org #4.16
Reported-by: Paolo Zeppegno <pzeppegno@gmail.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 arch/x86/kvm/vmx.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index aafcc9881e88..1502a2ac7884 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,20 @@ 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,
-				SECONDARY_EXEC_DESC);
+	if (((cr4 ^ kvm_read_cr4(vcpu)) & X86_CR4_UMIP) &&
+	    !boot_cpu_has(X86_FEATURE_UMIP)) {
+		if (WARN_ON_ONCE(!vmx_umip_emulated()))
+			return 1;
+
+		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 +9524,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.16.2

Re: [PATCH] KVM: vmx: update SECONDARY_EXEC_DESC only if CR4.UMIP changes

[Radim Krčmář]

2018-04-20 10:35-0700, Sean Christopherson:
> Update SECONDARY_EXEC_DESC in SECONDARY_VM_EXEC_CONTROL for UMIP
> emulation if and only if CR4.UMIP is being modified and UMIP is
> not supported by hardware, i.e. we're emulating UMIP.  If CR4.UMIP
> is not being changed then it's safe to assume that the previous
> invocation of vmx_set_cr4() correctly set SECONDARY_EXEC_DESC,

I think this is not true for nested VMX:  we're pretending that L1 has
the UMIP feature, so L1 won't set SECONDARY_EXEC_DESC when using UMIP
and the CR4 bit might not change upon nested transition, therefore KVM
won't enable the emulation for vmcs02.

> i.e. the desired value is already the current value.  This avoids
> unnecessary VMREAD/VMWRITE to SECONDARY_VM_EXEC_CONTROL, which
> is critical as not all processors support SECONDARY_VM_EXEC_CONTROL.
> 
> WARN once and signal a fault if CR4.UMIP is changing and UMIP can't
> be emulated, i.e. SECONDARY_EXEC_DESC can't be set.  Prior checks
> should prevent setting UMIP if it can't be emulated, i.e. UMIP
> shouldn't have been advertised to the guest if it can't be emulated,
> regardless of whether or not UMIP is supported in bare metal.

(UMIP can be advertised if the hardware supports it.)

> Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
> Cc: stable@vger.kernel.org #4.16
> Reported-by: Paolo Zeppegno <pzeppegno@gmail.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> @@ -4776,14 +4782,20 @@ 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,
> -				SECONDARY_EXEC_DESC);
> +	if (((cr4 ^ kvm_read_cr4(vcpu)) & X86_CR4_UMIP) &&
> +	    !boot_cpu_has(X86_FEATURE_UMIP)) {
> +		if (WARN_ON_ONCE(!vmx_umip_emulated()))

Setting the CR4 UMIP bit on a CPU that doesn't support it should fail on
reserved bit checks during VM entry;  I'd just wrap the current code in

  if (vmx_umip_emulated() && !boot_cpu_has(X86_FEATURE_UMIP))

and optimize the pointless VMCS writes in followup patches,

thanks.