Patch series to address limitations of OS microcode loading.
Review comments from Boris:
Changes since v1: Patch 1/3
- Check for revision to avoid duplicate microcode load in early load
- Added inline comments.
Changes since v1: Patch 2/3
- Change to native_wbinvd for early load.
Changes since v1: Patch 3/3
- Check for return code of stop_machine
- When microcode file load fails, stop on first error.
- If any of the present CPUs are offline, then stop reload.
This is just for being paranoid.
- Added more comments in commit log and inline in file.
- split some functionality from reload_store() per Boris's comments.
What's not done from review: TBD:
- Load microcode file only once. Added comments in source for future cleanup.
- Removing ucd->errors. (Gives a count of failed loads)
Ashok Raj (3):
x86/microcode/intel: Check microcode revision before updating sibling
threads
x86/microcode/intel: Perform a cache flush before ucode update.
x86/microcode: Quiesce all threads before a microcode update.
arch/x86/kernel/cpu/microcode/core.c | 207 ++++++++++++++++++++++++++++++----
arch/x86/kernel/cpu/microcode/intel.c | 43 ++++++-
2 files changed, 222 insertions(+), 28 deletions(-)
--
2.7.4
After updating microcode on one of the threads in the core, the
thread sibling automatically gets the update since the microcode
resources are shared. Check the ucode revision on the CPU before
performing a ucode update.
Signed-off-by: Ashok Raj <[email protected]>
Cc: X86 ML <[email protected]>
Cc: LKML <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Boris Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Arjan Van De Ven <[email protected]>
Updates:
v2: Address Boris's to cleanup apply_microcode_intel
v3: Fixups per Ingo: Spell Checks
---
arch/x86/kernel/cpu/microcode/intel.c | 28 +++++++++++++++++++++++++---
1 file changed, 25 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 09b95a7..137c9f5 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -589,6 +589,18 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
if (!mc)
return 0;
+ rev = intel_get_microcode_revision();
+
+ /*
+ * Its possible the microcode got updated
+ * because its sibling update was done earlier.
+ * Skip the update in that case.
+ */
+ if (rev >= mc->hdr.rev) {
+ uci->cpu_sig.rev = rev;
+ return UCODE_OK;
+ }
+
/* write microcode via MSR 0x79 */
native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
@@ -776,7 +788,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
{
struct microcode_intel *mc;
struct ucode_cpu_info *uci;
- struct cpuinfo_x86 *c;
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
static int prev_rev;
u32 rev;
@@ -793,6 +805,18 @@ static enum ucode_state apply_microcode_intel(int cpu)
return UCODE_NFOUND;
}
+ rev = intel_get_microcode_revision();
+ /*
+ * Its possible the microcode got updated
+ * because its sibling update was done earlier.
+ * Skip the update in that case.
+ */
+ if (rev >= mc->hdr.rev) {
+ uci->cpu_sig.rev = rev;
+ c->microcode = rev;
+ return UCODE_OK;
+ }
+
/* write microcode via MSR 0x79 */
wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
@@ -813,8 +837,6 @@ static enum ucode_state apply_microcode_intel(int cpu)
prev_rev = rev;
}
- c = &cpu_data(cpu);
-
uci->cpu_sig.rev = rev;
c->microcode = rev;
--
2.7.4
Microcode updates during OS load always assumed the other hyper-thread
was "quiet", but Linux never really did this. We've recently received
several issues on this, where things did not go well at scale
deployments, and the Intel microcode team asked us to make sure the
system is in a quiet state during these updates. Such updates are
rare events, so we use stop_machine() to ensure the whole system is
quiet.
The most robust solution is to have all the CPUs wait in
stop_machine() rendezvous before ucode update, and ensure we stay there
until all the CPUs have completed the microcode load. stop_machine ensures
that interrupts are disabled while waiting, and a spin_lock serializes
that only 1 cpu can perform the update.
Remember microcode resources are shared between the HT siblings of the same
core. So updating microcode on one thread automatically is good enough for the
sibling thread. Microcode update ensures that the version being updated is
greater than what is reported by the CPU. This ensures we do not perform a
redundant load on the HT sibling when one isn't necessary.
During early boot, we bring up one CPU at a time. Hence we only end up
loading for one of the sibling thread and subsequent sibling would already have
the latest copy of the microcode.
Signed-off-by: Ashok Raj <[email protected]>
Cc: X86 ML <[email protected]>
Cc: LKML <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Boris Petkov <[email protected]>
Cc: Arjan Van De Ven <[email protected]>
Changes from V1:
- Check for return code of stop_machine
- When microcode file load fails, stop on first error.
- If any of the present CPUs are offline, then stop reload.
- Added more comments in commitlog and inline in file.
- split some functionality from reload_store() per Boris's comments.
What's not done from review: TBD:
- Load microcode file only once.
- Removing ucd->errors. (Gives a count of failed loads)
---
arch/x86/kernel/cpu/microcode/core.c | 207 ++++++++++++++++++++++++++++++----
arch/x86/kernel/cpu/microcode/intel.c | 1 +
2 files changed, 183 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index aa1b9a4..20d2cbb 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -31,6 +31,10 @@
#include <linux/cpu.h>
#include <linux/fs.h>
#include <linux/mm.h>
+#include <linux/nmi.h>
+#include <linux/stop_machine.h>
+#include <linux/delay.h>
+#include <linux/topology.h>
#include <asm/microcode_intel.h>
#include <asm/cpu_device_id.h>
@@ -489,30 +493,185 @@ static void __exit microcode_dev_exit(void)
/* fake device for request_firmware */
static struct platform_device *microcode_pdev;
-static enum ucode_state reload_for_cpu(int cpu)
+static struct ucode_update_param {
+ spinlock_t ucode_lock; /* Serialize microcode updates */
+ atomic_t count; /* # of CPUs that attempt to load ucode */
+ atomic_t errors; /* # of CPUs ucode load failed */
+ atomic_t enter; /* ucode rendezvous count */
+ int timeout; /* Timeout to wait for all cpus to enter */
+} uc_data;
+
+static int do_ucode_update(int cpu, struct ucode_update_param *ucd)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- enum ucode_state ustate;
+ enum ucode_state retval = 0;
- if (!uci->valid)
- return UCODE_OK;
+ spin_lock(&ucd->ucode_lock);
+ retval = microcode_ops->apply_microcode(cpu);
+ spin_unlock(&ucd->ucode_lock);
+ if (retval > UCODE_NFOUND) {
+ atomic_inc(&ucd->errors);
+ pr_warn("microcode update to CPU %d failed\n", cpu);
+ }
+ atomic_inc(&ucd->count);
+
+ return retval;
+}
+
+/*
+ * Wait for upto 1sec for all CPUs
+ * to show up in the rendezvous function
+ */
+#define MAX_UCODE_RENDEZVOUS 1000000000 /* nanosec */
+#define SPINUNIT 100 /* 100ns */
+
+/*
+ * Each cpu waits for 1sec max.
+ */
+static int microcode_wait_timedout(int *time_out, void *data)
+{
+ struct ucode_update_param *ucd = data;
+ if (*time_out < SPINUNIT) {
+ pr_err("Not all CPUs entered ucode update handler %d CPUs missed rendezvous\n",
+ (num_online_cpus() - atomic_read(&ucd->enter)));
+ return 1;
+ }
+ *time_out -= SPINUNIT;
+ touch_nmi_watchdog();
+ return 0;
+}
+
+/*
+ * All cpus enter here before a ucode load upto 1 sec.
+ * If not all cpus showed up, we abort the ucode update
+ * and return. ucode update is serialized with the spinlock
+ */
+static int microcode_load_rendezvous(void *data)
+{
+ int cpu = smp_processor_id();
+ struct ucode_update_param *ucd = data;
+ int timeout = MAX_UCODE_RENDEZVOUS;
+ int total_cpus = num_online_cpus();
+
+ /*
+ * Wait for all cpu's to arrive
+ */
+ atomic_dec(&ucd->enter);
+ while(atomic_read(&ucd->enter)) {
+ if (microcode_wait_timedout(&timeout, ucd))
+ return 1;
+ ndelay(SPINUNIT);
+ }
+
+ do_ucode_update(cpu, ucd);
+
+ /*
+ * Wait for all cpu's to complete
+ * ucode update
+ */
+ while (atomic_read(&ucd->count) != total_cpus)
+ cpu_relax();
+ return 0;
+}
+
+/*
+ * If any of the cpu's present are offline, we avoid loading microcode
+ * to the rest of the system. This is simply to avoid having some CPUs with older
+ * microcode. In theory we would update for the upcoming CPU during early_load.
+ * but we want to be *PARANOID* and avoid such situations.
+ * What if some CPUs are offlined and with older microcode. There are two
+ * senarios:
+ * 1: Both CPUs are of a core are offline: We skip the update now, but
+ * If they are onlined later, we will do an update then. Online operations
+ * are serialized, so we will update one thread when the other is still
+ * offline. When the second CPU is onlined, it already has the updated
+ * microcode, since its sibling was updated earlier.
+ * 2: One CPU of the core is offline when we did the update. This is not
+ * a problem, since when we online the sibling it already has a copy
+ * since its sibling was already updated.
+ */
+static int check_online_cpus(void)
+{
+ if (cpumask_equal(cpu_online_mask, cpu_present_mask))
+ return 0;
+ pr_err("Not all CPUs online, please online all CPUs before reloading microcode\n");
+ return 1;
+}
+
+/*
+ * when loading microcode, its important for the HT sibling to be
+ * idle, otherwise there can be some bad interaction between the sibling
+ * executing code and the microcode update process on its thread sibling.
+ * To make this less * complicated we simply park all the cpus with a
+ * stop_machine(). This * gaurantees that all cpus are spinning with
+ * interrupts disabled.
+ * - Wait for all cpus to arrive.
+ * - serialize ucode updates, to avoid 2 cpus from updating ucode at the
+ * same time. This is required since microcode/intel.c also has some static
+ * variables that need protection otherwise. This serves both purposes.
+ * - After updates are completed, wait for all CPUs to complete before
+ * returing back to reload_store.
+ */
+static int perform_microcode_reload(struct ucode_update_param *ucd)
+{
+ int ret;
- ustate = microcode_ops->request_microcode_fw(cpu, µcode_pdev->dev, true);
- if (ustate != UCODE_OK)
- return ustate;
+ memset(ucd, 0, sizeof(struct ucode_update_param));
+ spin_lock_init(&ucd->ucode_lock);
+ atomic_set(&ucd->enter, num_online_cpus());
+ /*
+ * Wait for a 1 sec
+ */
+ ucd->timeout = USEC_PER_SEC;
+ ret = stop_machine(microcode_load_rendezvous, ucd, cpu_online_mask);
- return apply_microcode_on_target(cpu);
+ pr_debug("Total CPUS = %d unable to load on %d CPUs\n",
+ atomic_read(&ucd->count), atomic_read(&ucd->errors));
+
+ if (!(ret || atomic_read(&ucd->errors)))
+ return 0;
+
+ pr_warn("Update failed for %d CPUs\n", atomic_read(&ucd->errors));
+ return 1;
+}
+
+/*
+ * loads microcode files for all cpus.
+ * TBD: We load for each cpu which is useful
+ * if we support hetero cores. We really don't yet
+ * support hetero, so we could optimize this in future to load
+ * just for 1 cpu and reuse the same image for other cpus.
+ */
+static int reload_microcode_files(void)
+{
+ int cpu, ret = 0;
+
+ /*
+ * First load the microcode file for all cpu's
+ */
+ for_each_online_cpu(cpu) {
+ ret = microcode_ops->request_microcode_fw(cpu,
+ µcode_pdev->dev, true);
+ if (ret > UCODE_NFOUND) {
+ pr_warn("Error reloading microcode file for CPU %d\n", cpu);
+
+ /*
+ * set retval for the first encountered reload error
+ * stop further processing of ucode loads.
+ */
+ if (!ret)
+ ret = -EINVAL;
+ break;
+ }
+ }
+ return ret;
}
static ssize_t reload_store(struct device *dev,
struct device_attribute *attr,
const char *buf, size_t size)
{
- enum ucode_state tmp_ret = UCODE_OK;
- bool do_callback = false;
unsigned long val;
ssize_t ret = 0;
- int cpu;
ret = kstrtoul(buf, 0, &val);
if (ret)
@@ -523,26 +682,24 @@ static ssize_t reload_store(struct device *dev,
get_online_cpus();
mutex_lock(µcode_mutex);
- for_each_online_cpu(cpu) {
- tmp_ret = reload_for_cpu(cpu);
- if (tmp_ret > UCODE_NFOUND) {
- pr_warn("Error reloading microcode on CPU %d\n", cpu);
- /* set retval for the first encountered reload error */
- if (!ret)
- ret = -EINVAL;
- }
+ ret = check_online_cpus();
+ if (ret)
+ goto fail;
- if (tmp_ret == UCODE_UPDATED)
- do_callback = true;
- }
+ ret = reload_microcode_files();
+ if (ret)
+ goto fail;
+
+ pr_debug("Done loading microcode file for all CPUs\n");
- if (!ret && do_callback)
+ ret = perform_microcode_reload(&uc_data);
+ if (!ret)
microcode_check();
+fail:
mutex_unlock(µcode_mutex);
put_online_cpus();
-
if (!ret)
ret = size;
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 50e48c4..3d55f0b 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -833,6 +833,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
/* write microcode via MSR 0x79 */
wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
+ pr_debug("ucode loading for cpu %d done\n", cpu);
rev = intel_get_microcode_revision();
--
2.7.4
Microcode updates can be safer if the caches are clean.
Some of the issues around in certain Broadwell parts
can be addressed by doing a full cache flush.
Signed-off-by: Ashok Raj <[email protected]>
Cc: X86 ML <[email protected]>
Cc: LKML <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Tony Luck <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Boris Petkov <[email protected]>
Cc: Tom Lendacky <[email protected]>
Cc: Arjan Van De Ven <[email protected]>
---
arch/x86/kernel/cpu/microcode/intel.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 137c9f5..50e48c4 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -601,6 +601,13 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
return UCODE_OK;
}
+ /*
+ * Microcode updates can be safer if the caches are clean.
+ * Some of the issues around in certain Broadwell parts
+ * can be addressed by doing a full cache flush.
+ */
+ native_wbinvd();
+
/* write microcode via MSR 0x79 */
native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
@@ -817,6 +824,13 @@ static enum ucode_state apply_microcode_intel(int cpu)
return UCODE_OK;
}
+ /*
+ * Microcode updates can be safer if the caches are clean.
+ * Some of the issues around in certain Broadwell parts
+ * can be addressed by doing a full cache flush.
+ */
+ wbinvd();
+
/* write microcode via MSR 0x79 */
wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
--
2.7.4
On Wed, Feb 21, 2018 at 10:33:23PM -0800, Ashok Raj wrote:
> After updating microcode on one of the threads in the core, the
> thread sibling automatically gets the update since the microcode
> resources are shared. Check the ucode revision on the CPU before
> performing a ucode update.
>
> Signed-off-by: Ashok Raj <[email protected]>
> Cc: X86 ML <[email protected]>
> Cc: LKML <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Boris Petkov <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Arjan Van De Ven <[email protected]>
>
> Updates:
> v2: Address Boris's to cleanup apply_microcode_intel
> v3: Fixups per Ingo: Spell Checks
That changelog...
> ---
... comes under this line so that it can be ignored by tools.
> arch/x86/kernel/cpu/microcode/intel.c | 28 +++++++++++++++++++++++++---
> 1 file changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 09b95a7..137c9f5 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -589,6 +589,18 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
> if (!mc)
> return 0;
>
> + rev = intel_get_microcode_revision();
Ok, so I'm still wondering what this patch is trying to achieve.
intel_get_microcode_revision() does already *two* MSR reads and a CPUID.
So it is not speed improvements - it actually makes loading slower due to that
checking.
So why are we doing this again?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
On Wed, Feb 21, 2018 at 10:33:25PM -0800, Ashok Raj wrote:
> Microcode updates during OS load always assumed the other hyper-thread
> was "quiet", but Linux never really did this. We've recently received
> several issues on this, where things did not go well at scale
> deployments, and the Intel microcode team asked us to make sure the
> system is in a quiet state during these updates. Such updates are
> rare events, so we use stop_machine() to ensure the whole system is
> quiet.
>
> The most robust solution is to have all the CPUs wait in
> stop_machine() rendezvous before ucode update, and ensure we stay there
> until all the CPUs have completed the microcode load. stop_machine ensures
> that interrupts are disabled while waiting, and a spin_lock serializes
> that only 1 cpu can perform the update.
>
> Remember microcode resources are shared between the HT siblings of the same
> core. So updating microcode on one thread automatically is good enough for the
> sibling thread. Microcode update ensures that the version being updated is
> greater than what is reported by the CPU. This ensures we do not perform a
> redundant load on the HT sibling when one isn't necessary.
>
> During early boot, we bring up one CPU at a time. Hence we only end up
> loading for one of the sibling thread and subsequent sibling would already have
> the latest copy of the microcode.
>
> Signed-off-by: Ashok Raj <[email protected]>
> Cc: X86 ML <[email protected]>
> Cc: LKML <[email protected]>
> Cc: Tom Lendacky <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Tony Luck <[email protected]>
> Cc: Andi Kleen <[email protected]>
> Cc: Boris Petkov <[email protected]>
> Cc: Arjan Van De Ven <[email protected]>
>
> Changes from V1:
> - Check for return code of stop_machine
> - When microcode file load fails, stop on first error.
> - If any of the present CPUs are offline, then stop reload.
> - Added more comments in commitlog and inline in file.
> - split some functionality from reload_store() per Boris's comments.
>
> What's not done from review: TBD:
> - Load microcode file only once.
> - Removing ucd->errors. (Gives a count of failed loads)
> ---
> arch/x86/kernel/cpu/microcode/core.c | 207 ++++++++++++++++++++++++++++++----
> arch/x86/kernel/cpu/microcode/intel.c | 1 +
> 2 files changed, 183 insertions(+), 25 deletions(-)
Please integrate scripts/checkpatch.pl into your patch creation
workflow. Some of the warnings/errors *actually* make sense.
> diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
> index aa1b9a4..20d2cbb 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -31,6 +31,10 @@
> #include <linux/cpu.h>
> #include <linux/fs.h>
> #include <linux/mm.h>
> +#include <linux/nmi.h>
> +#include <linux/stop_machine.h>
> +#include <linux/delay.h>
> +#include <linux/topology.h>
>
> #include <asm/microcode_intel.h>
> #include <asm/cpu_device_id.h>
> @@ -489,30 +493,185 @@ static void __exit microcode_dev_exit(void)
> /* fake device for request_firmware */
> static struct platform_device *microcode_pdev;
>
> -static enum ucode_state reload_for_cpu(int cpu)
> +static struct ucode_update_param {
> + spinlock_t ucode_lock; /* Serialize microcode updates */
> + atomic_t count; /* # of CPUs that attempt to load ucode */
> + atomic_t errors; /* # of CPUs ucode load failed */
> + atomic_t enter; /* ucode rendezvous count */
> + int timeout; /* Timeout to wait for all cpus to enter */
> +} uc_data;
> +
> +static int do_ucode_update(int cpu, struct ucode_update_param *ucd)
> {
> - struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
> - enum ucode_state ustate;
> + enum ucode_state retval = 0;
>
> - if (!uci->valid)
> - return UCODE_OK;
> + spin_lock(&ucd->ucode_lock);
> + retval = microcode_ops->apply_microcode(cpu);
> + spin_unlock(&ucd->ucode_lock);
> + if (retval > UCODE_NFOUND) {
> + atomic_inc(&ucd->errors);
> + pr_warn("microcode update to CPU %d failed\n", cpu);
> + }
> + atomic_inc(&ucd->count);
> +
> + return retval;
> +}
> +
> +/*
> + * Wait for upto 1sec for all CPUs
> + * to show up in the rendezvous function
> + */
> +#define MAX_UCODE_RENDEZVOUS 1000000000 /* nanosec */
Please incorporate all review comments before sending the next version.
I already commented here:
1 * NSEC_PER_SEC
I don't want to be pointing out the same issues twice.
> +#define SPINUNIT 100 /* 100ns */
> +
> +/*
> + * Each cpu waits for 1sec max.
> + */
> +static int microcode_wait_timedout(int *time_out, void *data)
> +{
> + struct ucode_update_param *ucd = data;
> + if (*time_out < SPINUNIT) {
> + pr_err("Not all CPUs entered ucode update handler %d CPUs missed rendezvous\n",
> + (num_online_cpus() - atomic_read(&ucd->enter)));
> + return 1;
> + }
> + *time_out -= SPINUNIT;
> + touch_nmi_watchdog();
> + return 0;
> +}
> +
> +/*
> + * All cpus enter here before a ucode load upto 1 sec.
Ok, I think Ingo and I already pointed out that the proper spelling
is "CPU" not "cpu" or "cpu's" as I see it somewhere else *still* in
this patch. And I asked you already yesterday to go over everything and
correct that. But you're still sending me half-baked stuff. Are you
trying to make me ignore your patches?
...
> +/*
> + * loads microcode files for all cpus.
> + * TBD: We load for each cpu which is useful
> + * if we support hetero cores. We really don't yet
> + * support hetero, so we could optimize this in future to load
Let's just forget the heterogeneous cores. That's just nuts.
> + * just for 1 cpu and reuse the same image for other cpus.
> + */
> +static int reload_microcode_files(void)
> +{
> + int cpu, ret = 0;
> +
> + /*
> + * First load the microcode file for all cpu's
> + */
> + for_each_online_cpu(cpu) {
> + ret = microcode_ops->request_microcode_fw(cpu,
> + µcode_pdev->dev, true);
So looking at this more:
->request_microcode_fw() calls generic_load_microcode() which ends up saving the
microcode patch for early loading: save_mc_for_early() -> save_microcode_patch()
so the new patch is in the microcode cache.
Now, apply_microcode_intel() looks for a newer patch in the cache so you
should be able to do ->request_microcode_fw() only once on CPU 0 and
then the patch is there.
So I *think* you don't need that reload_microcode_files() dance but
you'll have to try it.
> + if (ret > UCODE_NFOUND) {
> + pr_warn("Error reloading microcode file for CPU %d\n", cpu);
> +
> + /*
> + * set retval for the first encountered reload error
> + * stop further processing of ucode loads.
> + */
> + if (!ret)
> + ret = -EINVAL;
> + break;
> + }
> + }
> + return ret;
Also, I'd like you to split this patch as it is becoming too big and
unwieldy to review. Split it in logical steps which contain one logical
change and commit message explains clearly what it is.
Thanks.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
On Thu, Feb 22, 2018 at 12:00:57PM +0100, Borislav Petkov wrote:
> >
> > Updates:
> > v2: Address Boris's to cleanup apply_microcode_intel
> > v3: Fixups per Ingo: Spell Checks
>
> That changelog...
>
> > ---
>
> ... comes under this line so that it can be ignored by tools.
Oops! using stg, and try to not remember too many things and write it up here.
Will remember to move things next time around.
>
> > arch/x86/kernel/cpu/microcode/intel.c | 28 +++++++++++++++++++++++++---
> > 1 file changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> > index 09b95a7..137c9f5 100644
> > --- a/arch/x86/kernel/cpu/microcode/intel.c
> > +++ b/arch/x86/kernel/cpu/microcode/intel.c
> > @@ -589,6 +589,18 @@ static int apply_microcode_early(struct ucode_cpu_info *uci, bool early)
> > if (!mc)
> > return 0;
> >
> > + rev = intel_get_microcode_revision();
>
> Ok, so I'm still wondering what this patch is trying to achieve.
>
> intel_get_microcode_revision() does already *two* MSR reads and a CPUID.
> So it is not speed improvements - it actually makes loading slower due to that
> checking.
Not a speed improvement.. Hopefully i didn't mislead somewhere :-(
>
> So why are we doing this again?
When you have two HT threads of the same core. Updating microcode on one of the threads
is just good enough. The recommendation for HT siblings is as follows.
Core C0 has two threads C0T0, and C0T1
- Not to simultaneously load microcode on both threads.
- (NEW) not to perform ucode load when the other HT thread is active.
Its safe to spin on a cached variable (like what the patch3 is trying to achieve)
The current code wasn't trying to enforce checking the loaded microcode revision on a thread
before attempting to load the microcode. While you comeback from resume, if C0T0 already
is up, and we loaded the early microcode, then when handling C0T1 there is no need to
do a wrmsrl to reapply microcode since its already loaded as part of C0T0.
This way we don't need any quiese of C0T0 while bringing up C0T1. We need to skip
the loading process avoid doing the ucode load in this case.
Hope this helps.. and if you need this as part of the file/commit log, we should
add that.
rev = intel_get_microcode_revision();
/*
* Its possible the microcode got updated
* because its sibling update was done earlier.
* Skip the update in that case.
*/
if (rev >= mc->hdr.rev) {
uci->cpu_sig.rev = rev;<----- Maybe we can avoid this in early load?
return UCODE_OK;
}
/*
* Microcode updates can be safer if the caches are clean.
* Some of the issues around in certain Broadwell parts
* can be addressed by doing a full cache flush.
*/
native_wbinvd();
/* write microcode via MSR 0x79 */
native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
On Thu, Feb 22, 2018 at 03:55:54AM -0800, Raj, Ashok wrote:
> The current code wasn't trying to enforce checking the loaded microcode revision on a thread
> before attempting to load the microcode. While you comeback from resume, if C0T0 already
> is up, and we loaded the early microcode, then when handling C0T1 there is no need to
> do a wrmsrl to reapply microcode since its already loaded as part of C0T0.
And I'm asking exactly this: is it simply "we don't need to do WRMSR" or
"we should not"?
Because avoiding the WRMSR costs more than simply doing it and letting
the HT thread ignore the supplied microcode.
If it is "we don't need to but there's nothing wrong when we do it" then
we don't need this patch. And I'm pretty sure "nothing wrong when we do
it" would be the answer. Otherwise we have bigger problems.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
On Thu, Feb 22, 2018 at 01:15:06PM +0100, Borislav Petkov wrote:
> On Thu, Feb 22, 2018 at 03:55:54AM -0800, Raj, Ashok wrote:
> > The current code wasn't trying to enforce checking the loaded microcode revision on a thread
> > before attempting to load the microcode. While you comeback from resume, if C0T0 already
> > is up, and we loaded the early microcode, then when handling C0T1 there is no need to
> > do a wrmsrl to reapply microcode since its already loaded as part of C0T0.
>
> And I'm asking exactly this: is it simply "we don't need to do WRMSR" or
> "we should not"?
>
> Because avoiding the WRMSR costs more than simply doing it and letting
> the HT thread ignore the supplied microcode.
This isn't a simple WRMSR like others. Microcode engine needs to do
a bunch of validation.
>
> If it is "we don't need to but there's nothing wrong when we do it" then
> we don't need this patch. And I'm pretty sure "nothing wrong when we do
> it" would be the answer. Otherwise we have bigger problems.
In the past the only guidance was to not load microcode at the same time to the
thread siblings of a core. We now have new guidance that the sibling must be
spinning and not doing other things that can introduce instability around loading
microcode.
I think its safer to not load when its not required vs forcing a load and depending
on the microcode interface to not interfere. If the rules change in future we don't
have to adapt again.
On Thu, Feb 22, 2018 at 05:19:18AM -0800, Raj, Ashok wrote:
> This isn't a simple WRMSR like others. Microcode engine needs to do
> a bunch of validation.
So this is slowly starting to resemble a real reason why not to. That
should be part of the code comment.
> We now have new guidance that the sibling must be spinning and not
> doing other things that can introduce instability around loading
> microcode.
Why isn't *this* in the code comment?
> I think its safer...
That's not good enough - it should be:
"It is not safe because... <insert reasons here>".
So that you can justify the overhead.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
On Thu, 22 Feb 2018, Raj, Ashok wrote:
> On Thu, Feb 22, 2018 at 01:15:06PM +0100, Borislav Petkov wrote:
> > On Thu, Feb 22, 2018 at 03:55:54AM -0800, Raj, Ashok wrote:
> > > The current code wasn't trying to enforce checking the loaded microcode revision on a thread
> > > before attempting to load the microcode. While you comeback from resume, if C0T0 already
> > > is up, and we loaded the early microcode, then when handling C0T1 there is no need to
> > > do a wrmsrl to reapply microcode since its already loaded as part of C0T0.
> >
> > And I'm asking exactly this: is it simply "we don't need to do WRMSR" or
> > "we should not"?
> >
> > Because avoiding the WRMSR costs more than simply doing it and letting
> > the HT thread ignore the supplied microcode.
>
> This isn't a simple WRMSR like others. Microcode engine needs to do
> a bunch of validation.
>
> >
> > If it is "we don't need to but there's nothing wrong when we do it" then
> > we don't need this patch. And I'm pretty sure "nothing wrong when we do
> > it" would be the answer. Otherwise we have bigger problems.
>
> In the past the only guidance was to not load microcode at the same time to the
> thread siblings of a core. We now have new guidance that the sibling must be
> spinning and not doing other things that can introduce instability around loading
> microcode.
Document that properly in the Intel SDM, *please*.
While at it, please verify with the microcode teams that the requirement
for 16-byte alignment of the microcode update as present in the Intel
SDM still stands. Linux does not enforce it on the early microcode
update loader when using an initramfs (but userspace can work around
that, and iucode_tool --early-fw does so). If that 16-byte alignment is
important, I could dust off some patches that fix it.
--
Henrique Holschuh
> > In the past the only guidance was to not load microcode at the same time to
> the
> > thread siblings of a core. We now have new guidance that the sibling must be
> > spinning and not doing other things that can introduce instability around
> loading
> > microcode.
>
> Document that properly in the Intel SDM, *please*.
yes we will update the SDM, just that is a much longer process than sending code out.
>
> While at it, please verify with the microcode teams that the requirement
> for 16-byte alignment of the microcode update as present in the Intel
> SDM still stands.
I'd be surprised if it did not.
>Linux does not enforce it on the early microcode
> update loader when using an initramfs (but userspace can work around
> that, and iucode_tool --early-fw does so). If that 16-byte alignment is
> important, I could dust off some patches that fix it.
hmm wonder what those tools do nowadays; Intel publishes the microcode in the linux native format since some time (and the .dat format is about to go away entirely)
On Thu, 22 Feb 2018, Van De Ven, Arjan wrote:
> > > In the past the only guidance was to not load microcode at the same time to
> > the
> > > thread siblings of a core. We now have new guidance that the sibling must be
> > > spinning and not doing other things that can introduce instability around
> > loading
> > > microcode.
> >
> > Document that properly in the Intel SDM, *please*.
>
> yes we will update the SDM, just that is a much longer process than sending code out.
>
> >
> > While at it, please verify with the microcode teams that the requirement
> > for 16-byte alignment of the microcode update as present in the Intel
> > SDM still stands.
>
> I'd be surprised if it did not.
The question would be better phrased as: important to which processors?
Just very old ones?
The late microcode update loader has it 16-byte aligned as a side-effect
of malloc() from what I recall when I tested it with SLUB, SLAB (I am
not sure about what the result was for SLOB), so it is an issue that was
introduced with the early initramfs microcode loader support.
However, the early loader doesn't have that benefit for the BSP. If the
16-byte alignment were important for most stuff since the Core2, we
should be crashing like crazy when attempting early microcode updates...
Thus, apparently, 4-byte alignment (which we do always get out of the
early initramfs) is good enough on most non-ancient processors when
updating the BSP the way Linux does it, since it does work for most
people.
I don't recall what the situation is for the APs for the early loader,
though.
> >Linux does not enforce it on the early microcode update loader when
> > using an initramfs (but userspace can work around that, and
> > iucode_tool --early-fw does so). If that 16-byte alignment is
> > important, I could dust off some patches that fix it.
>
> hmm wonder what those tools do nowadays; Intel publishes the microcode
> in the linux native format since some time (and the .dat format is
> about to go away entirely)
I think most of them don't force the alignment in userspace. The
exceptions are Debian, Ubuntu, and their derivatives when using
initramfs-tools (not dracut).
I do belive there are a few that generate /boot/ucode.initrd to load as
the first initrd via the bootloader using iucode_tool --write-earlyfw
(instead of using cpio). That would also have the workaround in place.
--
Henrique Holschuh