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

Andi Kleen
2016-06-02T22:54:44Z

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