diff mbox series

[08/30] x86/microcode/intel: Rip out mixed stepping support for Intel CPUs

Message ID 20230810160805.418991257@linutronix.de
State New
Headers
Series x86/microcode: Cleanup and late loading enhancements |

Commit Message

Thomas Gleixner Aug. 10, 2023, 6:37 p.m. UTC 
  From: Ashok Raj <ashok.raj@intel.com>

Mixed steppings aren't supported on Intel CPUs. Only one patch is required
for the entire system. The caching of micro code blobs which match the
family and model is therefore pointless and in fact it is disfunctional as
CPU hotplug updates use only a single microcode blob, i.e. the one where
*intel_ucode_patch points to.

Remove the microcode cache and make it an AMD local feature.

[ tglx: Save only at the end. Otherwise random microcode ends up in the
  	pointer for early loading ]

Originally-From: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/cpu/microcode/amd.c      |   10 +
 arch/x86/kernel/cpu/microcode/core.c     |    2 
 arch/x86/kernel/cpu/microcode/intel.c    |  201 ++++++++-----------------------
 arch/x86/kernel/cpu/microcode/internal.h |   10 -
 4 files changed, 64 insertions(+), 159 deletions(-)
---

Comments

Borislav Petkov Aug. 11, 2023, 10:25 p.m. UTC | #1 
On Thu, Aug 10, 2023 at 08:37:38PM +0200, Thomas Gleixner wrote:
> @@ -319,6 +264,7 @@ scan_microcode(void *data, size_t size,
>  {
>  	struct microcode_header_intel *mc_header;
>  	struct microcode_intel *patch = NULL;
> +	u32 cur_rev = uci->cpu_sig.rev;
>  	unsigned int mc_size;
>  
>  	while (size) {
> @@ -328,8 +274,7 @@ scan_microcode(void *data, size_t size,
>  		mc_header = (struct microcode_header_intel *)data;
>  
>  		mc_size = get_totalsize(mc_header);
> -		if (!mc_size ||
> -		    mc_size > size ||
> +		if (!mc_size || mc_size > size ||
>  		    intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
>  			break;
>  
> @@ -341,31 +286,16 @@ scan_microcode(void *data, size_t size,
>  			continue;
>  		}
>  
> -		if (save) {
> -			save_microcode_patch(uci, data, mc_size);
> +		/* BSP scan: Check whether there is newer microcode */
> +		if (save && cur_rev >= mc_header->rev)
>  			goto next;
> -		}
> -
>  
> -		if (!patch) {
> -			if (!has_newer_microcode(data,
> -						 uci->cpu_sig.sig,
> -						 uci->cpu_sig.pf,
> -						 uci->cpu_sig.rev))
> -				goto next;
> -
> -		} else {
> -			struct microcode_header_intel *phdr = &patch->hdr;
> -
> -			if (!has_newer_microcode(data,
> -						 phdr->sig,
> -						 phdr->pf,
> -						 phdr->rev))
> -				goto next;
> -		}
> +		/* Save scan: Check whether there is newer or matching microcode */
> +		if (save && cur_rev != mc_header->rev)
> +			goto next;

I'm confused: when you look at those statements when this patch is
applied, they look like this:

                /* BSP scan: Check whether there is newer microcode */
                if (save && cur_rev >= mc_header->rev)
                        goto next;

                /* Save scan: Check whether there is newer or matching microcode */
                if (save && cur_rev != mc_header->rev)
                        goto next;

You'd only hit the second one if

		cur_rev < mc_header->rev

but then that implies

		cur_rev != mc_header->rev

too. I *think* you wanna have the first test be only ">" as you're
looking for newer microcode.

Besides, __load_ucode_intel() is calling this function with safe ==
false so those statements would never check anything. I guess that's
still ok because the above intel_find_matching_signature() would match.

Hmmm?

Uff, this function is ugly and can be simplified. Perhaps that happens
later.


>  
> -		/* We have a newer patch, save it. */
>  		patch = data;
> +		cur_rev = mc_header->rev;
>  
>  next:
>  		data += mc_size;
> @@ -374,18 +304,22 @@ scan_microcode(void *data, size_t size,
>  	if (size)
>  		return NULL;
>  
> +	if (save && patch)
> +		save_microcode_patch(patch, mc_size);
> +
>  	return patch;
>  }
>  
>  static void show_saved_mc(void)
>  {
>  #ifdef DEBUG

Yeah, what Nikolay said - move the next one before this one and then the
show_saved_mc() hunks are gone.

> -	int i = 0, j;
>  	unsigned int sig, pf, rev, total_size, data_size, date;
> +	struct extended_sigtable *ext_header;
> +	struct extended_signature *ext_sig;
>  	struct ucode_cpu_info uci;
> -	struct ucode_patch *p;
> +	int j, ext_sigcount;
>  
> -	if (list_empty(&microcode_cache)) {
> +	if (!intel_ucode_patch) {
>  		pr_debug("no microcode data saved.\n");
>  		return;
>  	}

...

> @@ -451,7 +374,7 @@ static void save_mc_for_early(struct uco
>  
>  	mutex_lock(&x86_cpu_microcode_mutex);
>  
> -	save_microcode_patch(uci, mc, size);
> +	save_microcode_patch(mc, size);
>  	show_saved_mc();
>  
>  	mutex_unlock(&x86_cpu_microcode_mutex);
> @@ -675,26 +598,10 @@ void load_ucode_intel_ap(void)
>  	apply_microcode_early(&uci, true);
>  }
>  
> -static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
> +/* Accessor for microcode pointer */
> +static struct microcode_intel *ucode_get_patch(void)

static function - "get_patch" only is fine.
Thomas Gleixner Aug. 12, 2023, 7:16 a.m. UTC | #2 
On Sat, Aug 12 2023 at 00:25, Borislav Petkov wrote:
> On Thu, Aug 10, 2023 at 08:37:38PM +0200, Thomas Gleixner wrote:
> I'm confused: when you look at those statements when this patch is
> applied, they look like this:
>
>                 /* BSP scan: Check whether there is newer microcode */
>                 if (save && cur_rev >= mc_header->rev)
>                         goto next;

Bah. this clearly must be !save here.

> Uff, this function is ugly and can be simplified. Perhaps that happens
> later.

Yes, it is cleaned up later. I tried it the other way round, but that
didn't really work out.

Thanks,

        tglx
diff mbox series

Patch

--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -37,6 +37,16 @@ 
 
 #include "internal.h"
 
+struct ucode_patch {
+	struct list_head plist;
+	void *data;
+	unsigned int size;
+	u32 patch_id;
+	u16 equiv_cpu;
+};
+
+static LIST_HEAD(microcode_cache);
+
 static struct equiv_cpu_table {
 	unsigned int num_entries;
 	struct equiv_cpu_entry *entry;
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -46,8 +46,6 @@  static bool dis_ucode_ldr = true;
 
 bool initrd_gone;
 
-LIST_HEAD(microcode_cache);
-
 /*
  * Synchronization.
  *
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -41,10 +41,10 @@ 
 static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
 
 /* Current microcode patch used in early patching on the APs. */
-static struct microcode_intel *intel_ucode_patch;
+static struct microcode_intel *intel_ucode_patch __read_mostly;
 
 /* last level cache size per core */
-static int llc_size_per_core;
+static int llc_size_per_core __ro_after_init;
 
 int intel_cpu_collect_info(struct ucode_cpu_info *uci)
 {
@@ -233,81 +233,26 @@  static int has_newer_microcode(void *mc,
 	return intel_find_matching_signature(mc, csig, cpf);
 }
 
-static struct ucode_patch *memdup_patch(void *data, unsigned int size)
+static void save_microcode_patch(void *data, unsigned int size)
 {
-	struct ucode_patch *p;
+	struct microcode_header_intel *p;
 
-	p = kzalloc(sizeof(struct ucode_patch), GFP_KERNEL);
-	if (!p)
-		return NULL;
-
-	p->data = kmemdup(data, size, GFP_KERNEL);
-	if (!p->data) {
-		kfree(p);
-		return NULL;
-	}
-
-	return p;
-}
-
-static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigned int size)
-{
-	struct microcode_header_intel *mc_hdr, *mc_saved_hdr;
-	struct ucode_patch *iter, *tmp, *p = NULL;
-	bool prev_found = false;
-	unsigned int sig, pf;
-
-	mc_hdr = (struct microcode_header_intel *)data;
-
-	list_for_each_entry_safe(iter, tmp, &microcode_cache, plist) {
-		mc_saved_hdr = (struct microcode_header_intel *)iter->data;
-		sig	     = mc_saved_hdr->sig;
-		pf	     = mc_saved_hdr->pf;
-
-		if (intel_find_matching_signature(data, sig, pf)) {
-			prev_found = true;
-
-			if (mc_hdr->rev <= mc_saved_hdr->rev)
-				continue;
-
-			p = memdup_patch(data, size);
-			if (!p)
-				pr_err("Error allocating buffer %p\n", data);
-			else {
-				list_replace(&iter->plist, &p->plist);
-				kfree(iter->data);
-				kfree(iter);
-			}
-		}
-	}
-
-	/*
-	 * There weren't any previous patches found in the list cache; save the
-	 * newly found.
-	 */
-	if (!prev_found) {
-		p = memdup_patch(data, size);
-		if (!p)
-			pr_err("Error allocating buffer for %p\n", data);
-		else
-			list_add_tail(&p->plist, &microcode_cache);
-	}
+	kfree(intel_ucode_patch);
+	intel_ucode_patch = NULL;
 
+	p = kmemdup(data, size, GFP_KERNEL);
 	if (!p)
 		return;
 
-	if (!intel_find_matching_signature(p->data, uci->cpu_sig.sig, uci->cpu_sig.pf))
-		return;
-
 	/*
 	 * Save for early loading. On 32-bit, that needs to be a physical
 	 * address as the APs are running from physical addresses, before
 	 * paging has been enabled.
 	 */
 	if (IS_ENABLED(CONFIG_X86_32))
-		intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p->data);
+		intel_ucode_patch = (struct microcode_intel *)__pa_nodebug(p);
 	else
-		intel_ucode_patch = p->data;
+		intel_ucode_patch = (struct microcode_intel *)p;
 }
 
 /*
@@ -319,6 +264,7 @@  scan_microcode(void *data, size_t size,
 {
 	struct microcode_header_intel *mc_header;
 	struct microcode_intel *patch = NULL;
+	u32 cur_rev = uci->cpu_sig.rev;
 	unsigned int mc_size;
 
 	while (size) {
@@ -328,8 +274,7 @@  scan_microcode(void *data, size_t size,
 		mc_header = (struct microcode_header_intel *)data;
 
 		mc_size = get_totalsize(mc_header);
-		if (!mc_size ||
-		    mc_size > size ||
+		if (!mc_size || mc_size > size ||
 		    intel_microcode_sanity_check(data, false, MC_HEADER_TYPE_MICROCODE) < 0)
 			break;
 
@@ -341,31 +286,16 @@  scan_microcode(void *data, size_t size,
 			continue;
 		}
 
-		if (save) {
-			save_microcode_patch(uci, data, mc_size);
+		/* BSP scan: Check whether there is newer microcode */
+		if (save && cur_rev >= mc_header->rev)
 			goto next;
-		}
-
 
-		if (!patch) {
-			if (!has_newer_microcode(data,
-						 uci->cpu_sig.sig,
-						 uci->cpu_sig.pf,
-						 uci->cpu_sig.rev))
-				goto next;
-
-		} else {
-			struct microcode_header_intel *phdr = &patch->hdr;
-
-			if (!has_newer_microcode(data,
-						 phdr->sig,
-						 phdr->pf,
-						 phdr->rev))
-				goto next;
-		}
+		/* Save scan: Check whether there is newer or matching microcode */
+		if (save && cur_rev != mc_header->rev)
+			goto next;
 
-		/* We have a newer patch, save it. */
 		patch = data;
+		cur_rev = mc_header->rev;
 
 next:
 		data += mc_size;
@@ -374,18 +304,22 @@  scan_microcode(void *data, size_t size,
 	if (size)
 		return NULL;
 
+	if (save && patch)
+		save_microcode_patch(patch, mc_size);
+
 	return patch;
 }
 
 static void show_saved_mc(void)
 {
 #ifdef DEBUG
-	int i = 0, j;
 	unsigned int sig, pf, rev, total_size, data_size, date;
+	struct extended_sigtable *ext_header;
+	struct extended_signature *ext_sig;
 	struct ucode_cpu_info uci;
-	struct ucode_patch *p;
+	int j, ext_sigcount;
 
-	if (list_empty(&microcode_cache)) {
+	if (!intel_ucode_patch) {
 		pr_debug("no microcode data saved.\n");
 		return;
 	}
@@ -397,45 +331,34 @@  static void show_saved_mc(void)
 	rev	= uci.cpu_sig.rev;
 	pr_debug("CPU: sig=0x%x, pf=0x%x, rev=0x%x\n", sig, pf, rev);
 
-	list_for_each_entry(p, &microcode_cache, plist) {
-		struct microcode_header_intel *mc_saved_header;
-		struct extended_sigtable *ext_header;
-		struct extended_signature *ext_sig;
-		int ext_sigcount;
-
-		mc_saved_header = (struct microcode_header_intel *)p->data;
-
-		sig	= mc_saved_header->sig;
-		pf	= mc_saved_header->pf;
-		rev	= mc_saved_header->rev;
-		date	= mc_saved_header->date;
-
-		total_size	= get_totalsize(mc_saved_header);
-		data_size	= intel_microcode_get_datasize(mc_saved_header);
-
-		pr_debug("mc_saved[%d]: sig=0x%x, pf=0x%x, rev=0x%x, total size=0x%x, date = %04x-%02x-%02x\n",
-			 i++, sig, pf, rev, total_size,
-			 date & 0xffff,
-			 date >> 24,
-			 (date >> 16) & 0xff);
-
-		/* Look for ext. headers: */
-		if (total_size <= data_size + MC_HEADER_SIZE)
-			continue;
+	sig	= intel_ucode_patch->hdr.sig;
+	pf	= intel_ucode_patch->hdr.pf;
+	rev	= intel_ucode_patch->hdr.rev;
+	date	= intel_ucode_patch->hdr.date;
+
+	total_size	= get_totalsize(mc_saved_header);
+	data_size	= intel_microcode_get_datasize(mc_saved_header);
+
+	pr_debug("mc_saved: sig=0x%x, pf=0x%x, rev=0x%x, total size=0x%x, date = %04x-%02x-%02x\n",
+		 sig, pf, rev, total_size, date & 0xffff,
+		 date >> 24, (date >> 16) & 0xff);
 
-		ext_header = (void *)mc_saved_header + data_size + MC_HEADER_SIZE;
-		ext_sigcount = ext_header->count;
-		ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
+	/* Look for ext. headers: */
+	if (total_size <= data_size + MC_HEADER_SIZE)
+		return;
 
-		for (j = 0; j < ext_sigcount; j++) {
-			sig = ext_sig->sig;
-			pf = ext_sig->pf;
+	ext_header = (void *)intel_ucode_patch + data_size + MC_HEADER_SIZE;
+	ext_sigcount = ext_header->count;
+	ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
+
+	for (j = 0; j < ext_sigcount; j++) {
+		sig = ext_sig->sig;
+		pf = ext_sig->pf;
 
-			pr_debug("\tExtended[%d]: sig=0x%x, pf=0x%x\n",
-				 j, sig, pf);
+		pr_debug("\tExtended[%d]: sig=0x%x, pf=0x%x\n",
+			 j, sig, pf);
 
-			ext_sig++;
-		}
+		ext_sig++;
 	}
 #endif
 }
@@ -451,7 +374,7 @@  static void save_mc_for_early(struct uco
 
 	mutex_lock(&x86_cpu_microcode_mutex);
 
-	save_microcode_patch(uci, mc, size);
+	save_microcode_patch(mc, size);
 	show_saved_mc();
 
 	mutex_unlock(&x86_cpu_microcode_mutex);
@@ -675,26 +598,10 @@  void load_ucode_intel_ap(void)
 	apply_microcode_early(&uci, true);
 }
 
-static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
+/* Accessor for microcode pointer */
+static struct microcode_intel *ucode_get_patch(void)
 {
-	struct microcode_header_intel *phdr;
-	struct ucode_patch *iter, *tmp;
-
-	list_for_each_entry_safe(iter, tmp, &microcode_cache, plist) {
-
-		phdr = (struct microcode_header_intel *)iter->data;
-
-		if (phdr->rev <= uci->cpu_sig.rev)
-			continue;
-
-		if (!intel_find_matching_signature(phdr,
-						   uci->cpu_sig.sig,
-						   uci->cpu_sig.pf))
-			continue;
-
-		return iter->data;
-	}
-	return NULL;
+	return intel_ucode_patch;
 }
 
 void reload_ucode_intel(void)
@@ -704,7 +611,7 @@  void reload_ucode_intel(void)
 
 	intel_cpu_collect_info(&uci);
 
-	p = find_patch(&uci);
+	p = ucode_get_patch();
 	if (!p)
 		return;
 
@@ -748,7 +655,7 @@  static enum ucode_state apply_microcode_
 		return UCODE_ERROR;
 
 	/* Look for a newer patch in our cache: */
-	mc = find_patch(uci);
+	mc = ucode_get_patch();
 	if (!mc) {
 		mc = uci->mc;
 		if (!mc)
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -8,16 +8,6 @@ 
 #include <asm/cpu.h>
 #include <asm/microcode.h>
 
-struct ucode_patch {
-	struct list_head plist;
-	void *data;		/* Intel uses only this one */
-	unsigned int size;
-	u32 patch_id;
-	u16 equiv_cpu;
-};
-
-extern struct list_head microcode_cache;
-
 struct device;
 
 enum ucode_state {