When kernel do early microcode update, code printing only
new microcode version. But in this case we no have chance
to check which version was in cpu from BIOS vendor.
Patch add this info into output message.
Signed-off-by: Petr Oros <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index d9e460fc7a3b..78330d29cd4c 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -515,9 +515,11 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
* Print ucode update info.
*/
static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+print_ucode_info(struct ucode_cpu_info *uci, unsigned int date,
+ unsigned int prev_rev)
{
- pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
+ pr_info_once("microcode updated early from revision 0x%x to 0x%x, date = %04x-%02x-%02x\n",
+ prev_rev,
uci->cpu_sig.rev,
date & 0xffff,
date >> 24,
@@ -528,6 +530,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
static int delay_ucode_info;
static int current_mc_date;
+static int prev_revision;
/*
* Print early updated ucode info after printk works. This is delayed info dump.
@@ -538,7 +541,7 @@ void show_ucode_info_early(void)
if (delay_ucode_info) {
collect_cpu_info_early(&uci);
- print_ucode_info(&uci, current_mc_date);
+ print_ucode_info(&uci, current_mc_date, prev_revision);
delay_ucode_info = 0;
}
}
@@ -547,11 +550,12 @@ void show_ucode_info_early(void)
* At this point, we can not call printk() yet. Delay printing microcode info in
* show_ucode_info_early() until printk() works.
*/
-static void print_ucode(struct ucode_cpu_info *uci)
+static void print_ucode(struct ucode_cpu_info *uci, unsigned int prev_rev)
{
struct microcode_intel *mc;
int *delay_ucode_info_p;
int *current_mc_date_p;
+ int *prev_revision_p;
mc = uci->mc;
if (!mc)
@@ -559,13 +563,16 @@ static void print_ucode(struct ucode_cpu_info *uci)
delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
current_mc_date_p = (int *)__pa_nodebug(¤t_mc_date);
+ prev_revision_p = (int *)__pa_nodebug(&prev_revision);
*delay_ucode_info_p = 1;
*current_mc_date_p = mc->hdr.date;
+ *prev_revision_p = prev_rev;
}
#else
-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(struct ucode_cpu_info *uci,
+ unsigned int prev_rev)
{
struct microcode_intel *mc;
@@ -573,19 +580,21 @@ static inline void print_ucode(struct ucode_cpu_info *uci)
if (!mc)
return;
- print_ucode_info(uci, mc->hdr.date);
+ print_ucode_info(uci, mc->hdr.date, prev_rev);
}
#endif
static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
{
struct microcode_intel *mc;
- u32 rev;
+ u32 rev, prev_rev;
mc = uci->mc;
if (!mc)
return 0;
+ prev_rev = intel_get_microcode_revision();
+
/* write microcode via MSR 0x79 */
native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
@@ -596,9 +605,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
uci->cpu_sig.rev = rev;
if (early)
- print_ucode(uci);
+ print_ucode(uci, prev_rev);
else
- print_ucode_info(uci, mc->hdr.date);
+ print_ucode_info(uci, mc->hdr.date, prev_rev);
return 0;
}
--
2.16.1
On Fri, Jan 26, 2018 at 11:34:50AM +0100, Petr Oros wrote:
> When kernel do early microcode update, code printing only
> new microcode version. But in this case we no have chance
> to check which version was in cpu from BIOS vendor.
And we need that because?
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Borislav Petkov píše v Pá 26. 01. 2018 v 11:41 +0100:
> On Fri, Jan 26, 2018 at 11:34:50AM +0100, Petr Oros wrote:
> > When kernel do early microcode update, code printing only
> > new microcode version. But in this case we no have chance
> > to check which version was in cpu from BIOS vendor.
>
> And we need that because?
>
During time of facing the Spectre vulnerability and the requirement of handling
different microcode updates, it has shown, that it would be useful to have
additional information which microcode version was in BIOS, if the early-update
routine will apply an update.
On Fri, Jan 26, 2018 at 12:45:35PM +0100, Petr Oros wrote:
> During time of facing the Spectre vulnerability and the requirement of handling
> different microcode updates, it has shown, that it would be useful to have
> additional information which microcode version was in BIOS, if the early-update
> routine will apply an update.
Dmesg tells you if the update was applied or not.
And if you really wanna know the old microcode revision, you can boot
with "dis_ucode_ldr" and do "grep microcode /proc/cpuinfo".
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Borislav Petkov píše v Pá 26. 01. 2018 v 12:58 +0100:
> On Fri, Jan 26, 2018 at 12:45:35PM +0100, Petr Oros wrote:
> > During time of facing the Spectre vulnerability and the requirement of handling
> > different microcode updates, it has shown, that it would be useful to have
> > additional information which microcode version was in BIOS, if the early-update
> > routine will apply an update.
>
> Dmesg tells you if the update was applied or not.
>
> And if you really wanna know the old microcode revision, you can boot
> with "dis_ucode_ldr" and do "grep microcode /proc/cpuinfo".
>
Ohh, yes, it is easy and fast. Mainly in time when new microcodes are released
frequently and microcode can be early updated from initrd too.
But what in production? Edit boot params, restart server, grep /proc/cpuinfo and
restart again? Why i can not read it just from dmesg?
Thanks,
-Petr
On Fri, Jan 26, 2018 at 02:50:00PM +0100, Petr Oros wrote:
> But what in production? Edit boot params, restart server, grep /proc/cpuinfo and
> restart again? Why i can not read it just from dmesg?
Because you don't need the previous revision.
You only *happen* to need it now but that is being addressed too with
the blacklisting. And when you have broken microcode, it will say:
+ pr_warn("Intel Spectre v2 broken microcode detected; disabling SPEC_CTRL\n");
and if you have microcode which doesn't have IBRS, there won't be
"spec_ctrl" in /proc/cpuinfo.
I don't want people to start paying attention to microcode
revision numbers with the gazillion different revisions and
family/model/steppings out there and the crazy confusion that will ensue
from this.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
Borislav Petkov píše v Pá 26. 01. 2018 v 15:49 +0100:
> On Fri, Jan 26, 2018 at 02:50:00PM +0100, Petr Oros wrote:
> > But what in production? Edit boot params, restart server, grep /proc/cpuinfo and
> > restart again? Why i can not read it just from dmesg?
>
> Because you don't need the previous revision.
>
> You only *happen* to need it now but that is being addressed too with
> the blacklisting. And when you have broken microcode, it will say:
>
> + pr_warn("Intel Spectre v2 broken microcode detected; disabling SPEC_CTRL\n");
>
> and if you have microcode which doesn't have IBRS, there won't be
> "spec_ctrl" in /proc/cpuinfo.
>
> I don't want people to start paying attention to microcode
> revision numbers with the gazillion different revisions and
> family/model/steppings out there and the crazy confusion that will ensue
> from this.
>
We talk about dmesg log, it is place for diagnostic messages.
I think, people expected gazillion numbers here.
I adding only one number (microcode version) into log and only if patch is
applied. This is good also for future bugs, and for example for tools like
sosreport.
It is really better to have microcode version ready and not try find it and try
to collect after problem...
Btw: with hot microcode patch is possible to get previous version from dmesg
[ 0.680519] microcode: sig=0x306c3, pf=0x10, revision=0x23
Only with early it is not possible.
Thanks,
-Petr
Hello Borislav,
On Fri, 2018-01-26 at 15:49 +0100, Borislav Petkov wrote:
> On Fri, Jan 26, 2018 at 02:50:00PM +0100, Petr Oros wrote:
> > But what in production? Edit boot params, restart server, grep
> > /proc/cpuinfo and
> > restart again? Why i can not read it just from dmesg?
>
> Because you don't need the previous revision.
>
> You only *happen* to need it now but that is being addressed too with
> the blacklisting. And when you have broken microcode, it will say:
Although Spectre might be the most visible CPU issue, it's not the only
one. What if some issue causes failure during early microcode update?
What if the issue triggers only on update from a certain microcode
version? We should be transparent about what microcode version we
update from and to.
The double reboot with "dis_ucode_ldr" argument requires to schedule a
full system reboot just to figure out what version has been provided by
the system firmware.
> + pr_warn("Intel Spectre v2 broken microcode detected;
> disabling SPEC_CTRL\n");
>
> and if you have microcode which doesn't have IBRS, there won't be
> "spec_ctrl" in /proc/cpuinfo.
>
> I don't want people to start paying attention to microcode
> revision numbers with the gazillion different revisions and
> family/model/steppings out there and the crazy confusion that will
> ensue
> from this.
The current microcode version is already printed in the dmesg. Many
people do care what revision they are running and what provided this
revision. It is the most important information on triaging CPU issues,
especially if anything goes awry.
I would appreciate if you could pull this patch in.
Thank you,
-Stanislav
On Wed, Feb 07, 2018 at 03:02:13PM +0100, Stanislav Kozina wrote:
> Although Spectre might be the most visible CPU issue, it's not the only
> one. What if some issue causes failure during early microcode update?
And you think failure to update early microcode will allow you to see
*anything* printed on the screen?
All the upgrade failures I've seen so far result in freezes and triple
faults.
> What if the issue triggers only on update from a certain microcode
> version? We should be transparent about what microcode version we
> update from and to.
We blacklist those microcode revisions. And we're transparent:
pr_warn("Intel Spectre v2 broken microcode detected; disabling Speculation Control\n");
Note that we *don't* issue the old revision here either.
We issue it only for this one moronic late loading case where the
erratum is limited to a certain model of certain size and with certain
minimum revision (and when the moon and the sun are aligned properly):
if (c->x86 == 6 &&
c->x86_model == INTEL_FAM6_BROADWELL_X &&
c->x86_mask == 0x01 &&
llc_size_per_core > 2621440 &&
c->microcode < 0x0b000021) {
pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
because there we have a minimal good version.
And mind you, we don't really need it there either because we have
blacklisted the old revision and user can see it in /proc/cpuinfo.
> The double reboot with "dis_ucode_ldr" argument requires to schedule a
> full system reboot just to figure out what version has been provided by
> the system firmware.
With the proper blacklisting code - which is already upstream:
a5b296636453 ("x86/cpufeature: Blacklist SPEC_CTRL/PRED_CMD on early Spectre v2 microcodes")
you don't need to do any of that. You either get the upgrade or you
don't and if you don't, speculation control gets disabled.
> The current microcode version is already printed in the dmesg. Many
> people do care what revision they are running and what provided this
> revision. It is the most important information on triaging CPU issues,
> especially if anything goes awry.
The microcode revision is the most important information on triaging CPU
issues??!?! Yeah, right.
It sounds to me you're just grasping for arguments to validate having
the previous revision in dmesg.
Gah, how hard is it to understand: the microcode revision is a detail
like a gazillion other details pertaining to a platform.
Look at the commit above: that's 23 *different* revisions. Now, if you
wanna talk about a blacklisted revision, you need to add the CPU *model*
*and* *stepping* to the report too because those revisions are per model
and stepping.
And I wouldn't be surprised if some revisions are also *additionally*
determined by platform flags and whatnot.
That's making it worse, not better.
Now let's take a step back: you don't really need the previous revision
in the spectre case! Not really.
Why?
Well:
1. if the blacklisted revision can be safely updated to a good one, then
you don't care.
2. If it cannot be safely updated by the OS microcode loading mechanism,
then printing the previous revision doesn't help either: there you need
to *disable* the updating of the microcode so that the machine remains
somewhat usable and the update doesn't make it worse.
Still don't need the old revision printed.
3. If it is one of those blacklisted patches which makes the box
explode, then you more than ever don't need printing the old revision -
you need to disable updating to the blacklisted microcode.
So there's not a single case where you need to print the old revision
and confuse bug reporters and debuggers.
Instead, you need to get the proper blacklisting mechanism in place.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
> Now let's take a step back: you don't really need the previous revision
> in the spectre case! Not really.
I understand all your points.
We submitted the patch to ease debugging of microcode issues where the
systems impacted are not yet identified and thus can't be blacklisted.
Best Regards,
-Stanislav