From: Andi Kleen <[email protected]>
The Intel SDM Vol 3 9.11.1 Microcode update states that
the update revision field is signed. However we do the comparison
unsigned, as the comparison gets promoted. Change the field
to be signed, so that comparision is really signed.
v2: Change field.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/include/asm/microcode_intel.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 9067166..ed1884b 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -5,7 +5,7 @@
struct microcode_header_intel {
unsigned int hdrver;
- unsigned int rev;
+ int rev;
unsigned int date;
unsigned int sig;
unsigned int cksum;
--
1.8.3.1
From: Andi Kleen <[email protected]>
For testing purposes it can be useful to downgrade microcode.
Normally the driver only allows upgrading.
Add a module_param (default off) that allows downgrading.
Note the module_param can currently not be set for early
ucode update, only for late.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/kernel/microcode_intel_lib.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/kernel/microcode_intel_lib.c b/arch/x86/kernel/microcode_intel_lib.c
index ce69320..18d5325 100644
--- a/arch/x86/kernel/microcode_intel_lib.c
+++ b/arch/x86/kernel/microcode_intel_lib.c
@@ -26,11 +26,16 @@
#include <linux/uaccess.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/moduleparam.h>
#include <asm/microcode_intel.h>
#include <asm/processor.h>
#include <asm/msr.h>
+static bool allow_downgrade;
+module_param(allow_downgrade, bool, 0644);
+MODULE_PARM_DESC(allow_downgrade, "Allow downgrading microcode");
+
static inline int
update_match_cpu(unsigned int csig, unsigned int cpf,
unsigned int sig, unsigned int pf)
@@ -41,6 +46,8 @@ update_match_cpu(unsigned int csig, unsigned int cpf,
int
update_match_revision(struct microcode_header_intel *mc_header, int rev)
{
+ if (allow_downgrade)
+ return 1;
return (mc_header->rev <= rev) ? 0 : 1;
}
--
1.8.3.1
On Fri, 24 Jan 2014, Andi Kleen wrote:
> For testing purposes it can be useful to downgrade microcode.
> Normally the driver only allows upgrading.
The code is not prepared to work correctly when downgrading is allowed, in
the presence of shadowed microcode. When a firmware request results in more
than one microcode for the same cpuid, with overlapping pf_mask, the code
*depends* on the "never downgrade" logic to work.
Shadowed microcode *is* currently distributed by Intel, and as an artifact
of the f-m-s grouping, it is *guaranteed* to trigger the issue. When the
issue triggers, what microcode will be selected to be uploaded to the core
depends *only* on the order of the microcodes in the firmware file.
I see no documentation of this fact anywhere, and it is *anything* but
obvious.
That extremely obnoxious Intel microcode license forbids anyone to fix the
pf_mask metadata fields to remove shadowing, so we ship that stuff as-is to
in the distros. It *will* hit users.
Also, since you're going to mess with this, why don't you implement the
correct semanthics for microcode with the sign bit set? Making it signed
actually makes the current code behaviour worse.
Refer to: http://lkml.org/lkml/2011/3/21/522
Note that I was wrong about negative revision microcode not being found in
the wild. Intel has shipped entry-level server boards with pre-release
microcode several times, even on BIOS updates, and you're likely to get
access to such pre-release microcode if you're dealing with Intel firmware
partners that has full access to microcode updates.
--
"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
On Sat, Jan 25, 2014 at 02:35:58PM -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 24 Jan 2014, Andi Kleen wrote:
> > For testing purposes it can be useful to downgrade microcode.
> > Normally the driver only allows upgrading.
>
> The code is not prepared to work correctly when downgrading is allowed, in
> the presence of shadowed microcode. When a firmware request results in more
As I wrote it's only for testing purposes when you know what you're doing
(typically with a special micro code file)
Your whole argument is irrelevant, as it only applies to normal users
who should never use this option.
> Also, since you're going to mess with this, why don't you implement the
> correct semanthics for microcode with the sign bit set? Making it signed
> actually makes the current code behaviour worse.
>
> Refer to: http://lkml.org/lkml/2011/3/21/522
I don't think it makes it worse. In fact I'm essentially implementing
Burt's request "for explicit user action" with the new override option.
Anyways I suppose your rant killed the patch anyways. Congratulations!
-Andi
On Sat, 25 Jan 2014, Andi Kleen wrote:
> On Sat, Jan 25, 2014 at 02:35:58PM -0200, Henrique de Moraes Holschuh wrote:
> > On Fri, 24 Jan 2014, Andi Kleen wrote:
> > > For testing purposes it can be useful to downgrade microcode.
> > > Normally the driver only allows upgrading.
> >
> > The code is not prepared to work correctly when downgrading is allowed, in
> > the presence of shadowed microcode. When a firmware request results in more
>
> As I wrote it's only for testing purposes when you know what you're doing
> (typically with a special micro code file)
>
> Your whole argument is irrelevant, as it only applies to normal users
> who should never use this option.
It certainly could become a lot less relevant if any indication is given to
normal users that they should never enable the feature unless they really
know what they're doing.
It is NOT "only for testing purposes" when that fact is written nowhere an
user would see.
> > Also, since you're going to mess with this, why don't you implement the
> > correct semanthics for microcode with the sign bit set? Making it signed
> > actually makes the current code behaviour worse.
> >
> > Refer to: http://lkml.org/lkml/2011/3/21/522
>
> I don't think it makes it worse. In fact I'm essentially implementing
> Burt's request "for explicit user action" with the new override option.
You're correct. It doesn't make it worse, it makes it better. Your change
effectively forbids loading pre-release microcode on a box which has
production microcode, unless "downgrade mode" is enabled.
This is different from what Burt requested, but if you're implementing a
downgrade mode, it is certainly a much better behavior.
> Anyways I suppose your rant killed the patch anyways. Congratulations!
You decided to ignore feedback you got weeks ago that required just adding a
comment to the code or to the commit log to address. Whether or not you
consider said feedback particularly interesting or relevant does NOT give
you the right to just handwave it away only to later blame others should the
patch not get accepted.
--
"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