Skip to contentSkip to conversation

x86-microcode: run ucode update via workqueue

PATCH In review Peter Oruba 5 participants 18 messages Aug 6, 2008 – Aug 12, 2008 x86

lore
Share & tools
lore
#0 lore raw

cover x86-microcode: run ucode update via workqueue

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

#1 lore raw

PATCH 1/5 x86: microcode generic updates

---
 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(-)
arch/x86/kernel/microcode.c +37 -46
index ad136ad..b797692 100644
@@ -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;
 }
arch/x86/kernel/microcode_intel.c +50 -47

130 lines not on this page · raw message

include/asm-x86/microcode.h +14 -13
index 18b2aee..072bef1 100644
@@ -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);
+};
Signature · 1 line
1.5.4.5
#3 lore raw

PATCH 2/5 x86: Minor correction to header file

In reply to Peter Oruba
---
 include/asm-x86/microcode.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
include/asm-x86/microcode.h +2 -2
index 072bef1..fb08022 100644
@@ -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;
Signature · 1 line
1.5.4.5
#4 lore raw

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

Quoted 10 lines · Peter Oruba
---
 include/asm-x86/microcode.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
include/asm-x86/microcode.h +0 -0
index 072bef1..fb08022 100644

include/asm-x86/microcode.h line 2

Quoted 4 lines
include/asm-x86/microcode.h +2 -2
@@ -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

#5 lore raw

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

In reply to Peter Oruba
---
 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(-)
arch/x86/kernel/microcode.c +28 -21
index b797692..0264c76 100644
@@ -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();
arch/x86/kernel/microcode_intel.c +45 -29

132 lines not on this page · raw message

include/asm-x86/microcode.h +7 -2
index fb08022..69388fa 100644
@@ -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);
Signature · 1 line
1.5.4.5
#6 lore raw

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

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <[email protected]> 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)

Signature · 3 lines
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
#7 lore raw

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

2008/8/6 Arjan van de Ven <[email protected]>:

Quoted 6 lines

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <[email protected]> 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).

Quoted 3 lines

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.

Quoted 2 lines

(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().

Signature · 2 lines
Best regards,
Dmitry Adamushko
#9 lore raw

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

In reply to Arjan van de Ven
Quoted 11 lines · Arjan van de Ven

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <[email protected]> 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

#10 lore raw

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

Quoted 6 lines · On Wed, 06 Aug 2008 13:31:18 -0700 Max Krasnyansky <[email protected]> wrote:
Quoted 5 lines · Arjan van de Ven

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <[email protected]> wrote:

[ no description or reason ]

Quoted 8 lines
Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

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..

Quoted 2 lines

Max

Signature · 3 lines
If you want to reach me at my work email, use [email protected]
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org
#11 lore raw

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

In reply to Arjan van de Ven
Quoted 11 lines · Arjan van de Ven

On Wed, 6 Aug 2008 17:21:20 +0200
Peter Oruba <[email protected]> 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

#12 lore raw

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

In reply to Peter Oruba
---
 arch/x86/kernel/microcode_intel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/microcode_intel.c +1 -1
index 4ab6ce3..8d69470 100644
@@ -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);
Signature · 1 line
1.5.4.5
#13 lore raw

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

Quoted 10 lines · Peter Oruba
---
 arch/x86/kernel/microcode_intel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/microcode_intel.c +0 -0
index 4ab6ce3..8d69470 100644

arch/x86/kernel/microcode_intel.c static void apply_microcode(int cpu) · line 318

Quoted 5 lines
arch/x86/kernel/microcode_intel.c +1 -1
@@ -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

#14 lore raw

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

Quoted 20 lines · Ingo Molnar
Quoted 10 lines · Peter Oruba
---
 arch/x86/kernel/microcode_intel.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
arch/x86/kernel/microcode_intel.c +0 -0
index 4ab6ce3..8d69470 100644

arch/x86/kernel/microcode_intel.c static void apply_microcode(int cpu) · line 318

Quoted 5 lines
arch/x86/kernel/microcode_intel.c +1 -1
@@ -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?

Quoted 2 lines

Ingo

Yes, it's a fix for PATCH 3/5

-Peter

Signature · 6 lines
           |           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
#15 lore raw

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

In reply to Peter Oruba
---
 arch/x86/kernel/microcode_amd.c |  106 ++++++++++++++++++++++++--------------
 1 files changed, 67 insertions(+), 39 deletions(-)
arch/x86/kernel/microcode_amd.c +67 -39
index 33b2a21..37d26ec 100644
@@ -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 <[email protected]>");
-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,
Signature · 1 line
1.5.4.5
#16 lore raw
In reply to Peter Oruba

Hi Peter,

Quoted 3 lines

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.

Quoted 3 lines

--Peter

Signature · 2 lines
Best regards,
Dmitry Adamushko
#17 lore raw
Quoted 8 lines · Dmitry Adamushko

Hi Peter,

Quoted 2 lines

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