Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Sean Christopherson <seanjc@google.com>
Cc:     "H. Peter Anvin" <hpa@zytor.com>, linux-hyperv@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Sean Christopherson <seanjc@google.com>,
        "K. Y. Srinivasan" <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        Stephen Hemminger <sthemmin@microsoft.com>,
        Wei Liu <wei.liu@kernel.org>, Dexuan Cui <decui@microsoft.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        x86@kernel.org
Subject: Re: [PATCH 2/2] x86/hyperv: Move required MSRs check to initial
 platform probing
In-Reply-To: <20211028222148.2924457-3-seanjc@google.com>
References: <20211028222148.2924457-1-seanjc@google.com>
 <20211028222148.2924457-3-seanjc@google.com>
Date:   Fri, 29 Oct 2021 11:20:49 +0200
Message-ID: <87r1c4rxu6.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Sean Christopherson <seanjc@google.com> 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@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  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@redhat.com>

-- 
Vitaly