Re: [PATCH v1] x86: hyperv: report value of misc_features

From: Vitaly Kuznetsov <vkuznets@redhat.com>
To: Olaf Hering <olaf@aepfle.de>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>,
	Haiyang Zhang <haiyangz@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	Wei Liu <wei.liu@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	"H. Peter Anvin" <hpa@zytor.com>,
	x86@kernel.org, linux-hyperv@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Michael Kelley <mikelley@microsoft.com>
Subject: Re: [PATCH v1] x86: hyperv: report value of misc_features
Date: Wed, 08 Apr 2020 17:54:35 +0200	[thread overview]
Message-ID: <874ktu2csk.fsf@vitty.brq.redhat.com> (raw)
In-Reply-To: <20200407172739.31371-1-olaf@aepfle.de>

Olaf Hering <olaf@aepfle.de> writes:

> A few kernel features depend on ms_hyperv.misc_features, but unlike its
> siblings ->features and ->hints, the value was never reported during boot.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index caa032ce3fe3..53706fb56433 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -227,8 +227,8 @@ static void __init ms_hyperv_init_platform(void)
>  	ms_hyperv.misc_features = cpuid_edx(HYPERV_CPUID_FEATURES);
>  	ms_hyperv.hints    = cpuid_eax(HYPERV_CPUID_ENLIGHTMENT_INFO);
>  
> -	pr_info("Hyper-V: features 0x%x, hints 0x%x\n",
> -		ms_hyperv.features, ms_hyperv.hints);
> +	pr_info("Hyper-V: features 0x%x, hints 0x%x, misc 0x%x\n",
> +		ms_hyperv.features, ms_hyperv.hints, ms_hyperv.misc_features);

Several spare thoughts:

I'm always struggling to remember what is what here as these names don't
correspond to TLFS, could we maybe come up with better naming, e.g.

"Features.EAX", "Features.EDX", "Recommendations.EAX",...

On the other hand, there is more, e.g. nested virtualization
features. How is it useful for all users to see this in dmesg every
time? If we need it for development purposes, we can always run 'cpuid'
so maybe let's drop this altogether or print only with pr_debug? 

(Honestly: no strong opinion here, deferring to Microsoft folks).

>  
>  	ms_hyperv.max_vp_index = cpuid_eax(HYPERV_CPUID_IMPLEMENT_LIMITS);
>  	ms_hyperv.max_lp_index = cpuid_ebx(HYPERV_CPUID_IMPLEMENT_LIMITS);
>

-- 
Vitaly