[PATCH 0/8] x86, microcode, intel: fixes and enhancements

[PATCH 0/8] x86, microcode, intel: fixes and enhancements

[Henrique de Moraes Holschuh]

This patchset applies on top of:
git://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git microcode

It includes a regression fix (patch 2), an architectural requirement
violation fix (patch 7), and several enhancements.

Patch 7 (x86, microcode, intel: guard against misaligned microcode data)
has to align and restore the microcode data for each core that needs an
update.  The extra time to align the microcode data was not noticeable
in simple testing: it got lost in the noise of WRMSR 0x79.

FWIW, iucode-tool v1.0.3+ (https://gitorious.org/iucode-tool/pages/Home)
will pre-align the microcode in the early-initramfs, so that the kernel
won't have to do it.  Patch 7 documents two hacks to pre-align the
microcode data inside the early-initramfs so that people can write their
own tools to do it, if they want to.

Henrique de Moraes Holschuh (8):
  x86, microcode, intel: forbid some incorrect metadata
  x86, microcode, intel: don't update each HT core twice
  x86, microcode, intel: clarify log messages
  x86, microcode, intel: add error logging to early update driver
  x86, microcode, intel: don't check extsig entry checksum
  x86, microcode, intel: use cpuid explicitly instead of sync_core
  x86, microcode, intel: guard against misaligned microcode data
  x86, microcode, intel: defend apply_microcode_intel with BUG_ON

 Documentation/x86/early-microcode.txt       |   10 ++
 arch/x86/include/asm/microcode_intel.h      |   12 ++
 arch/x86/kernel/cpu/microcode/intel.c       |   58 ++++++----
 arch/x86/kernel/cpu/microcode/intel_early.c |  158 ++++++++++++++++-----------
 arch/x86/kernel/cpu/microcode/intel_lib.c   |   55 +++++-----
 5 files changed, 186 insertions(+), 107 deletions(-)

-- 
1.7.10.4

[PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

The Intel SDM vol 3A, section 9.11.1, and also table 9-6, requires that
the Data Size field be a multiple of 4 bytes.  All of the microcode
update header structures are dword-based, so the Total Size field must
also be a multiple of the dword size.

Ensure that data_size is a multiple of the dword size (4 bytes).  The
driver code assumes this to be true for both data_size and total_size,
and will not work correctly otherwise.

Futhermore, require that total_size be a multiple of 1024, as per the
Intel SDM, vol 3A, section 9.11.1, page 9-28; table 9-6, page 9-29, and
others.  Test added by request of Borislav Petkov.

Also refuse a microcode update with a microcode revision of zero.
According to the Intel SDM, vol 3A, section 9.11.7, page 9-36, a
microcode revision of zero is special:

    "CPUID returns a value in a model specific register in addition to
    its usual register return values.  The semantics of CPUID cause it
    to deposit an update ID value in the 64-bit model-specific register
    at address 08BH (IA32_BIOS_SIGN_ID).  If no update is present in the
    processor, the value in the MSR remains unmodified.  The BIOS must
    pre-load a zero into the MSR before executing CPUID.  If a read of
    the MSR at 8BH still returns zero after executing CPUID, this
    indicates that no update is present."

This effectively reserves revision zero to mean "no microcode update
installed on the processor": the microcode loader cannot differentiate
sucess from failure when updating microcode to the same revision as the
one currently installed on the processor, and this would always happen
to updates to revision zero in the BIOS/UEFI loader.

There is every reason to be paranoid about any microcode update with a
revision of zero, as Intel will never release such a microcode update.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel_lib.c |   25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index ce69320..25915e3 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -55,9 +55,10 @@ int microcode_sanity_check(void *mc, int print_err)
 	total_size = get_totalsize(mc_header);
 	data_size = get_datasize(mc_header);
 
-	if (data_size + MC_HEADER_SIZE > total_size) {
+	if ((data_size % DWSIZE) || (total_size % 1024) ||
+	    (data_size + MC_HEADER_SIZE > total_size)) {
 		if (print_err)
-			pr_err("error! Bad data size in microcode data file\n");
+			pr_err("error: bad data size or total size in microcode data file\n");
 		return -EINVAL;
 	}
 
@@ -83,6 +84,26 @@ int microcode_sanity_check(void *mc, int print_err)
 		ext_sigcount = ext_header->count;
 	}
 
+	/*
+	 * A version 1 loader cannot differentiate failure from success when
+	 * attempting a microcode update to the same revision as the one
+	 * currently installed.  The loader is supposed to never attempt a
+	 * same-version update (or a microcode downgrade, for that matter).
+	 *
+	 * This will always cause issues for microcode updates to revision zero
+	 * in the UEFI/BIOS microcode loader: the processor reports a revision
+	 * of zero when it is running without any microcode updates installed,
+	 * such as after a reset/power up.
+	 *
+	 * Intel will never issue a microcode update with a revision of zero
+	 * for the version 1 loader.  Reject it.
+	 */
+	if (mc_header->rev == 0) { /* reserved for "no-update-installed" */
+		if (print_err)
+			pr_err("error: restricted revision 0 in microcode data file\n");
+		return -EINVAL;
+	}
+
 	/* check extended table checksum */
 	if (ext_table_size) {
 		int ext_table_sum = 0;
-- 
1.7.10.4

Re: [PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:47PM -0300, Henrique de Moraes Holschuh wrote:
> The Intel SDM vol 3A, section 9.11.1, and also table 9-6, requires that
> the Data Size field be a multiple of 4 bytes.  All of the microcode
> update header structures are dword-based, so the Total Size field must
> also be a multiple of the dword size.
> 
> Ensure that data_size is a multiple of the dword size (4 bytes).  The
> driver code assumes this to be true for both data_size and total_size,
> and will not work correctly otherwise.
> 
> Futhermore, require that total_size be a multiple of 1024, as per the
> Intel SDM, vol 3A, section 9.11.1, page 9-28; table 9-6, page 9-29, and
> others.  Test added by request of Borislav Petkov.
> 
> Also refuse a microcode update with a microcode revision of zero.
> According to the Intel SDM, vol 3A, section 9.11.7, page 9-36, a
> microcode revision of zero is special:
> 
>     "CPUID returns a value in a model specific register in addition to
>     its usual register return values.  The semantics of CPUID cause it
>     to deposit an update ID value in the 64-bit model-specific register
>     at address 08BH (IA32_BIOS_SIGN_ID).  If no update is present in the
>     processor, the value in the MSR remains unmodified.  The BIOS must
>     pre-load a zero into the MSR before executing CPUID.  If a read of
>     the MSR at 8BH still returns zero after executing CPUID, this
>     indicates that no update is present."
> 
> This effectively reserves revision zero to mean "no microcode update
> installed on the processor": the microcode loader cannot differentiate
> sucess from failure when updating microcode to the same revision as the
> one currently installed on the processor, and this would always happen
> to updates to revision zero in the BIOS/UEFI loader.
> 
> There is every reason to be paranoid about any microcode update with a
> revision of zero, as Intel will never release such a microcode update.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel_lib.c |   25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index ce69320..25915e3 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -55,9 +55,10 @@ int microcode_sanity_check(void *mc, int print_err)
>  	total_size = get_totalsize(mc_header);
>  	data_size = get_datasize(mc_header);
>  
> -	if (data_size + MC_HEADER_SIZE > total_size) {
> +	if ((data_size % DWSIZE) || (total_size % 1024) ||
> +	    (data_size + MC_HEADER_SIZE > total_size)) {
>  		if (print_err)
> -			pr_err("error! Bad data size in microcode data file\n");
> +			pr_err("error: bad data size or total size in microcode data file\n");

Shorten:

	pr_err("error: bad data/total size in microcode data file\n");

>  		return -EINVAL;
>  	}
>  
> @@ -83,6 +84,26 @@ int microcode_sanity_check(void *mc, int print_err)
>  		ext_sigcount = ext_header->count;
>  	}
>  
> +	/*
> +	 * A version 1 loader cannot differentiate failure from success when
> +	 * attempting a microcode update to the same revision as the one
> +	 * currently installed.  The loader is supposed to never attempt a
> +	 * same-version update (or a microcode downgrade, for that matter).
> +	 *
> +	 * This will always cause issues for microcode updates to revision zero
> +	 * in the UEFI/BIOS microcode loader: the processor reports a revision
> +	 * of zero when it is running without any microcode updates installed,
> +	 * such as after a reset/power up.
> +	 *
> +	 * Intel will never issue a microcode update with a revision of zero
> +	 * for the version 1 loader.  Reject it.
> +	 */

This comment is too long. How about this instead:

	/*
	 * 0 is not a valid microcode revision as it is used to denote the
	 * failure of a microcode update, see MSR 0x8b (IA32_BIOS_SIGN_ID):
	 *
	 * "It is required that this register field be pre-loaded with zero
	 * prior to executing the CPUID, function 1. If the field remains
	 * equal to zero, then there is no microcode update loaded. Another
	 * non-zero value will be the signature."
	 */

This is one of those seldom times where the documentation is actually clear. :-)

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

On Sun, 05 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:47PM -0300, Henrique de Moraes Holschuh wrote:
> > -	if (data_size + MC_HEADER_SIZE > total_size) {
> > +	if ((data_size % DWSIZE) || (total_size % 1024) ||
> > +	    (data_size + MC_HEADER_SIZE > total_size)) {
> >  		if (print_err)
> > -			pr_err("error! Bad data size in microcode data file\n");
> > +			pr_err("error: bad data size or total size in microcode data file\n");
> 
> Shorten:
> 
> 	pr_err("error: bad data/total size in microcode data file\n");

will do.

> > +	/*
> > +	 * A version 1 loader cannot differentiate failure from success when
> > +	 * attempting a microcode update to the same revision as the one
> > +	 * currently installed.  The loader is supposed to never attempt a
> > +	 * same-version update (or a microcode downgrade, for that matter).
> > +	 *
> > +	 * This will always cause issues for microcode updates to revision zero
> > +	 * in the UEFI/BIOS microcode loader: the processor reports a revision
> > +	 * of zero when it is running without any microcode updates installed,
> > +	 * such as after a reset/power up.
> > +	 *
> > +	 * Intel will never issue a microcode update with a revision of zero
> > +	 * for the version 1 loader.  Reject it.
> > +	 */
> 
> This comment is too long. How about this instead:
> 
> 	/*
> 	 * 0 is not a valid microcode revision as it is used to denote the
> 	 * failure of a microcode update, see MSR 0x8b (IA32_BIOS_SIGN_ID):
> 	 *
> 	 * "It is required that this register field be pre-loaded with zero
> 	 * prior to executing the CPUID, function 1. If the field remains
> 	 * equal to zero, then there is no microcode update loaded. Another
> 	 * non-zero value will be the signature."
> 	 */
> 
> This is one of those seldom times where the documentation is actually clear. :-)

Not realy, because it got you confused! :-)

Zero does not denote a failure to update microcode.  What zero means, *when
you did the pre-load and issued a cpuid(1)*, is that the processor microcode
has not been updated since power-on/reset.

What flags a *sucessful* microcode update is a change on IA32_BIOS_SIGN_ID
(which must be read with the zero preload and cpuid(1) protocol).

If IA32_BIOS_SIGN_ID didn't change, the microcode update was rejected...
obviously, this only holds when you never attempt to update the microcode to
the same version the processor already had running.

And that's why we cannot detect whether a same-version update worked or not.

The reason Intel will never issue a microcode with revision zero is because
it cannot be safely applied by UEFI or BIOS at system power up: it would
look like a same-version update (IA32_BIOS_SIGN_ID would return zero before
the update, and would return zero after the update, whether it was applied
sucessfully or not).

And since Intel will never issue such microcode, we don't want to deal with
anything that claims to be a microcode update to revision zero.


IOW, this is a failure:

IA32_BIOS_SIGN_ID before the update is the same as IA32_BIOS_SIGN_ID after
the update attempt.

this is a sucessful update:

IA32_BIOS_SIGN_ID before the update is different from IA32_BIOS_SIGN_ID
after the update attempt.

In any case, you always need to do the zero-preload and cpuid(1) to read
IA32_BIOS_SIGN_ID.

-- 
  "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

Re: [PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Sun, Oct 05, 2014 at 04:37:03PM -0300, Henrique de Moraes Holschuh wrote:
> Not realy, because it got you confused! :-)

No, it didn't get me confused - it got you confused that I'm confused.

You need to read the comment as a *whole*. The zero value is special
because it is *used* to *denote* a failure. You can use any other
invalid revision value for that matter.

Maybe "denote" was not precise enough - maybe it should say "0 is an
invalid microcode revision and is used to detect the failure of a
microcode update" or similar. Yeah, "detect" sounds better.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Henrique de Moraes Holschuh]

On Sun, 05 Oct 2014, Borislav Petkov wrote:
> On Sun, Oct 05, 2014 at 04:37:03PM -0300, Henrique de Moraes Holschuh wrote:
> > Not realy, because it got you confused! :-)
> 
> No, it didn't get me confused - it got you confused that I'm confused.

Indeed.

> You need to read the comment as a *whole*. The zero value is special
> because it is *used* to *denote* a failure. You can use any other
> invalid revision value for that matter.

Well, the new wording is confusing me... so maybe we can try a third time?
:-)

> Maybe "denote" was not precise enough - maybe it should say "0 is an
> invalid microcode revision and is used to detect the failure of a
> microcode update" or similar. Yeah, "detect" sounds better.

How about this:

	/*
	 * 0 is not a valid microcode revision as it is used to detect the
	 * absence of any sucessful microcode update since reset /
	 * power-on, see MSR 0x8b (IA32_BIOS_SIGN_ID):
	 *
	 * "It is required that this register field be pre-loaded with zero
	 * prior to executing the CPUID, function 1. If the field remains
	 * equal to zero, then there is no microcode update loaded. Another
	 * non-zero value will be the signature."
	 */

?

-- 
  "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

Re: [PATCH 1/8] x86, microcode, intel: forbid some incorrect metadata

[Borislav Petkov]

On Sun, Oct 05, 2014 at 06:49:08PM -0300, Henrique de Moraes Holschuh wrote:
> How about this:
> 
> 	/*
> 	 * 0 is not a valid microcode revision as it is used to detect the
> 	 * absence of any sucessful microcode update since reset /
> 	 * power-on, see MSR 0x8b (IA32_BIOS_SIGN_ID):
> 	 *
> 	 * "It is required that this register field be pre-loaded with zero
> 	 * prior to executing the CPUID, function 1. If the field remains
> 	 * equal to zero, then there is no microcode update loaded. Another
> 	 * non-zero value will be the signature."
> 	 */
> 

Yep.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Henrique de Moraes Holschuh]

Fix a regression introduced by 506ed6b53e00ba303ad778122f08e1fca7cf5efb,
"x86, intel: Output microcode revision in /proc/cpuinfo", which added a
cache of the thread microcode revision to cpu_data()->microcode and
switched the microcode driver to using the cached value.

This caused the driver to needlessly update each processor core twice
when hyper-threading is enabled (once per hardware thread).  The early
microcode update code that runs during BSP/AP setup does not have this
problem.

Intel microcode update operations are extremely expensive.  The WRMSR
79H instruction could take anywhere from a hundred-thousand to several
million cycles to successfully apply a microcode update, depending on
processor model and microcode update size.

To avoid updating the same core twice per microcode update run, refresh
the microcode revision of each CPU (hardware thread) before deciding
whether it needs an update or not.

A silent version of collect_cpu_info() is required for this fix,
otherwise the logs produced by a microcode update run would be twice as
long and very confusing.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel.c |   39 ++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c6826d1..2c629d1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -87,7 +87,7 @@ MODULE_DESCRIPTION("Microcode Update Driver");
 MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
 MODULE_LICENSE("GPL");
 
-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
 	unsigned int val[2];
@@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 		csig->pf = 1 << ((val[1] >> 18) & 7);
 	}
 
-	csig->rev = c->microcode;
+	/* get the current microcode revision from MSR 0x8B */
+	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	sync_core();
+	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+
+	csig->rev = val[1];
+	c->microcode = val[1];  /* re-sync */
+}
+
+static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
+{
+	__collect_cpu_info(cpu_num, csig);
+
 	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
 		cpu_num, csig->sig, csig->pf, csig->rev);
 
@@ -118,7 +130,10 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
 	struct cpu_signature cpu_sig;
 	unsigned int csig, cpf, crev;
 
-	collect_cpu_info(cpu, &cpu_sig);
+	/* NOTE: cpu_data()->microcode will be outdated on HT
+	 * processors during an update run, it must be refreshed
+	 * from MSR 0x8B */
+	__collect_cpu_info(cpu, &cpu_sig);
 
 	csig = cpu_sig.sig;
 	cpf = cpu_sig.pf;
@@ -145,23 +160,21 @@ static int apply_microcode_intel(int cpu)
 		return 0;
 
 	/*
-	 * Microcode on this CPU could be updated earlier. Only apply the
-	 * microcode patch in mc_intel when it is newer than the one on this
-	 * CPU.
+	 * Microcode on this CPU might be already up-to-date.  Only apply
+	 * the microcode patch in mc_intel when it is newer than the one
+	 * on this CPU.
 	 */
 	if (get_matching_mc(mc_intel, cpu) == 0)
 		return 0;
 
-	/* write microcode via MSR 0x79 */
+	/* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
 	wrmsr(MSR_IA32_UCODE_WRITE,
-	      (unsigned long) mc_intel->bits,
-	      (unsigned long) mc_intel->bits >> 16 >> 16);
-	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
-	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+		lower_32_bits((unsigned long) mc_intel->bits),
+		upper_32_bits((unsigned long) mc_intel->bits));
 
 	/* get the current revision from MSR 0x8B */
+	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	sync_core();
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
 	if (val[1] != mc_intel->hdr.rev) {
-- 
1.7.10.4

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:48PM -0300, Henrique de Moraes Holschuh wrote:
> Fix a regression introduced by 506ed6b53e00ba303ad778122f08e1fca7cf5efb,
> "x86, intel: Output microcode revision in /proc/cpuinfo", which added a
> cache of the thread microcode revision to cpu_data()->microcode and
> switched the microcode driver to using the cached value.
> 
> This caused the driver to needlessly update each processor core twice
> when hyper-threading is enabled (once per hardware thread).  The early
> microcode update code that runs during BSP/AP setup does not have this
> problem.
> 
> Intel microcode update operations are extremely expensive.  The WRMSR
> 79H instruction could take anywhere from a hundred-thousand to several
> million cycles to successfully apply a microcode update, depending on
> processor model and microcode update size.
> 
> To avoid updating the same core twice per microcode update run, refresh
> the microcode revision of each CPU (hardware thread) before deciding
> whether it needs an update or not.
> 
> A silent version of collect_cpu_info() is required for this fix,
> otherwise the logs produced by a microcode update run would be twice as
> long and very confusing.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c |   39 ++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c6826d1..2c629d1 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -87,7 +87,7 @@ MODULE_DESCRIPTION("Microcode Update Driver");
>  MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
>  MODULE_LICENSE("GPL");
>  
> -static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> +static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
>  	unsigned int val[2];
> @@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>  		csig->pf = 1 << ((val[1] >> 18) & 7);
>  	}
>  
> -	csig->rev = c->microcode;
> +	/* get the current microcode revision from MSR 0x8B */
> +	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> +	sync_core();
> +	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
> +
> +	csig->rev = val[1];
> +	c->microcode = val[1];  /* re-sync */
> +}
> +
> +static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> +{
> +	__collect_cpu_info(cpu_num, csig);
> +
>  	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
>  		cpu_num, csig->sig, csig->pf, csig->rev);

We probably should downgrade this to pr_debug and use collect_cpu_info()
everywhere instead of having a __ version.

>  
> @@ -118,7 +130,10 @@ static int get_matching_mc(struct microcode_intel *mc_intel, int cpu)
>  	struct cpu_signature cpu_sig;
>  	unsigned int csig, cpf, crev;
>  
> -	collect_cpu_info(cpu, &cpu_sig);
> +	/* NOTE: cpu_data()->microcode will be outdated on HT
> +	 * processors during an update run, it must be refreshed
> +	 * from MSR 0x8B */
> +	__collect_cpu_info(cpu, &cpu_sig);
>  
>  	csig = cpu_sig.sig;
>  	cpf = cpu_sig.pf;
> @@ -145,23 +160,21 @@ static int apply_microcode_intel(int cpu)
>  		return 0;
>  
>  	/*
> -	 * Microcode on this CPU could be updated earlier. Only apply the
> -	 * microcode patch in mc_intel when it is newer than the one on this
> -	 * CPU.
> +	 * Microcode on this CPU might be already up-to-date.  Only apply
> +	 * the microcode patch in mc_intel when it is newer than the one
> +	 * on this CPU.
>  	 */
>  	if (get_matching_mc(mc_intel, cpu) == 0)
>  		return 0;
>  
> -	/* write microcode via MSR 0x79 */
> +	/* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */

No need for screaming here - we know MSR accesses are expensive. This
comment is totally useless here so drop it altogether.

>  	wrmsr(MSR_IA32_UCODE_WRITE,
> -	      (unsigned long) mc_intel->bits,
> -	      (unsigned long) mc_intel->bits >> 16 >> 16);
> -	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> -
> -	/* As documented in the SDM: Do a CPUID 1 here */
> -	sync_core();
> +		lower_32_bits((unsigned long) mc_intel->bits),
> +		upper_32_bits((unsigned long) mc_intel->bits));

wrmsrl() takes u64 directly - no need for the splitting.

>  	/* get the current revision from MSR 0x8B */
> +	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> +	sync_core();
>  	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
>  
>  	if (val[1] != mc_intel->hdr.rev) {
> -- 
> 1.7.10.4
> 
> 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Henrique de Moraes Holschuh]

On Mon, 20 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:48PM -0300, Henrique de Moraes Holschuh wrote:
> > -static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> > +static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> >  {
> >  	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
> >  	unsigned int val[2];
> > @@ -102,7 +102,19 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> >  		csig->pf = 1 << ((val[1] >> 18) & 7);
> >  	}
> >  
> > -	csig->rev = c->microcode;
> > +	/* get the current microcode revision from MSR 0x8B */
> > +	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> > +	sync_core();
> > +	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
> > +
> > +	csig->rev = val[1];
> > +	c->microcode = val[1];  /* re-sync */
> > +}
> > +
> > +static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
> > +{
> > +	__collect_cpu_info(cpu_num, csig);
> > +
> >  	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
> >  		cpu_num, csig->sig, csig->pf, csig->rev);
> 
> We probably should downgrade this to pr_debug and use collect_cpu_info()
> everywhere instead of having a __ version.

Over time, grepping for that information on reports and logs all over the
net has helped me a great deal.  In fact, it is in my backlog to add it to
the early microcode driver as well.

I really miss the full microcode ID information in /proc/cpuinfo, in fact.

If you want, I can modify the logging it in a future patch so that we print
it only once when all cores have the same sig, pf and revision (which should
cover 95% of the systems out there).

> > -	/* write microcode via MSR 0x79 */
> > +	/* write microcode via MSR 0x79. THIS IS VERY EXPENSIVE */
> 
> No need for screaming here - we know MSR accesses are expensive. This
> comment is totally useless here so drop it altogether.

MSR 79H writes are on a class of their own as far as "expensive" goes... On
a modern i3/i5/i7, it will take approximately one million cycles to complete
(the larger the microcode update, the longer it takes).

I don't think people usually associate MSR write with "takes one million
cycles to complete"...

That said, I will remove the comment.

> >  	wrmsr(MSR_IA32_UCODE_WRITE,
> > -	      (unsigned long) mc_intel->bits,
> > -	      (unsigned long) mc_intel->bits >> 16 >> 16);
> > -	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> > -
> > -	/* As documented in the SDM: Do a CPUID 1 here */
> > -	sync_core();
> > +		lower_32_bits((unsigned long) mc_intel->bits),
> > +		upper_32_bits((unsigned long) mc_intel->bits));
> 
> wrmsrl() takes u64 directly - no need for the splitting.

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?

-- 
  "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

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Borislav Petkov]

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:

$ grep microcode /proc/cpuinfo | head -1
microcode       : 0x6000822

if needed.

Now, the error cases where the upgrade fails for some unexpected reason
is what we want to know.

> 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?

If so,

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

   ->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?

   -> 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.

> MSR 79H writes are on a class of their own as far as "expensive" goes... On
> a modern i3/i5/i7, it will take approximately one million cycles to complete
> (the larger the microcode update, the longer it takes).
> 
> I don't think people usually associate MSR write with "takes one million
> cycles to complete"...

So? You don't do microcode updates all the time - it is done once during
boot and when cores come back online.

> 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, ..."

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Henrique de Moraes Holschuh]

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

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Borislav Petkov]

On Fri, Oct 31, 2014 at 04:43:45PM -0200, Henrique de Moraes Holschuh wrote:
> 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.

Ok, I see what you mean.

A couple of thoughs/suggestions for that:

/proc/cpuinfo is the place which gives you, well, CPU info. So all
the information pertinent to the CPU should be there. And since on bug
reports we do ask for /proc/cpuinfo along with dmesg, I think that
should be fine.

Now, we're not about reducing the log info in general but rather only
saying something when we really have to. Microcode updates should work
in the background and the user should not be bothered at all about it in
the normal, success case. Only when there are issues with it, only then
we want to say something.

> 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.

This is wrong: if! the early loader is enabled and initrd is supplied,
it should apply the microcode. That's why it is an early loader in the
first place.

The late loader is for the cases where you don't want to reboot the
system when applying microcode.

But all systems out there should enable the early loader - microcode
should be applied as early as possible.

> 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.

Why is that so? Can we improve it?

> 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.

Well, if you really need it, we could add it there:

cat /proc/cpuinfo:

...
vendor_id       : AuthenticAMD
cpu family      : 21
model           : 2
model name      : AMD FX(tm)-8350 Eight-Core Processor
stepping        : 0
microcode       : 0x6000832
BIOS microcode	: 0x6000822

but I don't see why you need it. Regardless of the effort, you want to have
microcode loader running on all systems, that's it.

> > > 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

processor type bits? What is that?

> 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.

Well, you have all that info:

grep . /sys/devices/system/cpu/cpu?/microcode/*
/sys/devices/system/cpu/cpu0/microcode/processor_flags:0x10
/sys/devices/system/cpu/cpu0/microcode/version:0x1b
/sys/devices/system/cpu/cpu1/microcode/processor_flags:0x10
/sys/devices/system/cpu/cpu1/microcode/version:0x1b
/sys/devices/system/cpu/cpu2/microcode/processor_flags:0x10
/sys/devices/system/cpu/cpu2/microcode/version:0x1b
/sys/devices/system/cpu/cpu3/microcode/processor_flags:0x10
/sys/devices/system/cpu/cpu3/microcode/version:0x1b

We can easily extend sysfs with more info required instead of spewing it
into dmesg.

You can access sysfs then in your scripts and query everything you need.
And use cpuid.ko and msr.ko too. I think with that you get all the info
needed.

> 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.

See above.

Bottom line is: dmesg should contain only error information when
microcode fails applying. All the remaining info should be in sysfs and
/proc/cpuinfo.

This is much cleaner IMO and will save us a lot of useless bug reports.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Henrique de Moraes Holschuh]

On Sat, 01 Nov 2014, Borislav Petkov wrote:
> Now, we're not about reducing the log info in general but rather only
> saying something when we really have to. Microcode updates should work
> in the background and the user should not be bothered at all about it in
> the normal, success case. Only when there are issues with it, only then
> we want to say something.

I understand your objective, yes.  I just don't agree with your reasoning
entirely.  I fully agree it should not bother the user by inducing doubts,
or being too verbose, though.

Suppose I do the work so that we get something like this:

<microcode driver>: sig XXX, pf XXX, rev YYY

or, if an update is done, we get this *instead*:

<microcode driver>: sig XXX, pf XXX, updated from rev YYY to rev ZZZ (YYYYMMDD)

And that will be all, unless you actually have multiple [different]
microcodes in the system (which is rare), in which case you will get more
lines.

You'd get the same one line if an update is triggered at runtime and it
manages to update at least one processor.

> The late loader is for the cases where you don't want to reboot the
> system when applying microcode.

I used it also as a fallback in Debian/Ubuntu should the early update
driver fail, but I have no idea whether it was effective or not because the
early microcode driver currently fails silently.

Anyway, the use of the late driver in initramfs (even as a fallback) is gone
from Debian since two weeks ago, and it will be gone from Ubuntu when they
sync to Debian once again.

> But all systems out there should enable the early loader - microcode
> should be applied as early as possible.

Agreed.

> > 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.
> 
> Why is that so? Can we improve it?

Yes, we can, but I'd still like to have my one log line per boot, please.

The log line has one feature /proc/cpuinfo won't ever have: it is always
there in the boot logs, even if you didn't ask for it in advance.

And the log currently has one feature that is valuable IMO: it is complete,
it has all the errors, warnings and useful information, and you just need
"grep microcode" to get it all.

> Well, if you really need it, we could add it there:
> 
> cat /proc/cpuinfo:
> 
> ...
> vendor_id       : AuthenticAMD
> cpu family      : 21
> model           : 2
> model name      : AMD FX(tm)-8350 Eight-Core Processor
> stepping        : 0
> microcode       : 0x6000832
> BIOS microcode	: 0x6000822

For /proc/cpuinfo to be complete for *tool use*, I'd really need
cpuid_eax(1), and on intel, the three bits from MSR 0x17 [52:50] in pf_mask
format (1 << (MSR 0x17[52:50])).

microcode sig: 0x0000000
microcode pfm: 0x00	[only on intel]

We could make the "BIOS microcode" line optional, and supress it when no
microcode update was applied (i.e. BIOS microcode == current microcode).

However, that's two new lines for AMD, and three for Intel.


Somewhat related, but independent of what we do to /proc/cpuinfo,
microcode.ko really should grow a signature field in sysfs, so that you
don't need both microcode.ko *and* cpuid.ko to know what microcode the box
needs.  I even have a patch that does this ready, if you want it.

And it is much better to deal with sysfs than parse /proc/cpuinfo...
cpuinfo is for humans, and it is quite important, but machine-parsing it
properly is annoying (requires a state machine, etc).

> but I don't see why you need it. Regardless of the effort, you want to have
> microcode loader running on all systems, that's it.

Eh, I don't _need_ it because right now the kernel log has everything I
want (as long as the late microcode module gets loaded) other than error
messages from the early loader.

> > > > 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
> 
> processor type bits? What is that?

It is a field composed by cpuid_eax(1) [13:12].  According to the Intel
SDM:

  00b: Original OEM processor
  01b: overdrive processor
  10b: dual processor
  11b: Intel reserved

This is the microcode for the Pentium Overdrive processor:
sig 0x00001632, pf mask 0x00, 1998-06-10, rev 0x0002, size 2048

where you can see that bits 13:12 of the signture are 01b (type: overdrive
processor).

Anyway, AFAIK the Intel SDM defines the microcode signature to be all 32
bits in cpuid_eax(1), including the reserved bits (which doesn't seem
right, you're usually told to mask these to zero, but whatever).

> You can access sysfs then in your scripts and query everything you need.
> And use cpuid.ko and msr.ko too. I think with that you get all the info
> needed.

msr.ko is a security nightmare.  The less it is used, the better.  I'd like
it to either go away entirely, or (more realistically) that it switched to
a processor model-aware whitelist of safe, in-use-by-userspace MSRs, and
reject everything else no matter what (even to processes with
CAP_SYS_RAWIO).

But yes, I can get that information as-is, and I don't even need msr.ko to
do it.  It may not be convenient at all, but the information is reachable
from userspace through cpuid.ko and the late microcode loader
(microcode.ko).

-- 
  "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

Re: [PATCH 2/8] x86, microcode, intel: don't update each HT core twice

[Borislav Petkov]

On Sat, Nov 01, 2014 at 05:20:26PM -0200, Henrique de Moraes Holschuh wrote:
> I understand your objective, yes.  I just don't agree with your reasoning
> entirely.  I fully agree it should not bother the user by inducing doubts,
> or being too verbose, though.
> 
> Suppose I do the work so that we get something like this:
> 
> <microcode driver>: sig XXX, pf XXX, rev YYY
> 
> or, if an update is done, we get this *instead*:
> 
> <microcode driver>: sig XXX, pf XXX, updated from rev YYY to rev ZZZ (YYYYMMDD)
> 
> And that will be all, unless you actually have multiple [different]
> microcodes in the system (which is rare), in which case you will get more
> lines.
> 
> You'd get the same one line if an update is triggered at runtime and it
> manages to update at least one processor.

And what is wrong with putting that info in /proc/cpuinfo and not saying
anything in dmesg?

> I used it also as a fallback in Debian/Ubuntu should the early update
> driver fail, but I have no idea whether it was effective or not because the
> early microcode driver currently fails silently.

The early driver should not fail - if it does we need to fix it. Feel
free to CC me on bug reports so that this gets sorted out and fixed.

And it fails silently because that early we cannot call anything
printk-like anyway as a debugging aid.

> Yes, we can, but I'd still like to have my one log line per boot, please.
> 
> The log line has one feature /proc/cpuinfo won't ever have: it is always
> there in the boot logs, even if you didn't ask for it in advance.

Yeah right. Even if dmesg ring buffer has wrapped and overwritten stuff.

The lines which are always there are the /proc/cpuinfo ones. And CPU
microcode belongs to the CPU so it is only natural to put it there.
Oh, and removing the dmesg likes will save you silly bug reports like
microcode being updated only on the even-numbered cores.

> For /proc/cpuinfo to be complete for *tool use*, I'd really need
> cpuid_eax(1),

No, you don't. You can *call* CPUID in your tool.

>  and on intel, the three bits from MSR 0x17 [52:50] in pf_mask
> format (1 << (MSR 0x17[52:50])).
> 
> microcode sig: 0x0000000
> microcode pfm: 0x00	[only on intel]
> 
> We could make the "BIOS microcode" line optional, and supress it when no
> microcode update was applied (i.e. BIOS microcode == current microcode).
> 
> However, that's two new lines for AMD, and three for Intel.

Not a problem.

> Somewhat related, but independent of what we do to /proc/cpuinfo,
> microcode.ko really should grow a signature field in sysfs, so that you
> don't need both microcode.ko *and* cpuid.ko to know what microcode the box
> needs.  I even have a patch that does this ready, if you want it.

You can call CPUID in your tool - it is not a privileged instruction
like accessing the MSRs, for example.

> And it is much better to deal with sysfs than parse /proc/cpuinfo...
> cpuinfo is for humans, and it is quite important, but machine-parsing it
> properly is annoying (requires a state machine, etc).

Really?! I'd use awk :-)

But whatever, we can put stuff in sysfs if you prefer.

> It is a field composed by cpuid_eax(1) [13:12].  According to the Intel
> SDM:
> 
>   00b: Original OEM processor
>   01b: overdrive processor
>   10b: dual processor
>   11b: Intel reserved
> 
> This is the microcode for the Pentium Overdrive processor:
> sig 0x00001632, pf mask 0x00, 1998-06-10, rev 0x0002, size 2048
> 
> where you can see that bits 13:12 of the signture are 01b (type: overdrive
> processor).

Ok.

> Anyway, AFAIK the Intel SDM defines the microcode signature to be all 32
> bits in cpuid_eax(1), including the reserved bits (which doesn't seem
> right, you're usually told to mask these to zero, but whatever).

They probably say somewhere too that those are Read-As-Zero too...

> msr.ko is a security nightmare.  The less it is used, the better.  I'd like
> it to either go away entirely, or (more realistically) that it switched to
> a processor model-aware whitelist of safe, in-use-by-userspace MSRs, and
> reject everything else no matter what (even to processes with
> CAP_SYS_RAWIO).

Right, I think people even blacklist it on production systems.

> But yes, I can get that information as-is, and I don't even need msr.ko to
> do it.  It may not be convenient at all, but the information is reachable
> from userspace through cpuid.ko and the late microcode loader
> (microcode.ko).

Ok, so far I gather all you need is CPUID which you can call - you
don't necessarily need cpuid.ko - and stuff exported through sysfs or
/proc/cpuinfo.

Sounds like a much better interface to me than grepping to fragile logs
which might get overwritten.

Btw, on bug reports, you can do a small script which goes and collects
all that info from the user's machine. I know alsa does something like
that.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 3/8] x86, microcode, intel: clarify log messages

[Henrique de Moraes Holschuh]

The Intel microcode update driver will skip the second hardware thread
on hyper-threaded cores during an update run, as the first hardware
thread will have updated the entire core.  This can confuse users,
because it will look like some CPUs were not updated in the system log.
Attempt to clarify the log messages to hint that we might be updating
more than one CPU (hardware thread) at a time.

Make sure all driver log messages conform to this template: "microcode:
CPU#: <message>".  The <message> might refer to the core, and not to the
hardware thread/CPU.

Reword error and debug messages for clarity or style.  Tag most error
messages as "error:", and warnings as "warning:".  Report conditions
which will stop a microcode update as errors, and conditions which will
not stop a microcode update as warnings.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel.c       |   10 +++++-----
 arch/x86/kernel/cpu/microcode/intel_early.c |   11 +++++++----
 arch/x86/kernel/cpu/microcode/intel_lib.c   |   12 ++++++------
 3 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 2c629d1..e99cdd8 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -115,7 +115,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 {
 	__collect_cpu_info(cpu_num, csig);
 
-	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
+	pr_info("CPU%d: sig=0x%x, pf=0x%x, revision=0x%x\n",
 		cpu_num, csig->sig, csig->pf, csig->rev);
 
 	return 0;
@@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
 	if (val[1] != mc_intel->hdr.rev) {
-		pr_err("CPU%d update to revision 0x%x failed\n",
+		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
 		       cpu_num, mc_intel->hdr.rev);
 		return -1;
 	}
-	pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
+	pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
 		cpu_num, val[1],
 		mc_intel->hdr.date & 0xffff,
 		mc_intel->hdr.date >> 24,
@@ -214,7 +214,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 
 		mc_size = get_totalsize(&mc_header);
 		if (!mc_size || mc_size > leftover) {
-			pr_err("error! Bad data in microcode data file\n");
+			pr_err("error: invalid microcode update data\n");
 			break;
 		}
 
@@ -268,7 +268,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
 	 */
 	save_mc_for_early(new_mc);
 
-	pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
+	pr_debug("CPU%d: found a matching microcode update with version 0x%x (current=0x%x)\n",
 		 cpu, new_rev, uci->cpu_sig.rev);
 out:
 	return state;
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f..f73fc0a 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -16,6 +16,9 @@
  *	as published by the Free Software Foundation; either version
  *	2 of the License, or (at your option) any later version.
  */
+
+#define pr_fmt(fmt) "microcode: " fmt
+
 #include <linux/module.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
@@ -418,7 +421,7 @@ static void __ref show_saved_mc(void)
 		pr_debug("no microcode data saved.\n");
 		return;
 	}
-	pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
+	pr_debug("total microcode entries saved: %d\n", mc_saved_data.mc_saved_count);
 
 	collect_cpu_info_early(&uci);
 
@@ -519,7 +522,7 @@ int save_mc_for_early(u8 *mc)
 	 */
 	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
 	if (ret) {
-		pr_err("Cannot save microcode patch.\n");
+		pr_warn("warning: could not store microcode update data for later use.\n");
 		goto out;
 	}
 
@@ -579,7 +582,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
 {
 	int cpu = smp_processor_id();
 
-	pr_info("CPU%d microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
+	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
 		cpu,
 		uci->cpu_sig.rev,
 		date & 0xffff,
@@ -701,7 +704,7 @@ int __init save_microcode_in_initrd_intel(void)
 	microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
 	ret = save_microcode(&mc_saved_data, mc_saved, count);
 	if (ret)
-		pr_err("Cannot save microcode patches from initrd.\n");
+		pr_warn("warning: failed to save early microcode update data for future use\n");
 
 	show_saved_mc();
 
diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 25915e3..1cc6494 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -64,7 +64,7 @@ int microcode_sanity_check(void *mc, int print_err)
 
 	if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
 		if (print_err)
-			pr_err("error! Unknown microcode update format\n");
+			pr_err("error: unknown microcode update format\n");
 		return -EINVAL;
 	}
 	ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
@@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
 		if ((ext_table_size < EXT_HEADER_SIZE)
 		 || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
 			if (print_err)
-				pr_err("error! Small exttable size in microcode data file\n");
+				pr_err("error: small exttable size in microcode data file\n");
 			return -EINVAL;
 		}
 		ext_header = mc + MC_HEADER_SIZE + data_size;
 		if (ext_table_size != exttable_size(ext_header)) {
 			if (print_err)
-				pr_err("error! Bad exttable size in microcode data file\n");
+				pr_err("error: bad exttable size in microcode data file\n");
 			return -EFAULT;
 		}
 		ext_sigcount = ext_header->count;
@@ -114,7 +114,7 @@ int microcode_sanity_check(void *mc, int print_err)
 			ext_table_sum += ext_tablep[i];
 		if (ext_table_sum) {
 			if (print_err)
-				pr_warn("aborting, bad extended signature table checksum\n");
+				pr_err("error: bad extended signature table checksum\n");
 			return -EINVAL;
 		}
 	}
@@ -126,7 +126,7 @@ int microcode_sanity_check(void *mc, int print_err)
 		orig_sum += ((int *)mc)[i];
 	if (orig_sum) {
 		if (print_err)
-			pr_err("aborting, bad checksum\n");
+			pr_err("error: bad microcode update checksum\n");
 		return -EINVAL;
 	}
 	if (!ext_table_size)
@@ -140,7 +140,7 @@ int microcode_sanity_check(void *mc, int print_err)
 			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
 		if (sum) {
 			if (print_err)
-				pr_err("aborting, bad checksum\n");
+				pr_err("error: bad extended signature checksum\n");
 			return -EINVAL;
 		}
 	}
-- 
1.7.10.4

Re: [PATCH 3/8] x86, microcode, intel: clarify log messages

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:49PM -0300, Henrique de Moraes Holschuh wrote:
> The Intel microcode update driver will skip the second hardware thread
> on hyper-threaded cores during an update run, as the first hardware
> thread will have updated the entire core.  This can confuse users,
> because it will look like some CPUs were not updated in the system log.
> Attempt to clarify the log messages to hint that we might be updating
> more than one CPU (hardware thread) at a time.
> 
> Make sure all driver log messages conform to this template: "microcode:
> CPU#: <message>".  The <message> might refer to the core, and not to the
> hardware thread/CPU.
> 
> Reword error and debug messages for clarity or style.  Tag most error
> messages as "error:", and warnings as "warning:".  Report conditions
> which will stop a microcode update as errors, and conditions which will
> not stop a microcode update as warnings.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c       |   10 +++++-----
>  arch/x86/kernel/cpu/microcode/intel_early.c |   11 +++++++----
>  arch/x86/kernel/cpu/microcode/intel_lib.c   |   12 ++++++------
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 2c629d1..e99cdd8 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -115,7 +115,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>  {
>  	__collect_cpu_info(cpu_num, csig);
>  
> -	pr_info("CPU%d sig=0x%x, pf=0x%x, revision=0x%x\n",
> +	pr_info("CPU%d: sig=0x%x, pf=0x%x, revision=0x%x\n",
>  		cpu_num, csig->sig, csig->pf, csig->rev);
>  
>  	return 0;
> @@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
>  	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
>  
>  	if (val[1] != mc_intel->hdr.rev) {
> -		pr_err("CPU%d update to revision 0x%x failed\n",
> +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
>  		       cpu_num, mc_intel->hdr.rev);
>  		return -1;
>  	}
> -	pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
> +	pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",

Those two above are not really needed IMO.

>  		cpu_num, val[1],
>  		mc_intel->hdr.date & 0xffff,
>  		mc_intel->hdr.date >> 24,
> @@ -214,7 +214,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
>  
>  		mc_size = get_totalsize(&mc_header);
>  		if (!mc_size || mc_size > leftover) {
> -			pr_err("error! Bad data in microcode data file\n");
> +			pr_err("error: invalid microcode update data\n");

What's wrong with the original message?

>  			break;
>  		}
>  
> @@ -268,7 +268,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
>  	 */
>  	save_mc_for_early(new_mc);
>  
> -	pr_debug("CPU%d found a matching microcode update with version 0x%x (current=0x%x)\n",
> +	pr_debug("CPU%d: found a matching microcode update with version 0x%x (current=0x%x)\n",
>  		 cpu, new_rev, uci->cpu_sig.rev);
>  out:
>  	return state;
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index b88343f..f73fc0a 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -16,6 +16,9 @@
>   *	as published by the Free Software Foundation; either version
>   *	2 of the License, or (at your option) any later version.
>   */
> +
> +#define pr_fmt(fmt) "microcode: " fmt
> +
>  #include <linux/module.h>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> @@ -418,7 +421,7 @@ static void __ref show_saved_mc(void)
>  		pr_debug("no microcode data saved.\n");
>  		return;
>  	}
> -	pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
> +	pr_debug("total microcode entries saved: %d\n", mc_saved_data.mc_saved_count);

That should be "Total microcode patches saved" - "entries" doesn't say a whole
lot.

>  
>  	collect_cpu_info_early(&uci);
>  
> @@ -519,7 +522,7 @@ int save_mc_for_early(u8 *mc)
>  	 */
>  	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
>  	if (ret) {
> -		pr_err("Cannot save microcode patch.\n");
> +		pr_warn("warning: could not store microcode update data for later use.\n");

Capitalize: "Warning: could... "

otherwise that message clarification makes sense.

>  		goto out;
>  	}
>  
> @@ -579,7 +582,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
>  {
>  	int cpu = smp_processor_id();
>  
> -	pr_info("CPU%d microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
> +	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",

No, please no "entire core" mentions - that'll only confuse people.
Simply think of logical cores as separate cores which share the
microcode hw. No need for more confusion.

>  		cpu,
>  		uci->cpu_sig.rev,
>  		date & 0xffff,
> @@ -701,7 +704,7 @@ int __init save_microcode_in_initrd_intel(void)
>  	microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
>  	ret = save_microcode(&mc_saved_data, mc_saved, count);
>  	if (ret)
> -		pr_err("Cannot save microcode patches from initrd.\n");
> +		pr_warn("warning: failed to save early microcode update data for future use\n");

This one actually loses info - the "initrd" part.

>  
>  	show_saved_mc();
>  
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index 25915e3..1cc6494 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -64,7 +64,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  
>  	if (mc_header->ldrver != 1 || mc_header->hdrver != 1) {
>  		if (print_err)
> -			pr_err("error! Unknown microcode update format\n");
> +			pr_err("error: unknown microcode update format\n");

Actually it should be like a real sentence:

	"Error: unknown ... format.\n"

>  		return -EINVAL;
>  	}
>  	ext_table_size = total_size - (MC_HEADER_SIZE + data_size);
> @@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
>  		if ((ext_table_size < EXT_HEADER_SIZE)
>  		 || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
>  			if (print_err)
> -				pr_err("error! Small exttable size in microcode data file\n");
> +				pr_err("error: small exttable size in microcode data file\n");

That doesn't tell me a whole lot - maybe "... truncated exttable in microcode data file" ?

>  			return -EINVAL;
>  		}
>  		ext_header = mc + MC_HEADER_SIZE + data_size;
>  		if (ext_table_size != exttable_size(ext_header)) {
>  			if (print_err)
> -				pr_err("error! Bad exttable size in microcode data file\n");
> +				pr_err("error: bad exttable size in microcode data file\n");

Ditto.

>  			return -EFAULT;
>  		}
>  		ext_sigcount = ext_header->count;
> @@ -114,7 +114,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  			ext_table_sum += ext_tablep[i];
>  		if (ext_table_sum) {
>  			if (print_err)
> -				pr_warn("aborting, bad extended signature table checksum\n");
> +				pr_err("error: bad extended signature table checksum\n");

Capitalize.

>  			return -EINVAL;
>  		}
>  	}
> @@ -126,7 +126,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  		orig_sum += ((int *)mc)[i];
>  	if (orig_sum) {
>  		if (print_err)
> -			pr_err("aborting, bad checksum\n");
> +			pr_err("error: bad microcode update checksum\n");

Ditto.

>  		return -EINVAL;
>  	}
>  	if (!ext_table_size)
> @@ -140,7 +140,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
>  		if (sum) {
>  			if (print_err)
> -				pr_err("aborting, bad checksum\n");
> +				pr_err("error: bad extended signature checksum\n");

"Aborting ..." was better.

>  			return -EINVAL;
>  		}
>  	}
> -- 
> 1.7.10.4
> 
> 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/8] x86, microcode, intel: clarify log messages

[Henrique de Moraes Holschuh]

On Mon, 20 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:49PM -0300, Henrique de Moraes Holschuh wrote:
> > The Intel microcode update driver will skip the second hardware thread
> > on hyper-threaded cores during an update run, as the first hardware
> > thread will have updated the entire core.  This can confuse users,
> > because it will look like some CPUs were not updated in the system log.
> > Attempt to clarify the log messages to hint that we might be updating
> > more than one CPU (hardware thread) at a time.
> > 
> > Make sure all driver log messages conform to this template: "microcode:
> > CPU#: <message>".  The <message> might refer to the core, and not to the
> > hardware thread/CPU.
> > 
> > Reword error and debug messages for clarity or style.  Tag most error
> > messages as "error:", and warnings as "warning:".  Report conditions
> > which will stop a microcode update as errors, and conditions which will
> > not stop a microcode update as warnings.
> > 
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

...

> > @@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
> >  	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
> >  
> >  	if (val[1] != mc_intel->hdr.rev) {
> > -		pr_err("CPU%d update to revision 0x%x failed\n",
> > +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
> >  		       cpu_num, mc_intel->hdr.rev);
> >  		return -1;
> >  	}
> > -	pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
> > +	pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
> 
> Those two above are not really needed IMO.

They are an attempt to make it more clear.  I've got bug reports about this
sort of issue before in Debian, people do wonder why some "cpus" are
updated, and others _apparently_ are not (because there are no messages for
them).

We have more reasons why an update fail.  Rejected by the processor is a
very specific one we want to single out (it basically means we are either
feeding crap to the processor _or_ violating one of the architectural
requirements).

> > @@ -214,7 +214,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
> >  
> >  		mc_size = get_totalsize(&mc_header);
> >  		if (!mc_size || mc_size > leftover) {
> > -			pr_err("error! Bad data in microcode data file\n");
> > +			pr_err("error: invalid microcode update data\n");
> 
> What's wrong with the original message?

This is a cosmetic fix, so that the messages follow the same pattern.

> > @@ -418,7 +421,7 @@ static void __ref show_saved_mc(void)
> >  		pr_debug("no microcode data saved.\n");
> >  		return;
> >  	}
> > -	pr_debug("Total microcode saved: %d\n", mc_saved_data.mc_saved_count);
> > +	pr_debug("total microcode entries saved: %d\n", mc_saved_data.mc_saved_count);
> 
> That should be "Total microcode patches saved" - "entries" doesn't say a whole
> lot.

We don't call them patches anywhere else in Intel's case.  But since you
prefer "patch", "patch" it is.

> >  	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
> >  	if (ret) {
> > -		pr_err("Cannot save microcode patch.\n");
> > +		pr_warn("warning: could not store microcode update data for later use.\n");
> 
> Capitalize: "Warning: could... "
> 
> otherwise that message clarification makes sense.

It is lowercase because we now prefix all pr_<level>() with "microcode: ",
so it will look like this:

"microcode: warning: foo"
"microcode: error: foo".

Should I capitalize this and all other messages of this sort?  It doesn't
look right to me to log "microcode: Error: foo" or "microcode: Warning:
foo".

> > @@ -579,7 +582,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> >  {
> >  	int cpu = smp_processor_id();
> >  
> > -	pr_info("CPU%d microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
> > +	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> 
> No, please no "entire core" mentions - that'll only confuse people.
> Simply think of logical cores as separate cores which share the
> microcode hw. No need for more confusion.

The status-quo confuses people, they wonder why some "cpus" were skipped,
and they file bugs about it or ask about it in the user forums.  I got a bug
report for a module-based AMD processor, and it took a few round-trips to
explain what was happening to the user...

If "entire core" is unclear, what wording would you suggest?

> > @@ -701,7 +704,7 @@ int __init save_microcode_in_initrd_intel(void)
> >  	microcode_pointer(mc_saved, mc_saved_in_initrd, initrd_start, count);
> >  	ret = save_microcode(&mc_saved_data, mc_saved, count);
> >  	if (ret)
> > -		pr_err("Cannot save microcode patches from initrd.\n");
> > +		pr_warn("warning: failed to save early microcode update data for future use\n");
> 
> This one actually loses info - the "initrd" part.

However, it isn't from the initrd: it comes from the early initramfs, which
is a different kind of beast.  We (distros) did have microcode data inside
the initrd/initramfs for the regular microcode driver, so the previous error
message could be misleading.

Would you perfer "warning: failed to save early initramfs microcode update
data for future use\n" ?

> > @@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
> >  		if ((ext_table_size < EXT_HEADER_SIZE)
> >  		 || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
> >  			if (print_err)
> > -				pr_err("error! Small exttable size in microcode data file\n");
> > +				pr_err("error: small exttable size in microcode data file\n");
> 
> That doesn't tell me a whole lot - maybe "... truncated exttable in microcode data file" ?

If it hit this codepath, it will not be due to "normal" truncation.  The
caller detects truncation earlier, checking the main header total_size
against the size of the data it got from userspace or the early initramfs.

The message really means: "exttable size incompatible with the size of a
valid table".

Would you prefer this alternate wording?

> >  		ext_header = mc + MC_HEADER_SIZE + data_size;
> >  		if (ext_table_size != exttable_size(ext_header)) {
> >  			if (print_err)
> > -				pr_err("error! Bad exttable size in microcode data file\n");
> > +				pr_err("error: bad exttable size in microcode data file\n");
> 
> Ditto.

I want to differentiate these two, they're caused by different issues, so
they should not be the same error message.

Would you prefer "exttable entry counter incompatible with exttable size",
which is what this message really means?

> > @@ -140,7 +140,7 @@ int microcode_sanity_check(void *mc, int print_err)
> >  			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
> >  		if (sum) {
> >  			if (print_err)
> > -				pr_err("aborting, bad checksum\n");
> > +				pr_err("error: bad extended signature checksum\n");
> 
> "Aborting ..." was better.

Well, every other error message in microcode_sanity_check() also means we
abort just the same.  It doesn't make sense to have "abort" just in this
one.

-- 
  "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

Re: [PATCH 3/8] x86, microcode, intel: clarify log messages

[Borislav Petkov]

On Tue, Oct 21, 2014 at 12:13:32PM -0200, Henrique de Moraes Holschuh wrote:
> > > @@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
> > >  	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
> > >  
> > >  	if (val[1] != mc_intel->hdr.rev) {
> > > -		pr_err("CPU%d update to revision 0x%x failed\n",
> > > +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
> > >  		       cpu_num, mc_intel->hdr.rev);
> > >  		return -1;
> > >  	}
> > > -	pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
> > > +	pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
> > 
> > Those two above are not really needed IMO.
> 
> They are an attempt to make it more clear.  I've got bug reports about this
> sort of issue before in Debian, people do wonder why some "cpus" are
> updated, and others _apparently_ are not (because there are no messages for
> them).

The fact that people open bugs doesn't always mean it is really a bug :-)

And there's a very simple answer to that:

$ grep microcode /proc/cpuinfo | uniq

should give only a single line.

And I'm pretty sure people will ask about "entire core" too. You simply
can't explain to them that the microcode engine is shared between
threads in a oneliner. :-)

Hrrm, I still don't like that "entire core" thing, how about something
like this:

	pr_info("CPU%d: update to revision 0x%x, date %04x-%02x-%02x\n"

this doesn't say at least that we're updating a CPU to a revision sth
sth but that we're doing the update on CPU%d. This should clearly
state what happens without making any assumptions about what the core
contains.

> We have more reasons why an update fail.  Rejected by the processor is a
> very specific one we want to single out (it basically means we are either
> feeding crap to the processor _or_ violating one of the architectural
> requirements).

That all doesn't matter - you only want to be able to pinpoint which
error message you're looking at and as long as they differ, you're fine.
Otherwise this is just an unnecessary churn.

> It is lowercase because we now prefix all pr_<level>() with "microcode: ",
> so it will look like this:
> 
> "microcode: warning: foo"
> "microcode: error: foo".
> 
> Should I capitalize this and all other messages of this sort?  It doesn't
> look right to me to log "microcode: Error: foo" or "microcode: Warning:
> foo".

Why not? You have the piece of machinery which generates the message,
i.e. "microcode" and then a sentence which starts with a capital letter.
I think this is the accepted practice in the kernel.

> The status-quo confuses people, they wonder why some "cpus" were skipped,
> and they file bugs about it or ask about it in the user forums.  I got a bug
> report for a module-based AMD processor, and it took a few round-trips to
> explain what was happening to the user...
> 
> If "entire core" is unclear, what wording would you suggest?

See above.

I just don't like the quantification "entire core" because later, when
CPU guys decide to split it more, we'll need to change it again.

> However, it isn't from the initrd: it comes from the early initramfs, which
> is a different kind of beast.  We (distros) did have microcode data inside
> the initrd/initramfs for the regular microcode driver, so the previous error
> message could be misleading.

Wait a minute, we do supply the early microcode by adding it to the
initrd: Documentation/x86/early-microcode.txt

Also, what you do on Debian doesn't necessarily mean all distros do it,
does it?

> Would you perfer "warning: failed to save early initramfs microcode update
> data for future use\n" ?

I guess we can say something method-agnostic without explicitly naming
that method...


> > > @@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
> > >  		if ((ext_table_size < EXT_HEADER_SIZE)
> > >  		 || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
> > >  			if (print_err)
> > > -				pr_err("error! Small exttable size in microcode data file\n");
> > > +				pr_err("error: small exttable size in microcode data file\n");
> > 
> > That doesn't tell me a whole lot - maybe "... truncated exttable in microcode data file" ?
> 
> If it hit this codepath, it will not be due to "normal" truncation.  The

What is a "normal" truncation?

> caller detects truncation earlier, checking the main header total_size
> against the size of the data it got from userspace or the early initramfs.

You mean this:

	if (data_size + MC_HEADER_SIZE > total_size)

?

If so, this is not truncation check - it sanity-checks what's in the
headers or the default sizes.


Btw, there's a typo in show_saved_mc()

pr_debug("mc_saved[%d]: sig=0x%x, pf=0x%x, rev=0x%x, toal size=0x%x, date = %04x-%02x-%02x\n",
						     ^^^^
> The message really means: "exttable size incompatible with the size of a
> valid table".

Let's call this:

	"Error: extended signature table size mismatch!\n"

because this is what this check does.

> I want to differentiate these two, they're caused by different issues, so
> they should not be the same error message.
> 
> Would you prefer "exttable entry counter incompatible with exttable size",
> which is what this message really means?

Yep, that sounds better.

> Well, every other error message in microcode_sanity_check() also means we
> abort just the same.  It doesn't make sense to have "abort" just in this
> one.

Ok.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/8] x86, microcode, intel: clarify log messages

[Henrique de Moraes Holschuh]

On Wed, 29 Oct 2014, Borislav Petkov wrote:
> On Tue, Oct 21, 2014 at 12:13:32PM -0200, Henrique de Moraes Holschuh wrote:
> > > > @@ -178,11 +178,11 @@ static int apply_microcode_intel(int cpu)
> > > >  	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
> > > >  
> > > >  	if (val[1] != mc_intel->hdr.rev) {
> > > > -		pr_err("CPU%d update to revision 0x%x failed\n",
> > > > +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n",
> > > >  		       cpu_num, mc_intel->hdr.rev);
> > > >  		return -1;
> > > >  	}
> > > > -	pr_info("CPU%d updated to revision 0x%x, date = %04x-%02x-%02x\n",
> > > > +	pr_info("CPU%d: entire core updated to revision 0x%x, date %04x-%02x-%02x\n",
> > > 
> > > Those two above are not really needed IMO.
> > 
> > They are an attempt to make it more clear.  I've got bug reports about this
> > sort of issue before in Debian, people do wonder why some "cpus" are
> > updated, and others _apparently_ are not (because there are no messages for
> > them).
> 
> The fact that people open bugs doesn't always mean it is really a bug :-)

Indeed.

> And I'm pretty sure people will ask about "entire core" too. You simply
> can't explain to them that the microcode engine is shared between
> threads in a oneliner. :-)

Unfortunately, I believe you're correct.

> Hrrm, I still don't like that "entire core" thing, how about something
> like this:
> 
> 	pr_info("CPU%d: update to revision 0x%x, date %04x-%02x-%02x\n"
> 
> this doesn't say at least that we're updating a CPU to a revision sth
> sth but that we're doing the update on CPU%d. This should clearly
> state what happens without making any assumptions about what the core
> contains.

Will do.  I don't think this addresses what I was trying to do, however it
_is_ better, and if you accept my proposal for the change in logging, the
confusing issue will be taken care off anyway.

> > We have more reasons why an update fail.  Rejected by the processor is a
> > very specific one we want to single out (it basically means we are either
> > feeding crap to the processor _or_ violating one of the architectural
> > requirements).
> 
> That all doesn't matter - you only want to be able to pinpoint which
> error message you're looking at and as long as they differ, you're fine.
> Otherwise this is just an unnecessary churn.

In this case, it is useful churn IMHO: it is a building block that I used
immediately and that I will also need later for further improvements, it is
used to log that the early microcode driver failed due to an error
(something we currently don't know at all), and it can be used to log why
(since we can use it to have as many different error messages when
attempting to update the BSP microcode as we need).

> > It is lowercase because we now prefix all pr_<level>() with "microcode: ",
> > so it will look like this:
> > 
> > "microcode: warning: foo"
> > "microcode: error: foo".
> > 
> > Should I capitalize this and all other messages of this sort?  It doesn't
> > look right to me to log "microcode: Error: foo" or "microcode: Warning:
> > foo".
> 
> Why not? You have the piece of machinery which generates the message,
> i.e. "microcode" and then a sentence which starts with a capital letter.
> I think this is the accepted practice in the kernel.

This is a matter of personal taste, so I will just do it your way, and
change the template to:

<driver>: Error|Warning: foo

> I just don't like the quantification "entire core" because later, when
> CPU guys decide to split it more, we'll need to change it again.

Yes.

> > However, it isn't from the initrd: it comes from the early initramfs, which
> > is a different kind of beast.  We (distros) did have microcode data inside
> > the initrd/initramfs for the regular microcode driver, so the previous error
> > message could be misleading.
> 
> Wait a minute, we do supply the early microcode by adding it to the
> initrd: Documentation/x86/early-microcode.txt

As far as the kernel is concerned, there are two ways to do that: early
initramfs image (used by the early microcode drivers), or adding the regular
microcode.ko module and /lib/firmware/intel-ucode/* inside the regular
compressed initramfs, where it will be modprobed.

Both share "initramfs" (or initrd) in the name, but they are obviously two
completely different things.

As far as userspace is concerned there is at least one extra variant (which
doesn't matter as far as the kernel is concerned): Arch linux uses two
separate initramfs images, and tells the bootloader to load both.  The first
one has the early initramfs with the microcode data, the second one has the
regular compressed initramfs with the boot machinery.  I don't know if
anyone else is doing it that way, but it is quite cool in that it reduces
the chances of some very horrible failure modes, so I will try to switch
Debian and Ubuntu to it next year.

> > Would you perfer "warning: failed to save early initramfs microcode update
> > data for future use\n" ?
> 
> I guess we can say something method-agnostic without explicitly naming
> that method...

Yes, however isn't that code used only to save the microcode data used from
the early initramfs (because otherwise it would be freed along with the rest
of __initdata), and for nothing else?

> > > > @@ -72,13 +72,13 @@ int microcode_sanity_check(void *mc, int print_err)
> > > >  		if ((ext_table_size < EXT_HEADER_SIZE)
> > > >  		 || ((ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE)) {
> > > >  			if (print_err)
> > > > -				pr_err("error! Small exttable size in microcode data file\n");
> > > > +				pr_err("error: small exttable size in microcode data file\n");
> > > 
> > > That doesn't tell me a whole lot - maybe "... truncated exttable in microcode data file" ?
> > 
> > If it hit this codepath, it will not be due to "normal" truncation.  The
> 
> What is a "normal" truncation?

When you're missing the rest of the data for whatever reason, e.g:
incomplete firmware file.

> > caller detects truncation earlier, checking the main header total_size
> > against the size of the data it got from userspace or the early initramfs.
> 
> You mean this:
> 
> 	if (data_size + MC_HEADER_SIZE > total_size)
> 
> ?
> 
> If so, this is not truncation check - it sanity-checks what's in the
> headers or the default sizes.

No, I mean this:

(early intel microcode driver caller code):
               mc_size = get_totalsize(mc_header);
               if (!mc_size || mc_size > leftover ||
                        microcode_sanity_check(ucode_ptr, 0) < 0)
		...

(regular intel microcode driver caller code):
                mc_size = get_totalsize(&mc_header);
                if (!mc_size || mc_size > leftover) {
                        pr_err("error! Bad data in microcode data file\n");
                        break;
                }

		...

                if (get_ucode_data(mc, ucode_ptr, mc_size) ||
                    microcode_sanity_check(mc, 1) < 0) {
                        break;
                }

Anything that was just partially downloaded (and not further corrupted) will
fail these tests before microcode_sanity_check() is even called,  Thus,
"caller detects truncation earlier".

> Btw, there's a typo in show_saved_mc()
> 
> pr_debug("mc_saved[%d]: sig=0x%x, pf=0x%x, rev=0x%x, toal size=0x%x, date = %04x-%02x-%02x\n",
> 						     ^^^^

Noted.  Might as well fix it, too :-)

> > The message really means: "exttable size incompatible with the size of a
> > valid table".
> 
> Let's call this:
> 
> 	"Error: extended signature table size mismatch!\n"
> 
> because this is what this check does.

Will do.

> > I want to differentiate these two, they're caused by different issues, so
> > they should not be the same error message.
> > 
> > Would you prefer "exttable entry counter incompatible with exttable size",
> > which is what this message really means?
> 
> Yep, that sounds better.

Ok, 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

Re: [PATCH 3/8] x86, microcode, intel: clarify log messages

[Borislav Petkov]

On Fri, Oct 31, 2014 at 06:08:02PM -0200, Henrique de Moraes Holschuh wrote:
> In this case, it is useful churn IMHO: it is a building block that I used
> immediately and that I will also need later for further improvements, it is
> used to log that the early microcode driver failed due to an error
> (something we currently don't know at all),

So we're again trying to improve at the wrong side. What we actually
need to do is fix the early loader in those cases where it is still
b0rked. If any. I have currently two more fixes for 32-bit early
loading, one generic, one for AMD, but on 64-bit it is solid. At least I
don't know about any issues.

And then once all is in place, I'd like to make the loader as silent as
possible because it is supposed to just work.

If one still wants to debug stuff then we can have our small shell
script to gather the needed info on a system and people can run it on
theirs when reporting issues.

> This is a matter of personal taste, so I will just do it your way, and
> change the template to:
> 
> <driver>: Error|Warning: foo

Just look at dmesg output on any system.

> ... I don't know if anyone else is doing it that way, but it is quite
> cool in that it reduces the chances of some very horrible failure
> modes, so I will try to switch Debian and Ubuntu to it next year.

Whatever you do, just keep the users in mind. Adding the microcode to
the initrd should be trivial. Maybe it should be added by a script, or
some postinst whatever funkyness, whatever... just make sure *anyone*
can create it. Not a tutorial somewhere on the net but a single command
like

update-initramfs --add-microcode ...

or whatever, I don't care, as long as it is simple and
it always works. I don't want to be pointing people to
Documentation/x86/early-microcode.txt and ask "but but, have you done
this already".

> Yes, however isn't that code used only to save the microcode data used from
> the early initramfs (because otherwise it would be freed along with the rest
> of __initdata), and for nothing else?

What do you mean with "only"?

This code saves the microcode patch which we've used in the early
loader, into a memory buffer so that we can use it later, for example,
after resuming the box or for when cores are coming back online as a
result of a CPU hotplug operation.

This is not really important, though, I'm thinking, because we have
redundant functionality from the "late" microcode loader which updates
the microcode in its CPU hotplug callback.

For that to work, though, we need the correct microcode installed in
/lib/firmware.

So packagers should be doing two important things:

* add microcode to the initrd for early loading
* add microcode to /lib/firmware/ for late loading when needed

I think with this we're covered solid. Unless I'm missing a hole.

> When you're missing the rest of the data for whatever reason, e.g:
> incomplete firmware file.

But this is the same, right?

	(ext_table_size - EXT_HEADER_SIZE) % EXT_SIGNATURE_SIZE

checks whether some of the extended signatures are incomplete. So we can
say:

	pr_err("error: Small exttable size/truncated extended signature in microcode data file.\n");

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Henrique de Moraes Holschuh]

Enhance the logging in the Intel early microcode update driver to
be able to report errors.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel_early.c |   94 +++++++++++++++------------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index f73fc0a..8ad50d6 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -31,6 +31,12 @@
 #include <asm/tlbflush.h>
 #include <asm/setup.h>
 
+enum {
+	INTEL_EARLYMCU_NONE = 0, /* did nothing */
+	INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
+	INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
+};
+
 static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
 static struct mc_saved_data {
 	unsigned int mc_saved_count;
@@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
 
 /*
  * Print ucode update info.
+ * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
+ * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
  */
-static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+static void print_ucode_info(const unsigned int status,
+			      const struct ucode_cpu_info *uci,
+			      const unsigned int data)
 {
 	int cpu = smp_processor_id();
-
-	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
-		cpu,
-		uci->cpu_sig.rev,
-		date & 0xffff,
-		date >> 24,
-		(date >> 16) & 0xff);
+	struct ucode_cpu_info ucil;
+
+	switch (status) {
+	case INTEL_EARLYMCU_NONE:
+		break;
+	case INTEL_EARLYMCU_UPDATEOK:
+		if (!uci) {
+			collect_cpu_info_early(&ucil);
+			uci = &ucil;
+		}
+		pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
+			cpu,
+			uci->cpu_sig.rev,
+			data & 0xffff,
+			data >> 24,
+			(data >> 16) & 0xff);
+		break;
+	case INTEL_EARLYMCU_REJECTED:
+		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
+		break;
+	}
 }
 
 #ifdef CONFIG_X86_32
 
-static int delay_ucode_info;
-static int current_mc_date;
+static unsigned int delay_ucode_info;
+static unsigned int delay_ucode_info_data;
 
 /*
  * Print early updated ucode info after printk works. This is delayed info dump.
+ * This is only used for the BSP.
  */
 void show_ucode_info_early(void)
 {
-	struct ucode_cpu_info uci;
-
-	if (delay_ucode_info) {
-		collect_cpu_info_early(&uci);
-		print_ucode_info(&uci, current_mc_date);
-		delay_ucode_info = 0;
-	}
+	print_ucode_info(delay_ucode_info, NULL, delay_ucode_info_data);
+	delay_ucode_info = INTEL_EARLYMCU_NONE;
 }
 
 /*
@@ -614,21 +633,18 @@ void show_ucode_info_early(void)
  * mc_saved_data.mc_saved and 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(const unsigned int status,
+			const struct ucode_cpu_info * const uci,
+			const unsigned int data)
 {
-	struct microcode_intel *mc_intel;
-	int *delay_ucode_info_p;
-	int *current_mc_date_p;
-
-	mc_intel = uci->mc;
-	if (mc_intel == NULL)
-		return;
+	unsigned int *delay_ucode_info_p;
+	unsigned int *delay_ucode_info_data_p;
 
-	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
-	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
+	delay_ucode_info_p = (unsigned int *)__pa_nodebug(&delay_ucode_info);
+	delay_ucode_info_data_p = (unsigned int *)__pa_nodebug(&delay_ucode_info_data);
 
-	*delay_ucode_info_p = 1;
-	*current_mc_date_p = mc_intel->hdr.date;
+	*delay_ucode_info_p = status;
+	*delay_ucode_info_data_p = data;
 }
 #else
 
@@ -641,15 +657,11 @@ static inline void flush_tlb_early(void)
 	__native_flush_tlb_global_irq_disabled();
 }
 
-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(const unsigned int status,
+			       const struct ucode_cpu_info * const uci,
+			       const unsigned int data)
 {
-	struct microcode_intel *mc_intel;
-
-	mc_intel = uci->mc;
-	if (mc_intel == NULL)
-		return;
-
-	print_ucode_info(uci, mc_intel->hdr.date);
+	print_ucode_info(status, uci, data);
 }
 #endif
 
@@ -674,8 +686,10 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-	if (val[1] != mc_intel->hdr.rev)
+	if (val[1] != mc_intel->hdr.rev) {
+		print_ucode(INTEL_EARLYMCU_REJECTED, uci, mc_intel->hdr.rev);
 		return -1;
+	}
 
 #ifdef CONFIG_X86_64
 	/* Flush global tlb. This is precaution. */
@@ -683,7 +697,7 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 #endif
 	uci->cpu_sig.rev = val[1];
 
-	print_ucode(uci);
+	print_ucode(INTEL_EARLYMCU_UPDATEOK, uci, mc_intel->hdr.date);
 
 	return 0;
 }
-- 
1.7.10.4

Re: [PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:50PM -0300, Henrique de Moraes Holschuh wrote:
> Enhance the logging in the Intel early microcode update driver to
> be able to report errors.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel_early.c |   94 +++++++++++++++------------
>  1 file changed, 54 insertions(+), 40 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index f73fc0a..8ad50d6 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -31,6 +31,12 @@
>  #include <asm/tlbflush.h>
>  #include <asm/setup.h>
>  
> +enum {
> +	INTEL_EARLYMCU_NONE = 0, /* did nothing */
> +	INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
> +	INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
> +};
> +
>  static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
>  static struct mc_saved_data {
>  	unsigned int mc_saved_count;
> @@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
>  
>  /*
>   * Print ucode update info.
> + * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
> + * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
>   */
> -static void
> -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> +static void print_ucode_info(const unsigned int status,
> +			      const struct ucode_cpu_info *uci,
> +			      const unsigned int data)
>  {
>  	int cpu = smp_processor_id();
> -
> -	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> -		cpu,
> -		uci->cpu_sig.rev,
> -		date & 0xffff,
> -		date >> 24,
> -		(date >> 16) & 0xff);
> +	struct ucode_cpu_info ucil;
> +
> +	switch (status) {
> +	case INTEL_EARLYMCU_NONE:
> +		break;
> +	case INTEL_EARLYMCU_UPDATEOK:
> +		if (!uci) {
> +			collect_cpu_info_early(&ucil);
> +			uci = &ucil;
> +		}
> +		pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> +			cpu,
> +			uci->cpu_sig.rev,
> +			data & 0xffff,
> +			data >> 24,
> +			(data >> 16) & 0xff);
> +		break;
> +	case INTEL_EARLYMCU_REJECTED:
> +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
> +		break;
> +	}
>  }
>  
>  #ifdef CONFIG_X86_32
>  
> -static int delay_ucode_info;
> -static int current_mc_date;
> +static unsigned int delay_ucode_info;
> +static unsigned int delay_ucode_info_data;

First of all, this really is date and not data and prefixing it with
"delay" really doesn't make it cleaner.

Then, this whole scheme can be simplified a bit by dropping
delay_ucode_info and using current_mc_date to test whether to print the
message or not. After printing, you set it back to 0.

And then you can drop the _REJECTED case as it is not needed.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Henrique de Moraes Holschuh]

On Mon, 20 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:50PM -0300, Henrique de Moraes Holschuh wrote:
> > Enhance the logging in the Intel early microcode update driver to
> > be able to report errors.
> > 
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > ---
> >  arch/x86/kernel/cpu/microcode/intel_early.c |   94 +++++++++++++++------------
> >  1 file changed, 54 insertions(+), 40 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> > index f73fc0a..8ad50d6 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> > @@ -31,6 +31,12 @@
> >  #include <asm/tlbflush.h>
> >  #include <asm/setup.h>
> >  
> > +enum {
> > +	INTEL_EARLYMCU_NONE = 0, /* did nothing */
> > +	INTEL_EARLYMCU_UPDATEOK, /* microcode updated */
> > +	INTEL_EARLYMCU_REJECTED, /* cpu rejected it */
> > +};
> > +
> >  static unsigned long mc_saved_in_initrd[MAX_UCODE_COUNT];
> >  static struct mc_saved_data {
> >  	unsigned int mc_saved_count;
> > @@ -576,37 +582,50 @@ scan_microcode(unsigned long start, unsigned long end,
> >  
> >  /*
> >   * Print ucode update info.
> > + * for status == INTEL_EARLYMCU_UPDATEOK, data should be the mcu date
> > + * for status == INTEL_EARLYMCU_REJECTED, data should be mcu revision
> >   */
> > -static void
> > -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> > +static void print_ucode_info(const unsigned int status,
> > +			      const struct ucode_cpu_info *uci,
> > +			      const unsigned int data)
> >  {
> >  	int cpu = smp_processor_id();
> > -
> > -	pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> > -		cpu,
> > -		uci->cpu_sig.rev,
> > -		date & 0xffff,
> > -		date >> 24,
> > -		(date >> 16) & 0xff);
> > +	struct ucode_cpu_info ucil;
> > +
> > +	switch (status) {
> > +	case INTEL_EARLYMCU_NONE:
> > +		break;
> > +	case INTEL_EARLYMCU_UPDATEOK:
> > +		if (!uci) {
> > +			collect_cpu_info_early(&ucil);
> > +			uci = &ucil;
> > +		}
> > +		pr_info("CPU%d: entire core updated early to revision 0x%x, date %04x-%02x-%02x\n",
> > +			cpu,
> > +			uci->cpu_sig.rev,
> > +			data & 0xffff,
> > +			data >> 24,
> > +			(data >> 16) & 0xff);
> > +		break;
> > +	case INTEL_EARLYMCU_REJECTED:
> > +		pr_err("CPU%d: update to revision 0x%x rejected by the processor\n", cpu, data);
> > +		break;
> > +	}
> >  }
> >  
> >  #ifdef CONFIG_X86_32
> >  
> > -static int delay_ucode_info;
> > -static int current_mc_date;
> > +static unsigned int delay_ucode_info;
> > +static unsigned int delay_ucode_info_data;
> 
> First of all, this really is date and not data and prefixing it with
> "delay" really doesn't make it cleaner.

For INTEL_EARLYMCU_UPDATEOK, it is the date.

For INTEL_EARLYMCU_REJECTED, it is the revision of the microcode that was
    rejected (as opposed to the revision of the microcode currently in the
    processor), because that's far more important to know than the date.

However, if you prefer, I could have _date, _oldrev, and _newrev, and
enhance the error messages a bit (updated from rev X to rev Y, date Z), etc.

> Then, this whole scheme can be simplified a bit by dropping
> delay_ucode_info and using current_mc_date to test whether to print the
> message or not. After printing, you set it back to 0.

In the INTEL_EARLYMCU_REJECTED case, the data we need to print might be
zero, and it has 32 bits.

Besides, this way one can trivially add new messages when required.  And we
will need to do that eventually.

In fact, I have a patch somewhere that needs to add a new failure message:
we have several failure cases which we want to differentiate, at the very
least "processor didn't like it" and "it looks corrupt, so we didn't even
try to install it".

If you want, I will add it when I resubmit this stack, instead of waiting
for the next one.

-- 
  "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

Re: [PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Borislav Petkov]

On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> In fact, I have a patch somewhere that needs to add a new failure message:
> we have several failure cases which we want to differentiate, at the very
> least "processor didn't like it" and "it looks corrupt, so we didn't even
> try to install it".

Actually, I don't want to be too chatty with the loader: if the
microcode blob passes checks but it is not for the current processor
we're running, not saying anything is what I want to do.

Why? Because I don't want to disturb people unnecessarily - if the
microcode doesn't apply and everything else checks out, you simply don't
need it.

If one really wants to know, one can always check /proc/cpuinfo and read
out the microcode revision from the blob. But that is for the 1% of the
curious ones - everyone else should simply install microcode blob and
boot machine. Fire and forget.

Only the abnormal error cases should be vocal in saying what's wrong so
that we can actually address those.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Joe Perches]

On Thu, 2014-10-30 at 18:41 +0100, Borislav Petkov wrote:
> On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> > In fact, I have a patch somewhere that needs to add a new failure message:
> > we have several failure cases which we want to differentiate, at the very
> > least "processor didn't like it" and "it looks corrupt, so we didn't even
> > try to install it".
> 
> Actually, I don't want to be too chatty with the loader: if the
> microcode blob passes checks but it is not for the current processor
> we're running, not saying anything is what I want to do.
> 
> Why? Because I don't want to disturb people unnecessarily - if the
> microcode doesn't apply and everything else checks out, you simply don't
> need it.
> 
> If one really wants to know, one can always check /proc/cpuinfo and read
> out the microcode revision from the blob. But that is for the 1% of the
> curious ones - everyone else should simply install microcode blob and
> boot machine. Fire and forget.
> 
> Only the abnormal error cases should be vocal in saying what's wrong so
> that we can actually address those.

Maybe make it emit at KERN_DEBUG

Re: [PATCH 4/8] x86, microcode, intel: add error logging to early update driver

[Henrique de Moraes Holschuh]

On Thu, 30 Oct 2014, Borislav Petkov wrote:
> On Tue, Oct 21, 2014 at 12:10:15PM -0200, Henrique de Moraes Holschuh wrote:
> > In fact, I have a patch somewhere that needs to add a new failure message:
> > we have several failure cases which we want to differentiate, at the very
> > least "processor didn't like it" and "it looks corrupt, so we didn't even
> > try to install it".
> 
> Actually, I don't want to be too chatty with the loader: if the
> microcode blob passes checks but it is not for the current processor
> we're running, not saying anything is what I want to do.
> 
> Why? Because I don't want to disturb people unnecessarily - if the
> microcode doesn't apply and everything else checks out, you simply don't
> need it.
> 
> If one really wants to know, one can always check /proc/cpuinfo and read
> out the microcode revision from the blob. But that is for the 1% of the
> curious ones - everyone else should simply install microcode blob and
> boot machine. Fire and forget.
> 
> Only the abnormal error cases should be vocal in saying what's wrong so
> that we can actually address those.

I've answered these points in one of the replies I just sent.

-- 
  "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

[PATCH 5/8] x86, microcode, intel: don't check extsig entry checksum

[Henrique de Moraes Holschuh]

The contents of the extended signature entries are already covered by
the extended table checksum, and the microcode driver should not be
attempting to check their internal checksum field.

Unlike the main microcode checksum field and the extended signature
table checksum field, the checksum fields inside the extended signature
entries are not meant to be processed by a microcode update loader.  The
extended signature entry checksum field's description in the Intel SDM,
vol 3A, table 9-6, page 9-30, reads in the first paragraph:

   "Used by utility software to decompose a microcode update into
    multiple microcode updates where each of the new updates is
    constructed without the optional Extended Processor Signature
    Table."

And the Linux microcode driver is not processing them correctly anyway.
The second paragraph of the signature entry checksum field's description
in the Intel SDM, vol 3A, table 9-6, page 9-30, reads:

   "To calculate the Checksum, substitute the Primary Processor
    Signature entry and the Processor Flags entry with the corresponding
    Extended Patch entry. Delete the Extended Processor Signature Table
    entries. The Checksum is correct when the summation of all DWORDs
    that comprise the created Extended Processor Patch results in
    00000000H."

Deleting the extended signature table changes the Total Size field, and
the Intel SDM paragraph above makes it very clear that such a change must
be accounted for by the checksum.  The current extended signature entry
checksum code in the Linux microcode driver, which has been in place
since 2003, will be thrown off by this and reject a valid microcode
update.

The microcode driver is better off by doing what the Intel SDM suggests,
and staying well clear of that checksum field.  It has already checked
the whole extended signature table's checksum, anyway.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel_lib.c |   20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
index 1cc6494..9200b83 100644
--- a/arch/x86/kernel/cpu/microcode/intel_lib.c
+++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
@@ -49,8 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
 	unsigned long total_size, data_size, ext_table_size;
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header = NULL;
-	int sum, orig_sum, ext_sigcount = 0, i;
-	struct extended_signature *ext_sig;
+	int orig_sum, i;
 
 	total_size = get_totalsize(mc_header);
 	data_size = get_datasize(mc_header);
@@ -81,7 +80,6 @@ int microcode_sanity_check(void *mc, int print_err)
 				pr_err("error: bad exttable size in microcode data file\n");
 			return -EFAULT;
 		}
-		ext_sigcount = ext_header->count;
 	}
 
 	/*
@@ -129,21 +127,7 @@ int microcode_sanity_check(void *mc, int print_err)
 			pr_err("error: bad microcode update checksum\n");
 		return -EINVAL;
 	}
-	if (!ext_table_size)
-		return 0;
-	/* check extended signature checksum */
-	for (i = 0; i < ext_sigcount; i++) {
-		ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
-			  EXT_SIGNATURE_SIZE * i;
-		sum = orig_sum
-			- (mc_header->sig + mc_header->pf + mc_header->cksum)
-			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
-		if (sum) {
-			if (print_err)
-				pr_err("error: bad extended signature checksum\n");
-			return -EINVAL;
-		}
-	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(microcode_sanity_check);
-- 
1.7.10.4

Re: [PATCH 5/8] x86, microcode, intel: don't check extsig entry checksum

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:51PM -0300, Henrique de Moraes Holschuh wrote:
> The contents of the extended signature entries are already covered by
> the extended table checksum, and the microcode driver should not be
> attempting to check their internal checksum field.
> 
> Unlike the main microcode checksum field and the extended signature
> table checksum field, the checksum fields inside the extended signature
> entries are not meant to be processed by a microcode update loader.  The
> extended signature entry checksum field's description in the Intel SDM,
> vol 3A, table 9-6, page 9-30, reads in the first paragraph:
> 
>    "Used by utility software to decompose a microcode update into
>     multiple microcode updates where each of the new updates is
>     constructed without the optional Extended Processor Signature
>     Table."
> 
> And the Linux microcode driver is not processing them correctly anyway.
> The second paragraph of the signature entry checksum field's description
> in the Intel SDM, vol 3A, table 9-6, page 9-30, reads:
> 
>    "To calculate the Checksum, substitute the Primary Processor
>     Signature entry and the Processor Flags entry with the corresponding
>     Extended Patch entry. Delete the Extended Processor Signature Table
>     entries. The Checksum is correct when the summation of all DWORDs
>     that comprise the created Extended Processor Patch results in
>     00000000H."
> 
> Deleting the extended signature table changes the Total Size field, and
> the Intel SDM paragraph above makes it very clear that such a change must
> be accounted for by the checksum.  The current extended signature entry
> checksum code in the Linux microcode driver, which has been in place
> since 2003, will be thrown off by this and reject a valid microcode
> update.

I don't know where you come up with this but the code you're removing
was added in 2013:

	e666dfa273db ("x86/microcode_intel_lib.c: Early update ucode on Intel's CPU")

and I'd strongly assume it was tested at the time. Now, the text in the
SDM is confusing, as most of the time is so I would think the code is
doing the right thing even if the text doesn't really say that clearly.

Fenghua, care to comment please?

Leaving in the rest for reference.

> The microcode driver is better off by doing what the Intel SDM suggests,
> and staying well clear of that checksum field.  It has already checked
> the whole extended signature table's checksum, anyway.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel_lib.c |   20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> index 1cc6494..9200b83 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> @@ -49,8 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  	unsigned long total_size, data_size, ext_table_size;
>  	struct microcode_header_intel *mc_header = mc;
>  	struct extended_sigtable *ext_header = NULL;
> -	int sum, orig_sum, ext_sigcount = 0, i;
> -	struct extended_signature *ext_sig;
> +	int orig_sum, i;
>  
>  	total_size = get_totalsize(mc_header);
>  	data_size = get_datasize(mc_header);
> @@ -81,7 +80,6 @@ int microcode_sanity_check(void *mc, int print_err)
>  				pr_err("error: bad exttable size in microcode data file\n");
>  			return -EFAULT;
>  		}
> -		ext_sigcount = ext_header->count;
>  	}
>  
>  	/*
> @@ -129,21 +127,7 @@ int microcode_sanity_check(void *mc, int print_err)
>  			pr_err("error: bad microcode update checksum\n");
>  		return -EINVAL;
>  	}
> -	if (!ext_table_size)
> -		return 0;
> -	/* check extended signature checksum */
> -	for (i = 0; i < ext_sigcount; i++) {
> -		ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
> -			  EXT_SIGNATURE_SIZE * i;
> -		sum = orig_sum
> -			- (mc_header->sig + mc_header->pf + mc_header->cksum)
> -			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
> -		if (sum) {
> -			if (print_err)
> -				pr_err("error: bad extended signature checksum\n");
> -			return -EINVAL;
> -		}
> -	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(microcode_sanity_check);
> -- 
> 1.7.10.4
> 
> 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 5/8] x86, microcode, intel: don't check extsig entry checksum

[Henrique de Moraes Holschuh]

On Thu, 30 Oct 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:51PM -0300, Henrique de Moraes Holschuh wrote:
> > The contents of the extended signature entries are already covered by
> > the extended table checksum, and the microcode driver should not be
> > attempting to check their internal checksum field.
> > 
> > Unlike the main microcode checksum field and the extended signature
> > table checksum field, the checksum fields inside the extended signature
> > entries are not meant to be processed by a microcode update loader.  The
> > extended signature entry checksum field's description in the Intel SDM,
> > vol 3A, table 9-6, page 9-30, reads in the first paragraph:
> > 
> >    "Used by utility software to decompose a microcode update into
> >     multiple microcode updates where each of the new updates is
> >     constructed without the optional Extended Processor Signature
> >     Table."
> > 
> > And the Linux microcode driver is not processing them correctly anyway.
> > The second paragraph of the signature entry checksum field's description
> > in the Intel SDM, vol 3A, table 9-6, page 9-30, reads:
> > 
> >    "To calculate the Checksum, substitute the Primary Processor
> >     Signature entry and the Processor Flags entry with the corresponding
> >     Extended Patch entry. Delete the Extended Processor Signature Table
> >     entries. The Checksum is correct when the summation of all DWORDs
> >     that comprise the created Extended Processor Patch results in
> >     00000000H."
> > 
> > Deleting the extended signature table changes the Total Size field, and
> > the Intel SDM paragraph above makes it very clear that such a change must
> > be accounted for by the checksum.  The current extended signature entry
> > checksum code in the Linux microcode driver, which has been in place
> > since 2003, will be thrown off by this and reject a valid microcode
> > update.
> 
> I don't know where you come up with this but the code you're removing
> was added in 2013:
> 
> 	e666dfa273db ("x86/microcode_intel_lib.c: Early update ucode on Intel's CPU")

Was it?  As far as I know, it is "heavily based" (i.e. copied and adapted)
on code from the regular microcode driver, and not rewritten from scrach.

If I am correct, this really is code from 2003, changed by 11 years worth of
file renaming, file splits, and refactoring.

It took me about 30 minutes to track the origin of this code in the regular
Intel microcode driver.  As far as I can tell, it was added to version 1.12
of the original driver, somewhere between v2.6.0-test7 and v2.6.0-test8.

The Intel engineers credited for the original change are: Nitin Kamble
<nitin.a.kamble@intel.com> and Jun Nakajima <jun.nakajima@intel.com>.
(information from the changelog at the top of the driver source).

You can see one of the earliest versions of the code here:
https://git.kernel.org/cgit/linux/kernel/git/history/history.git/tree/arch/i386/kernel/microcode.c?id=v2.6.0-test8

The extended table checksum gets calculated around line 323.  The possibly
problematic logic is in line 341 and 345, and matches what the modern driver
does.

> and I'd strongly assume it was tested at the time. Now, the text in the
> SDM is confusing, as most of the time is so I would think the code is
> doing the right thing even if the text doesn't really say that clearly.

I am not so sure.  Besides, it is entirely possible that the Intel SDM got
updated in that area after the september 2003 code drop.  This is very hard
to verify outside of Intel.   Or, maybe, the issue was detected internally
at Intel, and it was decided that the issue should be shelved until the day
extended microcode tables are actually required (which didn't happen yet).

> Fenghua, care to comment please?

I'd really appreciate that.  I have asked about it in the past, but got no
answers.

I am in fact hoping for a "this feature is dead as currently documented,
please remove the code until further notice" reply :-)  LoC reduction by
deleting something that was never used in 11 years, Yay!!

> Leaving in the rest for reference.
> 
> > The microcode driver is better off by doing what the Intel SDM suggests,
> > and staying well clear of that checksum field.  It has already checked
> > the whole extended signature table's checksum, anyway.
> > 
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > ---
> >  arch/x86/kernel/cpu/microcode/intel_lib.c |   20 ++------------------
> >  1 file changed, 2 insertions(+), 18 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_lib.c b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > index 1cc6494..9200b83 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_lib.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_lib.c
> > @@ -49,8 +49,7 @@ int microcode_sanity_check(void *mc, int print_err)
> >  	unsigned long total_size, data_size, ext_table_size;
> >  	struct microcode_header_intel *mc_header = mc;
> >  	struct extended_sigtable *ext_header = NULL;
> > -	int sum, orig_sum, ext_sigcount = 0, i;
> > -	struct extended_signature *ext_sig;
> > +	int orig_sum, i;
> >  
> >  	total_size = get_totalsize(mc_header);
> >  	data_size = get_datasize(mc_header);
> > @@ -81,7 +80,6 @@ int microcode_sanity_check(void *mc, int print_err)
> >  				pr_err("error: bad exttable size in microcode data file\n");
> >  			return -EFAULT;
> >  		}
> > -		ext_sigcount = ext_header->count;
> >  	}
> >  
> >  	/*
> > @@ -129,21 +127,7 @@ int microcode_sanity_check(void *mc, int print_err)
> >  			pr_err("error: bad microcode update checksum\n");
> >  		return -EINVAL;
> >  	}
> > -	if (!ext_table_size)
> > -		return 0;
> > -	/* check extended signature checksum */
> > -	for (i = 0; i < ext_sigcount; i++) {
> > -		ext_sig = (void *)ext_header + EXT_HEADER_SIZE +
> > -			  EXT_SIGNATURE_SIZE * i;
> > -		sum = orig_sum
> > -			- (mc_header->sig + mc_header->pf + mc_header->cksum)
> > -			+ (ext_sig->sig + ext_sig->pf + ext_sig->cksum);
> > -		if (sum) {
> > -			if (print_err)
> > -				pr_err("error: bad extended signature checksum\n");
> > -			return -EINVAL;
> > -		}
> > -	}
> > +
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(microcode_sanity_check);

-- 
  "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

Re: [PATCH 5/8] x86, microcode, intel: don't check extsig entry checksum

[Borislav Petkov]

On Fri, Oct 31, 2014 at 03:14:40PM -0200, Henrique de Moraes Holschuh wrote:
> > I don't know where you come up with this but the code you're removing
> > was added in 2013:
> > 
> > 	e666dfa273db ("x86/microcode_intel_lib.c: Early update ucode on Intel's CPU")
> 
> Was it?  As far as I know, it is "heavily based" (i.e. copied and adapted)
> on code from the regular microcode driver, and not rewritten from scrach.
> 
> If I am correct, this really is code from 2003, changed by 11 years worth of
> file renaming, file splits, and refactoring.
> 
> It took me about 30 minutes to track the origin of this code in the regular
> Intel microcode driver.  As far as I can tell, it was added to version 1.12
> of the original driver, somewhere between v2.6.0-test7 and v2.6.0-test8.
> 
> The Intel engineers credited for the original change are: Nitin Kamble
> <nitin.a.kamble@intel.com> and Jun Nakajima <jun.nakajima@intel.com>.
> (information from the changelog at the top of the driver source).
> 
> You can see one of the earliest versions of the code here:
> https://git.kernel.org/cgit/linux/kernel/git/history/history.git/tree/arch/i386/kernel/microcode.c?id=v2.6.0-test8
> 
> The extended table checksum gets calculated around line 323.  The possibly
> problematic logic is in line 341 and 345, and matches what the modern driver
> does.

Ok so e666dfa273db only adds code so I assumed it was new.

> I am not so sure.  Besides, it is entirely possible that the Intel SDM got
> updated in that area after the september 2003 code drop.  This is very hard
> to verify outside of Intel.   Or, maybe, the issue was detected internally
> at Intel, and it was decided that the issue should be shelved until the day
> extended microcode tables are actually required (which didn't happen yet).

Yeah, I wouldn't want to remove code from the loader without Intel
people saying something first.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 6/8] x86, microcode, intel: use cpuid explicitly instead of sync_core

[Henrique de Moraes Holschuh]

The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).

The microcode driver was abusing sync_core() to do this, probably
because it predates by nearly a decade the current "asm volatile
(:::"memory")" implementation of native_cpuid(), which is required for
the Intel MSR 8BH access protocol.

sync_core() semanthics are that of being a speculative execution
barrier, and not "run cpuid with EAX=1".

Change the Intel microcode driver to use native_cpuid instead, which is
more appropriate.  native_cpuid() is already in use by the early
microcode driver in one place.

Some small code reordering was done to avoid calling cpuid twice in
collect_cpu_info_early().

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/include/asm/microcode_intel.h      |   12 ++++++++++++
 arch/x86/kernel/cpu/microcode/intel.c       |    6 ++----
 arch/x86/kernel/cpu/microcode/intel_early.c |   25 +++++++------------------
 3 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index bbe296e..d40a45e 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -84,4 +84,16 @@ static inline int save_mc_for_early(u8 *mc)
 }
 #endif
 
+static inline unsigned int intel_ucode_cpuid_1(void)
+{
+	unsigned int eax, ebx, ecx, edx;
+
+	eax = 1;
+	ecx = 0;
+	/* extremely important: must be asm volatile(:::"memory") */
+	native_cpuid(&eax, &ebx, &ecx, &edx);
+
+	return eax;
+}
+
 #endif /* _ASM_X86_MICROCODE_INTEL_H */
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index e99cdd8..2182cec 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -94,8 +94,6 @@ static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 
 	memset(csig, 0, sizeof(*csig));
 
-	csig->sig = cpuid_eax(0x00000001);
-
 	if ((c->x86_model >= 5) || (c->x86 > 6)) {
 		/* get processor flags from MSR 0x17 */
 		rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
@@ -104,7 +102,7 @@ static void __collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 
 	/* get the current microcode revision from MSR 0x8B */
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-	sync_core();
+	csig->sig = intel_ucode_cpuid_1(); /* a cpuid(1) must happen here */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
 	csig->rev = val[1];
@@ -174,7 +172,7 @@ static int apply_microcode_intel(int cpu)
 
 	/* get the current revision from MSR 0x8B */
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-	sync_core();
+	intel_ucode_cpuid_1();
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 
 	if (val[1] != mc_intel->hdr.rev) {
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 8ad50d6..92629a8 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -379,7 +379,6 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 	unsigned int val[2];
 	u8 x86, x86_model;
 	struct cpu_signature csig;
-	unsigned int eax, ebx, ecx, edx;
 
 	csig.sig = 0;
 	csig.pf = 0;
@@ -387,10 +386,11 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 
 	memset(uci, 0, sizeof(*uci));
 
-	eax = 0x00000001;
-	ecx = 0;
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-	csig.sig = eax;
+	/* get the current revision from MSR 0x8B */
+	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	csig.sig = intel_ucode_cpuid_1(); /* a cpuid(1) must happen here */
+	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
+	csig.rev = val[1];
 
 	x86 = get_x86_family(csig.sig);
 	x86_model = get_x86_model(csig.sig);
@@ -400,15 +400,6 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 		native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
 		csig.pf = 1 << ((val[1] >> 18) & 7);
 	}
-	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
-	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
-
-	/* get the current revision from MSR 0x8B */
-	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
-
-	csig.rev = val[1];
 
 	uci->cpu_sig = csig;
 	uci->valid = 1;
@@ -679,12 +670,10 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 	native_wrmsr(MSR_IA32_UCODE_WRITE,
 	      (unsigned long) mc_intel->bits,
 	      (unsigned long) mc_intel->bits >> 16 >> 16);
-	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-
-	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
 
 	/* get the current revision from MSR 0x8B */
+	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
+	intel_ucode_cpuid_1();
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
 	if (val[1] != mc_intel->hdr.rev) {
 		print_ucode(INTEL_EARLYMCU_REJECTED, uci, mc_intel->hdr.rev);
-- 
1.7.10.4

Re: [PATCH 6/8] x86, microcode, intel: use cpuid explicitly instead of sync_core

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:52PM -0300, Henrique de Moraes Holschuh wrote:
> The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
> section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
> used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).
> 
> The microcode driver was abusing sync_core() to do this, probably
> because it predates by nearly a decade the current "asm volatile
> (:::"memory")" implementation of native_cpuid(), which is required for
> the Intel MSR 8BH access protocol.

Huh, what? Have you taken a look at sync_core() first?

> sync_core() semanthics are that of being a speculative execution
> barrier, and not "run cpuid with EAX=1".

Again, what?

Hmm, let's see:

static inline void sync_core(void)
{
 ...

        asm volatile("cpuid"
                     : "=a" (tmp)
                     : "0" (1)
                     : "ebx", "ecx", "edx", "memory");

What is the problem again?

I'm sorry but I don't understand what you're trying to fix here...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 6/8] x86, microcode, intel: use cpuid explicitly instead of sync_core

[Henrique de Moraes Holschuh]

On Fri, 07 Nov 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:52PM -0300, Henrique de Moraes Holschuh wrote:
> > The protocol to safely read MSR 8BH, described in the Intel SDM vol 3A,
> > section 9.11.7.1, explicitly determines that cpuid with EAX=1 must be
> > used between the wrmsr(0x8B, 0); and the rdmsr(0x8B).
> > 
> > The microcode driver was abusing sync_core() to do this, probably
> > because it predates by nearly a decade the current "asm volatile
> > (:::"memory")" implementation of native_cpuid(), which is required for
> > the Intel MSR 8BH access protocol.
> 
> Huh, what? Have you taken a look at sync_core() first?

Yes, I did.

> > sync_core() semanthics are that of being a speculative execution
> > barrier, and not "run cpuid with EAX=1".
> 
> Again, what?

sync_core() is a speculative execution barrier.  That's what it is
documented to do.  That's the reason _every_ caller other than the microcode
drivers call it.

In i486, sync_core() does a jmp.

In i586 and above, and x86-64, sync_core() does a cpuid(1).

sync_core() doesn't expect that its callers really want a cpuid(1).  If we
ever get a reason to use some other way to insert a speculative execution
barrier, sync_core() is likely to switch to it.

> What is the problem again?

No real problem, other than the fact that the microcode drivers call
sync_core() for what might as well be considered an internal implementation
detail of sync_core().

-- 
  "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

Re: [PATCH 6/8] x86, microcode, intel: use cpuid explicitly instead of sync_core

[Borislav Petkov]

On Fri, Nov 07, 2014 at 04:40:00PM -0200, Henrique de Moraes Holschuh wrote:
> No real problem, other than the fact that the microcode drivers call
> sync_core() for what might as well be considered an internal implementation
> detail of sync_core().

I think the comment there is enough.

But if you really want to be pedantic and make code more clear for
people who stare at it, this is what you should do:

---
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c6826d1e8082..59b98a4417ed 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -159,7 +159,7 @@ static int apply_microcode_intel(int cpu)
 	wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index b88343f7a3b3..6cb747113714 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -667,7 +667,7 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
-	sync_core();
+	cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
 	native_rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);
--

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

The full requirements for the memory area which holds the microcode
update binary data can be found in the Intel SDM, vol 3A, section
9.11.6, page 9-34.  They basically boil down to: 16-byte alignment, and
the data area must be entirely mapped if paging is already enabled.

The regular microcode update driver doesn't have to do anything special
to meet these requirements.  For peace of mind, add a check to
WARN_ONCE() when the alignment is (unexpectedly) incorrect, and abort
the microcode update.

However, the early microcode update driver can only expect 4-byte
alignment out of the early initramfs file format (which is actually good
enough for many Intel processors, but unless Intel oficially documents
this, we cannot make use of that fact).  The microcode update data will
not be aligned to a 16-byte boundary unless userspace takes special
steps to ensure it.

Change the early microcode driver to make a temporary copy of a portion
of the microcode header, and move the microcode data backwards
(overwriting the header) to a suitably aligned position, right before
issuing the microcode update WRMSR.

Unfortunately, to keep things simple, this has to be undone right after
the processor finishes the WRMSR.  Therefore, the alignment process will
have to be redone *for every processor core*.  This might end up not
being really noticeable, as the microcode update operation itself is
already extremely expensive in processor cycles.

If the microcode update data is already aligned in the first place, the
alignment process is skipped.  Users of large systems are encouraged to
use updated userspace that ensures 16-byte alignment of the microcode
data file contents inside the early initramfs image.

Add the relevant details to Documentation/x86/early-microcode.txt.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 Documentation/x86/early-microcode.txt       |   10 +++++++++
 arch/x86/kernel/cpu/microcode/intel.c       |    5 +++++
 arch/x86/kernel/cpu/microcode/intel_early.c |   30 +++++++++++++++++++++++++--
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/x86/early-microcode.txt b/Documentation/x86/early-microcode.txt
index d62bea6..c4f2ebd 100644
--- a/Documentation/x86/early-microcode.txt
+++ b/Documentation/x86/early-microcode.txt
@@ -14,6 +14,16 @@ during boot time. The microcode file in cpio name space is:
 on Intel: kernel/x86/microcode/GenuineIntel.bin
 on AMD  : kernel/x86/microcode/AuthenticAMD.bin
 
+For Intel processors, the microcode load process will be faster when special
+care is taken to ensure that the kernel/x86/microcode/GenuineIntel.bin file
+*data* inside the cpio archive is aligned to a paragraph (16-byte boundary).
+Standard pax/cpio can be coaxed into doing this by adding a padding file, e.g.
+"kernel/x86/microcode/.padding" with the appropriate size *right before* the
+kernel/x86/microcode/GenuineIntel.bin file.  Beware the required size for the
+padding file as it depends on the behavior of the tool used to create the cpio
+archive.  It is also possible to use a specific tool that appends enough NULs
+_to_ the file name (not _after_ the file name!) to align the file data.
+
 During BSP boot (before SMP starts), if the kernel finds the microcode file in
 the initrd file, it parses the microcode and saves matching microcode in memory.
 If matching microcode is found, it will be uploaded in BSP and later on in all
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 2182cec..40caef1 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -157,6 +157,11 @@ static int apply_microcode_intel(int cpu)
 	if (mc_intel == NULL)
 		return 0;
 
+	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
+	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
+		"microcode data incorrectly aligned"))
+		return -1;
+
 	/*
 	 * Microcode on this CPU might be already up-to-date.  Only apply
 	 * the microcode patch in mc_intel when it is newer than the one
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 92629a8..994c59b 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -662,14 +662,40 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 	struct microcode_intel *mc_intel;
 	unsigned int val[2];
 
+	char savedbuf[16];
+	void *mcu_data;
+	void *aligned_mcu_data;
+	unsigned int mcu_size = 0;
+
 	mc_intel = uci->mc;
 	if (mc_intel == NULL)
 		return 0;
 
+	mcu_data = mc_intel->bits;
+	aligned_mcu_data = mc_intel->bits;
+
+	/* Intel SDM vol 3A section 9.11.6, page 9-34: */
+	/* WRMSR MSR_IA32_UCODE_WRITE requires 16-byte alignment */
+	if ((unsigned long)(mcu_data) % 16) {
+		/* We have more than 16 bytes worth of microcode header
+		 * just before mc_intel->bits on a version 1 header */
+		BUILD_BUG_ON(offsetof(struct microcode_intel, bits) < 16);
+
+		aligned_mcu_data = (void *)((unsigned long)(mcu_data) & ~15UL);
+		mcu_size = get_datasize(&mc_intel->hdr);
+		memcpy(savedbuf, aligned_mcu_data, sizeof(savedbuf));
+		memmove(aligned_mcu_data, mcu_data, mcu_size);
+	}
+
 	/* write microcode via MSR 0x79 */
 	native_wrmsr(MSR_IA32_UCODE_WRITE,
-	      (unsigned long) mc_intel->bits,
-	      (unsigned long) mc_intel->bits >> 16 >> 16);
+		lower_32_bits((unsigned long)aligned_mcu_data),
+		upper_32_bits((unsigned long)aligned_mcu_data));
+
+	if (mcu_size) {
+		memmove(mcu_data, aligned_mcu_data, mcu_size);
+		memcpy(aligned_mcu_data, savedbuf, sizeof(savedbuf));
+	}
 
 	/* get the current revision from MSR 0x8B */
 	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
-- 
1.7.10.4

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Mon, 08 Sep 2014, Henrique de Moraes Holschuh wrote:
> Change the early microcode driver to make a temporary copy of a portion
> of the microcode header, and move the microcode data backwards
> (overwriting the header) to a suitably aligned position, right before
> issuing the microcode update WRMSR.

This one is missing a big thank you to Bill Davidsen for the "overwrite the
header" idea.  Once I get some comments about this series, I will respin
this one and credit him.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:53PM -0300, Henrique de Moraes Holschuh wrote:
> The full requirements for the memory area which holds the microcode
> update binary data can be found in the Intel SDM, vol 3A, section
> 9.11.6, page 9-34.  They basically boil down to: 16-byte alignment, and
> the data area must be entirely mapped if paging is already enabled.
> 
> The regular microcode update driver doesn't have to do anything special
> to meet these requirements.  For peace of mind, add a check to
> WARN_ONCE() when the alignment is (unexpectedly) incorrect, and abort
> the microcode update.
> 
> However, the early microcode update driver can only expect 4-byte
> alignment out of the early initramfs file format (which is actually good
> enough for many Intel processors, but unless Intel oficially documents
> this, we cannot make use of that fact).  The microcode update data will
> not be aligned to a 16-byte boundary unless userspace takes special
> steps to ensure it.
> 
> Change the early microcode driver to make a temporary copy of a portion
> of the microcode header, and move the microcode data backwards
> (overwriting the header) to a suitably aligned position, right before
> issuing the microcode update WRMSR.
> 
> Unfortunately, to keep things simple, this has to be undone right after
> the processor finishes the WRMSR.  Therefore, the alignment process will
> have to be redone *for every processor core*.  This might end up not
> being really noticeable, as the microcode update operation itself is
> already extremely expensive in processor cycles.
> 
> If the microcode update data is already aligned in the first place, the
> alignment process is skipped.  Users of large systems are encouraged to
> use updated userspace that ensures 16-byte alignment of the microcode
> data file contents inside the early initramfs image.
> 
> Add the relevant details to Documentation/x86/early-microcode.txt.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  Documentation/x86/early-microcode.txt       |   10 +++++++++
>  arch/x86/kernel/cpu/microcode/intel.c       |    5 +++++
>  arch/x86/kernel/cpu/microcode/intel_early.c |   30 +++++++++++++++++++++++++--
>  3 files changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/x86/early-microcode.txt b/Documentation/x86/early-microcode.txt
> index d62bea6..c4f2ebd 100644
> --- a/Documentation/x86/early-microcode.txt
> +++ b/Documentation/x86/early-microcode.txt
> @@ -14,6 +14,16 @@ during boot time. The microcode file in cpio name space is:
>  on Intel: kernel/x86/microcode/GenuineIntel.bin
>  on AMD  : kernel/x86/microcode/AuthenticAMD.bin
>  
> +For Intel processors, the microcode load process will be faster when special

faster??

> +care is taken to ensure that the kernel/x86/microcode/GenuineIntel.bin file
> +*data* inside the cpio archive is aligned to a paragraph (16-byte boundary).
> +Standard pax/cpio can be coaxed into doing this by adding a padding file, e.g.
> +"kernel/x86/microcode/.padding" with the appropriate size *right before* the
> +kernel/x86/microcode/GenuineIntel.bin file.  Beware the required size for the
> +padding file as it depends on the behavior of the tool used to create the cpio
> +archive.  It is also possible to use a specific tool that appends enough NULs
> +_to_ the file name (not _after_ the file name!) to align the file data.
> +
>  During BSP boot (before SMP starts), if the kernel finds the microcode file in
>  the initrd file, it parses the microcode and saves matching microcode in memory.
>  If matching microcode is found, it will be uploaded in BSP and later on in all
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 2182cec..40caef1 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -157,6 +157,11 @@ static int apply_microcode_intel(int cpu)
>  	if (mc_intel == NULL)
>  		return 0;
>  
> +	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
> +	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
> +		"microcode data incorrectly aligned"))

I wonder how many people would start complaining when this goes out the
door? Have you checked actually how the majority of the tools do layout
the microcode in the initrd?

> +		return -1;
> +
>  	/*
>  	 * Microcode on this CPU might be already up-to-date.  Only apply
>  	 * the microcode patch in mc_intel when it is newer than the one
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index 92629a8..994c59b 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -662,14 +662,40 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
>  	struct microcode_intel *mc_intel;
>  	unsigned int val[2];
>  
> +	char savedbuf[16];
> +	void *mcu_data;
> +	void *aligned_mcu_data;
> +	unsigned int mcu_size = 0;
> +
>  	mc_intel = uci->mc;
>  	if (mc_intel == NULL)
>  		return 0;
>  
> +	mcu_data = mc_intel->bits;
> +	aligned_mcu_data = mc_intel->bits;
> +
> +	/* Intel SDM vol 3A section 9.11.6, page 9-34: */
> +	/* WRMSR MSR_IA32_UCODE_WRITE requires 16-byte alignment */

Kernel comment style:

	/*
	 * Blabla.
	 * More bla.
	 */

> +	if ((unsigned long)(mcu_data) % 16) {
> +		/* We have more than 16 bytes worth of microcode header
> +		 * just before mc_intel->bits on a version 1 header */
> +		BUILD_BUG_ON(offsetof(struct microcode_intel, bits) < 16);

That's not really needed - I don't see struct microcode_header_intel
changing anytime soon.

> +
> +		aligned_mcu_data = (void *)((unsigned long)(mcu_data) & ~15UL);
> +		mcu_size = get_datasize(&mc_intel->hdr);
> +		memcpy(savedbuf, aligned_mcu_data, sizeof(savedbuf));
> +		memmove(aligned_mcu_data, mcu_data, mcu_size);
> +	}
> +
>  	/* write microcode via MSR 0x79 */
>  	native_wrmsr(MSR_IA32_UCODE_WRITE,
> -	      (unsigned long) mc_intel->bits,
> -	      (unsigned long) mc_intel->bits >> 16 >> 16);
> +		lower_32_bits((unsigned long)aligned_mcu_data),
> +		upper_32_bits((unsigned long)aligned_mcu_data));
> +
> +	if (mcu_size) {
> +		memmove(mcu_data, aligned_mcu_data, mcu_size);
> +		memcpy(aligned_mcu_data, savedbuf, sizeof(savedbuf));
> +	}
>  
>  	/* get the current revision from MSR 0x8B */
>  	native_wrmsr(MSR_IA32_UCODE_REV, 0, 0);
> -- 
> 1.7.10.4
> 
> 

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Fri, 07 Nov 2014, Borislav Petkov wrote:
> > --- a/Documentation/x86/early-microcode.txt
> > +++ b/Documentation/x86/early-microcode.txt
> > @@ -14,6 +14,16 @@ during boot time. The microcode file in cpio name space is:
> >  on Intel: kernel/x86/microcode/GenuineIntel.bin
> >  on AMD  : kernel/x86/microcode/AuthenticAMD.bin
> >  
> > +For Intel processors, the microcode load process will be faster when special
> 
> faster??

Well, it might well be lost in the noise given how slow a microcode update
is.

What I mean is that the early microcode driver "won't waste cpu time moving
data that is already aligned".

However, if the data is _not_ aligned (and it will *not* be aligned if you
use standard cpio without any tricks), the early driver will have to move it
around so that it respects the architectural requirement of 16-byte
alignment for the microcode update WRMSR.

> > index 2182cec..40caef1 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -157,6 +157,11 @@ static int apply_microcode_intel(int cpu)
> >  	if (mc_intel == NULL)
> >  		return 0;
> >  
> > +	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
> > +	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
> > +		"microcode data incorrectly aligned"))
> 
> I wonder how many people would start complaining when this goes out the
> door? Have you checked actually how the majority of the tools do layout
> the microcode in the initrd?

That WARN_ONCE() should only trigger if a bug shows its ugly face.  If it is
triggering in any other case, this patch is broken.

mc_intel->bits in the late driver really shouldn't depend at all on any
alignment from initramfs or whatever: that path is supposed to run on
microcode that came from the firmware loader, or the deprecated microcode
device, or which was memcpy'd from the initramfs to a kmalloc()'d area by
save_microcode() in the early driver.  All of these will always be 16-byte
aligned AFAIK.

So, the early driver gets alignment code as it will usually have to deal
with unaligned microcode data, and the late driver (which should never see
unaligned microcode data) gets a WARN_ONCE to alert us if something broke in
the kernel code.

Should I change the WARN_ONCE message to:

WARN_ONCE(... "kernel bug: microcode data incorrectly aligned") ?

> > +		return -1;
> > +
> >  	/*
> >  	 * Microcode on this CPU might be already up-to-date.  Only apply
> >  	 * the microcode patch in mc_intel when it is newer than the one
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> > index 92629a8..994c59b 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> > @@ -662,14 +662,40 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
> >  	struct microcode_intel *mc_intel;
> >  	unsigned int val[2];
> >  
> > +	char savedbuf[16];
> > +	void *mcu_data;
> > +	void *aligned_mcu_data;
> > +	unsigned int mcu_size = 0;
> > +
> >  	mc_intel = uci->mc;
> >  	if (mc_intel == NULL)
> >  		return 0;
> >  
> > +	mcu_data = mc_intel->bits;
> > +	aligned_mcu_data = mc_intel->bits;
> > +
> > +	/* Intel SDM vol 3A section 9.11.6, page 9-34: */
> > +	/* WRMSR MSR_IA32_UCODE_WRITE requires 16-byte alignment */
> 
> Kernel comment style:
> 
> 	/*
> 	 * Blabla.
> 	 * More bla.
> 	 */

Will fix.

> > +	if ((unsigned long)(mcu_data) % 16) {
> > +		/* We have more than 16 bytes worth of microcode header
> > +		 * just before mc_intel->bits on a version 1 header */
> > +		BUILD_BUG_ON(offsetof(struct microcode_intel, bits) < 16);
> 
> That's not really needed - I don't see struct microcode_header_intel
> changing anytime soon.

Neither do I.  But this has zero footprint on the resulting kernel, and
might save someone 10 years from now from trying to debug initramfs data
corruption.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Fri, Nov 07, 2014 at 08:54:25PM -0200, Henrique de Moraes Holschuh wrote:
> Well, it might well be lost in the noise given how slow a microcode update
> is.
> 
> What I mean is that the early microcode driver "won't waste cpu time moving
> data that is already aligned".

Yes, don't say "faster" but explain that we can save us the shuffling
of microcode data back and forth in the early loader if the microcode
update is 16-byte aligned in the initrd.

> That WARN_ONCE() should only trigger if a bug shows its ugly face.  If it is
> triggering in any other case, this patch is broken.
> 
> mc_intel->bits in the late driver really shouldn't depend at all on any
> alignment from initramfs or whatever: that path is supposed to run on
> microcode that came from the firmware loader, or the deprecated microcode

Right, the early path is apply_microcode_early() - I misread that part,
sorry.

> device, or which was memcpy'd from the initramfs to a kmalloc()'d area by
> save_microcode() in the early driver.  All of these will always be 16-byte
> aligned AFAIK.

Why will it always be 16-byte aligned? And if it is going to be always
16-byte aligned, why do we need the WARN_ONCE() at all?

> So, the early driver gets alignment code as it will usually have to deal
> with unaligned microcode data, and the late driver (which should never see
> unaligned microcode data) gets a WARN_ONCE to alert us if something broke in
> the kernel code.
> 
> Should I change the WARN_ONCE message to:
> 
> WARN_ONCE(... "kernel bug: microcode data incorrectly aligned") ?

No, you should either give a possible scenario where an unaligned
pointer can really happen or, alternatively, if you can prove that the
condition will never happen, drop the WARN_ONCE completely.

And please no improbable
maybe-it-could-once-in-a-blue-moon-when-the-stars-align-happen cases.

Oh, and even if the WARN_ONCE triggers, what can we do about it? If we
can't do anything about it, then we have a problem. If we can, then we
better do it and change the WARN_ONCE to an if-check which, if positive,
executes code that fixes the situation.

IOW, what I'm trying to say is, can we make the kmalloc'ed memory
16-byte aligned and if so, then copy the microcode data there and be
happy. And I think we can... :-)

> Neither do I.  But this has zero footprint on the resulting kernel, and
> might save someone 10 years from now from trying to debug initramfs data
> corruption.

... and someone might waste a lot of time 10 years from now trying
to fathom why the hell that thing was added, only to realize that
someone was being unreasonably overzealous. No, we don't add code for
hypothetical cases which are absolutely improbable.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Sat, 08 Nov 2014, Borislav Petkov wrote:
> On Fri, Nov 07, 2014 at 08:54:25PM -0200, Henrique de Moraes Holschuh wrote:
> > Well, it might well be lost in the noise given how slow a microcode update
> > is.
> > 
> > What I mean is that the early microcode driver "won't waste cpu time moving
> > data that is already aligned".
> 
> Yes, don't say "faster" but explain that we can save us the shuffling
> of microcode data back and forth in the early loader if the microcode
> update is 16-byte aligned in the initrd.

Will change.

> > That WARN_ONCE() should only trigger if a bug shows its ugly face.  If it is
> > triggering in any other case, this patch is broken.
> > 
> > mc_intel->bits in the late driver really shouldn't depend at all on any
> > alignment from initramfs or whatever: that path is supposed to run on
> > microcode that came from the firmware loader, or the deprecated microcode
> 
> Right, the early path is apply_microcode_early() - I misread that part,
> sorry.
> 
> > device, or which was memcpy'd from the initramfs to a kmalloc()'d area by
> > save_microcode() in the early driver.  All of these will always be 16-byte
> > aligned AFAIK.
> 
> Why will it always be 16-byte aligned? And if it is going to be always
> 16-byte aligned, why do we need the WARN_ONCE() at all?

...

> > So, the early driver gets alignment code as it will usually have to deal
> > with unaligned microcode data, and the late driver (which should never see
> > unaligned microcode data) gets a WARN_ONCE to alert us if something broke in
> > the kernel code.
> > 
> > Should I change the WARN_ONCE message to:
> > 
> > WARN_ONCE(... "kernel bug: microcode data incorrectly aligned") ?
> 
> No, you should either give a possible scenario where an unaligned
> pointer can really happen or, alternatively, if you can prove that the
> condition will never happen, drop the WARN_ONCE completely.

If I know of a non-kernel-bug scenario where it could happen, I'd have added
code that fixes the alignment like I did in the early driver...

AFAIK, that WARN_ON is only going to trigger if kmalloc changes and starts
returning less strictly aligned data, or if something is corrupting memory.

I will remove the WARN_ONCE, and place a comment in its place:

/*
 * the memory area holding the microcode update data must be 16-byte
 * aligned.  This is supposed to be guaranteed by kmalloc().
 */

> Oh, and even if the WARN_ONCE triggers, what can we do about it? If we
> can't do anything about it, then we have a problem. If we can, then we
> better do it and change the WARN_ONCE to an if-check which, if positive,
> executes code that fixes the situation.

All we could do is abort the update.  The real reason behind the
misalignment would have to be tracked in order to know what should be done
about it.

> IOW, what I'm trying to say is, can we make the kmalloc'ed memory
> 16-byte aligned and if so, then copy the microcode data there and be
> happy. And I think we can... :-)

Sure, we can.  But AFAIK kmalloc() is supposed to already give us a 16-byte
aligned data area.

> > Neither do I.  But this has zero footprint on the resulting kernel, and
> > might save someone 10 years from now from trying to debug initramfs data
> > corruption.
> 
> ... and someone might waste a lot of time 10 years from now trying
> to fathom why the hell that thing was added, only to realize that
> someone was being unreasonably overzealous. No, we don't add code for
> hypothetical cases which are absolutely improbable.

I will drop it.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Sat, Nov 08, 2014 at 07:57:49PM -0200, Henrique de Moraes Holschuh wrote:
> I will remove the WARN_ONCE, and place a comment in its place:
> 
> /*
>  * the memory area holding the microcode update data must be 16-byte
>  * aligned.  This is supposed to be guaranteed by kmalloc().
>  */

So this makes this comment pretty useless as it doesn't do anything
about the case where 16-byte alignment gets violated. Actually I was
expecting something else:

* you either write down *why* kmalloc guarantees alignment. From a quick
look it might but it might not, hint

#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)

* or you actually go and fix this by making sure all memory in the intel
loader is 16-byte aligned. Maybe a loader-specific kmalloc wrapper,
something which allocates a bit more and then aligns it properly, and so
on...

But simply adding a comment which doesn't do anything to solve the
situation doesn't make a lot of sense. And more importantly, doesn't
solve the situation.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Tue, 11 Nov 2014, Borislav Petkov wrote:
> On Sat, Nov 08, 2014 at 07:57:49PM -0200, Henrique de Moraes Holschuh wrote:
> * you either write down *why* kmalloc guarantees alignment. From a quick
> look it might but it might not, hint
> 
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)

Meh, I don't know where I came up with the wrong information that kmalloc
aligned to 16-bytes instead of 8 bytes.

I do wonder why I didn't hit this while testing, though.  Maybe an artifact
of slub, or just my luck that I never got a memory block that was not
aligned to 16 bytes.

I will fix this.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Tue, Nov 11, 2014 at 02:57:31PM -0200, Henrique de Moraes Holschuh wrote:
> Meh, I don't know where I came up with the wrong information that kmalloc
> aligned to 16-bytes instead of 8 bytes.
> 
> I do wonder why I didn't hit this while testing, though.  Maybe an artifact
> of slub, or just my luck that I never got a memory block that was not
> aligned to 16 bytes.

The ARCH_KMALLOC_MINALIGN is conditioned on ARCH_DMA_MINALIGN and a
bunch of other things. All I'm saying is, this needs a careful study
when, if at all, kmalloc will not give an 16-byte aligned buffer.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Tue, 11 Nov 2014, Borislav Petkov wrote:
> On Tue, Nov 11, 2014 at 02:57:31PM -0200, Henrique de Moraes Holschuh wrote:
> > Meh, I don't know where I came up with the wrong information that kmalloc
> > aligned to 16-bytes instead of 8 bytes.
> > 
> > I do wonder why I didn't hit this while testing, though.  Maybe an artifact
> > of slub, or just my luck that I never got a memory block that was not
> > aligned to 16 bytes.
> 
> The ARCH_KMALLOC_MINALIGN is conditioned on ARCH_DMA_MINALIGN and a
> bunch of other things. All I'm saying is, this needs a careful study
> when, if at all, kmalloc will not give an 16-byte aligned buffer.

I tried to do that in order to answer my own question.  After a quick look,
it looks like it ends up being an implementation detail of SLUB/SLAB/SLOB,
and it depends on the size of the object being allocated (i.e. if it is
bigger than KMALLOC_MAX_CACHE_SIZE).

I was always getting page-aligned memory blocks out of kmalloc() in my
testing because of that.

It won't be an ugly fix anyway, something like this (conceptual code):

/* as required by Intel SDM blahblahblah */
#define INTEL_MICROCODE_MINALIGN 16
#define INTEL_UCODE_PTR(x) PTR_ALIGN(x, INTEL_MICROCODE_MINALIGN)

void *intel_ucode_kmalloc(size_t size)
{
	void *p = kmalloc(size, GFP_KERNEL);
	if (likely(p == INTEL_UCODE_PTR(p))
		return p;

	kfree(p);
	return kmalloc(size + INTEL_MICROCODE_MINALIGN - 1, GFP_KERNEL);
}

For most users, that "p == INTEL_UCODE_PTR(p)" will always be true (SLUB
alocator, and microcode size >= 4096).

This way, we don't go around wasting pages when the microcode size is large
enough for the allocation to get kicked directly to the page allocator and
it is also is a multiple of PAGE_SIZE (which it often is).

I will need to add some defense against (size + 15) overflow in that custom
kmalloc, or ensure that we refuse ridiculously large microcode early (say,
larger than 4MiB).  Not a big deal, we probably already do this, and if we
don't, it will be an worthwhile enhancement by itself.  The largest
microcode I have been made aware of is ~64KiB.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Tue, Nov 11, 2014 at 05:54:00PM -0200, Henrique de Moraes Holschuh wrote:
> void *intel_ucode_kmalloc(size_t size)
> {
> 	void *p = kmalloc(size, GFP_KERNEL);

Actually I was thinking of this:

 	void *p = kmalloc(size + 16, GFP_KERNEL);
	if (!p)
		return -ENOMEM;

	if (unlikely((unsigned long)p & 0xf))
		p_a = ALIGN(p, 16);

You'd need to stash the original *p somewhere for freeing later, of
course.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Wed, 12 Nov 2014, Borislav Petkov wrote:
> On Tue, Nov 11, 2014 at 05:54:00PM -0200, Henrique de Moraes Holschuh wrote:
> > void *intel_ucode_kmalloc(size_t size)
> > {
> > 	void *p = kmalloc(size, GFP_KERNEL);
> 
> Actually I was thinking of this:
> 
>  	void *p = kmalloc(size + 16, GFP_KERNEL);
> 	if (!p)
> 		return -ENOMEM;
> 
> 	if (unlikely((unsigned long)p & 0xf))
> 		p_a = ALIGN(p, 16);
> 
> You'd need to stash the original *p somewhere for freeing later, of
> course.

Well, it is a trade-off: your version always add 16 bytes.  Intel microcode
is always a multiple of 1KiB, so these extra 16 bytes will often result in
allocating an extra page.

The detail is that: since most Intel microcodes are bigger than the kmalloc
cache, most of the time kmalloc will return page-aligned addresses, which
don't need any alignment.

Your version also needs to keep the original pointer around for kfree, which
is going to be annoying.

My version has the drawback that it requires the use of INTEL_UCODE_PTR(p)
to get to the microcode data, but you can just kfree(p), and it will only
add the 16 bytes when absolutely required.  This is nice, because it means
we won't waste an extra page in the most common case, and we don't have to
find a place to store any extra pointers.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Wed, Nov 12, 2014 at 10:18:47PM -0200, Henrique de Moraes Holschuh wrote:
> The detail is that: since most Intel microcodes are bigger than the kmalloc
> cache, most of the time kmalloc will return page-aligned addresses, which
> don't need any alignment.

Yeah, you keep saying that. Do you have an actual proof too?

Because if this turns out wrong, we'll end up doing two allocations
instead of one, which is dumb. My suggestion was to do one allocation
only.

> Your version also needs to keep the original pointer around for kfree, which
> is going to be annoying.
> 
> My version has the drawback that it requires the use of INTEL_UCODE_PTR(p)

Yeah, just drop that macro - use simply PTR_ALIGN and
INTEL_MICROCODE_MINALIGN.

> to get to the microcode data, but you can just kfree(p), and it will only
> add the 16 bytes when absolutely required.  This is nice, because it means
> we won't waste an extra page in the most common case, and we don't have to
> find a place to store any extra pointers.

Yeah yeah, proof please.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Thu, 13 Nov 2014, Borislav Petkov wrote:
> On Wed, Nov 12, 2014 at 10:18:47PM -0200, Henrique de Moraes Holschuh wrote:
> > The detail is that: since most Intel microcodes are bigger than the kmalloc
> > cache, most of the time kmalloc will return page-aligned addresses, which
> > don't need any alignment.
> 
> Yeah, you keep saying that. Do you have an actual proof too?

I believe so, from documention gathered through google... but I could be
wrong about this.

And since I have learned my lesson, I took your comment to mean "look
deeper".  I took the time to both try to understand somewhat the mm/ code
AND write a kernel module to do empiric testing before I wrote this reply,
instead of trusting the documentation.

And it turns out I failed at proving myself wrong, either because I am not
wrong, or by sheer unluckyness.


Important detail: intel microcodes have 1KiB size granularity, are never
smaller than 2048 bytes, and so far the largest ones are close to 64KiB.


For SLUB:

kmalloc() will do kmalloc_large() for anything > 8192 bytes and that should
return page-aligned data since it calls alloc_kmem_pages()/alloc_pages().

For 2048 bytes to 8192 bytes, we will get memory from one of the following
slabs: kmalloc-2048, kmalloc-4096 or kmalloc-8192.

As far as I could understand (and here I could easily be wrong as the mm/
slab cache code is not trivial to grok), those are going to be object-size
aligned, because the allocation size will be rounded up to the slab's object
size (i.e. 2048/4096/8192 bytes).  The objects are allocated from the start
of the slab (which is page-aligned), back-to-back.

Thus SLUB would always return 16-byte aligned memory for the size range of
Intel microcode.  In fact, it will return 2048-byte aligned memory in the
worst case (2048-byte microcode).

Empiric testing in x86-64 resulted in data compatible with the above
reasoning.


For SLAB:

SLAB is nasty to grok with a first look due to the whole complexity re. its
queues and cpu caches, and I don't think I understood the code properly.

intel microcode sized allocations end up in slabs with large objects that
are all of the same 2^n size (i.e. 2048 bytes, 4096 bytes, etc).  I could
not grok the code enough to assert what real alignment I could expect for
2KiB to 64KiB objects.

Empiric testing in x86-64 SLAB, done by allocating 63 objects of the same
size in a row, for all possible microcode sizes, did not result in a single
kmalloc() that was not sufficiently aligned, and in fact all of them were
object-size aligned, even for the smallest microcodes (2048 bytes).  I even
tried it with CONFIG_DEBUG_SLAB turned on and off to see if it changed
anything (it didn't).

So, while I didn't understand the code enough to prove that we'd mostly get
good microcode alignment out of SLAB, I couldn't get it to return pointers
that would require extra alignment either.  The worst case was 2048-byte
alignment, for microcodes with 2048 bytes.


For SLOB:

SLOB will call the page allocator for anything bigger than 4095 bytes, so
all microcodes from 4096 bytes and above will be page-aligned.

Only 2048-byte and 3072-byte microcode wouldn't go directly to the page
allocator.  This is microcode for ancient processors: Pentium M and earlier,
and the first NetBurst processors.

I didn't bother to test, but from the code, I expect that 2048-byte and
3072-byte sized microcode would *not* be aligned to 16 bytes.  However, we
have very few users of these ancient processors left.  Calling kmalloc(s);
kfree(); kmalloc(s+15); for these rare processors doesn't look like too much
of an issue IMHO.

SLOB was the only allocator that could result in microcode that needs
further alignment in my testing, and only for the small microcode (2KiB and
3KiB) of ancient processors.

> Because if this turns out wrong, we'll end up doing two allocations
> instead of one, which is dumb. My suggestion was to do one allocation
> only.

See above.  As far as I could verify, we wouldn't be doing two allocations
in the common cases.

I really don't like the idea of increasing the allocation order by one for
4KiB, 8KiB, 16KiB, 32KiB and 64KiB microcodes just to alocate 15 bytes of
extra padding that, at least as far as I could check, seem to be most often
not needed.

Note that I did not do any empiric testing on 32 bits.

> > Your version also needs to keep the original pointer around for kfree, which
> > is going to be annoying.
> > 
> > My version has the drawback that it requires the use of INTEL_UCODE_PTR(p)
> 
> Yeah, just drop that macro - use simply PTR_ALIGN and
> INTEL_MICROCODE_MINALIGN.

I will do that.

-- 
  "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

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Borislav Petkov]

On Sat, Nov 15, 2014 at 09:10:44PM -0200, Henrique de Moraes Holschuh wrote:
> And since I have learned my lesson, I took your comment to mean "look
> deeper".  I took the time to both try to understand somewhat the mm/ code
> AND write a kernel module to do empiric testing before I wrote this reply,
> instead of trusting the documentation.

Good! Thanks. :-)

> Important detail: intel microcodes have 1KiB size granularity, are never
> smaller than 2048 bytes, and so far the largest ones are close to 64KiB.

Right, we should be able to manage 16 pages allocation on most systems.

> For SLUB:
> 
> kmalloc() will do kmalloc_large() for anything > 8192 bytes and that should
> return page-aligned data since it calls alloc_kmem_pages()/alloc_pages().

Yep.

> For 2048 bytes to 8192 bytes, we will get memory from one of the following
> slabs: kmalloc-2048, kmalloc-4096 or kmalloc-8192.
> 
> As far as I could understand (and here I could easily be wrong as the mm/
> slab cache code is not trivial to grok), those are going to be object-size
> aligned, because the allocation size will be rounded up to the slab's object
> size (i.e. 2048/4096/8192 bytes).  The objects are allocated from the start
> of the slab (which is page-aligned), back-to-back.
> 
> Thus SLUB would always return 16-byte aligned memory for the size range of
> Intel microcode.  In fact, it will return 2048-byte aligned memory in the
> worst case (2048-byte microcode).

I'm wondering if those slab objects would have some sort of
metadata after them so that the nice alignment of 2048, 4096 and 8192
gets b0rked. Because there are those members there in slub_def.h:

/*
 * Slab cache management.
 */
struct kmem_cache {
	...
        int size;               /* The size of an object including meta data */
        int object_size;        /* The size of an object without meta data */

but dumping the values from sysfs show me that there's no metadata:

/sys/kernel/slab/kmalloc-2048/object_size:2048
/sys/kernel/slab/kmalloc-2048/slab_size:2048
/sys/kernel/slab/kmalloc-4096/object_size:4096
/sys/kernel/slab/kmalloc-4096/slab_size:4096
/sys/kernel/slab/kmalloc-8192/object_size:8192
/sys/kernel/slab/kmalloc-8192/slab_size:8192

so we should be fine.

> For SLAB:
> 
> SLAB is nasty to grok with a first look due to the whole complexity re. its
> queues and cpu caches, and I don't think I understood the code properly.
>
> intel microcode sized allocations end up in slabs with large objects that
> are all of the same 2^n size (i.e. 2048 bytes, 4096 bytes, etc).  I could
> not grok the code enough to assert what real alignment I could expect for
> 2KiB to 64KiB objects.

Well, 2KiB alignment certainly satisfies 16 bytes alignment.

> Empiric testing in x86-64 SLAB, done by allocating 63 objects of the same
> size in a row, for all possible microcode sizes, did not result in a single
> kmalloc() that was not sufficiently aligned, and in fact all of them were
> object-size aligned, even for the smallest microcodes (2048 bytes).  I even
> tried it with CONFIG_DEBUG_SLAB turned on and off to see if it changed
> anything (it didn't).

Ok.

> So, while I didn't understand the code enough to prove that we'd mostly get
> good microcode alignment out of SLAB, I couldn't get it to return pointers
> that would require extra alignment either.  The worst case was 2048-byte
> alignment, for microcodes with 2048 bytes.

Well, looking at calculate_alignment() in mm/slab_common.c
and since we're being called with SLAB_HWCACHE_ALIGN, i.e.
create_kmalloc_caches(ARCH_KMALLOC_FLAGS) in kmem_cache_init(), then the
loop in calculate_alignment() will definitely give higher alignment than
16 for the larger caches. This is, of course AFAICT.

> For SLOB:
> 
> SLOB will call the page allocator for anything bigger than 4095 bytes, so
> all microcodes from 4096 bytes and above will be page-aligned.
> 
> Only 2048-byte and 3072-byte microcode wouldn't go directly to the page
> allocator.  This is microcode for ancient processors: Pentium M and earlier,
> and the first NetBurst processors.
> 
> I didn't bother to test, but from the code, I expect that 2048-byte and
> 3072-byte sized microcode would *not* be aligned to 16 bytes.  However, we
> have very few users of these ancient processors left.  Calling kmalloc(s);
> kfree(); kmalloc(s+15); for these rare processors doesn't look like too much
> of an issue IMHO.
> 
> SLOB was the only allocator that could result in microcode that needs
> further alignment in my testing, and only for the small microcode (2KiB and
> 3KiB) of ancient processors.

Right, it looks like slob could give objects aligned to < 16.

Ok, please add the jist of this to the commit message without going into
unnecessary detail too much.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 7/8] x86, microcode, intel: guard against misaligned microcode data

[Henrique de Moraes Holschuh]

On Mon, 24 Nov 2014, Borislav Petkov wrote:
> On Sat, Nov 15, 2014 at 09:10:44PM -0200, Henrique de Moraes Holschuh wrote:
> > For SLAB:
> > 
> > SLAB is nasty to grok with a first look due to the whole complexity re. its
> > queues and cpu caches, and I don't think I understood the code properly.
> >
> > intel microcode sized allocations end up in slabs with large objects that
> > are all of the same 2^n size (i.e. 2048 bytes, 4096 bytes, etc).  I could
> > not grok the code enough to assert what real alignment I could expect for
> > 2KiB to 64KiB objects.
> 
> Well, 2KiB alignment certainly satisfies 16 bytes alignment.

Yeah, however I was not sure SLAB wouldn't spoil the fun by adding a header
before the slab and screwing up MIN(page-size, object-size) alignment inside
the slab.

Unlike SLUB, SLAB is documented to add an enemy-of-large-alignment
head-of-slab header in an attempt to make the world a sadder place where
even SIMD instructions (that want cache-line alignment) would suffer...

But my empiric testing didn't show any of that sadness for these larger
allocation sizes (2KiB and bigger).  They're all either page-aligned or
object-size aligned, regardless of whether SLAB debug mode was compiled in
or not. 

> > Empiric testing in x86-64 SLAB, done by allocating 63 objects of the same
> > size in a row, for all possible microcode sizes, did not result in a single
> > kmalloc() that was not sufficiently aligned, and in fact all of them were
> > object-size aligned, even for the smallest microcodes (2048 bytes).  I even
> > tried it with CONFIG_DEBUG_SLAB turned on and off to see if it changed
> > anything (it didn't).
> 
> Ok.
> 
> > So, while I didn't understand the code enough to prove that we'd mostly get
> > good microcode alignment out of SLAB, I couldn't get it to return pointers
> > that would require extra alignment either.  The worst case was 2048-byte
> > alignment, for microcodes with 2048 bytes.
> 
> Well, looking at calculate_alignment() in mm/slab_common.c
> and since we're being called with SLAB_HWCACHE_ALIGN, i.e.
> create_kmalloc_caches(ARCH_KMALLOC_FLAGS) in kmem_cache_init(), then the
> loop in calculate_alignment() will definitely give higher alignment than
> 16 for the larger caches. This is, of course AFAICT.

Ah, that explains it.  Thanks.  I missed the
create_kmalloc_caches(ARCH_KMALLOC_FLAGS) detail, which indeed ensures there
is no slab info at the head of the slab, so all intel microcode objects will
end up aligned to MIN(page-size, object-size).

> > For SLOB:
> > 
> > SLOB will call the page allocator for anything bigger than 4095 bytes, so
> > all microcodes from 4096 bytes and above will be page-aligned.
> > 
> > Only 2048-byte and 3072-byte microcode wouldn't go directly to the page
> > allocator.  This is microcode for ancient processors: Pentium M and earlier,
> > and the first NetBurst processors.
> > 
> > I didn't bother to test, but from the code, I expect that 2048-byte and
> > 3072-byte sized microcode would *not* be aligned to 16 bytes.  However, we
> > have very few users of these ancient processors left.  Calling kmalloc(s);
> > kfree(); kmalloc(s+15); for these rare processors doesn't look like too much
> > of an issue IMHO.
> > 
> > SLOB was the only allocator that could result in microcode that needs
> > further alignment in my testing, and only for the small microcode (2KiB and
> > 3KiB) of ancient processors.
> 
> Right, it looks like slob could give objects aligned to < 16.
> 
> Ok, please add the jist of this to the commit message without going into
> unnecessary detail too much.

Will do, thanks!

-- 
  "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

[PATCH 8/8] x86, microcode, intel: defend apply_microcode_intel with BUG_ON

[Henrique de Moraes Holschuh]

Microcode updates that requires an unknown loader should never reach the
apply_* functions (the code should have rejected it earlier).  Likewise
for an unknown microcode header layout.

Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
---
 arch/x86/kernel/cpu/microcode/intel.c       |    2 ++
 arch/x86/kernel/cpu/microcode/intel_early.c |    2 ++
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 40caef1..439681f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -157,6 +157,8 @@ static int apply_microcode_intel(int cpu)
 	if (mc_intel == NULL)
 		return 0;
 
+	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
+
 	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
 	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
 		"microcode data incorrectly aligned"))
diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
index 994c59b..095db11 100644
--- a/arch/x86/kernel/cpu/microcode/intel_early.c
+++ b/arch/x86/kernel/cpu/microcode/intel_early.c
@@ -671,6 +671,8 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
 	if (mc_intel == NULL)
 		return 0;
 
+	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
+
 	mcu_data = mc_intel->bits;
 	aligned_mcu_data = mc_intel->bits;
 
-- 
1.7.10.4

Re: [PATCH 8/8] x86, microcode, intel: defend apply_microcode_intel with BUG_ON

[Borislav Petkov]

On Mon, Sep 08, 2014 at 02:37:54PM -0300, Henrique de Moraes Holschuh wrote:
> Microcode updates that requires an unknown loader should never reach the
> apply_* functions (the code should have rejected it earlier).  Likewise
> for an unknown microcode header layout.
> 
> Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> ---
>  arch/x86/kernel/cpu/microcode/intel.c       |    2 ++
>  arch/x86/kernel/cpu/microcode/intel_early.c |    2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 40caef1..439681f 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -157,6 +157,8 @@ static int apply_microcode_intel(int cpu)
>  	if (mc_intel == NULL)
>  		return 0;
>  
> +	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
> +
>  	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
>  	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
>  		"microcode data incorrectly aligned"))
> diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> index 994c59b..095db11 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> @@ -671,6 +671,8 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
>  	if (mc_intel == NULL)
>  		return 0;
>  
> +	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
> +
>  	mcu_data = mc_intel->bits;
>  	aligned_mcu_data = mc_intel->bits;

Both not needed, because we're running all microcode through
microcode_sanity_check() first which already does that check.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 8/8] x86, microcode, intel: defend apply_microcode_intel with BUG_ON

[Henrique de Moraes Holschuh]

On Fri, 07 Nov 2014, Borislav Petkov wrote:
> On Mon, Sep 08, 2014 at 02:37:54PM -0300, Henrique de Moraes Holschuh wrote:
> > Microcode updates that requires an unknown loader should never reach the
> > apply_* functions (the code should have rejected it earlier).  Likewise
> > for an unknown microcode header layout.
> > 
> > Signed-off-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> > ---
> >  arch/x86/kernel/cpu/microcode/intel.c       |    2 ++
> >  arch/x86/kernel/cpu/microcode/intel_early.c |    2 ++
> >  2 files changed, 4 insertions(+)
> > 
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index 40caef1..439681f 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -157,6 +157,8 @@ static int apply_microcode_intel(int cpu)
> >  	if (mc_intel == NULL)
> >  		return 0;
> >  
> > +	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
> > +
> >  	/* Intel SDM vol 3A section 9.11.6, page 9-34 */
> >  	if (WARN_ONCE((unsigned long)(mc_intel->bits) % 16,
> >  		"microcode data incorrectly aligned"))
> > diff --git a/arch/x86/kernel/cpu/microcode/intel_early.c b/arch/x86/kernel/cpu/microcode/intel_early.c
> > index 994c59b..095db11 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel_early.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel_early.c
> > @@ -671,6 +671,8 @@ static int apply_microcode_early(struct mc_saved_data *mc_saved_data,
> >  	if (mc_intel == NULL)
> >  		return 0;
> >  
> > +	BUG_ON(mc_intel->hdr.hdrver != 1 || mc_intel->hdr.ldrver != 1);
> > +
> >  	mcu_data = mc_intel->bits;
> >  	aligned_mcu_data = mc_intel->bits;
> 
> Both not needed, because we're running all microcode through
> microcode_sanity_check() first which already does that check.

Yeah, that's why it is BUG_ON().

But if you feel this is too defensive, I will just drop this patch.

-- 
  "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

Re: [PATCH 8/8] x86, microcode, intel: defend apply_microcode_intel with BUG_ON

[Borislav Petkov]

On Fri, Nov 07, 2014 at 08:56:39PM -0200, Henrique de Moraes Holschuh wrote:
> But if you feel this is too defensive,

Yes I do.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--