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

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

[Olaf Hering]

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);
 
 	ms_hyperv.max_vp_index = cpuid_eax(HYPERV_CPUID_IMPLEMENT_LIMITS);
 	ms_hyperv.max_lp_index = cpuid_ebx(HYPERV_CPUID_IMPLEMENT_LIMITS);

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

[Vitaly Kuznetsov]

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

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

[Wei Liu]

On Wed, Apr 08, 2020 at 05:54:35PM +0200, Vitaly Kuznetsov wrote:
> 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).

I think making them closer to TLFS is a good idea.

Leaving them always printed is not a big concern to me. Sometimes you
don't get to run `cpuid` if the kernel crashes early.

Olaf's patch is fine as-is. I'm happy to take this patch (if not already
taken by x86 maintainers) given it makes at least one developer's life
easier.

Wei.

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