lkml.org 
[lkml]   [2011]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/3] x86, intel: Output microcode revision

* Andi Kleen <ak@linux.intel.com> wrote:

> On Wed, May 25, 2011 at 08:54:51AM +0200, Ingo Molnar wrote:
> >
> > > /* Index into per_cpu list: */
> > > u16 cpu_index;
> > >
> > > + /* CPU update signature */
> > > + u32 x86_cpu_update;
> >
> > This should be cpu_microcode_version instead. We already know its x86 so the
> > x86_ prefix is superfluous. 'cpu_update' is also rather ambigious and does not
>
> I followed the convention of the other fields in cpu_data, [...]

Look at the context diff above, it has 'cpu_index', so no, there was no
consistent convention to follow.

The fields are arguably pretty inconsistent in 'struct cpuinfo_x86',
but when adding new fields they should be sane.

> [...] but I'll drop the prefix.
>
> "cpu update" was the name that was suggested to me. Sorry if it's a
> bit vague. Apparently calling it microcode is not politically
> correct. I'll keep it for now. If you want to really change it
> please send another email.
>
> > So, during the course of developing this, did it occur to you
> > that other x86 CPUs should fill in this information as well?
>
> Yes, it did in fact, but I hope someone else will do that because I
> have no way to test it.

And not Cc:-ing anyone and not mentioning that the microcode driver
of other vendors needs to be updated was the right solution to raise
attention to that lack of means of testing? :-)

> > > - /* see notes above for revision 1.07. Apparent chip bug */
>
> This particular code pattern has no chip bug. The CPUID is required
> by the documentation! So whoever wrote it didn't read the
> documentation. So yes I dropped that obviously bogus comment.

And you thus 'obviously' forked away the reading of the microcode
version into another file, with the same 'obviously wrong' comment
left behind in another place?

Not very smart from you.

> > Why? If you think it's not a bug (but got documented meanwhile as
> > the official
>
> It always was documented this way.

FYI, the x86 microcode driver actually predates official public
documentation on the topic, so no, it was not always documented that
way ;-)

[ I suspect the CPUID was 'documented' after it was found out the
hard way that writing the MSR and reading it out without the CPUID
can give random junk as the version number. ]

> > way of updating the microcode and reading out the version) then
> > update the comments in a *separate* patch, and update the *other*
> > reference to it as well.
>
> I'm not sure about the other references. The documentation just
> states the CPUID is needed for reading the revision.
>
> Anyways I'll just leave the comment around for now.
>
> > > +
> > > + seq_printf(m, "\n");
> >
> > This too should say microcode version.
> >
> > Also, please move the field to the logical place, next to
> > "stepping:". The argument about parsers is bogus - anyone parsing
> > /proc/cpuinfo is not in a fastpath, full stop.
>
> The rationale was not cycles, but if someone was stupid enough to
> hardcode the line number offsets while parsing. You never know with
> user space /proc parsers and assuming the worst is always the best.
>
> I thought it was safest to add new fields at the end.

No, it's not a problem to add /proc/cpuinfo fields in the middle -
please add this new field to the logical place.

> > Also, the above sequence is rather suboptimal to begin with - we can and only
> > want to execute a *single* seq_printf() there.
>
> Sorry I don't understand the comment. Anyways as you say above
> it's no fast path, so it shouldn't matter.
>
> > Please factor out the reading of the microcode version - you have now created
> > two duplicated open-coded versions of it in cpu.c and microcode_intel.c, with
> > mismatching comments - not good.
>
> Huh? There's only a single one now.

That's not actually true. With your patches applied a trivial git
grep shows the two places reading the microcode version:

arch/x86/kernel/cpu/intel.c: wrmsr(MSR_IA32_UCODE_REV, 0, 0);
arch/x86/kernel/cpu/intel.c: rdmsr(MSR_IA32_UCODE_REV, lo, c->x86_cpu_update);
arch/x86/kernel/microcode_intel.c: wrmsr(MSR_IA32_UCODE_REV, 0, 0);
arch/x86/kernel/microcode_intel.c: rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

As i said you have now created two duplicated open-coded versions of
it in cpu.c and microcode_intel.c, with mismatching comments - not
good.

Andi, are you being obtuse and are you wasting my time intentionally?
You could have checked this yourself.

Thanks,

Ingo


\
 
 \ /
  Last update: 2011-05-25 21:01    [from the cache]
©2003-2011 Jasper Spaans