linux-kernel - Re: [PATCH] x86: Report Intel platform

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160602182707.GA15431@khazad-dum.debian.net>
Date:	Thu, 2 Jun 2016 15:27:07 -0300
From:	Henrique de Moraes Holschuh <hmh@....eng.br>
To:	Andi Kleen <andi@...stfloor.org>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] x86: Report Intel platform_id in /proc/cpuinfo

On Tue, 31 May 2016, Andi Kleen wrote:
> We have a need to distinguish systems based on their platform ID.
> For example this is useful to distinguish systems with L4 cache
> versus ones without.
> 
> There is a 5 bit identifier (also called processor flags) in

There is a 3 bit identifier...

> the IA32_PLATFORM_ID MSR that can give a more fine grained
> identification of the CPU than just the model number/stepping.
> 
> IA32_PLATFORM_ID is architectural.
> 
> The processor flags are already used in the microcode driver.
> The MSR can be also accessed through /dev/cpu/*/msr, but that
> requires root and is awkward.
> 
> This patch just exports the value retrieved by the microcode
> driver in /proc/cpuinfo. If the microcode driver is disabled
> it won't be shown, but that seems reasonable.

...

> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index ee81c54..6244a88 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -812,6 +812,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>  		/* get processor flags from MSR 0x17 */
>  		rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
>  		csig->pf = 1 << ((val[1] >> 18) & 7);
> +		cpu_data(cpu_num).platform_id = (val[1] >> 18) & 7;

See below.  It might be better to have "cpu_data(cpu_num).platform_id =
csig->pf" instead.

> diff --git a/arch/x86/kernel/cpu/proc.c b/arch/x86/kernel/cpu/proc.c
> index 18ca99f..1c4e4f5 100644
> --- a/arch/x86/kernel/cpu/proc.c
> +++ b/arch/x86/kernel/cpu/proc.c
> @@ -76,6 +76,8 @@ static int show_cpuinfo(struct seq_file *m, void *v)
>  		seq_puts(m, "stepping\t: unknown\n");
>  	if (c->microcode)
>  		seq_printf(m, "microcode\t: 0x%x\n", c->microcode);
> +	if (c->platform_id)
> +		seq_printf(m, "platform_id\t: %d\n", c->platform_id);

platform_id can meaningfully be zero on Intel, and in fact it often is (just
look for output from the microcode driver that logs pf=0x1).  In those
processors, (MSR(17) >> 18 & 7) is zero, and csig->pf is 1.

May I humbly suggest using the mask "(1 << value)" notation for platform_id
as used by the microcode driver?  We already do it for csig->pf, and it has
the advantage that mask notation will never be zero (so, the field is always
non-zero where relevant).

Alterntively, the patch could be changed to always print platform_id on
Intel processors, instead of just printing it out when it is non-zero.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh