Re: [PATCH v2 3/3] KVM: VMX: Gracefully handle faults on VMXON

[Vitaly Kuznetsov]

Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes:

> Gracefully handle faults on VMXON, e.g. #GP due to VMX being disabled by
> BIOS, instead of letting the fault crash the system.  Now that KVM uses
> cpufeatures to query support instead of reading MSR_IA32_FEAT_CTL
> directly, it's possible for a bug in a different subsystem to cause KVM
> to incorrectly attempt VMXON[*].  Crashing the system is especially
> annoying if the system is configured such that hardware_enable() will
> be triggered during boot.
>
> Oppurtunistically rename @addr to @vmxon_pointer and use a named param
> to reference it in the inline assembly.
>
> Print 0xdeadbeef in the ultra-"rare" case that reading MSR_IA32_FEAT_CTL
> also faults.
>
> [*] https://lkml.kernel.org/r/20200226231615.13664-1-sean.j.christopherson@xxxxxxxxx
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
>  arch/x86/kvm/vmx/vmx.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 07634caa560d..3aba51d782e2 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2218,18 +2218,33 @@ static __init int vmx_disabled_by_bios(void)
>  	       !boot_cpu_has(X86_FEATURE_VMX);
>  }
>  
> -static void kvm_cpu_vmxon(u64 addr)
> +static int kvm_cpu_vmxon(u64 vmxon_pointer)
>  {
> +	u64 msr;
> +
>  	cr4_set_bits(X86_CR4_VMXE);
>  	intel_pt_handle_vmx(1);
>  
> -	asm volatile ("vmxon %0" : : "m"(addr));
> +	asm_volatile_goto("1: vmxon %[vmxon_pointer]\n\t"
> +			  _ASM_EXTABLE(1b, %l[fault])
> +			  : : [vmxon_pointer] "m"(vmxon_pointer)
> +			  : : fault);
> +	return 0;
> +
> +fault:
> +	WARN_ONCE(1, "VMXON faulted, MSR_IA32_FEAT_CTL (0x3a) = 0x%llx\n",
> +		  rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr) ? 0xdeadbeef : msr);

We seem to be acting under an assumption that the fault is (likelt)
caused my disabled VMX feature but afaics the fault can be caused by
passing a bogus pointer too (but that would be a KVM bug, of course).

> +	intel_pt_handle_vmx(0);
> +	cr4_clear_bits(X86_CR4_VMXE);
> +
> +	return -EFAULT;
>  }
>  
>  static int hardware_enable(void)
>  {
>  	int cpu = raw_smp_processor_id();
>  	u64 phys_addr = __pa(per_cpu(vmxarea, cpu));
> +	int r;
>  
>  	if (cr4_read_shadow() & X86_CR4_VMXE)
>  		return -EBUSY;
> @@ -2246,7 +2261,10 @@ static int hardware_enable(void)
>  	INIT_LIST_HEAD(&per_cpu(blocked_vcpu_on_cpu, cpu));
>  	spin_lock_init(&per_cpu(blocked_vcpu_on_cpu_lock, cpu));
>  
> -	kvm_cpu_vmxon(phys_addr);
> +	r = kvm_cpu_vmxon(phys_addr);
> +	if (r)
> +		return r;
> +
>  	if (enable_ept)
>  		ept_sync_global();

Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>

-- 
Vitaly