2016-07-14 10:09+0200, Paolo Bonzini:
> On 13/07/2016 22:30, Radim Krčmář wrote:
>> 2016-07-12 21:20+0200, Paolo Bonzini:
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> @@ -3967,6 +3968,14 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu,
>>> unsigned long cr4)
>>> (to_vmx(vcpu)->rmode.vm86_active ?
>>> KVM_RMODE_VM_CR4_ALWAYS_ON : 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);
>>
>> If UMIP support is not exposed in CPUID, we ought to #GP(0), because it
>> is a write to reserved bits. It could also mean that the vm control is
>> not supported.
>
> Yes, this is done in kvm_set_cr4:
>
> if (cr4 & CR4_RESERVED_BITS)
> return 1;
> ...
> if (!guest_cpuid_has_umip(vcpu) && (cr4 & X86_CR4_UMIP))
> return 1;
Missed that,
>> And we could then return true in vmx_umip_emulated() when
>> boot_cpu_has(X86_FEATURE_UMIP).
>> (Just for self-documentation, because occurrence of X86_FEATURE_UMIP is
>> most likely a subset of SECONDARY_EXEC_DESC.)
>
> This is not necessary because this is how KVM computes
> CPUID[EAX=7,EBX=0].ECX:
>
> unsigned f_umip = kvm_x86_ops->umip_emulated() ? F(UMIP) : 0;
> ...
> const u32 kvm_cpuid_7_0_ecx_x86_features = F(PKU) | F(UMIP);
and that too, I'm really sorry for the review.
> ...
> // Mask userspace-provided value against supported features
> entry->ecx &= kvm_cpuid_7_0_ecx_x86_features;
> // Mask userspace-provided value against host features
> cpuid_mask(&entry->ecx, CPUID_7_ECX);
> // Finally add emulated features
> entry->ecx |= f_umip;
