From mboxrd@z Thu Jan  1 00:00:00 1970
From: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Subject: Re: [PATCH] KVM: vmx: update SECONDARY_EXEC_DESC only if CR4.UMIP
 changes
Date: Fri, 27 Apr 2018 17:39:19 +0200
Message-ID: <20180427153918.GB23874@flask>
References: <20180420173556.4708-1-sean.j.christopherson@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, pzeppegno@gmail.com,
        stable@vger.kernel.org
To: Sean Christopherson <sean.j.christopherson@intel.com>
Return-path: <stable-owner@vger.kernel.org>
Content-Disposition: inline
In-Reply-To: <20180420173556.4708-1-sean.j.christopherson@intel.com>
Sender: stable-owner@vger.kernel.org
List-Id: kvm.vger.kernel.org

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.