Message-ID: <20141031184344.GB27028@khazad-dum.debian.net>
Date:	Fri, 31 Oct 2014 16:43:45 -0200
From:	Henrique de Moraes Holschuh <hmh@....eng.br>
To:	Borislav Petkov <bp@...en8.de>
Cc:	linux-kernel@...r.kernel.org, H Peter Anvin <hpa@...or.com>
Subject: Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core
 twice

On Tue, 28 Oct 2014, Borislav Petkov wrote:
> On Mon, Oct 20, 2014 at 04:24:27PM -0200, Henrique de Moraes Holschuh wrote:
> > Over time, grepping for that information on reports and logs all over the
> > net has helped me a great deal.
> 
> Helped you how, for what? I still am searching for a justification to
> bother the user with the fact that her microcode just got upgraded. I
> mean, she can simply do:

Every time I issued a warning to users that they need to upgrade their
microcode due to either security errata or other nasty issue, the fact that
you can just grep for microcode in the logs to know whether it worked or not
*and* the reasons why it did/did not work has been helpful.

The logging also tells me whether the early or regular microcode driver was
the one that did the update.  This matters to me, as it helps me detect when
the regular microcode update driver is doing work that should have been done
by the early driver.

It was also really useful when I was searching the net for reports about a
specific processor signature, /proc/cpuinfo is really unhelpful for that.

It also gives me one interesting information that just looking at
/proc/cpuinfo doesn't: the version of the microcode installed by the user's
BIOS/UEFI.  This kind of information is relevant to some of the Linux
distros (at least to Debian) due to the non-free nature of the microcode
updates [note: this assumes the regular microcode driver was loaded].  It is
the only metric I have that tells me whether the effort is worth it.

Let me propose a way forward that would improve the status quo re. reducing
the logging, and still log the information I find useful:  If you agree, I
will write code to greatly reduce the number of log lines generated by the
combination of the early microcode and regular microcode intel driver.

I think I would be able to reduce it to *two* lines total at most, per boot,
in the general case.  And it will likely be just one line, depends on
whether the early microcode driver was compiled in or not, and whether it or
the regular microcode driver managed to update the processor.

I will write that code for the next patchset (after I update this one until
it gets into a shape you can accept).

> > I really miss the full microcode ID information in /proc/cpuinfo, in fact.
> 
> Full ID, you mean all fields of struct cpu_signature on Intel?

cpu signature, pf mask and revision.  Since we already log the date, might
as well keep it too.

>    ->sig - CPUID_EAX(1) which is in /proc/cpuinfo

Not in a format amicable to finding it through a search engine.  Besides, we
lose the processor type bits.  Given that we have a brand new i586 chip, who
is to say we won't see once again processors that have one of those bits set
in their signature?

>    ->pf - processor flags in MSR_0x17[52:50] - I guess you can read that
>    out with rdmsr 0x17. Why do we need to know that one except maybe to
>    verify why a patch doesn't get accepted by the loader?

This one has very limited use right now, yes.  But without it, I cannot even
tell whether the user has an up-to-date microcode or not when Intel releases
microcode updates that do not apply to all processors with the same
CPUID_EAX(1).  They haven't done this in the last couple years, but it is
needed for processors that are not that old, such as cpuid 0x30661 (Intel
Atom D25xx/N26xx,N28xx, from 2011).  It is likely it will be needed again.

>    -> rev - that's in MSR_IA32_UCODE_REV

> I'm not really sure we absolutely need those except for debugging. Thus
> the pr_debug() suggestion from my side.

It is for debugging, indeed.  But too much stuff never logs at the pr_debug
level, so you need to take special action.  If you accept my proposal for
log size reduction while still logging the stuff I find useful, this could
be pr_info() as it would *not* be per-core anymore.

> > This is old code, I guess it predates wrmsrl()...
> > 
> > Should I replace the old split version with wrmsrl() in this patch, or as a
> > separate patch?
> 
> Yes please. And then add to the commit message something of the sorts
> "While at it, ..."

Will do.

-- 
  "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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists