[PATCH 00/13] Make microcode loading more robust

[PATCH 00/13] Make microcode loading more robust

[Ashok Raj]

This series adds more robustness to microcode loading.

- Adds proper quiesce to sibling threads in an NMI handler.
- Fixes some nasty early loading bugs that are there for a long time.
- Adds a minimum revision ID to Intel patch meta-data, requested by Thomas.
- Turns microcode late loading back on by default.
- Adds some debugfs support to provide ability to test the microcode flows
  without the need for a new microcode.

This series has too many changes from previous post, but for anecdotal
references. 

https://lore.kernel.org/lkml/20220817051127.3323755-1-ashok.raj@intel.com/

Here is a summary of those patches.

patch1: Allow printing the old and new rev during an early update
patch2: Fix a potential bug during CPU hot-add flow for microcode update.
patch3: Fixes a nasty early loading bug that locks up with endless retries.
patch4: Add a helper to perform self NMI
patch5: Drop siblings in NMI while primary thread updates the microcode.
patch6: Rename refresh_fw to late_loading, in preparation for min-rev patches.
patch7: Move late loading warning to the same function as where taint happens.
patch8: Adds support for microcode meta-data to declare a minimum version 
patch9: Add a generic way to declare support for min-rev across vendors.
patch10: Drop wbinvd(), its not required after patch7.
patch11: Print microcode updated messages only when its successfully loaded
patch12: Issue a warning if MCE arrives while a microcode update is in progress.
patch13: Debug patch: To permit testing repeated loading of microcode.

Ashok Raj (12):
  x86/microcode/intel: Print old and new rev after early microcode
    update
  x86/microcode: Do not load from filesystem for CPU hot add
  x86/microcode/intel: Fix a hang if early loading microcode fails
  x86/microcode: Place siblings in NMI loop while update in progress
  x86/microcode: Rename refresh_fw to late_loading
  x86/microcode: Move late-load warning to earlier where kernel taint
    happens
  x86/microcode/intel: Add minimum required revision to microcode header
  x86/microcode: Add a generic mechanism to declare support for minrev
  x86/microcode/intel: Drop wbinvd() from microcode loading
  x86/microcode: Display revisions only when update is successful
  x86/mce: Warn of a microcode update is in progress when MCE arrives
  x86/microcode/intel: Add ability to update microcode even if rev is
    unchanged

Jacob Pan (1):
  x86/x2apic: Support x2apic self IPI with NMI_VECTOR

 arch/x86/include/asm/microcode.h       |  39 ++++++-
 arch/x86/include/asm/microcode_intel.h |   4 +-
 arch/x86/kernel/apic/x2apic_phys.c     |   6 +-
 arch/x86/kernel/cpu/mce/core.c         |   5 +
 arch/x86/kernel/cpu/microcode/amd.c    |   6 +-
 arch/x86/kernel/cpu/microcode/core.c   | 156 ++++++++++++++++++++++---
 arch/x86/kernel/cpu/microcode/intel.c  |  85 +++++++++-----
 arch/x86/kernel/cpu/microcode/nmi.c    |  72 ++++++++++++
 arch/x86/kernel/nmi.c                  |   7 ++
 arch/x86/Kconfig                       |   7 +-
 arch/x86/kernel/cpu/microcode/Makefile |   1 +
 11 files changed, 332 insertions(+), 56 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/microcode/nmi.c

-- 
2.34.1

[PATCH 01/13] x86/microcode/intel: Print old and new rev after early microcode update

[Ashok Raj]

Print the old and new versions of microcode after an early load is
complete. This is useful to know what version was loaded by BIOS before an
early microcode load.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1fcbd671f1df..cf1e2c30b230 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -435,10 +435,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
  * Print ucode update info.
  */
 static void
-print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
+print_ucode_info(u32 old_rev, struct ucode_cpu_info *uci, unsigned int date)
 {
-	pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
-		     uci->cpu_sig.rev,
+	pr_info_once("microcode updated early from 0x%x to revision 0x%x, date = %04x-%02x-%02x\n",
+		     old_rev, uci->cpu_sig.rev,
 		     date & 0xffff,
 		     date >> 24,
 		     (date >> 16) & 0xff);
@@ -448,6 +448,7 @@ print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
 
 static int delay_ucode_info;
 static int current_mc_date;
+static u32 early_old_rev;
 
 /*
  * Print early updated ucode info after printk works. This is delayed info dump.
@@ -458,7 +459,7 @@ void show_ucode_info_early(void)
 
 	if (delay_ucode_info) {
 		intel_cpu_collect_info(&uci);
-		print_ucode_info(&uci, current_mc_date);
+		print_ucode_info(early_old_rev, &uci, current_mc_date);
 		delay_ucode_info = 0;
 	}
 }
@@ -467,11 +468,12 @@ void show_ucode_info_early(void)
  * At this point, we can not call printk() yet. Delay printing microcode info in
  * show_ucode_info_early() until printk() works.
  */
-static void print_ucode(struct ucode_cpu_info *uci)
+static void print_ucode(u32 old_rev, struct ucode_cpu_info *uci)
 {
 	struct microcode_intel *mc;
 	int *delay_ucode_info_p;
 	int *current_mc_date_p;
+	u32 *old_rev_p;
 
 	mc = uci->mc;
 	if (!mc)
@@ -479,13 +481,15 @@ static void print_ucode(struct ucode_cpu_info *uci)
 
 	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
 	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
+	old_rev_p = (u32 *)__pa_nodebug(&early_old_rev);
 
 	*delay_ucode_info_p = 1;
 	*current_mc_date_p = mc->hdr.date;
+	*old_rev_p = old_rev;
 }
 #else
 
-static inline void print_ucode(struct ucode_cpu_info *uci)
+static inline void print_ucode(u32 old_rev, struct ucode_cpu_info *uci)
 {
 	struct microcode_intel *mc;
 
@@ -493,14 +497,14 @@ static inline void print_ucode(struct ucode_cpu_info *uci)
 	if (!mc)
 		return;
 
-	print_ucode_info(uci, mc->hdr.date);
+	print_ucode_info(old_rev, uci, mc->hdr.date);
 }
 #endif
 
 static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 {
 	struct microcode_intel *mc;
-	u32 rev;
+	u32 old_rev, rev;
 
 	mc = uci->mc;
 	if (!mc)
@@ -517,6 +521,7 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 		return UCODE_OK;
 	}
 
+	old_rev = rev;
 	/*
 	 * Writeback and invalidate caches before updating microcode to avoid
 	 * internal issues depending on what the microcode is updating.
@@ -533,9 +538,9 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	uci->cpu_sig.rev = rev;
 
 	if (early)
-		print_ucode(uci);
+		print_ucode(old_rev, uci);
 	else
-		print_ucode_info(uci, mc->hdr.date);
+		print_ucode_info(old_rev, uci, mc->hdr.date);
 
 	return 0;
 }
-- 
2.34.1

[PATCH 02/13] x86/microcode: Do not load from filesystem for CPU hot add

[Ashok Raj]

When request_microcode_fw() is called, refresh_fw parameter must be set
only when late loading. Currently during CPU hotplug, the path is as
follows.

mc_device_add() -> microcode_init_cpu() ->
				request_microcode_fw(refresh_hw=true)

Consider if a new microcode file was just copied, but have not yet
performed a late load. Now adding a new CPU will result in loading that
microcode file vs the rest of the CPUs in the system would have a previous
revision of what was loaded earlier.

Moreover this doesn't need to be any different from logical cpu online
flow, microcode_update_cpu() and mc_device_add() seem to be doing the
opposite things.

Seems like there is no good use for the refresh_fw parameter in
microcode_init_cpu(), so simply drop it.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6a41cee242f6..e4135b4fdbc6 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -599,7 +599,7 @@ static enum ucode_state microcode_resume_cpu(int cpu)
 	return UCODE_OK;
 }
 
-static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
+static enum ucode_state microcode_init_cpu(int cpu)
 {
 	enum ucode_state ustate;
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -614,7 +614,8 @@ static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 	if (system_state != SYSTEM_RUNNING)
 		return UCODE_NFOUND;
 
-	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, refresh_fw);
+	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev,
+						     false);
 	if (ustate == UCODE_NEW) {
 		pr_debug("CPU%d updated upon init\n", cpu);
 		apply_microcode_on_target(cpu);
@@ -633,7 +634,7 @@ static enum ucode_state microcode_update_cpu(int cpu)
 	if (uci->valid)
 		return microcode_resume_cpu(cpu);
 
-	return microcode_init_cpu(cpu, false);
+	return microcode_init_cpu(cpu);
 }
 
 static int mc_device_add(struct device *dev, struct subsys_interface *sif)
@@ -649,7 +650,7 @@ static int mc_device_add(struct device *dev, struct subsys_interface *sif)
 	if (err)
 		return err;
 
-	if (microcode_init_cpu(cpu, true) == UCODE_ERROR)
+	if (microcode_init_cpu(cpu) == UCODE_ERROR)
 		return -EINVAL;
 
 	return err;
-- 
2.34.1

[PATCH 03/13] x86/microcode/intel: Fix a hang if early loading microcode fails

[Ashok Raj]

When early loading of microcode fails for any reason other than the wrong
family-model-stepping, Linux can get into an infinite loop retrying the
same failed load.

A single retry is needed to handle any mixed stepping case.

Assume we have a microcode that fails to load for some reason.
load_ucode_ap() seems to retry if the loading fails. But it searches for
a new rev, but ends up finding the same copy. Hence it appears to repeat
the same load, retry loop for ever.

We can fix it as follows.

1) When the load_ucode_intel_bsp() fails, record the revision that failed.
2) In load_ucode_intel_ap() check if AP is same FMS as BP, and the code
that failed earlier isn't going to apply anyway. So use the saved
information to abort loading on AP's altogether.

load_ucode_intel_ap()
{
..
reget:
        if (!*iup) {
                patch = __load_ucode_intel(&uci);
		^^^^^ Finds the same patch every time.

                if (!patch)
                        return;

                *iup = patch;
        }

        uci.mc = *iup;

        if (apply_microcode_early(&uci, true)) {
	^^^^^^^^^^^^ apply fails
              /* Mixed-silicon system? Try to refetch the proper patch: */
              *iup = NULL;

              goto reget;
	      ^^^^^ Rince repeat.
        }

}

Fixes: 06b8534cb728 ("x86/microcode: Rework microcode loading")
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index cf1e2c30b230..0f7e4ba05c39 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -606,6 +606,7 @@ void __init load_ucode_intel_bsp(void)
 {
 	struct microcode_intel *patch;
 	struct ucode_cpu_info uci;
+	int rev, ret;
 
 	patch = __load_ucode_intel(&uci);
 	if (!patch)
@@ -613,13 +614,18 @@ void __init load_ucode_intel_bsp(void)
 
 	uci.mc = patch;
 
-	apply_microcode_early(&uci, true);
+	ret = apply_microcode_early(&uci, true);
+	if (ret) {
+		rev = patch->hdr.rev;
+		pr_err("Revision 0x%x failed during early loading\n", rev);
+	}
 }
 
 void load_ucode_intel_ap(void)
 {
 	struct microcode_intel *patch, **iup;
 	struct ucode_cpu_info uci;
+	bool retried = false;
 
 	if (IS_ENABLED(CONFIG_X86_32))
 		iup = (struct microcode_intel **) __pa_nodebug(&intel_ucode_patch);
@@ -638,9 +644,13 @@ void load_ucode_intel_ap(void)
 	uci.mc = *iup;
 
 	if (apply_microcode_early(&uci, true)) {
+		if (retried)
+			return;
+
 		/* Mixed-silicon system? Try to refetch the proper patch: */
 		*iup = NULL;
 
+		retried = true;
 		goto reget;
 	}
 }
-- 
2.34.1

[PATCH 04/13] x86/x2apic: Support x2apic self IPI with NMI_VECTOR

[Ashok Raj]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

X2APIC architecture introduced a dedicated register for sending self-IPI.
Though highly optimized for performance, its semantics limit the delivery
mode to fixed mode.  NMI vector is not supported, this created an
inconsistent behavior between X2APIC and others.

This patch adds support for X2APIC NMI_VECTOR by fall back to the slower
ICR method.

Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 arch/x86/kernel/apic/x2apic_phys.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
index 6bde05a86b4e..5f533b76adf6 100644
--- a/arch/x86/kernel/apic/x2apic_phys.c
+++ b/arch/x86/kernel/apic/x2apic_phys.c
@@ -149,7 +149,11 @@ int x2apic_phys_pkg_id(int initial_apicid, int index_msb)
 
 void x2apic_send_IPI_self(int vector)
 {
-	apic_write(APIC_SELF_IPI, vector);
+	if (vector == NMI_VECTOR)
+		apic->send_IPI_mask(cpumask_of(smp_processor_id()),
+				    NMI_VECTOR);
+	else
+		apic_write(APIC_SELF_IPI, vector);
 }
 
 static struct apic apic_x2apic_phys __ro_after_init = {
-- 
2.34.1

[PATCH 05/13] x86/microcode: Place siblings in NMI loop while update in progress

[Ashok Raj]

Microcode updates affect the state of the running CPU. In the case of
hyperthreads, the thread initiating the update is in a known state
(performing wrmsr 0x79), but its HT sibling can be executing arbitrary
instructions.

If one of these arbitrary instruction is being patched by the update at the
same time the sibling is trying to execute from it, its using microcode in
an unstable state.

Ensuring a rendezvous of all CPUs using stop_machine() ensures that
siblings are not executing any random user space code, and stop_machine()
also masks interrupts that can be masked.

The ones that can still slip in are the exceptions. They are:

NMI entry code and NMI handlers can also execute relatively arbitrary
instructions. This is an effort to ensure NMI doesn't slip until the wrmsr
has completed.

== Solution: NMI prevention during update ==

Before the stop_machine() rendezvous, an NMI handler is registered. The
handler is placed at the beginning of all other handlers. The siblings
then kick themselves into NMI by doing a self NMI IPI.

The handler does two things:

- Informs the primary thread that it has entered the NMI handler. Only
  after all siblings of a core have entered NMI, the primary proceeds
  with wrmsr to update microcode.
- It spins until the primary CPU has completed the wrmsr and informs the
  sibling to quit the NMI loop.

Also an important thing to remember is the microcode requests for exclusive
access to the core before performing an update. This effectively pulls the
sibling into microcode control until the wrmsr has released exclusive
access. Since the sibling is not executing any instructions while the
wrmsr completes, no other exceptions will surface on the sibling CPU.

Breakpoints can be another source that can lead do taking exceptions. But
on NMI entry, the kernel seems to be save/clear/restore the breakpoint
control register (DR7). local_db_save() and local_db_restore(). This
effectively eliminates any breakpoints leading the sibling into
uncontrolled execution.

The algorithm is something like this:

After stop_machine() all threads are executing __reload_late()

hold_sibling_in_nmi()
{
	if (cpu not do_nmi_trap)
		return;

	update sibling reached NMI for primary to continue

	while (primary not done with update)
		wait;

	return;
}

exc_nmi:IDT()
{
	....
	hold_sibling_in_nmi();
	...
}

__reload_late()
{

	entry_rendezvous(&late_cpus_in);

	if (this_cpu is first_cpu in the core)
		wait for siblings to drop in NMI
		apply_microcode()
		set completion to release sibling from NMI
	else
		set sibling info to drop into NMI
		send self_IPI(NMI_VECTOR);

wait_for_siblings:

	exit_rendezvous(&late_cpus_out);
}

reload_late()
{
	register_nmi_handler()
	stop_machine(__reload_late);
	unregister_nmi_handler();
}

Suggested-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode.h       |  33 ++++++++
 arch/x86/kernel/cpu/microcode/core.c   | 101 ++++++++++++++++++++++++-
 arch/x86/kernel/cpu/microcode/nmi.c    |  72 ++++++++++++++++++
 arch/x86/kernel/nmi.c                  |   7 ++
 arch/x86/kernel/cpu/microcode/Makefile |   1 +
 5 files changed, 210 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/microcode/nmi.c

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 7f7800e15ed0..23d25ef546a5 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -6,6 +6,33 @@
 #include <linux/earlycpio.h>
 #include <linux/initrd.h>
 
+/*
+ * Although this is a per-cpu structure, both the primary and siblings use
+ * only the primary structure to communicate.
+ *
+ * num_core_cpus	- Number of CPUs in the core
+ * callin		- Siblings set to inform primary on NMI arrival
+ * core_done		- Set by primary once microcode update has completed
+ */
+struct core_rendez {
+	int num_core_cpus;
+	atomic_t callin;
+	atomic_t core_done;
+	atomic_t failed;
+};
+
+DECLARE_PER_CPU(struct core_rendez, core_sync);
+
+/*
+ * The following structure is only used by secondary.
+ * Sets the primary per_cpu variable to be found inside the NMI handler to
+ * indicate this CPU  is supposed to drop into NMI. Its consulted in the
+ * NMI handler before entring the loop waiting for primary to finish the
+ * loading process. Once loading is complete the NMI handler clears this
+ * pointer.
+ */
+DECLARE_PER_CPU(struct core_rendez *, nmi_primary_ptr);
+
 struct ucode_patch {
 	struct list_head plist;
 	void *data;		/* Intel uses only this one */
@@ -136,4 +163,10 @@ static inline void reload_early_microcode(void)			{ }
 static inline void microcode_bsp_resume(void)			{ }
 #endif
 
+#ifdef CONFIG_MICROCODE_LATE_LOADING
+extern void hold_sibling_in_nmi(void);
+#else
+static inline void hold_sibling_in_nmi(void) { }
+#endif
+
 #endif /* _ASM_X86_MICROCODE_H */
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index e4135b4fdbc6..50652b019233 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -39,6 +39,8 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/apic.h>
+#include <asm/mce.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -432,6 +434,59 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
 	return 0;
 }
 
+/*
+ * This simply ensures that the self IPI with NMI to siblings is marked as
+ * handled.
+ */
+static int ucode_nmi_cb(unsigned int val, struct pt_regs *regs)
+{
+	return NMI_HANDLED;
+}
+
+/*
+ * Primary thread waits for all siblings to report that they have enterered
+ * the NMI handler
+ */
+static int __wait_for_core_siblings(struct core_rendez *rendez)
+{
+	int num_sibs = rendez->num_core_cpus - 1;
+	unsigned long long timeout = NSEC_PER_MSEC;
+	atomic_t *t = &rendez->callin;
+	int cpu = smp_processor_id();
+
+	while (atomic_read(t) < num_sibs) {
+		cpu_relax();
+		ndelay(SPINUNIT);
+		touch_nmi_watchdog();
+		timeout -= SPINUNIT;
+		if (timeout < SPINUNIT) {
+			pr_err("CPU%d timedout waiting for siblings\n", cpu);
+			atomic_inc(&rendez->failed);
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static void prepare_for_nmi(void)
+{
+	int cpu, first_cpu;
+	struct core_rendez *pcpu_core;
+
+	for_each_online_cpu(cpu) {
+		first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+		if (cpu != first_cpu)
+			continue;
+
+		pcpu_core = &per_cpu(core_sync, first_cpu);
+		pcpu_core->num_core_cpus =
+		     cpumask_weight(topology_sibling_cpumask(cpu));
+		atomic_set(&pcpu_core->callin, 0);
+		atomic_set(&pcpu_core->core_done, 0);
+		atomic_set(&pcpu_core->failed, 0);
+	}
+}
+
 /*
  * Returns:
  * < 0 - on error
@@ -439,14 +494,15 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
  */
 static int __reload_late(void *info)
 {
-	int cpu = smp_processor_id();
+	int first_cpu, cpu = smp_processor_id();
+	struct core_rendez *pcpu_core;
 	enum ucode_state err;
 	int ret = 0;
 
 	/*
 	 * Wait for all CPUs to arrive. A load will not be attempted unless all
 	 * CPUs show up.
-	 * */
+	 */
 	if (__wait_for_cpus(&late_cpus_in, NSEC_PER_SEC))
 		return -1;
 
@@ -457,10 +513,32 @@ static int __reload_late(void *info)
 	 * loading attempts happen on multiple threads of an SMT core. See
 	 * below.
 	 */
-	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	pcpu_core = &per_cpu(core_sync, first_cpu);
+
+	/*
+	 * Set the CPUs that we should hold in NMI until the primary has
+	 * completed the microcode update.
+	 */
+	if (first_cpu == cpu) {
+		/*
+		 * Wait for all siblings to enter
+		 * NMI before performing the update
+		 */
+		ret = __wait_for_core_siblings(pcpu_core);
+		if (ret || atomic_read(&pcpu_core->failed)) {
+			pr_err("CPU %d core lead timeout waiting for siblings\n", cpu);
+			ret = -1;
+		}
+		pr_debug("Primary CPU %d proceeding with update\n", cpu);
 		apply_microcode_local(&err);
-	else
+		atomic_set(&pcpu_core->core_done, 1);
+	} else {
+		/* We set the per-cpu of sibling in this case */
+		this_cpu_write(nmi_primary_ptr, pcpu_core);
+		apic->send_IPI_self(NMI_VECTOR);
 		goto wait_for_siblings;
+	}
 
 	if (err >= UCODE_NFOUND) {
 		if (err == UCODE_ERROR)
@@ -496,16 +574,31 @@ static int microcode_reload_late(void)
 	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
 	pr_err("You should switch to early loading, if possible.\n");
 
+	/*
+	 * Used for late_load entry and exit rendezvous
+	 */
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
+	prepare_for_nmi();
+
+	ret = register_nmi_handler(NMI_LOCAL, ucode_nmi_cb, NMI_FLAG_FIRST,
+				   "ucode_nmi");
+	if (ret) {
+		pr_err("Unable to register NMI handler\n");
+		goto done;
+	}
+
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check();
 
+	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
+
 	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
 		old, boot_cpu_data.microcode);
 
+done:
 	return ret;
 }
 
diff --git a/arch/x86/kernel/cpu/microcode/nmi.c b/arch/x86/kernel/cpu/microcode/nmi.c
new file mode 100644
index 000000000000..9245dd1e8c9d
--- /dev/null
+++ b/arch/x86/kernel/cpu/microcode/nmi.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2022 Ashok Raj <ashok.raj@intel.com>
+ *
+ * X86 CPU microcode update NMI handler.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/cpu.h>
+#include <linux/nmi.h>
+
+#include <asm/microcode.h>
+
+#define SPINUNIT	100 /* 100 nsec */
+
+DEFINE_PER_CPU(struct core_rendez, core_sync);
+DEFINE_PER_CPU(struct core_rendez *, nmi_primary_ptr);
+
+#define SPINUNIT 100 /* 100 nsec */
+
+static void delay(int ms)
+{
+	unsigned long timeout = jiffies + ((ms * HZ) / 1000);
+
+	while (time_before(jiffies, timeout))
+		cpu_relax();
+}
+
+/*
+ * Siblings wait until microcode update is completed by the primary thread.
+ */
+static int __wait_for_update(atomic_t *t)
+{
+	unsigned long long timeout = NSEC_PER_MSEC;
+
+	while (!arch_atomic_read(t)) {
+		cpu_relax();
+		delay(1);
+		//touch_nmi_watchdog();
+		timeout -= SPINUNIT;
+		if (timeout < SPINUNIT)
+			return 1;
+	}
+	return 0;
+}
+
+noinstr void hold_sibling_in_nmi(void)
+{
+	struct	 core_rendez *pcpu_core;
+	int ret = 0;
+
+	pcpu_core = this_cpu_read(nmi_primary_ptr);
+	if (likely(!pcpu_core))
+		return;
+
+	/*
+	 * Increment the callin to inform primary thread that the sibling
+	 * has arrived and parked in the NMI handler
+	 */
+	arch_atomic_inc(&pcpu_core->callin);
+
+	ret = __wait_for_update(&pcpu_core->core_done);
+	if (ret)
+		atomic_inc(&pcpu_core->failed);
+
+	/*
+	 * Clear the nmi_trap, so future NMI's won't be affected
+	 */
+	this_cpu_write(nmi_primary_ptr, NULL);
+}
diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
index cec0bfa3bc04..619afeaef07c 100644
--- a/arch/x86/kernel/nmi.c
+++ b/arch/x86/kernel/nmi.c
@@ -28,6 +28,7 @@
 #include <asm/cpu_entry_area.h>
 #include <asm/traps.h>
 #include <asm/mach_traps.h>
+#include <asm/microcode.h>
 #include <asm/nmi.h>
 #include <asm/x86_init.h>
 #include <asm/reboot.h>
@@ -505,6 +506,12 @@ DEFINE_IDTENTRY_RAW(exc_nmi)
 
 	this_cpu_write(nmi_dr7, local_db_save());
 
+	/*
+	 * If microcodeupdate is in progress, check and hold the sibling in
+	 * the NMI until primary has completed the update
+	 */
+	hold_sibling_in_nmi();
+
 	irq_state = irqentry_nmi_enter(regs);
 
 	inc_irq_stat(__nmi_count);
diff --git a/arch/x86/kernel/cpu/microcode/Makefile b/arch/x86/kernel/cpu/microcode/Makefile
index 34098d48c48f..e469990bba73 100644
--- a/arch/x86/kernel/cpu/microcode/Makefile
+++ b/arch/x86/kernel/cpu/microcode/Makefile
@@ -3,3 +3,4 @@ microcode-y				:= core.o
 obj-$(CONFIG_MICROCODE)			+= microcode.o
 microcode-$(CONFIG_MICROCODE_INTEL)	+= intel.o
 microcode-$(CONFIG_MICROCODE_AMD)	+= amd.o
+microcode-$(CONFIG_MICROCODE_LATE_LOADING) += nmi.o
-- 
2.34.1

[PATCH 06/13] x86/microcode: Rename refresh_fw to late_loading

[Ashok Raj]

Rename refresh_fw to late_loading to be more explicit about its purpose.

No functional change.

Suggested-by: Boris Petkov <bp@alien8.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode.h      | 2 +-
 arch/x86/kernel/cpu/microcode/amd.c   | 4 ++--
 arch/x86/kernel/cpu/microcode/intel.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 23d25ef546a5..401213fb2e4a 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -60,7 +60,7 @@ enum ucode_state {
 
 struct microcode_ops {
 	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
-						  bool refresh_fw);
+						  bool late_loading);
 
 	void (*microcode_fini_cpu) (int cpu);
 
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 5f38dd75cbc5..c18d3f01a452 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -891,7 +891,7 @@ load_microcode_amd(bool save, u8 family, const u8 *data, size_t size)
  * These might be larger than 2K.
  */
 static enum ucode_state request_microcode_amd(int cpu, struct device *device,
-					      bool refresh_fw)
+					      bool late_loading)
 {
 	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -900,7 +900,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	const struct firmware *fw;
 
 	/* reload ucode container only on the boot cpu */
-	if (!refresh_fw || !bsp)
+	if (!late_loading || !bsp)
 		return UCODE_OK;
 
 	if (c->x86 >= 0x15)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 0f7e4ba05c39..e08777ae9cc4 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -901,7 +901,7 @@ static bool is_blacklisted(unsigned int cpu)
 }
 
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
-					     bool refresh_fw)
+					     bool late_loading)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	const struct firmware *firmware;
-- 
2.34.1

[PATCH 07/13] x86/microcode: Move late-load warning to earlier where kernel taint happens

[Ashok Raj]

Move where the late loading warning is being issued to earlier in the call.
This would put the warn and taint in the same function.

Just a tidy thing, no functional changes.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 50652b019233..7a8fcb914b6a 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -571,9 +571,6 @@ static int microcode_reload_late(void)
 {
 	int old = boot_cpu_data.microcode, ret;
 
-	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
-	pr_err("You should switch to early loading, if possible.\n");
-
 	/*
 	 * Used for late_load entry and exit rendezvous
 	 */
@@ -624,6 +621,8 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		goto put;
 
+	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
+	pr_err("You should switch to early loading, if possible.\n");
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
 	if (tmp_ret != UCODE_NEW)
 		goto put;
-- 
2.34.1

[PATCH 08/13] x86/microcode/intel: Add minimum required revision to microcode header

[Ashok Raj]

In general users don't have the necessary information to determine
whether a late loading of a new microcode version has removed any feature
(MSR, CPUID etc) between what is currently loaded and this new microcode.
To address this issue, Intel has added a "minimum required version" field
to a previously reserved field in the file header. Microcode updates
should only be applied if the current microcode version is equal
to, or greater than this minimum required version.

Thomas made some suggestions[1] on how meta-data in the microcode file
could provide Linux with information to decide if the new microcode is
suitable candidate for late loading. But even the "simpler" option#1
requires a lot of metadata and corresponding kernel code to parse it.

The proposal here is an even simpler option. Simply "OS visible features"
such as CPUID and MSRs are the only two examples. The microcode must not
change these OS visible features because they cause problems after late
loading. When microcode changes features, microcode will change the min_rev
to prevent such microcodes from being late loaded.

Pseudo code for late loading is as follows:

if header.min_required_id == 0
        This is old format microcode, block late loading
else if current_ucode_version < header.min_required_id
        Current version is too old, block late loading of this microcode.
else
        OK to proceed with late loading.

Any microcode that modifies the interface to an OS-visible feature
will set the min_version to itself. This will enforce this microcode is
not suitable for late loading unless the currently loaded revision is
greater or equal to the new microcode affecting the change.

The enforcement is not in hardware and limited to kernel loader enforcing
the requirement. It is not required for early loading of microcode to
enforce this requirement, since the new features are only
evaluated after early loading in the boot process.


Test cases covered:

1. With new kernel, attempting to load an older format microcode with the
   min_rev=0 should be blocked by kernel.

   [  210.541802] Late loading denied: Microcode header does not specify a
   required min version.

2. New microcode with a non-zero min_rev in the header, but the specified
   min_rev is greater than what is currently loaded in the CPU should be
   blocked by kernel.

   245.139828] microcode: Late loading denied: Current revision 0x8f685300 is too old to update, must be at 0xaa000050 version or higher. Use early loading instead.

3. New microcode with a min_rev < currently loaded should allow loading the
   microcode

4. Build initrd with microcode that has min_rev=0, or min_rev > currently
   loaded should permit early loading microcode from initrd.

[1] https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/


Tested-by: William Xie <william.xie@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode_intel.h |  4 +++-
 arch/x86/kernel/cpu/microcode/intel.c  | 25 ++++++++++++++++++++++---
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..bc893dd68b82 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,9 @@ struct microcode_header_intel {
 	unsigned int            pf;
 	unsigned int            datasize;
 	unsigned int            totalsize;
-	unsigned int            reserved[3];
+	unsigned int            reserved1;
+	unsigned int		min_req_ver;
+	unsigned int		reserved3;
 };
 
 struct microcode_intel {
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index e08777ae9cc4..46edce811c69 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -163,13 +163,14 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 		intel_ucode_patch = p->data;
 }
 
-static int microcode_sanity_check(void *mc, int print_err)
+static int microcode_sanity_check(void *mc, int print_err, bool late_loading)
 {
 	unsigned long total_size, data_size, ext_table_size;
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header = NULL;
 	u32 sum, orig_sum, ext_sigcount = 0, i;
 	struct extended_signature *ext_sig;
+	struct ucode_cpu_info uci;
 
 	total_size = get_totalsize(mc_header);
 	data_size = get_datasize(mc_header);
@@ -240,6 +241,24 @@ static int microcode_sanity_check(void *mc, int print_err)
 		return -EINVAL;
 	}
 
+	/*
+	 * When late-loading, enforce that the current revision loaded on
+	 * the CPU is equal or greater than the value specified in the
+	 * new microcode header
+	 */
+	if (late_loading) {
+		if (!mc_header->min_req_ver) {
+			pr_warn("Late loading denied: Microcode header does not specify a required min version\n");
+			return -EINVAL;
+		}
+		intel_cpu_collect_info(&uci);
+		if (uci.cpu_sig.rev < mc_header->min_req_ver) {
+			pr_warn("Late loading denied: Current revision 0x%x too old to update, must be at 0x%x or higher. Use early loading instead\n",
+				uci.cpu_sig.rev, mc_header->min_req_ver);
+			return -EINVAL;
+		}
+	}
+
 	if (!ext_table_size)
 		return 0;
 
@@ -281,7 +300,7 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 		mc_size = get_totalsize(mc_header);
 		if (!mc_size ||
 		    mc_size > size ||
-		    microcode_sanity_check(data, 0) < 0)
+		    microcode_sanity_check(data, 0, false) < 0)
 			break;
 
 		size -= mc_size;
@@ -835,7 +854,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 		memcpy(mc, &mc_header, sizeof(mc_header));
 		data = mc + sizeof(mc_header);
 		if (!copy_from_iter_full(data, data_size, iter) ||
-		    microcode_sanity_check(mc, 1) < 0) {
+		    microcode_sanity_check(mc, 1, true) < 0) {
 			break;
 		}
 
-- 
2.34.1

[PATCH 09/13] x86/microcode: Add a generic mechanism to declare support for minrev

[Ashok Raj]

Intel microcode adds some meta-data to report a minimum required revision
before this new microcode can be late-loaded. There are no generic mechanism
to declare support for all vendors.

Add generic support to microcode to declare support, so the tainting and
late-loading can be permitted in those architectures that support reporting
a minrev in some form.

Late loading has added support for

- New images declaring a required minimum base version before a late-load
  is performed.
- Improved NMI handling during update to avoid sibling threads taking NMI's
  while primary is still not complete with the microcode update.

With these changes, late-loading can be re-enabled. Tainting only happens
on architectures that don't support minimum required version reporting.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode.h      |  1 +
 arch/x86/kernel/cpu/microcode/core.c  | 14 +++++++++++---
 arch/x86/kernel/cpu/microcode/intel.c |  6 ++++++
 arch/x86/Kconfig                      |  7 ++++---
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 401213fb2e4a..0c0bbc26560f 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -59,6 +59,7 @@ enum ucode_state {
 };
 
 struct microcode_ops {
+	int (*check_minrev) (void);
 	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
 						  bool late_loading);
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7a8fcb914b6a..46e9c2d8fae0 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -606,6 +606,7 @@ static ssize_t reload_store(struct device *dev,
 	enum ucode_state tmp_ret = UCODE_OK;
 	int bsp = boot_cpu_data.cpu_index;
 	unsigned long val;
+	int minrev;
 	ssize_t ret = 0;
 
 	ret = kstrtoul(buf, 0, &val);
@@ -621,8 +622,14 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		goto put;
 
-	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
-	pr_err("You should switch to early loading, if possible.\n");
+	if (microcode_ops->check_minrev())
+		minrev = microcode_ops->check_minrev();
+
+	if (!minrev) {
+		pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
+		pr_err("You should switch to early loading, if possible.\n");
+	}
+
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
 	if (tmp_ret != UCODE_NEW)
 		goto put;
@@ -637,7 +644,8 @@ static ssize_t reload_store(struct device *dev,
 	if (ret == 0)
 		ret = size;
 
-	add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+	if (!minrev)
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
 
 	return ret;
 }
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 46edce811c69..c8ee53fcf04d 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -950,7 +950,13 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	return ret;
 }
 
+static int intel_check_minrev(void)
+{
+	return 1;
+}
+
 static struct microcode_ops microcode_intel_ops = {
+	.check_minrev			  = intel_check_minrev,
 	.request_microcode_fw             = request_microcode_fw,
 	.collect_cpu_info                 = collect_cpu_info,
 	.apply_microcode                  = apply_microcode_intel,
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index f9920f1341c8..a01fce1092ce 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -1336,15 +1336,16 @@ config MICROCODE_AMD
 	  processors will be enabled.
 
 config MICROCODE_LATE_LOADING
-	bool "Late microcode loading (DANGEROUS)"
-	default n
+	bool "Late microcode loading"
+	default y
 	depends on MICROCODE
 	help
 	  Loading microcode late, when the system is up and executing instructions
 	  is a tricky business and should be avoided if possible. Just the sequence
 	  of synchronizing all cores and SMT threads is one fragile dance which does
 	  not guarantee that cores might not softlock after the loading. Therefore,
-	  use this at your own risk. Late loading taints the kernel too.
+	  use this at your own risk. Late loading taints the kernel, if it
+	  doesn't support a minimum required base version before an update.
 
 config X86_MSR
 	tristate "/dev/cpu/*/msr - Model-specific register support"
-- 
2.34.1

[PATCH 10/13] x86/microcode/intel: Drop wbinvd() from microcode loading

[Ashok Raj]

Some older processors had a bad interaction when updating microcode if the
caches were dirty causing machine checks. The wbinvd() was added to
mitigate that before performing microcode updates. Now that Linux checks
for the minimum version before performing an update, those microcode
revisions can't be loaded. Remove calls to wbinvd().

Reviewed-by: Tony Luck <tony.luck@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c8ee53fcf04d..c61aa661ac2f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -541,11 +541,6 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
 	}
 
 	old_rev = rev;
-	/*
-	 * Writeback and invalidate caches before updating microcode to avoid
-	 * internal issues depending on what the microcode is updating.
-	 */
-	native_wbinvd();
 
 	/* write microcode via MSR 0x79 */
 	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
@@ -773,12 +768,6 @@ static enum ucode_state apply_microcode_intel(int cpu)
 		goto out;
 	}
 
-	/*
-	 * Writeback and invalidate caches before updating microcode to avoid
-	 * internal issues depending on what the microcode is updating.
-	 */
-	native_wbinvd();
-
 	/* write microcode via MSR 0x79 */
 	wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
-- 
2.34.1

[PATCH 11/13] x86/microcode: Display revisions only when update is successful

[Ashok Raj]

Display the update message only when its successful

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 46e9c2d8fae0..c6cd815190b1 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -592,8 +592,9 @@ static int microcode_reload_late(void)
 
 	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
 
-	pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
-		old, boot_cpu_data.microcode);
+	if (!ret)
+		pr_info("Reload completed, microcode revision: 0x%x -> 0x%x\n",
+			old, boot_cpu_data.microcode);
 
 done:
 	return ret;
-- 
2.34.1

[PATCH 12/13] x86/mce: Warn of a microcode update is in progress when MCE arrives

[Ashok Raj]

Due to the nature of microcode updates to long flow instructions, its
possible if an MCE is taken when microcode update is in progress could be
dangerous. There is nothing the kernel can do to mitigate safely.

Drop some bread crumbs to note that a MCE happened while a microcode update
is also in progress.

Suggested-by: Boris Petkov <bp@alien8.de>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode.h     | 1 +
 arch/x86/kernel/cpu/mce/core.c       | 5 +++++
 arch/x86/kernel/cpu/microcode/core.c | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 0c0bbc26560f..38b501d842de 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -41,6 +41,7 @@ struct ucode_patch {
 };
 
 extern struct list_head microcode_cache;
+extern int ucode_updating;
 
 struct cpu_signature {
 	unsigned int sig;
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2c8ec5c71712..1f05aec9880f 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -46,6 +46,7 @@
 #include <linux/hardirq.h>
 
 #include <asm/intel-family.h>
+#include <asm/microcode.h>
 #include <asm/processor.h>
 #include <asm/traps.h>
 #include <asm/tlbflush.h>
@@ -1425,6 +1426,10 @@ noinstr void do_machine_check(struct pt_regs *regs)
 	else if (unlikely(!mca_cfg.initialized))
 		return unexpected_machine_check(regs);
 
+	instrumentation_begin();
+	if (unlikely(ucode_updating))
+		pr_warn("MCE triggered while microcode update is in progress\n");
+	instrumentation_end();
 	if (mce_flags.skx_repmov_quirk && quirk_skylake_repmov())
 		goto clear;
 
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index c6cd815190b1..eb2caa74de01 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -49,6 +49,8 @@ static bool dis_ucode_ldr = true;
 
 bool initrd_gone;
 
+int ucode_updating;
+
 LIST_HEAD(microcode_cache);
 
 /*
@@ -586,7 +588,9 @@ static int microcode_reload_late(void)
 		goto done;
 	}
 
+	ucode_updating = 1;
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
+	ucode_updating = 0;
 	if (ret == 0)
 		microcode_check();
 
-- 
2.34.1

[PATCH 13/13] x86/microcode/intel: Add ability to update microcode even if rev is unchanged

[Ashok Raj]

This comes in handy for testing without the need for a new microcode file.
It introduces a new boot parameter ucode_load_same, or it can be switched
dynamically at run time via debugfs file
/sys/kernel/debug/microcode/load_same.

NOT_FOR_INCLUSION:
Leave it to the discretion of Boris if its suitable for inclusion. It will
at least serve to validate some parts of the series without the need for a
new microcode.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode.h      |  2 ++
 arch/x86/kernel/cpu/microcode/amd.c   |  2 +-
 arch/x86/kernel/cpu/microcode/core.c  | 22 +++++++++++++++++-----
 arch/x86/kernel/cpu/microcode/intel.c |  4 ++--
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 38b501d842de..4baca634c2f7 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -43,6 +43,8 @@ struct ucode_patch {
 extern struct list_head microcode_cache;
 extern int ucode_updating;
 
+extern bool ucode_load_same;
+
 struct cpu_signature {
 	unsigned int sig;
 	unsigned int pf;
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index c18d3f01a452..124461e2d7a1 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -694,7 +694,7 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
 
 	/* need to apply patch? */
-	if (rev >= mc_amd->hdr.patch_id) {
+	if (rev >= mc_amd->hdr.patch_id && !ucode_load_same) {
 		ret = UCODE_OK;
 		goto out;
 	}
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index eb2caa74de01..632c7a1fcffb 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -23,6 +23,7 @@
 #include <linux/miscdevice.h>
 #include <linux/capability.h>
 #include <linux/firmware.h>
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/delay.h>
 #include <linux/mutex.h>
@@ -46,6 +47,7 @@
 
 static struct microcode_ops	*microcode_ops;
 static bool dis_ucode_ldr = true;
+bool ucode_load_same;
 
 bool initrd_gone;
 
@@ -542,11 +544,12 @@ static int __reload_late(void *info)
 		goto wait_for_siblings;
 	}
 
-	if (err >= UCODE_NFOUND) {
-		if (err == UCODE_ERROR)
+	if (ret || err >= UCODE_NFOUND) {
+		if (err == UCODE_ERROR ||
+		    (err == UCODE_NFOUND && !ucode_load_same)) {
 			pr_warn("Error reloading microcode on CPU %d\n", cpu);
-
-		ret = -1;
+			ret = -1;
+		}
 	}
 
 wait_for_siblings:
@@ -636,9 +639,12 @@ static ssize_t reload_store(struct device *dev,
 	}
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
-	if (tmp_ret != UCODE_NEW)
+	if (tmp_ret != UCODE_NEW && !ucode_load_same)
 		goto put;
 
+	if (tmp_ret != UCODE_NEW)
+		pr_info("Force loading ucode\n");
+
 	mutex_lock(&microcode_mutex);
 	ret = microcode_reload_late();
 	mutex_unlock(&microcode_mutex);
@@ -841,6 +847,7 @@ static const struct attribute_group cpu_root_microcode_group = {
 static int __init microcode_init(void)
 {
 	struct cpuinfo_x86 *c = &boot_cpu_data;
+	static struct dentry *dentry_ucode;
 	int error;
 
 	if (dis_ucode_ldr)
@@ -884,7 +891,12 @@ static int __init microcode_init(void)
 	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
 				  mc_cpu_online, mc_cpu_down_prep);
 
+	dentry_ucode = debugfs_create_dir("microcode", NULL);
+	debugfs_create_bool("load_same", 0644, dentry_ucode, &ucode_load_same);
+
 	pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
+	pr_info("ucode_load_same is %s\n",
+		ucode_load_same ? "enabled" : "disabled");
 
 	return 0;
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c61aa661ac2f..c9f1e6f5e53b 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -763,7 +763,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	 * already.
 	 */
 	rev = intel_get_microcode_revision();
-	if (rev >= mc->hdr.rev) {
+	if (rev >= mc->hdr.rev && !ucode_load_same) {
 		ret = UCODE_OK;
 		goto out;
 	}
@@ -779,7 +779,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
 		return UCODE_ERROR;
 	}
 
-	if (bsp && rev != prev_rev) {
+	if (bsp && (rev != prev_rev || ucode_load_same)) {
 		pr_info("updated to revision 0x%x, date = %04x-%02x-%02x\n",
 			rev,
 			mc->hdr.date & 0xffff,
-- 
2.34.1

Re: [PATCH 09/13] x86/microcode: Add a generic mechanism to declare support for minrev

[Huang, Kai]

On Fri, 2022-10-14 at 13:09 -0700, Ashok Raj wrote:
> @@ -606,6 +606,7 @@ static ssize_t reload_store(struct device *dev,
>  	enum ucode_state tmp_ret = UCODE_OK;
>  	int bsp = boot_cpu_data.cpu_index;
>  	unsigned long val;
> +	int minrev;
>  	ssize_t ret = 0;
>  
>  	ret = kstrtoul(buf, 0, &val);
> @@ -621,8 +622,14 @@ static ssize_t reload_store(struct device *dev,
>  	if (ret)
>  		goto put;
>  
> -	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> -	pr_err("You should switch to early loading, if possible.\n");
> +	if (microcode_ops->check_minrev())
> +		minrev = microcode_ops->check_minrev();
> +
> +	if (!minrev) {
> +		pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> +		pr_err("You should switch to early loading, if possible.\n");
> +	}
> +

Hi Ashok,

IIUC a variable in stack isn't initialized to 0 automatically.  So looks if
check_minrev() callback is NULL, you will get an uninitialized 'minrev' in the
above if() statement check.

-- 
Thanks,
-Kai

Re: [PATCH 01/13] x86/microcode/intel: Print old and new rev after early microcode update

[Borislav Petkov]

On Fri, Oct 14, 2022 at 01:09:01PM -0700, Ashok Raj wrote:
> @@ -435,10 +435,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
>   * Print ucode update info.
>   */
>  static void
> -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> +print_ucode_info(u32 old_rev, struct ucode_cpu_info *uci, unsigned int date)
>  {
> -	pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
> -		     uci->cpu_sig.rev,
> +	pr_info_once("microcode updated early from 0x%x to revision 0x%x, date = %04x-%02x-%02x\n",

That already has the "microcode: " prefix from pr_fmt so make that

		     "early update: 0x%x -> 0x%x, date = %04x-%02x-%02x\n"

and fix the late update message too, pls, to look the same without the
"early" in there.

> @@ -479,13 +481,15 @@ static void print_ucode(struct ucode_cpu_info *uci)
>  
>  	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
>  	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
> +	old_rev_p = (u32 *)__pa_nodebug(&early_old_rev);

No, you should add a

	u32 prev_rev;

to struct ucode_cpu_info, save it there and access it where needed.

Then I can do the same prev -> next revision dumping on the AMD side.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 09/13] x86/microcode: Add a generic mechanism to declare support for minrev

[Ashok Raj]

On Tue, Oct 18, 2022 at 09:03:03PM -0700, Huang, Kai wrote:
> On Fri, 2022-10-14 at 13:09 -0700, Ashok Raj wrote:
> > @@ -606,6 +606,7 @@ static ssize_t reload_store(struct device *dev,
> >  	enum ucode_state tmp_ret = UCODE_OK;
> >  	int bsp = boot_cpu_data.cpu_index;
> >  	unsigned long val;
> > +	int minrev;
> >  	ssize_t ret = 0;
> >  
> >  	ret = kstrtoul(buf, 0, &val);
> > @@ -621,8 +622,14 @@ static ssize_t reload_store(struct device *dev,
> >  	if (ret)
> >  		goto put;
> >  
> > -	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> > -	pr_err("You should switch to early loading, if possible.\n");
> > +	if (microcode_ops->check_minrev())
> > +		minrev = microcode_ops->check_minrev();
> > +
> > +	if (!minrev) {
> > +		pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
> > +		pr_err("You should switch to early loading, if possible.\n");
> > +	}
> > +
> 
> Hi Ashok,
> 
> IIUC a variable in stack isn't initialized to 0 automatically.  So looks if
> check_minrev() callback is NULL, you will get an uninitialized 'minrev' in the
> above if() statement check.
> 

Thanks for the review Kai. 

Correct, that's a miss. I originally had a weak function bound for AMD.
But later i removed and forgot to initialize the local variable.

I've already queued this for next post. Just waiting for a few more review
comments to come on.

Cheers,
Ashok

Re: [PATCH 01/13] x86/microcode/intel: Print old and new rev after early microcode update

[Ashok Raj]

On Wed, Oct 19, 2022 at 12:12:00PM +0200, Borislav Petkov wrote:
> On Fri, Oct 14, 2022 at 01:09:01PM -0700, Ashok Raj wrote:
> > @@ -435,10 +435,10 @@ static bool load_builtin_intel_microcode(struct cpio_data *cp)
> >   * Print ucode update info.
> >   */
> >  static void
> > -print_ucode_info(struct ucode_cpu_info *uci, unsigned int date)
> > +print_ucode_info(u32 old_rev, struct ucode_cpu_info *uci, unsigned int date)
> >  {
> > -	pr_info_once("microcode updated early to revision 0x%x, date = %04x-%02x-%02x\n",
> > -		     uci->cpu_sig.rev,
> > +	pr_info_once("microcode updated early from 0x%x to revision 0x%x, date = %04x-%02x-%02x\n",
> 
> That already has the "microcode: " prefix from pr_fmt so make that
> 
> 		     "early update: 0x%x -> 0x%x, date = %04x-%02x-%02x\n"
> 
> and fix the late update message too, pls, to look the same without the
> "early" in there.

Good point, Fixed this.

> 
> > @@ -479,13 +481,15 @@ static void print_ucode(struct ucode_cpu_info *uci)
> >  
> >  	delay_ucode_info_p = (int *)__pa_nodebug(&delay_ucode_info);
> >  	current_mc_date_p = (int *)__pa_nodebug(&current_mc_date);
> > +	old_rev_p = (u32 *)__pa_nodebug(&early_old_rev);
> 
> No, you should add a
> 
> 	u32 prev_rev;
> 
> to struct ucode_cpu_info, save it there and access it where needed.

This seems like some 32bit restriction that we can't use printk early? I
wasn't sure, so I just followed what was done prior.

Today it seems we are doing the print just for BSP, not for all CPUs. Just
like what we do after update. Do you want to save this for all CPUs in
ucode_cpu_info[] for the previous revision?

Also its weird that cpu/common.c calls show_ucode_info_early(), but only
defined in cpu/microcode/intel.c. There is none in the AMD side.

Do you think we could have a generic function in cpu/microcore/core.c, that
calls vendor specific calls like the other early functions?
> 
> Then I can do the same prev -> next revision dumping on the AMD side.
> 

Cheers,
Ashok

Re: [PATCH 04/13] x86/x2apic: Support x2apic self IPI with NMI_VECTOR

[Ashok Raj]

On Fri, Oct 14, 2022 at 01:09:04PM -0700, Ashok Raj wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> X2APIC architecture introduced a dedicated register for sending self-IPI.
> Though highly optimized for performance, its semantics limit the delivery
> mode to fixed mode.  NMI vector is not supported, this created an
> inconsistent behavior between X2APIC and others.
> 
> This patch adds support for X2APIC NMI_VECTOR by fall back to the slower
> ICR method.
> 
> Suggested-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Forgot to add my sob here.. I;ll fix it in the resend.
> ---
>  arch/x86/kernel/apic/x2apic_phys.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/apic/x2apic_phys.c b/arch/x86/kernel/apic/x2apic_phys.c
> index 6bde05a86b4e..5f533b76adf6 100644
> --- a/arch/x86/kernel/apic/x2apic_phys.c
> +++ b/arch/x86/kernel/apic/x2apic_phys.c
> @@ -149,7 +149,11 @@ int x2apic_phys_pkg_id(int initial_apicid, int index_msb)
>  
>  void x2apic_send_IPI_self(int vector)
>  {
> -	apic_write(APIC_SELF_IPI, vector);
> +	if (vector == NMI_VECTOR)
> +		apic->send_IPI_mask(cpumask_of(smp_processor_id()),
> +				    NMI_VECTOR);
> +	else
> +		apic_write(APIC_SELF_IPI, vector);
>  }

Wanted to send this early if people are planning to test

Similar helper is required for legacy xapic as well. The lack of it helped
test the timeout path's :-).. I'll integrated it when i send the next round
with feedback once i have enough. I'll also send this in the next update.

diff --git a/arch/x86/kernel/apic/ipi.c b/arch/x86/kernel/apic/ipi.c
index 2a6509e8c840..e967c49609ef 100644
--- a/arch/x86/kernel/apic/ipi.c
+++ b/arch/x86/kernel/apic/ipi.c
@@ -239,7 +239,11 @@ void default_send_IPI_all(int vector)

 void default_send_IPI_self(int vector)
 {
-       __default_send_IPI_shortcut(APIC_DEST_SELF, vector);
+       if (unlikely(vector == NMI_VECTOR))
+               apic->send_IPI_mask(cpumask_of(smp_processor_id()),
+                                   NMI_VECTOR);
+       else
+               __default_send_IPI_shortcut(APIC_DEST_SELF, vector);
 }

 #ifdef CONFIG_X86_32



>  
>  static struct apic apic_x2apic_phys __ro_after_init = {
> -- 
> 2.34.1
> 

Re: [PATCH 13/13] x86/microcode/intel: Add ability to update microcode even if rev is unchanged

[Ashok Raj]

Hi Boris,

There is a minor bug in this force reload option I added for testing.

See below:

On Fri, Oct 14, 2022 at 01:09:13PM -0700, Ashok Raj wrote:

[snip]

> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index eb2caa74de01..632c7a1fcffb 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -23,6 +23,7 @@
>  #include <linux/miscdevice.h>
>  #include <linux/capability.h>
>  #include <linux/firmware.h>
> +#include <linux/debugfs.h>
>  #include <linux/kernel.h>
>  #include <linux/delay.h>
>  #include <linux/mutex.h>
> @@ -46,6 +47,7 @@
>  
>  static struct microcode_ops	*microcode_ops;
>  static bool dis_ucode_ldr = true;
> +bool ucode_load_same;
>  
>  bool initrd_gone;
>  
> @@ -542,11 +544,12 @@ static int __reload_late(void *info)
>  		goto wait_for_siblings;
>  	}
>  
> -	if (err >= UCODE_NFOUND) {
> -		if (err == UCODE_ERROR)
> +	if (ret || err >= UCODE_NFOUND) {
> +		if (err == UCODE_ERROR ||
> +		    (err == UCODE_NFOUND && !ucode_load_same)) {
>  			pr_warn("Error reloading microcode on CPU %d\n", cpu);
> -
> -		ret = -1;
> +			ret = -1;
> +		}
>  	}
>  
>  wait_for_siblings:
> @@ -636,9 +639,12 @@ static ssize_t reload_store(struct device *dev,
>  	}
>  
>  	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
> -	if (tmp_ret != UCODE_NEW)
> +	if (tmp_ret != UCODE_NEW && !ucode_load_same)
>  		goto put;
>  
> +	if (tmp_ret != UCODE_NEW)
> +		pr_info("Force loading ucode\n");
> +

The above needs additional check for UCODE_ERROR. If you are testing with
old microcode with minrev=0, noticed this went and tried to perform an
update without the UCODE_ERROR check. 

I've queued for the next update.


-       if (tmp_ret != UCODE_NEW)
+       if (tmp_ret == UCODE_ERROR ||
+           (tmp_ret != UCODE_NEW && !ucode_load_same))
                goto put;

+       if (tmp_ret != UCODE_NEW)
+               pr_info("Force loading ucode\n");
+

>  	mutex_lock(&microcode_mutex);
>  	ret = microcode_reload_late();
>  	mutex_unlock(&microcode_mutex);
> @@ -841,6 +847,7 @@ static const struct attribute_group cpu_root_microcode_group = {
>  static int __init microcode_init(void)
>  {
>  	struct cpuinfo_x86 *c = &boot_cpu_data;
> +	static struct dentry *dentry_ucode;
>  	int error;
>  
>  	if (dis_ucode_ldr)
> @@ -884,7 +891,12 @@ static int __init microcode_init(void)
>  	cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/microcode:online",
>  				  mc_cpu_online, mc_cpu_down_prep);
>  
> +	dentry_ucode = debugfs_create_dir("microcode", NULL);
> +	debugfs_create_bool("load_same", 0644, dentry_ucode, &ucode_load_same);
> +
>  	pr_info("Microcode Update Driver: v%s.", DRIVER_VERSION);
> +	pr_info("ucode_load_same is %s\n",
> +		ucode_load_same ? "enabled" : "disabled");
>  
>  	return 0;
>  
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index c61aa661ac2f..c9f1e6f5e53b 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -763,7 +763,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
>  	 * already.
>  	 */
>  	rev = intel_get_microcode_revision();
> -	if (rev >= mc->hdr.rev) {
> +	if (rev >= mc->hdr.rev && !ucode_load_same) {
>  		ret = UCODE_OK;
>  		goto out;
>  	}
> @@ -779,7 +779,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
>  		return UCODE_ERROR;
>  	}
>  
> -	if (bsp && rev != prev_rev) {
> +	if (bsp && (rev != prev_rev || ucode_load_same)) {
>  		pr_info("updated to revision 0x%x, date = %04x-%02x-%02x\n",
>  			rev,
>  			mc->hdr.date & 0xffff,
> -- 
> 2.34.1
> 

Re: [PATCH 02/13] x86/microcode: Do not load from filesystem for CPU hot add

[Borislav Petkov]

On Fri, Oct 14, 2022 at 01:09:02PM -0700, Ashok Raj wrote:
> When request_microcode_fw() is called, refresh_fw parameter must be set
> only when late loading. Currently during CPU hotplug, the path is as
> follows.
> 
> mc_device_add() -> microcode_init_cpu() ->
> 				request_microcode_fw(refresh_hw=true)

So that ugly refresh_fw thing was getting on my nerves for a while
now so I took a long stern look today. And ended up axing off a lot
of ancient crud which was simply getting in the way and is completely
useless.

The diffstat says it all:

 arch/x86/include/asm/microcode.h      |   4 +-
 arch/x86/kernel/cpu/intel.c           |   1 -
 arch/x86/kernel/cpu/microcode/amd.c   |   5 +-
 arch/x86/kernel/cpu/microcode/core.c  | 214 ++++++++++++++++++-------------------------------------------------------------
 arch/x86/kernel/cpu/microcode/intel.c |   3 +-
 5 files changed, 52 insertions(+), 175 deletions(-)

and initial smoke-testing with CPU hotplug and suspend looks good.

I'll hammer on it some more in the coming days.

Patches as a reply to this message.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 1/5] x86/microcode: Rip out the subsys interface gunk

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This is a left-over from the old days when CPU hotplug wasn't as robust
as it is now. Currently, microcode gets loaded early on the CPU init
path and there's no need to attempt to load it again.

The only thing that the subsys interface init path was doing is adding
the

  /sys/devices/system/cpu/cpu*/microcode/

hierarchy.

So add a function which gets called on each CPU after all the necessary
driver setup has happened.

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6a41cee242f6..a3aedc93afd9 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -601,8 +601,8 @@ static enum ucode_state microcode_resume_cpu(int cpu)
 
 static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 {
-	enum ucode_state ustate;
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	enum ucode_state ustate;
 
 	if (uci->valid)
 		return UCODE_OK;
@@ -636,44 +636,6 @@ static enum ucode_state microcode_update_cpu(int cpu)
 	return microcode_init_cpu(cpu, false);
 }
 
-static int mc_device_add(struct device *dev, struct subsys_interface *sif)
-{
-	int err, cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return 0;
-
-	pr_debug("CPU%d added\n", cpu);
-
-	err = sysfs_create_group(&dev->kobj, &mc_attr_group);
-	if (err)
-		return err;
-
-	if (microcode_init_cpu(cpu, true) == UCODE_ERROR)
-		return -EINVAL;
-
-	return err;
-}
-
-static void mc_device_remove(struct device *dev, struct subsys_interface *sif)
-{
-	int cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return;
-
-	pr_debug("CPU%d removed\n", cpu);
-	microcode_fini_cpu(cpu);
-	sysfs_remove_group(&dev->kobj, &mc_attr_group);
-}
-
-static struct subsys_interface mc_cpu_interface = {
-	.name			= "microcode",
-	.subsys			= &cpu_subsys,
-	.add_dev		= mc_device_add,
-	.remove_dev		= mc_device_remove,
-};
-
 /**
  * microcode_bsp_resume - Update boot CPU microcode during resume.
  */
@@ -713,6 +675,9 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	struct device *dev;
 
 	dev = get_cpu_device(cpu);
+
+	microcode_fini_cpu(cpu);
+
 	/* Suspend is in progress, only remove the interface */
 	sysfs_remove_group(&dev->kobj, &mc_attr_group);
 	pr_debug("CPU%d removed\n", cpu);
@@ -720,6 +685,18 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	return 0;
 }
 
+static void setup_online_cpu(void *info)
+{
+	int cpu = smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	memset(uci, 0, sizeof(*uci));
+
+	microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
+	mc_cpu_online(cpu);
+}
+
 static struct attribute *cpu_root_microcode_attrs[] = {
 #ifdef CONFIG_MICROCODE_LATE_LOADING
 	&dev_attr_reload.attr,
@@ -755,23 +732,19 @@ static int __init microcode_init(void)
 	if (IS_ERR(microcode_pdev))
 		return PTR_ERR(microcode_pdev);
 
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-	error = subsys_interface_register(&mc_cpu_interface);
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
-	if (error)
-		goto out_pdev;
-
 	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
 				   &cpu_root_microcode_group);
 
 	if (error) {
 		pr_err("Error creating microcode group!\n");
-		goto out_driver;
+		goto out_pdev;
 	}
 
+	/* Do per-CPU setup */
+	cpus_read_lock();
+	on_each_cpu(setup_online_cpu, NULL, 0);
+	cpus_read_unlock();
+
 	register_syscore_ops(&mc_syscore_ops);
 	cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting",
 				  mc_cpu_starting, NULL);
@@ -782,15 +755,6 @@ static int __init microcode_init(void)
 
 	return 0;
 
- out_driver:
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-
-	subsys_interface_unregister(&mc_cpu_interface);
-
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
  out_pdev:
 	platform_device_unregister(microcode_pdev);
 	return error;
-- 
2.35.1

[PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Get rid of all the IPI-sending functions and their wrappers and use
those which are supposed to be called on each CPU.

Thus:

- microcode_init_cpu() gets called on each CPU on init, applying any new
  microcode that the driver might've found on the filesystem.

- mc_cpu_starting() simply tries to apply cached microcode as this is
  the cpuhp starting callback which gets called on CPU resume too.

And since the driver init function is a late initcall, there is
filesystem by then so a new firmware load attempt can simply be done. In
case a new one is there. Which is weird to begin with - how would the
initrd contain an older revision than what's on the fs since former gets
created by using the blobs from the filesystem.

Oh well, it is cheap to do so why not...

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index a3aedc93afd9..0aa6609e748c 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -319,60 +319,6 @@ void reload_early_microcode(void)
 	}
 }
 
-static void collect_cpu_info_local(void *arg)
-{
-	struct cpu_info_ctx *ctx = arg;
-
-	ctx->err = microcode_ops->collect_cpu_info(smp_processor_id(),
-						   ctx->cpu_sig);
-}
-
-static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
-{
-	struct cpu_info_ctx ctx = { .cpu_sig = cpu_sig, .err = 0 };
-	int ret;
-
-	ret = smp_call_function_single(cpu, collect_cpu_info_local, &ctx, 1);
-	if (!ret)
-		ret = ctx.err;
-
-	return ret;
-}
-
-static int collect_cpu_info(int cpu)
-{
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	int ret;
-
-	memset(uci, 0, sizeof(*uci));
-
-	ret = collect_cpu_info_on_target(cpu, &uci->cpu_sig);
-	if (!ret)
-		uci->valid = 1;
-
-	return ret;
-}
-
-static void apply_microcode_local(void *arg)
-{
-	enum ucode_state *err = arg;
-
-	*err = microcode_ops->apply_microcode(smp_processor_id());
-}
-
-static int apply_microcode_on_target(int cpu)
-{
-	enum ucode_state err;
-	int ret;
-
-	ret = smp_call_function_single(cpu, apply_microcode_local, &err, 1);
-	if (!ret) {
-		if (err == UCODE_ERROR)
-			ret = 1;
-	}
-	return ret;
-}
-
 /* fake device for request_firmware */
 static struct platform_device	*microcode_pdev;
 
@@ -458,7 +404,7 @@ static int __reload_late(void *info)
 	 * below.
 	 */
 	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
-		apply_microcode_local(&err);
+		err = microcode_ops->apply_microcode(cpu);
 	else
 		goto wait_for_siblings;
 
@@ -480,7 +426,7 @@ static int __reload_late(void *info)
 	 * revision.
 	 */
 	if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu)
-		apply_microcode_local(&err);
+		err = microcode_ops->apply_microcode(cpu);
 
 	return ret;
 }
@@ -589,51 +535,15 @@ static void microcode_fini_cpu(int cpu)
 		microcode_ops->microcode_fini_cpu(cpu);
 }
 
-static enum ucode_state microcode_resume_cpu(int cpu)
-{
-	if (apply_microcode_on_target(cpu))
-		return UCODE_ERROR;
-
-	pr_debug("CPU%d updated upon resume\n", cpu);
-
-	return UCODE_OK;
-}
-
-static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
-{
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	enum ucode_state ustate;
-
-	if (uci->valid)
-		return UCODE_OK;
-
-	if (collect_cpu_info(cpu))
-		return UCODE_ERROR;
-
-	/* --dimm. Trigger a delayed update? */
-	if (system_state != SYSTEM_RUNNING)
-		return UCODE_NFOUND;
-
-	ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev, refresh_fw);
-	if (ustate == UCODE_NEW) {
-		pr_debug("CPU%d updated upon init\n", cpu);
-		apply_microcode_on_target(cpu);
-	}
-
-	return ustate;
-}
-
-static enum ucode_state microcode_update_cpu(int cpu)
+static enum ucode_state microcode_init_cpu(int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
-	/* Refresh CPU microcode revision after resume. */
-	collect_cpu_info(cpu);
+	memset(uci, 0, sizeof(*uci));
 
-	if (uci->valid)
-		return microcode_resume_cpu(cpu);
+	microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
 
-	return microcode_init_cpu(cpu, false);
+	return microcode_ops->apply_microcode(cpu);
 }
 
 /**
@@ -651,14 +561,14 @@ void microcode_bsp_resume(void)
 }
 
 static struct syscore_ops mc_syscore_ops = {
-	.resume			= microcode_bsp_resume,
+	.resume	= microcode_bsp_resume,
 };
 
 static int mc_cpu_starting(unsigned int cpu)
 {
-	microcode_update_cpu(cpu);
-	pr_debug("CPU%d added\n", cpu);
-	return 0;
+	enum ucode_state err = microcode_ops->apply_microcode(cpu);
+
+	return err == UCODE_ERROR;
 }
 
 static int mc_cpu_online(unsigned int cpu)
@@ -688,11 +598,13 @@ static int mc_cpu_down_prep(unsigned int cpu)
 static void setup_online_cpu(void *info)
 {
 	int cpu = smp_processor_id();
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
-	memset(uci, 0, sizeof(*uci));
+	enum ucode_state err;
 
-	microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+	err = microcode_init_cpu(cpu);
+	if (err == UCODE_ERROR) {
+		pr_err("Error applying microcode on CPU%d\n", cpu);
+		return;
+	}
 
 	mc_cpu_online(cpu);
 }
@@ -740,6 +652,13 @@ static int __init microcode_init(void)
 		goto out_pdev;
 	}
 
+	/*
+	 * Try to load microcode once on the BSP in case the initrd has older revision.
+	 *  Frankly, I have no clue how that can happen but hey, loading here is cheap so
+	 * why not.
+	 */
+	microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, &microcode_pdev->dev, true);
+
 	/* Do per-CPU setup */
 	cpus_read_lock();
 	on_each_cpu(setup_online_cpu, NULL, 0);
-- 
2.35.1

[PATCH 3/5] x86/microcode: Kill refresh_fw

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

request_microcode_fw() can always request firmware now so drop this
superfluous argument.

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

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 74ecc2bd6cd0..d4c36fbd1d39 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -33,8 +33,7 @@ enum ucode_state {
 };
 
 struct microcode_ops {
-	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
-						  bool refresh_fw);
+	enum ucode_state (*request_microcode_fw) (int cpu, struct device *);
 
 	void (*microcode_fini_cpu) (int cpu);
 
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index e7410e98fc1f..b103d5e5f447 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -891,8 +891,7 @@ load_microcode_amd(bool save, u8 family, const u8 *data, size_t size)
  *
  * These might be larger than 2K.
  */
-static enum ucode_state request_microcode_amd(int cpu, struct device *device,
-					      bool refresh_fw)
+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);
@@ -901,7 +900,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	const struct firmware *fw;
 
 	/* reload ucode container only on the boot cpu */
-	if (!refresh_fw || !bsp)
+	if (!bsp)
 		return UCODE_OK;
 
 	if (c->x86 >= 0x15)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 0aa6609e748c..8401b7235a68 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -477,7 +477,7 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		goto put;
 
-	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
+	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev);
 	if (tmp_ret != UCODE_NEW)
 		goto put;
 
@@ -657,7 +657,7 @@ static int __init microcode_init(void)
 	 *  Frankly, I have no clue how that can happen but hey, loading here is cheap so
 	 * why not.
 	 */
-	microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, &microcode_pdev->dev, true);
+	microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, &microcode_pdev->dev);
 
 	/* Do per-CPU setup */
 	cpus_read_lock();
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 1fcbd671f1df..8c35c70029bf 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -885,8 +885,7 @@ static bool is_blacklisted(unsigned int cpu)
 	return false;
 }
 
-static enum ucode_state request_microcode_fw(int cpu, struct device *device,
-					     bool refresh_fw)
+static enum ucode_state request_microcode_fw(int cpu, struct device *device)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	const struct firmware *firmware;
-- 
2.35.1

[PATCH 4/5] x86/microcode: Do some minor fixups

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

Improve debugging printks and fixup formatting.

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 8401b7235a68..6b025143b073 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -568,6 +568,8 @@ static int mc_cpu_starting(unsigned int cpu)
 {
 	enum ucode_state err = microcode_ops->apply_microcode(cpu);
 
+	pr_debug("%s: CPU%d, err: %d\n", __func__, cpu, err);
+
 	return err == UCODE_ERROR;
 }
 
@@ -590,7 +592,7 @@ static int mc_cpu_down_prep(unsigned int cpu)
 
 	/* Suspend is in progress, only remove the interface */
 	sysfs_remove_group(&dev->kobj, &mc_attr_group);
-	pr_debug("CPU%d removed\n", cpu);
+	pr_debug("%s: CPU%d\n", __func__, cpu);
 
 	return 0;
 }
@@ -639,14 +641,11 @@ static int __init microcode_init(void)
 	if (!microcode_ops)
 		return -ENODEV;
 
-	microcode_pdev = platform_device_register_simple("microcode", -1,
-							 NULL, 0);
+	microcode_pdev = platform_device_register_simple("microcode", -1, NULL, 0);
 	if (IS_ERR(microcode_pdev))
 		return PTR_ERR(microcode_pdev);
 
-	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
-				   &cpu_root_microcode_group);
-
+	error = sysfs_create_group(&cpu_subsys.dev_root->kobj, &cpu_root_microcode_group);
 	if (error) {
 		pr_err("Error creating microcode group!\n");
 		goto out_pdev;
-- 
2.35.1

[PATCH 5/5] x86/microcode: Drop struct ucode_cpu_info.valid

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

It is not needed anymore.

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

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index d4c36fbd1d39..d5a58bde091c 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -49,7 +49,6 @@ struct microcode_ops {
 
 struct ucode_cpu_info {
 	struct cpu_signature	cpu_sig;
-	int			valid;
 	void			*mc;
 };
 extern struct ucode_cpu_info ucode_cpu_info[];
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 2d7ea5480ec3..beb8ca596784 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -210,7 +210,6 @@ int intel_cpu_collect_info(struct ucode_cpu_info *uci)
 	csig.rev = intel_get_microcode_revision();
 
 	uci->cpu_sig = csig;
-	uci->valid = 1;
 
 	return 0;
 }
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6b025143b073..026bf644344a 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -554,9 +554,9 @@ void microcode_bsp_resume(void)
 	int cpu = smp_processor_id();
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
-	if (uci->valid && uci->mc)
+	if (uci->mc)
 		microcode_ops->apply_microcode(cpu);
-	else if (!uci->mc)
+	else
 		reload_early_microcode();
 }
 
-- 
2.35.1

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Ashok Raj]

On Wed, Oct 19, 2022 at 07:54:23PM +0200, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Get rid of all the IPI-sending functions and their wrappers and use
> those which are supposed to be called on each CPU.

Good cleanup overall!.. Trying to apply them and do a quick test.

[snip]

> 
> +	/*
> +	 * Try to load microcode once on the BSP in case the initrd has older revision.
> +	 *  Frankly, I have no clue how that can happen but hey, loading here is cheap so
> +	 * why not.
> +	 */
> +	microcode_ops->request_microcode_fw(boot_cpu_data.cpu_index, &microcode_pdev->dev, true);
> +

You need to call a microcode_ops->apply_microcode() too if you want to
apply. But you also want to pay attention to the return code too and call
it appropriately.

>  	/* Do per-CPU setup */
>  	cpus_read_lock();
>  	on_each_cpu(setup_online_cpu, NULL, 0);
> -- 
> 2.35.1
> 

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

On Wed, Oct 19, 2022 at 12:22:57PM -0700, Ashok Raj wrote:
> Good cleanup overall!.. Trying to apply them and do a quick test.

Thx. Use 6.1-rc1 as base.

> You need to call a microcode_ops->apply_microcode() too if you want to
> apply.

That's done ...

> But you also want to pay attention to the return code too and call it
> appropriately.

Ah ok, yes, I need to check ->request_microcode_fw's retval.

> >  	/* Do per-CPU setup */
> >  	cpus_read_lock();
> >  	on_each_cpu(setup_online_cpu, NULL, 0);

... in here.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

On Wed, Oct 19, 2022 at 09:37:59PM +0200, Borislav Petkov wrote:
> Ah ok, yes, I need to check ->request_microcode_fw's retval.

On a second thought, no I don't have to: the request routine will try to
load any new microcode but if it fails, ->apply_microcode later simply
won't find it in the cache. And that's perfectly fine.

setup_online_cpu() still needs to run on each CPU unconditionally,
though, in order to setup the sysfs groups.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Ashok Raj]

On Thu, Oct 20, 2022 at 10:18:01AM +0200, Borislav Petkov wrote:
> On Wed, Oct 19, 2022 at 09:37:59PM +0200, Borislav Petkov wrote:
> > Ah ok, yes, I need to check ->request_microcode_fw's retval.
> 
> On a second thought, no I don't have to: the request routine will try to
> load any new microcode but if it fails, ->apply_microcode later simply
> won't find it in the cache. And that's perfectly fine.
> 
> setup_online_cpu() still needs to run on each CPU unconditionally,
> though, in order to setup the sysfs groups.

That's true, its benign for this step.

I tried rebase everything I have, (i need to drop 2 of my patches after
your 5 patch series is applied on top of 6.1.0-rc1. 

Everything works fine, which is good news.

Except I can't get this new load you added in microcode_init() to work. 
It always gets UCODE_NFOUND.

- BIOS had version X
- Early applied Y > X
- I copied a new version Z > Y. 

But i only see the early loaded version. I haven't looked at it yet, but
will try to look today. 

Cheers,
Ashok

[PATCH -v2 1/5] x86/microcode: Rip out the subsys interface gunk

[Borislav Petkov]

A new version of this one:

From: Borislav Petkov <bp@suse.de>
Date: Wed, 19 Oct 2022 18:13:06 +0200
Subject: x86/microcode: Rip out the subsys interface gunk

This is a left-over from the old days when CPU hotplug wasn't as robust
as it is now. Currently, microcode gets loaded early on the CPU init
path and there's no need to attempt to load it again, which that subsys
interface callback is doing.

The only other thing that the subsys interface init path was doing is
adding the

  /sys/devices/system/cpu/cpu*/microcode/

hierarchy.

So add a function which gets called on each CPU after all the necessary
driver setup has happened. Use schedule_on_each_cpu() which can block
because the sysfs creating code does kmem_cache_zalloc() which can block
too and the initial version of this where it did that setup in an IPI
handler of on_each_cpu() can cause a deadlock of the sort:

  lock(fs_reclaim);
  <Interrupt>
    lock(fs_reclaim);

as the IPI handler runs in IRQ context.

Signed-off-by: Borislav Petkov <bp@suse.de>

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6a41cee242f6..4c222e667567 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -601,8 +601,8 @@ static enum ucode_state microcode_resume_cpu(int cpu)
 
 static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 {
-	enum ucode_state ustate;
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	enum ucode_state ustate;
 
 	if (uci->valid)
 		return UCODE_OK;
@@ -636,44 +636,6 @@ static enum ucode_state microcode_update_cpu(int cpu)
 	return microcode_init_cpu(cpu, false);
 }
 
-static int mc_device_add(struct device *dev, struct subsys_interface *sif)
-{
-	int err, cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return 0;
-
-	pr_debug("CPU%d added\n", cpu);
-
-	err = sysfs_create_group(&dev->kobj, &mc_attr_group);
-	if (err)
-		return err;
-
-	if (microcode_init_cpu(cpu, true) == UCODE_ERROR)
-		return -EINVAL;
-
-	return err;
-}
-
-static void mc_device_remove(struct device *dev, struct subsys_interface *sif)
-{
-	int cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return;
-
-	pr_debug("CPU%d removed\n", cpu);
-	microcode_fini_cpu(cpu);
-	sysfs_remove_group(&dev->kobj, &mc_attr_group);
-}
-
-static struct subsys_interface mc_cpu_interface = {
-	.name			= "microcode",
-	.subsys			= &cpu_subsys,
-	.add_dev		= mc_device_add,
-	.remove_dev		= mc_device_remove,
-};
-
 /**
  * microcode_bsp_resume - Update boot CPU microcode during resume.
  */
@@ -713,6 +675,9 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	struct device *dev;
 
 	dev = get_cpu_device(cpu);
+
+	microcode_fini_cpu(cpu);
+
 	/* Suspend is in progress, only remove the interface */
 	sysfs_remove_group(&dev->kobj, &mc_attr_group);
 	pr_debug("CPU%d removed\n", cpu);
@@ -720,6 +685,18 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	return 0;
 }
 
+static void setup_online_cpu(struct work_struct *work)
+{
+	int cpu = smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	memset(uci, 0, sizeof(*uci));
+
+	microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
+	mc_cpu_online(cpu);
+}
+
 static struct attribute *cpu_root_microcode_attrs[] = {
 #ifdef CONFIG_MICROCODE_LATE_LOADING
 	&dev_attr_reload.attr,
@@ -755,23 +732,17 @@ static int __init microcode_init(void)
 	if (IS_ERR(microcode_pdev))
 		return PTR_ERR(microcode_pdev);
 
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-	error = subsys_interface_register(&mc_cpu_interface);
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
-	if (error)
-		goto out_pdev;
-
 	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
 				   &cpu_root_microcode_group);
 
 	if (error) {
 		pr_err("Error creating microcode group!\n");
-		goto out_driver;
+		goto out_pdev;
 	}
 
+	/* Do per-CPU setup */
+	schedule_on_each_cpu(setup_online_cpu);
+
 	register_syscore_ops(&mc_syscore_ops);
 	cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting",
 				  mc_cpu_starting, NULL);
@@ -782,15 +753,6 @@ static int __init microcode_init(void)
 
 	return 0;
 
- out_driver:
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-
-	subsys_interface_unregister(&mc_cpu_interface);
-
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
  out_pdev:
 	platform_device_unregister(microcode_pdev);
 	return error;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

On Thu, Oct 20, 2022 at 08:04:53AM -0700, Ashok Raj wrote:
> - BIOS had version X
> - Early applied Y > X
> - I copied a new version Z > Y.

When exactly do you copy a new version?

Please write down the exact steps you're doing.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Ashok Raj]

On Fri, Oct 21, 2022 at 11:28:10AM +0200, Borislav Petkov wrote:
> On Thu, Oct 20, 2022 at 08:04:53AM -0700, Ashok Raj wrote:
> > - BIOS had version X
> > - Early applied Y > X
> > - I copied a new version Z > Y.
> 
> When exactly do you copy a new version?
> 
> Please write down the exact steps you're doing.

- Place a new version of microcode in /lib/firmware/intel-ucode/
- make install, it also ends up creating a new initrd
  image with the added microcode.
- Now put another new version in the same default directory.

0000000 0001 0000 0081 2b00 2022 0906 06f8 0008
------------------^^^^^^^^^ New revision
0000020 220a 855c 0001 0000 0087 0000 7b80 0008
0000040 7c00 0008 0000 0000 0041 2b00 0000 0000
----------------------------^^^^^^^^^ minrev

- reboot

During boot, i see early update message

microcode: early update: 0x2b000041 -> 0x2b000070, date = 2022-08-22

0x41 is the version from BIOS, 0x70 is what got updated in early boot.
0x81 is sitting in the default directory.

After boot expect to see 0x81 as the revision. But its left with what was
loaded at initrd image. The filesystem still had 0x81, and can successfuly
load after boot is completed.

Cheers,
Ashok

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

On Fri, Oct 21, 2022 at 03:21:03AM -0700, Ashok Raj wrote:
> After boot expect to see 0x81 as the revision. But its left with what
> was loaded at initrd image.

Are you testing with all patches, including yours or just the 5 I sent
you?

> The filesystem still had 0x81, and can successfuly load after boot is
> completed.

By doing late load through sysfs or what does "can successfuly load"
mean exactly?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Ashok Raj]

Hi Boris

On Fri, Oct 21, 2022 at 12:57:32PM +0200, Borislav Petkov wrote:
> On Fri, Oct 21, 2022 at 03:21:03AM -0700, Ashok Raj wrote:
> > After boot expect to see 0x81 as the revision. But its left with what
> > was loaded at initrd image.
> 
> Are you testing with all patches, including yours or just the 5 I sent
> you?

I tested just with only your patches

I use Ubuntu

Did a unmkinitrd and verified that 0x70 is in it. After extract.

araj@araj-ucode:/tmp/temp/early2/kernel/x86/microcode$ od -x GenuineIntel.bin | grep "0070 2b00"
12102000 0001 0000 0070 2b00 2022 0822 06f8 0008
-------------------^^^^ ^^^^

12102060 0000 0000 00e0 0000 0001 0003 0070 2b00

Patches in my work tree.. I use stgit mostly
araj@araj-ucode:~/work/linux.git.trees$ qyet
     1	+ v2-bp1
     2	+ bp2
     3	+ bp3
     4	+ bp4
     5	+ bp5

Now just copy the 0x81 ucode in /lib/firmware/intel-ucode/
araj@araj-ucode:~/work/ucode$ od -x /lib/firmware/intel-ucode/06-8f-05 | head
0000000 0001 0000 0081 2b00 2022 0906 06f8 0008
------------------^^^^ ^^^^

After reboot that file is unchanged.

dmesg shows:
[    0.000000] microcode: early update: 0x2b000041 -> 0x2b000070, date = 2022-08
-22

araj@araj-ucode:~$ cat /proc/cpuinfo | grep microcode | sort -u
microcode	: 0x2b000070

root@araj-ucode:/home/araj# echo 1 > /sys/devices/system/cpu/microcode/reload 
root@araj-ucode:/home/araj# cat /proc/cpuinfo | grep microcode | sort -u
microcode	: 0x2b000081

dmesg:
[  654.103784] microcode: Attempting late microcode loading - it is dangerous and taints the kernel.
[  654.113807] microcode: You should switch to early loading, if possible.
[  654.613154] microcode: update 0x2b000070 -> 0x2b000081, date = 2022-09-06


> 
> > The filesystem still had 0x81, and can successfuly load after boot is
> > completed.
> 
> By doing late load through sysfs or what does "can successfuly load"
> mean exactly?

I meant by echo 1 > reload

Cheers,
Ashok

Re: [PATCH 2/5] x86/microcode: Simplify init path even more

[Borislav Petkov]

On Fri, Oct 21, 2022 at 04:39:55AM -0700, Ashok Raj wrote:
> I tested just with only your patches

Yeah, it can't work:

[    1.365806] microcode: microcode_init: will request
[    1.365812] microcode: request_microcode_fw: name: intel-ucode/06-3a-09
[    1.365999] platform microcode: loading /lib/firmware/updates/6.1.0-rc1+/intel-ucode/06-3a-09 failed for no such file or directory.
[    1.366032] platform microcode: loading /lib/firmware/updates/intel-ucode/06-3a-09 failed for no such file or directory.
[    1.366061] platform microcode: loading /lib/firmware/6.1.0-rc1+/intel-ucode/06-3a-09 failed for no such file or directory.
[    1.366088] platform microcode: loading /lib/firmware/intel-ucode/06-3a-09 failed for no such file or directory.
[    1.366092] firmware_class: _request_firmware: fw_get_filesystem_firmware: retval: -2
[    1.366096] platform microcode: Direct firmware load for intel-ucode/06-3a-09 failed with error -2
[    1.366174] firmware_class: request_firmware_direct: ret: -2
[    1.366178] microcode: data file intel-ucode/06-3a-09 load failed
[    1.366248] microcode: sig=0x306a9, pf=0x10, revision=0x21
[    1.366380] microcode: Microcode Update Driver: v2.2.

because that happens before the hdd driver has even been loaded yet.

My assumption that late_initcall() is late enough was wrong.

But that's ok - it was a weird use case anyway. One is supposed to use
early loading anyway.

Ok, lemme remove that loading attempt and test them more.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

[PATCH 1/5] x86/microcode: Rip out the subsys interface gunk

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>

This is a left-over from the old days when CPU hotplug wasn't as robust
as it is now. Currently, microcode gets loaded early on the CPU init
path and there's no need to attempt to load it again, which that subsys
interface callback is doing.

The only other thing that the subsys interface init path was doing is
adding the

  /sys/devices/system/cpu/cpu*/microcode/

hierarchy.

So add a function which gets called on each CPU after all the necessary
driver setup has happened. Use schedule_on_each_cpu() which can block
because the sysfs creating code does kmem_cache_zalloc() which can block
too and the initial version of this where it did that setup in an IPI
handler of on_each_cpu() can cause a deadlock of the sort:

  lock(fs_reclaim);
  <Interrupt>
    lock(fs_reclaim);

as the IPI handler runs in IRQ context.

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 6a41cee242f6..4c222e667567 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -601,8 +601,8 @@ static enum ucode_state microcode_resume_cpu(int cpu)
 
 static enum ucode_state microcode_init_cpu(int cpu, bool refresh_fw)
 {
-	enum ucode_state ustate;
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	enum ucode_state ustate;
 
 	if (uci->valid)
 		return UCODE_OK;
@@ -636,44 +636,6 @@ static enum ucode_state microcode_update_cpu(int cpu)
 	return microcode_init_cpu(cpu, false);
 }
 
-static int mc_device_add(struct device *dev, struct subsys_interface *sif)
-{
-	int err, cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return 0;
-
-	pr_debug("CPU%d added\n", cpu);
-
-	err = sysfs_create_group(&dev->kobj, &mc_attr_group);
-	if (err)
-		return err;
-
-	if (microcode_init_cpu(cpu, true) == UCODE_ERROR)
-		return -EINVAL;
-
-	return err;
-}
-
-static void mc_device_remove(struct device *dev, struct subsys_interface *sif)
-{
-	int cpu = dev->id;
-
-	if (!cpu_online(cpu))
-		return;
-
-	pr_debug("CPU%d removed\n", cpu);
-	microcode_fini_cpu(cpu);
-	sysfs_remove_group(&dev->kobj, &mc_attr_group);
-}
-
-static struct subsys_interface mc_cpu_interface = {
-	.name			= "microcode",
-	.subsys			= &cpu_subsys,
-	.add_dev		= mc_device_add,
-	.remove_dev		= mc_device_remove,
-};
-
 /**
  * microcode_bsp_resume - Update boot CPU microcode during resume.
  */
@@ -713,6 +675,9 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	struct device *dev;
 
 	dev = get_cpu_device(cpu);
+
+	microcode_fini_cpu(cpu);
+
 	/* Suspend is in progress, only remove the interface */
 	sysfs_remove_group(&dev->kobj, &mc_attr_group);
 	pr_debug("CPU%d removed\n", cpu);
@@ -720,6 +685,18 @@ static int mc_cpu_down_prep(unsigned int cpu)
 	return 0;
 }
 
+static void setup_online_cpu(struct work_struct *work)
+{
+	int cpu = smp_processor_id();
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	memset(uci, 0, sizeof(*uci));
+
+	microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
+	mc_cpu_online(cpu);
+}
+
 static struct attribute *cpu_root_microcode_attrs[] = {
 #ifdef CONFIG_MICROCODE_LATE_LOADING
 	&dev_attr_reload.attr,
@@ -755,23 +732,17 @@ static int __init microcode_init(void)
 	if (IS_ERR(microcode_pdev))
 		return PTR_ERR(microcode_pdev);
 
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-	error = subsys_interface_register(&mc_cpu_interface);
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
-	if (error)
-		goto out_pdev;
-
 	error = sysfs_create_group(&cpu_subsys.dev_root->kobj,
 				   &cpu_root_microcode_group);
 
 	if (error) {
 		pr_err("Error creating microcode group!\n");
-		goto out_driver;
+		goto out_pdev;
 	}
 
+	/* Do per-CPU setup */
+	schedule_on_each_cpu(setup_online_cpu);
+
 	register_syscore_ops(&mc_syscore_ops);
 	cpuhp_setup_state_nocalls(CPUHP_AP_MICROCODE_LOADER, "x86/microcode:starting",
 				  mc_cpu_starting, NULL);
@@ -782,15 +753,6 @@ static int __init microcode_init(void)
 
 	return 0;
 
- out_driver:
-	cpus_read_lock();
-	mutex_lock(&microcode_mutex);
-
-	subsys_interface_unregister(&mc_cpu_interface);
-
-	mutex_unlock(&microcode_mutex);
-	cpus_read_unlock();
-
  out_pdev:
 	platform_device_unregister(microcode_pdev);
 	return error;
-- 
2.35.1