Re: [PATCH] x86: Report Intel platform_id in /proc/cpuinfo

[Andi Kleen]

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