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 for Android: free password hash cracker in your pocket

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

Message-ID: <20160602225439.GF13997@two.firstfloor.org>
Date:	Thu, 2 Jun 2016 15:54:40 -0700
From:	Andi Kleen <andi@...stfloor.org>
To:	Henrique de Moraes Holschuh <hmh@....eng.br>
Cc:	Andi Kleen <andi@...stfloor.org>, 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 Thu, Jun 02, 2016 at 03:27:07PM -0300, Henrique de Moraes Holschuh wrote:
> 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...

Thanks.

> > 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.

Ok.

> 
> > 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).

That would be confusing because the value is used in some documents.
I would prefer to match them.

> 
> 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.

What I can do is to add an extra flag and print it when the flag is set,
even if it is zero.

Thanks for the review.

-Andi