Re: [PATCH 2/2] x86/hyperv: Move required MSRs check to initial platform probing

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> Explicitly check for MSR_HYPERCALL and MSR_VP_INDEX support when probing
> for running as a Hyper-V guest instead of waiting until hyperv_init() to
> detect the bogus configuration.  Add messages to give the admin a heads
> up that they are likely running on a broken virtual machine setup.
>
> At best, silently disabling Hyper-V is confusing and difficult to debug,
> e.g. the kernel _says_ it's using all these fancy Hyper-V features, but
> always falls back to the native versions.  At worst, the half baked setup
> will crash/hang the kernel.
>
> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/hyperv/hv_init.c      |  9 +--------
>  arch/x86/kernel/cpu/mshyperv.c | 20 +++++++++++++++-----
>  2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/hyperv/hv_init.c b/arch/x86/hyperv/hv_init.c
> index 6cc845c026d4..abfb09610e22 100644
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -347,20 +347,13 @@ static void __init hv_get_partition_id(void)
>   */
>  void __init hyperv_init(void)
>  {
> -	u64 guest_id, required_msrs;
> +	u64 guest_id;
>  	union hv_x64_msr_hypercall_contents hypercall_msr;
>  	int cpuhp;
>  
>  	if (x86_hyper_type != X86_HYPER_MS_HYPERV)
>  		return;
>  
> -	/* Absolutely required MSRs */
> -	required_msrs = HV_MSR_HYPERCALL_AVAILABLE |
> -		HV_MSR_VP_INDEX_AVAILABLE;
> -
> -	if ((ms_hyperv.features & required_msrs) != required_msrs)
> -		return;
> -
>  	if (hv_common_init())
>  		return;
>  
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index e095c28d27ae..ef6316fef99f 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -163,12 +163,22 @@ static uint32_t  __init ms_hyperv_platform(void)
>  	cpuid(HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS,
>  	      &eax, &hyp_signature[0], &hyp_signature[1], &hyp_signature[2]);
>  
> -	if (eax >= HYPERV_CPUID_MIN &&
> -	    eax <= HYPERV_CPUID_MAX &&
> -	    !memcmp("Microsoft Hv", hyp_signature, 12))
> -		return HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
> +	if (eax < HYPERV_CPUID_MIN || eax > HYPERV_CPUID_MAX ||
> +	    memcmp("Microsoft Hv", hyp_signature, 12))
> +		return 0;
>  
> -	return 0;
> +	/* HYPERCALL and VP_INDEX MSRs are mandatory for all features. */
> +	eax = cpuid_eax(HYPERV_CPUID_FEATURES);
> +	if (!(eax & HV_MSR_HYPERCALL_AVAILABLE)) {
> +		pr_warn("x86/hyperv: HYPERCALL MSR not available.\n");
> +		return 0;
> +	}
> +	if (!(eax & HV_MSR_VP_INDEX_AVAILABLE)) {
> +		pr_warn("x86/hyperv: VP_INDEX MSR not available.\n");
> +		return 0;
> +	}
> +
> +	return HYPERV_CPUID_VENDOR_AND_MAX_FUNCTIONS;
>  }
>  
>  static unsigned char hv_get_nmi_reason(void)

In theory, we can get away without VP_INDEX MSR as e.g. PV spinlocks
don't need it but it will require us to add the check to all other
features which actually need it and disable them so it's probably not
worth the effort (and unless we're running on KVM/QEMU which actually
*can* create such configuration, it's likely impossible to meet such
setup in real life).

Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>

-- 
Vitaly