[PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Ashok Raj]

During microcode development, its often required to test different versions
of microcode. Intel microcode loader enforces loading only if the revision is
greater than what is currently loaded on the cpu. Overriding this behavior
allows us to reuse the same revision during development cycles.
This facilty also allows us to share debug microcode with development
partners for getting feedback before microcode release.

Microcode developers should have other ways to check which
of their internal version is actually loaded. For e.g. checking a
temporary MSR for instance. In order to reload the same microcode do as
shown below.

 # echo 2 > /sys/devices/system/cpu/microcode/reload

 as root.


I tested this on top of the parallel ucode load patch

https://lore.kernel.org/r/1566506627-16536-2-git-send-email-mihai.carabas@oracle.com/

v2: [Mihai] Address comments from Boris
	- Support for AMD
	- add taint flag
	- removed global force_ucode_load and parameterized it.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Mihai Carabas <mihai.carabas@oracle.com>
cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Mihai Carabas <mihai.carabas@oracle.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Jon Grimm <Jon.Grimm@amd.com>
Cc: kanth.ghatraju@oracle.com
Cc: konrad.wilk@oracle.com
Cc: patrick.colp@oracle.com
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: x86-ml <x86@kernel.org>
Cc: linux-kernel@vger.kernel.org
---
 Documentation/x86/microcode.rst       |  7 ++++
 arch/x86/include/asm/microcode.h      |  4 +--
 arch/x86/kernel/cpu/microcode/amd.c   | 19 ++++++-----
 arch/x86/kernel/cpu/microcode/core.c  | 31 ++++++++++++------
 arch/x86/kernel/cpu/microcode/intel.c | 61 ++++++++++++++++++++++++++---------
 5 files changed, 88 insertions(+), 34 deletions(-)

diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37..ad804eb 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -110,6 +110,13 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+The microcode will not be updated if the existing revision is newer or the
+same. In order to force a reload you can run the following command::
+
+  # echo 2 > /sys/devices/system/cpu/microcode/reload
+
+as root.
+
 Builtin microcode
 =================
 
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 2b7cc53..f5bc849 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -36,7 +36,7 @@ struct microcode_ops {
 				const void __user *buf, size_t size);
 
 	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
-						  bool refresh_fw);
+						  bool refresh_fw, bool force_ucode_load);
 
 	void (*microcode_fini_cpu) (int cpu);
 
@@ -46,7 +46,7 @@ struct microcode_ops {
 	 * are being called.
 	 * See also the "Synchronization" section in microcode_core.c.
 	 */
-	enum ucode_state (*apply_microcode) (int cpu);
+	enum ucode_state (*apply_microcode) (int cpu, bool force_ucode_load);
 	int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
 };
 
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 4ddadf6..1d6e4e2 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -539,7 +539,8 @@ void load_ucode_amd_ap(unsigned int cpuid_1_eax)
 }
 
 static enum ucode_state
-load_microcode_amd(bool save, u8 family, const u8 *data, size_t size);
+load_microcode_amd(bool save, u8 family, const u8 *data, size_t size,
+		   bool force_ucode_load);
 
 int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 {
@@ -557,7 +558,7 @@ int __init save_microcode_in_initrd_amd(unsigned int cpuid_1_eax)
 	if (!desc.mc)
 		return -EINVAL;
 
-	ret = load_microcode_amd(true, x86_family(cpuid_1_eax), desc.data, desc.size);
+	ret = load_microcode_amd(true, x86_family(cpuid_1_eax), desc.data, desc.size, false);
 	if (ret > UCODE_UPDATED)
 		return -EINVAL;
 
@@ -666,7 +667,7 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	return 0;
 }
 
-static enum ucode_state apply_microcode_amd(int cpu)
+static enum ucode_state apply_microcode_amd(int cpu, bool force_ucode_load)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct microcode_amd *mc_amd;
@@ -689,7 +690,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 && !force_ucode_load) {
 		ret = UCODE_OK;
 		goto out;
 	}
@@ -835,7 +836,8 @@ static enum ucode_state __load_microcode_amd(u8 family, const u8 *data,
 }
 
 static enum ucode_state
-load_microcode_amd(bool save, u8 family, const u8 *data, size_t size)
+load_microcode_amd(bool save, u8 family, const u8 *data, size_t size,
+		   bool force_ucode_load)
 {
 	struct ucode_patch *p;
 	enum ucode_state ret;
@@ -853,7 +855,7 @@ load_microcode_amd(bool save, u8 family, const u8 *data, size_t size)
 	if (!p) {
 		return ret;
 	} else {
-		if (boot_cpu_data.microcode >= p->patch_id)
+		if (boot_cpu_data.microcode >= p->patch_id && !force_ucode_load)
 			return ret;
 
 		ret = UCODE_NEW;
@@ -886,7 +888,8 @@ 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 refresh_fw,
+					      bool force_ucode_load)
 {
 	char fw_name[36] = "amd-ucode/microcode_amd.bin";
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -910,7 +913,7 @@ static enum ucode_state request_microcode_amd(int cpu, struct device *device,
 	if (!verify_container(fw->data, fw->size, false))
 		goto fw_release;
 
-	ret = load_microcode_amd(bsp, c->x86, fw->data, fw->size);
+	ret = load_microcode_amd(bsp, c->x86, fw->data, fw->size, force_ucode_load);
 
  fw_release:
 	release_firmware(fw);
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7019d4b..c4181b7 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -376,7 +376,7 @@ static void apply_microcode_local(void *arg)
 {
 	enum ucode_state *err = arg;
 
-	*err = microcode_ops->apply_microcode(smp_processor_id());
+	*err = microcode_ops->apply_microcode(smp_processor_id(), false);
 }
 
 static int apply_microcode_on_target(int cpu)
@@ -550,6 +550,7 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
  */
 static int __reload_late(void *info)
 {
+	bool force_ucode_load = (bool) info;
 	int cpu = smp_processor_id();
 	enum ucode_state err;
 	int ret = 0;
@@ -569,7 +570,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(smp_processor_id(), force_ucode_load);
 	else
 		goto wait_for_siblings;
 
@@ -600,14 +601,14 @@ static int __reload_late(void *info)
  * Reload microcode late on all CPUs. Wait for a sec until they
  * all gather together.
  */
-static int microcode_reload_late(void)
+static int microcode_reload_late(bool force_ucode_load)
 {
 	int ret;
 
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
-	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
+	ret = stop_machine_cpuslocked(__reload_late, (void *)force_ucode_load, cpu_online_mask);
 	if (ret > 0)
 		microcode_check();
 
@@ -622,6 +623,7 @@ static ssize_t reload_store(struct device *dev,
 {
 	enum ucode_state tmp_ret = UCODE_OK;
 	int bsp = boot_cpu_data.cpu_index;
+	bool force_ucode_load = false;
 	unsigned long val;
 	ssize_t ret = 0;
 
@@ -629,10 +631,21 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (val != 1)
+	if (!val || val > 2)
 		return size;
 
-	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
+	/*
+	 * Check if the value is 2 to permit reloading microcode even if the revision
+	 * is unchanged. This is typically used during development of microcode and
+	 * changing rev is a pain. Also this is why we also added here a
+	 * TAINT_CPU_OUT_OF_SPEC taint.
+	 */
+	if (val == 2) {
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		force_ucode_load = true;
+	}
+
+	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true, force_ucode_load);
 	if (tmp_ret != UCODE_NEW)
 		return size;
 
@@ -643,7 +656,7 @@ static ssize_t reload_store(struct device *dev,
 		goto put;
 
 	mutex_lock(&microcode_mutex);
-	ret = microcode_reload_late();
+	ret = microcode_reload_late(force_ucode_load);
 	mutex_unlock(&microcode_mutex);
 
 put:
@@ -717,7 +730,7 @@ 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, refresh_fw, false);
 	if (ustate == UCODE_NEW) {
 		pr_debug("CPU%d updated upon init\n", cpu);
 		apply_microcode_on_target(cpu);
@@ -786,7 +799,7 @@ static void mc_bp_resume(void)
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 	if (uci->valid && uci->mc)
-		microcode_ops->apply_microcode(cpu);
+		microcode_ops->apply_microcode(cpu, false);
 	else if (!uci->mc)
 		reload_early_microcode();
 }
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6a99535..631b5a4 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -90,11 +90,13 @@ static int find_matching_signature(void *mc, unsigned int csig, int cpf)
 /*
  * Returns 1 if update has been found, 0 otherwise.
  */
-static int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev)
+static int has_newer_microcode(void *mc, unsigned int csig, int cpf, int new_rev,
+			    bool force_ucode_load)
 {
 	struct microcode_header_intel *mc_hdr = mc;
 
-	if (mc_hdr->rev <= new_rev)
+	if ((mc_hdr->rev < new_rev) || (mc_hdr->rev == new_rev &&
+					!force_ucode_load))
 		return 0;
 
 	return find_matching_signature(mc, csig, cpf);
@@ -359,7 +361,8 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 			if (!has_newer_microcode(data,
 						 uci->cpu_sig.sig,
 						 uci->cpu_sig.pf,
-						 uci->cpu_sig.rev))
+						 uci->cpu_sig.rev,
+						 false))
 				goto next;
 
 		} else {
@@ -368,7 +371,8 @@ scan_microcode(void *data, size_t size, struct ucode_cpu_info *uci, bool save)
 			if (!has_newer_microcode(data,
 						 phdr->sig,
 						 phdr->pf,
-						 phdr->rev))
+						 phdr->rev,
+						 false))
 				goto next;
 		}
 
@@ -721,7 +725,7 @@ void load_ucode_intel_ap(void)
 	}
 }
 
-static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
+static struct microcode_intel *find_patch(struct ucode_cpu_info *uci, bool force_ucode_load)
 {
 	struct microcode_header_intel *phdr;
 	struct ucode_patch *iter, *tmp;
@@ -730,7 +734,8 @@ static struct microcode_intel *find_patch(struct ucode_cpu_info *uci)
 
 		phdr = (struct microcode_header_intel *)iter->data;
 
-		if (phdr->rev <= uci->cpu_sig.rev)
+		if ((phdr->rev < uci->cpu_sig.rev) ||
+			(phdr->rev == uci->cpu_sig.rev && !force_ucode_load))
 			continue;
 
 		if (!find_matching_signature(phdr,
@@ -750,7 +755,7 @@ void reload_ucode_intel(void)
 
 	collect_cpu_info_early(&uci);
 
-	p = find_patch(&uci);
+	p = find_patch(&uci, false);
 	if (!p)
 		return;
 
@@ -787,7 +792,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 	return 0;
 }
 
-static enum ucode_state apply_microcode_intel(int cpu)
+static enum ucode_state apply_microcode_intel(int cpu, bool force_ucode_load)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -802,7 +807,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
 		return UCODE_ERROR;
 
 	/* Look for a newer patch in our cache: */
-	mc = find_patch(uci);
+	mc = find_patch(uci, force_ucode_load);
 	if (!mc) {
 		mc = uci->mc;
 		if (!mc)
@@ -815,11 +820,35 @@ 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) {
 		ret = UCODE_OK;
 		goto out;
 	}
 
+	if (rev == mc->hdr.rev) {
+		/*
+		 * If this isn't the first cpu in the core, just skip
+		 * the actual update since all thread siblings share
+		 * the same microcode resource. We simply update the per-cpu
+		 * revision value that is used in display via /proc/cpuinfo.
+		 */
+		if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu) {
+			ret = UCODE_OK;
+			goto out;
+		} else {
+			/*
+			 * If this is the first cpu in the core, but user
+			 * wanted to force-load the microcode again, then
+			 * go through the real update on the first cpu in every
+			 * core
+			 */
+			if (!force_ucode_load) {
+				ret = UCODE_OK;
+				goto out;
+			}
+		}
+	}
+
 	/*
 	 * Writeback and invalidate caches before updating microcode to avoid
 	 * internal issues depending on what the microcode is updating.
@@ -859,7 +888,8 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	return ret;
 }
 
-static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
+static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter,
+					    bool force_ucode_load)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	unsigned int curr_mc_size = 0, new_mc_size = 0;
@@ -907,7 +937,7 @@ static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 
 		csig = uci->cpu_sig.sig;
 		cpf = uci->cpu_sig.pf;
-		if (has_newer_microcode(mc, csig, cpf, new_rev)) {
+		if (has_newer_microcode(mc, csig, cpf, new_rev, force_ucode_load)) {
 			vfree(new_mc);
 			new_rev = mc_header.rev;
 			new_mc  = mc;
@@ -967,7 +997,8 @@ static bool is_blacklisted(unsigned int cpu)
 }
 
 static enum ucode_state request_microcode_fw(int cpu, struct device *device,
-					     bool refresh_fw)
+					     bool refresh_fw,
+					     bool force_ucode_load)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	const struct firmware *firmware;
@@ -990,7 +1021,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device,
 	kvec.iov_base = (void *)firmware->data;
 	kvec.iov_len = firmware->size;
 	iov_iter_kvec(&iter, WRITE, &kvec, 1, firmware->size);
-	ret = generic_load_microcode(cpu, &iter);
+	ret = generic_load_microcode(cpu, &iter, force_ucode_load);
 
 	release_firmware(firmware);
 
@@ -1010,7 +1041,7 @@ request_microcode_user(int cpu, const void __user *buf, size_t size)
 	iov.iov_len = size;
 	iov_iter_init(&iter, WRITE, &iov, 1, size);
 
-	return generic_load_microcode(cpu, &iter);
+	return generic_load_microcode(cpu, &iter, false);
 }
 
 static struct microcode_ops microcode_intel_ops = {
-- 
2.7.4

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

Hi Boris,

sorry i added the wrong message id in the commit log. 

https://lore.kernel.org/r/20190824085300.GB16813@zn.tnic/

On Wed, Aug 28, 2019 at 10:33:22PM -0700, Ashok Raj wrote:
> During microcode development, its often required to test different versions
> of microcode. Intel microcode loader enforces loading only if the revision is
> greater than what is currently loaded on the cpu. Overriding this behavior
> allows us to reuse the same revision during development cycles.
> This facilty also allows us to share debug microcode with development
> partners for getting feedback before microcode release.
> 
> Microcode developers should have other ways to check which
> of their internal version is actually loaded. For e.g. checking a
> temporary MSR for instance. In order to reload the same microcode do as
> shown below.
> 
>  # echo 2 > /sys/devices/system/cpu/microcode/reload
> 
>  as root.
> 
> 
> I tested this on top of the parallel ucode load patch
> 
> https://lore.kernel.org/r/1566506627-16536-2-git-send-email-mihai.carabas@oracle.com/

Please update with the following: 

https://lore.kernel.org/r/20190824085300.GB16813@zn.tnic/

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Wed, Aug 28, 2019 at 10:33:22PM -0700, Ashok Raj wrote:
> During microcode development, its often required to test different versions
> of microcode. Intel microcode loader enforces loading only if the revision is
> greater than what is currently loaded on the cpu. Overriding this behavior
> allows us to reuse the same revision during development cycles.
> This facilty also allows us to share debug microcode with development
> partners for getting feedback before microcode release.
> 
> Microcode developers should have other ways to check which
> of their internal version is actually loaded. For e.g. checking a
> temporary MSR for instance. In order to reload the same microcode do as
> shown below.
> 
>  # echo 2 > /sys/devices/system/cpu/microcode/reload
> 
>  as root.
> 
> 
> I tested this on top of the parallel ucode load patch
> 
> https://lore.kernel.org/r/1566506627-16536-2-git-send-email-mihai.carabas@oracle.com/
> 
> v2: [Mihai] Address comments from Boris
> 	- Support for AMD
> 	- add taint flag
> 	- removed global force_ucode_load and parameterized it.

As I've said before, I don't like the churn in this version and how it
turns out. I'll have a look at how to do this cleanly when I get some
free cycles.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

On Thu, Aug 29, 2019 at 08:09:42AM +0200, Borislav Petkov wrote:
> On Wed, Aug 28, 2019 at 10:33:22PM -0700, Ashok Raj wrote:
> > During microcode development, its often required to test different versions
> > of microcode. Intel microcode loader enforces loading only if the revision is
> > greater than what is currently loaded on the cpu. Overriding this behavior
> > allows us to reuse the same revision during development cycles.
> > This facilty also allows us to share debug microcode with development
> > partners for getting feedback before microcode release.
> > 
> > Microcode developers should have other ways to check which
> > of their internal version is actually loaded. For e.g. checking a
> > temporary MSR for instance. In order to reload the same microcode do as
> > shown below.
> > 
> >  # echo 2 > /sys/devices/system/cpu/microcode/reload
> > 
> >  as root.
> > 
> > 
> > I tested this on top of the parallel ucode load patch
> > 
> > https://lore.kernel.org/r/1566506627-16536-2-git-send-email-mihai.carabas@oracle.com/
> > 
> > v2: [Mihai] Address comments from Boris
> > 	- Support for AMD
> > 	- add taint flag
> > 	- removed global force_ucode_load and parameterized it.
> 
> As I've said before, I don't like the churn in this version and how it
> turns out. I'll have a look at how to do this cleanly when I get some
> free cycles.

Thanks Boris. I'll wait for your updates. I remember your comment on another
simplification from the Boris Ostrovsky https://lore.kernel.org/r/20190828191618.GO4920@zn.tnic/

Mihai rolled in your suggestions noted above.

BTW, We only need to force on the late-load. Its not needed during early load.

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Thu, Aug 29, 2019 at 06:02:14AM -0700, Raj, Ashok wrote:
> > As I've said before, I don't like the churn in this version and how it
> > turns out. I'll have a look at how to do this cleanly when I get some
> > free cycles.

Ok, here's an untested rough version. It is doing a couple of things in
one go, including cleanups, which I'll eventually separate out but it
should suffice for now to show the intent: namely, reload the microcode
revision which late loading has loaded already in the past. See, pls, if
that suffices for your microcoders' needs.

Thx.

---
diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37982ed..4b2b9c350cea 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -110,6 +110,17 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+Normally, microcode will be updated only if the current revision's
+number is smaller than the new one.
+
+Reload of the current revision can be done by doing::
+
+  # echo 2 > /sys/devices/system/cpu/microcode/reload
+
+as root. This is meant to have a quicker turnaround when testing
+microcode patches and will taint the kernel so do not use it if you
+don't know what you're doing.
+
 Builtin microcode
 =================
 
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index 2b7cc5397f80..3e53ce875e55 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -35,19 +35,24 @@ struct microcode_ops {
 	enum ucode_state (*request_microcode_user) (int cpu,
 				const void __user *buf, size_t size);
 
-	enum ucode_state (*request_microcode_fw) (int cpu, struct device *,
+	enum ucode_state (*request_microcode_fw)(int cpu, struct device *dev,
 						  bool refresh_fw);
 
-	void (*microcode_fini_cpu) (int cpu);
+	void (*microcode_fini_cpu)(int cpu);
 
 	/*
 	 * The generic 'microcode_core' part guarantees that
-	 * the callbacks below run on a target cpu when they
+	 * the callbacks below run on a target CPU when they
 	 * are being called.
-	 * See also the "Synchronization" section in microcode_core.c.
+	 * See also the "Synchronization" section in microcode/core.c.
 	 */
-	enum ucode_state (*apply_microcode) (int cpu);
+	enum ucode_state (*apply_microcode)(unsigned int cpu);
 	int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
+
+	/*
+	 * Attempt to reapply microcode without any checking whatsoever.
+	 */
+	enum ucode_state (*apply_microcode_nocheck)(unsigned int cpu);
 };
 
 struct ucode_cpu_info {
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index a0e52bd00ecc..e9e2bd9e5d4d 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -666,7 +666,7 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
 	return 0;
 }
 
-static enum ucode_state apply_microcode_amd(int cpu)
+static enum ucode_state apply_microcode_amd(unsigned int cpu)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct microcode_amd *mc_amd;
@@ -675,7 +675,8 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	enum ucode_state ret;
 	u32 rev, dummy;
 
-	BUG_ON(raw_smp_processor_id() != cpu);
+	if (WARN_ON(raw_smp_processor_id() != cpu))
+		return UCODE_ERROR;
 
 	uci = ucode_cpu_info + cpu;
 
@@ -716,6 +717,28 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	return ret;
 }
 
+static enum ucode_state apply_microcode_nocheck_amd(unsigned int cpu)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct microcode_amd *mc;
+
+	if (WARN_ON(raw_smp_processor_id() != cpu))
+		return UCODE_ERROR;
+
+	if (!uci->mc)
+		return UCODE_NFOUND;
+
+	mc = uci->mc;
+
+	if (__apply_microcode_amd(mc)) {
+		pr_err("CPU%d: update failed for patch_level=0x%08x\n",
+			cpu, mc->hdr.patch_id);
+		return UCODE_ERROR;
+	}
+
+	return UCODE_OK;
+}
+
 static size_t install_equiv_cpu_table(const u8 *buf, size_t buf_size)
 {
 	u32 equiv_tbl_len;
@@ -933,11 +956,12 @@ static void microcode_fini_cpu_amd(int cpu)
 }
 
 static struct microcode_ops microcode_amd_ops = {
-	.request_microcode_user           = request_microcode_user,
-	.request_microcode_fw             = request_microcode_amd,
-	.collect_cpu_info                 = collect_cpu_info_amd,
-	.apply_microcode                  = apply_microcode_amd,
-	.microcode_fini_cpu               = microcode_fini_cpu_amd,
+	.request_microcode_user		= request_microcode_user,
+	.request_microcode_fw		= request_microcode_amd,
+	.collect_cpu_info		= collect_cpu_info_amd,
+	.apply_microcode		= apply_microcode_amd,
+	.apply_microcode_nocheck	= apply_microcode_nocheck_amd,
+	.microcode_fini_cpu		= microcode_fini_cpu_amd,
 };
 
 struct microcode_ops * __init init_amd_microcode(void)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index 7019d4b2df0c..345deaf55119 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -42,6 +42,8 @@
 
 #define DRIVER_VERSION	"2.2"
 
+typedef void (*apply_fn_t)(void *arg);
+
 static struct microcode_ops	*microcode_ops;
 static bool dis_ucode_ldr = true;
 
@@ -379,6 +381,13 @@ static void apply_microcode_local(void *arg)
 	*err = microcode_ops->apply_microcode(smp_processor_id());
 }
 
+static void apply_microcode_nocheck(void *arg)
+{
+	enum ucode_state *err = arg;
+
+	*err = microcode_ops->apply_microcode_nocheck(smp_processor_id());
+}
+
 static int apply_microcode_on_target(int cpu)
 {
 	enum ucode_state err;
@@ -550,6 +559,7 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
  */
 static int __reload_late(void *info)
 {
+	apply_fn_t ap_func = (apply_fn_t)(info);
 	int cpu = smp_processor_id();
 	enum ucode_state err;
 	int ret = 0;
@@ -569,7 +579,7 @@ static int __reload_late(void *info)
 	 * below.
 	 */
 	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
-		apply_microcode_local(&err);
+		ap_func(&err);
 	else
 		goto wait_for_siblings;
 
@@ -591,7 +601,7 @@ static int __reload_late(void *info)
 	 * revision.
 	 */
 	if (cpumask_first(topology_sibling_cpumask(cpu)) != cpu)
-		apply_microcode_local(&err);
+		ap_func(&err);
 
 	return ret;
 }
@@ -600,14 +610,14 @@ static int __reload_late(void *info)
  * Reload microcode late on all CPUs. Wait for a sec until they
  * all gather together.
  */
-static int microcode_reload_late(void)
+static int microcode_reload_late(apply_fn_t apply_func)
 {
 	int ret;
 
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
 
-	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
+	ret = stop_machine_cpuslocked(__reload_late, apply_func, cpu_online_mask);
 	if (ret > 0)
 		microcode_check();
 
@@ -629,8 +639,12 @@ static ssize_t reload_store(struct device *dev,
 	if (ret)
 		return ret;
 
-	if (val != 1)
+	if (val == 2) {
+		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);
+		return microcode_reload_late(apply_microcode_nocheck);
+	} else if (val != 1) {
 		return size;
+	}
 
 	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);
 	if (tmp_ret != UCODE_NEW)
@@ -643,7 +657,7 @@ static ssize_t reload_store(struct device *dev,
 		goto put;
 
 	mutex_lock(&microcode_mutex);
-	ret = microcode_reload_late();
+	ret = microcode_reload_late(apply_microcode_local);
 	mutex_unlock(&microcode_mutex);
 
 put:
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6a99535d7f37..4cc438833f3f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -787,7 +787,7 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
 	return 0;
 }
 
-static enum ucode_state apply_microcode_intel(int cpu)
+static enum ucode_state apply_microcode_intel(unsigned int cpu)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
@@ -859,6 +859,31 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	return ret;
 }
 
+static enum ucode_state apply_microcode_nocheck_intel(unsigned int cpu)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct microcode_intel *mc;
+
+	if (WARN_ON(raw_smp_processor_id() != cpu))
+		return UCODE_ERROR;
+
+	if (!uci->mc)
+		return UCODE_NFOUND;
+
+	mc = uci->mc;
+
+	/*
+	 * 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);
+
+	return UCODE_OK;
+}
+
 static enum ucode_state generic_load_microcode(int cpu, struct iov_iter *iter)
 {
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -1014,10 +1039,11 @@ request_microcode_user(int cpu, const void __user *buf, size_t size)
 }
 
 static struct microcode_ops microcode_intel_ops = {
-	.request_microcode_user		  = request_microcode_user,
-	.request_microcode_fw             = request_microcode_fw,
-	.collect_cpu_info                 = collect_cpu_info,
-	.apply_microcode                  = apply_microcode_intel,
+	.request_microcode_user		= request_microcode_user,
+	.request_microcode_fw		= request_microcode_fw,
+	.collect_cpu_info		= collect_cpu_info,
+	.apply_microcode		= apply_microcode_intel,
+	.apply_microcode_nocheck	= apply_microcode_nocheck_intel,
 };
 
 static int __init calc_llc_size_per_core(struct cpuinfo_x86 *c)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Boris Ostrovsky]

On 9/3/19 12:46 PM, Borislav Petkov wrote:
>
>  
> @@ -629,8 +639,12 @@ static ssize_t reload_store(struct device *dev,
>  	if (ret)
>  		return ret;
>  
> -	if (val != 1)
> +	if (val == 2) {
> +		add_taint(TAINT_CPU_OUT_OF_SPEC, LOCKDEP_STILL_OK);

Why do we need to taint kernel here? We are not making any changes.


> +		return microcode_reload_late(apply_microcode_nocheck);
> +	} else if (val != 1) {
>  		return size;
> +	}
>  
>  	tmp_ret = microcode_ops->request_microcode_fw(bsp, &microcode_pdev->dev, true);

This won't allow people to load from new microcode blob which I thought
was one of the objectives behind this new feature.

-boris

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Boris Petkov]

On September 5, 2019 12:06:54 AM GMT+02:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>Why do we need to taint kernel here? We are not making any changes.

Because this is not a normal operation we want users to do. This is only for testing microcode quicker.

>This won't allow people to load from new microcode blob which I thought
>was one of the objectives behind this new feature.

You load a new blob the usual way: echo 1 > ...

This is the "unusual" way where you reload an already loaded revision only.

-- 
Sent from a small device: formatting sux and brevity is inevitable.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

On Thu, Sep 05, 2019 at 12:12:29AM +0200, Boris Petkov wrote:
> On September 5, 2019 12:06:54 AM GMT+02:00, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> >Why do we need to taint kernel here? We are not making any changes.
> 
> Because this is not a normal operation we want users to do. This is only for testing microcode quicker.
> 
> >This won't allow people to load from new microcode blob which I thought
> >was one of the objectives behind this new feature.
> 
> You load a new blob the usual way: echo 1 > ...
> 
> This is the "unusual" way where you reload an already loaded revision only.

But echo 2 > reload would allow reading a microcode file from 
/lib/firmware/intel-ucode/ even if the revision hasn't changed right?

#echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
and we want to permit that with the echo 2 option.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Wed, Sep 04, 2019 at 05:21:32PM -0700, Raj, Ashok wrote:
> But echo 2 > reload would allow reading a microcode file from 
> /lib/firmware/intel-ucode/ even if the revision hasn't changed right?
> 
> #echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
> and we want to permit that with the echo 2 option.

Then before we continue with this, please specify what the exact
requirements are. Talk to your microcoders or whoever is going to use
this and give the exact use cases which should be supported and describe
them in detail.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

On Thu, 5 Sep 2019, Borislav Petkov wrote:
> On Wed, Sep 04, 2019 at 05:21:32PM -0700, Raj, Ashok wrote:
> > But echo 2 > reload would allow reading a microcode file from 
> > /lib/firmware/intel-ucode/ even if the revision hasn't changed right?
> > 
> > #echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
> > and we want to permit that with the echo 2 option.
> 
> Then before we continue with this, please specify what the exact
> requirements are. Talk to your microcoders or whoever is going to use
> this and give the exact use cases which should be supported and describe
> them in detail.

Plus I have to ask whether this facility makes any sense outside of
microcode development. If so, then please explain the use case and give
a proper justification why this needs to be in the mainline kernel.

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

Hi Boris

On Thu, Sep 05, 2019 at 09:20:29AM +0200, Borislav Petkov wrote:
> On Wed, Sep 04, 2019 at 05:21:32PM -0700, Raj, Ashok wrote:
> > But echo 2 > reload would allow reading a microcode file from 
> > /lib/firmware/intel-ucode/ even if the revision hasn't changed right?
> > 
> > #echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
> > and we want to permit that with the echo 2 option.
> 
> Then before we continue with this, please specify what the exact
> requirements are. Talk to your microcoders or whoever is going to use
> this and give the exact use cases which should be supported and describe
> them in detail.

https://lore.kernel.org/lkml/1567056803-6640-1-git-send-email-ashok.raj@intel.com/

The original description said to load a new microcode file, the content
could have changed, but revision in the header hasn't increased. 

The other rules are same, i.e we can't go backwards. There is another
SVN (Security version number) embedded in the microcode which won't allow
going backwards anyway. 

I'll get back to you if there are additional uses, but allowing the facility to 
actually read the file achieves the same purpose as using the in-kernel copy.

I have used it multiple times during development :-)

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Thu, Sep 05, 2019 at 12:40:44PM -0700, Raj, Ashok wrote:
> The original description said to load a new microcode file, the content
> could have changed, but revision in the header hasn't increased.

How does the hardware even accept a revision which is the same as the
one already loaded?

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

On Thu, Sep 05, 2019 at 09:49:50PM +0200, Borislav Petkov wrote:
> On Thu, Sep 05, 2019 at 12:40:44PM -0700, Raj, Ashok wrote:
> > The original description said to load a new microcode file, the content
> > could have changed, but revision in the header hasn't increased.
> 
> How does the hardware even accept a revision which is the same as the
> one already loaded?

Hardware will allow a reload as long as the loaded ucode svn <= the version being updated.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

Raj,

On Thu, 5 Sep 2019, Raj, Ashok wrote:
> On Thu, Sep 05, 2019 at 09:20:29AM +0200, Borislav Petkov wrote:
> > On Wed, Sep 04, 2019 at 05:21:32PM -0700, Raj, Ashok wrote:
> > > But echo 2 > reload would allow reading a microcode file from 
> > > /lib/firmware/intel-ucode/ even if the revision hasn't changed right?
> > > 
> > > #echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
> > > and we want to permit that with the echo 2 option.
> > 
> > Then before we continue with this, please specify what the exact
> > requirements are. Talk to your microcoders or whoever is going to use
> > this and give the exact use cases which should be supported and describe
> > them in detail.
> 
> https://lore.kernel.org/lkml/1567056803-6640-1-git-send-email-ashok.raj@intel.com/
> 
> The original description said to load a new microcode file, the content
> could have changed, but revision in the header hasn't increased. 
> 
> The other rules are same, i.e we can't go backwards. There is another
> SVN (Security version number) embedded in the microcode which won't allow
> going backwards anyway. 
> 
> I'll get back to you if there are additional uses, but allowing the facility to 
> actually read the file achieves the same purpose as using the in-kernel copy.
> 
> I have used it multiple times during development :-)

That's all nice, but what it the general use case for this outside of Intel's
microcode development and testing?

We all know that late microcode loading has severe limitations and we
really don't want to proliferate that further if not absolutely required
for something which needs this in a place which cannot deal with out of
tree patches or have some special kernel package for that purpose
installed. Intel's microcode devel/testing does not qualify obviously.

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

Hi Thomas,


On Thu, Sep 05, 2019 at 11:22:31PM +0200, Thomas Gleixner wrote:
> Raj,
> 
> On Thu, 5 Sep 2019, Raj, Ashok wrote:
> > On Thu, Sep 05, 2019 at 09:20:29AM +0200, Borislav Petkov wrote:
> > > On Wed, Sep 04, 2019 at 05:21:32PM -0700, Raj, Ashok wrote:
> > > > But echo 2 > reload would allow reading a microcode file from 
> > > > /lib/firmware/intel-ucode/ even if the revision hasn't changed right?
> > > > 
> > > > #echo 1 > reload wouldn't load if the revision on disk is same as what's loaded,
> > > > and we want to permit that with the echo 2 option.
> > > 
> > > Then before we continue with this, please specify what the exact
> > > requirements are. Talk to your microcoders or whoever is going to use
> > > this and give the exact use cases which should be supported and describe
> > > them in detail.
> > 
> > https://lore.kernel.org/lkml/1567056803-6640-1-git-send-email-ashok.raj@intel.com/
> > 
> > The original description said to load a new microcode file, the content
> > could have changed, but revision in the header hasn't increased. 
> > 
> > The other rules are same, i.e we can't go backwards. There is another
> > SVN (Security version number) embedded in the microcode which won't allow
> > going backwards anyway. 
> > 
> > I'll get back to you if there are additional uses, but allowing the facility to 
> > actually read the file achieves the same purpose as using the in-kernel copy.
> > 
> > I have used it multiple times during development :-)
> 
> That's all nice, but what it the general use case for this outside of Intel's
> microcode development and testing?
> 
> We all know that late microcode loading has severe limitations and we
> really don't want to proliferate that further if not absolutely required

Several customers have asked this to check the safety of late loads. They want
to validate in production setup prior to rolling late-load to all production systems.

Thanks
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Thu, Sep 05, 2019 at 03:27:06PM -0700, Raj, Ashok wrote:
> Several customers have asked this to check the safety of late loads.
> They want to validate in production setup prior to rolling late-load
> to all production systems.

First of all, they should use *early* loading. I don't know how many
times I need to say that, maybe I should print it on T-shirts.

And then they can reboot. If you're thinking of the
"we-can't-reboot-because-we're-cloud" argument don't even bother to
bring it - I've had it more than enough and no, just because cloud
people don't want to reboot, it doesn't mean we will support some insane
use case.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

Raj,

On Thu, 5 Sep 2019, Raj, Ashok wrote:
> On Thu, Sep 05, 2019 at 11:22:31PM +0200, Thomas Gleixner wrote:
> > That's all nice, but what it the general use case for this outside of Intel's
> > microcode development and testing?
> > 
> > We all know that late microcode loading has severe limitations and we
> > really don't want to proliferate that further if not absolutely required
> 
> Several customers have asked this to check the safety of late loads. They want
> to validate in production setup prior to rolling late-load to all production systems.

Groan. Late loading _IS_ broken by definition and it was so forever.

What your customers are asking for is a receipe for disaster. They can
check the safety of late loading forever, it will not magically become safe
because they do so.

If you want late loading, then the whole approach needs to be reworked from
ground up. You need to make sure that all CPUs are in a safe state,
i.e. where switching of CPU feature bits of all sorts can be done with the
guarantee that no CPU will return to the wrong code path after coming out
of safe state and that any kernel internal state which depends on the
previous set of CPU feature bits has been mopped up and switched over
before CPUs are released.

That does not exist and unless it does, late loading is just going to cause
trouble nothing else.

So, no. We are not merging something which is known to be broken and then
we have to deal with the subtle fallout and the bug reports forever. Not to
talk about having to fend of half baken duct tape patches which try to glue
things together.

The only sensible patch for that is to remove any trace of late loading
crappola once and forever.

Sorry, -ENOPONIES

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Johannes Erdfelt]

On Fri, Sep 06, 2019, Thomas Gleixner <tglx@linutronix.de> wrote:
> What your customers are asking for is a receipe for disaster. They can
> check the safety of late loading forever, it will not magically become safe
> because they do so.
> 
> If you want late loading, then the whole approach needs to be reworked from
> ground up. You need to make sure that all CPUs are in a safe state,
> i.e. where switching of CPU feature bits of all sorts can be done with the
> guarantee that no CPU will return to the wrong code path after coming out
> of safe state and that any kernel internal state which depends on the
> previous set of CPU feature bits has been mopped up and switched over
> before CPUs are released.

You say that switching of CPU feature bits is problematic, but adding
new features should result only in a warning ("x86/CPU: CPU features
have changed after loading microcode, but might not take effect.").

Removing a CPU feature bit could be problematic. Other than HLE being
removed on Haswell (which the kernel shouldn't use anyway), have there
been any other cases?

I ask because we have successfully used late microcode loading on tens
of thousands of hosts. I'm a bit worried to see that there is a push to
remove a feature that we currently rely on.

JE

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Fri, Sep 06, 2019 at 07:40:39AM -0700, Johannes Erdfelt wrote:
> You say that switching of CPU feature bits is problematic, but adding
> new features should result only in a warning ("x86/CPU: CPU features
> have changed after loading microcode, but might not take effect.").

That's the only sane thing we can do ATM - warn the user to reboot.

> Removing a CPU feature bit could be problematic. Other than HLE being
> removed on Haswell (which the kernel shouldn't use anyway), have there
> been any other cases?

I hope there aren't. There are cases, however, where late loading cannot
fix an issue, like paging on some old Atoms. And then you *must* do
early loading or stick it in the BIOS. But we all know how the latter
works in practice.

> I ask because we have successfully used late microcode loading on tens
> of thousands of hosts.

How do you deal with all the mitigations microcode loaded late?

> I'm a bit worried to see that there is a push to remove a feature that
> we currently rely on.

I'd love to remove it. And the fact that people rely on it more instead
of fixing their infrastructure to reboot machines and do early microcode
updates is making it worse. Microcode update should be batched with
kernel updates and that's it. They happen normally once-twice per year -
except the last two years but the last two years are not normal anyway
- and done. No need to do some crazy CPUID features reloading dances in
the kernel and making sure cores will see the updated paths and so on.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Johannes Erdfelt]

On Fri, Sep 06, 2019, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Sep 06, 2019 at 07:40:39AM -0700, Johannes Erdfelt wrote:
> > I ask because we have successfully used late microcode loading on tens
> > of thousands of hosts.
> 
> How do you deal with all the mitigations microcode loaded late?

We developed livepatches to add the necessary support. I understand we
aren't the typical Linux user. We do custom development and validation
to support our use case.

That said, we very much rely on late microcode loading and it has helped
us and our customers significantly.

> > I'm a bit worried to see that there is a push to remove a feature that
> > we currently rely on.
> 
> I'd love to remove it. And the fact that people rely on it more instead
> of fixing their infrastructure to reboot machines and do early microcode
> updates is making it worse. Microcode update should be batched with
> kernel updates and that's it. They happen normally once-twice per year -
> except the last two years but the last two years are not normal anyway
> - and done. No need to do some crazy CPUID features reloading dances in
> the kernel and making sure cores will see the updated paths and so on.

It's really easy to say "fix your infrastructure" when you're not
running that infrastructure.

Reboots suck. Customers hate it. Operations hates it. When you get into
the number of hosts we have, you run into all kinds of weird failure
scenarios. (What do you mean that the NIC that was working just fine
before the reboot is no longer seen on the PCI bus?)

The more reboots we can avoid, the better it is for us and our
customers.

I understand that it could be unsafe to late load some rare microcode
updates (theoretical or not). However, that is certainly the exception.
We have done this multiple times on our fleet and we plan to continue
doing so in the future.

There just aren't any good alternatives currently.

JE

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Fri, Sep 06, 2019 at 08:46:18AM -0700, Johannes Erdfelt wrote:
> That said, we very much rely on late microcode loading and it has helped
> us and our customers significantly.

You do realize that you rely on an update method which *won't* work in
all possible cases and then you *will* have to reboot if the microcode
patching *must* happen early, do you?

> It's really easy to say "fix your infrastructure" when you're not
> running that infrastructure.

I'm not saying you should fix your infrastructure now - I'm saying you
should keep that in mind when thinking whether to rely more on late
loading or not. Who knows, maybe newer generation machines in the fleet
could do load balancing, live migration, whatever fancy new cloud stuff
it is, to facilitate a proper reboot.

Or someone could rewrite arch/x86/ to rediscover new features upon a
microcode reload or a feature disabling. And do that in a clean way. Who
knows...

> Reboots suck. Customers hate it. Operations hates it. When you get into
> the number of hosts we have, you run into all kinds of weird failure
> scenarios. (What do you mean that the NIC that was working just fine
> before the reboot is no longer seen on the PCI bus?)

Yeah, I've heard all the stories.

> The more reboots we can avoid, the better it is for us and our
> customers.

So how do you update the kernels on those machines? Or you live-patch in
the new functionality too?

> I understand that it could be unsafe to late load some rare microcode
> updates (theoretical or not). However, that is certainly the exception.
> We have done this multiple times on our fleet and we plan to continue
> doing so in the future.

The fact that it has worked for you does not make it right. It won't
magically become safe, as tglx said.

But since you do custom development, you should be fine, it seems.

Practically speaking, late loading probably won't disappear as it is
being used apparently. Just don't expect that it will get "extended" if
that extension brings with itself fallout and duct tape fixes left and
right.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Konrad Rzeszutek Wilk]

> Or someone could rewrite arch/x86/ to rediscover new features upon a
> microcode reload or a feature disabling. And do that in a clean way. Who
> knows...

The clean way to do microcode reloading and the vast amount of re-initialization
that has to happen is the definitly what we all want.

It may not surprise you that we have tinkered with this, but what we have
is very far from 'clean'.

Do you have insights on the best way to restructure the code for this?

..snip..

> Practically speaking, late loading probably won't disappear as it is
> being used apparently. Just don't expect that it will get "extended" if
> that extension brings with itself fallout and duct tape fixes left and
> right.

We don't want duct-tape.

I am hoping you can help in figuring out a way that will be acceptable.

We did an analysis of some of the the things that we would need to address
to make this work, but sadly it only covered the "oh crud, this has to
be thought off", but not the overall architecture.

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Johannes Erdfelt]

On Fri, Sep 06, 2019, Borislav Petkov <bp@alien8.de> wrote:
> On Fri, Sep 06, 2019 at 08:46:18AM -0700, Johannes Erdfelt wrote:
> > That said, we very much rely on late microcode loading and it has helped
> > us and our customers significantly.
> 
> You do realize that you rely on an update method which *won't* work in
> all possible cases and then you *will* have to reboot if the microcode
> patching *must* happen early, do you?

Yeah. I even explained some cases where it wouldn't work.

If we get a microcode update that isn't late loadable, then yes, we have
to do something different.

That doesn't mean that late loading isn't still useful. In practice, it
can be very valuable. It isn't bad or dangerous in vast majority of
cases. We haven't had a microcode update across all of the models of
Intel CPUs we have (going back a handful of generations) in the past
almost two years that wasn't safely late loadable.

Just as I can't know for sure that every future microcode update will be
safely late loadable, you can't know for sure that every future microcode
update won't be safely late loadable.

> > It's really easy to say "fix your infrastructure" when you're not
> > running that infrastructure.
> 
> I'm not saying you should fix your infrastructure now - I'm saying you
> should keep that in mind when thinking whether to rely more on late
> loading or not. Who knows, maybe newer generation machines in the fleet
> could do load balancing, live migration, whatever fancy new cloud stuff
> it is, to facilitate a proper reboot.

We are well aware of the downsides of late microcode loading and live
patching. However, like I said, it's currently the best tools available.

We are certainly looking at other options, but some aren't feasible for
mitigating security vulnerabilities where time to getting patched is
very much important.

> > The more reboots we can avoid, the better it is for us and our
> > customers.
> 
> So how do you update the kernels on those machines? Or you live-patch in
> the new functionality too?

Depends on what kind of patch is needed. Livepatching, while having it's
own set of problems, has been very valuable for us.

We do use other techniques as well particularly when it's not time
sensitive.

> > I understand that it could be unsafe to late load some rare microcode
> > updates (theoretical or not). However, that is certainly the exception.
> > We have done this multiple times on our fleet and we plan to continue
> > doing so in the future.
> 
> The fact that it has worked for you does not make it right. It won't
> magically become safe, as tglx said.

It very much makes it right because it's still a tool that can be used
safely in the right cases. Just because it can't be used 100% of the time
(even if it is close to that in practice) doesn't make it magically unsafe
either.

> Practically speaking, late loading probably won't disappear as it is
> being used apparently. Just don't expect that it will get "extended" if
> that extension brings with itself fallout and duct tape fixes left and
> right.

I don't have a particular use case for the patchset at hand and I'm
certainly not arguing for or against this patchset.

But I do get concerned when there is talk about removing a feature we
currently use extensively.

I'm happy to hear it will likely not be removed and I hope it was partly
because I spoke up to show that is actively being used and it's important
to us.

JE

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

Hi Thomas,

On Fri, Sep 06, 2019 at 02:51:17PM +0200, Thomas Gleixner wrote:
> Raj,
> 
> On Thu, 5 Sep 2019, Raj, Ashok wrote:
> > On Thu, Sep 05, 2019 at 11:22:31PM +0200, Thomas Gleixner wrote:
> > > That's all nice, but what it the general use case for this outside of Intel's
> > > microcode development and testing?
> > > 
> > > We all know that late microcode loading has severe limitations and we
> > > really don't want to proliferate that further if not absolutely required
> > 
> > Several customers have asked this to check the safety of late loads. They want
> > to validate in production setup prior to rolling late-load to all production systems.
> 
> Groan. Late loading _IS_ broken by definition and it was so forever.

Lets tighten the seat belts :-).. I'm with you that late-loading has
shown weakness more recently than earlier. There are several obvious reasons
that you are well aware. But there is a lot that *must* be done to make sure
the guard rails are tight enough for deplopying late-load. 100% agree on that
to make sure the interface and mechanism needs to be improved for robustness
but not a candidate for removal. Certainly this is an argument that would help
me drive towards that objective internally.

> 
> What your customers are asking for is a receipe for disaster. They can
> check the safety of late loading forever, it will not magically become safe
> because they do so.
> 
> If you want late loading, then the whole approach needs to be reworked from
> ground up. You need to make sure that all CPUs are in a safe state,
> i.e. where switching of CPU feature bits of all sorts can be done with the
> guarantee that no CPU will return to the wrong code path after coming out
> of safe state and that any kernel internal state which depends on the
> previous set of CPU feature bits has been mopped up and switched over
> before CPUs are released.
> 
> That does not exist and unless it does, late loading is just going to cause
> trouble nothing else.
> 
> So, no. We are not merging something which is known to be broken and then
> we have to deal with the subtle fallout and the bug reports forever. Not to

When we did the late-load changes last year we added a warning if any
of the cpuid bits either dissappear or new ones appear. Maybe we should
have tainted the kernel to track that so its not that subtle anymore. 

> talk about having to fend of half baken duct tape patches which try to glue
> things together.
> 
> The only sensible patch for that is to remove any trace of late loading
> crappola once and forever.
> 
> Sorry, -ENOPONIES

:-)

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Fri, Sep 06, 2019 at 12:43:53PM -0400, Konrad Rzeszutek Wilk wrote:
> Do you have insights on the best way to restructure the code for this?

Well, I only started thinking about this when you guys brought it on and
you were actually serious about it. :)

So IINM, this is one of the livepatch problems where the universe before
the patching and after the patching needs to be consistent, so to speak.

But how do you handle the case where feature detection has happened, a
bunch of code is active now and running because the feature is there and
then you disable that feature and all that code does what? And what do
you tell the user programs which are using it?

Sounds a lot like a reboot to me. :-P

Was the code programmed with the ability to be gracefully disabled at
any point in time, in mind? I doubt that. I don't think any of the CPU
feature supporting code has been programmed to be disabled at arbitrary
points in time.

Now, can you do something like suspend the CPUs, disable some
features and resume them and make them re-detect it all again and act
accordingly?

Sure, we do some of that but not comprehensively...

So my gut feeling is this is just the tip of the iceberg and the devil
is in the detail and all sorts of similar proverbs.

Best guess would be, try to do it and see what kinds of problems you
encounter and try to fix them cleanly. I betcha you'll be busy a long
time...

:-)

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Fri, Sep 06, 2019 at 09:52:07AM -0700, Johannes Erdfelt wrote:
> That doesn't mean that late loading isn't still useful.

If it weren't useful, it would've been gone a long time ago. No one is
arguing whether it is useful or not.

> Just as I can't know for sure that every future microcode update will be
> safely late loadable, you can't know for sure that every future microcode
> update won't be safely late loadable.

Well, you know what can happen so good luck, I guess.

> We do use other techniques as well particularly when it's not time
> sensitive.

So you reboot or not? Do you do reboot-similar techniques where you can
potentially do early microcode loading too?

> It very much makes it right because it's still a tool that can be used
> safely in the right cases. Just because it can't be used 100% of the time
> (even if it is close to that in practice) doesn't make it magically unsafe
> either.

As I said, good luck with that. It's not like you haven't been warned
about what can happen.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

On Fri, 6 Sep 2019, Johannes Erdfelt wrote:
> On Fri, Sep 06, 2019, Thomas Gleixner <tglx@linutronix.de> wrote:
> > What your customers are asking for is a receipe for disaster. They can
> > check the safety of late loading forever, it will not magically become safe
> > because they do so.
> > 
> > If you want late loading, then the whole approach needs to be reworked from
> > ground up. You need to make sure that all CPUs are in a safe state,
> > i.e. where switching of CPU feature bits of all sorts can be done with the
> > guarantee that no CPU will return to the wrong code path after coming out
> > of safe state and that any kernel internal state which depends on the
> > previous set of CPU feature bits has been mopped up and switched over
> > before CPUs are released.
> 
> You say that switching of CPU feature bits is problematic, but adding
> new features should result only in a warning ("x86/CPU: CPU features
> have changed after loading microcode, but might not take effect.").
> 
> Removing a CPU feature bit could be problematic. Other than HLE being
> removed on Haswell (which the kernel shouldn't use anyway), have there
> been any other cases?
> 
> I ask because we have successfully used late microcode loading on tens
> of thousands of hosts. I'm a bit worried to see that there is a push to
> remove a feature that we currently rely on.

The point is that you know what's on stake so you can evaluate precisely
upfront whether that works or not and you have experienced kernel engineers
on staff who can tell you which kind of ucode change is going to explode in
your face and which on does not.

So it's the special case of a large cloud company with experts on staff.

Now map that to the average user/sysadmin. If we proliferate this, then the
inevitable consequence will be that those people read about how great that
is and how it made your customers happy yadayadayada. Now they go and do
the same thing and guess what happens? It explodes in their face, they send
bug reports and someone else will send lousy patches to paper over the
problem. None of this ends on your desk.

Yes you can surely argue that if you give people a gun then they can shoot
themself into their foot. But in that case it's a irresponsible argument
which just put's your interest above the general rule of not offering
things which are bound to break in all flavours of wreckage especially in
the hard to diagnose way.

So if we want to do late microcode loading in a sane way then there are
only a few options and none of them exist today:

 1) Micro-code contains a description of CPUID bits which are going to be
    exposed after the load. Then the kernel can sanity check whether this
    changes anything relevant or not. If there is a relevant change it can
    reject the load and tell the admin that a reboot is required.

 2) Rework CPUID feature handling so that it can reevaluate and reconfigure
    the running system safely. There are a lot of things you need for that:

    A) Introduce a safe state for CPUs to reach which guarantees that none
       of the CPUs will return from that state via a code path which
       depends on previous state and might now go the other route with data
       on the stack which only fits the previous configuration.

    B) Make all the cpufeature thingies run time switchable. That means
       that you need to keep quite some code around which is currently init
       only. That also means that you have to provide backout code for
       things which set up data corresponding to cpu feature bits and so
       forth.

So #2 might be finished in about 20 years from now with the result that
some of the code pathes might simply still have a

     if (cpufeature_changed())
     	   panic();

because there are things which you cannot back out. So the only sane
solution is to panic. Which is not a solution as it would be much more sane
to prevent late loading upfront and force people to reboot proper.

Now #1 is actually a sensible and feasible solution which can be pulled off
in a reasonably short time frame, avoids all the bound to be ugly and
failure laden attempts of fixing late loading completely and provides a
usable and safe solution for joe user, jack admin and the super experts at
big-cloud corporate.

That is not requiring any new format of microcode payload, as this can be
nicely done as a metadata package which comes with the microcode
payload. So you get the following backwards compatible states:

  Kernel  metadata	  result

  old	  don't care	  refuse late load

  new	  No   		  refuse late load

  new	  Yes		  decide based on metadata

Thoughts?

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

On Fri, Sep 06, 2019 at 11:16:00PM +0200, Thomas Gleixner wrote:
> 
> So if we want to do late microcode loading in a sane way then there are
> only a few options and none of them exist today:
> 
>  1) Micro-code contains a description of CPUID bits which are going to be
>     exposed after the load. Then the kernel can sanity check whether this
>     changes anything relevant or not. If there is a relevant change it can
>     reject the load and tell the admin that a reboot is required.

This is pretty much what we had in mind when we suggested to the uCode teams.

Just a process of providing a meta data file to accompany every uCode release.

IMO new cpuid bits are probably less harmful than old ones dissappearing.



> 
>  2) Rework CPUID feature handling so that it can reevaluate and reconfigure
>     the running system safely. There are a lot of things you need for that:
> 
>     A) Introduce a safe state for CPUs to reach which guarantees that none
>        of the CPUs will return from that state via a code path which
>        depends on previous state and might now go the other route with data
>        on the stack which only fits the previous configuration.
> 
>     B) Make all the cpufeature thingies run time switchable. That means
>        that you need to keep quite some code around which is currently init
>        only. That also means that you have to provide backout code for
>        things which set up data corresponding to cpu feature bits and so
>        forth.
> 
> So #2 might be finished in about 20 years from now with the result that
> some of the code pathes might simply still have a

Maybe we can catch the kernel side in 20 years.. user space would still be 
busted, or have a fault way to control new cpuid much like how we do for
VM's. 

> 
>      if (cpufeature_changed())
>      	   panic();
> 
> because there are things which you cannot back out. So the only sane
> solution is to panic. Which is not a solution as it would be much more sane
> to prevent late loading upfront and force people to reboot proper.
> 
> Now #1 is actually a sensible and feasible solution which can be pulled off
> in a reasonably short time frame, avoids all the bound to be ugly and
> failure laden attempts of fixing late loading completely and provides a
> usable and safe solution for joe user, jack admin and the super experts at
> big-cloud corporate.
> 
> That is not requiring any new format of microcode payload, as this can be
> nicely done as a metadata package which comes with the microcode
> payload. So you get the following backwards compatible states:
> 
>   Kernel  metadata	  result
> 
>   old	  don't care	  refuse late load
> 
>   new	  No   		  refuse late load
> 
>   new	  Yes		  decide based on metadata
> 
> Thoughts?

This is 100% in line with what we proposed... 

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

On Fri, 6 Sep 2019, Raj, Ashok wrote:
> On Fri, Sep 06, 2019 at 11:16:00PM +0200, Thomas Gleixner wrote:
> > Now #1 is actually a sensible and feasible solution which can be pulled off
> > in a reasonably short time frame, avoids all the bound to be ugly and
> > failure laden attempts of fixing late loading completely and provides a
> > usable and safe solution for joe user, jack admin and the super experts at
> > big-cloud corporate.
> > 
> > That is not requiring any new format of microcode payload, as this can be
> > nicely done as a metadata package which comes with the microcode
> > payload. So you get the following backwards compatible states:
> > 
> >   Kernel  metadata	  result
> > 
> >   old	  don't care	  refuse late load
> > 
> >   new	  No   		  refuse late load
> > 
> >   new	  Yes		  decide based on metadata
> > 
> > Thoughts?
> 
> This is 100% in line with what we proposed... 

So what it hindering you to implement that? ucode teams whining about the
little bit of extra work?

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

On Sat, 7 Sep 2019, Thomas Gleixner wrote:
> On Fri, 6 Sep 2019, Raj, Ashok wrote:
> > On Fri, Sep 06, 2019 at 11:16:00PM +0200, Thomas Gleixner wrote:
> > > Now #1 is actually a sensible and feasible solution which can be pulled off
> > > in a reasonably short time frame, avoids all the bound to be ugly and
> > > failure laden attempts of fixing late loading completely and provides a
> > > usable and safe solution for joe user, jack admin and the super experts at
> > > big-cloud corporate.
> > > 
> > > That is not requiring any new format of microcode payload, as this can be
> > > nicely done as a metadata package which comes with the microcode
> > > payload. So you get the following backwards compatible states:
> > > 
> > >   Kernel  metadata	  result
> > > 
> > >   old	  don't care	  refuse late load
> > > 
> > >   new	  No   		  refuse late load
> > > 
> > >   new	  Yes		  decide based on metadata
> > > 
> > > Thoughts?
> > 
> > This is 100% in line with what we proposed... 
> 
> So what it hindering you to implement that? ucode teams whining about the
> little bit of extra work?

That said, there is also a distinct lack of information about micro code
loading in a safe way in general. We absolutely do not know whether a micro
code update affects any instruction which might be in use during the update
on a sibling. Right now it's all load and pray and the SDM is not really
helpful with that either.

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

On Mon, Sep 16, 2019 at 12:36:11PM +0200, Thomas Gleixner wrote:
> > On Fri, 6 Sep 2019, Raj, Ashok wrote:
> > > On Fri, Sep 06, 2019 at 11:16:00PM +0200, Thomas Gleixner wrote:
> > > > Now #1 is actually a sensible and feasible solution which can be pulled off
> > > > in a reasonably short time frame, avoids all the bound to be ugly and
> > > > failure laden attempts of fixing late loading completely and provides a
> > > > usable and safe solution for joe user, jack admin and the super experts at
> > > > big-cloud corporate.
> > > > 
> > > > That is not requiring any new format of microcode payload, as this can be
> > > > nicely done as a metadata package which comes with the microcode
> > > > payload. So you get the following backwards compatible states:
> > > > 
> > > >   Kernel  metadata	  result
> > > > 
> > > >   old	  don't care	  refuse late load
> > > > 
> > > >   new	  No   		  refuse late load
> > > > 
> > > >   new	  Yes		  decide based on metadata
> > > > 
> > > > Thoughts?
> > > 
> > > This is 100% in line with what we proposed... 
> > 
> > So what it hindering you to implement that? ucode teams whining about the
> > little bit of extra work?
> 
> That said, there is also a distinct lack of information about micro code
> loading in a safe way in general. We absolutely do not know whether a micro
> code update affects any instruction which might be in use during the update
> on a sibling. Right now it's all load and pray and the SDM is not really
> helpful with that either.
> 

Guilty as charged :-). In general we do not expect microcode updates to 
remove any cpuid bits (Not that it hasn't happened, but it slipped through
the cracks). 

microode updates should be of 3 types.

- Only loadable from BIOS (Only via FIT tables)
- Suitable for early load (things that take cpuid bits for e.g.)
- Suitable for late-load. (Where no cpuid bits should change etc).

Today the way we load after a stop_machine() all threads in the system are
held hostage until all the cores have done the update. The thread sibling
is also in the rendezvous loop. 

Do you think we still have that risk with a sibling thread? 
(Assuming future ucodes don't do weird things like what happened in 
that case where a cpuid was removed via an update)

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Thomas Gleixner]

Raj,

On Mon, 16 Sep 2019, Raj, Ashok wrote:
> On Mon, Sep 16, 2019 at 12:36:11PM +0200, Thomas Gleixner wrote:
> > That said, there is also a distinct lack of information about micro code
> > loading in a safe way in general. We absolutely do not know whether a micro
> > code update affects any instruction which might be in use during the update
> > on a sibling. Right now it's all load and pray and the SDM is not really
> > helpful with that either.
> > 
> 
> Guilty as charged :-). In general we do not expect microcode updates to 
> remove any cpuid bits (Not that it hasn't happened, but it slipped through
> the cracks). 
> 
> microode updates should be of 3 types.
> 
> - Only loadable from BIOS (Only via FIT tables)
> - Suitable for early load (things that take cpuid bits for e.g.)
> - Suitable for late-load. (Where no cpuid bits should change etc).
> 
> Today the way we load after a stop_machine() all threads in the system are
> held hostage until all the cores have done the update. The thread sibling
> is also in the rendezvous loop. 

I know. See below.
 
> Do you think we still have that risk with a sibling thread? 
> (Assuming future ucodes don't do weird things like what happened in 
> that case where a cpuid was removed via an update)

Well, yes. The sibling executes a limited set of instructions in a loop,
but it might be hit by an NMI or MCE which executes even more instructions.

So what happens if the ucode update "fixes" one of the executed
instructions on the fly? Is that guaranteed to be safe? There is nothing
which says so.

A decade ago I experimented with putting the spinning CPUs into MWAIT,
which caused havoc. Did neither have time nor the stomach to dig into that
further, but the ucode update _did_ fix an issue with MWAIT according to
the version history.

That's why I'm worried about instructions being "fixed" which are executed
in parallel on the sibling.

An authorative statement vs. that would be appreciated. Preferrably in form
of an extension of the SDM, but an upfront statement in this thread would
be a good start.

Thanks,

	tglx

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Borislav Petkov]

On Tue, Sep 17, 2019 at 08:37:10AM +0200, Thomas Gleixner wrote:
> So what happens if the ucode update "fixes" one of the executed
> instructions on the fly? Is that guaranteed to be safe? There is nothing
> which says so.

You'd expect that when you load microcode on the core, the one thread
does the loading and the other SMT thread is in a holding pattern. That
would be optimal.

Considering the dancing through hoops we're doing to keep all threads
quiesced, I'd be sceptical that is the case...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Raj, Ashok]

Hi Thomas,

On Tue, Sep 17, 2019 at 08:37:10AM +0200, Thomas Gleixner wrote:
> > microode updates should be of 3 types.
> > 
> > - Only loadable from BIOS (Only via FIT tables)
> > - Suitable for early load (things that take cpuid bits for e.g.)
> > - Suitable for late-load. (Where no cpuid bits should change etc).
> > 
> > Today the way we load after a stop_machine() all threads in the system are
> > held hostage until all the cores have done the update. The thread sibling
> > is also in the rendezvous loop. 
> 
> I know. See below.
>  
> > Do you think we still have that risk with a sibling thread? 
> > (Assuming future ucodes don't do weird things like what happened in 
> > that case where a cpuid was removed via an update)
> 
> Well, yes. The sibling executes a limited set of instructions in a loop,
> but it might be hit by an NMI or MCE which executes even more instructions.

There is a plan to solve the NMI issue. Although there is one case we might
be showing as a spurious that might not be nice. If #MCE's showup there is
nothing we can do at that point. These are most likely unrecoverable. 
But we want to make sure we could atleast follow through with a proper reset.

Let me gather my thoughts on that when i have the patch ready to handle
those senarios.

> 
> So what happens if the ucode update "fixes" one of the executed
> instructions on the fly? Is that guaranteed to be safe? There is nothing
> which says so.
> 
> A decade ago I experimented with putting the spinning CPUs into MWAIT,
> which caused havoc. Did neither have time nor the stomach to dig into that
> further, but the ucode update _did_ fix an issue with MWAIT according to
> the version history.

Excellent point. 
> 
> That's why I'm worried about instructions being "fixed" which are executed
> in parallel on the sibling.
> 
> An authorative statement vs. that would be appreciated. Preferrably in form
> of an extension of the SDM, but an upfront statement in this thread would
> be a good start.

I have started the conversation internally. Once we have something solid
I'll share in the list, and also follow up with updates to SDM. 

Cheers,
Ashok

Re: [PATCH] x86/microcode: Add an option to reload microcode even if revision is unchanged

[Mihai Carabas]

La 07.09.2019 00:16, Thomas Gleixner a scris:
> On Fri, 6 Sep 2019, Johannes Erdfelt wrote:
>> On Fri, Sep 06, 2019, Thomas Gleixner <tglx@linutronix.de> wrote:
>>> What your customers are asking for is a receipe for disaster. They can
>>> check the safety of late loading forever, it will not magically become safe
>>> because they do so.
>>>
>>> If you want late loading, then the whole approach needs to be reworked from
>>> ground up. You need to make sure that all CPUs are in a safe state,
>>> i.e. where switching of CPU feature bits of all sorts can be done with the
>>> guarantee that no CPU will return to the wrong code path after coming out
>>> of safe state and that any kernel internal state which depends on the
>>> previous set of CPU feature bits has been mopped up and switched over
>>> before CPUs are released.
>>
>> You say that switching of CPU feature bits is problematic, but adding
>> new features should result only in a warning ("x86/CPU: CPU features
>> have changed after loading microcode, but might not take effect.").
>>
>> Removing a CPU feature bit could be problematic. Other than HLE being
>> removed on Haswell (which the kernel shouldn't use anyway), have there
>> been any other cases?
>>
>> I ask because we have successfully used late microcode loading on tens
>> of thousands of hosts. I'm a bit worried to see that there is a push to
>> remove a feature that we currently rely on.
> 
> The point is that you know what's on stake so you can evaluate precisely
> upfront whether that works or not and you have experienced kernel engineers
> on staff who can tell you which kind of ucode change is going to explode in
> your face and which on does not.
> 
> So it's the special case of a large cloud company with experts on staff.
> 
> Now map that to the average user/sysadmin. If we proliferate this, then the
> inevitable consequence will be that those people read about how great that
> is and how it made your customers happy yadayadayada. Now they go and do
> the same thing and guess what happens? It explodes in their face, they send
> bug reports and someone else will send lousy patches to paper over the
> problem. None of this ends on your desk.
> 
> Yes you can surely argue that if you give people a gun then they can shoot
> themself into their foot. But in that case it's a irresponsible argument
> which just put's your interest above the general rule of not offering
> things which are bound to break in all flavours of wreckage especially in
> the hard to diagnose way.
> 
> So if we want to do late microcode loading in a sane way then there are
> only a few options and none of them exist today:
> 
>   1) Micro-code contains a description of CPUID bits which are going to be
>      exposed after the load. Then the kernel can sanity check whether this
>      changes anything relevant or not. If there is a relevant change it can
>      reject the load and tell the admin that a reboot is required.
> 
>   2) Rework CPUID feature handling so that it can reevaluate and reconfigure
>      the running system safely. There are a lot of things you need for that:
> 
>      A) Introduce a safe state for CPUs to reach which guarantees that none
>         of the CPUs will return from that state via a code path which
>         depends on previous state and might now go the other route with data
>         on the stack which only fits the previous configuration.
> 
>      B) Make all the cpufeature thingies run time switchable. That means
>         that you need to keep quite some code around which is currently init
>         only. That also means that you have to provide backout code for
>         things which set up data corresponding to cpu feature bits and so
>         forth.
> 
> So #2 might be finished in about 20 years from now with the result that
> some of the code pathes might simply still have a
> 
>       if (cpufeature_changed())
>       	   panic();
> 
> because there are things which you cannot back out. So the only sane
> solution is to panic. Which is not a solution as it would be much more sane
> to prevent late loading upfront and force people to reboot proper.
> 
> Now #1 is actually a sensible and feasible solution which can be pulled off
> in a reasonably short time frame, avoids all the bound to be ugly and
> failure laden attempts of fixing late loading completely and provides a
> usable and safe solution for joe user, jack admin and the super experts at
> big-cloud corporate.
> 
> That is not requiring any new format of microcode payload, as this can be
> nicely done as a metadata package which comes with the microcode
> payload. So you get the following backwards compatible states:
> 
>    Kernel  metadata	  result
> 
>    old	  don't care	  refuse late load
> 
>    new	  No   		  refuse late load
> 
>    new	  Yes		  decide based on metadata
> 
> Thoughts?


Internally, we have fix-up multiple corner cases about the late 
microcode loading. We have written some code to handle new features 
showing up but we know they are a bunch of hacks (for sure it lacks of 
different checks that needs to be done before using the new features). I 
am going to take Thomas' suggestion and work on an RFC series.

Thank you,
Mihai Carabas