Re: [tip:x86/urgent] x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6

[Thomas Gleixner]

On Mon, 19 Dec 2016, H. Peter Anvin wrote:
> P.S. I would love to (a) move the CPUID bits into a structure instead of
> passing all those crazy pointers around, and (b) stop passing struct
> cpuinfo * around when we only use it for the current processor anyway (a
> lot of these functions are in fact completely invalid if we don't); we
> could define this_cpu_info as (*this_cpu_ptr(_info)) -- basically
> what I have above -- or directly use percpu functions to access these
> variables.

The whole cpu identify code wants to be rewritten completely. It's full of
conditionals and places which read the same cpuid leaf over and over. The
whole thing is just insane.

The proper solution is to have a single function which reads all available
leafs into a data structure and then do all the conditional computations
and decisions based on that.

Thanks,

tglx

Re: [tip:x86/urgent] x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6

[Thomas Gleixner]

On Mon, 19 Dec 2016, H. Peter Anvin wrote:
> P.S. I would love to (a) move the CPUID bits into a structure instead of
> passing all those crazy pointers around, and (b) stop passing struct
> cpuinfo * around when we only use it for the current processor anyway (a
> lot of these functions are in fact completely invalid if we don't); we
> could define this_cpu_info as (*this_cpu_ptr(_info)) -- basically
> what I have above -- or directly use percpu functions to access these
> variables.

The whole cpu identify code wants to be rewritten completely. It's full of
conditionals and places which read the same cpuid leaf over and over. The
whole thing is just insane.

The proper solution is to have a single function which reads all available
leafs into a data structure and then do all the conditional computations
and decisions based on that.

Thanks,

tglx

Re: [tip:x86/urgent] x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6

[H. Peter Anvin]

On 12/19/16 02:56, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  3df8d9208569ef0b2313e516566222d745f3b94b
> Gitweb: http://git.kernel.org/tip/3df8d9208569ef0b2313e516566222d745f3b94b
> Author: Andy Lutomirski 
> AuthorDate: Thu, 15 Dec 2016 10:14:42 -0800
> Committer:  Thomas Gleixner 
> CommitDate: Mon, 19 Dec 2016 11:50:24 +0100
> 
> x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6
> 
> A typo (or mis-merge?) resulted in leaf 6 only being probed if
> cpuid_level >= 7.
> 
> Fixes: 2ccd71f1b278 ("x86/cpufeature: Move some of the scattered feature bits 
> to x86_capability")
> Signed-off-by: Andy Lutomirski 
> Acked-by: Borislav Petkov 
> Cc: Brian Gerst 
> Link: 
> http://lkml.kernel.org/r/6ea30c0e9daec21e488b54761881a6dfcf3e04d0.1481825597.git.l...@kernel.org
> Signed-off-by: Thomas Gleixner 
> 
> ---
>  arch/x86/kernel/cpu/common.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 1f6b50a..dc1697c 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -667,13 +667,14 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>   c->x86_capability[CPUID_1_EDX] = edx;
>   }
>  
> + /* Thermal and Power Management Leaf: level 0x0006 (eax) */
> + if (c->cpuid_level >= 0x0006)
> + c->x86_capability[CPUID_6_EAX] = cpuid_eax(0x0006);
> +
>   /* Additional Intel-defined flags: level 0x0007 */
>   if (c->cpuid_level >= 0x0007) {
>   cpuid_count(0x0007, 0, , , , );
> -
>   c->x86_capability[CPUID_7_0_EBX] = ebx;
> -
> - c->x86_capability[CPUID_6_EAX] = cpuid_eax(0x0006);
>   c->x86_capability[CPUID_7_ECX] = ecx;
>   }

Perhaps we should have something like:

void cpuid_cond(u32 leaf, u32 subleaf,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
{
const struct cpuinfo_x86 *c = this_cpu_ptr(_info);
u32 maxleaf;
int cpuid_level = c->cpuid_level;

switch (leaf >> 16) {
case 0x:
maxleaf = c->cpuid_level;
break;
case 0x8000:
maxleaf = c->extended_cpuid_level;
break;
case 0x4000 .. 0x40ff:
/* Not ideal, can we do better? */
maxleaf = cpu_has(X86_FEATURE_HYPERVISOR)
  ? 0x40ff : 0;
break;
/* Add Transmeta 0x8086 and VIA/Cyrix 0xc000? */
default:
/* Can we do better? */
if (c->x86_vendor_id) == X86_VENDOR_INTEL)
maxleaf = 0;
else
maxleaf = leaf | 0x;
break;
}

if (cpuid_level >= 0 && leaf >= maxleaf) {
cpuid_count(leaf, subleaf, eax, ebx, ecx, edx);
} else {
*eax = *ebx = *ecx = *edx = 0;
}
}

-hpa

P.S. I would love to (a) move the CPUID bits into a structure instead of
passing all those crazy pointers around, and (b) stop passing struct
cpuinfo * around when we only use it for the current processor anyway (a
lot of these functions are in fact completely invalid if we don't); we
could define this_cpu_info as (*this_cpu_ptr(_info)) -- basically
what I have above -- or directly use percpu functions to access these
variables.

Re: [tip:x86/urgent] x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6

[H. Peter Anvin]

On 12/19/16 02:56, tip-bot for Andy Lutomirski wrote:
> Commit-ID:  3df8d9208569ef0b2313e516566222d745f3b94b
> Gitweb: http://git.kernel.org/tip/3df8d9208569ef0b2313e516566222d745f3b94b
> Author: Andy Lutomirski 
> AuthorDate: Thu, 15 Dec 2016 10:14:42 -0800
> Committer:  Thomas Gleixner 
> CommitDate: Mon, 19 Dec 2016 11:50:24 +0100
> 
> x86/cpu: Probe CPUID leaf 6 even when cpuid_level == 6
> 
> A typo (or mis-merge?) resulted in leaf 6 only being probed if
> cpuid_level >= 7.
> 
> Fixes: 2ccd71f1b278 ("x86/cpufeature: Move some of the scattered feature bits 
> to x86_capability")
> Signed-off-by: Andy Lutomirski 
> Acked-by: Borislav Petkov 
> Cc: Brian Gerst 
> Link: 
> http://lkml.kernel.org/r/6ea30c0e9daec21e488b54761881a6dfcf3e04d0.1481825597.git.l...@kernel.org
> Signed-off-by: Thomas Gleixner 
> 
> ---
>  arch/x86/kernel/cpu/common.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 1f6b50a..dc1697c 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -667,13 +667,14 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>   c->x86_capability[CPUID_1_EDX] = edx;
>   }
>  
> + /* Thermal and Power Management Leaf: level 0x0006 (eax) */
> + if (c->cpuid_level >= 0x0006)
> + c->x86_capability[CPUID_6_EAX] = cpuid_eax(0x0006);
> +
>   /* Additional Intel-defined flags: level 0x0007 */
>   if (c->cpuid_level >= 0x0007) {
>   cpuid_count(0x0007, 0, , , , );
> -
>   c->x86_capability[CPUID_7_0_EBX] = ebx;
> -
> - c->x86_capability[CPUID_6_EAX] = cpuid_eax(0x0006);
>   c->x86_capability[CPUID_7_ECX] = ecx;
>   }

Perhaps we should have something like:

void cpuid_cond(u32 leaf, u32 subleaf,
u32 *eax, u32 *ebx, u32 *ecx, u32 *edx)
{
const struct cpuinfo_x86 *c = this_cpu_ptr(_info);
u32 maxleaf;
int cpuid_level = c->cpuid_level;

switch (leaf >> 16) {
case 0x:
maxleaf = c->cpuid_level;
break;
case 0x8000:
maxleaf = c->extended_cpuid_level;
break;
case 0x4000 .. 0x40ff:
/* Not ideal, can we do better? */
maxleaf = cpu_has(X86_FEATURE_HYPERVISOR)
  ? 0x40ff : 0;
break;
/* Add Transmeta 0x8086 and VIA/Cyrix 0xc000? */
default:
/* Can we do better? */
if (c->x86_vendor_id) == X86_VENDOR_INTEL)
maxleaf = 0;
else
maxleaf = leaf | 0x;
break;
}

if (cpuid_level >= 0 && leaf >= maxleaf) {
cpuid_count(leaf, subleaf, eax, ebx, ecx, edx);
} else {
*eax = *ebx = *ecx = *edx = 0;
}
}

-hpa

P.S. I would love to (a) move the CPUID bits into a structure instead of
passing all those crazy pointers around, and (b) stop passing struct
cpuinfo * around when we only use it for the current processor anyway (a
lot of these functions are in fact completely invalid if we don't); we
could define this_cpu_info as (*this_cpu_ptr(_info)) -- basically
what I have above -- or directly use percpu functions to access these
variables.