2009-04-20 20:17:19

by Dmitry Adamushko

Subject: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

From: Dmitry Adamushko <[email protected]>
Subject: x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Solve an issue described in 6f66cbc63081fd70e3191b4dbb796746780e5ae1
in a different way, without resorting to set_cpus_allowed();

* in fact, only collect_cpu_info and apply_microcode callbacks require to be run
on a target cpu, others will do just fine on any other cpu.

We impose this requirement and update all the relevant places;

* cleanup of the synchronization logic of the 'microcode_core' part

The generic 'microcode_core' part guarantees that only a single cpu is being
updated at any particular moment of time.

In general, there is no need for any additional sync. logic in arch-specific parts
(the patch removes existing spinlocks).

See also the "Synchronization" section in microcode_core.c.

- don't call sysfs_remove_group() from mc_sysdev_add() in case when a proper
ucode was not found/loaded.

In general, I think that we shouldn't treat this case as an error.
Say, what if a user upgrades manually via /dev/microcode later on?
I'd say let's keep sysfs files around.

This also avoids warnings being triggered in some cases (there were a few reports on lkml).
e.g. http://lkml.org/lkml/2009/4/16/453 and http://marc.info/?t=123964721700008&r=1&w=2

* some minor cleanups


Remarks:

* perhaps, error-handling need to be made more consistent;

* BUG_ON() in collect_cpu_info_local() and apply_microcode_local()
are signs of paranoia. Remove them?
This would also avoid the need for 'cpu' member in struct collect_for_cpu and remove struct apply_for_cpu
altogether.
The reason why apply_for_cpu is there at all, is that gcc spews warnings for void* <-> int translations
on my setup. Alternatively, define 'cpu' as long? (long <-> int or also re-define 'cpu' in callbacks'
definitions - not good).


Signed-off-by: Dmitry Adamushko <[email protected]>
CC: Rusty Russell <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Andreas Herrmann <[email protected]>
CC: Peter Oruba <[email protected]>
CC: Arjan van de Ven <[email protected]>
CC: Hugh Dickins <[email protected]>


diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index c882664..0389381 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -13,10 +13,16 @@ struct microcode_ops {
int (*request_microcode_user) (int cpu, const void __user *buf, size_t size);
int (*request_microcode_fw) (int cpu, struct device *device);

- void (*apply_microcode) (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
+ * are being called.
+ * See also the "Synchronization" section in microcode_core.c.
+ */
+ void (*apply_microcode) (int cpu);
int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
- void (*microcode_fini_cpu) (int cpu);
};

struct ucode_cpu_info {
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 453b579..0900d9c 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -17,7 +17,6 @@
#include <linux/capability.h>
#include <linux/miscdevice.h>
#include <linux/firmware.h>
-#include <linux/spinlock.h>
#include <linux/cpumask.h>
#include <linux/pci_ids.h>
#include <linux/uaccess.h>
@@ -79,9 +78,6 @@ struct microcode_amd {
#define UCODE_CONTAINER_SECTION_HDR 8
#define UCODE_CONTAINER_HEADER_SIZE 12

-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static struct equiv_cpu_entry *equiv_cpu_table;

static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -146,7 +142,6 @@ static int get_matching_microcode(int cpu, void *mc, int rev)

static void apply_microcode_amd(int cpu)
{
- unsigned long flags;
u32 rev, dummy;
int cpu_num = raw_smp_processor_id();
struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
@@ -158,11 +153,9 @@ static void apply_microcode_amd(int cpu)
if (mc_amd == NULL)
return;

- spin_lock_irqsave(&microcode_update_lock, flags);
wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
/* get patch id after patching */
rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

/* check current patch id and patch's id for match */
if (rev != mc_amd->hdr.patch_id) {
@@ -327,9 +320,6 @@ static int request_microcode_fw(int cpu, struct device *device)
const struct firmware *firmware;
int ret;

- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
ret = request_firmware(&firmware, fw_name, device);
if (ret) {
printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 2e0eb41..9ee151f 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -101,36 +101,97 @@ MODULE_LICENSE("GPL");

static struct microcode_ops *microcode_ops;

-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ * the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
static DEFINE_MUTEX(microcode_mutex);

struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
EXPORT_SYMBOL_GPL(ucode_cpu_info);

+/*
+ * Operations that are run on a target cpu:
+ */
+
+struct collect_for_cpu {
+ struct cpu_signature *cpu_sig;
+ int cpu;
+};
+
+static long collect_cpu_info_local(void *arg)
+{
+ struct collect_for_cpu *cfc = arg;
+
+ BUG_ON(cfc->cpu != raw_smp_processor_id());
+
+ return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+ struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
+
+ return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
+}
+
+static void collect_cpu_info(int cpu)
+{
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+ memset(uci, 0, sizeof(*uci));
+ if (!collect_cpu_info_on_target(cpu, &uci->cpu_sig))
+ uci->valid = 1;
+}
+
+struct apply_for_cpu {
+ int cpu;
+};
+
+static long apply_microcode_local(void *arg)
+{
+ struct apply_for_cpu *afc = arg;
+
+ BUG_ON(afc->cpu != raw_smp_processor_id());
+ microcode_ops->apply_microcode(afc->cpu);
+
+ return 0;
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+ struct apply_for_cpu afc = { .cpu = cpu };
+
+ return work_on_cpu(cpu, apply_microcode_local, &afc);
+}
+
#ifdef CONFIG_MICROCODE_OLD_INTERFACE
static int do_microcode_update(const void __user *buf, size_t size)
{
- cpumask_t old;
int error = 0;
int cpu;

- old = current->cpus_allowed;
-
for_each_online_cpu(cpu) {
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!uci->valid)
continue;

- set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
error = microcode_ops->request_microcode_user(cpu, buf, size);
if (error < 0)
- goto out;
+ break;
if (!error)
- microcode_ops->apply_microcode(cpu);
+ apply_microcode_on_target(cpu);
}
-out:
- set_cpus_allowed_ptr(current, &old);
+
return error;
}

@@ -205,17 +266,16 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
/* fake device for request_firmware */
static struct platform_device *microcode_pdev;

-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
int err = 0;

mutex_lock(&microcode_mutex);
if (uci->valid) {
- err = microcode_ops->request_microcode_fw(smp_processor_id(),
- &microcode_pdev->dev);
+ err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
+ apply_microcode_on_target(cpu);
}
mutex_unlock(&microcode_mutex);
return err;
@@ -235,7 +295,7 @@ static ssize_t reload_store(struct sys_device *dev,
if (val == 1) {
get_online_cpus();
if (cpu_online(cpu))
- err = work_on_cpu(cpu, reload_for_cpu, NULL);
+ err = reload_for_cpu(cpu);
put_online_cpus();
}
if (err)
@@ -275,7 +335,7 @@ static struct attribute_group mc_attr_group = {
.name = "microcode",
};

-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

@@ -283,85 +343,44 @@ static void __microcode_fini_cpu(int cpu)
uci->valid = 0;
}

-static void microcode_fini_cpu(int cpu)
-{
- mutex_lock(&microcode_mutex);
- __microcode_fini_cpu(cpu);
- mutex_unlock(&microcode_mutex);
-}
-
-static void collect_cpu_info(int cpu)
+static void microcode_resume_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- memset(uci, 0, sizeof(*uci));
- if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
- uci->valid = 1;
+ if (!uci->valid || !uci->mc)
+ return;
+
+ pr_debug("microcode: CPU%d updated upon resume\n", cpu);
+ apply_microcode_on_target(cpu);
}

-static int microcode_resume_cpu(int cpu)
+static void microcode_init_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- struct cpu_signature nsig;
+ int err;

- pr_debug("microcode: CPU%d resumed\n", cpu);
+ collect_cpu_info(cpu);

- if (!uci->mc)
- return 1;
+ if (!uci->valid)
+ return;
+ if (system_state != SYSTEM_RUNNING)
+ return;

- /*
- * Let's verify that the 'cached' ucode does belong
- * to this cpu (a bit of paranoia):
- */
- if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "failed to collect_cpu_info for resuming cpu #%d\n",
- cpu);
- return -1;
+ err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
+ if (!err) {
+ pr_debug("microcode: CPU%d updated upon init\n", cpu);
+ apply_microcode_on_target(cpu);
}
-
- if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "cached ucode doesn't match the resuming cpu #%d\n",
- cpu);
- /* Should we look for a new ucode here? */
- return 1;
- }
-
- return 0;
}

-static long microcode_update_cpu(void *unused)
+static void microcode_update_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
- int err = 0;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- /*
- * Check if the system resume is in progress (uci->valid != NULL),
- * otherwise just request a firmware:
- */
- if (uci->valid) {
- err = microcode_resume_cpu(smp_processor_id());
- } else {
- collect_cpu_info(smp_processor_id());
- if (uci->valid && system_state == SYSTEM_RUNNING)
- err = microcode_ops->request_microcode_fw(
- smp_processor_id(),
- &microcode_pdev->dev);
- }
- if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
- return err;
-}
-
-static int microcode_init_cpu(int cpu)
-{
- int err;
- mutex_lock(&microcode_mutex);
- err = work_on_cpu(cpu, microcode_update_cpu, NULL);
- mutex_unlock(&microcode_mutex);
-
- return err;
+ if (uci->valid)
+ microcode_resume_cpu(cpu);
+ else
+ microcode_init_cpu(cpu);
}

static int mc_sysdev_add(struct sys_device *sys_dev)
@@ -379,9 +398,10 @@ static int mc_sysdev_add(struct sys_device *sys_dev)
if (err)
return err;

- err = microcode_init_cpu(cpu);
- if (err)
- sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
+ microcode_init_cpu(cpu);
+ if (!uci->valid)
+ /* can't work with this cpu */
+ err = -1;

return err;
}
@@ -402,12 +422,23 @@ static int mc_sysdev_remove(struct sys_device *sys_dev)
static int mc_sysdev_resume(struct sys_device *dev)
{
int cpu = dev->id;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!cpu_online(cpu))
return 0;

- /* only CPU 0 will apply ucode here */
- microcode_update_cpu(NULL);
+ /*
+ * All non-bootup cpus are still disabled,
+ * so only CPU 0 will apply ucode here.
+ *
+ * Moreover, there can be no concurrent
+ * updates from any other places at this point.
+ */
+ WARN_ON(cpu != 0);
+
+ if (uci->valid && uci->mc)
+ microcode_ops->apply_microcode(cpu);
+
return 0;
}

@@ -427,9 +458,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
switch (action) {
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- if (microcode_init_cpu(cpu))
- printk(KERN_ERR "microcode: failed to init CPU%d\n",
- cpu);
+ microcode_update_cpu(cpu);
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
pr_debug("microcode: CPU%d added\n", cpu);
@@ -471,9 +500,6 @@ static int __init microcode_init(void)
return -ENODEV;
}

- error = microcode_dev_init();
- if (error)
- return error;
microcode_pdev = platform_device_register_simple("microcode", -1,
NULL, 0);
if (IS_ERR(microcode_pdev)) {
@@ -482,14 +508,26 @@ static int __init microcode_init(void)
}

get_online_cpus();
+ /*
+ * --dimm. Hmm, we can avoid it if we perhaps first
+ * try to apply ucode in mc_sysdev_add() and only
+ * then create a sysfs group.
+ */
+ mutex_lock(&microcode_mutex);
error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+ mutex_unlock(&microcode_mutex);
+
put_online_cpus();
+
if (error) {
- microcode_dev_exit();
platform_device_unregister(microcode_pdev);
return error;
}

+ error = microcode_dev_init();
+ if (error)
+ return error;
+
register_hotcpu_notifier(&mc_cpu_notifier);

printk(KERN_INFO
@@ -507,7 +545,9 @@ static void __exit microcode_exit(void)
unregister_hotcpu_notifier(&mc_cpu_notifier);

get_online_cpus();
+ mutex_lock(&microcode_mutex);
sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+ mutex_unlock(&microcode_mutex);
put_online_cpus();

platform_device_unregister(microcode_pdev);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 149b9ec..bc824bb 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -75,7 +75,6 @@
#include <linux/miscdevice.h>
#include <linux/firmware.h>
#include <linux/smp_lock.h>
-#include <linux/spinlock.h>
#include <linux/cpumask.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
@@ -150,13 +149,9 @@ struct extended_sigtable {

#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
- unsigned long flags;
unsigned int val[2];

memset(csig, 0, sizeof(*csig));
@@ -176,15 +171,11 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}

- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
-
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
/* see notes above for revision 1.07. Apparent chip bug */
sync_core();
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
csig->sig, csig->pf, csig->rev);
@@ -322,7 +313,6 @@ static void apply_microcode(int cpu)
{
struct microcode_intel *mc_intel;
struct ucode_cpu_info *uci;
- unsigned long flags;
unsigned int val[2];
int cpu_num;

@@ -336,9 +326,6 @@ static void apply_microcode(int cpu)
if (mc_intel == NULL)
return;

- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
-
/* write microcode via MSR 0x79 */
wrmsr(MSR_IA32_UCODE_WRITE,
(unsigned long) mc_intel->bits,
@@ -351,7 +338,6 @@ static void apply_microcode(int cpu)
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

- spin_unlock_irqrestore(&microcode_update_lock, flags);
if (val[1] != mc_intel->hdr.rev) {
printk(KERN_ERR "microcode: CPU%d update from revision "
"0x%x to 0x%x failed\n",
@@ -445,8 +431,6 @@ static int request_microcode_fw(int cpu, struct device *device)
const struct firmware *firmware;
int ret;

- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
sprintf(name, "intel-ucode/%02x-%02x-%02x",
c->x86, c->x86_model, c->x86_mask);
ret = request_firmware(&firmware, name, device);
@@ -470,9 +454,6 @@ static int get_ucode_user(void *to, const void *from, size_t n)

static int request_microcode_user(int cpu, const void __user *buf, size_t size)
{
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
}

2009-04-21 08:30:42

by Ingo Molnar

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Dmitry Adamushko <[email protected]> wrote:

> From: Dmitry Adamushko <[email protected]>
> Subject: x86 microcode: work_on_cpu and cleanup of the synchronization logic

Note, this does not apply cleanly to latest -git anymore, due to
this fixlet for the warnings:

0917798: x86: fix microcode driver newly spewing warnings

Mind resending with that resolved?

> diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h

( Patch submission nitpick: please always include a diffstat. Thanks! )

Ingo

2009-04-21 20:07:37

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

The version below is based on the latest -git.


From: Dmitry Adamushko <[email protected]>
Subject: x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Solve an issue described in 6f66cbc63081fd70e3191b4dbb796746780e5ae1
in a different way, without resorting to set_cpus_allowed();

* in fact, only collect_cpu_info and apply_microcode callbacks require to be run
on a target cpu, others will do just fine on any other cpu.

We impose this requirement and update all the relevant places;

* cleanup of the synchronization logic of the 'microcode_core' part

The generic 'microcode_core' part guarantees that only a single cpu is being
updated at any particular moment of time.

In general, there is no need for any additional sync. logic in arch-specific parts
(the patch removes existing spinlocks).

See also the "Synchronization" section in microcode_core.c.

* some minor cleanups


Remarks:

* BUG_ON() in collect_cpu_info_local() and apply_microcode_local()
are signs of paranoia. Remove them?

This would also avoid the need for 'cpu' member in struct collect_for_cpu
and remove struct apply_for_cpu altogether.

The reason why apply_for_cpu is there at all, is that gcc spews warnings
for void* <-> int translations on my setup. Alternatively, define 'cpu' as long?
(long <-> int or also re-define 'cpu' in callbacks' definitions -> not nice).


Signed-off-by: Dmitry Adamushko <[email protected]>
CC: Rusty Russell <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Andreas Herrmann <[email protected]>
CC: Peter Oruba <[email protected]>
CC: Arjan van de Ven <[email protected]>
CC: Hugh Dickins <[email protected]>


diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index c882664..0389381 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -13,10 +13,16 @@ struct microcode_ops {
int (*request_microcode_user) (int cpu, const void __user *buf, size_t size);
int (*request_microcode_fw) (int cpu, struct device *device);

- void (*apply_microcode) (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
+ * are being called.
+ * See also the "Synchronization" section in microcode_core.c.
+ */
+ void (*apply_microcode) (int cpu);
int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
- void (*microcode_fini_cpu) (int cpu);
};

struct ucode_cpu_info {
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 453b579..0900d9c 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -17,7 +17,6 @@
#include <linux/capability.h>
#include <linux/miscdevice.h>
#include <linux/firmware.h>
-#include <linux/spinlock.h>
#include <linux/cpumask.h>
#include <linux/pci_ids.h>
#include <linux/uaccess.h>
@@ -79,9 +78,6 @@ struct microcode_amd {
#define UCODE_CONTAINER_SECTION_HDR 8
#define UCODE_CONTAINER_HEADER_SIZE 12

-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static struct equiv_cpu_entry *equiv_cpu_table;

static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -146,7 +142,6 @@ static int get_matching_microcode(int cpu, void *mc, int rev)

static void apply_microcode_amd(int cpu)
{
- unsigned long flags;
u32 rev, dummy;
int cpu_num = raw_smp_processor_id();
struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
@@ -158,11 +153,9 @@ static void apply_microcode_amd(int cpu)
if (mc_amd == NULL)
return;

- spin_lock_irqsave(&microcode_update_lock, flags);
wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
/* get patch id after patching */
rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

/* check current patch id and patch's id for match */
if (rev != mc_amd->hdr.patch_id) {
@@ -327,9 +320,6 @@ static int request_microcode_fw(int cpu, struct device *device)
const struct firmware *firmware;
int ret;

- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
ret = request_firmware(&firmware, fw_name, device);
if (ret) {
printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 98c470c..cddf25e 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -101,36 +101,97 @@ MODULE_LICENSE("GPL");

static struct microcode_ops *microcode_ops;

-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ * the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
static DEFINE_MUTEX(microcode_mutex);

struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
EXPORT_SYMBOL_GPL(ucode_cpu_info);

+/*
+ * Operations that are run on a target cpu:
+ */
+
+struct collect_for_cpu {
+ struct cpu_signature *cpu_sig;
+ int cpu;
+};
+
+static long collect_cpu_info_local(void *arg)
+{
+ struct collect_for_cpu *cfc = arg;
+
+ BUG_ON(cfc->cpu != raw_smp_processor_id());
+
+ return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+ struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
+
+ return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
+}
+
+static void collect_cpu_info(int cpu)
+{
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+ memset(uci, 0, sizeof(*uci));
+ if (!collect_cpu_info_on_target(cpu, &uci->cpu_sig))
+ uci->valid = 1;
+}
+
+struct apply_for_cpu {
+ int cpu;
+};
+
+static long apply_microcode_local(void *arg)
+{
+ struct apply_for_cpu *afc = arg;
+
+ BUG_ON(afc->cpu != raw_smp_processor_id());
+ microcode_ops->apply_microcode(afc->cpu);
+
+ return 0;
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+ struct apply_for_cpu afc = { .cpu = cpu };
+
+ return work_on_cpu(cpu, apply_microcode_local, &afc);
+}
+
#ifdef CONFIG_MICROCODE_OLD_INTERFACE
static int do_microcode_update(const void __user *buf, size_t size)
{
- cpumask_t old;
int error = 0;
int cpu;

- old = current->cpus_allowed;
-
for_each_online_cpu(cpu) {
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!uci->valid)
continue;

- set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
error = microcode_ops->request_microcode_user(cpu, buf, size);
if (error < 0)
- goto out;
+ break;
if (!error)
- microcode_ops->apply_microcode(cpu);
+ apply_microcode_on_target(cpu);
}
-out:
- set_cpus_allowed_ptr(current, &old);
+
return error;
}

@@ -205,17 +266,16 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
/* fake device for request_firmware */
static struct platform_device *microcode_pdev;

-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
int err = 0;

mutex_lock(&microcode_mutex);
if (uci->valid) {
- err = microcode_ops->request_microcode_fw(smp_processor_id(),
- &microcode_pdev->dev);
+ err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
+ apply_microcode_on_target(cpu);
}
mutex_unlock(&microcode_mutex);
return err;
@@ -235,7 +295,7 @@ static ssize_t reload_store(struct sys_device *dev,
if (val == 1) {
get_online_cpus();
if (cpu_online(cpu))
- err = work_on_cpu(cpu, reload_for_cpu, NULL);
+ err = reload_for_cpu(cpu);
put_online_cpus();
}
if (err)
@@ -275,7 +335,7 @@ static struct attribute_group mc_attr_group = {
.name = "microcode",
};

-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

@@ -283,85 +343,44 @@ static void __microcode_fini_cpu(int cpu)
uci->valid = 0;
}

-static void microcode_fini_cpu(int cpu)
-{
- mutex_lock(&microcode_mutex);
- __microcode_fini_cpu(cpu);
- mutex_unlock(&microcode_mutex);
-}
-
-static void collect_cpu_info(int cpu)
+static void microcode_resume_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- memset(uci, 0, sizeof(*uci));
- if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
- uci->valid = 1;
+ if (!uci->valid || !uci->mc)
+ return;
+
+ pr_debug("microcode: CPU%d updated upon resume\n", cpu);
+ apply_microcode_on_target(cpu);
}

-static int microcode_resume_cpu(int cpu)
+static void microcode_init_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- struct cpu_signature nsig;
-
- pr_debug("microcode: CPU%d resumed\n", cpu);
-
- if (!uci->mc)
- return 1;
-
- /*
- * Let's verify that the 'cached' ucode does belong
- * to this cpu (a bit of paranoia):
- */
- if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "failed to collect_cpu_info for resuming cpu #%d\n",
- cpu);
- return -1;
- }
-
- if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "cached ucode doesn't match the resuming cpu #%d\n",
- cpu);
- /* Should we look for a new ucode here? */
- return 1;
- }
+ int err;

- return 0;
-}
+ collect_cpu_info(cpu);

-static long microcode_update_cpu(void *unused)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
- int err = 0;
+ if (!uci->valid)
+ return;
+ if (system_state != SYSTEM_RUNNING)
+ return;

- /*
- * Check if the system resume is in progress (uci->valid != NULL),
- * otherwise just request a firmware:
- */
- if (uci->valid) {
- err = microcode_resume_cpu(smp_processor_id());
- } else {
- collect_cpu_info(smp_processor_id());
- if (uci->valid && system_state == SYSTEM_RUNNING)
- err = microcode_ops->request_microcode_fw(
- smp_processor_id(),
- &microcode_pdev->dev);
+ err = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);
+ if (!err) {
+ pr_debug("microcode: CPU%d updated upon init\n", cpu);
+ apply_microcode_on_target(cpu);
}
- if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
- return err;
}

-static int microcode_init_cpu(int cpu)
+static void microcode_update_cpu(int cpu)
{
- int err;
- mutex_lock(&microcode_mutex);
- err = work_on_cpu(cpu, microcode_update_cpu, NULL);
- mutex_unlock(&microcode_mutex);
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- return err;
+ if (uci->valid)
+ microcode_resume_cpu(cpu);
+ else
+ microcode_init_cpu(cpu);
}

static int mc_sysdev_add(struct sys_device *sys_dev)
@@ -379,7 +398,11 @@ static int mc_sysdev_add(struct sys_device *sys_dev)
if (err)
return err;

- err = microcode_init_cpu(cpu);
+ microcode_init_cpu(cpu);
+ if (!uci->valid) {
+ WARN_ON(1);
+ err = -1
+ }

return err;
}
@@ -400,12 +423,23 @@ static int mc_sysdev_remove(struct sys_device *sys_dev)
static int mc_sysdev_resume(struct sys_device *dev)
{
int cpu = dev->id;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!cpu_online(cpu))
return 0;

- /* only CPU 0 will apply ucode here */
- microcode_update_cpu(NULL);
+ /*
+ * All non-bootup cpus are still disabled,
+ * so only CPU 0 will apply ucode here.
+ *
+ * Moreover, there can be no concurrent
+ * updates from any other places at this point.
+ */
+ WARN_ON(cpu != 0);
+
+ if (uci->valid && uci->mc)
+ microcode_ops->apply_microcode(cpu);
+
return 0;
}

@@ -425,9 +459,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
switch (action) {
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- if (microcode_init_cpu(cpu))
- printk(KERN_ERR "microcode: failed to init CPU%d\n",
- cpu);
+ microcode_update_cpu(cpu);
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
pr_debug("microcode: CPU%d added\n", cpu);
@@ -469,9 +501,6 @@ static int __init microcode_init(void)
return -ENODEV;
}

- error = microcode_dev_init();
- if (error)
- return error;
microcode_pdev = platform_device_register_simple("microcode", -1,
NULL, 0);
if (IS_ERR(microcode_pdev)) {
@@ -480,14 +509,26 @@ static int __init microcode_init(void)
}

get_online_cpus();
+ /*
+ * --dimm. Hmm, we can avoid it if we perhaps first
+ * try to apply ucode in mc_sysdev_add() and only
+ * then create a sysfs group.
+ */
+ mutex_lock(&microcode_mutex);
error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+ mutex_unlock(&microcode_mutex);
+
put_online_cpus();
+
if (error) {
- microcode_dev_exit();
platform_device_unregister(microcode_pdev);
return error;
}

+ error = microcode_dev_init();
+ if (error)
+ return error;
+
register_hotcpu_notifier(&mc_cpu_notifier);

printk(KERN_INFO
@@ -505,7 +546,9 @@ static void __exit microcode_exit(void)
unregister_hotcpu_notifier(&mc_cpu_notifier);

get_online_cpus();
+ mutex_lock(&microcode_mutex);
sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+ mutex_unlock(&microcode_mutex);
put_online_cpus();

platform_device_unregister(microcode_pdev);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 149b9ec..bc824bb 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -75,7 +75,6 @@
#include <linux/miscdevice.h>
#include <linux/firmware.h>
#include <linux/smp_lock.h>
-#include <linux/spinlock.h>
#include <linux/cpumask.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
@@ -150,13 +149,9 @@ struct extended_sigtable {

#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
- unsigned long flags;
unsigned int val[2];

memset(csig, 0, sizeof(*csig));
@@ -176,15 +171,11 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}

- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
-
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
/* see notes above for revision 1.07. Apparent chip bug */
sync_core();
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
csig->sig, csig->pf, csig->rev);
@@ -322,7 +313,6 @@ static void apply_microcode(int cpu)
{
struct microcode_intel *mc_intel;
struct ucode_cpu_info *uci;
- unsigned long flags;
unsigned int val[2];
int cpu_num;

@@ -336,9 +326,6 @@ static void apply_microcode(int cpu)
if (mc_intel == NULL)
return;

- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
-
/* write microcode via MSR 0x79 */
wrmsr(MSR_IA32_UCODE_WRITE,
(unsigned long) mc_intel->bits,
@@ -351,7 +338,6 @@ static void apply_microcode(int cpu)
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

- spin_unlock_irqrestore(&microcode_update_lock, flags);
if (val[1] != mc_intel->hdr.rev) {
printk(KERN_ERR "microcode: CPU%d update from revision "
"0x%x to 0x%x failed\n",
@@ -445,8 +431,6 @@ static int request_microcode_fw(int cpu, struct device *device)
const struct firmware *firmware;
int ret;

- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
sprintf(name, "intel-ucode/%02x-%02x-%02x",
c->x86, c->x86_model, c->x86_mask);
ret = request_firmware(&firmware, name, device);
@@ -470,9 +454,6 @@ static int get_ucode_user(void *to, const void *from, size_t n)

static int request_microcode_user(int cpu, const void __user *buf, size_t size)
{
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
}

2009-04-21 20:09:53

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/21 Dmitry Adamushko <[email protected]>:
>
> The version below is based on the latest -git.

Sorry, forgot to add a diif-stat.

arch/x86/include/asm/microcode.h | 10 ++-
arch/x86/kernel/microcode_amd.c | 10 --
arch/x86/kernel/microcode_core.c | 223 ++++++++++++++++++++++---------------
arch/x86/kernel/microcode_intel.c | 19 ---
4 files changed, 141 insertions(+), 121 deletions(-)

There are about 20 new comment lines.


--
Best regards,
Dmitry Adamushko

2009-04-22 10:18:55

by Ingo Molnar

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Dmitry Adamushko <[email protected]> wrote:

> +static long collect_cpu_info_local(void *arg)
> +{
> + struct collect_for_cpu *cfc = arg;
> +
> + BUG_ON(cfc->cpu != raw_smp_processor_id());
> +
> + return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
> +}
> +
> +static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
> +{
> + struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
> +
> + return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
> +}

Couldnt this be done without work_on_cpu(), by using
smp_call_function()?

Ingo

2009-04-22 10:33:22

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/22 Ingo Molnar <[email protected]>:
>
> * Dmitry Adamushko <[email protected]> wrote:
>
>> +static long collect_cpu_info_local(void *arg)
>> +{
>> + struct collect_for_cpu *cfc = arg;
>> +
>> + BUG_ON(cfc->cpu != raw_smp_processor_id());
>> +
>> + return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
>> +}
>> +
>> +static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
>> +{
>> + struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
>> +
>> + return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
>> +}
>
> Couldnt this be done without work_on_cpu(), by using
> smp_call_function()?

It should be definitely possible. Will send an updated version.

p.s. grrr... this idea should have come into my mind in the first place :-/


>
> Ingo
>

--
Best regards,
Dmitry Adamushko

2009-04-22 10:34:28

by Ingo Molnar

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Dmitry Adamushko <[email protected]> wrote:

> + microcode_init_cpu(cpu);
> + if (!uci->valid) {
> + WARN_ON(1);
> + err = -1

ahem :-)

Ingo

2009-04-22 10:36:55

by Ingo Molnar

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Dmitry Adamushko <[email protected]> wrote:

> 2009/4/22 Ingo Molnar <[email protected]>:
> >
> > * Dmitry Adamushko <[email protected]> wrote:
> >
> >> +static long collect_cpu_info_local(void *arg)
> >> +{
> >> + struct collect_for_cpu *cfc = arg;
> >> +
> >> + BUG_ON(cfc->cpu != raw_smp_processor_id());
> >> +
> >> + return microcode_ops->collect_cpu_info(cfc->cpu, cfc->cpu_sig);
> >> +}
> >> +
> >> +static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
> >> +{
> >> + struct collect_for_cpu cfc = { .cpu_sig = cpu_sig, .cpu = cpu };
> >> +
> >> + return work_on_cpu(cpu, collect_cpu_info_local, &cfc);
> >> +}
> >
> > Couldnt this be done without work_on_cpu(), by using
> > smp_call_function()?
>
> It should be definitely possible. Will send an updated version.
>
> p.s. grrr... this idea should have come into my mind in the first place :-/

well you made all the hard work of tightening all the locking to
make this all real easy, right?

Ingo

2009-04-22 22:24:49

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

Version 3

diff with v.2

- use smp_call_function_single() instead of work_on_cpu() as suggested by Ingo;
- add 'enum ucode_state' as return value of request_microcode_{fw,user}
- minor cleanups


From: Dmitry Adamushko <[email protected]>

Subject: x86 microcode: smp_call_function_single() and cleanup of synchronization logic

* Solve an issue described in 6f66cbc63081fd70e3191b4dbb796746780e5ae1
in a different way, without resorting to set_cpus_allowed();

* in fact, only collect_cpu_info and apply_microcode callbacks must run
on a target cpu, others will do just fine on any other.
smp_call_function_single() is used to run these callbacks on a target cpu.

* cleanup of synchronization logic of the 'microcode_core' part

The generic 'microcode_core' part guarantees that only a single cpu is being
updated at any particular moment of time.

In general, there is no need for any additional sync. mechanism in arch-specific parts
(the patch removes existing spinlocks).

See also the "Synchronization" section in microcode_core.c.

* use 'enum ucode_state' as return value of request_microcode_{fw,user} to
gain more flexibility by distinguishing between real error cases and situations when an appropriate
ucode was not found (which is not an error per-se).

* some minor cleanups


arch/x86/include/asm/microcode.h | 19 ++-
arch/x86/kernel/microcode_amd.c | 58 +++-----
arch/x86/kernel/microcode_core.c | 261 ++++++++++++++++++++++---------------
arch/x86/kernel/microcode_intel.c | 74 ++++-------
4 files changed, 220 insertions(+), 192 deletions(-)

(some part of '+' is taken by comments)



Signed-off-by: Dmitry Adamushko <[email protected]>
CC: Rusty Russell <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Andreas Herrmann <[email protected]>
CC: Peter Oruba <[email protected]>
CC: Arjan van de Ven <[email protected]>
CC: Hugh Dickins <[email protected]>

diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index c882664..251620b 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -9,14 +9,25 @@ struct cpu_signature {

struct device;

+enum ucode_state { UCODE_ERROR, UCODE_OK, UCODE_NFOUND };
+
struct microcode_ops {
- int (*request_microcode_user) (int cpu, const void __user *buf, size_t size);
- int (*request_microcode_fw) (int cpu, struct device *device);
+ enum ucode_state (*request_microcode_user) (int cpu,
+ const void __user *buf, size_t size);

- void (*apply_microcode) (int cpu);
+ enum ucode_state (*request_microcode_fw) (int cpu,
+ struct device *device);

- int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
void (*microcode_fini_cpu) (int cpu);
+
+ /*
+ * The generic 'microcode_core' part guarantees that
+ * the callbacks below run on a target cpu when they
+ * are being called.
+ * See also the "Synchronization" section in microcode_core.c.
+ */
+ int (*apply_microcode) (int cpu);
+ int (*collect_cpu_info) (int cpu, struct cpu_signature *csig);
};

struct ucode_cpu_info {
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 453b579..c8be20f 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -13,25 +13,13 @@
* Licensed under the terms of the GNU General Public
* License version 2. See file COPYING for details.
*/
-#include <linux/platform_device.h>
-#include <linux/capability.h>
-#include <linux/miscdevice.h>
#include <linux/firmware.h>
-#include <linux/spinlock.h>
-#include <linux/cpumask.h>
#include <linux/pci_ids.h>
#include <linux/uaccess.h>
#include <linux/vmalloc.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/sched.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/cpu.h>
#include <linux/pci.h>
-#include <linux/fs.h>
-#include <linux/mm.h>

#include <asm/microcode.h>
#include <asm/processor.h>
@@ -79,9 +67,6 @@ struct microcode_amd {
#define UCODE_CONTAINER_SECTION_HDR 8
#define UCODE_CONTAINER_HEADER_SIZE 12

-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static struct equiv_cpu_entry *equiv_cpu_table;

static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
@@ -144,9 +129,8 @@ static int get_matching_microcode(int cpu, void *mc, int rev)
return 1;
}

-static void apply_microcode_amd(int cpu)
+static int apply_microcode_amd(int cpu)
{
- unsigned long flags;
u32 rev, dummy;
int cpu_num = raw_smp_processor_id();
struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
@@ -156,25 +140,25 @@ static void apply_microcode_amd(int cpu)
BUG_ON(cpu_num != cpu);

if (mc_amd == NULL)
- return;
+ return 0;

- spin_lock_irqsave(&microcode_update_lock, flags);
wrmsrl(MSR_AMD64_PATCH_LOADER, (u64)(long)&mc_amd->hdr.data_code);
/* get patch id after patching */
rdmsr(MSR_AMD64_PATCH_LEVEL, rev, dummy);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

/* check current patch id and patch's id for match */
if (rev != mc_amd->hdr.patch_id) {
printk(KERN_ERR "microcode: CPU%d: update failed "
"(for patch_level=0x%x)\n", cpu, mc_amd->hdr.patch_id);
- return;
+ return -1;
}

printk(KERN_INFO "microcode: CPU%d: updated (new patch_level=0x%x)\n",
cpu, rev);

uci->cpu_sig.rev = rev;
+
+ return 0;
}

static int get_ucode_data(void *to, const u8 *from, size_t n)
@@ -263,7 +247,8 @@ static void free_equiv_cpu_table(void)
}
}

-static int generic_load_microcode(int cpu, const u8 *data, size_t size)
+static enum ucode_state
+generic_load_microcode(int cpu, const u8 *data, size_t size)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
const u8 *ucode_ptr = data;
@@ -272,12 +257,13 @@ static int generic_load_microcode(int cpu, const u8 *data, size_t size)
int new_rev = uci->cpu_sig.rev;
unsigned int leftover;
unsigned long offset;
+ enum ucode_state state = UCODE_OK;

offset = install_equiv_cpu_table(ucode_ptr);
if (!offset) {
printk(KERN_ERR "microcode: failed to create "
"equivalent cpu table\n");
- return -EINVAL;
+ return UCODE_ERROR;
}

ucode_ptr += offset;
@@ -312,28 +298,27 @@ static int generic_load_microcode(int cpu, const u8 *data, size_t size)
pr_debug("microcode: CPU%d found a matching microcode "
"update with version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
- } else
+ } else {
vfree(new_mc);
- }
+ state = UCODE_ERROR;
+ }
+ } else
+ state = UCODE_NFOUND;

free_equiv_cpu_table();

- return (int)leftover;
+ return state;
}

-static int request_microcode_fw(int cpu, struct device *device)
+static enum ucode_state request_microcode_fw(int cpu, struct device *device)
{
const char *fw_name = "amd-ucode/microcode_amd.bin";
const struct firmware *firmware;
- int ret;
-
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
+ enum ucode_state ret;

- ret = request_firmware(&firmware, fw_name, device);
- if (ret) {
+ if (request_firmware(&firmware, fw_name, device)) {
printk(KERN_ERR "microcode: failed to load file %s\n", fw_name);
- return ret;
+ return UCODE_NFOUND;
}

ret = generic_load_microcode(cpu, firmware->data, firmware->size);
@@ -343,11 +328,12 @@ static int request_microcode_fw(int cpu, struct device *device)
return ret;
}

-static int request_microcode_user(int cpu, const void __user *buf, size_t size)
+static enum ucode_state
+request_microcode_user(int cpu, const void __user *buf, size_t size)
{
printk(KERN_INFO "microcode: AMD microcode update via "
"/dev/cpu/microcode not supported\n");
- return -1;
+ return UCODE_ERROR;
}

static void microcode_fini_cpu_amd(int cpu)
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 98c470c..f43794b 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -71,27 +71,17 @@
* Thanks to Stuart Swales for pointing out this bug.
*/
#include <linux/platform_device.h>
-#include <linux/capability.h>
#include <linux/miscdevice.h>
-#include <linux/firmware.h>
#include <linux/smp_lock.h>
-#include <linux/spinlock.h>
-#include <linux/cpumask.h>
-#include <linux/uaccess.h>
-#include <linux/vmalloc.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/mutex.h>
-#include <linux/sched.h>
-#include <linux/init.h>
-#include <linux/slab.h>
#include <linux/cpu.h>
#include <linux/fs.h>
#include <linux/mm.h>

#include <asm/microcode.h>
#include <asm/processor.h>
-#include <asm/msr.h>

MODULE_DESCRIPTION("Microcode Update Driver");
MODULE_AUTHOR("Tigran Aivazian <[email protected]>");
@@ -101,36 +91,110 @@ MODULE_LICENSE("GPL");

static struct microcode_ops *microcode_ops;

-/* no concurrent ->write()s are allowed on /dev/cpu/microcode */
+/*
+ * Synchronization.
+ *
+ * All non cpu-hotplug-callback call sites use:
+ *
+ * - microcode_mutex to synchronize with each other;
+ * - get/put_online_cpus() to synchronize with
+ * the cpu-hotplug-callback call sites.
+ *
+ * We guarantee that only a single cpu is being
+ * updated at any particular moment of time.
+ */
static DEFINE_MUTEX(microcode_mutex);

struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
EXPORT_SYMBOL_GPL(ucode_cpu_info);

+/*
+ * Operations that are run on a target cpu:
+ */
+
+struct cpu_info_ctx {
+ struct cpu_signature *cpu_sig;
+ int err;
+};
+
+static void collect_cpu_info_local(void *arg)
+{
+ struct cpu_info_ctx *ctx = arg;
+
+ ctx->err = microcode_ops->collect_cpu_info(smp_processor_id(),
+ ctx->cpu_sig);
+}
+
+static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
+{
+ struct cpu_info_ctx ctx = { .cpu_sig = cpu_sig, .err = 0 };
+ int ret;
+
+ ret = smp_call_function_single(cpu, collect_cpu_info_local, &ctx, 1);
+ if (!ret)
+ ret = ctx.err;
+
+ return ret;
+}
+
+static int collect_cpu_info(int cpu)
+{
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ int ret;
+
+ memset(uci, 0, sizeof(*uci));
+
+ ret = collect_cpu_info_on_target(cpu, &uci->cpu_sig);
+ if (!ret)
+ uci->valid = 1;
+
+ return ret;
+}
+
+struct apply_microcode_ctx {
+ int err;
+};
+
+static void apply_microcode_local(void *arg)
+{
+ struct apply_microcode_ctx *ctx = arg;
+
+ ctx->err = microcode_ops->apply_microcode(smp_processor_id());
+}
+
+static int apply_microcode_on_target(int cpu)
+{
+ struct apply_microcode_ctx ctx = { .err = 0 };
+ int ret;
+
+ ret = smp_call_function_single(cpu, apply_microcode_local, &ctx, 1);
+ if (!ret)
+ ret = ctx.err;
+
+ return ret;
+}
+
#ifdef CONFIG_MICROCODE_OLD_INTERFACE
static int do_microcode_update(const void __user *buf, size_t size)
{
- cpumask_t old;
int error = 0;
int cpu;

- old = current->cpus_allowed;
-
for_each_online_cpu(cpu) {
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ enum ucode_state ustate;

if (!uci->valid)
continue;

- set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
- error = microcode_ops->request_microcode_user(cpu, buf, size);
- if (error < 0)
- goto out;
- if (!error)
- microcode_ops->apply_microcode(cpu);
+ ustate = microcode_ops->request_microcode_user(cpu, buf, size);
+ if (ustate == UCODE_ERROR) {
+ error = -1;
+ break;
+ } else if (ustate == UCODE_OK)
+ apply_microcode_on_target(cpu);
}
-out:
- set_cpus_allowed_ptr(current, &old);
+
return error;
}

@@ -205,19 +269,22 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
/* fake device for request_firmware */
static struct platform_device *microcode_pdev;

-static long reload_for_cpu(void *unused)
+static int reload_for_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
int err = 0;

mutex_lock(&microcode_mutex);
if (uci->valid) {
- err = microcode_ops->request_microcode_fw(smp_processor_id(),
- &microcode_pdev->dev);
- if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
+ enum ucode_state ustate = microcode_ops->request_microcode_fw(cpu,
+ &microcode_pdev->dev);
+ if (ustate == UCODE_OK)
+ apply_microcode_on_target(cpu);
+ else if (ustate == UCODE_ERROR)
+ err = -1;
}
mutex_unlock(&microcode_mutex);
+
return err;
}

@@ -235,7 +302,7 @@ static ssize_t reload_store(struct sys_device *dev,
if (val == 1) {
get_online_cpus();
if (cpu_online(cpu))
- err = work_on_cpu(cpu, reload_for_cpu, NULL);
+ err = reload_for_cpu(cpu);
put_online_cpus();
}
if (err)
@@ -275,7 +342,7 @@ static struct attribute_group mc_attr_group = {
.name = "microcode",
};

-static void __microcode_fini_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

@@ -283,103 +350,73 @@ static void __microcode_fini_cpu(int cpu)
uci->valid = 0;
}

-static void microcode_fini_cpu(int cpu)
-{
- mutex_lock(&microcode_mutex);
- __microcode_fini_cpu(cpu);
- mutex_unlock(&microcode_mutex);
-}
+enum { UCODE_UPDATE_ERR, UCODE_UPDATE_OK, UCODE_UPDATE_NAVAIL };

-static void collect_cpu_info(int cpu)
+static enum ucode_state microcode_resume_cpu(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

- memset(uci, 0, sizeof(*uci));
- if (!microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig))
- uci->valid = 1;
+ if (!uci->mc)
+ return UCODE_NFOUND;
+
+ pr_debug("microcode: CPU%d updated upon resume\n", cpu);
+ apply_microcode_on_target(cpu);
+
+ return UCODE_OK;
}

-static int microcode_resume_cpu(int cpu)
+static enum ucode_state microcode_init_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- struct cpu_signature nsig;
+ enum ucode_state ustate;

- pr_debug("microcode: CPU%d resumed\n", cpu);
-
- if (!uci->mc)
- return 1;
+ if (collect_cpu_info(cpu))
+ return UCODE_ERROR;

- /*
- * Let's verify that the 'cached' ucode does belong
- * to this cpu (a bit of paranoia):
+ /*
+ * --dimm. Review this case.
+ * If relevant, trigger a delayed update?
*/
- if (microcode_ops->collect_cpu_info(cpu, &nsig)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "failed to collect_cpu_info for resuming cpu #%d\n",
- cpu);
- return -1;
- }
+ if (system_state != SYSTEM_RUNNING)
+ return UCODE_NFOUND;
+
+ ustate = microcode_ops->request_microcode_fw(cpu, &microcode_pdev->dev);

- if ((nsig.sig != uci->cpu_sig.sig) || (nsig.pf != uci->cpu_sig.pf)) {
- __microcode_fini_cpu(cpu);
- printk(KERN_ERR "cached ucode doesn't match the resuming cpu #%d\n",
- cpu);
- /* Should we look for a new ucode here? */
- return 1;
+ if (ustate == UCODE_OK) {
+ pr_debug("microcode: CPU%d updated upon init\n", cpu);
+ apply_microcode_on_target(cpu);
}

- return 0;
+ return ustate;
}

-static long microcode_update_cpu(void *unused)
+static enum ucode_state microcode_update_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + smp_processor_id();
- int err = 0;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ enum ucode_state ustate;

- /*
- * Check if the system resume is in progress (uci->valid != NULL),
- * otherwise just request a firmware:
- */
- if (uci->valid) {
- err = microcode_resume_cpu(smp_processor_id());
- } else {
- collect_cpu_info(smp_processor_id());
- if (uci->valid && system_state == SYSTEM_RUNNING)
- err = microcode_ops->request_microcode_fw(
- smp_processor_id(),
- &microcode_pdev->dev);
- }
- if (!err)
- microcode_ops->apply_microcode(smp_processor_id());
- return err;
-}
+ if (uci->valid)
+ ustate = microcode_resume_cpu(cpu);
+ else
+ ustate = microcode_init_cpu(cpu);

-static int microcode_init_cpu(int cpu)
-{
- int err;
- mutex_lock(&microcode_mutex);
- err = work_on_cpu(cpu, microcode_update_cpu, NULL);
- mutex_unlock(&microcode_mutex);
-
- return err;
+ return ustate;
}

static int mc_sysdev_add(struct sys_device *sys_dev)
{
int err, cpu = sys_dev->id;
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!cpu_online(cpu))
return 0;

pr_debug("microcode: CPU%d added\n", cpu);
- memset(uci, 0, sizeof(*uci));

err = sysfs_create_group(&sys_dev->kobj, &mc_attr_group);
if (err)
return err;

- err = microcode_init_cpu(cpu);
+ if (microcode_init_cpu(cpu) == UCODE_ERROR)
+ err = -1;

return err;
}
@@ -400,12 +437,23 @@ static int mc_sysdev_remove(struct sys_device *sys_dev)
static int mc_sysdev_resume(struct sys_device *dev)
{
int cpu = dev->id;
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;

if (!cpu_online(cpu))
return 0;

- /* only CPU 0 will apply ucode here */
- microcode_update_cpu(NULL);
+ /*
+ * All non-bootup cpus are still disabled,
+ * so only CPU 0 will apply ucode here.
+ *
+ * Moreover, there can be no concurrent
+ * updates from any other places at this point.
+ */
+ WARN_ON(cpu != 0);
+
+ if (uci->valid && uci->mc)
+ microcode_ops->apply_microcode(cpu);
+
return 0;
}

@@ -425,9 +473,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
switch (action) {
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- if (microcode_init_cpu(cpu))
- printk(KERN_ERR "microcode: failed to init CPU%d\n",
- cpu);
+ microcode_update_cpu(cpu);
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
pr_debug("microcode: CPU%d added\n", cpu);
@@ -469,9 +515,6 @@ static int __init microcode_init(void)
return -ENODEV;
}

- error = microcode_dev_init();
- if (error)
- return error;
microcode_pdev = platform_device_register_simple("microcode", -1,
NULL, 0);
if (IS_ERR(microcode_pdev)) {
@@ -480,14 +523,22 @@ static int __init microcode_init(void)
}

get_online_cpus();
+ mutex_lock(&microcode_mutex);
+
error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
+
+ mutex_unlock(&microcode_mutex);
put_online_cpus();
+
if (error) {
- microcode_dev_exit();
platform_device_unregister(microcode_pdev);
return error;
}

+ error = microcode_dev_init();
+ if (error)
+ return error;
+
register_hotcpu_notifier(&mc_cpu_notifier);

printk(KERN_INFO
@@ -505,7 +556,11 @@ static void __exit microcode_exit(void)
unregister_hotcpu_notifier(&mc_cpu_notifier);

get_online_cpus();
+ mutex_lock(&microcode_mutex);
+
sysdev_driver_unregister(&cpu_sysdev_class, &mc_sysdev_driver);
+
+ mutex_unlock(&microcode_mutex);
put_online_cpus();

platform_device_unregister(microcode_pdev);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 149b9ec..52256a5 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -70,24 +70,11 @@
* Fix sigmatch() macro to handle old CPUs with pf == 0.
* Thanks to Stuart Swales for pointing out this bug.
*/
-#include <linux/platform_device.h>
-#include <linux/capability.h>
-#include <linux/miscdevice.h>
#include <linux/firmware.h>
-#include <linux/smp_lock.h>
-#include <linux/spinlock.h>
-#include <linux/cpumask.h>
#include <linux/uaccess.h>
-#include <linux/vmalloc.h>
#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/sched.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/cpu.h>
-#include <linux/fs.h>
-#include <linux/mm.h>
+#include <linux/vmalloc.h>

#include <asm/microcode.h>
#include <asm/processor.h>
@@ -150,13 +137,9 @@ struct extended_sigtable {

#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)

-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu_num);
- unsigned long flags;
unsigned int val[2];

memset(csig, 0, sizeof(*csig));
@@ -176,15 +159,11 @@ static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
csig->pf = 1 << ((val[1] >> 18) & 7);
}

- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
-
wrmsr(MSR_IA32_UCODE_REV, 0, 0);
/* see notes above for revision 1.07. Apparent chip bug */
sync_core();
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], csig->rev);
- spin_unlock_irqrestore(&microcode_update_lock, flags);

pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
csig->sig, csig->pf, csig->rev);
@@ -318,11 +297,10 @@ get_matching_microcode(struct cpu_signature *cpu_sig, void *mc, int rev)
return 0;
}

-static void apply_microcode(int cpu)
+static int apply_microcode(int cpu)
{
struct microcode_intel *mc_intel;
struct ucode_cpu_info *uci;
- unsigned long flags;
unsigned int val[2];
int cpu_num;

@@ -334,10 +312,7 @@ static void apply_microcode(int cpu)
BUG_ON(cpu_num != cpu);

if (mc_intel == NULL)
- return;
-
- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(&microcode_update_lock, flags);
+ return 0;

/* write microcode via MSR 0x79 */
wrmsr(MSR_IA32_UCODE_WRITE,
@@ -351,12 +326,11 @@ static void apply_microcode(int cpu)
/* get the current revision from MSR 0x8B */
rdmsr(MSR_IA32_UCODE_REV, val[0], val[1]);

- spin_unlock_irqrestore(&microcode_update_lock, flags);
if (val[1] != mc_intel->hdr.rev) {
printk(KERN_ERR "microcode: CPU%d update from revision "
"0x%x to 0x%x failed\n",
cpu_num, uci->cpu_sig.rev, val[1]);
- return;
+ return -1;
}
printk(KERN_INFO "microcode: CPU%d updated from revision "
"0x%x to 0x%x, date = %04x-%02x-%02x \n",
@@ -366,15 +340,18 @@ static void apply_microcode(int cpu)
(mc_intel->hdr.date >> 16) & 0xff);

uci->cpu_sig.rev = val[1];
+
+ return 0;
}

-static int generic_load_microcode(int cpu, void *data, size_t size,
- int (*get_ucode_data)(void *, const void *, size_t))
+static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
+ int (*get_ucode_data)(void *, const void *, size_t))
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
u8 *ucode_ptr = data, *new_mc = NULL, *mc;
int new_rev = uci->cpu_sig.rev;
unsigned int leftover = size;
+ enum ucode_state state = UCODE_OK;

while (leftover) {
struct microcode_header_intel mc_header;
@@ -412,11 +389,15 @@ static int generic_load_microcode(int cpu, void *data, size_t size,
leftover -= mc_size;
}

- if (!new_mc)
+ if (leftover) {
+ if (new_mc)
+ vfree(new_mc);
+ state = UCODE_ERROR;
goto out;
+ }

- if (leftover) {
- vfree(new_mc);
+ if (!new_mc) {
+ state = UCODE_NFOUND;
goto out;
}

@@ -427,9 +408,8 @@ static int generic_load_microcode(int cpu, void *data, size_t size,
pr_debug("microcode: CPU%d found a matching microcode update with"
" version 0x%x (current=0x%x)\n",
cpu, new_rev, uci->cpu_sig.rev);
-
- out:
- return (int)leftover;
+out:
+ return state;
}

static int get_ucode_fw(void *to, const void *from, size_t n)
@@ -438,21 +418,19 @@ static int get_ucode_fw(void *to, const void *from, size_t n)
return 0;
}

-static int request_microcode_fw(int cpu, struct device *device)
+static enum ucode_state request_microcode_fw(int cpu, struct device *device)
{
char name[30];
struct cpuinfo_x86 *c = &cpu_data(cpu);
const struct firmware *firmware;
- int ret;
+ enum ucode_state ret;

- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
sprintf(name, "intel-ucode/%02x-%02x-%02x",
c->x86, c->x86_model, c->x86_mask);
- ret = request_firmware(&firmware, name, device);
- if (ret) {
+
+ if (request_firmware(&firmware, name, device)) {
pr_debug("microcode: data file %s load failed\n", name);
- return ret;
+ return UCODE_NFOUND;
}

ret = generic_load_microcode(cpu, (void *)firmware->data,
@@ -468,11 +446,9 @@ static int get_ucode_user(void *to, const void *from, size_t n)
return copy_from_user(to, from, n);
}

-static int request_microcode_user(int cpu, const void __user *buf, size_t size)
+static enum ucode_state
+request_microcode_user(int cpu, const void __user *buf, size_t size)
{
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user);
}

2009-04-23 08:28:11

by Ingo Molnar

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

* Dmitry Adamushko <[email protected]> wrote:

> Version 3
>
> diff with v.2
>
> - use smp_call_function_single() instead of work_on_cpu() as suggested by Ingo;
> - add 'enum ucode_state' as return value of request_microcode_{fw,user}
> - minor cleanups

This version looks really nice structurally. What type of testing
have you done on the patch, on what CPU type?

Thanks,

Ingo

2009-04-23 08:55:41

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/23 Ingo Molnar <[email protected]>:
>
> * Dmitry Adamushko <[email protected]> wrote:
>
>> Version 3
>>
>> diff with v.2
>>
>> - use smp_call_function_single() instead of work_on_cpu() as suggested by Ingo;
>> - add 'enum ucode_state' as return value of request_microcode_{fw,user}
>> - minor cleanups
>
> This version looks really nice structurally. What type of testing
> have you done on the patch, on what CPU type?

Quite limited testing. It looks like there is no newer ucode availble
for my dual-core Intel Core2 Duo (Thinkpad R60) so what I did is as
follows:

- change update_match_revision() in microcode_intel.c in a way that
allows loading of a ucode with a revision == the current revision
(rev. 0x57 in my case). From the POV of the microcode module it
nevertheless looks like an update;

- update via /dev/microcode (CONFIG_MICROCODE_OLD_INTERFACE) with a .dat file;

- suspend -> resume (reload of ucode via cpu-callbacks)

These tests worked for me.

Obviously, it'd be nice if more people could give it a try (e.g.
update with a firmware image and also on AMD - although the changes in
microcode_amd.c are quite straightforward).


p.s. argh... just noticed that the following line is redundant in
microcode_core.c (forgot to remove it)

+enum { UCODE_UPDATE_ERR, UCODE_UPDATE_OK, UCODE_UPDATE_NAVAIL };


>
> Thanks,
>
> Ingo
>

--
Best regards,
Dmitry Adamushko

2009-04-23 18:25:43

by Hugh Dickins

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

On Thu, 23 Apr 2009, Dmitry Adamushko wrote:
> 2009/4/23 Ingo Molnar <[email protected]>:
> > * Dmitry Adamushko <[email protected]> wrote:
> >
> >> Version 3
> >>
> >> diff with v.2
> >>
> >> - use smp_call_function_single() instead of work_on_cpu() as suggested by Ingo;
> >> - add 'enum ucode_state' as return value of request_microcode_{fw,user}
> >> - minor cleanups
> >
> > This version looks really nice structurally.

Yes, it looks the right way to go to me too.

> > What type of testing have you done on the patch, on what CPU type?
>
> Quite limited testing. It looks like there is no newer ucode availble
> for my dual-core Intel Core2 Duo (Thinkpad R60) so what I did is as
> follows:
>
> - change update_match_revision() in microcode_intel.c in a way that
> allows loading of a ucode with a revision == the current revision
> (rev. 0x57 in my case). From the POV of the microcode module it
> nevertheless looks like an update;

My P4 Xeon and Core2s and Atom all want newer microcode from the
latest microcode.dat, and your version seems to work fine on them.

>
> - update via /dev/microcode (CONFIG_MICROCODE_OLD_INTERFACE) with a .dat file;
>
> - suspend -> resume (reload of ucode via cpu-callbacks)
>
> These tests worked for me.

Yes, and I've also tried #if 0'ing the MICROCODE_OLD_INTERFACE code,
converting the microcode.dat to binary, and placing that in
/lib/firmware/intel-ucode/06-0f-0a or wherever: the firmware
interface seems to be working too.

>
> Obviously, it'd be nice if more people could give it a try (e.g.
> update with a firmware image and also on AMD - although the changes in
> microcode_amd.c are quite straightforward).

But like you I don't have an AMD to test.

And just like last time I came here, I find there are more alternative
ways through all this than I'm familiar with, and have time to test:
device versus firmware, Intel versus AMD, startup versus resume versus
online, builtin versus modular. I get easily confused without more
time to spend on it.

That SYSTEM_RUNNING test you've flagged with "--dimm. Review this case":
yes, that's still necessary for when CONFIG_MICROCODE=y (and no initrd?),
as I have - trying to get the firmware at that point will hang. I did
try changing module_init(microcode_init) to late_initcall(microcode_init),
but that didn't solve it. And skipping out at that point does leave CPU0
not updated. But you've not made anything worse there, and most people
will have CONFIG_MICROCODE=m.

>
> p.s. argh... just noticed that the following line is redundant in
> microcode_core.c (forgot to remove it)
>
> +enum { UCODE_UPDATE_ERR, UCODE_UPDATE_OK, UCODE_UPDATE_NAVAIL };

A couple of things that worried me.

I guess your mutex Synchronization works out, but are interrupts
still disabled around the critical wrmsr()s, wherever they're getting
called from?

And you've a habit of returning -1 in error cases, which later gets
muddled in with errnos, so that it would amount to -EPERM, which is
probably not what you want.

Hugh

2009-04-23 22:16:48

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/23 Hugh Dickins <[email protected]>:
> On Thu, 23 Apr 2009, Dmitry Adamushko wrote:
>> [ ... ]
>> - change update_match_revision() in microcode_intel.c in a way that
>> allows loading of a ucode with a revision == the current revision
>> (rev. 0x57 in my case). From the POV of the microcode module it
>> nevertheless looks like an update;
>
> My P4 Xeon and Core2s and Atom all want newer microcode from the
> latest microcode.dat, and your version seems to work fine on them.

Thanks for the tests and review!

> [ ... ]
>
> That SYSTEM_RUNNING test you've flagged with "--dimm. Review this case":
> yes, that's still necessary for when CONFIG_MICROCODE=y (and no initrd?),
> as I have - trying to get the firmware at that point will hang. I did
> try changing module_init(microcode_init) to late_initcall(microcode_init),
> but that didn't solve it. And skipping out at that point does leave CPU0
> not updated. But you've not made anything worse there, and most people
> will have CONFIG_MICROCODE=m.

Yeah. Then at least for the sake of consistency it makes sense to
consider doing some kind of delayed update for CPU0 in this case. Will
check.

>>
>> p.s. argh... just noticed that the following line is redundant in
>> microcode_core.c (forgot to remove it)
>>
>> +enum { UCODE_UPDATE_ERR, UCODE_UPDATE_OK, UCODE_UPDATE_NAVAIL };
>
> A couple of things that worried me.
>
> I guess your mutex Synchronization works out, but are interrupts
> still disabled around the critical wrmsr()s, wherever they're getting
> called from?

Yes, *msr() calls are only done from functions that are now being
called via smp_call_function_single(). The later seems to always do it
with disabled interrupts. The only exception is mc_sysdev_resume()
calling ->apply_microcode() directly but this one in turn is always
called with disabled interrupts.

But now that you mentioned it I wonder if we may actually need a
spinlock there... can we have multi-threaded cpus/cores with (all |
some) shared msr registers?


> And you've a habit of returning -1 in error cases, which later gets
> muddled in with errnos, so that it would amount to -EPERM, which is
> probably not what you want.

Indeed, will fix. Thanks!


>
> Hugh
>

--
Best regards,
Dmitry Adamushko

2009-04-24 12:38:08

by Hugh Dickins

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
> 2009/4/23 Hugh Dickins <[email protected]>:
> >
> > I guess your mutex Synchronization works out, but are interrupts
> > still disabled around the critical wrmsr()s, wherever they're getting
> > called from?
>
> Yes, *msr() calls are only done from functions that are now being
> called via smp_call_function_single(). The later seems to always do it
> with disabled interrupts. The only exception is mc_sysdev_resume()
> calling ->apply_microcode() directly but this one in turn is always
> called with disabled interrupts.
>
> But now that you mentioned it I wonder if we may actually need a
> spinlock there... can we have multi-threaded cpus/cores with (all |
> some) shared msr registers?

Good thinking, yes we can and do, unless I'm misinterpreting the
evidence. Though P4 Xeon and Atom startup messages give the opposite
impression, claiming to update all cpus from lower revision, more
careful tests starting from "maxcpus=1" and then "echo 1 >online"
(which, unless you've fiddled around putting the microcode_ctl'ed
microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
to update at online time on Intel) shows that the later onlined
siblings already have the updated microcode applied to their
previously onlined siblings. Which isn't surprising, but I'd
been lulled into thinking the opposite by the startup sequence.

Please add "HT versus not" to my earlier list of confusions.

microcode_mutex still covers most cases: is it the case of onlining
two threads at the same time that slips through? Is that permitted
at the outer level? Though even if it is, I'd feel safest to have
the spin_lock_irqsaves back (or if not, comment to say why not needed).

Hugh

2009-04-24 14:11:22

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/24 Hugh Dickins <[email protected]>:
> On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
>> 2009/4/23 Hugh Dickins <[email protected]>:
>> >
>> > I guess your mutex Synchronization works out, but are interrupts
>> > still disabled around the critical wrmsr()s, wherever they're getting
>> > called from?
>>
>> Yes, *msr() calls are only done from functions that are now being
>> called via smp_call_function_single(). The later seems to always do it
>> with disabled interrupts. The only exception is mc_sysdev_resume()
>> calling ->apply_microcode() directly but this one in turn is always
>> called with disabled interrupts.
>>
>> But now that you mentioned it I wonder if we may actually need a
>> spinlock there... can we have multi-threaded cpus/cores with (all |
>> some) shared msr registers?
>
> Good thinking, yes we can and do, unless I'm misinterpreting the
> evidence. Though P4 Xeon and Atom startup messages give the opposite
> impression, claiming to update all cpus from lower revision, more
> careful tests starting from "maxcpus=1" and then "echo 1 >online"
> (which, unless you've fiddled around putting the microcode_ctl'ed
> microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
> to update at online time on Intel) shows that the later onlined
> siblings already have the updated microcode applied to their
> previously onlined siblings. Which isn't surprising, but I'd
> been lulled into thinking the opposite by the startup sequence.

Ah, stupid me :-/ These differences in behavior during the startup and
the later update reveal a real bug in my patch.

this part:

mutex_lock(&microcode_mutex);
error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
mutex_unlock(&microcode_mutex);

sysdev_driver_register() calls mc_sysdev_driver's ->add() (which is
mc_sysdev_add()) for each cpu in a loop. Obviously, "microcode_mutex"
can't help to serialize these calls, oops. A very obvious thing but I
missed it.

>
> Please add "HT versus not" to my earlier list of confusions.
>
> microcode_mutex still covers most cases: is it the case of onlining
> two threads at the same time that slips through? Is that permitted
> at the outer level?

If the threads are onlined with cpu_up() then it should be ok - no
concurrent cpu_up()s are allowed. I'll check it out.

> Though even if it is, I'd feel safest to have
> the spin_lock_irqsaves back (or if not, comment to say why not needed).

I'll verify regarding the initialization of HT threads (I'd imagine
that it's indeed via cpu_up(), at the very least for the sake of
consistency as they pretend to be 'normal' cpus to upper layers, e.g.
can be offline/online-ed).

I'm also thinking if the synchronization with "microcode_mutex" is way
too strong/restrictive in this case. Perhaps we actually can add some
parallelism here (with spinlocks in arch-specific parts only where
necessary).

On the other hand, I think that we can optimize cases when a few cpus
are being updated one after another (upon modprobe microcode or
writing into /dev/microcode).

Assumption: most of the CPUs (maybe with an exception of the boot-cpu
when its ucode is updated by BIOS) upgrade from revisions A to B,
where A and B are the same for all of them (well, at least B -- the
most recent one [*]).

Then why bother loading/traversing firmware (or traversing .dat files)
for each of them?

[*] btw., are all CPUs on SMP systems similar wrt model, stepping?

Even if not, we could do some caching so that if cpu-2 asks for
intel-ucode/06-0f-0a and we know that cpu-1 has just done the same and
still has a proper ucode in its buffer, then we just make a copy.


>
> Hugh
>

--
Best regards,
Dmitry Adamushko

2009-04-24 15:31:43

by Hugh Dickins

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
>
> Assumption: most of the CPUs (maybe with an exception of the boot-cpu
> when its ucode is updated by BIOS) upgrade from revisions A to B,
> where A and B are the same for all of them (well, at least B -- the
> most recent one [*]).
>
> Then why bother loading/traversing firmware (or traversing .dat files)
> for each of them?
>
> [*] btw., are all CPUs on SMP systems similar wrt model, stepping?

I'm not an authority on these matters, but I think it's true to say:
that's almost always the case in practice, and some vendors refuse to
support combinations of non-identical CPUs; but it's not necessarily
the case. I think there was a product from one vendor (SGI??) which
most commonly had a mixture.

>
> Even if not, we could do some caching so that if cpu-2 asks for
> intel-ucode/06-0f-0a and we know that cpu-1 has just done the same and
> still has a proper ucode in its buffer, then we just make a copy.

Okay, but don't spend too much code and ingenuity upon it. And note
that when Intel get around to it (and the distros have caught up),
they will be moving away from the all-in-one /etc/microcode.dat
MICROCODE_OLD_INTERFACE, to the /lib/firmware/intel-ucode/06-0f-0a
style - so I don't think optimizing .dat read time is worth much.

Indeed, MICROCODE_OLD_INTERFACE is driven by /sbin/microcode_ctl
anyway, I don't think you should be second-guessing what it might
feed you.

Hugh

2009-04-24 17:01:32

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/24 Dmitry Adamushko <[email protected]>:
> 2009/4/24 Hugh Dickins <[email protected]>:
>> On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
>>> 2009/4/23 Hugh Dickins <[email protected]>:
>>> >
>>> > I guess your mutex Synchronization works out, but are interrupts
>>> > still disabled around the critical wrmsr()s, wherever they're getting
>>> > called from?
>>>
>>> Yes, *msr() calls are only done from functions that are now being
>>> called via smp_call_function_single(). The later seems to always do it
>>> with disabled interrupts. The only exception is mc_sysdev_resume()
>>> calling ->apply_microcode() directly but this one in turn is always
>>> called with disabled interrupts.
>>>
>>> But now that you mentioned it I wonder if we may actually need a
>>> spinlock there... can we have multi-threaded cpus/cores with (all |
>>> some) shared msr registers?
>>
>> Good thinking, yes we can and do, unless I'm misinterpreting the
>> evidence. Though P4 Xeon and Atom startup messages give the opposite
>> impression, claiming to update all cpus from lower revision, more
>> careful tests starting from "maxcpus=1" and then "echo 1 >online"
>> (which, unless you've fiddled around putting the microcode_ctl'ed
>> microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
>> to update at online time on Intel) shows that the later onlined
>> siblings already have the updated microcode applied to their
>> previously onlined siblings. Which isn't surprising, but I'd
>> been lulled into thinking the opposite by the startup sequence.
>
> Ah, stupid me :-/ These differences in behavior during the startup and
> the later update reveal a real bug in my patch.
>
> this part:
>
> mutex_lock(&microcode_mutex);
> error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
> mutex_unlock(&microcode_mutex);
>
> sysdev_driver_register() calls mc_sysdev_driver's ->add() (which is
> mc_sysdev_add()) for each cpu in a loop. Obviously, "microcode_mutex"
> can't help to serialize these calls, oops. A very obvious thing but I
> missed it.

Well, never mind about this one. I might have really been in an
altered state of mind and now need to wait until it comes back to
(let's call it) normality.
All ucode operations are synchronous so obviously we don't need to
serialize actions that run sequentially one after another. Sorry for
the noise here.

But then I wonder why behavior (the fact that all threads seem to
upgrade to a newer version during the startup but they seem to already
be 'up-to-date' if onlined later) during the startup is different...
Too pity that I can't see it with my setups (heh, I perhaps could play
with it by actually downgrading cpus to older ucode).


--
Best regards,
Dmitry Adamushko

2009-04-24 18:23:23

by Hugh Dickins

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
> 2009/4/24 Dmitry Adamushko <[email protected]>:
> > 2009/4/24 Hugh Dickins <[email protected]>:
> >>
> >> Good thinking, yes we can and do, unless I'm misinterpreting the
> >> evidence. Though P4 Xeon and Atom startup messages give the opposite
> >> impression, claiming to update all cpus from lower revision, more
> >> careful tests starting from "maxcpus=1" and then "echo 1 >online"
> >> (which, unless you've fiddled around putting the microcode_ctl'ed
> >> microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
> >> to update at online time on Intel) shows that the later onlined
> >> siblings already have the updated microcode applied to their
> >> previously onlined siblings. Which isn't surprising, but I'd
> >> been lulled into thinking the opposite by the startup sequence.
...
>
> But then I wonder why behavior (the fact that all threads seem to
> upgrade to a newer version during the startup but they seem to already
> be 'up-to-date' if onlined later) during the startup is different...

I believe it's because the module_init microcode_init() calls
sysdev_driver_register(), which does mc_sysdev_add() of (all possible?)
cpus, which for online cpus calls microcode_init_cpu(), which does
collect_cpu_info() then, if SYSTEM_RUNNING, request_microcode_fw()
and apply_microcode_on_target() (names with your patch applied).

If the microcode driver is builtin (so gets here before SYSTEM_RUNNING),
or if it's for Intel with no firmware in /lib/firmware/intel-ucode/X-X-X
yet, the cpu_sig is thus obtained for all online cpus, before initscripts
run /sbin/microcode_ctl to update from /etc/microcode.dat successfully:
the "updated from revision" message shows uci->cpu_sig.rev as it
was saved earlier, rather than reevaluating it just before update.

That's confusing for us, and confusing when resume shows updated from
high revision to same high revision (though, I think, the revision
should in fact have reverted during suspend); but might be even more
worrying to HT users if it were corrected (it would seem as if only
half their cpus got updated, when before all were). I don't know.

> Too pity that I can't see it with my setups (heh, I perhaps could play
> with it by actually downgrading cpus to older ucode).

Please, Intel, ship this man some out-of-date hardware!

(You're sure your cpus really are up-to-date? I thought several
of my boxes were, but then discovered a modinfo line in openSUSE
11.1's /etc/init.d/microcode.ctl, which had been added since 10.3,
which was now disabling it when microcode driver built into kernel.)

Hugh

2009-04-25 10:30:30

by Dmitry Adamushko

Subject: Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic

2009/4/24 Hugh Dickins <[email protected]>:
> On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
>> 2009/4/24 Dmitry Adamushko <[email protected]>:
>> > 2009/4/24 Hugh Dickins <[email protected]>:
>> >>
>> >> Good thinking, yes we can and do, unless I'm misinterpreting the
>> >> evidence. Though P4 Xeon and Atom startup messages give the opposite
>> >> impression, claiming to update all cpus from lower revision, more
>> >> careful tests starting from "maxcpus=1" and then "echo 1 >online"
>> >> (which, unless you've fiddled around putting the microcode_ctl'ed
>> >> microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
>> >> to update at online time on Intel) shows that the later onlined
>> >> siblings already have the updated microcode applied to their
>> >> previously onlined siblings. Which isn't surprising, but I'd
>> >> been lulled into thinking the opposite by the startup sequence.
> ...
>>
>> But then I wonder why behavior (the fact that all threads seem to
>> upgrade to a newer version during the startup but they seem to already
>> be 'up-to-date' if onlined later) during the startup is different...
>
> I believe it's because the module_init microcode_init() calls
> sysdev_driver_register(), which does mc_sysdev_add() of (all possible?)
> cpus, which for online cpus calls microcode_init_cpu(), which does
> collect_cpu_info() then, if SYSTEM_RUNNING, request_microcode_fw()
> and apply_microcode_on_target() (names with your patch applied).
>
> If the microcode driver is builtin (so gets here before SYSTEM_RUNNING),
> or if it's for Intel with no firmware in /lib/firmware/intel-ucode/X-X-X
> yet, the cpu_sig is thus obtained for all online cpus, before initscripts
> run /sbin/microcode_ctl to update from /etc/microcode.dat successfully:
> the "updated from revision" message shows uci->cpu_sig.rev as it
> was saved earlier, rather than reevaluating it just before update.
>
> That's confusing for us, and confusing when resume shows updated from
> high revision to same high revision (though, I think, the revision
> should in fact have reverted during suspend); but might be even more
> worrying to HT users if it were corrected (it would seem as if only
> half their cpus got updated, when before all were). I don't know.

Perhaps, it doesn't make sense to cache 'cpu_sig' at all. I'll ponder
on it a bit and send a new patch.

Thanks a lot for your feedback!

>
>> Too pity that I can't see it with my setups (heh, I perhaps could play
>> with it by actually downgrading cpus to older ucode).
>
> Please, Intel, ship this man some out-of-date hardware!
>
> (You're sure your cpus really are up-to-date? I thought several
> of my boxes were, but then discovered a modinfo line in openSUSE
> 11.1's /etc/init.d/microcode.ctl, which had been added since 10.3,
> which was now disabling it when microcode driver built into kernel.)
>

hmm, I downloaded - what seems to be - the recent .dat file from the
Intel's site and run microcode_ctl (a binary, no additional scripts
manually). I will check the site for updates again.

>
> Hugh
>

--
Best regards,
Dmitry Adamushko