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@xxxxxxxxxxxxxxx #4.16
> Reported-by: Paolo Zeppegno <pzeppegno@xxxxxxxxx>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> 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.