Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1560859 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 13/13] x86/microcode/AMD: Remove AP scanning optimization Borislav Petkov <bp@alien8.de> - 2017-01-17 18:50 +0100
    Re: [PATCH 13/13] x86/microcode/AMD: Remove AP scanning  optimization Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:50 +0100

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

From: Borislav Petkov <bp@suse.de>

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

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

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

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

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

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

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

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

[toc] | [next] | [standalone]

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

On Tue, 17 Jan 2017, Borislav Petkov wrote:

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web