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

[Paolo Bonzini]

On 13/07/2016 22:30, Radim KrÄmÃÅ wrote:
> 2016-07-12 21:20+0200, Paolo Bonzini:
>> UMIP (User-Mode Instruction Prevention) is a feature of future
>> Intel processors (Cannonlake?) that blocks SLDT, SGDT, STR, SIDT
>> and SMSW from user-mode processes.
>>
>> On Intel systems it's *almost* possible to emulate it; it slows
>> down the instructions when they're executed in ring 0, but they
>> are really never executed in practice.  The catch is that SMSW
>> doesn't cause a vmexit, and hence SMSW will not fault.
>>
>> When UMIP is enabled but not supported by the host, descriptor table
>> exits are enabled, and the emulator takes care of injecting a #GP when
>> any of SLDT, SGDT, STR, SIDT are encountered.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
>> ---
>> 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;

>> +		hw_cr4 &= ~X86_CR4_UMIP;
>> +	} else
>> +		vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
>> +				SECONDARY_EXEC_DESC);
> 
> I think we don't have to do anything when the CPU supports UMIP,
> 
>  if (!boot_cpu_has(X86_FEATURE_UMIP) {
>    if ((cr4 & X86_CR4_UMIP)) { ... } else ...
>  }

Right.

> 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);
	...
	// 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;

Paolo

>> @@ -8597,7 +8627,8 @@ static bool vmx_xsaves_supported(void)
>>  
>>  static bool vmx_umip_emulated(void)
>>  {
>> -	return false;
>> +	return vmcs_config.cpu_based_2nd_exec_ctrl &
>> +		SECONDARY_EXEC_DESC;
>>  }