[PATCH 00/13] x86/microcode: 4.11 queue

[PATCH 00/13] x86/microcode: 4.11 queue

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Hi,

so this is more of a lessons-learned pile after the rewriting of the
whole patch loading method and not caching addresses. It largely
simplifies the loader - just look at the diffstat - without any
functionality loss (I hope :-)). The driver is also very much readable
now with unified, common paths where possible.

What is more, 6/13 reworks the whole AMD container parsing into much
more readable separation of functionality and functions doing only one
thing properly.

13/13 is also another nice simplification for the AP update path which
looks almost straightforward in comparison with what we had before.

All has been tested on a lot of boxes and in different configurations:

* builtin vs initrd microcode
* old microcode_ctl application method for Intel
* "echo 1 > /sys/devices/system/cpu/microcode/reload" late method
* CONFIG_RANDOMIZE_MEMORY

both on Intel and AMD machines.

Please queue for 4.11.

Thanks.

Borislav Petkov (13):
  x86/microcode/intel: Drop stashed AP patch pointer optimization
  x86/microcode: Use own MSR accessors
  x86/microcode/AMD: Clean up find_equiv_id()
  x86/microcode/AMD: Shorten function parameter's name
  x86/microcode/AMD: Extend the container struct
  x86/microcode/AMD: Rework container parsing
  x86/microcode: Decrease CPUID use
  x86/microcode/AMD: Get rid of global this_equiv_id
  x86/microcode/AMD: Use find_microcode_in_initrd()
  x86/microcode/AMD: Check patch level only on the BSP
  x86/microcode/AMD: Unify load_ucode_amd_ap()
  x86/microcode/AMD: Simplify saving from initrd
  x86/microcode/AMD: Remove AP scanning optimization

 arch/x86/include/asm/microcode.h       |  29 +-
 arch/x86/include/asm/microcode_amd.h   |   2 -
 arch/x86/include/asm/microcode_intel.h |   4 +-
 arch/x86/kernel/cpu/microcode/amd.c    | 501 +++++++++++----------------------
 arch/x86/kernel/cpu/microcode/core.c   |  81 ++++--
 arch/x86/kernel/cpu/microcode/intel.c  |  13 +-
 6 files changed, 248 insertions(+), 382 deletions(-)

-- 
2.11.0

[PATCH 01/13] x86/microcode/intel: Drop stashed AP patch pointer optimization

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This was meant to save us the scanning of the microcode containter in
the initrd since the first AP had already done that but it can also hurt
us:

Imagine a single hyperthreaded CPU (Intel(R) Atom(TM) CPU N270 for
example) which updates the microcode on the BSP but since the microcode
engine is shared between the two threads, the update on CPU1 doesn't
happen because it has already happened on CPU0 and we don't find a newer
microcode revision.

Which doesn't set the intel_ucode_patch pointer and at initrd
jettisoning time and we don't save the microcode patch for later
application.

Now, when we suspend to RAM, the loaded microcode gets cleared so we
need to reload but there's no patch saved in the cache.

Removing this optimization fixes this issue and all is fine and dandy.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/intel.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 3f329b74e040..8325d8a09ab0 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -41,7 +41,7 @@
 
 static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
 
-/* Current microcode patch used in early patching */
+/* Current microcode patch used in early patching on the APs. */
 struct microcode_intel *intel_ucode_patch;
 
 static inline bool cpu_signatures_match(unsigned int s1, unsigned int p1,
@@ -607,12 +607,6 @@ int __init save_microcode_in_initrd_intel(void)
 	struct ucode_cpu_info uci;
 	struct cpio_data cp;
 
-	/*
-	 * AP loading didn't find any microcode patch, no need to save anything.
-	 */
-	if (!intel_ucode_patch || IS_ERR(intel_ucode_patch))
-		return 0;
-
 	if (!load_builtin_intel_microcode(&cp))
 		cp = find_microcode_in_initrd(ucode_path, false);
 
@@ -628,7 +622,6 @@ int __init save_microcode_in_initrd_intel(void)
 	return 0;
 }
 
-
 /*
  * @res_patch, output: a pointer to the patch we found.
  */
-- 
2.11.0

Re: [PATCH 01/13] x86/microcode/intel: Drop stashed AP patch pointer optimization

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> This was meant to save us the scanning of the microcode containter in
> the initrd since the first AP had already done that but it can also hurt
> us:
> 
> Imagine a single hyperthreaded CPU (Intel(R) Atom(TM) CPU N270 for
> example) which updates the microcode on the BSP but since the microcode
> engine is shared between the two threads, the update on CPU1 doesn't
> happen because it has already happened on CPU0 and we don't find a newer
> microcode revision.
> 
> Which doesn't set the intel_ucode_patch pointer and at initrd
> jettisoning time and we don't save the microcode patch for later
> application.
> 
> Now, when we suspend to RAM, the loaded microcode gets cleared so we
> need to reload but there's no patch saved in the cache.
> 
> Removing this optimization fixes this issue and all is fine and dandy.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

So this one needs to go into x86/urgent

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 02/13] x86/microcode: Use own MSR accessors

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Having tracepoints to the MSR accessors makes them unsuitable for early
microcode loading: think 32-bit before paging is enabled and us chasing
pointers to test whether a tracepoint is enabled or not. Results in a
reliable triple fault.

Thus, define microcode loader-specific MSR accessors which do the bare
minimum of reading or writing an MSR and nothing more.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/microcode.h       | 29 +++++++++++++++++++++--------
 arch/x86/include/asm/microcode_intel.h |  4 ++--
 arch/x86/kernel/cpu/microcode/amd.c    |  6 +++---
 arch/x86/kernel/cpu/microcode/intel.c  |  4 ++--
 4 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 38711df3bcb5..fbecea6e46e2 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -5,20 +5,33 @@
 #include <linux/earlycpio.h>
 #include <linux/initrd.h>
 
-#define native_rdmsr(msr, val1, val2)			\
+static inline unsigned long long __rdmsr(unsigned int msr)
+{
+	DECLARE_ARGS(val, low, high);
+
+	asm volatile("1: rdmsr\n"
+		     "2:\n"
+		     : EAX_EDX_RET(val, low, high) : "c" (msr));
+	return EAX_EDX_VAL(val, low, high);
+}
+
+#define microcode_rdmsr(msr, val1, val2)		\
 do {							\
-	u64 __val = native_read_msr((msr));		\
+	u64 __val = __rdmsr((msr));			\
 	(void)((val1) = (u32)__val);			\
 	(void)((val2) = (u32)(__val >> 32));		\
 } while (0)
 
-#define native_wrmsr(msr, low, high)			\
-	native_write_msr(msr, low, high)
+static inline void microcode_wrmsr(unsigned int msr, u64 val)
+{
+	u32 low, high;
+
+	low  = (u32)val;
+	high = (u32)(val >> 32);
 
-#define native_wrmsrl(msr, val)				\
-	native_write_msr((msr),				\
-			 (u32)((u64)(val)),		\
-			 (u32)((u64)(val) >> 32))
+	asm volatile("wrmsr\n"
+		     :: "c" (msr), "a"(low), "d" (high) : "memory");
+}
 
 struct ucode_patch {
 	struct list_head plist;
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index e793fc9a9b20..7330b2b79484 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -56,13 +56,13 @@ static inline u32 intel_get_microcode_revision(void)
 {
 	u32 rev, dummy;
 
-	native_wrmsrl(MSR_IA32_UCODE_REV, 0);
+	microcode_wrmsr(MSR_IA32_UCODE_REV, 0);
 
 	/* As documented in the SDM: Do a CPUID 1 here */
 	native_cpuid_eax(1);
 
 	/* get the current revision from MSR 0x8B */
-	native_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
+	microcode_rdmsr(MSR_IA32_UCODE_REV, dummy, rev);
 
 	return rev;
 }
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6a31e2691f3a..3f89e6712afe 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -198,10 +198,10 @@ static int __apply_microcode_amd(struct microcode_amd *mc_amd)
 {
 	u32 rev, dummy;
 
-	native_wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
+	microcode_wrmsr(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
 
 	/* verify patch application was successful */
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 	if (rev != mc_amd->hdr.patch_id)
 		return -1;
 
@@ -656,7 +656,7 @@ bool check_current_patch_level(u32 *rev, bool early)
 	bool ret = false;
 	u32 *levels;
 
-	native_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
 
 	if (IS_ENABLED(CONFIG_X86_32) && early)
 		levels = (u32 *)__pa_nodebug(&final_levels);
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 8325d8a09ab0..1c6f12fa3108 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -387,7 +387,7 @@ static int collect_cpu_info_early(struct ucode_cpu_info *uci)
 
 	if ((model >= 5) || (family > 6)) {
 		/* get processor flags from MSR 0x17 */
-		native_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
+		microcode_rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
 		csig.pf = 1 << ((val[1] >> 18) & 7);
 	}
 
@@ -582,7 +582,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 		return 0;
 
 	/* write microcode via MSR 0x79 */
-	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
+	microcode_wrmsr(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
 	rev = intel_get_microcode_revision();
 	if (rev != mc->hdr.rev)
-- 
2.11.0

Re: [PATCH 02/13] x86/microcode: Use own MSR accessors

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
> index 38711df3bcb5..fbecea6e46e2 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -5,20 +5,33 @@
>  #include <linux/earlycpio.h>
>  #include <linux/initrd.h>
>  
> -#define native_rdmsr(msr, val1, val2)			\
> +static inline unsigned long long __rdmsr(unsigned int msr)
> +{
> +	DECLARE_ARGS(val, low, high);
> +
> +	asm volatile("1: rdmsr\n"
> +		     "2:\n"
> +		     : EAX_EDX_RET(val, low, high) : "c" (msr));
> +	return EAX_EDX_VAL(val, low, high);
> +}
> +
> +#define microcode_rdmsr(msr, val1, val2)		\
>  do {							\
> -	u64 __val = native_read_msr((msr));		\
> +	u64 __val = __rdmsr((msr));			\
>  	(void)((val1) = (u32)__val);			\
>  	(void)((val2) = (u32)(__val >> 32));		\
>  } while (0)
>  
> -#define native_wrmsr(msr, low, high)			\
> -	native_write_msr(msr, low, high)
> +static inline void microcode_wrmsr(unsigned int msr, u64 val)
> +{
> +	u32 low, high;
> +
> +	low  = (u32)val;
> +	high = (u32)(val >> 32);
>  
> -#define native_wrmsrl(msr, val)				\
> -	native_write_msr((msr),				\
> -			 (u32)((u64)(val)),		\
> -			 (u32)((u64)(val) >> 32))
> +	asm volatile("wrmsr\n"
> +		     :: "c" (msr), "a"(low), "d" (high) : "memory");
> +}

msr.h already has:

wrmsr_notrace() so adding wrmsrl_notrace() to it should be a nobrainer.

Providing rdmsr_notrace() in msr.h is a natural extension of the existing
interfaces.

That would get rid of all the extra microcode specific MSR accessors which
are just yet another copy of stuff in msr.h.

Thanks,

	tglx

Re: [PATCH 02/13] x86/microcode: Use own MSR accessors

[Borislav Petkov]

On Tue, Jan 17, 2017 at 06:51:06PM +0100, Thomas Gleixner wrote:
> That would get rid of all the extra microcode specific MSR accessors which
> are just yet another copy of stuff in msr.h.

Well, I did think about reusing those but last time I did, they received
those tracepoints (apparently, we're sprinkling dumb tracepoints left
and right because good ol' staring at the code is just too hard) which
simply doesn't work on 32-bit before paging is enabled.

Then, __native_write_msr_notrace() has exception handling which doesn't
work before paging has been enabled on 32-bit - this is when the 32-bit
microcode update path happens due to paging hardware bugs in CPUs which
are fixed in microcode. So we must run that early on 32-bit.

So before someone decides to add more "functionality" to the generic MSR
accessors and break the microcode loader once more, I'd really really
prefer to have private accessors. They're small enough so shouldn't be
that much of a bloat.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 02/13] x86/microcode: Use own MSR accessors

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> On Tue, Jan 17, 2017 at 06:51:06PM +0100, Thomas Gleixner wrote:
> > That would get rid of all the extra microcode specific MSR accessors which
> > are just yet another copy of stuff in msr.h.
> 
> Well, I did think about reusing those but last time I did, they received
> those tracepoints (apparently, we're sprinkling dumb tracepoints left
> and right because good ol' staring at the code is just too hard) which
> simply doesn't work on 32-bit before paging is enabled.
>
> Then, __native_write_msr_notrace() has exception handling which doesn't
> work before paging has been enabled on 32-bit - this is when the 32-bit
> microcode update path happens due to paging hardware bugs in CPUs which
> are fixed in microcode. So we must run that early on 32-bit.

Well, the exception handling is irrelevant in that case. If the MSR access
succeeds then the exception handling is not invoked. If it fails then it
does not matter much whether you die from the MSR #GP or from the exception
handling attempt #GP.

> So before someone decides to add more "functionality" to the generic MSR
> accessors and break the microcode loader once more, I'd really really
> prefer to have private accessors. They're small enough so shouldn't be
> that much of a bloat.

I can understand that, but we better have real native functions in msr.h
which do exactly what they are supposed to do: read or write the MSR,
nothing else. Add a comment to those function which should be immutable and
threaten that offenders will be slapped with stinking trouts or frozen
sharks.

We really want to have a single place for all of this MSR stuff, because
following your argumentation will be a perfect precedence for more private
implementations.

Thanks,

	tglx

Re: [PATCH 02/13] x86/microcode: Use own MSR accessors

[Borislav Petkov]

On Tue, Jan 17, 2017 at 08:12:24PM +0100, Thomas Gleixner wrote:
> We really want to have a single place for all of this MSR stuff, because
> following your argumentation will be a perfect precedence for more private
> implementations.

Yes, you're so right. Ok, how about this untested thing. The warning
should be saucy enough :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Tue, 17 Jan 2017 23:29:10 +0100
Subject: [PATCH] x86/MSR: Carve out bare minimum accessors

Add __rdmsr() and __wrmsr() which *only* read and write an MSR with
exception handling. Those are going to be used in early code, like the
microcode loader, which cannot stomach tracing code piggybacking on the
MSR operation.

While at it, get rid of __native_write_msr_notrace().

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/apic.h |  2 +-
 arch/x86/include/asm/msr.h  | 51 +++++++++++++++++++++++++++------------------
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 0c5fbc68e82d..eff8e36aaf72 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -195,7 +195,7 @@ static inline void native_apic_msr_write(u32 reg, u32 v)
 
 static inline void native_apic_msr_eoi_write(u32 reg, u32 v)
 {
-	wrmsr_notrace(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
+	__wrmsr(APIC_BASE_MSR + (APIC_EOI >> 4), APIC_EOI_ACK, 0);
 }
 
 static inline u32 native_apic_msr_read(u32 reg)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index db0b90c3b03e..898dba2e2e2c 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -80,7 +80,14 @@ static inline void do_trace_read_msr(unsigned int msr, u64 val, int failed) {}
 static inline void do_trace_rdpmc(unsigned int msr, u64 val, int failed) {}
 #endif
 
-static inline unsigned long long native_read_msr(unsigned int msr)
+/*
+ * __rdmsr() and __wrmsr() are the two primitives which are the bare minimum MSR
+ * accessors and should not have any tracing or other functionality piggybacking
+ * on them - those are *purely* for accessing MSRs and nothing more. So don't even
+ * think of extending them - you will be slapped with a stinking trout or a frozen
+ * shark will reach you, wherever you are! You've been warned.
+ */
+static inline unsigned long long notrace __rdmsr(unsigned int msr)
 {
 	DECLARE_ARGS(val, low, high);
 
@@ -88,11 +95,30 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 		     "2:\n"
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_rdmsr_unsafe)
 		     : EAX_EDX_RET(val, low, high) : "c" (msr));
-	if (msr_tracepoint_active(__tracepoint_read_msr))
-		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), 0);
+
 	return EAX_EDX_VAL(val, low, high);
 }
 
+static inline void notrace __wrmsr(unsigned int msr, u32 low, u32 high)
+{
+	asm volatile("1: wrmsr\n"
+		     "2:\n"
+		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
+		     : : "c" (msr), "a"(low), "d" (high) : "memory");
+}
+
+static inline unsigned long long native_read_msr(unsigned int msr)
+{
+	unsigned long long val;
+
+	val = __rdmsr(msr);
+
+	if (msr_tracepoint_active(__tracepoint_read_msr))
+		do_trace_read_msr(msr, val, 0);
+
+	return val;
+}
+
 static inline unsigned long long native_read_msr_safe(unsigned int msr,
 						      int *err)
 {
@@ -116,29 +142,14 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 
 /* Can be uninlined because referenced by paravirt */
 static inline void notrace
-__native_write_msr_notrace(unsigned int msr, u32 low, u32 high)
-{
-	asm volatile("1: wrmsr\n"
-		     "2:\n"
-		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_wrmsr_unsafe)
-		     : : "c" (msr), "a"(low), "d" (high) : "memory");
-}
-
-/* Can be uninlined because referenced by paravirt */
-static inline void notrace
 native_write_msr(unsigned int msr, u32 low, u32 high)
 {
-	__native_write_msr_notrace(msr, low, high);
+	__wrmsr(msr, low, high);
+
 	if (msr_tracepoint_active(__tracepoint_write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
 }
 
-static inline void
-wrmsr_notrace(unsigned int msr, u32 low, u32 high)
-{
-	__native_write_msr_notrace(msr, low, high);
-}
-
 /* Can be uninlined because referenced by paravirt */
 static inline int notrace
 native_write_msr_safe(unsigned int msr, u32 low, u32 high)
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 02/13] x86/microcode: Use own MSR accessors

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> Date: Tue, 17 Jan 2017 23:29:10 +0100
> Subject: [PATCH] x86/MSR: Carve out bare minimum accessors
> 
> Add __rdmsr() and __wrmsr() which *only* read and write an MSR with
> exception handling. Those are going to be used in early code, like the
> microcode loader, which cannot stomach tracing code piggybacking on the
> MSR operation.
> 
> While at it, get rid of __native_write_msr_notrace().

Looks good.

[PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

No need to have it marked "inline" - let gcc decide. Also, shorten the
argument name and simplify while-test.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 3f89e6712afe..889fd61bc033 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -97,17 +97,16 @@ static size_t compute_container_size(u8 *data, u32 total_size)
 	return size;
 }
 
-static inline u16 find_equiv_id(struct equiv_cpu_entry *equiv_cpu_table,
-				unsigned int sig)
+static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 {
 	int i = 0;
 
-	if (!equiv_cpu_table)
+	if (!equiv_table)
 		return 0;
 
-	while (equiv_cpu_table[i].installed_cpu != 0) {
-		if (sig == equiv_cpu_table[i].installed_cpu)
-			return equiv_cpu_table[i].equiv_cpu;
+	while (equiv_table[i].installed_cpu) {
+		if (sig == equiv_table[i].installed_cpu)
+			return equiv_table[i].equiv_cpu;
 
 		i++;
 	}
-- 
2.11.0

Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> No need to have it marked "inline" - let gcc decide. Also, shorten the
> argument name and simplify while-test.
> 
> No functionality change.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kernel/cpu/microcode/amd.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 3f89e6712afe..889fd61bc033 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -97,17 +97,16 @@ static size_t compute_container_size(u8 *data, u32 total_size)
>  	return size;
>  }
>  
> -static inline u16 find_equiv_id(struct equiv_cpu_entry *equiv_cpu_table,
> -				unsigned int sig)
> +static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
>  {
>  	int i = 0;
>  
> -	if (!equiv_cpu_table)
> +	if (!equiv_table)
>  		return 0;
>  
> -	while (equiv_cpu_table[i].installed_cpu != 0) {
> -		if (sig == equiv_cpu_table[i].installed_cpu)
> -			return equiv_cpu_table[i].equiv_cpu;
> +	while (equiv_table[i].installed_cpu) {
> +		if (sig == equiv_table[i].installed_cpu)
> +			return equiv_table[i].equiv_cpu;
>  
>  		i++;

While you are at it, please rewrite that thing as a for loop, which is the
obvious correct thing:

	for (i = 0; equiv_table[i].installed_cpu; i++)

and even simpler:

	for (; equiv_table->installed_cpu; equiv_table++)
	
Hmm?

Thanks,

	tglx

Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()

[Borislav Petkov]

On Tue, Jan 17, 2017 at 06:54:48PM +0100, Thomas Gleixner wrote:
> and even simpler:
> 
> 	for (; equiv_table->installed_cpu; equiv_table++)

Very nice, thanks!

Here's v1.1:

---
From: Borislav Petkov <bp@suse.de>
Date: Sun, 18 Dec 2016 12:05:50 +0100
Subject: [PATCH -v1.1 03/13] x86/microcode/AMD: Clean up find_equiv_id()

No need to have it marked "inline" - let gcc decide. Also, shorten the
argument name and simplify while-test.

While at it, make it into a proper for-loop and simplify it even more,
as tglx suggests.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 3f89e6712afe..142fb9950e2a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -97,20 +97,12 @@ static size_t compute_container_size(u8 *data, u32 total_size)
 	return size;
 }
 
-static inline u16 find_equiv_id(struct equiv_cpu_entry *equiv_cpu_table,
-				unsigned int sig)
+static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 {
-	int i = 0;
-
-	if (!equiv_cpu_table)
-		return 0;
+	for (; equiv_table && equiv_table->installed_cpu; equiv_table++)
+		if (sig == equiv_table->installed_cpu)
+			return equiv_table->equiv_cpu;
 
-	while (equiv_cpu_table[i].installed_cpu != 0) {
-		if (sig == equiv_cpu_table[i].installed_cpu)
-			return equiv_cpu_table[i].equiv_cpu;
-
-		i++;
-	}
 	return 0;
 }
 
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> +	for (; equiv_table && equiv_table->installed_cpu; equiv_table++)
> +		if (sig == equiv_table->installed_cpu)
> +			return equiv_table->equiv_cpu;

This would be perfect if you just kept the braces around the for loop.

	for (; cond; incr)
		do_something();

parses perfectly fine as it matches the expectation of a single line statement
following the for().

	for (; cond; incr)
		if (othercond)
			do_something();

not so much because we expect a single line statement due to the lack of a
opening brace after the for()

	for (; cond; incr) {
		if (othercond)
			do_something();
	}

That's how it parses best. The opening brace after the for() tells us: here
comes a multiline statement. And the inner if (othercond) w/o the opening
brace tells: here comes a single line statement.

Reading code/patches very much depends on patterns and structuring. If they
are consistent the reading flow is undisturbed.

Thanks,

	tglx

Re: [PATCH 03/13] x86/microcode/AMD: Clean up find_equiv_id()

[Borislav Petkov]

On Tue, Jan 17, 2017 at 08:02:59PM +0100, Thomas Gleixner wrote:
> That's how it parses best. The opening brace after the for() tells us: here
> comes a multiline statement. And the inner if (othercond) w/o the opening
> brace tells: here comes a single line statement.
> 
> Reading code/patches very much depends on patterns and structuring. If they
> are consistent the reading flow is undisturbed.

Yeah, very true.

---
From: Borislav Petkov <bp@suse.de>
Date: Sun, 18 Dec 2016 12:05:50 +0100
Subject: [PATCH] x86/microcode/AMD: Clean up find_equiv_id()

No need to have it marked "inline" - let gcc decide. Also, shorten the
argument name and simplify while-test.

While at it, make it into a proper for-loop and simplify it even more,
as tglx suggests.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6a31e2691f3a..5c1509a38048 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -97,20 +97,13 @@ static size_t compute_container_size(u8 *data, u32 total_size)
 	return size;
 }
 
-static inline u16 find_equiv_id(struct equiv_cpu_entry *equiv_cpu_table,
-				unsigned int sig)
+static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 {
-	int i = 0;
-
-	if (!equiv_cpu_table)
-		return 0;
-
-	while (equiv_cpu_table[i].installed_cpu != 0) {
-		if (sig == equiv_cpu_table[i].installed_cpu)
-			return equiv_cpu_table[i].equiv_cpu;
-
-		i++;
+	for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
+		if (sig == equiv_table->installed_cpu)
+			return equiv_table->equiv_cpu;
 	}
+
 	return 0;
 }
 
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 04/13] x86/microcode/AMD: Shorten function parameter's name

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

The whole driver calls this "mc", do that here too.

No functionality change.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 889fd61bc033..ef36b613db62 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -193,15 +193,15 @@ find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)
 	return eq_id;
 }
 
-static int __apply_microcode_amd(struct microcode_amd *mc_amd)
+static int __apply_microcode_amd(struct microcode_amd *mc)
 {
 	u32 rev, dummy;
 
-	microcode_wrmsr(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
+	microcode_wrmsr(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc->hdr.data_code);
 
 	/* verify patch application was successful */
 	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-	if (rev != mc_amd->hdr.patch_id)
+	if (rev != mc->hdr.patch_id)
 		return -1;
 
 	return 0;
-- 
2.11.0

Re: [PATCH 04/13] x86/microcode/AMD: Shorten function parameter's name

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> The whole driver calls this "mc", do that here too.
> 
> No functionality change.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 05/13] x86/microcode/AMD: Extend the container struct

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Make it into a container descriptor which is being passed around and
stores important info like the matching container and the patch for the
current CPU.

Later patches will use this and thus get rid of a double container
parsing.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index ef36b613db62..7073588a2a09 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -42,9 +42,13 @@ static struct equiv_cpu_entry *equiv_cpu_table;
 
 /*
  * This points to the current valid container of microcode patches which we will
- * save from the initrd/builtin before jettisoning its contents.
+ * save from the initrd/builtin before jettisoning its contents. @mc is the
+ * microcode patch we found to match.
  */
-struct container {
+struct cont_desc {
+	struct microcode_amd *mc;
+	u32 psize;
+	u16 eq_id;
 	u8 *data;
 	size_t size;
 } cont;
@@ -119,9 +123,9 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
  * table or 0 if none found.
  */
 static u16
-find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)
+find_proper_container(u8 *ucode, size_t size, struct cont_desc *desc)
 {
-	struct container ret = { NULL, 0 };
+	struct cont_desc ret = { 0 };
 	u32 eax, ebx, ecx, edx;
 	struct equiv_cpu_entry *eq;
 	int offset, left;
@@ -164,7 +168,7 @@ find_proper_container(u8 *ucode, size_t size, struct container *ret_cont)
 			 */
 			left = ret.size - offset;
 
-			*ret_cont = ret;
+			*desc = ret;
 			return eq_id;
 		}
 
@@ -219,11 +223,11 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  * Returns true if container found (sets @ret_cont), false otherwise.
  */
 static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
-				      struct container *ret_cont)
+				      struct cont_desc *desc)
 {
 	u8 (*patch)[PATCH_MAX_SIZE];
 	u32 rev, *header, *new_rev;
-	struct container ret;
+	struct cont_desc ret;
 	int offset, left;
 	u16 eq_id = 0;
 	u8  *data;
@@ -276,8 +280,8 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
 		left   -= offset;
 	}
 
-	if (ret_cont)
-		*ret_cont = ret;
+	if (desc)
+		*desc = ret;
 
 	return true;
 }
-- 
2.11.0

Re: [PATCH 05/13] x86/microcode/AMD: Extend the container struct

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> -struct container {
> +struct cont_desc {
> +	struct microcode_amd *mc;
> +	u32 psize;
> +	u16 eq_id;
>  	u8 *data;
>  	size_t size;

While at it can you please structure the members in tabular fashion?

struct cont_desc {
	struct microcode_amd	*mc;
	u32			psize;
	...
	
Other than that:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 06/13] x86/microcode/AMD: Rework container parsing

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

It was pretty clumsy before and the whole work of parsing the microcode
containers was spread around the functions wrongly.

Clean it up so that there's a main scan_containers() function which
iterates over the microcode blob and picks apart the containers glued
together. For each container, it calls a parse_container() helper which
concentrates on one container only: sanity-checking, parsing, counting
microcode patches in there, etc.

It makes much more sense now and it is actually very readable. Oh, and
we luvz a diffstat removing more crap than adding.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 241 ++++++++++++++++--------------------
 1 file changed, 108 insertions(+), 133 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 7073588a2a09..1893f41c2d29 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -64,43 +64,6 @@ static u16 this_equiv_id;
 static const char
 ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
 
-static size_t compute_container_size(u8 *data, u32 total_size)
-{
-	size_t size = 0;
-	u32 *header = (u32 *)data;
-
-	if (header[0] != UCODE_MAGIC ||
-	    header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
-	    header[2] == 0)                            /* size */
-		return size;
-
-	size = header[2] + CONTAINER_HDR_SZ;
-	total_size -= size;
-	data += size;
-
-	while (total_size) {
-		u16 patch_size;
-
-		header = (u32 *)data;
-
-		if (header[0] != UCODE_UCODE_TYPE)
-			break;
-
-		/*
-		 * Sanity-check patch size.
-		 */
-		patch_size = header[1];
-		if (patch_size > PATCH_MAX_SIZE)
-			break;
-
-		size	   += patch_size + SECTION_HDR_SIZE;
-		data	   += patch_size + SECTION_HDR_SIZE;
-		total_size -= patch_size + SECTION_HDR_SIZE;
-	}
-
-	return size;
-}
-
 static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 {
 	int i = 0;
@@ -121,80 +84,109 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
  * This scans the ucode blob for the proper container as we can have multiple
  * containers glued together. Returns the equivalence ID from the equivalence
  * table or 0 if none found.
+ * Returns the amount of bytes consumed while scanning. @desc contains all the
+ * data we're going to use in later stages of the application.
  */
-static u16
-find_proper_container(u8 *ucode, size_t size, struct cont_desc *desc)
+static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 {
-	struct cont_desc ret = { 0 };
-	u32 eax, ebx, ecx, edx;
 	struct equiv_cpu_entry *eq;
-	int offset, left;
-	u16 eq_id = 0;
-	u32 *header;
-	u8 *data;
+	ssize_t orig_size = size;
+	u32 *hdr = (u32 *)ucode;
+	u32 eax, ebx, ecx, edx;
+	u16 eq_id;
+	u8 *buf;
 
-	data   = ucode;
-	left   = size;
-	header = (u32 *)data;
+	/* Am I looking at an equivalence table header? */
+	if (hdr[0] != UCODE_MAGIC ||
+	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
+	    hdr[2] == 0) {
+		desc->eq_id = 0;
+		return CONTAINER_HDR_SZ;
+	}
 
+	buf = ucode;
 
-	/* find equiv cpu table */
-	if (header[0] != UCODE_MAGIC ||
-	    header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
-	    header[2] == 0)                            /* size */
-		return eq_id;
+	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
 
-	eax = 0x00000001;
+	eax = 1;
 	ecx = 0;
 	native_cpuid(&eax, &ebx, &ecx, &edx);
 
-	while (left > 0) {
-		eq = (struct equiv_cpu_entry *)(data + CONTAINER_HDR_SZ);
+	/* Find the equivalence ID of our CPU in this table: */
+	eq_id = find_equiv_id(eq, eax);
 
-		ret.data = data;
+	buf  += hdr[2] + CONTAINER_HDR_SZ;
+	size -= hdr[2] + CONTAINER_HDR_SZ;
 
-		/* Advance past the container header */
-		offset = header[2] + CONTAINER_HDR_SZ;
-		data  += offset;
-		left  -= offset;
+	/*
+	 * Scan through the rest of the container to find where it ends. We do
+	 * some basic sanity-checking too.
+	 */
+	while (size > 0) {
+		struct microcode_amd *mc;
+		u32 patch_size;
 
-		eq_id = find_equiv_id(eq, eax);
-		if (eq_id) {
-			ret.size = compute_container_size(ret.data, left + offset);
+		hdr = (u32 *)buf;
 
-			/*
-			 * truncate how much we need to iterate over in the
-			 * ucode update loop below
-			 */
-			left = ret.size - offset;
+		if (hdr[0] != UCODE_UCODE_TYPE)
+			break;
 
-			*desc = ret;
-			return eq_id;
+		/* Sanity-check patch size. */
+		patch_size = hdr[1];
+		if (patch_size > PATCH_MAX_SIZE) {
+			/* Something corrupted the container, invalidate it. */
+			eq_id = 0;
+			break;
 		}
 
-		/*
-		 * support multiple container files appended together. if this
-		 * one does not have a matching equivalent cpu entry, we fast
-		 * forward to the next container file.
-		 */
-		while (left > 0) {
-			header = (u32 *)data;
+		/* Skip patch section header: */
+		buf  += SECTION_HDR_SIZE;
+		size -= SECTION_HDR_SIZE;
 
-			if (header[0] == UCODE_MAGIC &&
-			    header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
-				break;
-
-			offset = header[1] + SECTION_HDR_SIZE;
-			data  += offset;
-			left  -= offset;
+		mc = (struct microcode_amd *)buf;
+		if (eq_id == mc->hdr.processor_rev_id) {
+			desc->psize = patch_size;
+			desc->mc = mc;
 		}
 
-		/* mark where the next microcode container file starts */
-		offset    = data - (u8 *)ucode;
-		ucode     = data;
+		buf  += patch_size;
+		size -= patch_size;
+	}
+
+	/*
+	 * If we have found an eq_id, it means we're looking at the container
+	 * which has a patch for this CPU so return 0 to mean, @ucode already
+	 * points to it and it will be parsed later. Otherwise, we return the
+	 * size we scanned so that we can advance to the next container in the
+	 * buffer.
+	 */
+	if (eq_id) {
+		desc->eq_id = eq_id;
+		desc->data  = ucode;
+		desc->size  = orig_size - size;
+
+		return 0;
 	}
 
-	return eq_id;
+	return orig_size - size;
+}
+
+/*
+ * Scan the ucode blob for the proper container as we can have multiple
+ * containers glued together.
+ */
+static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
+{
+	ssize_t rem = size;
+
+	while (rem >= 0) {
+		ssize_t s = parse_container(ucode, rem, desc);
+		if (!s)
+			return;
+
+		ucode += s;
+		rem   -= s;
+	}
 }
 
 static int __apply_microcode_amd(struct microcode_amd *mc)
@@ -220,17 +212,16 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  * load_microcode_amd() to save equivalent cpu table and microcode patches in
  * kernel heap memory.
  *
- * Returns true if container found (sets @ret_cont), false otherwise.
+ * Returns true if container found (sets @desc), false otherwise.
  */
 static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
-				      struct cont_desc *desc)
+				      struct cont_desc *ret_desc)
 {
+	struct cont_desc desc = { 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
-	u32 rev, *header, *new_rev;
-	struct cont_desc ret;
-	int offset, left;
-	u16 eq_id = 0;
-	u8  *data;
+	struct microcode_amd *mc;
+	u32 rev, *new_rev;
+	bool ret = false;
 
 #ifdef CONFIG_X86_32
 	new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
@@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
 #endif
 
 	if (check_current_patch_level(&rev, true))
-		return false;
-
-	eq_id = find_proper_container(ucode, size, &ret);
-	if (!eq_id)
-		return false;
-
-	this_equiv_id = eq_id;
-	header = (u32 *)ret.data;
-
-	/* We're pointing to an equiv table, skip over it. */
-	data = ret.data +  header[2] + CONTAINER_HDR_SZ;
-	left = ret.size - (header[2] + CONTAINER_HDR_SZ);
-
-	while (left > 0) {
-		struct microcode_amd *mc;
+		return ret;
 
-		header = (u32 *)data;
-		if (header[0] != UCODE_UCODE_TYPE || /* type */
-		    header[1] == 0)                  /* size */
-			break;
+	scan_containers(ucode, size, &desc);
+	if (!desc.eq_id)
+		return ret;
 
-		mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE);
+	this_equiv_id = desc.eq_id;
 
-		if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) {
+	mc = desc.mc;
+	if (!mc)
+		return ret;
 
-			if (!__apply_microcode_amd(mc)) {
-				rev = mc->hdr.patch_id;
-				*new_rev = rev;
+	if (rev >= mc->hdr.patch_id)
+		return ret;
 
-				if (save_patch)
-					memcpy(patch, mc, min_t(u32, header[1], PATCH_MAX_SIZE));
-			}
-		}
+	if (!__apply_microcode_amd(mc)) {
+		*new_rev = mc->hdr.patch_id;
+		ret      = true;
 
-		offset  = header[1] + SECTION_HDR_SIZE;
-		data   += offset;
-		left   -= offset;
+		if (save_patch)
+			memcpy(patch, mc, min_t(u32, desc.psize, PATCH_MAX_SIZE));
 	}
 
-	if (desc)
-		*desc = ret;
+	if (ret_desc)
+		*ret_desc = desc;
 
-	return true;
+	return ret;
 }
 
 static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
@@ -402,6 +377,7 @@ void load_ucode_amd_ap(unsigned int family)
 		}
 
 		if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
+			cont.data = NULL;
 			cont.size = -1;
 			return;
 		}
@@ -440,7 +416,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 {
 	enum ucode_state ret;
 	int retval = 0;
-	u16 eq_id;
 
 	if (!cont.data) {
 		if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
@@ -456,8 +431,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 				return -EINVAL;
 			}
 
-			eq_id = find_proper_container(cp.data, cp.size, &cont);
-			if (!eq_id) {
+			scan_containers(cp.data, cp.size, &cont);
+			if (!cont.eq_id) {
 				cont.size = -1;
 				return -EINVAL;
 			}
-- 
2.11.0

Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
> +	/* Find the equivalence ID of our CPU in this table: */
> +	eq_id = find_equiv_id(eq, eax);

So here we figure out whether the blob claims to have a patch for that cpu.

> +	/*
> +	 * Scan through the rest of the container to find where it ends. We do
> +	 * some basic sanity-checking too.
> +	 */

Stupid question. Why do we need to walk through that blob if we already
know that it does not contain a patch for this cpu, i.e. eq_id == 0 ?

I assume it has to do with the multiple containers glued together in the
blob, but that should be mentioned in the comment.

> +	while (size > 0) {
> +		struct microcode_amd *mc;
> +		u32 patch_size;
>  
> -		eq_id = find_equiv_id(eq, eax);
> -		if (eq_id) {
> -			ret.size = compute_container_size(ret.data, left + offset);
> +		hdr = (u32 *)buf;
>  
> -			/*
> -			 * truncate how much we need to iterate over in the
> -			 * ucode update loop below
> -			 */
> -			left = ret.size - offset;
> +		if (hdr[0] != UCODE_UCODE_TYPE)
> +			break;
>  
> -			*desc = ret;
> -			return eq_id;
> +		/* Sanity-check patch size. */
> +		patch_size = hdr[1];
> +		if (patch_size > PATCH_MAX_SIZE) {
> +			/* Something corrupted the container, invalidate it. */
> +			eq_id = 0;
> +			break;
>  		}
>  
> -		/*
> -		 * support multiple container files appended together. if this
> -		 * one does not have a matching equivalent cpu entry, we fast
> -		 * forward to the next container file.
> -		 */
> -		while (left > 0) {
> -			header = (u32 *)data;
> +		/* Skip patch section header: */
> +		buf  += SECTION_HDR_SIZE;
> +		size -= SECTION_HDR_SIZE;
>  
> -			if (header[0] == UCODE_MAGIC &&
> -			    header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
> -				break;
> -
> -			offset = header[1] + SECTION_HDR_SIZE;
> -			data  += offset;
> -			left  -= offset;
> +		mc = (struct microcode_amd *)buf;
> +		if (eq_id == mc->hdr.processor_rev_id) {
> +			desc->psize = patch_size;
> +			desc->mc = mc;

So here you set patch_size and mc when the eq_id is matching. I assume we
continue the scan for the same reason as we do the scan for eq_id = 0, right?

>  		}
>  
> -		/* mark where the next microcode container file starts */
> -		offset    = data - (u8 *)ucode;
> -		ucode     = data;
> +		buf  += patch_size;
> +		size -= patch_size;
> +	}
> +
> +	/*
> +	 * If we have found an eq_id, it means we're looking at the container
> +	 * which has a patch for this CPU so return 0 to mean, @ucode already
> +	 * points to it and it will be parsed later. Otherwise, we return the
> +	 * size we scanned so that we can advance to the next container in the
> +	 * buffer.
> +	 */
> +	if (eq_id) {

Now this one is dangerous. If the blob is corrupted we might have exited
the loop above due to

> +		if (hdr[0] != UCODE_UCODE_TYPE)
> +			break;

before the eq_id matching happened. In that case we return success, but
desc->psize and desc->mc are not set. Not what you want, right?

> +		desc->eq_id = eq_id;
> +		desc->data  = ucode;
> +		desc->size  = orig_size - size;
> +
> +		return 0;
  
> @@ -241,49 +232,33 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
>  #endif
>  
>  	if (check_current_patch_level(&rev, true))
> -		return false;

So this becomes
> +		return ret;

which is not really better than the original code. I think we really should
only use the variable when there is something which can change between two
points, but that's my personal preference and up to you :)

> +	if (!desc.eq_id)
> +		return ret;
>  
> -		mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE);
> +	this_equiv_id = desc.eq_id;

Why are you storing the id when you don't have an idea whether the patch is
actually available and useable? There might be a proper reason, but w/o a
comment or access to the microcode crystalball it's hard to tell.

>  static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
> @@ -402,6 +377,7 @@ void load_ucode_amd_ap(unsigned int family)
>  		}
>  
>  		if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
> +			cont.data = NULL;
>  			cont.size = -1;

What's the point of fiddling with the local variable at all if we return
right away?

>  			return;
>  		}
> @@ -440,7 +416,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
>  {
>  	enum ucode_state ret;
>  	int retval = 0;
> -	u16 eq_id;
>  
>  	if (!cont.data) {
>  		if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
> @@ -456,8 +431,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
>  				return -EINVAL;
>  			}
>  
> -			eq_id = find_proper_container(cp.data, cp.size, &cont);
> -			if (!eq_id) {
> +			scan_containers(cp.data, cp.size, &cont);
> +			if (!cont.eq_id) {
>  				cont.size = -1;

Ditto. That might be fixed in a seperate patch because thats existing code.

>  				return -EINVAL;

Thanks,

	tglx

Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

[Borislav Petkov]

On Tue, Jan 17, 2017 at 09:29:05PM +0100, Thomas Gleixner wrote:
> Stupid question. Why do we need to walk through that blob if we already
> know that it does not contain a patch for this cpu, i.e. eq_id == 0 ?
> 
> I assume it has to do with the multiple containers glued together in the
> blob, but that should be mentioned in the comment.

It is mentioned a bit further down in the comment:

        /*
         * If we have found an eq_id, it means we're looking at the container
         * which has a patch for this CPU so return 0 to mean, @ucode already
         * points to it and it will be parsed later. Otherwise, we return the
         * size we scanned so that we can advance to the next container in the
         * buffer.
         */

> So here you set patch_size and mc when the eq_id is matching. I assume we
> continue the scan for the same reason as we do the scan for eq_id = 0, right?

Yes. Also, theoretically, some later container might hold an even newer
patch so we will update that pointer when we get there.

> Now this one is dangerous. If the blob is corrupted we might have exited
> the loop above due to
> 
> > +		if (hdr[0] != UCODE_UCODE_TYPE)
> > +			break;
> 
> before the eq_id matching happened. In that case we return success, but
> desc->psize and desc->mc are not set. Not what you want, right?

Ah, good catch. I guess I'll have to do here:

                        /* Something corrupted the container, invalidate it. */
                        eq_id = 0;
                        break;

too.

> So this becomes
> > +		return ret;
> 
> which is not really better than the original code. I think we really should
> only use the variable when there is something which can change between two
> points, but that's my personal preference and up to you :)

Ok, changed. This check goes away later anyway - it gets moved to the
BSP early path.

> Why are you storing the id when you don't have an idea whether the patch is
> actually available and useable? There might be a proper reason, but w/o a
> comment or access to the microcode crystalball it's hard to tell.

That is gone later, as you discovered yourself.

The thing is, it is hard to change things in this driver one at a time
without breaking it so I had to restrain myself.

Updated and untested version below. Testing is scheduled for the coming
days :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Sun, 18 Dec 2016 20:53:09 +0100
Subject: [PATCH] x86/microcode/AMD: Rework container parsing

It was pretty clumsy before and the whole work of parsing the microcode
containers was spread around the functions wrongly.

Clean it up so that there's a main scan_containers() function which
iterates over the microcode blob and picks apart the containers glued
together. For each container, it calls a parse_container() helper which
concentrates on one container only: sanity-checking, parsing, counting
microcode patches in there, etc.

It makes much more sense now and it is actually very readable. Oh, and
we luvz a diffstat removing more crap than adding.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 242 ++++++++++++++++--------------------
 1 file changed, 110 insertions(+), 132 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6935ceaab4fa..52dffc590ff0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -64,43 +64,6 @@ static u16 this_equiv_id;
 static const char
 ucode_path[] __maybe_unused = "kernel/x86/microcode/AuthenticAMD.bin";
 
-static size_t compute_container_size(u8 *data, u32 total_size)
-{
-	size_t size = 0;
-	u32 *header = (u32 *)data;
-
-	if (header[0] != UCODE_MAGIC ||
-	    header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
-	    header[2] == 0)                            /* size */
-		return size;
-
-	size = header[2] + CONTAINER_HDR_SZ;
-	total_size -= size;
-	data += size;
-
-	while (total_size) {
-		u16 patch_size;
-
-		header = (u32 *)data;
-
-		if (header[0] != UCODE_UCODE_TYPE)
-			break;
-
-		/*
-		 * Sanity-check patch size.
-		 */
-		patch_size = header[1];
-		if (patch_size > PATCH_MAX_SIZE)
-			break;
-
-		size	   += patch_size + SECTION_HDR_SIZE;
-		data	   += patch_size + SECTION_HDR_SIZE;
-		total_size -= patch_size + SECTION_HDR_SIZE;
-	}
-
-	return size;
-}
-
 static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
 {
 	for (; equiv_table && equiv_table->installed_cpu; equiv_table++) {
@@ -115,80 +78,112 @@ static u16 find_equiv_id(struct equiv_cpu_entry *equiv_table, u32 sig)
  * This scans the ucode blob for the proper container as we can have multiple
  * containers glued together. Returns the equivalence ID from the equivalence
  * table or 0 if none found.
+ * Returns the amount of bytes consumed while scanning. @desc contains all the
+ * data we're going to use in later stages of the application.
  */
-static u16
-find_proper_container(u8 *ucode, size_t size, struct cont_desc *desc)
+static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 {
-	struct cont_desc ret = { 0 };
-	u32 eax, ebx, ecx, edx;
 	struct equiv_cpu_entry *eq;
-	int offset, left;
-	u16 eq_id = 0;
-	u32 *header;
-	u8 *data;
+	ssize_t orig_size = size;
+	u32 *hdr = (u32 *)ucode;
+	u32 eax, ebx, ecx, edx;
+	u16 eq_id;
+	u8 *buf;
 
-	data   = ucode;
-	left   = size;
-	header = (u32 *)data;
+	/* Am I looking at an equivalence table header? */
+	if (hdr[0] != UCODE_MAGIC ||
+	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
+	    hdr[2] == 0) {
+		desc->eq_id = 0;
+		return CONTAINER_HDR_SZ;
+	}
 
+	buf = ucode;
 
-	/* find equiv cpu table */
-	if (header[0] != UCODE_MAGIC ||
-	    header[1] != UCODE_EQUIV_CPU_TABLE_TYPE || /* type */
-	    header[2] == 0)                            /* size */
-		return eq_id;
+	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
 
-	eax = 0x00000001;
+	eax = 1;
 	ecx = 0;
 	native_cpuid(&eax, &ebx, &ecx, &edx);
 
-	while (left > 0) {
-		eq = (struct equiv_cpu_entry *)(data + CONTAINER_HDR_SZ);
-
-		ret.data = data;
+	/* Find the equivalence ID of our CPU in this table: */
+	eq_id = find_equiv_id(eq, eax);
 
-		/* Advance past the container header */
-		offset = header[2] + CONTAINER_HDR_SZ;
-		data  += offset;
-		left  -= offset;
+	buf  += hdr[2] + CONTAINER_HDR_SZ;
+	size -= hdr[2] + CONTAINER_HDR_SZ;
 
-		eq_id = find_equiv_id(eq, eax);
-		if (eq_id) {
-			ret.size = compute_container_size(ret.data, left + offset);
+	/*
+	 * Scan through the rest of the container to find where it ends. We do
+	 * some basic sanity-checking too.
+	 */
+	while (size > 0) {
+		struct microcode_amd *mc;
+		u32 patch_size;
 
-			/*
-			 * truncate how much we need to iterate over in the
-			 * ucode update loop below
-			 */
-			left = ret.size - offset;
+		hdr = (u32 *)buf;
 
-			*desc = ret;
-			return eq_id;
+		if (hdr[0] != UCODE_UCODE_TYPE) {
+			/* Invalidate container. */
+			eq_id = 0;
+			break;
 		}
 
-		/*
-		 * support multiple container files appended together. if this
-		 * one does not have a matching equivalent cpu entry, we fast
-		 * forward to the next container file.
-		 */
-		while (left > 0) {
-			header = (u32 *)data;
+		/* Sanity-check patch size. */
+		patch_size = hdr[1];
+		if (patch_size > PATCH_MAX_SIZE) {
+			/* Something corrupted the container, invalidate it. */
+			eq_id = 0;
+			break;
+		}
 
-			if (header[0] == UCODE_MAGIC &&
-			    header[1] == UCODE_EQUIV_CPU_TABLE_TYPE)
-				break;
+		/* Skip patch section header: */
+		buf  += SECTION_HDR_SIZE;
+		size -= SECTION_HDR_SIZE;
 
-			offset = header[1] + SECTION_HDR_SIZE;
-			data  += offset;
-			left  -= offset;
+		mc = (struct microcode_amd *)buf;
+		if (eq_id == mc->hdr.processor_rev_id) {
+			desc->psize = patch_size;
+			desc->mc = mc;
 		}
 
-		/* mark where the next microcode container file starts */
-		offset    = data - (u8 *)ucode;
-		ucode     = data;
+		buf  += patch_size;
+		size -= patch_size;
+	}
+
+	/*
+	 * If we have found an eq_id, it means we're looking at the container
+	 * which has a patch for this CPU so return 0 to mean, @ucode already
+	 * points to it and it will be parsed later. Otherwise, we return the
+	 * size we scanned so that we can advance to the next container in the
+	 * buffer.
+	 */
+	if (eq_id) {
+		desc->eq_id = eq_id;
+		desc->data  = ucode;
+		desc->size  = orig_size - size;
+
+		return 0;
 	}
 
-	return eq_id;
+	return orig_size - size;
+}
+
+/*
+ * Scan the ucode blob for the proper container as we can have multiple
+ * containers glued together.
+ */
+static void scan_containers(u8 *ucode, size_t size, struct cont_desc *desc)
+{
+	ssize_t rem = size;
+
+	while (rem >= 0) {
+		ssize_t s = parse_container(ucode, rem, desc);
+		if (!s)
+			return;
+
+		ucode += s;
+		rem   -= s;
+	}
 }
 
 static int __apply_microcode_amd(struct microcode_amd *mc)
@@ -214,17 +209,16 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  * load_microcode_amd() to save equivalent cpu table and microcode patches in
  * kernel heap memory.
  *
- * Returns true if container found (sets @ret_cont), false otherwise.
+ * Returns true if container found (sets @desc), false otherwise.
  */
 static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
-				      struct cont_desc *desc)
+				      struct cont_desc *ret_desc)
 {
+	struct cont_desc desc = { 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
-	u32 rev, *header, *new_rev;
-	struct cont_desc ret;
-	int offset, left;
-	u16 eq_id = 0;
-	u8  *data;
+	struct microcode_amd *mc;
+	u32 rev, *new_rev;
+	bool ret = false;
 
 #ifdef CONFIG_X86_32
 	new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
@@ -237,47 +231,31 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
 	if (check_current_patch_level(&rev, true))
 		return false;
 
-	eq_id = find_proper_container(ucode, size, &ret);
-	if (!eq_id)
-		return false;
-
-	this_equiv_id = eq_id;
-	header = (u32 *)ret.data;
-
-	/* We're pointing to an equiv table, skip over it. */
-	data = ret.data +  header[2] + CONTAINER_HDR_SZ;
-	left = ret.size - (header[2] + CONTAINER_HDR_SZ);
-
-	while (left > 0) {
-		struct microcode_amd *mc;
-
-		header = (u32 *)data;
-		if (header[0] != UCODE_UCODE_TYPE || /* type */
-		    header[1] == 0)                  /* size */
-			break;
+	scan_containers(ucode, size, &desc);
+	if (!desc.eq_id)
+		return ret;
 
-		mc = (struct microcode_amd *)(data + SECTION_HDR_SIZE);
+	this_equiv_id = desc.eq_id;
 
-		if (eq_id == mc->hdr.processor_rev_id && rev < mc->hdr.patch_id) {
+	mc = desc.mc;
+	if (!mc)
+		return ret;
 
-			if (!__apply_microcode_amd(mc)) {
-				rev = mc->hdr.patch_id;
-				*new_rev = rev;
+	if (rev >= mc->hdr.patch_id)
+		return ret;
 
-				if (save_patch)
-					memcpy(patch, mc, min_t(u32, header[1], PATCH_MAX_SIZE));
-			}
-		}
+	if (!__apply_microcode_amd(mc)) {
+		*new_rev = mc->hdr.patch_id;
+		ret      = true;
 
-		offset  = header[1] + SECTION_HDR_SIZE;
-		data   += offset;
-		left   -= offset;
+		if (save_patch)
+			memcpy(patch, mc, min_t(u32, desc.psize, PATCH_MAX_SIZE));
 	}
 
-	if (desc)
-		*desc = ret;
+	if (ret_desc)
+		*ret_desc = desc;
 
-	return true;
+	return ret;
 }
 
 static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
@@ -396,6 +374,7 @@ void load_ucode_amd_ap(unsigned int family)
 		}
 
 		if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
+			cont.data = NULL;
 			cont.size = -1;
 			return;
 		}
@@ -434,7 +413,6 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 {
 	enum ucode_state ret;
 	int retval = 0;
-	u16 eq_id;
 
 	if (!cont.data) {
 		if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
@@ -450,8 +428,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 				return -EINVAL;
 			}
 
-			eq_id = find_proper_container(cp.data, cp.size, &cont);
-			if (!eq_id) {
+			scan_containers(cp.data, cp.size, &cont);
+			if (!cont.eq_id) {
 				cont.size = -1;
 				return -EINVAL;
 			}
-- 
2.11.0


-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

[Borislav Petkov]

On Wed, Jan 18, 2017 at 12:31:24AM +0100, Borislav Petkov wrote:
> Ah, good catch. I guess I'll have to do here:
> 
>                         /* Something corrupted the container, invalidate it. */
>                         eq_id = 0;
>                         break;
> 
> too.

Ok, I should've done the below from the beginning. I should be looking
at desc->mc which says whether we have found a patch or not. And then
prepare retvals accordingly.

The equivalence id (eq_id) is then used to signal that fact.

And in a future patch, I'll check desc->mc after that function returns
and not eq_id. (I had to do eq_id because of the global this_equiv_id
but that's gone now too).

        /*
         * Scan through the rest of the container to find where it ends. We do
         * some basic sanity-checking too.
         */
        while (size > 0) {
                struct microcode_amd *mc;
                u32 patch_size;

                hdr = (u32 *)buf;

                if (hdr[0] != UCODE_UCODE_TYPE)
                        break;

                /* Sanity-check patch size. */
                patch_size = hdr[1];
                if (patch_size > PATCH_MAX_SIZE)
                        break;

                /* Skip patch section header: */
                buf  += SECTION_HDR_SIZE;
                size -= SECTION_HDR_SIZE;

                mc = (struct microcode_amd *)buf;
                if (eq_id == mc->hdr.processor_rev_id) {
                        desc->psize = patch_size;
                        desc->mc = mc;
                }

                buf  += patch_size;
                size -= patch_size;
        }

        /*
         * If we have found a patch (desc->mc), it means we're looking at the
         * container which has a patch for this CPU so return 0 to mean, @ucode
         * already points to the proper container. Otherwise, we return the size
         * we scanned so that we can advance to the next container in the
         * buffer.
         */
        if (desc->mc) {
                desc->eq_id = eq_id;
                desc->data  = ucode;
                desc->size  = orig_size - size;

                return 0;
        }

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH 06/13] x86/microcode/AMD: Rework container parsing

[Borislav Petkov]

On Wed, Jan 18, 2017 at 03:44:46PM +0100, Borislav Petkov wrote:
> And in a future patch, I'll check desc->mc after that function returns
> and not eq_id. (I had to do eq_id because of the global this_equiv_id
> but that's gone now too).

... which is pretty nice, I can get rid of eq_id now as it is private to
the container scanning code now and the callers shouldn't care. Cool.

Now on to test whether that actually makes sense :-)

---
From: Borislav Petkov <bp@suse.de>
Date: Wed, 18 Jan 2017 15:55:26 +0100
Subject: [PATCH] x86/microcode/AMD: Remove struct cont_desc.eq_id

The equivalence ID was needed outside of the container scanning logic
but now, after this has been cleaned up, not anymore. Now, cont_desc.mc
is used to denote whether the container we're looking at has the proper
microcode patch for this CPU or not.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 5e1b57747c2f..7889ae492af0 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -49,7 +49,6 @@ struct cont_desc {
 	struct microcode_amd *mc;
 	u32		     cpuid_1_eax;
 	u32		     psize;
-	u16		     eq_id;
 	u8		     *data;
 	size_t		     size;
 };
@@ -92,10 +91,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	/* Am I looking at an equivalence table header? */
 	if (hdr[0] != UCODE_MAGIC ||
 	    hdr[1] != UCODE_EQUIV_CPU_TABLE_TYPE ||
-	    hdr[2] == 0) {
-		desc->eq_id = 0;
+	    hdr[2] == 0)
 		return CONTAINER_HDR_SZ;
-	}
 
 	buf = ucode;
 
@@ -147,9 +144,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	 * buffer.
 	 */
 	if (desc->mc) {
-		desc->eq_id = eq_id;
-		desc->data  = ucode;
-		desc->size  = orig_size - size;
+		desc->data = ucode;
+		desc->size = orig_size - size;
 
 		return 0;
 	}
@@ -220,8 +216,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_p
 	desc.cpuid_1_eax = cpuid_1_eax;
 
 	scan_containers(ucode, size, &desc);
-	if (!desc.eq_id)
-		return ret;
 
 	mc = desc.mc;
 	if (!mc)
@@ -341,7 +335,7 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 	desc.cpuid_1_eax = cpuid_1_eax;
 
 	scan_containers(cp.data, cp.size, &desc);
-	if (!desc.eq_id)
+	if (!desc.mc)
 		return -EINVAL;
 
 	ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax),
-- 
2.11.0

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH 07/13] x86/microcode: Decrease CPUID use

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Get CPUID(1).EAX value once per CPU and propagate value into the callers
instead of conveniently calling it every time.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c  | 52 +++++++++++++++++-------------------
 arch/x86/kernel/cpu/microcode/core.c | 38 ++++++++++----------------
 2 files changed, 38 insertions(+), 52 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 1893f41c2d29..35c01243dec6 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -47,6 +47,7 @@ static struct equiv_cpu_entry *equiv_cpu_table;
  */
 struct cont_desc {
 	struct microcode_amd *mc;
+	u32 cpuid_1_eax;
 	u32 psize;
 	u16 eq_id;
 	u8 *data;
@@ -92,7 +93,6 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 	struct equiv_cpu_entry *eq;
 	ssize_t orig_size = size;
 	u32 *hdr = (u32 *)ucode;
-	u32 eax, ebx, ecx, edx;
 	u16 eq_id;
 	u8 *buf;
 
@@ -108,12 +108,8 @@ static ssize_t parse_container(u8 *ucode, ssize_t size, struct cont_desc *desc)
 
 	eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
 
-	eax = 1;
-	ecx = 0;
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-
 	/* Find the equivalence ID of our CPU in this table: */
-	eq_id = find_equiv_id(eq, eax);
+	eq_id = find_equiv_id(eq, desc->cpuid_1_eax);
 
 	buf  += hdr[2] + CONTAINER_HDR_SZ;
 	size -= hdr[2] + CONTAINER_HDR_SZ;
@@ -214,8 +210,9 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  *
  * Returns true if container found (sets @desc), false otherwise.
  */
-static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
-				      struct cont_desc *ret_desc)
+static bool
+apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
+			  bool save_patch, struct cont_desc *ret_desc)
 {
 	struct cont_desc desc = { 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
@@ -234,6 +231,8 @@ static bool apply_microcode_early_amd(void *ucode, size_t size, bool save_patch,
 	if (check_current_patch_level(&rev, true))
 		return ret;
 
+	desc.cpuid_1_eax = cpuid_1_eax;
+
 	scan_containers(ucode, size, &desc);
 	if (!desc.eq_id)
 		return ret;
@@ -276,10 +275,9 @@ static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
 #endif
 }
 
-void __init load_ucode_amd_bsp(unsigned int family)
+void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 {
 	struct ucode_cpu_info *uci;
-	u32 eax, ebx, ecx, edx;
 	struct cpio_data cp;
 	const char *path;
 	bool use_pa;
@@ -294,19 +292,16 @@ void __init load_ucode_amd_bsp(unsigned int family)
 		use_pa	= false;
 	}
 
-	if (!get_builtin_microcode(&cp, family))
+	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
 		cp = find_microcode_in_initrd(path, use_pa);
 
 	if (!(cp.data && cp.size))
 		return;
 
-	/* Get BSP's CPUID.EAX(1), needed in load_microcode_amd() */
-	eax = 1;
-	ecx = 0;
-	native_cpuid(&eax, &ebx, &ecx, &edx);
-	uci->cpu_sig.sig = eax;
+	/* Needed in load_microcode_amd() */
+	uci->cpu_sig.sig = cpuid_1_eax;
 
-	apply_microcode_early_amd(cp.data, cp.size, true, NULL);
+	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL);
 }
 
 #ifdef CONFIG_X86_32
@@ -317,7 +312,7 @@ void __init load_ucode_amd_bsp(unsigned int family)
  * In save_microcode_in_initrd_amd() BSP's patch is copied to amd_ucode_patch,
  * which is used upon resume from suspend.
  */
-void load_ucode_amd_ap(unsigned int family)
+void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 {
 	struct microcode_amd *mc;
 	struct cpio_data cp;
@@ -328,7 +323,7 @@ void load_ucode_amd_ap(unsigned int family)
 		return;
 	}
 
-	if (!get_builtin_microcode(&cp, family))
+	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
 		cp = find_microcode_in_initrd((const char *)__pa_nodebug(ucode_path), true);
 
 	if (!(cp.data && cp.size))
@@ -338,14 +333,14 @@ void load_ucode_amd_ap(unsigned int family)
 	 * This would set amd_ucode_patch above so that the following APs can
 	 * use it directly instead of going down this path again.
 	 */
-	apply_microcode_early_amd(cp.data, cp.size, true, NULL);
+	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL);
 }
 #else
-void load_ucode_amd_ap(unsigned int family)
+void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 {
 	struct equiv_cpu_entry *eq;
 	struct microcode_amd *mc;
-	u32 rev, eax;
+	u32 rev;
 	u16 eq_id;
 
 	/* 64-bit runs with paging enabled, thus early==false. */
@@ -360,7 +355,7 @@ void load_ucode_amd_ap(unsigned int family)
 			return;
 
 reget:
-		if (!get_builtin_microcode(&cp, family)) {
+		if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax))) {
 #ifdef CONFIG_BLK_DEV_INITRD
 			cp = find_cpio_data(ucode_path, (void *)initrd_start,
 					    initrd_end - initrd_start, NULL);
@@ -376,17 +371,16 @@ void load_ucode_amd_ap(unsigned int family)
 			}
 		}
 
-		if (!apply_microcode_early_amd(cp.data, cp.size, false, &cont)) {
+		if (!apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, false, &cont)) {
 			cont.data = NULL;
 			cont.size = -1;
 			return;
 		}
 	}
 
-	eax = cpuid_eax(0x00000001);
 	eq  = (struct equiv_cpu_entry *)(cont.data + CONTAINER_HDR_SZ);
 
-	eq_id = find_equiv_id(eq, eax);
+	eq_id = find_equiv_id(eq, cpuid_1_eax);
 	if (!eq_id)
 		return;
 
@@ -412,7 +406,7 @@ void load_ucode_amd_ap(unsigned int family)
 static enum ucode_state
 load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size);
 
-int __init save_microcode_in_initrd_amd(unsigned int fam)
+int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 {
 	enum ucode_state ret;
 	int retval = 0;
@@ -431,6 +425,8 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 				return -EINVAL;
 			}
 
+			cont.cpuid_1_eax = cpuid_1_eax;
+
 			scan_containers(cp.data, cp.size, &cont);
 			if (!cont.eq_id) {
 				cont.size = -1;
@@ -441,7 +437,7 @@ int __init save_microcode_in_initrd_amd(unsigned int fam)
 			return -EINVAL;
 	}
 
-	ret = load_microcode_amd(smp_processor_id(), fam, cont.data, cont.size);
+	ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax), cont.data, cont.size);
 	if (ret != UCODE_OK)
 		retval = -EINVAL;
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 2af69d27da62..437996c9be67 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -64,10 +64,6 @@ static DEFINE_MUTEX(microcode_mutex);
 
 struct ucode_cpu_info		ucode_cpu_info[NR_CPUS];
 
-/*
- * Operations that are run on a target cpu:
- */
-
 struct cpu_info_ctx {
 	struct cpu_signature	*cpu_sig;
 	int			err;
@@ -76,7 +72,6 @@ struct cpu_info_ctx {
 static bool __init check_loader_disabled_bsp(void)
 {
 	static const char *__dis_opt_str = "dis_ucode_ldr";
-	u32 a, b, c, d;
 
 #ifdef CONFIG_X86_32
 	const char *cmdline = (const char *)__pa_nodebug(boot_command_line);
@@ -92,16 +87,12 @@ static bool __init check_loader_disabled_bsp(void)
 	if (!have_cpuid_p())
 		return *res;
 
-	a = 1;
-	c = 0;
-	native_cpuid(&a, &b, &c, &d);
-
 	/*
 	 * CPUID(1).ECX[31]: reserved for hypervisor use. This is still not
 	 * completely accurate as xen pv guests don't see that CPUID bit set but
 	 * that's good enough as they don't land on the BSP path anyway.
 	 */
-	if (c & BIT(31))
+	if (native_cpuid_ecx(1) & BIT(31))
 		return *res;
 
 	if (cmdline_find_option_bool(cmdline, option) <= 0)
@@ -131,23 +122,22 @@ bool get_builtin_firmware(struct cpio_data *cd, const char *name)
 
 void __init load_ucode_bsp(void)
 {
-	int vendor;
-	unsigned int family;
+	unsigned int vendor, cpuid_1_eax;
 
 	if (check_loader_disabled_bsp())
 		return;
 
-	vendor = x86_cpuid_vendor();
-	family = x86_cpuid_family();
+	vendor	    = x86_cpuid_vendor();
+	cpuid_1_eax = native_cpuid_eax(1);
 
 	switch (vendor) {
 	case X86_VENDOR_INTEL:
-		if (family >= 6)
+		if (x86_family(cpuid_1_eax) >= 6)
 			load_ucode_intel_bsp();
 		break;
 	case X86_VENDOR_AMD:
-		if (family >= 0x10)
-			load_ucode_amd_bsp(family);
+		if (x86_family(cpuid_1_eax) >= 0x10)
+			load_ucode_amd_bsp(cpuid_1_eax);
 		break;
 	default:
 		break;
@@ -165,22 +155,22 @@ static bool check_loader_disabled_ap(void)
 
 void load_ucode_ap(void)
 {
-	int vendor, family;
+	unsigned int vendor, cpuid_1_eax;
 
 	if (check_loader_disabled_ap())
 		return;
 
-	vendor = x86_cpuid_vendor();
-	family = x86_cpuid_family();
+	vendor	    = x86_cpuid_vendor();
+	cpuid_1_eax = native_cpuid_eax(1);
 
 	switch (vendor) {
 	case X86_VENDOR_INTEL:
-		if (family >= 6)
+		if (x86_family(cpuid_1_eax) >= 6)
 			load_ucode_intel_ap();
 		break;
 	case X86_VENDOR_AMD:
-		if (family >= 0x10)
-			load_ucode_amd_ap(family);
+		if (x86_family(cpuid_1_eax) >= 0x10)
+			load_ucode_amd_ap(cpuid_1_eax);
 		break;
 	default:
 		break;
@@ -198,7 +188,7 @@ static int __init save_microcode_in_initrd(void)
 		break;
 	case X86_VENDOR_AMD:
 		if (c->x86 >= 0x10)
-			return save_microcode_in_initrd_amd(c->x86);
+			return save_microcode_in_initrd_amd(cpuid_eax(1));
 		break;
 	default:
 		break;
-- 
2.11.0

Re: [PATCH 07/13] x86/microcode: Decrease CPUID use

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Get CPUID(1).EAX value once per CPU and propagate value into the callers
> instead of conveniently calling it every time.
> 

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 08/13] x86/microcode/AMD: Get rid of global this_equiv_id

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

We have a container which we update/prepare each time before applying a
patch.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 35c01243dec6..e5cb7e73cb3b 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -56,7 +56,6 @@ struct cont_desc {
 
 static u32 ucode_new_rev;
 static u8 amd_ucode_patch[PATCH_MAX_SIZE];
-static u16 this_equiv_id;
 
 /*
  * Microcode patch container file is prepended to the initrd in cpio
@@ -237,8 +236,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
 	if (!desc.eq_id)
 		return ret;
 
-	this_equiv_id = desc.eq_id;
-
 	mc = desc.mc;
 	if (!mc)
 		return ret;
@@ -384,7 +381,7 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 	if (!eq_id)
 		return;
 
-	if (eq_id == this_equiv_id) {
+	if (eq_id == cont.eq_id) {
 		mc = (struct microcode_amd *)amd_ucode_patch;
 
 		if (mc && rev < mc->hdr.patch_id) {
-- 
2.11.0

Re: [PATCH 08/13] x86/microcode/AMD: Get rid of global this_equiv_id

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> We have a container which we update/prepare each time before applying a
> patch.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Ok, this addresses the this_equiv_id storage thing I asked before :)

[PATCH 09/13] x86/microcode/AMD: Use find_microcode_in_initrd()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Use the generic helper instead of semi-open-coding the procedure.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index e5cb7e73cb3b..340d636512c9 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -346,17 +346,15 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 
 	/* First AP hasn't cached it yet, go through the blob. */
 	if (!cont.data) {
-		struct cpio_data cp = { NULL, 0, "" };
+		struct cpio_data cp;
 
 		if (cont.size == -1)
 			return;
 
 reget:
 		if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax))) {
-#ifdef CONFIG_BLK_DEV_INITRD
-			cp = find_cpio_data(ucode_path, (void *)initrd_start,
-					    initrd_end - initrd_start, NULL);
-#endif
+			cp = find_microcode_in_initrd(ucode_path, false);
+
 			if (!(cp.data && cp.size)) {
 				/*
 				 * Mark it so that other APs do not scan again
@@ -410,13 +408,9 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 
 	if (!cont.data) {
 		if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
-			struct cpio_data cp = { NULL, 0, "" };
-
-#ifdef CONFIG_BLK_DEV_INITRD
-			cp = find_cpio_data(ucode_path, (void *)initrd_start,
-					    initrd_end - initrd_start, NULL);
-#endif
+			struct cpio_data cp;
 
+			cp = find_microcode_in_initrd(ucode_path, false);
 			if (!(cp.data && cp.size)) {
 				cont.size = -1;
 				return -EINVAL;
-- 
2.11.0

Re: [PATCH 09/13] x86/microcode/AMD: Use find_microcode_in_initrd()

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Use the generic helper instead of semi-open-coding the procedure.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 10/13] x86/microcode/AMD: Check patch level only on the BSP

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Check final patch levels for AMD only on the BSP. This way, we decide
early and only once whether to continue loading or to leave the loader
disabled on such systems.

Simplify a lot.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/include/asm/microcode_amd.h |  2 -
 arch/x86/kernel/cpu/microcode/amd.c  | 77 ++++++------------------------------
 arch/x86/kernel/cpu/microcode/core.c | 51 +++++++++++++++++++++---
 3 files changed, 56 insertions(+), 74 deletions(-)

diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
index 3e3e20be829a..3d57009e168b 100644
--- a/arch/x86/include/asm/microcode_amd.h
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -54,6 +54,4 @@ static inline int __init
 save_microcode_in_initrd_amd(unsigned int family) { return -EINVAL; }
 void reload_ucode_amd(void) {}
 #endif
-
-extern bool check_current_patch_level(u32 *rev, bool early);
 #endif /* _ASM_X86_MICROCODE_AMD_H */
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 340d636512c9..782e01311e4e 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -216,7 +216,7 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
 	struct cont_desc desc = { 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
 	struct microcode_amd *mc;
-	u32 rev, *new_rev;
+	u32 rev, dummy, *new_rev;
 	bool ret = false;
 
 #ifdef CONFIG_X86_32
@@ -227,8 +227,7 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
 	patch	= &amd_ucode_patch;
 #endif
 
-	if (check_current_patch_level(&rev, true))
-		return ret;
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
 	desc.cpuid_1_eax = cpuid_1_eax;
 
@@ -337,13 +336,8 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 {
 	struct equiv_cpu_entry *eq;
 	struct microcode_amd *mc;
-	u32 rev;
 	u16 eq_id;
 
-	/* 64-bit runs with paging enabled, thus early==false. */
-	if (check_current_patch_level(&rev, false))
-		return;
-
 	/* First AP hasn't cached it yet, go through the blob. */
 	if (!cont.data) {
 		struct cpio_data cp;
@@ -380,6 +374,10 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 		return;
 
 	if (eq_id == cont.eq_id) {
+		u32 rev, dummy;
+
+		microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
+
 		mc = (struct microcode_amd *)amd_ucode_patch;
 
 		if (mc && rev < mc->hdr.patch_id) {
@@ -445,19 +443,14 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 void reload_ucode_amd(void)
 {
 	struct microcode_amd *mc;
-	u32 rev;
-
-	/*
-	 * early==false because this is a syscore ->resume path and by
-	 * that time paging is long enabled.
-	 */
-	if (check_current_patch_level(&rev, false))
-		return;
+	u32 rev, dummy;
 
 	mc = (struct microcode_amd *)amd_ucode_patch;
 	if (!mc)
 		return;
 
+	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
+
 	if (rev < mc->hdr.patch_id) {
 		if (!__apply_microcode_amd(mc)) {
 			ucode_new_rev = mc->hdr.patch_id;
@@ -595,60 +588,13 @@ static unsigned int verify_patch_size(u8 family, u32 patch_size,
 	return patch_size;
 }
 
-/*
- * Those patch levels cannot be updated to newer ones and thus should be final.
- */
-static u32 final_levels[] = {
-	0x01000098,
-	0x0100009f,
-	0x010000af,
-	0, /* T-101 terminator */
-};
-
-/*
- * Check the current patch level on this CPU.
- *
- * @rev: Use it to return the patch level. It is set to 0 in the case of
- * error.
- *
- * Returns:
- *  - true: if update should stop
- *  - false: otherwise
- */
-bool check_current_patch_level(u32 *rev, bool early)
-{
-	u32 lvl, dummy, i;
-	bool ret = false;
-	u32 *levels;
-
-	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
-
-	if (IS_ENABLED(CONFIG_X86_32) && early)
-		levels = (u32 *)__pa_nodebug(&final_levels);
-	else
-		levels = final_levels;
-
-	for (i = 0; levels[i]; i++) {
-		if (lvl == levels[i]) {
-			lvl = 0;
-			ret = true;
-			break;
-		}
-	}
-
-	if (rev)
-		*rev = lvl;
-
-	return ret;
-}
-
 static int apply_microcode_amd(int cpu)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct microcode_amd *mc_amd;
 	struct ucode_cpu_info *uci;
 	struct ucode_patch *p;
-	u32 rev;
+	u32 rev, dummy;
 
 	BUG_ON(raw_smp_processor_id() != cpu);
 
@@ -661,8 +607,7 @@ static int apply_microcode_amd(int cpu)
 	mc_amd  = p->data;
 	uci->mc = p->data;
 
-	if (check_current_patch_level(&rev, false))
-		return -1;
+	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
 	/* need to apply patch? */
 	if (rev >= mc_amd->hdr.patch_id) {
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 437996c9be67..8650b58b4564 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -69,6 +69,42 @@ struct cpu_info_ctx {
 	int			err;
 };
 
+/*
+ * Those patch levels cannot be updated to newer ones and thus should be final.
+ */
+static u32 final_levels[] = {
+	0x01000098,
+	0x0100009f,
+	0x010000af,
+	0, /* T-101 terminator */
+};
+
+/*
+ * Check the current patch level on this CPU.
+ *
+ * Returns:
+ *  - true: if update should stop
+ *  - false: otherwise
+ */
+static bool amd_check_current_patch_level(void)
+{
+	u32 lvl, dummy, i;
+	u32 *levels;
+
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, lvl, dummy);
+
+	if (IS_ENABLED(CONFIG_X86_32))
+		levels = (u32 *)__pa_nodebug(&final_levels);
+	else
+		levels = final_levels;
+
+	for (i = 0; levels[i]; i++) {
+		if (lvl == levels[i])
+			return true;
+	}
+	return false;
+}
+
 static bool __init check_loader_disabled_bsp(void)
 {
 	static const char *__dis_opt_str = "dis_ucode_ldr";
@@ -95,6 +131,11 @@ static bool __init check_loader_disabled_bsp(void)
 	if (native_cpuid_ecx(1) & BIT(31))
 		return *res;
 
+	if (x86_cpuid_vendor() == X86_VENDOR_AMD) {
+		if (amd_check_current_patch_level())
+			return *res;
+	}
+
 	if (cmdline_find_option_bool(cmdline, option) <= 0)
 		*res = false;
 
@@ -122,15 +163,14 @@ bool get_builtin_firmware(struct cpio_data *cd, const char *name)
 
 void __init load_ucode_bsp(void)
 {
-	unsigned int vendor, cpuid_1_eax;
+	unsigned int cpuid_1_eax;
 
 	if (check_loader_disabled_bsp())
 		return;
 
-	vendor	    = x86_cpuid_vendor();
 	cpuid_1_eax = native_cpuid_eax(1);
 
-	switch (vendor) {
+	switch (x86_cpuid_vendor()) {
 	case X86_VENDOR_INTEL:
 		if (x86_family(cpuid_1_eax) >= 6)
 			load_ucode_intel_bsp();
@@ -155,15 +195,14 @@ static bool check_loader_disabled_ap(void)
 
 void load_ucode_ap(void)
 {
-	unsigned int vendor, cpuid_1_eax;
+	unsigned int cpuid_1_eax;
 
 	if (check_loader_disabled_ap())
 		return;
 
-	vendor	    = x86_cpuid_vendor();
 	cpuid_1_eax = native_cpuid_eax(1);
 
-	switch (vendor) {
+	switch (x86_cpuid_vendor()) {
 	case X86_VENDOR_INTEL:
 		if (x86_family(cpuid_1_eax) >= 6)
 			load_ucode_intel_ap();
-- 
2.11.0

Re: [PATCH 10/13] x86/microcode/AMD: Check patch level only on the BSP

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> Check final patch levels for AMD only on the BSP. This way, we decide
> early and only once whether to continue loading or to leave the loader
> disabled on such systems.
> 
> Simplify a lot.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
>  void __init load_ucode_bsp(void)
>  {
> -	unsigned int vendor, cpuid_1_eax;
> +	unsigned int cpuid_1_eax;
>  
>  	if (check_loader_disabled_bsp())
>  		return;
>  
> -	vendor	    = x86_cpuid_vendor();
>  	cpuid_1_eax = native_cpuid_eax(1);
>  
> -	switch (vendor) {
> +	switch (x86_cpuid_vendor()) {
>  	case X86_VENDOR_INTEL:
>  		if (x86_family(cpuid_1_eax) >= 6)
>  			load_ucode_intel_bsp();
> @@ -155,15 +195,14 @@ static bool check_loader_disabled_ap(void)
>  
>  void load_ucode_ap(void)
>  {
> -	unsigned int vendor, cpuid_1_eax;
> +	unsigned int cpuid_1_eax;
>  
>  	if (check_loader_disabled_ap())
>  		return;
>  
> -	vendor	    = x86_cpuid_vendor();
>  	cpuid_1_eax = native_cpuid_eax(1);
>  
> -	switch (vendor) {
> +	switch (x86_cpuid_vendor()) {
>  	case X86_VENDOR_INTEL:
>  		if (x86_family(cpuid_1_eax) >= 6)
>  			load_ucode_intel_ap();

These two hunks look unrelated to $subject. Please seperate them out.

Thanks,

	tglx

[PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Use a version for both bitness by adding a helper which does the actual
container finding and parsing which can be used on any CPU - BSP or AP.
Streamlines the paths more.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 84 ++++++++++++++-----------------------
 1 file changed, 31 insertions(+), 53 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 782e01311e4e..f1a61f181c9a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -271,7 +271,7 @@ static bool get_builtin_microcode(struct cpio_data *cp, unsigned int family)
 #endif
 }
 
-void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
+void __load_ucode_amd(unsigned int cpuid_1_eax, struct cpio_data *ret)
 {
 	struct ucode_cpu_info *uci;
 	struct cpio_data cp;
@@ -291,95 +291,74 @@ void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
 		cp = find_microcode_in_initrd(path, use_pa);
 
-	if (!(cp.data && cp.size))
-		return;
-
 	/* Needed in load_microcode_amd() */
 	uci->cpu_sig.sig = cpuid_1_eax;
 
-	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL);
+	*ret = cp;
 }
 
-#ifdef CONFIG_X86_32
-/*
- * On 32-bit, since AP's early load occurs before paging is turned on, we
- * cannot traverse cpu_equiv_table and microcode_cache in kernel heap memory.
- * So during cold boot, AP will apply_ucode_in_initrd() just like the BSP.
- * In save_microcode_in_initrd_amd() BSP's patch is copied to amd_ucode_patch,
- * which is used upon resume from suspend.
- */
-void load_ucode_amd_ap(unsigned int cpuid_1_eax)
+void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 {
-	struct microcode_amd *mc;
-	struct cpio_data cp;
-
-	mc = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
-	if (mc->hdr.patch_id && mc->hdr.processor_rev_id) {
-		__apply_microcode_amd(mc);
-		return;
-	}
+	struct cpio_data cp = { };
 
-	if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax)))
-		cp = find_microcode_in_initrd((const char *)__pa_nodebug(ucode_path), true);
+	__load_ucode_amd(cpuid_1_eax, &cp);
 
 	if (!(cp.data && cp.size))
 		return;
 
-	/*
-	 * This would set amd_ucode_patch above so that the following APs can
-	 * use it directly instead of going down this path again.
-	 */
 	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL);
 }
-#else
+
 void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 {
 	struct equiv_cpu_entry *eq;
 	struct microcode_amd *mc;
+	struct cont_desc *desc;
 	u16 eq_id;
 
+	if (IS_ENABLED(CONFIG_X86_32)) {
+		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
+		desc = (struct cont_desc *)__pa_nodebug(&cont);
+	} else {
+		mc   = (struct microcode_amd *)amd_ucode_patch;
+		desc = &cont;
+	}
+
 	/* First AP hasn't cached it yet, go through the blob. */
-	if (!cont.data) {
-		struct cpio_data cp;
+	if (!desc->data) {
+		struct cpio_data cp = { };
 
-		if (cont.size == -1)
+		if (desc->size == -1)
 			return;
 
 reget:
-		if (!get_builtin_microcode(&cp, x86_family(cpuid_1_eax))) {
-			cp = find_microcode_in_initrd(ucode_path, false);
-
-			if (!(cp.data && cp.size)) {
-				/*
-				 * Mark it so that other APs do not scan again
-				 * for no real reason and slow down boot
-				 * needlessly.
-				 */
-				cont.size = -1;
-				return;
-			}
+		__load_ucode_amd(cpuid_1_eax, &cp);
+		if (!(cp.data && cp.size)) {
+			/*
+			 * Mark it so that other APs do not scan again for no
+			 * real reason and slow down boot needlessly.
+			 */
+			desc->size = -1;
+			return;
 		}
 
-		if (!apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, false, &cont)) {
-			cont.data = NULL;
-			cont.size = -1;
+		if (!apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, false, desc)) {
+			desc->data = NULL;
+			desc->size = -1;
 			return;
 		}
 	}
 
-	eq  = (struct equiv_cpu_entry *)(cont.data + CONTAINER_HDR_SZ);
+	eq  = (struct equiv_cpu_entry *)(desc->data + CONTAINER_HDR_SZ);
 
 	eq_id = find_equiv_id(eq, cpuid_1_eax);
 	if (!eq_id)
 		return;
 
-	if (eq_id == cont.eq_id) {
+	if (eq_id == desc->eq_id) {
 		u32 rev, dummy;
 
 		microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-
-		mc = (struct microcode_amd *)amd_ucode_patch;
-
 		if (mc && rev < mc->hdr.patch_id) {
 			if (!__apply_microcode_amd(mc))
 				ucode_new_rev = mc->hdr.patch_id;
@@ -394,7 +373,6 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 		goto reget;
 	}
 }
-#endif /* CONFIG_X86_32 */
 
 static enum ucode_state
 load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size);
-- 
2.11.0

Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:
>  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
>  {
>  	struct equiv_cpu_entry *eq;
>  	struct microcode_amd *mc;
> +	struct cont_desc *desc;
>  	u16 eq_id;
>  
> +	if (IS_ENABLED(CONFIG_X86_32)) {
> +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> +	} else {
> +		mc   = (struct microcode_amd *)amd_ucode_patch;
> +		desc = &cont;

Bah! Now I realize that 'cont' is not a local variable as I assumed when
looking at the other patch. 'cont' is a pretty bad name for a (file) global
variable. Can we please use a more obvious name ?

While at it please make that thing static as there cant be a user outside
of that file.

> +	}
> +
>  	/* First AP hasn't cached it yet, go through the blob. */
> -	if (!cont.data) {
> -		struct cpio_data cp;
> +	if (!desc->data) {
> +		struct cpio_data cp = { };
>  
> -		if (cont.size == -1)
> +		if (desc->size == -1)
>  			return;

I'm not really fond of abusing the size member for this. And that '-1'
seems to have different meanings depending on other members. Really not
intuitive.

Please introduce a proper state member which tells what the descriptor
struct contains, i.e. EMPTY, VALID, INVALID ....

Thanks,

	tglx

Re: [PATCH 11/13] x86/microcode/AMD: Unify load_ucode_amd_ap()

[Thomas Gleixner]

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

> On Tue, 17 Jan 2017, Borislav Petkov wrote:
> >  void load_ucode_amd_ap(unsigned int cpuid_1_eax)
> >  {
> >  	struct equiv_cpu_entry *eq;
> >  	struct microcode_amd *mc;
> > +	struct cont_desc *desc;
> >  	u16 eq_id;
> >  
> > +	if (IS_ENABLED(CONFIG_X86_32)) {
> > +		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
> > +		desc = (struct cont_desc *)__pa_nodebug(&cont);
> > +	} else {
> > +		mc   = (struct microcode_amd *)amd_ucode_patch;
> > +		desc = &cont;
> 
> Bah! Now I realize that 'cont' is not a local variable as I assumed when
> looking at the other patch. 'cont' is a pretty bad name for a (file) global
> variable. Can we please use a more obvious name ?
> 
> While at it please make that thing static as there cant be a user outside
> of that file.
> 
> > +	}
> > +
> >  	/* First AP hasn't cached it yet, go through the blob. */
> > -	if (!cont.data) {
> > -		struct cpio_data cp;
> > +	if (!desc->data) {
> > +		struct cpio_data cp = { };
> >  
> > -		if (cont.size == -1)
> > +		if (desc->size == -1)
> >  			return;
> 
> I'm not really fond of abusing the size member for this. And that '-1'
> seems to have different meanings depending on other members. Really not
> intuitive.
> 
> Please introduce a proper state member which tells what the descriptor
> struct contains, i.e. EMPTY, VALID, INVALID ....

Reading further through the series I see that you remove all that cruft at
the end. Dammit, I never start reading books in the final chapter, maybe I
should do that with patch series :)

It might be nice nevertheless to have an initial patch which cleans up the
name of that 'cont' variable and that status thing, but it might not be
worth the trouble as you remove it at the end anyway, which makes a lot of
sense btw. Your call.

Thanks,

	tglx

[PATCH 12/13] x86/microcode/AMD: Simplify saving from initrd

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

No need to use the previously stashed info in the container - simply go
ahead and parse the initrd once more. It simplifies and streamlines the
code a whole lot.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 43 +++++++++++--------------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index f1a61f181c9a..1ee33793c74a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -379,43 +379,26 @@ load_microcode_amd(int cpu, u8 family, const u8 *data, size_t size);
 
 int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 {
+	struct cont_desc desc;
 	enum ucode_state ret;
-	int retval = 0;
-
-	if (!cont.data) {
-		if (IS_ENABLED(CONFIG_X86_32) && (cont.size != -1)) {
-			struct cpio_data cp;
-
-			cp = find_microcode_in_initrd(ucode_path, false);
-			if (!(cp.data && cp.size)) {
-				cont.size = -1;
-				return -EINVAL;
-			}
+	struct cpio_data cp;
 
-			cont.cpuid_1_eax = cpuid_1_eax;
+	cp = find_microcode_in_initrd(ucode_path, false);
+	if (!(cp.data && cp.size))
+		return -EINVAL;
 
-			scan_containers(cp.data, cp.size, &cont);
-			if (!cont.eq_id) {
-				cont.size = -1;
-				return -EINVAL;
-			}
+	desc.cpuid_1_eax = cpuid_1_eax;
 
-		} else
-			return -EINVAL;
-	}
+	scan_containers(cp.data, cp.size, &desc);
+	if (!desc.eq_id)
+		return -EINVAL;
 
-	ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax), cont.data, cont.size);
+	ret = load_microcode_amd(smp_processor_id(), x86_family(cpuid_1_eax),
+				 desc.data, desc.size);
 	if (ret != UCODE_OK)
-		retval = -EINVAL;
-
-	/*
-	 * This will be freed any msec now, stash patches for the current
-	 * family and switch to patch cache for cpu hotplug, etc later.
-	 */
-	cont.data = NULL;
-	cont.size = 0;
+		return -EINVAL;
 
-	return retval;
+	return 0;
 }
 
 void reload_ucode_amd(void)
-- 
2.11.0

Re: [PATCH 12/13] x86/microcode/AMD: Simplify saving from initrd

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> No need to use the previously stashed info in the container - simply go
> ahead and parse the initrd once more. It simplifies and streamlines the
> code a whole lot.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH 13/13] x86/microcode/AMD: Remove AP scanning optimization

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

The idea was to not scan the microcode blob on each AP (Application
Processor) during boot and thus save us some milliseconds. However, on
architectures where the microcode engine is shared between threads, this
doesn't work. Here's why:

The microcode on CPU0, i.e., the first thread, gets updated. The second
thread, i.e., CPU1, i.e., the first AP walks into load_ucode_amd_ap(),
sees that there's no container cached and goes and scans for the proper
blob.

It finds it and as a last step of apply_microcode_early_amd(), it tries
to apply the patch but that core has already the updated microcode
revision which it has received through CPU0's update. So it returns
false and we do desc->size = -1 to prevent other APs from scanning.

However, the next AP, CPU2, has a different microcode engine which
hasn't been updated yet. The desc->size == -1 test prevents it from
scanning the blob anew and we fail to update it.

The fix is much more straight-forward than it looks: the BSP
(BootStrapping Processor), i.e., CPU0, caches the microcode patch
in amd_ucode_patch. We use that on the AP and try to apply it.
In the 99.9999% of cases where we have homogeneous cores - *not*
mixed-steppings - the application will be successful and we're good to
go.

In the remaining small set of systems, we will simply rescan the blob
and find (or not, if none present) the proper patch and apply it then.

Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/cpu/microcode/amd.c | 74 +++++++++----------------------------
 1 file changed, 18 insertions(+), 56 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 1ee33793c74a..c9bb436830e3 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -52,7 +52,7 @@ struct cont_desc {
 	u16 eq_id;
 	u8 *data;
 	size_t size;
-} cont;
+};
 
 static u32 ucode_new_rev;
 static u8 amd_ucode_patch[PATCH_MAX_SIZE];
@@ -210,8 +210,7 @@ static int __apply_microcode_amd(struct microcode_amd *mc)
  * Returns true if container found (sets @desc), false otherwise.
  */
 static bool
-apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
-			  bool save_patch, struct cont_desc *ret_desc)
+apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size, bool save_patch)
 {
 	struct cont_desc desc = { 0 };
 	u8 (*patch)[PATCH_MAX_SIZE];
@@ -250,9 +249,6 @@ apply_microcode_early_amd(u32 cpuid_1_eax, void *ucode, size_t size,
 			memcpy(patch, mc, min_t(u32, desc.psize, PATCH_MAX_SIZE));
 	}
 
-	if (ret_desc)
-		*ret_desc = desc;
-
 	return ret;
 }
 
@@ -302,76 +298,42 @@ void __init load_ucode_amd_bsp(unsigned int cpuid_1_eax)
 	struct cpio_data cp = { };
 
 	__load_ucode_amd(cpuid_1_eax, &cp);
-
 	if (!(cp.data && cp.size))
 		return;
 
-	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true, NULL);
+	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, true);
 }
 
 void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 {
-	struct equiv_cpu_entry *eq;
 	struct microcode_amd *mc;
-	struct cont_desc *desc;
-	u16 eq_id;
+	struct cpio_data cp;
+	u32 *new_rev, rev, dummy;
 
 	if (IS_ENABLED(CONFIG_X86_32)) {
-		mc   = (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
-		desc = (struct cont_desc *)__pa_nodebug(&cont);
+		mc	= (struct microcode_amd *)__pa_nodebug(amd_ucode_patch);
+		new_rev = (u32 *)__pa_nodebug(&ucode_new_rev);
+
 	} else {
-		mc   = (struct microcode_amd *)amd_ucode_patch;
-		desc = &cont;
+		mc	= (struct microcode_amd *)amd_ucode_patch;
+		new_rev = &ucode_new_rev;
 	}
 
-	/* First AP hasn't cached it yet, go through the blob. */
-	if (!desc->data) {
-		struct cpio_data cp = { };
-
-		if (desc->size == -1)
-			return;
-
-reget:
-		__load_ucode_amd(cpuid_1_eax, &cp);
-		if (!(cp.data && cp.size)) {
-			/*
-			 * Mark it so that other APs do not scan again for no
-			 * real reason and slow down boot needlessly.
-			 */
-			desc->size = -1;
-			return;
-		}
+	microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
-		if (!apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, false, desc)) {
-			desc->data = NULL;
-			desc->size = -1;
+	/* Check whether we have saved a new patch already: */
+	if (*new_rev && rev < mc->hdr.patch_id) {
+		if (!__apply_microcode_amd(mc)) {
+			*new_rev = mc->hdr.patch_id;
 			return;
 		}
 	}
 
-	eq  = (struct equiv_cpu_entry *)(desc->data + CONTAINER_HDR_SZ);
-
-	eq_id = find_equiv_id(eq, cpuid_1_eax);
-	if (!eq_id)
+	__load_ucode_amd(cpuid_1_eax, &cp);
+	if (!(cp.data && cp.size))
 		return;
 
-	if (eq_id == desc->eq_id) {
-		u32 rev, dummy;
-
-		microcode_rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
-		if (mc && rev < mc->hdr.patch_id) {
-			if (!__apply_microcode_amd(mc))
-				ucode_new_rev = mc->hdr.patch_id;
-		}
-
-	} else {
-
-		/*
-		 * AP has a different equivalence ID than BSP, looks like
-		 * mixed-steppings silicon so go through the ucode blob anew.
-		 */
-		goto reget;
-	}
+	apply_microcode_early_amd(cpuid_1_eax, cp.data, cp.size, false);
 }
 
 static enum ucode_state
-- 
2.11.0

Re: [PATCH 13/13] x86/microcode/AMD: Remove AP scanning optimization

[Thomas Gleixner]

On Tue, 17 Jan 2017, Borislav Petkov wrote:

> From: Borislav Petkov <bp@suse.de>
> 
> The idea was to not scan the microcode blob on each AP (Application
> Processor) during boot and thus save us some milliseconds. However, on
> architectures where the microcode engine is shared between threads, this
> doesn't work. Here's why:
> 
> The microcode on CPU0, i.e., the first thread, gets updated. The second
> thread, i.e., CPU1, i.e., the first AP walks into load_ucode_amd_ap(),
> sees that there's no container cached and goes and scans for the proper
> blob.
> 
> It finds it and as a last step of apply_microcode_early_amd(), it tries
> to apply the patch but that core has already the updated microcode
> revision which it has received through CPU0's update. So it returns
> false and we do desc->size = -1 to prevent other APs from scanning.
>
> However, the next AP, CPU2, has a different microcode engine which
> hasn't been updated yet. The desc->size == -1 test prevents it from
> scanning the blob anew and we fail to update it.

Well, that could be solved by a proper state member in the global container
descriptor. But your solution is better in the end.

> The fix is much more straight-forward than it looks: the BSP
> (BootStrapping Processor), i.e., CPU0, caches the microcode patch
> in amd_ucode_patch. We use that on the AP and try to apply it.
> In the 99.9999% of cases where we have homogeneous cores - *not*
> mixed-steppings - the application will be successful and we're good to
> go.
> 
> In the remaining small set of systems, we will simply rescan the blob
> and find (or not, if none present) the proper patch and apply it then.

Makes sense, but how does such a system handle the suspend/resume case when
the micro code is in the initrd? Are you caching the per cpu patches
somewhere?

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>