Re: [RFC PATCH 4/4] KVM: vmx: add support for emulating UMIP

[Radim KrÄmÃÅ]

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;