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

["Radim Krčmář" <rkrcmar@redhat.com>]

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.