[PATCH v2 0/4] Making microcode loading more robust

[PATCH v2 0/4] Making microcode loading more robust

[Ashok Raj]

Hi Boris,

Here is an update that handles siblings in NMI loop.

Changes since v1:
Patch 1,2,3: No Change.
Patch 4: 
- Move from lazy NMI handling to sending siblings in NMI early. This makes
  things more robust, since there might be something we could trip over
  before the handler is called that might be patched by the update.
  [ Suggested by Andy Cooper ]

TBD: Handle DR7, temporarily disable until the update is done
   [ Suggested by Andy Lutomirsky ]


Ashok Raj (4):
  x86/microcode/intel: Check against CPU signature before saving
    microcode
  x86/microcode/intel: Allow a late-load only if a min rev is specified
  x86/microcode: Avoid any chance of MCE's during microcode update
  x86/microcode: Place siblings in NMI loop while update in progress

 arch/x86/include/asm/mce.h             |   4 +
 arch/x86/include/asm/microcode_intel.h |   4 +-
 arch/x86/kernel/cpu/mce/core.c         |   9 ++
 arch/x86/kernel/cpu/microcode/core.c   | 202 ++++++++++++++++++++++++-
 arch/x86/kernel/cpu/microcode/intel.c  |  34 ++++-
 5 files changed, 242 insertions(+), 11 deletions(-)

-- 
2.32.0

[PATCH v2 1/4] x86/microcode/intel: Check against CPU signature before saving microcode

[Ashok Raj]

When save_microcode_patch() is looking to replace an existing microcode in
the cache, current code is *only* checks the CPU sig/pf in the main
header. Microcode can carry additional sig/pf combinations in the extended
signature table, which is completely missed today.

For e.g. Current patch is a multi-stepping patch and new incoming patch is
a specific patch just for this CPUs stepping.

patch1:
fms3 <--- header FMS
...
ext_sig:
fms1
fms2

patch2: new
fms2 <--- header FMS

Current code takes only fms3 and checks with patch2 fms2.

saved_patch.header.fms3 != new_patch.header.fms2, so save_microcode_patch
saves it to the end of list instead of replacing patch1 with patch2.

There is no functional user observable issue since find_patch() skips
patch versions that are <= current_patch and will land on patch2 properly.

Nevertheless this will just end up storing every patch that isn't required.
Kernel just needs to store the latest patch. Otherwise its a memory leak
that sits in kernel and never used.

Cc: stable@vger.kernel.org
Fixes: fe055896c040 ("x86/microcode: Merge the early microcode loader")
Tested-by: William Xie <william.xie@intel.com>
Reported-by: William Xie <william.xie@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 025c8f0cd948..c4b11e2fbe33 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -114,10 +114,18 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 
 	list_for_each_entry_safe(iter, tmp, &microcode_cache, plist) {
 		mc_saved_hdr = (struct microcode_header_intel *)iter->data;
-		sig	     = mc_saved_hdr->sig;
-		pf	     = mc_saved_hdr->pf;
 
-		if (find_matching_signature(data, sig, pf)) {
+		sig = uci->cpu_sig.sig;
+		pf  = uci->cpu_sig.pf;
+
+		/*
+		 * Compare the current CPUs signature with the ones in the
+		 * cache to identify the right candidate to replace. At any
+		 * given time, we should have no more than one valid patch
+		 * file for a given CPU fms+pf in the cache list.
+		 */
+
+		if (find_matching_signature(iter->data, sig, pf)) {
 			prev_found = true;
 
 			if (mc_hdr->rev <= mc_saved_hdr->rev)
-- 
2.32.0

[PATCH v2 2/4] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Ashok Raj]

In general users don't have the necessary information to determine
whether a late-load 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.

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

Thomas made some suggestions on how meta-data in the microcode file could
provide Linux with information to decide if the new microcode is suitable
candidate for late-load. 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. The criteria for a microcode to
be a viable late-load candidate is that no CPUID or OS visible MSR features
are removed with respect to an earlier version of the microcode.

Pseudocode for late-load is as follows:

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

Any microcode that removes a feature will set the min_version to itself.
This will enforce this microcode is not suitable for late-loading.

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] microcode: Header MUST specify min version for late-load

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: Current revision 0x8f685300 is too old to update,
must be at 0xaa000050 version or higher

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.


Tested-by: William Xie <william.xie@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  | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..16b8715e0984 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_id;
+	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 c4b11e2fbe33..1eb202ec2302 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
 	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);
@@ -248,6 +249,25 @@ static int microcode_sanity_check(void *mc, int print_err)
 		return -EINVAL;
 	}
 
+	/*
+	 * Enforce for late-load that min_req_id is specified in the header.
+	 * Otherwise its an old format microcode, reject it.
+	 */
+	if (print_err) {
+		if (!mc_header->min_req_id) {
+			pr_warn("Header MUST specify min version for late-load\n");
+			return -EINVAL;
+		}
+
+		intel_cpu_collect_info(&uci);
+		if (uci.cpu_sig.rev < mc_header->min_req_id) {
+			pr_warn("Current revision 0x%x is too old to update,"
+				"must  be at 0x%x version or higher\n",
+				uci.cpu_sig.rev, mc_header->min_req_id);
+			return -EINVAL;
+		}
+	}
+
 	if (!ext_table_size)
 		return 0;
 
-- 
2.32.0

[PATCH v2 3/4] x86/microcode: Avoid any chance of MCE's during microcode update

[Ashok Raj]

When a microcode update is in progress, several instructions and MSR's can
be patched by the update. During the update in progress, touching any of
the resources being patched could result in unpredictable results. If
thread0 is doing the update and thread1 happens to get a MCE, the handler
might read an MSR that's being patched.

In order to have predictable behavior, to avoid this scenario we set the MCIP in
all threads. Since MCE's can't be nested, HW will automatically promote to
shutdown condition.

After the update is completed, MCIP flag is cleared. The system is going to
shutdown anyway, since the MCE could be a fatal error, or even recoverable
errors in kernel space are treated as unrecoverable.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/mce.h           |  4 ++++
 arch/x86/kernel/cpu/mce/core.c       |  9 +++++++++
 arch/x86/kernel/cpu/microcode/core.c | 11 +++++++++++
 3 files changed, 24 insertions(+)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cc73061e7255..2aef6120e23f 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -207,12 +207,16 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_clear(struct cpuinfo_x86 *c);
 int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
 			       u64 lapic_id);
+extern void mce_set_mcip(void);
+extern void mce_clear_mcip(void);
 #else
 static inline int mcheck_init(void) { return 0; }
 static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
 static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
 					     u64 lapic_id) { return -EINVAL; }
+static inline void mce_set_mcip(void) {}
+static inline void mce_clear_mcip(void) {}
 #endif
 
 void mce_setup(struct mce *m);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2c8ec5c71712..72b49d95bb3b 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -402,6 +402,15 @@ static noinstr void mce_wrmsrl(u32 msr, u64 v)
 		     : : "c" (msr), "a"(low), "d" (high) : "memory");
 }
 
+void mce_set_mcip(void)
+{
+	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x1);
+}
+
+void mce_clear_mcip(void)
+{
+	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x0);
+}
 /*
  * Collect all global (w.r.t. this processor) status about this machine
  * check into our "mce" struct so that we can use it later to assess
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index ad57e0e4d674..d24e1c754c27 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -39,6 +39,7 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/mce.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -450,6 +451,14 @@ static int __reload_late(void *info)
 	if (__wait_for_cpus(&late_cpus_in, NSEC_PER_SEC))
 		return -1;
 
+	/*
+	 * Its dangerous to let MCE while microcode update is in progress.
+	 * Its extremely rare and even if happens they are fatal errors.
+	 * But reading patched areas before the update is complete can be
+	 * leading to unpredictable results. Setting MCIP will guarantee
+	 * the platform is taken to reset predictively.
+	 */
+	mce_set_mcip();
 	/*
 	 * On an SMT system, it suffices to load the microcode on one sibling of
 	 * the core because the microcode engine is shared between the threads.
@@ -457,6 +466,7 @@ 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)
 		apply_microcode_local(&err);
 	else
@@ -473,6 +483,7 @@ static int __reload_late(void *info)
 	if (__wait_for_cpus(&late_cpus_out, NSEC_PER_SEC))
 		panic("Timeout during microcode update!\n");
 
+	mce_clear_mcip();
 	/*
 	 * At least one thread has completed update on each core.
 	 * For others, simply call the update to make sure the
-- 
2.32.0

[PATCH v2 4/4] x86/microcode: Handle NMI's during microcode update.

[Ashok Raj]

Microcode updates need a guarantee that the thread sibling that is waiting
for the update to finish on the primary core will not execute any
instructions until the update is complete. This is required to guarantee
any MSR or instruction that's being patched will be executed before the
update is complete.

After the stop_machine() rendezvous, an NMI handler is registered. If an
NMI were to happen while the microcode update is not complete, the
secondary thread will spin until the ucode update state is cleared.

Couple of choices discussed are:

1. Rendezvous inside the NMI handler, and also perform the update from
   within the handler. This seemed too risky and might cause instability
   with the races that we would need to solve. This would be a difficult
   choice.
   	1.a Since the primary thread of every core is performing a wrmsr
	for the update, once the wrmsr has started, it can't be
	interrupted. Hence its not required to NMI the primary thread of
	the core. Only the secondary thread needs to be parked in NMI
	before the update begins.
	Suggested by From Andy Cooper
2. Thomas (tglx) suggested that we could look into masking all the LVT
   originating NMI's. Such as LINT1, Perf control LVT entries and such.
   Since we are in the rendezvous loop, we don't need to worry about any
   NMI IPI's generated by the OS.

   The one we didn't have any control over is the ACPI mechanism of sending
   notifications to kernel for Firmware First Processing (FFM). Apparently
   it seems there is a PCH register that BIOS in SMI would write to
   generate such an interrupt (ACPI GHES).
3. This is a simpler option. OS registers an NMI handler and doesn't do any
   NMI rendezvous dance. But if an NMI were to happen, we check if any of
   the CPUs thread siblings have an update in progress. Only those CPUs
   would take an NMI. The thread performing the wrmsr() will only take an
   NMI after the completion of the wrmsr 0x79 flow.

   [ Lutomirsky thinks this is weak, and what happens from taking the
   interrupt and the path to the registered callback handler might be
   exposed.]

   Seems like 1.a is the best candidate.

The algorithm is something like this:

After stop_machine() all threads are executing __reload_late()

nmi_callback()
{
	if (!in_ucode_update)
		return NMI_DONE;
	if (cpu not in sibling_mask)
		return NMI_DONE;
	update sibling reached NMI for primary to continue

	while (cpu in sibling_mask)
		wait;
	return NMI_HANDLED;
}

__reload_late()
{

	entry_rendezvous(&late_cpus_in);
	set_mcip()
	if (this_cpu is first_cpu in the core)
		wait for siblings to drop in NMI
		apply_microcode()
	else
		wait_for_siblings;

wait_for_siblings:

	exit_rendezvous(&late_cpus_out);
	clear_mcip
}

reload_late()
{
	register_nmi_handler()
	prepare_mask of all sibling cpus()
	update state = ucode in progress;
	send NMI to all threads in sibling_mask
	unregister_nmi_handler();
}

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index d24e1c754c27..26ceb4e37a53 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -39,7 +39,9 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/apic.h>
 #include <asm/mce.h>
+#include <asm/nmi.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -411,6 +413,12 @@ static int check_online_cpus(void)
 
 static atomic_t late_cpus_in;
 static atomic_t late_cpus_out;
+static atomic_t nmi_cpus;	// number of CPUs that enter NMI
+static atomic_t nmi_timeouts;   // number of siblings that timeout
+static atomic_t nmi_siblings;   // Nmber of siblings that enter NMI
+static atomic_t in_ucode_update;// Are we in microcode update?
+
+static struct cpumask all_sibling_mask;
 
 static int __wait_for_cpus(atomic_t *t, long long timeout)
 {
@@ -433,6 +441,74 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
 	return 0;
 }
 
+struct core_rendez {
+	int num_core_cpus;
+	atomic_t callin;
+};
+
+static DEFINE_PER_CPU(struct core_rendez, core_sync);
+
+static int ucode_nmi_cb(unsigned int val, struct pt_regs *regs)
+{
+	int first_cpu, cpu = smp_processor_id();
+	int timeout = 100 * NSEC_PER_USEC;
+	struct core_rendez *rendez;
+
+	atomic_inc(&nmi_cpus);
+	if (!atomic_read(&in_ucode_update))
+		return NMI_DONE;
+
+	if (!cpumask_test_cpu(cpu, &all_sibling_mask))
+		return NMI_DONE;
+
+	atomic_inc(&nmi_siblings);
+	pr_debug("CPU %d made into NMI handler\n", cpu);
+	/*
+	 * primary thread waits for this
+	 * before performing update
+	 */
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	rendez = &per_cpu(core_sync, first_cpu);
+
+	atomic_inc(&rendez->callin);
+	while (timeout < NSEC_PER_USEC) {
+		if (timeout < NSEC_PER_USEC) {
+			atomic_inc(&nmi_timeouts);
+			pr_debug("CPU %d sibling timedout\n",cpu);
+			break;
+		}
+		ndelay(NSEC_PER_USEC);
+		timeout -= NSEC_PER_USEC;
+		touch_nmi_watchdog();
+		/*
+		 * Once primary clears it from all_sibling_mask, we are
+		 * released from the NMI loop
+		 */
+		if (!cpumask_test_cpu(cpu, &all_sibling_mask)) {
+			pr_debug("CPU %d breaking from NMI\n",cpu);
+			break;
+		}
+	}
+	return NMI_HANDLED;
+}
+
+/*
+ * Primary thread clears the cpumask to release the siblings from the NMI
+ * jail
+ */
+
+static void clear_nmi_cpus(void)
+{
+	int first_cpu, wait_cpu, cpu = smp_processor_id();
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu)
+			continue;
+		cpumask_clear_cpu(wait_cpu, &all_sibling_mask);
+	}
+}
+
 /*
  * Returns:
  * < 0 - on error
@@ -440,14 +516,14 @@ 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();
 	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;
 
@@ -459,6 +535,7 @@ static int __reload_late(void *info)
 	 * the platform is taken to reset predictively.
 	 */
 	mce_set_mcip();
+
 	/*
 	 * On an SMT system, it suffices to load the microcode on one sibling of
 	 * the core because the microcode engine is shared between the threads.
@@ -466,13 +543,47 @@ static int __reload_late(void *info)
 	 * loading attempts happen on multiple threads of an SMT core. See
 	 * below.
 	 */
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
 
-	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
+	/*
+	 * Set the CPUs that we should hold in NMI until the primary has
+	 * completed the microcode update.
+	 */
+	if (first_cpu == cpu) {
+		struct core_rendez *pcpu_core = &per_cpu(core_sync, cpu);
+		int num_sibs = pcpu_core->num_core_cpus - 1;
+		int timeout = 100 * NSEC_PER_USEC;
+
+		/*
+		 * Wait for all siblings to enter
+		 * NMI before performing the update
+		 */
+		while (atomic_read(&pcpu_core->callin) < num_sibs) {
+			if (timeout < NSEC_PER_USEC) {
+				timeout -= NSEC_PER_USEC;
+				ndelay(NSEC_PER_USEC);
+				touch_nmi_watchdog();
+			} else {
+				/*
+				 * Some unknown reason the siblings didn't
+				 * check in with primary. We just signal
+				 * failure, but better to update since
+				 * other cores might be successful and we
+				 * don't want to leave this alone
+				 */
+				pr_err("CPU %d core lead timeout waiting for"
+				       " siblings\n", cpu);
+				ret = -1;
+				break;
+			}
+		}
+		pr_debug("Primary CPU %d proceeding with update\n", cpu);
 		apply_microcode_local(&err);
-	else
+		clear_nmi_cpus();
+	} else
 		goto wait_for_siblings;
 
-	if (err >= UCODE_NFOUND) {
+	if (ret || err >= UCODE_NFOUND) {
 		if (err == UCODE_ERROR)
 			pr_warn("Error reloading microcode on CPU %d\n", cpu);
 
@@ -496,26 +607,92 @@ static int __reload_late(void *info)
 	return ret;
 }
 
+static void set_nmi_cpus(int cpu)
+{
+	int first_cpu, wait_cpu;
+	struct core_rendez *pcpu_core = &per_cpu(core_sync, cpu);
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu) {
+			pcpu_core->num_core_cpus =
+					cpumask_weight(topology_sibling_cpumask(wait_cpu));
+			continue;
+		}
+		cpumask_set_cpu(wait_cpu, &all_sibling_mask);
+	}
+}
+
+static void prepare_siblings(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, cpu_online_mask) {
+		set_nmi_cpus(cpu);
+	}
+}
+
 /*
  * Reload microcode late on all CPUs. Wait for a sec until they
  * all gather together.
  */
 static int microcode_reload_late(void)
 {
-	int ret;
+	int ret = 0;
 
 	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);
 
+	/*
+	 * in_ucode_update: Global state while in ucode update
+	 * nmi_cpus: Count of CPUs entering NMI while ucode in progress
+	 * nmi_siblings: Count of siblings that enter NMI
+	 * nmi_timeouts: Count of siblings that fail to see mask clear
+	 */
+	atomic_set(&in_ucode_update,0);
+	atomic_set(&nmi_cpus, 0);
+	atomic_set(&nmi_timeouts, 0);
+	atomic_set(&nmi_siblings, 0);
+
+	cpumask_clear(&all_sibling_mask);
+
+	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;
+	}
+
+	/*
+	 * Prepare everything for siblings threads to drop into NMI while
+	 * the update is in progress.
+	 */
+	prepare_siblings();
+	atomic_set(&in_ucode_update, 1);
+	apic->send_IPI_mask(&all_sibling_mask, NMI_VECTOR);
+	pr_debug("Sent NMI broadcast to all sibling cpus\n");
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check();
 
-	pr_info("Reload completed, microcode revision: 0x%x\n", boot_cpu_data.microcode);
+	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
+
+	pr_debug("Total CPUs that entered NMI     ... %d\n",
+			atomic_read(&nmi_cpus));
+	pr_debug("Total siblings that entered NMI ... %d\n",
+		  	atomic_read(&nmi_siblings));
+	pr_debug("Total siblings timedout         ... %d\n",
+			atomic_read(&nmi_timeouts));
+	pr_info("Reload completed, microcode revision: 0x%x\n",
+	        boot_cpu_data.microcode);
 
+done:
 	return ret;
 }
 
-- 
2.32.0

[PATCH v2 4/4] x86/microcode: Place siblings in NMI loop while update in progress

[Ashok Raj]

Microcode updates need a guarantee that the thread sibling that is waiting
for the update to finish on the primary core will not execute any
instructions until the update is complete. This is required to guarantee
any MSR or instruction that's being patched will be executed before the
update is complete.

After the stop_machine() rendezvous, an NMI handler is registered. If an
NMI were to happen while the microcode update is not complete, the
secondary thread will spin until the ucode update state is cleared.

Couple of choices discussed are:

1. Rendezvous inside the NMI handler, and also perform the update from
   within the handler. This seemed too risky and might cause instability
   with the races that we would need to solve. This would be a difficult
   choice.
   	1.a Since the primary thread of every core is performing a wrmsr
	for the update, once the wrmsr has started, it can't be
	interrupted. Hence its not required to NMI the primary thread of
	the core. Only the secondary thread needs to be parked in NMI
	before the update begins.
	Suggested by From Andy Cooper
2. Thomas (tglx) suggested that we could look into masking all the LVT
   originating NMI's. Such as LINT1, Perf control LVT entries and such.
   Since we are in the rendezvous loop, we don't need to worry about any
   NMI IPI's generated by the OS.

   The one we didn't have any control over is the ACPI mechanism of sending
   notifications to kernel for Firmware First Processing (FFM). Apparently
   it seems there is a PCH register that BIOS in SMI would write to
   generate such an interrupt (ACPI GHES).
3. This is a simpler option. OS registers an NMI handler and doesn't do any
   NMI rendezvous dance. But if an NMI were to happen, we check if any of
   the CPUs thread siblings have an update in progress. Only those CPUs
   would take an NMI. The thread performing the wrmsr() will only take an
   NMI after the completion of the wrmsr 0x79 flow.

   [ Lutomirsky thinks this is weak, and what happens from taking the
   interrupt and the path to the registered callback handler might be
   exposed.]

   Seems like 1.a is the best candidate.

The algorithm is something like this:

After stop_machine() all threads are executing __reload_late()

nmi_callback()
{
	if (!in_ucode_update)
		return NMI_DONE;
	if (cpu not in sibling_mask)
		return NMI_DONE;
	update sibling reached NMI for primary to continue

	while (cpu in sibling_mask)
		wait;
	return NMI_HANDLED;
}

__reload_late()
{

	entry_rendezvous(&late_cpus_in);
	set_mcip()
	if (this_cpu is first_cpu in the core)
		wait for siblings to drop in NMI
		apply_microcode()
	else
		wait_for_siblings;

wait_for_siblings:

	exit_rendezvous(&late_cpus_out);
	clear_mcip
}

reload_late()
{
	register_nmi_handler()
	prepare_mask of all sibling cpus()
	update state = ucode in progress;
	send NMI to all threads in sibling_mask
	unregister_nmi_handler();
}

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

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index d24e1c754c27..26ceb4e37a53 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -39,7 +39,9 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/apic.h>
 #include <asm/mce.h>
+#include <asm/nmi.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -411,6 +413,12 @@ static int check_online_cpus(void)
 
 static atomic_t late_cpus_in;
 static atomic_t late_cpus_out;
+static atomic_t nmi_cpus;	// number of CPUs that enter NMI
+static atomic_t nmi_timeouts;   // number of siblings that timeout
+static atomic_t nmi_siblings;   // Nmber of siblings that enter NMI
+static atomic_t in_ucode_update;// Are we in microcode update?
+
+static struct cpumask all_sibling_mask;
 
 static int __wait_for_cpus(atomic_t *t, long long timeout)
 {
@@ -433,6 +441,74 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
 	return 0;
 }
 
+struct core_rendez {
+	int num_core_cpus;
+	atomic_t callin;
+};
+
+static DEFINE_PER_CPU(struct core_rendez, core_sync);
+
+static int ucode_nmi_cb(unsigned int val, struct pt_regs *regs)
+{
+	int first_cpu, cpu = smp_processor_id();
+	int timeout = 100 * NSEC_PER_USEC;
+	struct core_rendez *rendez;
+
+	atomic_inc(&nmi_cpus);
+	if (!atomic_read(&in_ucode_update))
+		return NMI_DONE;
+
+	if (!cpumask_test_cpu(cpu, &all_sibling_mask))
+		return NMI_DONE;
+
+	atomic_inc(&nmi_siblings);
+	pr_debug("CPU %d made into NMI handler\n", cpu);
+	/*
+	 * primary thread waits for this
+	 * before performing update
+	 */
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	rendez = &per_cpu(core_sync, first_cpu);
+
+	atomic_inc(&rendez->callin);
+	while (timeout < NSEC_PER_USEC) {
+		if (timeout < NSEC_PER_USEC) {
+			atomic_inc(&nmi_timeouts);
+			pr_debug("CPU %d sibling timedout\n",cpu);
+			break;
+		}
+		ndelay(NSEC_PER_USEC);
+		timeout -= NSEC_PER_USEC;
+		touch_nmi_watchdog();
+		/*
+		 * Once primary clears it from all_sibling_mask, we are
+		 * released from the NMI loop
+		 */
+		if (!cpumask_test_cpu(cpu, &all_sibling_mask)) {
+			pr_debug("CPU %d breaking from NMI\n",cpu);
+			break;
+		}
+	}
+	return NMI_HANDLED;
+}
+
+/*
+ * Primary thread clears the cpumask to release the siblings from the NMI
+ * jail
+ */
+
+static void clear_nmi_cpus(void)
+{
+	int first_cpu, wait_cpu, cpu = smp_processor_id();
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu)
+			continue;
+		cpumask_clear_cpu(wait_cpu, &all_sibling_mask);
+	}
+}
+
 /*
  * Returns:
  * < 0 - on error
@@ -440,14 +516,14 @@ 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();
 	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;
 
@@ -459,6 +535,7 @@ static int __reload_late(void *info)
 	 * the platform is taken to reset predictively.
 	 */
 	mce_set_mcip();
+
 	/*
 	 * On an SMT system, it suffices to load the microcode on one sibling of
 	 * the core because the microcode engine is shared between the threads.
@@ -466,13 +543,47 @@ static int __reload_late(void *info)
 	 * loading attempts happen on multiple threads of an SMT core. See
 	 * below.
 	 */
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
 
-	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
+	/*
+	 * Set the CPUs that we should hold in NMI until the primary has
+	 * completed the microcode update.
+	 */
+	if (first_cpu == cpu) {
+		struct core_rendez *pcpu_core = &per_cpu(core_sync, cpu);
+		int num_sibs = pcpu_core->num_core_cpus - 1;
+		int timeout = 100 * NSEC_PER_USEC;
+
+		/*
+		 * Wait for all siblings to enter
+		 * NMI before performing the update
+		 */
+		while (atomic_read(&pcpu_core->callin) < num_sibs) {
+			if (timeout < NSEC_PER_USEC) {
+				timeout -= NSEC_PER_USEC;
+				ndelay(NSEC_PER_USEC);
+				touch_nmi_watchdog();
+			} else {
+				/*
+				 * Some unknown reason the siblings didn't
+				 * check in with primary. We just signal
+				 * failure, but better to update since
+				 * other cores might be successful and we
+				 * don't want to leave this alone
+				 */
+				pr_err("CPU %d core lead timeout waiting for"
+				       " siblings\n", cpu);
+				ret = -1;
+				break;
+			}
+		}
+		pr_debug("Primary CPU %d proceeding with update\n", cpu);
 		apply_microcode_local(&err);
-	else
+		clear_nmi_cpus();
+	} else
 		goto wait_for_siblings;
 
-	if (err >= UCODE_NFOUND) {
+	if (ret || err >= UCODE_NFOUND) {
 		if (err == UCODE_ERROR)
 			pr_warn("Error reloading microcode on CPU %d\n", cpu);
 
@@ -496,26 +607,92 @@ static int __reload_late(void *info)
 	return ret;
 }
 
+static void set_nmi_cpus(int cpu)
+{
+	int first_cpu, wait_cpu;
+	struct core_rendez *pcpu_core = &per_cpu(core_sync, cpu);
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu) {
+			pcpu_core->num_core_cpus =
+					cpumask_weight(topology_sibling_cpumask(wait_cpu));
+			continue;
+		}
+		cpumask_set_cpu(wait_cpu, &all_sibling_mask);
+	}
+}
+
+static void prepare_siblings(void)
+{
+	int cpu;
+
+	for_each_cpu(cpu, cpu_online_mask) {
+		set_nmi_cpus(cpu);
+	}
+}
+
 /*
  * Reload microcode late on all CPUs. Wait for a sec until they
  * all gather together.
  */
 static int microcode_reload_late(void)
 {
-	int ret;
+	int ret = 0;
 
 	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);
 
+	/*
+	 * in_ucode_update: Global state while in ucode update
+	 * nmi_cpus: Count of CPUs entering NMI while ucode in progress
+	 * nmi_siblings: Count of siblings that enter NMI
+	 * nmi_timeouts: Count of siblings that fail to see mask clear
+	 */
+	atomic_set(&in_ucode_update,0);
+	atomic_set(&nmi_cpus, 0);
+	atomic_set(&nmi_timeouts, 0);
+	atomic_set(&nmi_siblings, 0);
+
+	cpumask_clear(&all_sibling_mask);
+
+	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;
+	}
+
+	/*
+	 * Prepare everything for siblings threads to drop into NMI while
+	 * the update is in progress.
+	 */
+	prepare_siblings();
+	atomic_set(&in_ucode_update, 1);
+	apic->send_IPI_mask(&all_sibling_mask, NMI_VECTOR);
+	pr_debug("Sent NMI broadcast to all sibling cpus\n");
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check();
 
-	pr_info("Reload completed, microcode revision: 0x%x\n", boot_cpu_data.microcode);
+	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
+
+	pr_debug("Total CPUs that entered NMI     ... %d\n",
+			atomic_read(&nmi_cpus));
+	pr_debug("Total siblings that entered NMI ... %d\n",
+		  	atomic_read(&nmi_siblings));
+	pr_debug("Total siblings timedout         ... %d\n",
+			atomic_read(&nmi_timeouts));
+	pr_info("Reload completed, microcode revision: 0x%x\n",
+	        boot_cpu_data.microcode);
 
+done:
 	return ret;
 }
 
-- 
2.32.0

Re: [PATCH v2 4/4] x86/microcode: Place siblings in NMI loop while update in progress

[Dave Hansen]

On 8/15/22 21:37, Ashok Raj wrote:
> Microcode updates need a guarantee that the thread sibling that is waiting
> for the update to finish on the primary core will not execute any
> instructions until the update is complete. This is required to guarantee
> any MSR or instruction that's being patched will be executed before the
> update is complete.

I don't think this is a great way to describe the problem.  Even in this
patch, the thread sibling is doing something far from "not executing any
instructions".

	Microcode updates affect the state of the running CPU.  In the
	case of hyperthreads, the thread initiating the update is in a
	known state (WRMSR), but its hyperthreads can be running around
	executing arbitrary instructions.

	If one of these arbitrary instructions is being patched by the
	update, <insert symptoms here>.  The existing code uses
	stop_machine() to keep userspace from running and puts the
	kernel in a relatively safe wait loop.  But, that loop can be
	still be interrupted by NMIs.

	The NMI code and NMI handlers can also execute relatively
	arbitrary instructions.

	== Solution ==

	Park sibling CPU threads in a newly-registered NMI handler while
	the primary CPU thread does the microcode update.

	This ensures that...  It removes the possibility for...


> After the stop_machine() rendezvous, an NMI handler is registered. If an
> NMI were to happen while the microcode update is not complete, the
> secondary thread will spin until the ucode update state is cleared.

This is a case where switching it imperative voice would make it a lot
more clear.

Also, maybe I'm blind or misreading this, but the code _looks_ to do
this, in this order:

> +	ret = register_nmi_handler(NMI_LOCAL, ucode_nmi_cb, NMI_FLAG_FIRST,
> +				   "ucode_nmi");
...
>  	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);

It registers the NMI handler _before_ the stop_machine().

The description so far is (maybe) telling us what the code does.  But,
it doesn't say what the end result is.  *How* does this address the problem?

I _think_ all this ends up doing in the end is ensuring that arbitrary
NMI handlers don't run since it uses NMI_FLAG_FIRST.  I'm not completely
sure how it guarantees that it *is* first.

Also, for this to be bulletproof, what would have to be true?  I *think*
the entire NMI entry path and _this_ (new) handler has to be free of
instructions that could go bonkers during a microcode update.  How do we
know what those instructions *are*, btw?  Have you audited the NMI entry
path to make sure it is free of those instructions?  Is everything still
safe even with CONFIG_DEBUG_ENTRY in place where NMIs can be nested?

This is also the place in the changelog where you can discuss the
attributes of this solution.  It's all C code, for instance.  That is
both good (easy to code up and review) but, bad because the C compiler
might be doing fancy things that go amok during a ucode update.

For instance, you can discuss why this can safely ignore the primary
thread instead of calling it out as one of the "couple of choices".

> Couple of choices discussed are:
> 
> 1. Rendezvous inside the NMI handler, and also perform the update from
>    within the handler. This seemed too risky and might cause instability
>    with the races that we would need to solve. This would be a difficult
>    choice.
>    	1.a Since the primary thread of every core is performing a wrmsr
> 	for the update, once the wrmsr has started, it can't be
> 	interrupted. Hence its not required to NMI the primary thread of
> 	the core. Only the secondary thread needs to be parked in NMI
> 	before the update begins.
> 	Suggested by From Andy Cooper

What are the races that need to be solved?  What would make it unstable?

> 2. Thomas (tglx) suggested that we could look into masking all the LVT
>    originating NMI's. Such as LINT1, Perf control LVT entries and such.
>    Since we are in the rendezvous loop, we don't need to worry about any
>    NMI IPI's generated by the OS.
> 
>    The one we didn't have any control over is the ACPI mechanism of sending
>    notifications to kernel for Firmware First Processing (FFM). Apparently
>    it seems there is a PCH register that BIOS in SMI would write to
>    generate such an interrupt (ACPI GHES).
> 3. This is a simpler option. OS registers an NMI handler and doesn't do any
>    NMI rendezvous dance. But if an NMI were to happen, we check if any of
>    the CPUs thread siblings have an update in progress. Only those CPUs
>    would take an NMI. The thread performing the wrmsr() will only take an
>    NMI after the completion of the wrmsr 0x79 flow.
> 
>    [ Lutomirsky thinks this is weak, and what happens from taking the
>    interrupt and the path to the registered callback handler might be
>    exposed.]
> 
>    Seems like 1.a is the best candidate.
> 
> The algorithm is something like this:
> 
> After stop_machine() all threads are executing __reload_late()
> 
> nmi_callback()
> {
> 	if (!in_ucode_update)
> 		return NMI_DONE;

I'm not sure 'in_ucode_update' is even needed.  It seems redundant with
the sibling_mask, especially since this callback is not even registered
until after 'in_ucode_update=1'.

>  static atomic_t late_cpus_in;
>  static atomic_t late_cpus_out;
> +static atomic_t nmi_cpus;	// number of CPUs that enter NMI
> +static atomic_t nmi_timeouts;   // number of siblings that timeout
> +static atomic_t nmi_siblings;   // Nmber of siblings that enter NMI
> +static atomic_t in_ucode_update;// Are we in microcode update?
> +
> +static struct cpumask all_sibling_mask;

I haven't gone through this in detail.  But, this is a *LOT* of state
and my gut feeling is that there is some major simplification that can
be done here.  I don't doubt that this works, but I do doubt that this
is as simple and straightforward as we can make it.

I don't get, for instance, why you care about rendez->callin.  Why does
it matter if a CPU is in the NMI handler loop versus being in the
stop_machine() handler?  With nested NMIs, you need to handle being
anywhere in the NMI entry code, and you can end up in there from either
the stop_machine() hander *OR* the new NMI handler.