[PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

The following patchset adds early microcode patch loading support on
AMD systems, on top of the framework introduced by:
  https://lkml.org/lkml/2012/12/21/193

V2:
* Fixed warnings when compiling without early loading
* Picked up a typo fix [PATCH 2/3] from Boris

Borislav Petkov (1):
  x86, microcode, intel: Correct typo in printk

Jacob Shin (2):
  x86/microcode: vendor abstract out save_microcode_in_initrd()
  x86/microcode: early microcode patch loading support on AMD

 arch/x86/Kconfig                        |   16 +-
 arch/x86/include/asm/microcode.h        |    1 -
 arch/x86/include/asm/microcode_amd.h    |   14 ++
 arch/x86/include/asm/microcode_intel.h  |    3 +
 arch/x86/kernel/microcode_amd.c         |  338 +++++++++++++++++++++++++++----
 arch/x86/kernel/microcode_core_early.c  |   17 ++
 arch/x86/kernel/microcode_intel_early.c |    4 +-
 7 files changed, 344 insertions(+), 49 deletions(-)
 create mode 100644 arch/x86/include/asm/microcode_amd.h

-- 
1.7.9.5

[PATCH V2 1/3] x86/microcode: vendor abstract out save_microcode_in_initrd()

[Jacob Shin]

Currently save_microcode_in_initrd() is declared in vendor neutural
microcode.h file, but defined in vendor specific
microcode_intel_early.c file. Vendor abstract it out to
microcode_core_early.c with a wrapper function.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 arch/x86/include/asm/microcode_intel.h  |    5 +++++
 arch/x86/kernel/microcode_core_early.c  |   10 ++++++++++
 arch/x86/kernel/microcode_intel_early.c |    2 +-
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 5356f92..538e407 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -67,10 +67,15 @@ update_match_revision(struct microcode_header_intel *mc_header, int rev);
 extern void __init load_ucode_intel_bsp(void);
 extern void __cpuinit load_ucode_intel_ap(void);
 extern void show_ucode_info_early(void);
+extern int __init save_microcode_in_initrd_intel(void);
 #else
 static inline __init void load_ucode_intel_bsp(void) {}
 static inline __cpuinit void load_ucode_intel_ap(void) {}
 static inline void show_ucode_info_early(void) {}
+static inline int __init save_microcode_in_initrd_intel(void)
+{
+	return -EINVAL;
+}
 #endif
 
 #if defined(CONFIG_MICROCODE_INTEL_EARLY) && defined(CONFIG_HOTPLUG_CPU)
diff --git a/arch/x86/kernel/microcode_core_early.c b/arch/x86/kernel/microcode_core_early.c
index 833d51d..0d19ac5 100644
--- a/arch/x86/kernel/microcode_core_early.c
+++ b/arch/x86/kernel/microcode_core_early.c
@@ -98,3 +98,13 @@ void __cpuinit load_ucode_ap(void)
 	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
 		load_ucode_intel_ap();
 }
+
+int __init save_microcode_in_initrd(void)
+{
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+
+	if (c->x86_vendor == X86_VENDOR_INTEL && c->x86 >= 6)
+		return save_microcode_in_initrd_intel();
+
+	return 0;
+}
diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index 2e9e128..88f0edc 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -699,7 +699,7 @@ static int __cpuinit apply_microcode_early(struct mc_saved_data *mc_saved_data,
  * This function converts microcode patch offsets previously stored in
  * mc_saved_in_initrd to pointers and stores the pointers in mc_saved_data.
  */
-int __init save_microcode_in_initrd(void)
+int __init save_microcode_in_initrd_intel(void)
 {
 	unsigned int count = mc_saved_data.mc_saved_count;
 	struct microcode_intel *mc_saved[MAX_UCODE_COUNT];
-- 
1.7.9.5

Re: [PATCH V2 1/3] x86/microcode: vendor abstract out save_microcode_in_initrd()

[Borislav Petkov]

On Thu, May 23, 2013 at 10:40:16AM -0500, Jacob Shin wrote:
> Currently save_microcode_in_initrd() is declared in vendor neutural
> microcode.h file, but defined in vendor specific
> microcode_intel_early.c file. Vendor abstract it out to
> microcode_core_early.c with a wrapper function.
> 
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  arch/x86/include/asm/microcode_intel.h  |    5 +++++
>  arch/x86/kernel/microcode_core_early.c  |   10 ++++++++++
>  arch/x86/kernel/microcode_intel_early.c |    2 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
> index 5356f92..538e407 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -67,10 +67,15 @@ update_match_revision(struct microcode_header_intel *mc_header, int rev);
>  extern void __init load_ucode_intel_bsp(void);
>  extern void __cpuinit load_ucode_intel_ap(void);
>  extern void show_ucode_info_early(void);
> +extern int __init save_microcode_in_initrd_intel(void);
>  #else
>  static inline __init void load_ucode_intel_bsp(void) {}
>  static inline __cpuinit void load_ucode_intel_ap(void) {}
>  static inline void show_ucode_info_early(void) {}
> +static inline int __init save_microcode_in_initrd_intel(void)
> +{
> +	return -EINVAL;
> +}

We leave those on one line as in the diff I sent you earlier.

-- 
Regards/Gruss,
    Boris.

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

[PATCH V2 2/3] x86, microcode, intel: Correct typo in printk

[Jacob Shin]

From: Borislav Petkov <bp@suse.de>

User-visible so correct it.

Signed-off-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 arch/x86/kernel/microcode_intel_early.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
index 88f0edc..5e558eb 100644
--- a/arch/x86/kernel/microcode_intel_early.c
+++ b/arch/x86/kernel/microcode_intel_early.c
@@ -529,7 +529,7 @@ int save_mc_for_early(u8 *mc)
 	 */
 	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
 	if (ret) {
-		pr_err("Can not save microcode patch.\n");
+		pr_err("Cannot save microcode patches from initrd.\n");
 		goto out;
 	}
 
-- 
1.7.9.5

Re: [PATCH V2 2/3] x86, microcode, intel: Correct typo in printk

[Joe Perches]

On Thu, 2013-05-23 at 10:40 -0500, Jacob Shin wrote:
> User-visible so correct it.
[]
> diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
[]
> @@ -529,7 +529,7 @@ int save_mc_for_early(u8 *mc)
[]
> -		pr_err("Can not save microcode patch.\n");
> +		pr_err("Cannot save microcode patches from initrd.\n");

It seems you enhanced the message rather than fixed
a typo so I think the subject line isn't correct.

Re: [PATCH V2 2/3] x86, microcode, intel: Correct typo in printk

[Borislav Petkov]

On Thu, May 23, 2013 at 10:40:17AM -0500, Jacob Shin wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> User-visible so correct it.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  arch/x86/kernel/microcode_intel_early.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
> index 88f0edc..5e558eb 100644
> --- a/arch/x86/kernel/microcode_intel_early.c
> +++ b/arch/x86/kernel/microcode_intel_early.c
> @@ -529,7 +529,7 @@ int save_mc_for_early(u8 *mc)
>  	 */
>  	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
>  	if (ret) {
> -		pr_err("Can not save microcode patch.\n");
> +		pr_err("Cannot save microcode patches from initrd.\n");

This doesn't look like the diff I sent you, please redo against latest
Linus tree when redoing your patchset.

Re: [PATCH V2 2/3] x86, microcode, intel: Correct typo in printk

[Jacob Shin]

On Fri, May 24, 2013 at 10:43:50AM +0200, Borislav Petkov wrote:
> On Thu, May 23, 2013 at 10:40:17AM -0500, Jacob Shin wrote:
> > From: Borislav Petkov <bp@suse.de>
> > 
> > User-visible so correct it.
> > 
> > Signed-off-by: Borislav Petkov <bp@suse.de>
> > Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> > ---
> >  arch/x86/kernel/microcode_intel_early.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/microcode_intel_early.c b/arch/x86/kernel/microcode_intel_early.c
> > index 88f0edc..5e558eb 100644
> > --- a/arch/x86/kernel/microcode_intel_early.c
> > +++ b/arch/x86/kernel/microcode_intel_early.c
> > @@ -529,7 +529,7 @@ int save_mc_for_early(u8 *mc)
> >  	 */
> >  	ret = save_microcode(&mc_saved_data, mc_saved_tmp, mc_saved_count);
> >  	if (ret) {
> > -		pr_err("Can not save microcode patch.\n");
> > +		pr_err("Cannot save microcode patches from initrd.\n");
> 
> This doesn't look like the diff I sent you, please redo against latest
> Linus tree when redoing your patchset.

Oops, the patch didn't apply cleanly for me, so I hand fixed it up,
but I think I did it wrong. I'll fix that. Thanks.

[PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

Add support for early microcode patch loading on AMD.

Signed-off-by: Jacob Shin <jacob.shin@amd.com>
---
 arch/x86/Kconfig                       |   16 +-
 arch/x86/include/asm/microcode.h       |    1 -
 arch/x86/include/asm/microcode_amd.h   |   17 ++
 arch/x86/include/asm/microcode_intel.h |    1 +
 arch/x86/kernel/microcode_amd.c        |  338 ++++++++++++++++++++++++++++----
 arch/x86/kernel/microcode_core_early.c |    7 +
 6 files changed, 333 insertions(+), 47 deletions(-)
 create mode 100644 arch/x86/include/asm/microcode_amd.h

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3a5bced..fab72e7 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
 	depends on MICROCODE_INTEL
 
 config MICROCODE_INTEL_EARLY
-	bool "Early load microcode"
+	def_bool n
 	depends on MICROCODE_INTEL && BLK_DEV_INITRD
+
+config MICROCODE_AMD_EARLY
+	def_bool n
+	depends on MICROCODE_AMD && BLK_DEV_INITRD
+
+config MICROCODE_EARLY
+	bool "Early load microcode"
+	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
+	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
+	select MICROCODE_AMD_EARLY if MICROCODE_AMD
 	default y
 	help
 	  This option provides functionality to read additional microcode data
@@ -1099,10 +1109,6 @@ config MICROCODE_INTEL_EARLY
 	  microcode to CPU's as early as possible. No functional change if no
 	  microcode data is glued to the initrd, therefore it's safe to say Y.
 
-config MICROCODE_EARLY
-	def_bool y
-	depends on MICROCODE_INTEL_EARLY
-
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
 	---help---
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 6825e2e..f4be4cc 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -58,7 +58,6 @@ static inline void __exit exit_amd_microcode(void) {}
 #endif
 
 #ifdef CONFIG_MICROCODE_EARLY
-#define MAX_UCODE_COUNT 128
 extern void __init load_ucode_bsp(void);
 extern __init void load_ucode_ap(void);
 extern int __init save_microcode_in_initrd(void);
diff --git a/arch/x86/include/asm/microcode_amd.h b/arch/x86/include/asm/microcode_amd.h
new file mode 100644
index 0000000..376123c
--- /dev/null
+++ b/arch/x86/include/asm/microcode_amd.h
@@ -0,0 +1,17 @@
+#ifndef _ASM_X86_MICROCODE_AMD_H
+#define _ASM_X86_MICROCODE_AMD_H
+
+#ifdef CONFIG_MICROCODE_AMD_EARLY
+extern void __init load_ucode_amd_bsp(void);
+extern void __cpuinit load_ucode_amd_ap(void);
+extern int __init save_microcode_in_initrd_amd(void);
+#else
+static inline void __init load_ucode_amd_bsp(void) {}
+static inline void __cpuinit load_ucode_amd_ap(void) {}
+static inline int __init save_microcode_in_initrd_amd(void)
+{
+	return -EINVAL;
+}
+#endif
+
+#endif /* _ASM_X86_MICROCODE_AMD_H */
diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 538e407..db4a2f9 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -64,6 +64,7 @@ extern int
 update_match_revision(struct microcode_header_intel *mc_header, int rev);
 
 #ifdef CONFIG_MICROCODE_INTEL_EARLY
+#define MAX_UCODE_COUNT 128
 extern void __init load_ucode_intel_bsp(void);
 extern void __cpuinit load_ucode_intel_ap(void);
 extern void show_ucode_info_early(void);
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index efdec7c..cda647e 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -27,10 +27,13 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pci.h>
+#include <linux/earlycpio.h>
 
 #include <asm/microcode.h>
+#include <asm/microcode_amd.h>
 #include <asm/processor.h>
 #include <asm/msr.h>
+#include <asm/setup.h>
 
 MODULE_DESCRIPTION("AMD Microcode Update Driver");
 MODULE_AUTHOR("Peter Oruba");
@@ -84,23 +87,28 @@ struct ucode_patch {
 
 static LIST_HEAD(pcache);
 
-static u16 find_equiv_id(unsigned int cpu)
+static u16 _find_equiv_id(struct equiv_cpu_entry *eq,
+			  struct ucode_cpu_info *uci)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	int i = 0;
 
-	if (!equiv_cpu_table)
+	if (!eq)
 		return 0;
 
-	while (equiv_cpu_table[i].installed_cpu != 0) {
-		if (uci->cpu_sig.sig == equiv_cpu_table[i].installed_cpu)
-			return equiv_cpu_table[i].equiv_cpu;
+	while (eq[i].installed_cpu != 0) {
+		if (uci->cpu_sig.sig == eq[i].installed_cpu)
+			return eq[i].equiv_cpu;
 
 		i++;
 	}
 	return 0;
 }
 
+static u16 find_equiv_id(unsigned int cpu)
+{
+	return _find_equiv_id(equiv_cpu_table, ucode_cpu_info + cpu);
+}
+
 static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
 {
 	int i = 0;
@@ -173,9 +181,17 @@ static struct ucode_patch *find_patch(unsigned int cpu)
 static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_patch *p;
 
 	csig->sig = cpuid_eax(0x00000001);
 	csig->rev = c->microcode;
+
+	/* if a patch was early loaded, tell microcode_core.c about it */
+	p = find_patch(cpu);
+	if (p && (p->patch_id == csig->rev))
+		uci->mc = p->data;
+
 	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
 
 	return 0;
@@ -215,24 +231,14 @@ static unsigned int verify_patch_size(int cpu, u32 patch_size,
 	return patch_size;
 }
 
-static int apply_microcode_amd(int cpu)
+static int _apply_microcode_amd(int cpu, void *data, struct cpuinfo_x86 *c,
+				struct ucode_cpu_info *uci, bool silent)
 {
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct microcode_amd *mc_amd;
-	struct ucode_cpu_info *uci;
-	struct ucode_patch *p;
 	u32 rev, dummy;
 
-	BUG_ON(raw_smp_processor_id() != cpu);
-
-	uci = ucode_cpu_info + cpu;
-
-	p = find_patch(cpu);
-	if (!p)
-		return 0;
-
-	mc_amd  = p->data;
-	uci->mc = p->data;
+	mc_amd  = data;
+	uci->mc = data;
 
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
@@ -247,18 +253,37 @@ static int apply_microcode_amd(int cpu)
 	/* verify patch application was successful */
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 	if (rev != mc_amd->hdr.patch_id) {
-		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
-		       cpu, mc_amd->hdr.patch_id);
+		if (!silent)
+			pr_err("CPU%d: update failed for patch_level=0x%08x\n",
+				cpu, mc_amd->hdr.patch_id);
 		return -1;
 	}
 
-	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
+	if (!silent)
+		pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
 	uci->cpu_sig.rev = rev;
 	c->microcode = rev;
 
 	return 0;
 }
 
+static int apply_microcode_amd(int cpu)
+{
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	struct ucode_cpu_info *uci;
+	struct ucode_patch *p;
+
+	BUG_ON(raw_smp_processor_id() != cpu);
+
+	uci = ucode_cpu_info + cpu;
+
+	p = find_patch(cpu);
+	if (!p)
+		return 0;
+
+	return _apply_microcode_amd(cpu, p->data, c, uci, false);
+}
+
 static int install_equiv_cpu_table(const u8 *buf)
 {
 	unsigned int *ibuf = (unsigned int *)buf;
@@ -398,6 +423,44 @@ static enum ucode_state load_microcode_amd(int cpu, const u8 *data, size_t size)
 	return UCODE_OK;
 }
 
+#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
+#define MPB_MAX_SIZE F15H_MPB_MAX_SIZE
+static u8 bsp_mpb[MPB_MAX_SIZE];
+#endif
+static enum ucode_state request_microcode_fw(int cpu, const struct firmware *fw)
+{
+	enum ucode_state ret = UCODE_ERROR;
+
+	if (*(u32 *)fw->data != UCODE_MAGIC) {
+		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
+		return ret;
+	}
+
+	/* free old equiv table */
+	free_equiv_cpu_table();
+
+	ret = load_microcode_amd(cpu, fw->data, fw->size);
+	if (ret != UCODE_OK)
+		cleanup();
+
+#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
+	/*
+	 * on X86_32 early load, while CPU hotplugging on, we cannot traverse
+	 * pcache since paging is not turneded on yet. so stash away BSP's MPB
+	 * when a new fw file is installed.
+	 */
+	if (cpu_data(cpu).cpu_index == boot_cpu_data.cpu_index) {
+		struct ucode_patch *p;
+
+		p = find_patch(cpu);
+		if (p)
+			memcpy(bsp_mpb, p->data, min_t(u32, ksize(p->data),
+						       MPB_MAX_SIZE));
+	}
+#endif
+	return ret;
+}
+
 /*
  * AMD microcode firmware naming convention, up to family 15h they are in
  * the legacy file:
@@ -419,7 +482,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 {
 	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	enum ucode_state ret = UCODE_NFOUND;
+	enum ucode_state ret;
 	const struct firmware *fw;
 
 	/* reload ucode container only on the boot cpu */
@@ -431,26 +494,11 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 
 	if (request_firmware(&fw, (const char *)fw_name, device)) {
 		pr_err("failed to load file %s\n", fw_name);
-		goto out;
+		return UCODE_NFOUND;
 	}
 
-	ret = UCODE_ERROR;
-	if (*(u32 *)fw->data != UCODE_MAGIC) {
-		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
-		goto fw_release;
-	}
-
-	/* free old equiv table */
-	free_equiv_cpu_table();
-
-	ret = load_microcode_amd(cpu, fw->data, fw->size);
-	if (ret != UCODE_OK)
-		cleanup();
-
- fw_release:
+	ret = request_microcode_fw(cpu, fw);
 	release_firmware(fw);
-
- out:
 	return ret;
 }
 
@@ -475,6 +523,214 @@ static struct microcode_ops microcode_amd_ops = {
 	.microcode_fini_cpu               = microcode_fini_cpu_amd,
 };
 
+#ifdef CONFIG_MICROCODE_AMD_EARLY
+/*
+ * Early Loading Support
+ */
+
+static void __cpuinit collect_cpu_info_amd_early(struct cpuinfo_x86 *c,
+						 struct ucode_cpu_info *uci)
+{
+	u32 rev, eax;
+
+	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, eax);
+	eax = cpuid_eax(0x00000001);
+
+	uci->cpu_sig.sig = eax;
+	uci->cpu_sig.rev = rev;
+	c->microcode = rev;
+	c->x86 = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
+}
+
+/*
+ * microcode patch container file is prepended to the initrd in cpio format.
+ * see Documentation/x86/early-microcode.txt
+ */
+static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
+
+static struct cpio_data __init
+request_firmware_in_initrd(void)
+{
+	long offset = 0;
+
+	return find_cpio_data(ucode_path,
+			(void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET),
+			boot_params.hdr.ramdisk_size, &offset);
+}
+
+static struct cpio_data __init request_firmware_in_initrd_early(void)
+{
+#ifdef CONFIG_X86_32
+	struct boot_params *boot_params_p;
+	long offset = 0;
+
+	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
+	return find_cpio_data((char *)__pa_nodebug(ucode_path),
+			(void *)boot_params_p->hdr.ramdisk_image,
+			boot_params_p->hdr.ramdisk_size, &offset);
+#else
+	return request_firmware_in_initrd();
+#endif
+}
+
+static int __init
+find_equiv_cpu_table_early(const u8 *buf, struct equiv_cpu_entry **eq)
+{
+	unsigned int *ibuf = (unsigned int *)buf;
+	unsigned int type = ibuf[1];
+	unsigned int size = ibuf[2];
+
+	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size)
+		return -EINVAL;
+
+	/*
+	 * during BSP load, vmalloc() is not available yet.
+	 * so just use equivalent cpu table in initrd memory in place,
+	 * no need to copy it. on X86_64, first AP to load will actually
+	 * "install" the equiv_cpu_table. on X86_32, before mm frees up initrd.
+	 */
+	*eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
+
+	/* add header length */
+	return size + CONTAINER_HDR_SZ;
+}
+
+static enum ucode_state __init
+load_microcode_amd_early(int cpu, const u8 *data, size_t size)
+{
+	unsigned int leftover;
+	u8 *fw = (u8 *)data;
+	int offset;
+	u16 equiv_id;
+	struct equiv_cpu_entry *eq;
+	struct cpuinfo_x86 c;
+	struct ucode_cpu_info uci;
+
+	collect_cpu_info_amd_early(&c, &uci);
+
+	offset = find_equiv_cpu_table_early(data, &eq);
+	if (offset < 0)
+		return UCODE_ERROR;
+	fw += offset;
+	leftover = size - offset;
+
+	if (*(u32 *)fw != UCODE_UCODE_TYPE)
+		return UCODE_ERROR;
+
+	equiv_id = _find_equiv_id(eq, &uci);
+	if (!equiv_id)
+		return UCODE_NFOUND;
+
+	while (leftover) {
+		struct microcode_amd *mc;
+		int patch_size = *(u32 *)(fw + 4);
+
+		/*
+		 * during BSP load, vmalloc() is not available yet,
+		 * so simply find and apply the matching microcode patch in
+		 * initrd memory in place. on X86_64, first AP to load will
+		 * actually "cache" the patches in kernel memory.
+		 */
+		mc = (struct microcode_amd *)(fw + SECTION_HDR_SIZE);
+		if (equiv_id == mc->hdr.processor_rev_id)
+			if (_apply_microcode_amd(cpu, mc, &c, &uci, true) == 0)
+				break;
+
+		offset    = patch_size + SECTION_HDR_SIZE;
+		fw	 += offset;
+		leftover -= offset;
+	}
+
+	return UCODE_OK;
+}
+
+static void collect_cpu_info_amd_early_bsp(void *arg)
+{
+	unsigned int cpu = smp_processor_id();
+	struct cpuinfo_x86 dummy;
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	collect_cpu_info_amd_early(&dummy, uci);
+}
+
+int __init save_microcode_in_initrd_amd(void)
+{
+	struct cpio_data cd;
+	struct firmware fw;
+	struct ucode_cpu_info *uci = ucode_cpu_info + boot_cpu_data.cpu_index;
+
+	if (equiv_cpu_table)
+		return 0;
+
+	cd = request_firmware_in_initrd();
+	if (!cd.data)
+		return -EINVAL;
+
+	fw.data = cd.data;
+	fw.size = cd.size;
+
+	if (!uci->cpu_sig.sig)
+		smp_call_function_single(boot_cpu_data.cpu_index,
+					 collect_cpu_info_amd_early_bsp, NULL,
+					 1);
+
+	if (request_microcode_fw(boot_cpu_data.cpu_index, &fw) != UCODE_OK)
+		return -EINVAL;
+
+	return 0;
+}
+
+void __init load_ucode_amd_bsp(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct cpio_data fw = request_firmware_in_initrd_early();
+
+	if (!fw.data)
+		return;
+
+	load_microcode_amd_early(cpu, fw.data, fw.size);
+}
+
+#ifdef CONFIG_X86_32
+/*
+ * on X86_32 AP load, since paging is turned off and vmalloc() is not available
+ * yet, we cannot install the equivalent cpu table nor cache the microcode
+ * patches in kernel memory, so just take the BSP code path. unless we are
+ * CPU hotplugging on (i.e. resume from suspend), which then we will load from
+ * bsp_mpb instead.
+ */
+void __cpuinit load_ucode_amd_ap(void)
+{
+	unsigned int cpu = smp_processor_id();
+	struct microcode_amd *mc;
+	struct cpuinfo_x86 c;
+	struct ucode_cpu_info uci;
+
+	mc = (struct microcode_amd *)__pa_nodebug(bsp_mpb);
+	if (mc->hdr.patch_id && mc->hdr.processor_rev_id)
+		_apply_microcode_amd(cpu, mc, &c, &uci, true);
+	else
+		load_ucode_amd_bsp();
+}
+#else /* !CONFIG_X86_32 */
+/*
+ * on X86_64 AP load, we can vmalloc(). we can go through the normal (non-early)
+ * code path, we just have to make sure we prepare cpu_data and ucode_cpu_info.
+ */
+void __cpuinit load_ucode_amd_ap(void)
+{
+	unsigned int cpu = smp_processor_id();
+
+	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
+
+	if (cpu && !equiv_cpu_table)
+		if (save_microcode_in_initrd_amd())
+			return;
+
+	apply_microcode_amd(cpu);
+}
+#endif /* end CONFIG_X86_32 */
+#endif /* end CONFIG_MICROCODE_AMD_EARLY */
+
 struct microcode_ops * __init init_amd_microcode(void)
 {
 	struct cpuinfo_x86 *c = &cpu_data(0);
diff --git a/arch/x86/kernel/microcode_core_early.c b/arch/x86/kernel/microcode_core_early.c
index 0d19ac5..1221c22 100644
--- a/arch/x86/kernel/microcode_core_early.c
+++ b/arch/x86/kernel/microcode_core_early.c
@@ -18,6 +18,7 @@
  */
 #include <linux/module.h>
 #include <asm/microcode_intel.h>
+#include <asm/microcode_amd.h>
 #include <asm/processor.h>
 
 #define QCHAR(a, b, c, d) ((a) + ((b) << 8) + ((c) << 16) + ((d) << 24))
@@ -83,6 +84,8 @@ void __init load_ucode_bsp(void)
 
 	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
 		load_ucode_intel_bsp();
+	else if (vendor == X86_VENDOR_AMD && x86 >= 0x10)
+		load_ucode_amd_bsp();
 }
 
 void __cpuinit load_ucode_ap(void)
@@ -97,6 +100,8 @@ void __cpuinit load_ucode_ap(void)
 
 	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
 		load_ucode_intel_ap();
+	else if (vendor == X86_VENDOR_AMD && x86 >= 0x10)
+		load_ucode_amd_ap();
 }
 
 int __init save_microcode_in_initrd(void)
@@ -105,6 +110,8 @@ int __init save_microcode_in_initrd(void)
 
 	if (c->x86_vendor == X86_VENDOR_INTEL && c->x86 >= 6)
 		return save_microcode_in_initrd_intel();
+	else if (c->x86_vendor == X86_VENDOR_AMD && c->x86 >= 0x10)
+		return save_microcode_in_initrd_amd();
 
 	return 0;
 }
-- 
1.7.9.5

RE: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Yu, Fenghua]

> From: Jacob Shin [mailto:jacob.shin@amd.com]
> Add support for early microcode patch loading on AMD.
> 
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  arch/x86/Kconfig                       |   16 +-
>  arch/x86/include/asm/microcode.h       |    1 -
>  arch/x86/include/asm/microcode_amd.h   |   17 ++
>  arch/x86/include/asm/microcode_intel.h |    1 +
>  arch/x86/kernel/microcode_amd.c        |  338
> ++++++++++++++++++++++++++++----
>  arch/x86/kernel/microcode_core_early.c |    7 +
>  6 files changed, 333 insertions(+), 47 deletions(-)
>  create mode 100644 arch/x86/include/asm/microcode_amd.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3a5bced..fab72e7 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
>  	depends on MICROCODE_INTEL
> 
>  config MICROCODE_INTEL_EARLY
> -	bool "Early load microcode"
> +	def_bool n
>  	depends on MICROCODE_INTEL && BLK_DEV_INITRD
> +
> +config MICROCODE_AMD_EARLY
> +	def_bool n
> +	depends on MICROCODE_AMD && BLK_DEV_INITRD
> +
> +config MICROCODE_EARLY
> +	bool "Early load microcode"
> +	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
> +	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
> +	select MICROCODE_AMD_EARLY if MICROCODE_AMD
>  	default y
>  	help
>  	  This option provides functionality to read additional microcode
> data
> @@ -1099,10 +1109,6 @@ config MICROCODE_INTEL_EARLY
>  	  microcode to CPU's as early as possible. No functional change
> if no
>  	  microcode data is glued to the initrd, therefore it's safe to
> say Y.
> 
> -config MICROCODE_EARLY
> -	def_bool y
> -	depends on MICROCODE_INTEL_EARLY
> -
>  config X86_MSR
>  	tristate "/dev/cpu/*/msr - Model-specific register support"
>  	---help---
> diff --git a/arch/x86/include/asm/microcode.h
> b/arch/x86/include/asm/microcode.h
> index 6825e2e..f4be4cc 100644
> --- a/arch/x86/include/asm/microcode.h
> +++ b/arch/x86/include/asm/microcode.h
> @@ -58,7 +58,6 @@ static inline void __exit exit_amd_microcode(void) {}
>  #endif
> 
>  #ifdef CONFIG_MICROCODE_EARLY
> -#define MAX_UCODE_COUNT 128
>  extern void __init load_ucode_bsp(void);
>  extern __init void load_ucode_ap(void);
>  extern int __init save_microcode_in_initrd(void);
> diff --git a/arch/x86/include/asm/microcode_amd.h
> b/arch/x86/include/asm/microcode_amd.h
> new file mode 100644
> index 0000000..376123c
> --- /dev/null
> +++ b/arch/x86/include/asm/microcode_amd.h
> @@ -0,0 +1,17 @@
> +#ifndef _ASM_X86_MICROCODE_AMD_H
> +#define _ASM_X86_MICROCODE_AMD_H
> +
> +#ifdef CONFIG_MICROCODE_AMD_EARLY
> +extern void __init load_ucode_amd_bsp(void);
> +extern void __cpuinit load_ucode_amd_ap(void);
> +extern int __init save_microcode_in_initrd_amd(void);
> +#else
> +static inline void __init load_ucode_amd_bsp(void) {}
> +static inline void __cpuinit load_ucode_amd_ap(void) {}
> +static inline int __init save_microcode_in_initrd_amd(void)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +#endif /* _ASM_X86_MICROCODE_AMD_H */
> diff --git a/arch/x86/include/asm/microcode_intel.h
> b/arch/x86/include/asm/microcode_intel.h
> index 538e407..db4a2f9 100644
> --- a/arch/x86/include/asm/microcode_intel.h
> +++ b/arch/x86/include/asm/microcode_intel.h
> @@ -64,6 +64,7 @@ extern int
>  update_match_revision(struct microcode_header_intel *mc_header, int
> rev);
> 
>  #ifdef CONFIG_MICROCODE_INTEL_EARLY
> +#define MAX_UCODE_COUNT 128
>  extern void __init load_ucode_intel_bsp(void);
>  extern void __cpuinit load_ucode_intel_ap(void);
>  extern void show_ucode_info_early(void);
> diff --git a/arch/x86/kernel/microcode_amd.c
> b/arch/x86/kernel/microcode_amd.c
> index efdec7c..cda647e 100644
> --- a/arch/x86/kernel/microcode_amd.c
> +++ b/arch/x86/kernel/microcode_amd.c
> @@ -27,10 +27,13 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/earlycpio.h>
> 
>  #include <asm/microcode.h>
> +#include <asm/microcode_amd.h>
>  #include <asm/processor.h>
>  #include <asm/msr.h>
> +#include <asm/setup.h>
> 
>  MODULE_DESCRIPTION("AMD Microcode Update Driver");
>  MODULE_AUTHOR("Peter Oruba");
> @@ -84,23 +87,28 @@ struct ucode_patch {
> 
>  static LIST_HEAD(pcache);
> 
> -static u16 find_equiv_id(unsigned int cpu)
> +static u16 _find_equiv_id(struct equiv_cpu_entry *eq,
> +			  struct ucode_cpu_info *uci)
>  {
> -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>  	int i = 0;
> 
> -	if (!equiv_cpu_table)
> +	if (!eq)
>  		return 0;
> 
> -	while (equiv_cpu_table[i].installed_cpu != 0) {
> -		if (uci->cpu_sig.sig == equiv_cpu_table[i].installed_cpu)
> -			return equiv_cpu_table[i].equiv_cpu;
> +	while (eq[i].installed_cpu != 0) {
> +		if (uci->cpu_sig.sig == eq[i].installed_cpu)
> +			return eq[i].equiv_cpu;
> 
>  		i++;
>  	}
>  	return 0;
>  }
> 
> +static u16 find_equiv_id(unsigned int cpu)
> +{
> +	return _find_equiv_id(equiv_cpu_table, ucode_cpu_info + cpu);
> +}
> +
>  static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
>  {
>  	int i = 0;
> @@ -173,9 +181,17 @@ static struct ucode_patch *find_patch(unsigned int
> cpu)
>  static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> +	struct ucode_patch *p;
> 
>  	csig->sig = cpuid_eax(0x00000001);
>  	csig->rev = c->microcode;
> +
> +	/* if a patch was early loaded, tell microcode_core.c about it */
> +	p = find_patch(cpu);
> +	if (p && (p->patch_id == csig->rev))
> +		uci->mc = p->data;
> +
>  	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
> 
>  	return 0;
> @@ -215,24 +231,14 @@ static unsigned int verify_patch_size(int cpu,
> u32 patch_size,
>  	return patch_size;
>  }
> 
> -static int apply_microcode_amd(int cpu)
> +static int _apply_microcode_amd(int cpu, void *data, struct
> cpuinfo_x86 *c,
> +				struct ucode_cpu_info *uci, bool silent)
>  {
> -	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	struct microcode_amd *mc_amd;
> -	struct ucode_cpu_info *uci;
> -	struct ucode_patch *p;
>  	u32 rev, dummy;
> 
> -	BUG_ON(raw_smp_processor_id() != cpu);
> -
> -	uci = ucode_cpu_info + cpu;
> -
> -	p = find_patch(cpu);
> -	if (!p)
> -		return 0;
> -
> -	mc_amd  = p->data;
> -	uci->mc = p->data;
> +	mc_amd  = data;
> +	uci->mc = data;
> 
>  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> 
> @@ -247,18 +253,37 @@ static int apply_microcode_amd(int cpu)
>  	/* verify patch application was successful */
>  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
>  	if (rev != mc_amd->hdr.patch_id) {
> -		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> -		       cpu, mc_amd->hdr.patch_id);
> +		if (!silent)
> +			pr_err("CPU%d: update failed for
> patch_level=0x%08x\n",
> +				cpu, mc_amd->hdr.patch_id);
>  		return -1;
>  	}
> 
> -	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> +	if (!silent)
> +		pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);

In 32-bit, this info won't be printed. Then microcode is silently updated. This may make it difficult for user to catch new microcode update info. And this is not consistent to 64-bit which prints the info.

You might print this info later when printk is usable like Intel ucode does.

>  	uci->cpu_sig.rev = rev;
>  	c->microcode = rev;
> 
>  	return 0;
>  }
> 
> +static int apply_microcode_amd(int cpu)

This apply_microcode_amd(int cpu) can be simplified as apply_microcode_amd(void).

> +{
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	struct ucode_cpu_info *uci;
> +	struct ucode_patch *p;
> +
> +	BUG_ON(raw_smp_processor_id() != cpu);
> +

Then you can get cpu=smp_processor_id() here and remove this BUG_ON(). The code will be simpler and easier to understand.

> +	uci = ucode_cpu_info + cpu;
> +
> +	p = find_patch(cpu);
> +	if (!p)
> +		return 0;
> +
> +	return _apply_microcode_amd(cpu, p->data, c, uci, false);
> +}
> +
>  static int install_equiv_cpu_table(const u8 *buf)
>  {
>  	unsigned int *ibuf = (unsigned int *)buf;
> @@ -398,6 +423,44 @@ static enum ucode_state load_microcode_amd(int cpu,
> const u8 *data, size_t size)
>  	return UCODE_OK;
>  }
> 
> +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> +#define MPB_MAX_SIZE F15H_MPB_MAX_SIZE
> +static u8 bsp_mpb[MPB_MAX_SIZE];
> +#endif
> +static enum ucode_state request_microcode_fw(int cpu, const struct
> firmware *fw)
> +{
> +	enum ucode_state ret = UCODE_ERROR;
> +
> +	if (*(u32 *)fw->data != UCODE_MAGIC) {
> +		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> +		return ret;
> +	}
> +
> +	/* free old equiv table */
> +	free_equiv_cpu_table();
> +
> +	ret = load_microcode_amd(cpu, fw->data, fw->size);
> +	if (ret != UCODE_OK)
> +		cleanup();
> +
> +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> +	/*
> +	 * on X86_32 early load, while CPU hotplugging on, we cannot
> traverse
> +	 * pcache since paging is not turneded on yet. so stash away
> BSP's MPB
> +	 * when a new fw file is installed.
> +	 */
> +	if (cpu_data(cpu).cpu_index == boot_cpu_data.cpu_index) {
> +		struct ucode_patch *p;
> +
> +		p = find_patch(cpu);
> +		if (p)
> +			memcpy(bsp_mpb, p->data, min_t(u32, ksize(p->data),
> +						       MPB_MAX_SIZE));
> +	}
> +#endif
> +	return ret;
> +}
> +
>  /*
>   * AMD microcode firmware naming convention, up to family 15h they are
> in
>   * the legacy file:
> @@ -419,7 +482,7 @@ static enum ucode_state request_microcode_amd(int
> cpu, struct device *device,
>  {
>  	char fw_name[36] = "amd-ucode/microcode_amd.bin";
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> -	enum ucode_state ret = UCODE_NFOUND;
> +	enum ucode_state ret;
>  	const struct firmware *fw;
> 
>  	/* reload ucode container only on the boot cpu */
> @@ -431,26 +494,11 @@ static enum ucode_state request_microcode_amd(int
> cpu, struct device *device,
> 
>  	if (request_firmware(&fw, (const char *)fw_name, device)) {
>  		pr_err("failed to load file %s\n", fw_name);
> -		goto out;
> +		return UCODE_NFOUND;
>  	}
> 
> -	ret = UCODE_ERROR;
> -	if (*(u32 *)fw->data != UCODE_MAGIC) {
> -		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> -		goto fw_release;
> -	}
> -
> -	/* free old equiv table */
> -	free_equiv_cpu_table();
> -
> -	ret = load_microcode_amd(cpu, fw->data, fw->size);
> -	if (ret != UCODE_OK)
> -		cleanup();
> -
> - fw_release:
> +	ret = request_microcode_fw(cpu, fw);
>  	release_firmware(fw);
> -
> - out:
>  	return ret;
>  }
> 
> @@ -475,6 +523,214 @@ static struct microcode_ops microcode_amd_ops = {
>  	.microcode_fini_cpu               = microcode_fini_cpu_amd,
>  };
> 
> +#ifdef CONFIG_MICROCODE_AMD_EARLY
> +/*
> + * Early Loading Support
> + */
> +
> +static void __cpuinit collect_cpu_info_amd_early(struct cpuinfo_x86 *c,
> +						 struct ucode_cpu_info *uci)
> +{
> +	u32 rev, eax;
> +
> +	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, eax);
> +	eax = cpuid_eax(0x00000001);
> +
> +	uci->cpu_sig.sig = eax;
> +	uci->cpu_sig.rev = rev;
> +	c->microcode = rev;
> +	c->x86 = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
> +}
> +
> +/*
> + * microcode patch container file is prepended to the initrd in cpio
> format.
> + * see Documentation/x86/early-microcode.txt
> + */
> +static __initdata char ucode_path[] =
> "kernel/x86/microcode/AuthenticAMD.bin";
> +
> +static struct cpio_data __init
> +request_firmware_in_initrd(void)
> +{
> +	long offset = 0;
> +
> +	return find_cpio_data(ucode_path,
> +			(void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET),
> +			boot_params.hdr.ramdisk_size, &offset);
> +}
> +
> +static struct cpio_data __init request_firmware_in_initrd_early(void)
> +{
> +#ifdef CONFIG_X86_32
> +	struct boot_params *boot_params_p;
> +	long offset = 0;
> +
> +	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
> +	return find_cpio_data((char *)__pa_nodebug(ucode_path),
> +			(void *)boot_params_p->hdr.ramdisk_image,
> +			boot_params_p->hdr.ramdisk_size, &offset);
> +#else
> +	return request_firmware_in_initrd();
> +#endif
> +}
> +
> +static int __init
> +find_equiv_cpu_table_early(const u8 *buf, struct equiv_cpu_entry **eq)
> +{
> +	unsigned int *ibuf = (unsigned int *)buf;
> +	unsigned int type = ibuf[1];
> +	unsigned int size = ibuf[2];
> +
> +	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size)
> +		return -EINVAL;
> +
> +	/*
> +	 * during BSP load, vmalloc() is not available yet.
> +	 * so just use equivalent cpu table in initrd memory in place,
> +	 * no need to copy it. on X86_64, first AP to load will actually
> +	 * "install" the equiv_cpu_table. on X86_32, before mm frees up
> initrd.
> +	 */
> +	*eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
> +
> +	/* add header length */
> +	return size + CONTAINER_HDR_SZ;
> +}
> +
> +static enum ucode_state __init
> +load_microcode_amd_early(int cpu, const u8 *data, size_t size)
> +{
> +	unsigned int leftover;
> +	u8 *fw = (u8 *)data;
> +	int offset;
> +	u16 equiv_id;
> +	struct equiv_cpu_entry *eq;
> +	struct cpuinfo_x86 c;
> +	struct ucode_cpu_info uci;
> +
> +	collect_cpu_info_amd_early(&c, &uci);
> +
> +	offset = find_equiv_cpu_table_early(data, &eq);
> +	if (offset < 0)
> +		return UCODE_ERROR;
> +	fw += offset;
> +	leftover = size - offset;
> +
> +	if (*(u32 *)fw != UCODE_UCODE_TYPE)
> +		return UCODE_ERROR;
> +
> +	equiv_id = _find_equiv_id(eq, &uci);
> +	if (!equiv_id)
> +		return UCODE_NFOUND;
> +
> +	while (leftover) {
> +		struct microcode_amd *mc;
> +		int patch_size = *(u32 *)(fw + 4);
> +
> +		/*
> +		 * during BSP load, vmalloc() is not available yet,
> +		 * so simply find and apply the matching microcode patch in
> +		 * initrd memory in place. on X86_64, first AP to load will
> +		 * actually "cache" the patches in kernel memory.
> +		 */
> +		mc = (struct microcode_amd *)(fw + SECTION_HDR_SIZE);
> +		if (equiv_id == mc->hdr.processor_rev_id)
> +			if (_apply_microcode_amd(cpu, mc, &c, &uci, true) ==
> 0)
> +				break;
> +
> +		offset    = patch_size + SECTION_HDR_SIZE;
> +		fw	 += offset;
> +		leftover -= offset;
> +	}
> +
> +	return UCODE_OK;
> +}
> +
> +static void collect_cpu_info_amd_early_bsp(void *arg)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpuinfo_x86 dummy;
> +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> +	collect_cpu_info_amd_early(&dummy, uci);
> +}
> +
> +int __init save_microcode_in_initrd_amd(void)
> +{
> +	struct cpio_data cd;
> +	struct firmware fw;
> +	struct ucode_cpu_info *uci = ucode_cpu_info +
> boot_cpu_data.cpu_index;
> +
> +	if (equiv_cpu_table)
> +		return 0;
> +
> +	cd = request_firmware_in_initrd();
> +	if (!cd.data)
> +		return -EINVAL;
> +
> +	fw.data = cd.data;
> +	fw.size = cd.size;
> +
> +	if (!uci->cpu_sig.sig)
> +		smp_call_function_single(boot_cpu_data.cpu_index,
> +					 collect_cpu_info_amd_early_bsp, NULL,
> +					 1);
> +
> +	if (request_microcode_fw(boot_cpu_data.cpu_index, &fw) !=
> UCODE_OK)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +void __init load_ucode_amd_bsp(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpio_data fw = request_firmware_in_initrd_early();
> +
> +	if (!fw.data)
> +		return;
> +
> +	load_microcode_amd_early(cpu, fw.data, fw.size);
> +}
> +
> +#ifdef CONFIG_X86_32
> +/*
> + * on X86_32 AP load, since paging is turned off and vmalloc() is not
> available
> + * yet, we cannot install the equivalent cpu table nor cache the
> microcode
> + * patches in kernel memory, so just take the BSP code path. unless we
> are
> + * CPU hotplugging on (i.e. resume from suspend), which then we will
> load from
> + * bsp_mpb instead.
> + */
> +void __cpuinit load_ucode_amd_ap(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct microcode_amd *mc;
> +	struct cpuinfo_x86 c;
> +	struct ucode_cpu_info uci;
> +
> +	mc = (struct microcode_amd *)__pa_nodebug(bsp_mpb);
> +	if (mc->hdr.patch_id && mc->hdr.processor_rev_id)
> +		_apply_microcode_amd(cpu, mc, &c, &uci, true);
> +	else
> +		load_ucode_amd_bsp();
> +}
> +#else /* !CONFIG_X86_32 */
> +/*
> + * on X86_64 AP load, we can vmalloc(). we can go through the normal
> (non-early)
> + * code path, we just have to make sure we prepare cpu_data and
> ucode_cpu_info.
> + */
> +void __cpuinit load_ucode_amd_ap(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +
> +	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
> +
> +	if (cpu && !equiv_cpu_table)
> +		if (save_microcode_in_initrd_amd())
> +			return;
> +
> +	apply_microcode_amd(cpu);
> +}
> +#endif /* end CONFIG_X86_32 */
> +#endif /* end CONFIG_MICROCODE_AMD_EARLY */
> +
>  struct microcode_ops * __init init_amd_microcode(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(0);
> diff --git a/arch/x86/kernel/microcode_core_early.c
> b/arch/x86/kernel/microcode_core_early.c
> index 0d19ac5..1221c22 100644
> --- a/arch/x86/kernel/microcode_core_early.c
> +++ b/arch/x86/kernel/microcode_core_early.c
> @@ -18,6 +18,7 @@
>   */
>  #include <linux/module.h>
>  #include <asm/microcode_intel.h>
> +#include <asm/microcode_amd.h>
>  #include <asm/processor.h>
> 
>  #define QCHAR(a, b, c, d) ((a) + ((b) << 8) + ((c) << 16) + ((d) <<
> 24))
> @@ -83,6 +84,8 @@ void __init load_ucode_bsp(void)
> 
>  	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
>  		load_ucode_intel_bsp();
> +	else if (vendor == X86_VENDOR_AMD && x86 >= 0x10)
> +		load_ucode_amd_bsp();
>  }
> 
>  void __cpuinit load_ucode_ap(void)
> @@ -97,6 +100,8 @@ void __cpuinit load_ucode_ap(void)
> 
>  	if (vendor == X86_VENDOR_INTEL && x86 >= 6)
>  		load_ucode_intel_ap();
> +	else if (vendor == X86_VENDOR_AMD && x86 >= 0x10)
> +		load_ucode_amd_ap();
>  }
> 
>  int __init save_microcode_in_initrd(void)
> @@ -105,6 +110,8 @@ int __init save_microcode_in_initrd(void)
> 
>  	if (c->x86_vendor == X86_VENDOR_INTEL && c->x86 >= 6)
>  		return save_microcode_in_initrd_intel();
> +	else if (c->x86_vendor == X86_VENDOR_AMD && c->x86 >= 0x10)
> +		return save_microcode_in_initrd_amd();
> 
>  	return 0;
>  }
> --
> 1.7.9.5
> 

Re: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

On Fri, May 24, 2013 at 03:33:38PM +0000, Yu, Fenghua wrote:
> > From: Jacob Shin [mailto:jacob.shin@amd.com]
> > Add support for early microcode patch loading on AMD.
> > 
> > Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> > ---
> >  arch/x86/Kconfig                       |   16 +-
> >  arch/x86/include/asm/microcode.h       |    1 -
> >  arch/x86/include/asm/microcode_amd.h   |   17 ++
> >  arch/x86/include/asm/microcode_intel.h |    1 +
> >  arch/x86/kernel/microcode_amd.c        |  338
> > ++++++++++++++++++++++++++++----
> >  arch/x86/kernel/microcode_core_early.c |    7 +
> >  6 files changed, 333 insertions(+), 47 deletions(-)
> >  create mode 100644 arch/x86/include/asm/microcode_amd.h
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 3a5bced..fab72e7 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
> >  	depends on MICROCODE_INTEL
> > 
> >  config MICROCODE_INTEL_EARLY
> > -	bool "Early load microcode"
> > +	def_bool n
> >  	depends on MICROCODE_INTEL && BLK_DEV_INITRD
> > +
> > +config MICROCODE_AMD_EARLY
> > +	def_bool n
> > +	depends on MICROCODE_AMD && BLK_DEV_INITRD
> > +
> > +config MICROCODE_EARLY
> > +	bool "Early load microcode"
> > +	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
> > +	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
> > +	select MICROCODE_AMD_EARLY if MICROCODE_AMD
> >  	default y
> >  	help
> >  	  This option provides functionality to read additional microcode
> > data
> > @@ -1099,10 +1109,6 @@ config MICROCODE_INTEL_EARLY
> >  	  microcode to CPU's as early as possible. No functional change
> > if no
> >  	  microcode data is glued to the initrd, therefore it's safe to
> > say Y.
> > 
> > -config MICROCODE_EARLY
> > -	def_bool y
> > -	depends on MICROCODE_INTEL_EARLY
> > -
> >  config X86_MSR
> >  	tristate "/dev/cpu/*/msr - Model-specific register support"
> >  	---help---
> > diff --git a/arch/x86/include/asm/microcode.h
> > b/arch/x86/include/asm/microcode.h
> > index 6825e2e..f4be4cc 100644
> > --- a/arch/x86/include/asm/microcode.h
> > +++ b/arch/x86/include/asm/microcode.h
> > @@ -58,7 +58,6 @@ static inline void __exit exit_amd_microcode(void) {}
> >  #endif
> > 
> >  #ifdef CONFIG_MICROCODE_EARLY
> > -#define MAX_UCODE_COUNT 128
> >  extern void __init load_ucode_bsp(void);
> >  extern __init void load_ucode_ap(void);
> >  extern int __init save_microcode_in_initrd(void);
> > diff --git a/arch/x86/include/asm/microcode_amd.h
> > b/arch/x86/include/asm/microcode_amd.h
> > new file mode 100644
> > index 0000000..376123c
> > --- /dev/null
> > +++ b/arch/x86/include/asm/microcode_amd.h
> > @@ -0,0 +1,17 @@
> > +#ifndef _ASM_X86_MICROCODE_AMD_H
> > +#define _ASM_X86_MICROCODE_AMD_H
> > +
> > +#ifdef CONFIG_MICROCODE_AMD_EARLY
> > +extern void __init load_ucode_amd_bsp(void);
> > +extern void __cpuinit load_ucode_amd_ap(void);
> > +extern int __init save_microcode_in_initrd_amd(void);
> > +#else
> > +static inline void __init load_ucode_amd_bsp(void) {}
> > +static inline void __cpuinit load_ucode_amd_ap(void) {}
> > +static inline int __init save_microcode_in_initrd_amd(void)
> > +{
> > +	return -EINVAL;
> > +}
> > +#endif
> > +
> > +#endif /* _ASM_X86_MICROCODE_AMD_H */
> > diff --git a/arch/x86/include/asm/microcode_intel.h
> > b/arch/x86/include/asm/microcode_intel.h
> > index 538e407..db4a2f9 100644
> > --- a/arch/x86/include/asm/microcode_intel.h
> > +++ b/arch/x86/include/asm/microcode_intel.h
> > @@ -64,6 +64,7 @@ extern int
> >  update_match_revision(struct microcode_header_intel *mc_header, int
> > rev);
> > 
> >  #ifdef CONFIG_MICROCODE_INTEL_EARLY
> > +#define MAX_UCODE_COUNT 128
> >  extern void __init load_ucode_intel_bsp(void);
> >  extern void __cpuinit load_ucode_intel_ap(void);
> >  extern void show_ucode_info_early(void);
> > diff --git a/arch/x86/kernel/microcode_amd.c
> > b/arch/x86/kernel/microcode_amd.c
> > index efdec7c..cda647e 100644
> > --- a/arch/x86/kernel/microcode_amd.c
> > +++ b/arch/x86/kernel/microcode_amd.c
> > @@ -27,10 +27,13 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > +#include <linux/earlycpio.h>
> > 
> >  #include <asm/microcode.h>
> > +#include <asm/microcode_amd.h>
> >  #include <asm/processor.h>
> >  #include <asm/msr.h>
> > +#include <asm/setup.h>
> > 
> >  MODULE_DESCRIPTION("AMD Microcode Update Driver");
> >  MODULE_AUTHOR("Peter Oruba");
> > @@ -84,23 +87,28 @@ struct ucode_patch {
> > 
> >  static LIST_HEAD(pcache);
> > 
> > -static u16 find_equiv_id(unsigned int cpu)
> > +static u16 _find_equiv_id(struct equiv_cpu_entry *eq,
> > +			  struct ucode_cpu_info *uci)
> >  {
> > -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> >  	int i = 0;
> > 
> > -	if (!equiv_cpu_table)
> > +	if (!eq)
> >  		return 0;
> > 
> > -	while (equiv_cpu_table[i].installed_cpu != 0) {
> > -		if (uci->cpu_sig.sig == equiv_cpu_table[i].installed_cpu)
> > -			return equiv_cpu_table[i].equiv_cpu;
> > +	while (eq[i].installed_cpu != 0) {
> > +		if (uci->cpu_sig.sig == eq[i].installed_cpu)
> > +			return eq[i].equiv_cpu;
> > 
> >  		i++;
> >  	}
> >  	return 0;
> >  }
> > 
> > +static u16 find_equiv_id(unsigned int cpu)
> > +{
> > +	return _find_equiv_id(equiv_cpu_table, ucode_cpu_info + cpu);
> > +}
> > +
> >  static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
> >  {
> >  	int i = 0;
> > @@ -173,9 +181,17 @@ static struct ucode_patch *find_patch(unsigned int
> > cpu)
> >  static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
> >  {
> >  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> > +	struct ucode_patch *p;
> > 
> >  	csig->sig = cpuid_eax(0x00000001);
> >  	csig->rev = c->microcode;
> > +
> > +	/* if a patch was early loaded, tell microcode_core.c about it */
> > +	p = find_patch(cpu);
> > +	if (p && (p->patch_id == csig->rev))
> > +		uci->mc = p->data;
> > +
> >  	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
> > 
> >  	return 0;
> > @@ -215,24 +231,14 @@ static unsigned int verify_patch_size(int cpu,
> > u32 patch_size,
> >  	return patch_size;
> >  }
> > 
> > -static int apply_microcode_amd(int cpu)
> > +static int _apply_microcode_amd(int cpu, void *data, struct
> > cpuinfo_x86 *c,
> > +				struct ucode_cpu_info *uci, bool silent)
> >  {
> > -	struct cpuinfo_x86 *c = &cpu_data(cpu);
> >  	struct microcode_amd *mc_amd;
> > -	struct ucode_cpu_info *uci;
> > -	struct ucode_patch *p;
> >  	u32 rev, dummy;
> > 
> > -	BUG_ON(raw_smp_processor_id() != cpu);
> > -
> > -	uci = ucode_cpu_info + cpu;
> > -
> > -	p = find_patch(cpu);
> > -	if (!p)
> > -		return 0;
> > -
> > -	mc_amd  = p->data;
> > -	uci->mc = p->data;
> > +	mc_amd  = data;
> > +	uci->mc = data;
> > 
> >  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> > 
> > @@ -247,18 +253,37 @@ static int apply_microcode_amd(int cpu)
> >  	/* verify patch application was successful */
> >  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> >  	if (rev != mc_amd->hdr.patch_id) {
> > -		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> > -		       cpu, mc_amd->hdr.patch_id);
> > +		if (!silent)
> > +			pr_err("CPU%d: update failed for
> > patch_level=0x%08x\n",
> > +				cpu, mc_amd->hdr.patch_id);
> >  		return -1;
> >  	}
> > 
> > -	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> > +	if (!silent)
> > +		pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> 
> In 32-bit, this info won't be printed. Then microcode is silently updated. This may make it difficult for user to catch new microcode update info. And this is not consistent to 64-bit which prints the info.
> 
> You might print this info later when printk is usable like Intel ucode does.

Okay, will do.

> 
> >  	uci->cpu_sig.rev = rev;
> >  	c->microcode = rev;
> > 
> >  	return 0;
> >  }
> > 
> > +static int apply_microcode_amd(int cpu)
> 
> This apply_microcode_amd(int cpu) can be simplified as apply_microcode_amd(void).

Actually, this function is part of the microcode_ops (non-early
loading called by microcode_core.c) so I cannot change the function
parameters.

Thanks!

Re: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Borislav Petkov]

On Thu, May 23, 2013 at 10:40:18AM -0500, Jacob Shin wrote:
> Add support for early microcode patch loading on AMD.
> 
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  arch/x86/Kconfig                       |   16 +-
>  arch/x86/include/asm/microcode.h       |    1 -
>  arch/x86/include/asm/microcode_amd.h   |   17 ++
>  arch/x86/include/asm/microcode_intel.h |    1 +
>  arch/x86/kernel/microcode_amd.c        |  338 ++++++++++++++++++++++++++++----
>  arch/x86/kernel/microcode_core_early.c |    7 +
>  6 files changed, 333 insertions(+), 47 deletions(-)
>  create mode 100644 arch/x86/include/asm/microcode_amd.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3a5bced..fab72e7 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
>  	depends on MICROCODE_INTEL
>  
>  config MICROCODE_INTEL_EARLY
> -	bool "Early load microcode"
> +	def_bool n
>  	depends on MICROCODE_INTEL && BLK_DEV_INITRD
> +
> +config MICROCODE_AMD_EARLY
> +	def_bool n
> +	depends on MICROCODE_AMD && BLK_DEV_INITRD
> +
> +config MICROCODE_EARLY
> +	bool "Early load microcode"
> +	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
> +	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
> +	select MICROCODE_AMD_EARLY if MICROCODE_AMD
>  	default y
>  	help
>  	  This option provides functionality to read additional microcode data

This whole microcode Kconfig game seems kinda too granulary to me.
I mean, distros will enable all of them anyway (both AMD and Intel
microcode loading) and since we can safely enable early loading even if
there's no microcode in the initrd, we can add in the early code too.

So how about simplifying this a lot by having only:

config MICROCODE
        tristate "CPU microcode loading support"
        select FW_LOADER

config MICROCODE_EARLY
	depends on BLK_DEV_INITRD && MICROCODE

and drop all this vendor differentiation. Microcode core code checks
vendors so it won't be loaded on anything unsupported, etc.

hpa, Ingo, what do you guys think?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Borislav Petkov]

On Thu, May 23, 2013 at 10:40:18AM -0500, Jacob Shin wrote:
> Add support for early microcode patch loading on AMD.
> 
> Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> ---
>  arch/x86/Kconfig                       |   16 +-
>  arch/x86/include/asm/microcode.h       |    1 -
>  arch/x86/include/asm/microcode_amd.h   |   17 ++
>  arch/x86/include/asm/microcode_intel.h |    1 +
>  arch/x86/kernel/microcode_amd.c        |  338 ++++++++++++++++++++++++++++----
>  arch/x86/kernel/microcode_core_early.c |    7 +
>  6 files changed, 333 insertions(+), 47 deletions(-)
>  create mode 100644 arch/x86/include/asm/microcode_amd.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3a5bced..fab72e7 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
>  	depends on MICROCODE_INTEL
>  
>  config MICROCODE_INTEL_EARLY
> -	bool "Early load microcode"
> +	def_bool n

Why? We want to load ucode early by default.

>  	depends on MICROCODE_INTEL && BLK_DEV_INITRD
> +
> +config MICROCODE_AMD_EARLY
> +	def_bool n

ditto.

> +	depends on MICROCODE_AMD && BLK_DEV_INITRD
> +
> +config MICROCODE_EARLY
> +	bool "Early load microcode"
> +	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
> +	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
> +	select MICROCODE_AMD_EARLY if MICROCODE_AMD

It looks to me the BLK_DEV_INITRD dependency should be moved here and the
respective early loading code should be dependent on the cpu support...

Crap, this whole microcode Kconfig menu needs a bit of scrubbing.

Anyway, this whole Kconfig microcode section could use a simplification
but this is not the subject of this patchset - I'll try to address it
after this, as stated in another mail.

[ … ]

> diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
> index efdec7c..cda647e 100644
> --- a/arch/x86/kernel/microcode_amd.c
> +++ b/arch/x86/kernel/microcode_amd.c
> @@ -27,10 +27,13 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
> +#include <linux/earlycpio.h>

Looks like this include is not needed.

>  #include <asm/microcode.h>
> +#include <asm/microcode_amd.h>
>  #include <asm/processor.h>
>  #include <asm/msr.h>
> +#include <asm/setup.h>

ditto.

At least I can't seem to trigger any build failures when I comment them
out.

>  MODULE_DESCRIPTION("AMD Microcode Update Driver");
>  MODULE_AUTHOR("Peter Oruba");
> @@ -84,23 +87,28 @@ struct ucode_patch {
>  
>  static LIST_HEAD(pcache);
>  
> -static u16 find_equiv_id(unsigned int cpu)
> +static u16 _find_equiv_id(struct equiv_cpu_entry *eq,
> +			  struct ucode_cpu_info *uci)
>  {
> -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
>  	int i = 0;
>  
> -	if (!equiv_cpu_table)
> +	if (!eq)
>  		return 0;
>  
> -	while (equiv_cpu_table[i].installed_cpu != 0) {
> -		if (uci->cpu_sig.sig == equiv_cpu_table[i].installed_cpu)
> -			return equiv_cpu_table[i].equiv_cpu;
> +	while (eq[i].installed_cpu != 0) {
> +		if (uci->cpu_sig.sig == eq[i].installed_cpu)
> +			return eq[i].equiv_cpu;
>  
>  		i++;
>  	}
>  	return 0;
>  }
>  
> +static u16 find_equiv_id(unsigned int cpu)
> +{
> +	return _find_equiv_id(equiv_cpu_table, ucode_cpu_info + cpu);
> +}
> +
>  static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
>  {
>  	int i = 0;
> @@ -173,9 +181,17 @@ static struct ucode_patch *find_patch(unsigned int cpu)
>  static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> +	struct ucode_patch *p;
>  
>  	csig->sig = cpuid_eax(0x00000001);
>  	csig->rev = c->microcode;
> +
> +	/* if a patch was early loaded, tell microcode_core.c about it */
> +	p = find_patch(cpu);
> +	if (p && (p->patch_id == csig->rev))
> +		uci->mc = p->data;

Why do we even need this? Hotplug should take care of reappying
microcode on all cores.

>  	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
>  
>  	return 0;
> @@ -215,24 +231,14 @@ static unsigned int verify_patch_size(int cpu, u32 patch_size,
>  	return patch_size;
>  }
>  
> -static int apply_microcode_amd(int cpu)
> +static int _apply_microcode_amd(int cpu, void *data, struct cpuinfo_x86 *c,
> +				struct ucode_cpu_info *uci, bool silent)
>  {
> -	struct cpuinfo_x86 *c = &cpu_data(cpu);
>  	struct microcode_amd *mc_amd;
> -	struct ucode_cpu_info *uci;
> -	struct ucode_patch *p;
>  	u32 rev, dummy;
>  
> -	BUG_ON(raw_smp_processor_id() != cpu);
> -
> -	uci = ucode_cpu_info + cpu;
> -
> -	p = find_patch(cpu);
> -	if (!p)
> -		return 0;
> -
> -	mc_amd  = p->data;
> -	uci->mc = p->data;
> +	mc_amd  = data;
> +	uci->mc = data;

Right, come to think of it, we don't use this uci->mc thing anywhere
except in mc_bp_resume. But even there, we call ->apply_microcode()
which on AMD searches the patch in the patch cache so no need for this
assignment at all.

>  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
>  
> @@ -247,18 +253,37 @@ static int apply_microcode_amd(int cpu)
>  	/* verify patch application was successful */
>  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
>  	if (rev != mc_amd->hdr.patch_id) {
> -		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> -		       cpu, mc_amd->hdr.patch_id);
> +		if (!silent)
> +			pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> +				cpu, mc_amd->hdr.patch_id);
>  		return -1;
>  	}
>  
> -	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> +	if (!silent)
> +		pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
>  	uci->cpu_sig.rev = rev;
>  	c->microcode = rev;
>  
>  	return 0;
>  }
>  
> +static int apply_microcode_amd(int cpu)
> +{
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	struct ucode_cpu_info *uci;
> +	struct ucode_patch *p;
> +
> +	BUG_ON(raw_smp_processor_id() != cpu);
> +
> +	uci = ucode_cpu_info + cpu;
> +
> +	p = find_patch(cpu);
> +	if (!p)
> +		return 0;
> +
> +	return _apply_microcode_amd(cpu, p->data, c, uci, false);
> +}
> +
>  static int install_equiv_cpu_table(const u8 *buf)
>  {
>  	unsigned int *ibuf = (unsigned int *)buf;
> @@ -398,6 +423,44 @@ static enum ucode_state load_microcode_amd(int cpu, const u8 *data, size_t size)
>  	return UCODE_OK;
>  }
>  
> +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> +#define MPB_MAX_SIZE F15H_MPB_MAX_SIZE

Is this an obscure way to say "I want the biggest patch size required,
thus I'm taking the 4K size of F15h?

Please make it explicit:

#define MPB_MAX_SIZE PAGE_SIZE

If some future family decided it needs bigger microcode, then we can
adjust it.

> +static u8 bsp_mpb[MPB_MAX_SIZE];
> +#endif
> +static enum ucode_state request_microcode_fw(int cpu, const struct firmware *fw)

This function has the same name as one of the members in the
microcode_ops. Misleading. Remember, this is code to be stared at by
humans not be simply virtual addresses jumped to by a prefetcher.

> +{
> +	enum ucode_state ret = UCODE_ERROR;
> +
> +	if (*(u32 *)fw->data != UCODE_MAGIC) {
> +		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> +		return ret;
> +	}

AFAICT, this function can be called early and we want to avoid printk
there...

> +
> +	/* free old equiv table */
> +	free_equiv_cpu_table();

Huh, we use the equiv table in the initrd early, we can't free it.

> +
> +	ret = load_microcode_amd(cpu, fw->data, fw->size);
> +	if (ret != UCODE_OK)
> +		cleanup();
> +
> +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> +	/*
> +	 * on X86_32 early load, while CPU hotplugging on, we cannot traverse
> +	 * pcache since paging is not turneded on yet. so stash away BSP's MPB
> +	 * when a new fw file is installed.
> +	 */

This comment needs a bit of scrubbing, spell- and readability check.

And it says we cannot traverse the pcache and yet we traverse it with
find_patch().

???

> +	if (cpu_data(cpu).cpu_index == boot_cpu_data.cpu_index) {
> +		struct ucode_patch *p;
> +
> +		p = find_patch(cpu);
> +		if (p)
> +			memcpy(bsp_mpb, p->data, min_t(u32, ksize(p->data),
> +						       MPB_MAX_SIZE));
> +	}
> +#endif
> +	return ret;
> +}
> +
>  /*
>   * AMD microcode firmware naming convention, up to family 15h they are in
>   * the legacy file:
> @@ -419,7 +482,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
>  {
>  	char fw_name[36] = "amd-ucode/microcode_amd.bin";
>  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> -	enum ucode_state ret = UCODE_NFOUND;
> +	enum ucode_state ret;
>  	const struct firmware *fw;
>  
>  	/* reload ucode container only on the boot cpu */
> @@ -431,26 +494,11 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
>  
>  	if (request_firmware(&fw, (const char *)fw_name, device)) {
>  		pr_err("failed to load file %s\n", fw_name);
> -		goto out;
> +		return UCODE_NFOUND;
>  	}
>  
> -	ret = UCODE_ERROR;
> -	if (*(u32 *)fw->data != UCODE_MAGIC) {
> -		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> -		goto fw_release;
> -	}
> -
> -	/* free old equiv table */
> -	free_equiv_cpu_table();
> -
> -	ret = load_microcode_amd(cpu, fw->data, fw->size);
> -	if (ret != UCODE_OK)
> -		cleanup();
> -
> - fw_release:
> +	ret = request_microcode_fw(cpu, fw);
>  	release_firmware(fw);
> -
> - out:
>  	return ret;
>  }
>  
> @@ -475,6 +523,214 @@ static struct microcode_ops microcode_amd_ops = {
>  	.microcode_fini_cpu               = microcode_fini_cpu_amd,
>  };
>  
> +#ifdef CONFIG_MICROCODE_AMD_EARLY
> +/*
> + * Early Loading Support
> + */
> +
> +static void __cpuinit collect_cpu_info_amd_early(struct cpuinfo_x86 *c,
> +						 struct ucode_cpu_info *uci)
> +{
> +	u32 rev, eax;
> +
> +	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, eax);
> +	eax = cpuid_eax(0x00000001);
> +
> +	uci->cpu_sig.sig = eax;
> +	uci->cpu_sig.rev = rev;
> +	c->microcode = rev;
> +	c->x86 = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
> +}
> +
> +/*
> + * microcode patch container file is prepended to the initrd in cpio format.
> + * see Documentation/x86/early-microcode.txt
> + */
> +static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
> +
> +static struct cpio_data __init
> +request_firmware_in_initrd(void)

This function naming is wrong - we're not requesting but we're searching
for the ucode blob in the initrd. So "find_ucode_in_initrd()" would be a
more fitting name.

> +{
> +	long offset = 0;
> +
> +	return find_cpio_data(ucode_path,
> +			(void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET),
> +			boot_params.hdr.ramdisk_size, &offset);
> +}
> +
> +static struct cpio_data __init request_firmware_in_initrd_early(void)

ditto.

> +{
> +#ifdef CONFIG_X86_32

Now this could well use a comment about why we're using physical
addresses on 32-bit.

> +	struct boot_params *boot_params_p;
> +	long offset = 0;
> +
> +	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
> +	return find_cpio_data((char *)__pa_nodebug(ucode_path),
> +			(void *)boot_params_p->hdr.ramdisk_image,
> +			boot_params_p->hdr.ramdisk_size, &offset);
> +#else
> +	return request_firmware_in_initrd();
> +#endif
> +}
> +
> +static int __init
> +find_equiv_cpu_table_early(const u8 *buf, struct equiv_cpu_entry **eq)
> +{
> +	unsigned int *ibuf = (unsigned int *)buf;
> +	unsigned int type = ibuf[1];
> +	unsigned int size = ibuf[2];
> +
> +	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size)
> +		return -EINVAL;
> +
> +	/*
> +	 * during BSP load, vmalloc() is not available yet.
> +	 * so just use equivalent cpu table in initrd memory in place,
> +	 * no need to copy it. on X86_64, first AP to load will actually
> +	 * "install" the equiv_cpu_table. on X86_32, before mm frees up initrd.
> +	 */

This comment needs scrubbing like block formatting, converting it into
real sentences and actually trying to explain what do you mean with that
last part "on X86_32..." - I can't read that. IOW, something like that:

        /*
         * vmalloc() is not available early, while the microcode is loaded on
         * the BSP. So just use the equivalent cpu table in the initrd, without
         * copying it. On x86_64, the first AP to load will actually "install"
	 * the equiv_cpu_table while on X86_32...
         */

> +	*eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
> +
> +	/* add header length */

No need for it - CONTAINER_HDR_SZ should be descriptive enough a name.

> +	return size + CONTAINER_HDR_SZ;
> +}
> +
> +static enum ucode_state __init
> +load_microcode_amd_early(int cpu, const u8 *data, size_t size)

Btw, this function is called only from load_ucode_amd_bsp, so call it
accordingly: __load_ucode_amd_bsp() or so.

> +{
> +	unsigned int leftover;
> +	u8 *fw = (u8 *)data;
> +	int offset;
> +	u16 equiv_id;
> +	struct equiv_cpu_entry *eq;
> +	struct cpuinfo_x86 c;

This looks wrong - so you're creating a variable on the stack just so
that you can pass it to _apply_microcode_amd(). After the function
finishes, all the assignments to it are gone.

Looks like bad design to me.

> +	struct ucode_cpu_info uci;
> +
> +	collect_cpu_info_amd_early(&c, &uci);
> +
> +	offset = find_equiv_cpu_table_early(data, &eq);
> +	if (offset < 0)
> +		return UCODE_ERROR;
> +	fw += offset;
> +	leftover = size - offset;
> +
> +	if (*(u32 *)fw != UCODE_UCODE_TYPE)
> +		return UCODE_ERROR;
> +
> +	equiv_id = _find_equiv_id(eq, &uci);
> +	if (!equiv_id)
> +		return UCODE_NFOUND;
> +
> +	while (leftover) {
> +		struct microcode_amd *mc;
> +		int patch_size = *(u32 *)(fw + 4);
> +
> +		/*
> +		 * during BSP load, vmalloc() is not available yet,
> +		 * so simply find and apply the matching microcode patch in
> +		 * initrd memory in place. on X86_64, first AP to load will
> +		 * actually "cache" the patches in kernel memory.
> +		 */
> +		mc = (struct microcode_amd *)(fw + SECTION_HDR_SIZE);
> +		if (equiv_id == mc->hdr.processor_rev_id)
> +			if (_apply_microcode_amd(cpu, mc, &c, &uci, true) == 0)

			if (!_apply...())

> +				break;
> +
> +		offset    = patch_size + SECTION_HDR_SIZE;
> +		fw	 += offset;
> +		leftover -= offset;
> +	}
> +
> +	return UCODE_OK;
> +}
> +
> +static void collect_cpu_info_amd_early_bsp(void *arg)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpuinfo_x86 dummy;
> +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> +	collect_cpu_info_amd_early(&dummy, uci);
> +}
> +
> +int __init save_microcode_in_initrd_amd(void)

Ok, I don't understand:

We do all the collect_cpu_info and microcode loading both on the BSP and
the APs. Why do we need to do that here too?

This is called later from save_microcode_in_initrd() which is an
initcall but by that time microcode has been applied already everywhere.
So why do it here too? I'm guessing so that you can stash away the
ucode.

Also, this function is called on the AP path if we don't have an
equiv_table. Sounds to me, the BSP could do this work for the remaining
APs and they could simply apply the patches.

> +{
> +	struct cpio_data cd;
> +	struct firmware fw;
> +	struct ucode_cpu_info *uci = ucode_cpu_info + boot_cpu_data.cpu_index;
> +
> +	if (equiv_cpu_table)
> +		return 0;
> +
> +	cd = request_firmware_in_initrd();
> +	if (!cd.data)
> +		return -EINVAL;
> +
> +	fw.data = cd.data;
> +	fw.size = cd.size;
> +
> +	if (!uci->cpu_sig.sig)
> +		smp_call_function_single(boot_cpu_data.cpu_index,
> +					 collect_cpu_info_amd_early_bsp, NULL,
> +					 1);
> +
> +	if (request_microcode_fw(boot_cpu_data.cpu_index, &fw) != UCODE_OK)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +void __init load_ucode_amd_bsp(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct cpio_data fw = request_firmware_in_initrd_early();
> +
> +	if (!fw.data)
> +		return;
> +
> +	load_microcode_amd_early(cpu, fw.data, fw.size);
> +}
> +
> +#ifdef CONFIG_X86_32
> +/*
> + * on X86_32 AP load, since paging is turned off and vmalloc() is not available
> + * yet, we cannot install the equivalent cpu table nor cache the microcode
> + * patches in kernel memory, so just take the BSP code path. unless we are
> + * CPU hotplugging on (i.e. resume from suspend), which then we will load from
> + * bsp_mpb instead.
> + */
> +void __cpuinit load_ucode_amd_ap(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +	struct microcode_amd *mc;
> +	struct cpuinfo_x86 c;
> +	struct ucode_cpu_info uci;
> +
> +	mc = (struct microcode_amd *)__pa_nodebug(bsp_mpb);
> +	if (mc->hdr.patch_id && mc->hdr.processor_rev_id)
> +		_apply_microcode_amd(cpu, mc, &c, &uci, true);
> +	else
> +		load_ucode_amd_bsp();

What's going on here? The _ap() function does load...bsp()?

This whole code looks really convoluted to me and I'm going to stop
trying to understand what's going on. Please do a careful analysis of
what calls what, when. Then, carve out shared functionality carefully
into a function and call that function exactly what it does.

AFAICT, from reading the code, there a couple of things you need to do
(correct me if I'm missing anything):

* load ucode on the BSP

* load ucode on the APs

* save the ucode image from initrd

But you have functions like _apply_microcode_amd where just so that you
can reuse functionality, you create the passing arguments on the stack
just so that it works. And this is not optimal.

A better design, IHMO, would be to carve out *only* the application
functionality, i.e. the RD/WRMSR dance and call it __apply_microcode
which you can call from the early code. The rest of the glue should go
in another function which actually saves c->microcode, etc.

This makes it very hard to follow the code and is not a proper design.

And it should be very clear just by looking at the function, when and
how is this function called.

Please add more thought and care when designing this for the next
version.

You could also split this patch so that it is more clear what happens.

Another thing to consider is, for example, which work is done only once
and should thus be done only on the BSP so that the if-clauses on the AP
path can be removed.

Also, avoid repetitive work like save_microcode_in_initrd_amd() which is
done from the initcall and on the APs. I don't think this is needed so
it shouldn't happen.

And so on, and so on.

> +}
> +#else /* !CONFIG_X86_32 */
> +/*
> + * on X86_64 AP load, we can vmalloc(). we can go through the normal (non-early)
> + * code path, we just have to make sure we prepare cpu_data and ucode_cpu_info.
> + */
> +void __cpuinit load_ucode_amd_ap(void)
> +{
> +	unsigned int cpu = smp_processor_id();
> +
> +	collect_cpu_info_amd_early(&cpu_data(cpu), ucode_cpu_info + cpu);
> +
> +	if (cpu && !equiv_cpu_table)
> +		if (save_microcode_in_initrd_amd())
> +			return;
> +
> +	apply_microcode_amd(cpu);
> +}
> +#endif /* end CONFIG_X86_32 */
> +#endif /* end CONFIG_MICROCODE_AMD_EARLY */

No need for the "end" thing.

> +
>  struct microcode_ops * __init init_amd_microcode(void)
>  {
>  	struct cpuinfo_x86 *c = &cpu_data(0);

[ … ]

Thanks.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

On Wed, May 29, 2013 at 12:45:29AM +0200, Borislav Petkov wrote:
> On Thu, May 23, 2013 at 10:40:18AM -0500, Jacob Shin wrote:
> > Add support for early microcode patch loading on AMD.
> > 
> > Signed-off-by: Jacob Shin <jacob.shin@amd.com>
> > ---
> >  arch/x86/Kconfig                       |   16 +-
> >  arch/x86/include/asm/microcode.h       |    1 -
> >  arch/x86/include/asm/microcode_amd.h   |   17 ++
> >  arch/x86/include/asm/microcode_intel.h |    1 +
> >  arch/x86/kernel/microcode_amd.c        |  338 ++++++++++++++++++++++++++++----
> >  arch/x86/kernel/microcode_core_early.c |    7 +
> >  6 files changed, 333 insertions(+), 47 deletions(-)
> >  create mode 100644 arch/x86/include/asm/microcode_amd.h
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 3a5bced..fab72e7 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1090,8 +1090,18 @@ config MICROCODE_INTEL_LIB
> >  	depends on MICROCODE_INTEL
> >  
> >  config MICROCODE_INTEL_EARLY
> > -	bool "Early load microcode"
> > +	def_bool n
> 
> Why? We want to load ucode early by default.
> 
> >  	depends on MICROCODE_INTEL && BLK_DEV_INITRD
> > +
> > +config MICROCODE_AMD_EARLY
> > +	def_bool n
> 
> ditto.
> 
> > +	depends on MICROCODE_AMD && BLK_DEV_INITRD
> > +
> > +config MICROCODE_EARLY
> > +	bool "Early load microcode"
> > +	depends on (MICROCODE_INTEL || MICROCODE_AMD) && BLK_DEV_INITRD
> > +	select MICROCODE_INTEL_EARLY if MICROCODE_INTEL
> > +	select MICROCODE_AMD_EARLY if MICROCODE_AMD
> 
> It looks to me the BLK_DEV_INITRD dependency should be moved here and the
> respective early loading code should be dependent on the cpu support...
> 
> Crap, this whole microcode Kconfig menu needs a bit of scrubbing.
> 
> Anyway, this whole Kconfig microcode section could use a simplification
> but this is not the subject of this patchset - I'll try to address it
> after this, as stated in another mail.

Yes, I'll simplify the Kconfig as you suggested in your ealier email.

> 
> [ … ]
> 
> > diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
> > index efdec7c..cda647e 100644
> > --- a/arch/x86/kernel/microcode_amd.c
> > +++ b/arch/x86/kernel/microcode_amd.c
> > @@ -27,10 +27,13 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/pci.h>
> > +#include <linux/earlycpio.h>
> 
> Looks like this include is not needed.
> 
> >  #include <asm/microcode.h>
> > +#include <asm/microcode_amd.h>
> >  #include <asm/processor.h>
> >  #include <asm/msr.h>
> > +#include <asm/setup.h>
> 
> ditto.
> 
> At least I can't seem to trigger any build failures when I comment them
> out.

Hm.. I couldn't build without it at some point, I'll take them out but they
might be indrectly included anyways.

> 
> >  MODULE_DESCRIPTION("AMD Microcode Update Driver");
> >  MODULE_AUTHOR("Peter Oruba");
> > @@ -84,23 +87,28 @@ struct ucode_patch {
> >  
> >  static LIST_HEAD(pcache);
> >  
> > -static u16 find_equiv_id(unsigned int cpu)
> > +static u16 _find_equiv_id(struct equiv_cpu_entry *eq,
> > +			  struct ucode_cpu_info *uci)
> >  {
> > -	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> >  	int i = 0;
> >  
> > -	if (!equiv_cpu_table)
> > +	if (!eq)
> >  		return 0;
> >  
> > -	while (equiv_cpu_table[i].installed_cpu != 0) {
> > -		if (uci->cpu_sig.sig == equiv_cpu_table[i].installed_cpu)
> > -			return equiv_cpu_table[i].equiv_cpu;
> > +	while (eq[i].installed_cpu != 0) {
> > +		if (uci->cpu_sig.sig == eq[i].installed_cpu)
> > +			return eq[i].equiv_cpu;
> >  
> >  		i++;
> >  	}
> >  	return 0;
> >  }
> >  
> > +static u16 find_equiv_id(unsigned int cpu)
> > +{
> > +	return _find_equiv_id(equiv_cpu_table, ucode_cpu_info + cpu);
> > +}
> > +
> >  static u32 find_cpu_family_by_equiv_cpu(u16 equiv_cpu)
> >  {
> >  	int i = 0;
> > @@ -173,9 +181,17 @@ static struct ucode_patch *find_patch(unsigned int cpu)
> >  static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
> >  {
> >  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> > +	struct ucode_patch *p;
> >  
> >  	csig->sig = cpuid_eax(0x00000001);
> >  	csig->rev = c->microcode;
> > +
> > +	/* if a patch was early loaded, tell microcode_core.c about it */
> > +	p = find_patch(cpu);
> > +	if (p && (p->patch_id == csig->rev))
> > +		uci->mc = p->data;
> 
> Why do we even need this? Hotplug should take care of reappying
> microcode on all cores.

It is needed because otherwise BSP does not reapply microcode on resume, see
microcode_core.c BSP resume code path.

> 
> >  	pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
> >  
> >  	return 0;
> > @@ -215,24 +231,14 @@ static unsigned int verify_patch_size(int cpu, u32 patch_size,
> >  	return patch_size;
> >  }
> >  
> > -static int apply_microcode_amd(int cpu)
> > +static int _apply_microcode_amd(int cpu, void *data, struct cpuinfo_x86 *c,
> > +				struct ucode_cpu_info *uci, bool silent)
> >  {
> > -	struct cpuinfo_x86 *c = &cpu_data(cpu);
> >  	struct microcode_amd *mc_amd;
> > -	struct ucode_cpu_info *uci;
> > -	struct ucode_patch *p;
> >  	u32 rev, dummy;
> >  
> > -	BUG_ON(raw_smp_processor_id() != cpu);
> > -
> > -	uci = ucode_cpu_info + cpu;
> > -
> > -	p = find_patch(cpu);
> > -	if (!p)
> > -		return 0;
> > -
> > -	mc_amd  = p->data;
> > -	uci->mc = p->data;
> > +	mc_amd  = data;
> > +	uci->mc = data;
> 
> Right, come to think of it, we don't use this uci->mc thing anywhere
> except in mc_bp_resume. But even there, we call ->apply_microcode()
> which on AMD searches the patch in the patch cache so no need for this
> assignment at all.

Right, but at least we need a non-zero value, I can stick "1" in there I guess.
Whatever you prefer.

> 
> >  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> >  
> > @@ -247,18 +253,37 @@ static int apply_microcode_amd(int cpu)
> >  	/* verify patch application was successful */
> >  	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
> >  	if (rev != mc_amd->hdr.patch_id) {
> > -		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> > -		       cpu, mc_amd->hdr.patch_id);
> > +		if (!silent)
> > +			pr_err("CPU%d: update failed for patch_level=0x%08x\n",
> > +				cpu, mc_amd->hdr.patch_id);
> >  		return -1;
> >  	}
> >  
> > -	pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> > +	if (!silent)
> > +		pr_info("CPU%d: new patch_level=0x%08x\n", cpu, rev);
> >  	uci->cpu_sig.rev = rev;
> >  	c->microcode = rev;
> >  
> >  	return 0;
> >  }
> >  
> > +static int apply_microcode_amd(int cpu)
> > +{
> > +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> > +	struct ucode_cpu_info *uci;
> > +	struct ucode_patch *p;
> > +
> > +	BUG_ON(raw_smp_processor_id() != cpu);
> > +
> > +	uci = ucode_cpu_info + cpu;
> > +
> > +	p = find_patch(cpu);
> > +	if (!p)
> > +		return 0;
> > +
> > +	return _apply_microcode_amd(cpu, p->data, c, uci, false);
> > +}
> > +
> >  static int install_equiv_cpu_table(const u8 *buf)
> >  {
> >  	unsigned int *ibuf = (unsigned int *)buf;
> > @@ -398,6 +423,44 @@ static enum ucode_state load_microcode_amd(int cpu, const u8 *data, size_t size)
> >  	return UCODE_OK;
> >  }
> >  
> > +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> > +#define MPB_MAX_SIZE F15H_MPB_MAX_SIZE
> 
> Is this an obscure way to say "I want the biggest patch size required,
> thus I'm taking the 4K size of F15h?
> 
> Please make it explicit:
> 
> #define MPB_MAX_SIZE PAGE_SIZE
> 
> If some future family decided it needs bigger microcode, then we can
> adjust it.

Okay will do.

> 
> > +static u8 bsp_mpb[MPB_MAX_SIZE];
> > +#endif
> > +static enum ucode_state request_microcode_fw(int cpu, const struct firmware *fw)
> 
> This function has the same name as one of the members in the
> microcode_ops. Misleading. Remember, this is code to be stared at by
> humans not be simply virtual addresses jumped to by a prefetcher.

Okay, will do.

> 
> > +{
> > +	enum ucode_state ret = UCODE_ERROR;
> > +
> > +	if (*(u32 *)fw->data != UCODE_MAGIC) {
> > +		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> > +		return ret;
> > +	}
> 
> AFAICT, this function can be called early and we want to avoid printk
> there...

You're right, will change.

> 
> > +
> > +	/* free old equiv table */
> > +	free_equiv_cpu_table();
> 
> Huh, we use the equiv table in the initrd early, we can't free it.
> 
> > +
> > +	ret = load_microcode_amd(cpu, fw->data, fw->size);
> > +	if (ret != UCODE_OK)
> > +		cleanup();
> > +
> > +#if defined(CONFIG_MICROCODE_AMD_EARLY) && defined(CONFIG_X86_32)
> > +	/*
> > +	 * on X86_32 early load, while CPU hotplugging on, we cannot traverse
> > +	 * pcache since paging is not turneded on yet. so stash away BSP's MPB
> > +	 * when a new fw file is installed.
> > +	 */
> 
> This comment needs a bit of scrubbing, spell- and readability check.
> 
> And it says we cannot traverse the pcache and yet we traverse it with
> find_patch().
> 
> ???

What I meant was that we can traverse it now, but on 32 bits, later when we
go into suspend and resume, we since we load before paging is turned on, we
cannot traverse through equiv_cpu_table and pcache.

> 
> > +	if (cpu_data(cpu).cpu_index == boot_cpu_data.cpu_index) {
> > +		struct ucode_patch *p;
> > +
> > +		p = find_patch(cpu);
> > +		if (p)
> > +			memcpy(bsp_mpb, p->data, min_t(u32, ksize(p->data),
> > +						       MPB_MAX_SIZE));
> > +	}
> > +#endif
> > +	return ret;
> > +}
> > +
> >  /*
> >   * AMD microcode firmware naming convention, up to family 15h they are in
> >   * the legacy file:
> > @@ -419,7 +482,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
> >  {
> >  	char fw_name[36] = "amd-ucode/microcode_amd.bin";
> >  	struct cpuinfo_x86 *c = &cpu_data(cpu);
> > -	enum ucode_state ret = UCODE_NFOUND;
> > +	enum ucode_state ret;
> >  	const struct firmware *fw;
> >  
> >  	/* reload ucode container only on the boot cpu */
> > @@ -431,26 +494,11 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
> >  
> >  	if (request_firmware(&fw, (const char *)fw_name, device)) {
> >  		pr_err("failed to load file %s\n", fw_name);
> > -		goto out;
> > +		return UCODE_NFOUND;
> >  	}
> >  
> > -	ret = UCODE_ERROR;
> > -	if (*(u32 *)fw->data != UCODE_MAGIC) {
> > -		pr_err("invalid magic value (0x%08x)\n", *(u32 *)fw->data);
> > -		goto fw_release;
> > -	}
> > -
> > -	/* free old equiv table */
> > -	free_equiv_cpu_table();
> > -
> > -	ret = load_microcode_amd(cpu, fw->data, fw->size);
> > -	if (ret != UCODE_OK)
> > -		cleanup();
> > -
> > - fw_release:
> > +	ret = request_microcode_fw(cpu, fw);
> >  	release_firmware(fw);
> > -
> > - out:
> >  	return ret;
> >  }
> >  
> > @@ -475,6 +523,214 @@ static struct microcode_ops microcode_amd_ops = {
> >  	.microcode_fini_cpu               = microcode_fini_cpu_amd,
> >  };
> >  
> > +#ifdef CONFIG_MICROCODE_AMD_EARLY
> > +/*
> > + * Early Loading Support
> > + */
> > +
> > +static void __cpuinit collect_cpu_info_amd_early(struct cpuinfo_x86 *c,
> > +						 struct ucode_cpu_info *uci)
> > +{
> > +	u32 rev, eax;
> > +
> > +	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, eax);
> > +	eax = cpuid_eax(0x00000001);
> > +
> > +	uci->cpu_sig.sig = eax;
> > +	uci->cpu_sig.rev = rev;
> > +	c->microcode = rev;
> > +	c->x86 = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
> > +}
> > +
> > +/*
> > + * microcode patch container file is prepended to the initrd in cpio format.
> > + * see Documentation/x86/early-microcode.txt
> > + */
> > +static __initdata char ucode_path[] = "kernel/x86/microcode/AuthenticAMD.bin";
> > +
> > +static struct cpio_data __init
> > +request_firmware_in_initrd(void)
> 
> This function naming is wrong - we're not requesting but we're searching
> for the ucode blob in the initrd. So "find_ucode_in_initrd()" would be a
> more fitting name.

Okay will change.

> 
> > +{
> > +	long offset = 0;
> > +
> > +	return find_cpio_data(ucode_path,
> > +			(void *)(boot_params.hdr.ramdisk_image + PAGE_OFFSET),
> > +			boot_params.hdr.ramdisk_size, &offset);
> > +}
> > +
> > +static struct cpio_data __init request_firmware_in_initrd_early(void)
> 
> ditto.
> 
> > +{
> > +#ifdef CONFIG_X86_32
> 
> Now this could well use a comment about why we're using physical
> addresses on 32-bit.

Okay.

> 
> > +	struct boot_params *boot_params_p;
> > +	long offset = 0;
> > +
> > +	boot_params_p = (struct boot_params *)__pa_nodebug(&boot_params);
> > +	return find_cpio_data((char *)__pa_nodebug(ucode_path),
> > +			(void *)boot_params_p->hdr.ramdisk_image,
> > +			boot_params_p->hdr.ramdisk_size, &offset);
> > +#else
> > +	return request_firmware_in_initrd();
> > +#endif
> > +}
> > +
> > +static int __init
> > +find_equiv_cpu_table_early(const u8 *buf, struct equiv_cpu_entry **eq)
> > +{
> > +	unsigned int *ibuf = (unsigned int *)buf;
> > +	unsigned int type = ibuf[1];
> > +	unsigned int size = ibuf[2];
> > +
> > +	if (type != UCODE_EQUIV_CPU_TABLE_TYPE || !size)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * during BSP load, vmalloc() is not available yet.
> > +	 * so just use equivalent cpu table in initrd memory in place,
> > +	 * no need to copy it. on X86_64, first AP to load will actually
> > +	 * "install" the equiv_cpu_table. on X86_32, before mm frees up initrd.
> > +	 */
> 
> This comment needs scrubbing like block formatting, converting it into
> real sentences and actually trying to explain what do you mean with that
> last part "on X86_32..." - I can't read that. IOW, something like that:
> 
>         /*
>          * vmalloc() is not available early, while the microcode is loaded on
>          * the BSP. So just use the equivalent cpu table in the initrd, without
>          * copying it. On x86_64, the first AP to load will actually "install"
> 	 * the equiv_cpu_table while on X86_32...
>          */

Okay will try and word it better.

> 
> > +	*eq = (struct equiv_cpu_entry *)(buf + CONTAINER_HDR_SZ);
> > +
> > +	/* add header length */
> 
> No need for it - CONTAINER_HDR_SZ should be descriptive enough a name.
> 
> > +	return size + CONTAINER_HDR_SZ;
> > +}
> > +
> > +static enum ucode_state __init
> > +load_microcode_amd_early(int cpu, const u8 *data, size_t size)
> 
> Btw, this function is called only from load_ucode_amd_bsp, so call it
> accordingly: __load_ucode_amd_bsp() or so.

Okay.

> 
> > +{
> > +	unsigned int leftover;
> > +	u8 *fw = (u8 *)data;
> > +	int offset;
> > +	u16 equiv_id;
> > +	struct equiv_cpu_entry *eq;
> > +	struct cpuinfo_x86 c;
> 
> This looks wrong - so you're creating a variable on the stack just so
> that you can pass it to _apply_microcode_amd(). After the function
> finishes, all the assignments to it are gone.
> 
> Looks like bad design to me.
> 
> > +	struct ucode_cpu_info uci;
> > +
> > +	collect_cpu_info_amd_early(&c, &uci);
> > +
> > +	offset = find_equiv_cpu_table_early(data, &eq);
> > +	if (offset < 0)
> > +		return UCODE_ERROR;
> > +	fw += offset;
> > +	leftover = size - offset;
> > +
> > +	if (*(u32 *)fw != UCODE_UCODE_TYPE)
> > +		return UCODE_ERROR;
> > +
> > +	equiv_id = _find_equiv_id(eq, &uci);
> > +	if (!equiv_id)
> > +		return UCODE_NFOUND;
> > +
> > +	while (leftover) {
> > +		struct microcode_amd *mc;
> > +		int patch_size = *(u32 *)(fw + 4);
> > +
> > +		/*
> > +		 * during BSP load, vmalloc() is not available yet,
> > +		 * so simply find and apply the matching microcode patch in
> > +		 * initrd memory in place. on X86_64, first AP to load will
> > +		 * actually "cache" the patches in kernel memory.
> > +		 */
> > +		mc = (struct microcode_amd *)(fw + SECTION_HDR_SIZE);
> > +		if (equiv_id == mc->hdr.processor_rev_id)
> > +			if (_apply_microcode_amd(cpu, mc, &c, &uci, true) == 0)
> 
> 			if (!_apply...())
> 
> > +				break;
> > +
> > +		offset    = patch_size + SECTION_HDR_SIZE;
> > +		fw	 += offset;
> > +		leftover -= offset;
> > +	}
> > +
> > +	return UCODE_OK;
> > +}
> > +
> > +static void collect_cpu_info_amd_early_bsp(void *arg)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct cpuinfo_x86 dummy;
> > +	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> > +	collect_cpu_info_amd_early(&dummy, uci);
> > +}
> > +
> > +int __init save_microcode_in_initrd_amd(void)
> 
> Ok, I don't understand:
> 
> We do all the collect_cpu_info and microcode loading both on the BSP and
> the APs. Why do we need to do that here too?
> 
> This is called later from save_microcode_in_initrd() which is an
> initcall but by that time microcode has been applied already everywhere.
> So why do it here too? I'm guessing so that you can stash away the
> ucode.
> 
> Also, this function is called on the AP path if we don't have an
> equiv_table. Sounds to me, the BSP could do this work for the remaining
> APs and they could simply apply the patches.

BSP cannot vmalloc() for both 32 and 64 bits, so we cannot "install"
equiv_cpu_table, also cannot create pcache yet.

> 
> > +{
> > +	struct cpio_data cd;
> > +	struct firmware fw;
> > +	struct ucode_cpu_info *uci = ucode_cpu_info + boot_cpu_data.cpu_index;
> > +
> > +	if (equiv_cpu_table)
> > +		return 0;
> > +
> > +	cd = request_firmware_in_initrd();
> > +	if (!cd.data)
> > +		return -EINVAL;
> > +
> > +	fw.data = cd.data;
> > +	fw.size = cd.size;
> > +
> > +	if (!uci->cpu_sig.sig)
> > +		smp_call_function_single(boot_cpu_data.cpu_index,
> > +					 collect_cpu_info_amd_early_bsp, NULL,
> > +					 1);
> > +
> > +	if (request_microcode_fw(boot_cpu_data.cpu_index, &fw) != UCODE_OK)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +void __init load_ucode_amd_bsp(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct cpio_data fw = request_firmware_in_initrd_early();
> > +
> > +	if (!fw.data)
> > +		return;
> > +
> > +	load_microcode_amd_early(cpu, fw.data, fw.size);
> > +}
> > +
> > +#ifdef CONFIG_X86_32
> > +/*
> > + * on X86_32 AP load, since paging is turned off and vmalloc() is not available
> > + * yet, we cannot install the equivalent cpu table nor cache the microcode
> > + * patches in kernel memory, so just take the BSP code path. unless we are
> > + * CPU hotplugging on (i.e. resume from suspend), which then we will load from
> > + * bsp_mpb instead.
> > + */
> > +void __cpuinit load_ucode_amd_ap(void)
> > +{
> > +	unsigned int cpu = smp_processor_id();
> > +	struct microcode_amd *mc;
> > +	struct cpuinfo_x86 c;
> > +	struct ucode_cpu_info uci;
> > +
> > +	mc = (struct microcode_amd *)__pa_nodebug(bsp_mpb);
> > +	if (mc->hdr.patch_id && mc->hdr.processor_rev_id)
> > +		_apply_microcode_amd(cpu, mc, &c, &uci, true);
> > +	else
> > +		load_ucode_amd_bsp();
> 
> What's going on here? The _ap() function does load...bsp()?

On 32 bits, APs cannot vmalloc(), so we take the same code path as the BSPs
loading from initrd memory, and not installing equiv_cpu_table and not creating
pcache.

> 
> This whole code looks really convoluted to me and I'm going to stop
> trying to understand what's going on. Please do a careful analysis of
> what calls what, when. Then, carve out shared functionality carefully
> into a function and call that function exactly what it does.
> 
> AFAICT, from reading the code, there a couple of things you need to do
> (correct me if I'm missing anything):
> 
> * load ucode on the BSP
> 
> * load ucode on the APs
> 
> * save the ucode image from initrd
> 
> But you have functions like _apply_microcode_amd where just so that you
> can reuse functionality, you create the passing arguments on the stack
> just so that it works. And this is not optimal.
> 
> A better design, IHMO, would be to carve out *only* the application
> functionality, i.e. the RD/WRMSR dance and call it __apply_microcode
> which you can call from the early code. The rest of the glue should go
> in another function which actually saves c->microcode, etc.
> 
> This makes it very hard to follow the code and is not a proper design.
> 
> And it should be very clear just by looking at the function, when and
> how is this function called.
> 
> Please add more thought and care when designing this for the next
> version.
> 
> You could also split this patch so that it is more clear what happens.
> 
> Another thing to consider is, for example, which work is done only once
> and should thus be done only on the BSP so that the if-clauses on the AP
> path can be removed.
> 
> Also, avoid repetitive work like save_microcode_in_initrd_amd() which is
> done from the initcall and on the APs. I don't think this is needed so
> it shouldn't happen.
> 
> And so on, and so on.

Okay I'll just rewrite the the whole thing tomorrow,

I'll separate out the early stuff by itself into microcode_amd_early.c

And try and word the comments clearer.

Thanks.

Re: [PATCH V2 3/3] x86/microcode: early microcode patch loading support on AMD

[Borislav Petkov]

On Tue, May 28, 2013 at 06:43:07PM -0500, Jacob Shin wrote:
> > Anyway, this whole Kconfig microcode section could use a simplification
> > but this is not the subject of this patchset - I'll try to address it
> > after this, as stated in another mail.
> 
> Yes, I'll simplify the Kconfig as you suggested in your ealier email.

You don't necessarily have to - as I explained in the other mail,
distros end up enabling both anyway so we probably don't need the vendor
split. Instead, simply having microcode loading and early microcode
loading functionality seems like enough to me.

But this is another conversation so you don't have to address it now.

> > At least I can't seem to trigger any build failures when I comment them
> > out.
> 
> Hm.. I couldn't build without it at some point, I'll take them out but they
> might be indrectly included anyways.

Yes, let's leave them out. I'll run randconfig builds when your patches
are ready and we'll see.

...

> > Why do we even need this? Hotplug should take care of reappying
> > microcode on all cores.
> 
> It is needed because otherwise BSP does not reapply microcode on resume, see
> microcode_core.c BSP resume code path.

Ah, we don't hotplug the BSP, sure - I was looking at the the
mc_cpu_notifier...

...

> > Right, come to think of it, we don't use this uci->mc thing anywhere
> > except in mc_bp_resume. But even there, we call ->apply_microcode()
> > which on AMD searches the patch in the patch cache so no need for this
> > assignment at all.
> 
> Right, but at least we need a non-zero value, I can stick "1" in there I guess.
> Whatever you prefer.

That's for the BSP, right? Yeah, ok, then please just leave it like it
is but add a nice comment why we're doing it.

...


> > This comment needs a bit of scrubbing, spell- and readability check.
> > 
> > And it says we cannot traverse the pcache and yet we traverse it with
> > find_patch().
> > 
> > ???
> 
> What I meant was that we can traverse it now, but on 32 bits, later when we
> go into suspend and resume, we since we load before paging is turned on, we
> cannot traverse through equiv_cpu_table and pcache.

Ok, I get it now.

...

> BSP cannot vmalloc() for both 32 and 64 bits, so we cannot "install"
> equiv_cpu_table, also cannot create pcache yet.

But we can use the one in the initrd, extract the information and save
it locally like you do with the bsp_mpb. Then, when later another
microcode blob gets loaded, we simply not use the initial one.

I don't know whether that is a more favourable approach, though - I'm
just conjecturing here.

> On 32 bits, APs cannot vmalloc(), so we take the same code path as the BSPs
> loading from initrd memory, and not installing equiv_cpu_table and not creating
> pcache.

... which sounds to me like this functionality needs to be extracted in
a function callable by BSP and AP path.

> Okay I'll just rewrite the the whole thing tomorrow,
>
> I'll separate out the early stuff by itself into microcode_amd_early.c
>
> And try and word the comments clearer.

Thanks.

-- 
Regards/Gruss,
    Boris.

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

RE: [PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[Yu, Fenghua]

> From: Jacob Shin [mailto:jacob.shin@amd.com]
> The following patchset adds early microcode patch loading support on
> AMD systems, on top of the framework introduced by:
>   https://lkml.org/lkml/2012/12/21/193
> 

Could you please change Documentation/x86/early-microcode.txt for AMD? At least please add AMD microcode file in cpio name space: kernel/x86/microcode/AuthenticAMD.bin. Users can generate the cpio file with AMD microcode based on your document.

Thanks.

-Fenghua

 

Re: [PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

On Thu, May 23, 2013 at 09:01:21PM +0000, Yu, Fenghua wrote:
> > From: Jacob Shin [mailto:jacob.shin@amd.com]
> > The following patchset adds early microcode patch loading support on
> > AMD systems, on top of the framework introduced by:
> >   https://lkml.org/lkml/2012/12/21/193
> > 
> 
> Could you please change Documentation/x86/early-microcode.txt for AMD? At least please add AMD microcode file in cpio name space: kernel/x86/microcode/AuthenticAMD.bin. Users can generate the cpio file with AMD microcode based on your document.

Ah, yes will do. I'll send out another revision tomorrow.

Thanks!

Re: [PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[Borislav Petkov]

On Thu, May 23, 2013 at 04:23:01PM -0500, Jacob Shin wrote:
> On Thu, May 23, 2013 at 09:01:21PM +0000, Yu, Fenghua wrote:
> > > From: Jacob Shin [mailto:jacob.shin@amd.com]
> > > The following patchset adds early microcode patch loading support on
> > > AMD systems, on top of the framework introduced by:
> > >   https://lkml.org/lkml/2012/12/21/193
> > > 
> > 
> > Could you please change Documentation/x86/early-microcode.txt for AMD? At least please add AMD microcode file in cpio name space: kernel/x86/microcode/AuthenticAMD.bin. Users can generate the cpio file with AMD microcode based on your document.
> 
> Ah, yes will do. I'll send out another revision tomorrow.

Jacob, would you please wait a bit longer when sending new revisions after
people have had time to review the current one first?

A couple of days is normally a good timeout. :)

Thanks.

Re: [PATCH V2 0/3] x86/microcode: early microcode patch loading support on AMD

[Jacob Shin]

On Fri, May 24, 2013 at 10:29:48AM +0200, Borislav Petkov wrote:
> On Thu, May 23, 2013 at 04:23:01PM -0500, Jacob Shin wrote:
> > On Thu, May 23, 2013 at 09:01:21PM +0000, Yu, Fenghua wrote:
> > > > From: Jacob Shin [mailto:jacob.shin@amd.com]
> > > > The following patchset adds early microcode patch loading support on
> > > > AMD systems, on top of the framework introduced by:
> > > >   https://lkml.org/lkml/2012/12/21/193
> > > > 
> > > 
> > > Could you please change Documentation/x86/early-microcode.txt for AMD? At least please add AMD microcode file in cpio name space: kernel/x86/microcode/AuthenticAMD.bin. Users can generate the cpio file with AMD microcode based on your document.
> > 
> > Ah, yes will do. I'll send out another revision tomorrow.
> 
> Jacob, would you please wait a bit longer when sending new revisions after
> people have had time to review the current one first?
> 
> A couple of days is normally a good timeout. :)

Okay, I'll revisit this next week then, thanks!