[patch 0/5] x86-microcode: run ucode update via workqueue

[patch 0/5] x86-microcode: run ucode update via workqueue

[Peter Oruba]

Referring to: http://lkml.org/lkml/2008/8/3/114
Dmitry, thanks for your work.

Rebased Dmitry's patch set to x86-microcode and incorporated changes into AMD microcode patch loader. No issues with
cpu-hotplug anymore, tested on Intel as well as AMD.

--Peter

[patch 1/5] [PATCH 1/5] x86: microcode generic updates

[Peter Oruba]

[-- Attachment #1: 0001-x86-microcode-generic-updates.patch --]
[-- Type: text/plain, Size: 10716 bytes --]

Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode.c       |   83 ++++++++++++++-----------------
 arch/x86/kernel/microcode_intel.c |   97 +++++++++++++++++++------------------
 include/asm-x86/microcode.h       |   27 +++++-----
 3 files changed, 101 insertions(+), 106 deletions(-)

diff --git a/arch/x86/kernel/microcode.c b/arch/x86/kernel/microcode.c
index ad136ad..b797692 100644
--- a/arch/x86/kernel/microcode.c
+++ b/arch/x86/kernel/microcode.c
@@ -234,22 +234,6 @@ MODULE_ALIAS_MISCDEV(MICROCODE_MINOR);
 struct platform_device *microcode_pdev;
 EXPORT_SYMBOL_GPL(microcode_pdev);
 
-static void microcode_init_cpu(int cpu, int resume)
-{
-	cpumask_t old;
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
-	old = current->cpus_allowed;
-
-	set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
-	mutex_lock(&microcode_mutex);
-	microcode_ops->collect_cpu_info(cpu);
-	if (uci->valid && system_state == SYSTEM_RUNNING && !resume)
-		microcode_ops->cpu_request_microcode(cpu);
-	mutex_unlock(&microcode_mutex);
-	set_cpus_allowed_ptr(current, &old);
-}
-
 static ssize_t reload_store(struct sys_device *dev,
 			    struct sysdev_attribute *attr,
 			    const char *buf, size_t sz)
@@ -266,14 +250,15 @@ static ssize_t reload_store(struct sys_device *dev,
 		cpumask_t old = current->cpus_allowed;
 
 		get_online_cpus();
-		set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
-
-		mutex_lock(&microcode_mutex);
-		if (uci->valid)
-			err = microcode_ops->cpu_request_microcode(cpu);
-		mutex_unlock(&microcode_mutex);
+		if (cpu_online(cpu)) {
+			set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
+			mutex_lock(&microcode_mutex);
+			if (uci->valid)
+				err = microcode_ops->cpu_request_microcode(cpu);
+			mutex_unlock(&microcode_mutex);
+			set_cpus_allowed_ptr(current, &old);
+		}
 		put_online_cpus();
-		set_cpus_allowed_ptr(current, &old);
 	}
 	if (err)
 		return err;
@@ -312,7 +297,16 @@ static struct attribute_group mc_attr_group = {
 	.name = "microcode",
 };
 
-static int __mc_sysdev_add(struct sys_device *sys_dev, int resume)
+static void microcode_init_cpu(int cpu)
+{
+        cpumask_t old = current->cpus_allowed;
+
+        set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
+        microcode_ops->microcode_update_cpu(cpu);
+        set_cpus_allowed_ptr(current, &old);
+}
+
+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;
@@ -327,16 +321,11 @@ static int __mc_sysdev_add(struct sys_device *sys_dev, int resume)
 	if (err)
 		return err;
 
-	microcode_init_cpu(cpu, resume);
+	microcode_init_cpu(cpu);
 
 	return 0;
 }
 
-static int mc_sysdev_add(struct sys_device *sys_dev)
-{
-	return __mc_sysdev_add(sys_dev, 0);
-}
-
 static int mc_sysdev_remove(struct sys_device *sys_dev)
 {
 	int cpu = sys_dev->id;
@@ -362,6 +351,16 @@ static int mc_sysdev_resume(struct sys_device *dev)
 	return 0;
 }
 
+static void microcode_set_cpu_frozen(int cpu)
+{
+        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+        mutex_lock(&microcode_mutex);
+        if (uci->valid)
+                uci->resume = 1;
+        mutex_unlock(&microcode_mutex);
+}
+
 static struct sysdev_driver mc_sysdev_driver = {
 	.add = mc_sysdev_add,
 	.remove = mc_sysdev_remove,
@@ -376,34 +375,26 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 
 	sys_dev = get_cpu_sysdev(cpu);
 	switch (action) {
-	case CPU_UP_CANCELED_FROZEN:
-		/* The CPU refused to come up during a system resume */
-		microcode_ops->microcode_fini_cpu(cpu);
-		break;
 	case CPU_ONLINE:
-	case CPU_DOWN_FAILED:
-		mc_sysdev_add(sys_dev);
-		break;
 	case CPU_ONLINE_FROZEN:
-		/* System-wide resume is in progress, try to apply microcode */
-		if (microcode_ops->apply_microcode_check_cpu(cpu)) {
-			/* The application of microcode failed */
-			microcode_ops->microcode_fini_cpu(cpu);
-			__mc_sysdev_add(sys_dev, 1);
-			break;
-		}
+	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
 		if (sysfs_create_group(&sys_dev->kobj, &mc_attr_group))
 			printk(KERN_ERR "microcode: Failed to create the sysfs "
 				"group for CPU%d\n", cpu);
 		break;
 	case CPU_DOWN_PREPARE:
-		mc_sysdev_remove(sys_dev);
-		break;
 	case CPU_DOWN_PREPARE_FROZEN:
 		/* Suspend is in progress, only remove the interface */
 		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
 		break;
+	case CPU_DEAD:
+	case CPU_UP_CANCELED_FROZEN:
+		/* The CPU refused to come up during a system resume */
+		microcode_ops->microcode_fini_cpu(cpu);
+		break;
+	case CPU_DEAD_FROZEN:
+		microcode_set_cpu_frozen(cpu);
 	}
 	return NOTIFY_OK;
 }
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index a61986e..efe2ee9 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -127,10 +127,9 @@ extern struct mutex microcode_mutex;
 
 extern struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
 
-static void collect_cpu_info(int cpu_num)
+static void collect_cpu_info(int cpu_num, struct ucode_cpu_info *uci)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu_num);
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
 	unsigned int val[2];
 
 	/* We should bind the task to the CPU */
@@ -456,70 +455,74 @@ static int cpu_request_microcode(int cpu)
 	return error;
 }
 
-static int apply_microcode_check_cpu(int cpu)
+static void microcode_fini_cpu(int cpu)
 {
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
 	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	cpumask_t old;
-	unsigned int val[2];
-	int err = 0;
+	
+	mutex_lock(&microcode_mutex);
+	uci->valid = 0;
+	uci->resume = 0;
+	vfree(uci->mc.mc_intel);
+	uci->mc.mc_intel = NULL;
+	mutex_unlock(&microcode_mutex);
+}
 
-	/* Check if the microcode is available */
-	if (!uci->mc.mc_intel)
+static int microcode_resume_cpu(int cpu, struct ucode_cpu_info *new_uci)
+{
+	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
+	if (uci->resume)
 		return 0;
 
-	old = current->cpus_allowed;
-	set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
+	uci->resume = 0;
 
-	/* Check if the microcode we have in memory matches the CPU */
-	if (c->x86_vendor != X86_VENDOR_INTEL || c->x86 < 6 ||
-	    cpu_has(c, X86_FEATURE_IA64) || uci->sig != cpuid_eax(0x00000001))
-		err = -EINVAL;
-
-	if (!err && ((c->x86_model >= 5) || (c->x86 > 6))) {
-		/* get processor flags from MSR 0x17 */
-		rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
-		if (uci->pf != (1 << ((val[1] >> 18) & 7)))
-			err = -EINVAL;
-	}
-
-	if (!err) {
-		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], val[1]);
-		if (uci->rev != val[1])
-			err = -EINVAL;
-	}
+	/* check if the 'cached' microcode is ok: */
+	if (!uci->mc.mc_intel)
+		return 1;
 
-	if (!err)
+	if (new_uci->sig == uci->sig &&
+	    new_uci->pf  == uci->pf &&
+	    new_uci->rev == uci->rev) {
 		apply_microcode(cpu);
-	else
-		printk(KERN_ERR "microcode: Could not apply microcode to CPU%d:"
-			" sig=0x%x, pf=0x%x, rev=0x%x\n",
-			cpu, uci->sig, uci->pf, uci->rev);
+	} else {
+		microcode_fini_cpu(cpu);
+		*uci = *new_uci;
+	}
 
-	set_cpus_allowed_ptr(current, &old);
-	return err;
+	return 1;
 }
 
-static void microcode_fini_cpu(int cpu)
+void microcode_update_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+        struct ucode_cpu_info *uci = ucode_cpu_info + cpu, new_uci;
 
-	mutex_lock(&microcode_mutex);
-	uci->valid = 0;
-	vfree(uci->mc.mc_intel);
-	uci->mc.mc_intel = NULL;
-	mutex_unlock(&microcode_mutex);
+        memset (&new_uci, 0, sizeof(new_uci));
+
+        mutex_lock(&microcode_mutex);
+
+        collect_cpu_info(cpu, &new_uci);
+
+        if (new_uci.valid) {
+                /*                                                                                                    
+                 * Check if the system resume is in progress,                                                         
+                 * otherwise just request a firmware:                                                                 
+                 */
+                if (!microcode_resume_cpu(cpu, &new_uci)) {
+                        *uci = new_uci;
+
+                        if (system_state == SYSTEM_RUNNING)
+                                cpu_request_microcode(cpu);
+                }
+        }
+
+        mutex_unlock(&microcode_mutex);
 }
 
 static struct microcode_ops microcode_intel_ops = {
 	.get_next_ucode                   = get_next_ucode,
+	.microcode_update_cpu             = microcode_update_cpu,
 	.get_matching_microcode           = get_matching_microcode,
 	.microcode_sanity_check           = microcode_sanity_check,
-	.apply_microcode_check_cpu        = apply_microcode_check_cpu,
 	.cpu_request_microcode            = cpu_request_microcode,
 	.collect_cpu_info                 = collect_cpu_info,
 	.apply_microcode                  = apply_microcode,
diff --git a/include/asm-x86/microcode.h b/include/asm-x86/microcode.h
index 18b2aee..072bef1 100644
--- a/include/asm-x86/microcode.h
+++ b/include/asm-x86/microcode.h
@@ -1,19 +1,6 @@
 extern int microcode_init(void *opaque, struct module *module);
 extern void microcode_exit(void);
 
-struct microcode_ops {
-	long (*get_next_ucode)(void **mc, long offset);
-	long (*microcode_get_next_ucode)(void **mc, long offset);
-	int (*get_matching_microcode)(void *mc, int cpu);
-	int (*apply_microcode_check_cpu)(int cpu);
-	int (*microcode_sanity_check)(void *mc);
-	int (*cpu_request_microcode)(int cpu);
-	void (*collect_cpu_info)(int cpu_num);
-	void (*apply_microcode)(int cpu);
-	void (*microcode_fini_cpu)(int cpu);
-	void (*clear_patch)(void *data);
-};
-
 struct microcode_header_intel {
 	unsigned int            hdrver;
 	unsigned int            rev;
@@ -77,6 +64,7 @@ struct microcode_amd {
 
 struct ucode_cpu_info {
 	int valid;
+	int resume;
 	unsigned int sig;
 	unsigned int pf;
 	unsigned int rev;
@@ -85,3 +73,16 @@ struct ucode_cpu_info {
 		struct microcode_amd *mc_amd;
 	} mc;
 };
+
+struct microcode_ops {
+	long (*get_next_ucode)(void **mc, long offset);
+	void (*microcode_update_cpu)(int cpu);
+	long (*microcode_get_next_ucode)(void **mc, long offset);
+	int (*get_matching_microcode)(void *mc, int cpu);
+	int (*microcode_sanity_check)(void *mc);
+	int (*cpu_request_microcode)(int cpu);
+	void (*collect_cpu_info)(int cpu_num, struct ucode_cpu_info *uci);
+	void (*apply_microcode)(int cpu);
+	void (*microcode_fini_cpu)(int cpu);
+	void (*clear_patch)(void *data);
+};
-- 
1.5.4.5

Re: [patch 1/5] [PATCH 1/5] x86: microcode generic updates

[Ingo Molnar]

* Peter Oruba <peter.oruba@amd.com> wrote:

> Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
> Signed-off-by: Peter Oruba <peter.oruba@amd.com>

hm, missing changelog entry.

	Ingo

[patch 2/5] [PATCH 2/5] x86: Minor correction to header file

[Peter Oruba]

[-- Attachment #1: 0002-x86-Minor-correction-to-header-file.patch --]
[-- Type: text/plain, Size: 601 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 include/asm-x86/microcode.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/asm-x86/microcode.h b/include/asm-x86/microcode.h
index 072bef1..fb08022 100644
--- a/include/asm-x86/microcode.h
+++ b/include/asm-x86/microcode.h
@@ -1,5 +1,5 @@
-extern int microcode_init(void *opaque, struct module *module);
-extern void microcode_exit(void);
+int microcode_init(void *opaque, struct module *module);
+void microcode_exit(void);
 
 struct microcode_header_intel {
 	unsigned int            hdrver;
-- 
1.5.4.5

Re: [patch 2/5] [PATCH 2/5] x86: Minor correction to header file

[Ingo Molnar]

* Peter Oruba <peter.oruba@amd.com> wrote:

> Signed-off-by: Peter Oruba <peter.oruba@amd.com>
> ---
>  include/asm-x86/microcode.h |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/asm-x86/microcode.h b/include/asm-x86/microcode.h
> index 072bef1..fb08022 100644
> --- a/include/asm-x86/microcode.h
> +++ b/include/asm-x86/microcode.h
> @@ -1,5 +1,5 @@
> -extern int microcode_init(void *opaque, struct module *module);
> -extern void microcode_exit(void);
> +int microcode_init(void *opaque, struct module *module);
> +void microcode_exit(void);

why? extern is the typical way to specify interfaces.

	Ingo

[patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Peter Oruba]

[-- Attachment #1: 0003-x86-Run-Intel-ucode-updates-via-workqueue.patch --]
[-- Type: text/plain, Size: 10499 bytes --]

Signed-off-by: Dmitry Adamushko <dmitry.adamushko@gmail.com>
Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode.c       |   49 ++++++++++++++----------
 arch/x86/kernel/microcode_intel.c |   74 ++++++++++++++++++++++--------------
 include/asm-x86/microcode.h       |    9 +++-
 3 files changed, 80 insertions(+), 52 deletions(-)

diff --git a/arch/x86/kernel/microcode.c b/arch/x86/kernel/microcode.c
index b797692..0264c76 100644
--- a/arch/x86/kernel/microcode.c
+++ b/arch/x86/kernel/microcode.c
@@ -110,6 +110,14 @@ EXPORT_SYMBOL_GPL(microcode_mutex);
 struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
 EXPORT_SYMBOL_GPL(ucode_cpu_info);
 
+struct ucode_work_info ucode_work_info[NR_CPUS];
+EXPORT_SYMBOL_GPL(ucode_work_info);
+
+static inline struct ucode_cpu_info *get_ucode_cpu_info(int cpu)
+{
+	return &ucode_work_info[cpu].uci;
+}
+
 #ifdef CONFIG_MICROCODE_OLD_INTERFACE
 void __user *user_buffer;		/* user area microcode data buffer */
 EXPORT_SYMBOL_GPL(user_buffer);
@@ -238,7 +246,7 @@ static ssize_t reload_store(struct sys_device *dev,
 			    struct sysdev_attribute *attr,
 			    const char *buf, size_t sz)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + dev->id;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(dev->id);
 	char *end;
 	unsigned long val = simple_strtoul(buf, &end, 0);
 	int err = 0;
@@ -268,7 +276,7 @@ static ssize_t reload_store(struct sys_device *dev,
 static ssize_t version_show(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + dev->id;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(dev->id);
 
 	return sprintf(buf, "0x%x\n", uci->rev);
 }
@@ -276,7 +284,7 @@ static ssize_t version_show(struct sys_device *dev,
 static ssize_t pf_show(struct sys_device *dev,
 			struct sysdev_attribute *attr, char *buf)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + dev->id;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(dev->id);
 
 	return sprintf(buf, "0x%x\n", uci->pf);
 }
@@ -297,31 +305,22 @@ static struct attribute_group mc_attr_group = {
 	.name = "microcode",
 };
 
-static void microcode_init_cpu(int cpu)
-{
-        cpumask_t old = current->cpus_allowed;
-
-        set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
-        microcode_ops->microcode_update_cpu(cpu);
-        set_cpus_allowed_ptr(current, &old);
-}
-
 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;
+	struct ucode_work_info *wi = ucode_work_info + cpu;
 
 	if (!cpu_online(cpu))
 		return 0;
 
 	pr_debug("microcode: CPU%d added\n", cpu);
-	memset(uci, 0, sizeof(*uci));
+	memset(&wi->uci, 0, sizeof(wi->uci));
 
 	err = sysfs_create_group(&sys_dev->kobj, &mc_attr_group);
 	if (err)
 		return err;
 
-	microcode_init_cpu(cpu);
+	schedule_work_on(cpu, &wi->work);
 
 	return 0;
 }
@@ -353,12 +352,14 @@ static int mc_sysdev_resume(struct sys_device *dev)
 
 static void microcode_set_cpu_frozen(int cpu)
 {
-        struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_work_info *wi = ucode_work_info + cpu;
+
+	pr_debug("microcode: CPU%d frozen\n", cpu);
 
-        mutex_lock(&microcode_mutex);
-        if (uci->valid)
-                uci->resume = 1;
-        mutex_unlock(&microcode_mutex);
+	mutex_lock(&microcode_mutex);
+	if (wi->uci.valid)
+		wi->resume = 1;
+	mutex_unlock(&microcode_mutex);
 }
 
 static struct sysdev_driver mc_sysdev_driver = {
@@ -377,6 +378,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 	switch (action) {
 	case CPU_ONLINE:
 	case CPU_ONLINE_FROZEN:
+		schedule_work_on(cpu, &ucode_work_info[cpu].work);
 	case CPU_DOWN_FAILED:
 	case CPU_DOWN_FAILED_FROZEN:
 		if (sysfs_create_group(&sys_dev->kobj, &mc_attr_group))
@@ -387,6 +389,7 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
 	case CPU_DOWN_PREPARE_FROZEN:
 		/* Suspend is in progress, only remove the interface */
 		sysfs_remove_group(&sys_dev->kobj, &mc_attr_group);
+		cancel_work_sync(&ucode_work_info[cpu].work);
 		break;
 	case CPU_DEAD:
 	case CPU_UP_CANCELED_FROZEN:
@@ -406,7 +409,7 @@ static struct notifier_block __refdata mc_cpu_notifier = {
 int microcode_init(void *opaque, struct module *module)
 {
 	struct microcode_ops *ops = (struct microcode_ops *)opaque;
-	int error;
+	int error, i;
 
 	if (microcode_ops) {
 		printk(KERN_ERR "microcode: already loaded the other module\n");
@@ -425,6 +428,10 @@ int microcode_init(void *opaque, struct module *module)
 		return PTR_ERR(microcode_pdev);
 	}
 
+	for (i = 0; i < NR_CPUS; i++)
+		INIT_WORK(&ucode_work_info[i].work,
+			  microcode_ops->microcode_update_cpu);
+
 	get_online_cpus();
 	error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
 	put_online_cpus();
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index efe2ee9..4ab6ce3 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -126,6 +126,12 @@ static DEFINE_SPINLOCK(microcode_update_lock);
 extern struct mutex microcode_mutex;
 
 extern struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
+extern struct ucode_work_info ucode_work_info[NR_CPUS];
+
+static inline struct ucode_cpu_info *get_ucode_cpu_info(int cpu)
+{
+	return &ucode_work_info[cpu].uci;
+}
 
 static void collect_cpu_info(int cpu_num, struct ucode_cpu_info *uci)
 {
@@ -159,6 +165,7 @@ static void collect_cpu_info(int cpu_num, struct ucode_cpu_info *uci)
 	sync_core();
 	/* get the current revision from MSR 0x8B */
 	rdmsr(MSR_IA32_UCODE_REV, val[0], uci->rev);
+
 	pr_debug("microcode: collect_cpu_info : sig=0x%x, pf=0x%x, rev=0x%x\n",
 			uci->sig, uci->pf, uci->rev);
 }
@@ -166,7 +173,7 @@ static void collect_cpu_info(int cpu_num, struct ucode_cpu_info *uci)
 static inline int microcode_update_match(int cpu_num,
 	struct microcode_header_intel *mc_header, int sig, int pf)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu_num);
 
 	if (!sigmatch(sig, uci->sig, pf, uci->pf)
 		|| mc_header->rev <= uci->rev)
@@ -260,7 +267,7 @@ static int microcode_sanity_check(void *mc)
  */
 static int get_matching_microcode(void *mc, int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
 	struct microcode_header_intel *mc_header = mc;
 	struct extended_sigtable *ext_header;
 	unsigned long total_size = get_totalsize(mc_header);
@@ -308,7 +315,7 @@ static void apply_microcode(int cpu)
 	unsigned long flags;
 	unsigned int val[2];
 	int cpu_num = raw_smp_processor_id();
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
 
 	/* We should bind the task to the CPU */
 	BUG_ON(cpu_num != cpu);
@@ -457,24 +464,28 @@ static int cpu_request_microcode(int cpu)
 
 static void microcode_fini_cpu(int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	
+	struct ucode_work_info *wi = ucode_work_info + cpu;
+
 	mutex_lock(&microcode_mutex);
-	uci->valid = 0;
-	uci->resume = 0;
-	vfree(uci->mc.mc_intel);
-	uci->mc.mc_intel = NULL;
+	wi->uci.valid = 0;
+	vfree(wi->uci.mc.mc_intel);
+	wi->uci.mc.mc_intel = NULL;
+	wi->resume = 0;
 	mutex_unlock(&microcode_mutex);
 }
 
 static int microcode_resume_cpu(int cpu, struct ucode_cpu_info *new_uci)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_work_info *wi = ucode_work_info + cpu;
+	struct ucode_cpu_info *uci = &wi->uci;
 
-	if (uci->resume)
+	if (!wi->resume)
 		return 0;
 
-	uci->resume = 0;
+	pr_debug("microcode: CPU%d resumed (ucode: %p)\n",
+		 cpu, uci->mc.mc_intel);
+
+	wi->resume = 0;
 
 	/* check if the 'cached' microcode is ok: */
 	if (!uci->mc.mc_intel)
@@ -492,30 +503,35 @@ static int microcode_resume_cpu(int cpu, struct ucode_cpu_info *new_uci)
 	return 1;
 }
 
-void microcode_update_cpu(int cpu)
+static void microcode_update_cpu(struct work_struct *w)
 {
-        struct ucode_cpu_info *uci = ucode_cpu_info + cpu, new_uci;
+	struct ucode_work_info *wi = container_of(w, struct ucode_work_info,
+						  work);
+	int cpu = raw_smp_processor_id(); /* keventd is per-cpu */
+	struct ucode_cpu_info new_uci;
 
-        memset (&new_uci, 0, sizeof(new_uci));
+	pr_debug("microcode: CPU%d workqueue\n", cpu);
 
-        mutex_lock(&microcode_mutex);
+	memset(&new_uci, 0, sizeof(new_uci));
 
-        collect_cpu_info(cpu, &new_uci);
+	mutex_lock(&microcode_mutex);
 
-        if (new_uci.valid) {
-                /*                                                                                                    
-                 * Check if the system resume is in progress,                                                         
-                 * otherwise just request a firmware:                                                                 
-                 */
-                if (!microcode_resume_cpu(cpu, &new_uci)) {
-                        *uci = new_uci;
+	collect_cpu_info(cpu, &new_uci);
 
-                        if (system_state == SYSTEM_RUNNING)
-                                cpu_request_microcode(cpu);
-                }
-        }
+	if (new_uci.valid) {
+                /*
+		 * Check if the system resume is in progress,
+		 * otherwise just request a firmware:
+		 */
+		if (!microcode_resume_cpu(cpu, &new_uci)) {
+			wi->uci = new_uci;
 
-        mutex_unlock(&microcode_mutex);
+			if (system_state == SYSTEM_RUNNING)
+				cpu_request_microcode(cpu);
+		}
+	}
+
+	mutex_unlock(&microcode_mutex);
 }
 
 static struct microcode_ops microcode_intel_ops = {
diff --git a/include/asm-x86/microcode.h b/include/asm-x86/microcode.h
index fb08022..69388fa 100644
--- a/include/asm-x86/microcode.h
+++ b/include/asm-x86/microcode.h
@@ -64,7 +64,6 @@ struct microcode_amd {
 
 struct ucode_cpu_info {
 	int valid;
-	int resume;
 	unsigned int sig;
 	unsigned int pf;
 	unsigned int rev;
@@ -74,9 +73,15 @@ struct ucode_cpu_info {
 	} mc;
 };
 
+struct ucode_work_info {
+	struct ucode_cpu_info uci;
+	int resume;
+	struct work_struct work;
+};
+
 struct microcode_ops {
 	long (*get_next_ucode)(void **mc, long offset);
-	void (*microcode_update_cpu)(int cpu);
+	void (*microcode_update_cpu)(struct work_struct *w);
 	long (*microcode_get_next_ucode)(void **mc, long offset);
 	int (*get_matching_microcode)(void *mc, int cpu);
 	int (*microcode_sanity_check)(void *mc);
-- 
1.5.4.5

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Arjan van de Ven]

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <peter.oruba@amd.com> wrote:

[ no description or reason ]

Why is this?

I'm not very happy about this.. it means practically that this stuff
*has* to run late. Probably later than we want to.
(Like.. we may want to redo the microcode during resume.. which is
not a schedulable context)


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Dmitry Adamushko]

2008/8/6 Arjan van de Ven <arjan@infradead.org>:
> On Wed, 6 Aug 2008 17:21:20 +0200
> Peter Oruba <peter.oruba@amd.com> wrote:
>
> [ no description or reason ]
>
> Why is this?

More details are available here.

I've also suggested to do ucode-updates as early as possible  from
start_secondary() (or whatever low-level code).

The second patch (do via workqueue) was just a proof-of-concept (to
show that it does fix a crash).

>
> I'm not very happy about this.. it means practically that this stuff
> *has* to run late. Probably later than we want to.

Currently, it runs from cpu-hotplug-notifier(CPU_ONLINE, ...)
[ and crashes with .26+ due to a reason you may find via the
aforementioned link ] - by this moment, at least kernel threads may
running on this cpu -- so it's not that early.

> (Like.. we may want to redo the microcode during resume.. which is
> not a schedulable context)

I'm not sure what's "schedulable context" here terms but the existing
code makes use of set_cpus_allowed_ptr() which means a caller expects
to be able to be migrated onto a target cpu and do ucode-update while
running on it.

Moreover, the way set_cpus_allowed_ptr() is used there seems to be
race wrt. sched_setaffinity().


-- 
Best regards,
Dmitry Adamushko

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Dmitry Adamushko]

2008/8/6 Dmitry Adamushko <dmitry.adamushko@gmail.com>:
> 2008/8/6 Arjan van de Ven <arjan@infradead.org>:
>> On Wed, 6 Aug 2008 17:21:20 +0200
>> Peter Oruba <peter.oruba@amd.com> wrote:
>>
>> [ no description or reason ]
>>
>> Why is this?
>
> More details are available here.

forgot a link:

http://lkml.org/lkml/2008/8/3/114


-- 
Best regards,
Dmitry Adamushko

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Max Krasnyansky]

Arjan van de Ven wrote:
> On Wed, 6 Aug 2008 17:21:20 +0200
> Peter Oruba <peter.oruba@amd.com> wrote:
> 
> [ no description or reason ]
> 
> Why is this?
> 
> I'm not very happy about this.. it means practically that this stuff
> *has* to run late. Probably later than we want to.
> (Like.. we may want to redo the microcode during resume.. which is
> not a schedulable context)

Dmitry and I tried to figure out how soon does it need to run.
Nobody had a strong argument why it must run synchronously in the 
hotplug path. Sure we want it as soon as possible and I'd say workqueue 
is soon enough.
Existing hotplug path does not guaranty any ordering and original 
microcode interface was driven from user-space. So clearly it was not 
considered very critical.

Max

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Arjan van de Ven]

On Wed, 06 Aug 2008 13:31:18 -0700
Max Krasnyansky <maxk@qualcomm.com> wrote:

> Arjan van de Ven wrote:
> > On Wed, 6 Aug 2008 17:21:20 +0200
> > Peter Oruba <peter.oruba@amd.com> wrote:
> > 
> > [ no description or reason ]
> > 
> > Why is this?
> > 
> > I'm not very happy about this.. it means practically that this stuff
> > *has* to run late. Probably later than we want to.
> > (Like.. we may want to redo the microcode during resume.. which is
> > not a schedulable context)
> 
> Dmitry and I tried to figure out how soon does it need to run.

you're not going to like the answer, but it's "as soon as possible".
Unlike normal boot, hotplug is a case where the bios hasn't been able
to put a normal microcode in. 

> Nobody had a strong argument why it must run synchronously in the 
> hotplug path.

Ok we as Intel really want it as early as possible.

> Sure we want it as soon as possible and I'd say
> workqueue is soon enough.
> Existing hotplug path does not guaranty any ordering and original 
> microcode interface was driven from user-space. So clearly it was not 
> considered very critical.

well that's why it changed to no longer use the userspace driven thing..


> 
> Max


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [patch 3/5] [PATCH 3/5] x86: Run Intel ucode-updates via workqueue.

[Ingo Molnar]

* Arjan van de Ven <arjan@infradead.org> wrote:

> On Wed, 6 Aug 2008 17:21:20 +0200
> Peter Oruba <peter.oruba@amd.com> wrote:
> 
> [ no description or reason ]
> 
> Why is this?
> 
> I'm not very happy about this.. it means practically that this stuff 
> *has* to run late. Probably later than we want to. (Like.. we may want 
> to redo the microcode during resume.. which is not a schedulable 
> context)

also, with the hotplug notifier call sequence fixed/changed now, there's 
no pressing reason to do this change.

	Ingo

[patch 4/5] [PATCH 4/5] x86: Minor correction to Intel microcode patch loader.

[Peter Oruba]

[-- Attachment #1: 0004-x86-Minor-correction-to-Intel-microcode-patch-loade.patch --]
[-- Type: text/plain, Size: 688 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_intel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 4ab6ce3..8d69470 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -315,7 +315,7 @@ static void apply_microcode(int cpu)
 	unsigned long flags;
 	unsigned int val[2];
 	int cpu_num = raw_smp_processor_id();
-	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu_num);
 
 	/* We should bind the task to the CPU */
 	BUG_ON(cpu_num != cpu);
-- 
1.5.4.5

Re: [patch 4/5] [PATCH 4/5] x86: Minor correction to Intel microcode patch loader.

[Ingo Molnar]

* Peter Oruba <peter.oruba@amd.com> wrote:

> Signed-off-by: Peter Oruba <peter.oruba@amd.com>
> ---
>  arch/x86/kernel/microcode_intel.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
> index 4ab6ce3..8d69470 100644
> --- a/arch/x86/kernel/microcode_intel.c
> +++ b/arch/x86/kernel/microcode_intel.c
> @@ -315,7 +315,7 @@ static void apply_microcode(int cpu)
>  	unsigned long flags;
>  	unsigned int val[2];
>  	int cpu_num = raw_smp_processor_id();
> -	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
> +	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu_num);

hm, this does not apply to tip/x86/microcode nor tip/master nor 
linus/master. Does it fix a bug in one of the earlier patches in this 
5/5 queue?

	Ingo

Re: [patch 4/5] [PATCH 4/5] x86: Minor correction to Intel microcode patch loader.

[Peter Oruba]

Ingo Molnar schrieb:
> * Peter Oruba <peter.oruba@amd.com> wrote:
> 
>> Signed-off-by: Peter Oruba <peter.oruba@amd.com>
>> ---
>>  arch/x86/kernel/microcode_intel.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
>> index 4ab6ce3..8d69470 100644
>> --- a/arch/x86/kernel/microcode_intel.c
>> +++ b/arch/x86/kernel/microcode_intel.c
>> @@ -315,7 +315,7 @@ static void apply_microcode(int cpu)
>>  	unsigned long flags;
>>  	unsigned int val[2];
>>  	int cpu_num = raw_smp_processor_id();
>> -	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
>> +	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu_num);
> 
> hm, this does not apply to tip/x86/microcode nor tip/master nor 
> linus/master. Does it fix a bug in one of the earlier patches in this 
> 5/5 queue?
> 
> 	Ingo
> 
Yes, it's a fix for PATCH 3/5

-Peter

-- 
            |           AMD Saxony Limited Liability Company & Co. KG
  Operating |         Wilschdorfer Landstr. 101, 01109 Dresden, Germany
  System    |                  Register Court Dresden: HRA 4896
  Research  |              General Partner authorized to represent:
  Center    |             AMD Saxony LLC (Wilmington, Delaware, US)
            | General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

[patch 5/5] [PATCH 5/5] x86: Run AMD microcode updates via workqueue.

[Peter Oruba]

[-- Attachment #1: 0005-x86-Run-AMD-microcode-updates-via-workqueue.patch --]
[-- Type: text/plain, Size: 5430 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_amd.c |  106 ++++++++++++++++++++++++--------------
 1 files changed, 67 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 33b2a21..37d26ec 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -14,6 +14,7 @@
  *  License version 2. See file COPYING for details.
 */
 
+/* #define DEBUG */ /* pr_debug */
 #include <linux/capability.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -40,7 +41,7 @@
 
 MODULE_DESCRIPTION("AMD Microcode Update Driver");
 MODULE_AUTHOR("Peter Oruba <peter.oruba@amd.com>");
-MODULE_LICENSE("GPL v2");
+MODULE_LICENSE("GPL");
 
 #define UCODE_MAGIC                0x00414d44
 #define UCODE_EQUIV_CPU_TABLE_TYPE 0x00000000
@@ -65,11 +66,16 @@ extern struct mutex (microcode_mutex);
 struct equiv_cpu_entry *equiv_cpu_table;
 
 extern struct ucode_cpu_info ucode_cpu_info[NR_CPUS];
+extern struct ucode_work_info ucode_work_info[NR_CPUS];
 
-static void collect_cpu_info_amd(int cpu)
+static inline struct ucode_cpu_info *get_ucode_cpu_info(int cpu)
+{
+	return &ucode_work_info[cpu].uci;
+}
+
+static void collect_cpu_info_amd(int cpu, struct ucode_cpu_info *uci)
 {
 	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
 	/* We should bind the task to the CPU */
 	BUG_ON(raw_smp_processor_id() != cpu);
@@ -95,7 +101,7 @@ static void collect_cpu_info_amd(int cpu)
 
 static int get_matching_microcode_amd(void *mc, int cpu)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu);
 	struct microcode_header_amd *mc_header = mc;
 	unsigned long total_size = get_totalsize(mc_header);
 	void *new_mc;
@@ -215,7 +221,7 @@ static void apply_microcode_amd(int cpu)
 	unsigned int eax, edx;
 	unsigned int rev;
 	int cpu_num = raw_smp_processor_id();
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
+	struct ucode_cpu_info *uci = get_ucode_cpu_info(cpu_num);
 
 	/* We should bind the task to the CPU */
 	BUG_ON(cpu_num != cpu);
@@ -437,61 +443,83 @@ static int cpu_request_microcode_amd(int cpu)
 	return error;
 }
 
-static int apply_microcode_check_cpu_amd(int cpu)
+static void microcode_fini_cpu_amd(int cpu)
 {
-	struct cpuinfo_x86 *c = &cpu_data(cpu);
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-	unsigned int rev;
-	cpumask_t old;
-	int err = 0;
+	struct ucode_work_info *wi = ucode_work_info + cpu;
 
-	/* Check if the microcode is available */
-	if (!uci->mc.mc_amd)
-		return 0;
+	mutex_lock(&microcode_mutex);
+	wi->uci.valid = 0;
+	vfree(wi->uci.mc.mc_amd);
+	wi->uci.mc.mc_amd = NULL;
+	wi->resume = 0;
+	mutex_unlock(&microcode_mutex);
+}
+
+static int microcode_resume_cpu(int cpu, struct ucode_cpu_info *new_uci)
+{
+	struct ucode_work_info *wi = ucode_work_info + cpu;
+	struct ucode_cpu_info *uci = &wi->uci;
 
-	old = current->cpus_allowed;
-	set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
+	if (!wi->resume)
+		return 0;
 
-	/* Check if the microcode we have in memory matches the CPU */
-	if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 16)
-		err = -EINVAL;
+	pr_debug("microcode: CPU%d resumed (ucode: %p)\n",
+		 cpu, uci->mc.mc_amd);
 
-	if (!err) {
-		asm volatile("movl %1, %%ecx; rdmsr"
-		     : "=a" (rev)
-		     : "i" (0x0000008B) : "ecx");
+	wi->resume = 0;
 
-		if (uci->rev != rev)
-			err = -EINVAL;
-	}
+        /* check if the 'cached' microcode is ok: */
+	if (!uci->mc.mc_amd)
+		return 1;
 
-	if (!err)
+	if (new_uci->sig == uci->sig &&
+	    new_uci->pf  == uci->pf &&
+	    new_uci->rev == uci->rev) {
 		apply_microcode_amd(cpu);
-	else
-		printk(KERN_ERR "microcode: Could not apply microcode to CPU%d:"
-		       " rev=0x%x\n",
-		       cpu,  uci->rev);
+	} else {
+		microcode_fini_cpu_amd(cpu);
+		*uci = *new_uci;
+	}
 
-	set_cpus_allowed(current, old);
-	return err;
+	return 1;
 }
 
-static void microcode_fini_cpu_amd(int cpu)
+static void microcode_update_cpu_amd(struct work_struct *w)
 {
-	struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+	struct ucode_work_info *wi = container_of(w, struct ucode_work_info,
+						  work);
+	int cpu = raw_smp_processor_id(); /* keventd is per-cpu */
+	struct ucode_cpu_info new_uci;
+
+	pr_debug("microcode: CPU%d workqueue\n", cpu);
+
+	memset(&new_uci, 0, sizeof(new_uci));
 
 	mutex_lock(&microcode_mutex);
-	uci->valid = 0;
-	vfree(uci->mc.mc_amd);
-	uci->mc.mc_amd = NULL;
+
+	collect_cpu_info_amd(cpu, &new_uci);
+
+	if (new_uci.valid) {
+		/*
+		 * Check if the system resume is in progress,
+		 * otherwise just request a firmware:
+		 */
+		if (!microcode_resume_cpu(cpu, &new_uci)) {
+			wi->uci = new_uci;
+
+			if (system_state == SYSTEM_RUNNING)
+				cpu_request_microcode_amd(cpu);
+		}
+	}
+
 	mutex_unlock(&microcode_mutex);
 }
 
 static struct microcode_ops microcode_amd_ops = {
 	.get_next_ucode                   = get_next_ucode_amd,
+	.microcode_update_cpu             = microcode_update_cpu_amd,
 	.get_matching_microcode           = get_matching_microcode_amd,
 	.microcode_sanity_check           = NULL,
-	.apply_microcode_check_cpu        = apply_microcode_check_cpu_amd,
 	.cpu_request_microcode            = cpu_request_microcode_amd,
 	.collect_cpu_info                 = collect_cpu_info_amd,
 	.apply_microcode                  = apply_microcode_amd,
-- 
1.5.4.5

Re: [patch 0/5] x86-microcode: run ucode update via workqueue

[Dmitry Adamushko]

Hi Peter,

>
> Rebased Dmitry's patch set to x86-microcode and incorporated changes into AMD microcode patch loader. No issues with
> cpu-hotplug anymore, tested on Intel as well as AMD.

btw., I still think we should consider running ucode-updates from the
low-level arch code. But as it's perhaps a material for .28, there
should be plenty of time. I also have in mind a few possible
generalizations/improvemets.
I'll base them on top of your recent patches.


>
> --Peter
>

-- 
Best regards,
Dmitry Adamushko

Re: [patch 0/5] x86-microcode: run ucode update via workqueue

[Max Krasnyansky]

Dmitry Adamushko wrote:
> Hi Peter,
> 
>> Rebased Dmitry's patch set to x86-microcode and incorporated changes into AMD microcode patch loader. No issues with
>> cpu-hotplug anymore, tested on Intel as well as AMD.
> 
> btw., I still think we should consider running ucode-updates from the
> low-level arch code. But as it's perhaps a material for .28, there
> should be plenty of time. 

That'd be ok. Somehow I think it's not worth the effort though. If the 
machine was able to boot to the point that it can run user-space 
(request_firmware) with the old microcode it can probably sustain itself 
a bit longer while waiting for the workqueue thread to be scheduled.
You probably have more exciting things to do ;-)

The latest patches look ok btw.

Max