[ resending ]
Hi,
this is in response to Mike's patch "Limit the number of microcode
messages".
What's about the following (yet preliminary and not thoroughly tested)
approach?
patch-1:
simplify 'struct ucode_cpu_info' and related functional logic.
patch-2:
reduce a number of similar 'microcode version' messages by printing a
single message for all cpus with equal microcode version, like:
(1)
[ 96.589437] microcode: original microcode versions...
[ 96.589451] microcode: CPU0-1: sig=0x6f2, pf=0x20, revision=0x57
(2)
[ 96.603176] microcode: microcode versions after update...
[ 96.603193] microcode: CPU0-1: sig=0x6f2, pf=0x20, revision=0x57
The new approach is used in microcode_init() [ i.e. when loading a
module ] and microcode_write(), that's when we update all the cpus at
once.
reload_for_cpu() and update-all-cpus-upon-resuming() use the old
approach - a new microcode version is being reported upon applying it.
The latter might employ the similar 'report-for-all' approach as above
but that would somewhat complicate the logic. Anyways, there are plenty
of per-cpu messages upon system resuming so having some more
update-microcode related ones won't harm that muc, I guess :-)
(Not-yet-)signed-off-by: Dmitry Adaushko <[email protected]>
---
diff --git a/arch/x86/include/asm/microcode.h b/arch/x86/include/asm/microcode.h
index ef51b50..68fd54c 100644
--- a/arch/x86/include/asm/microcode.h
+++ b/arch/x86/include/asm/microcode.h
@@ -31,8 +31,6 @@ struct microcode_ops {
};
struct ucode_cpu_info {
- struct cpu_signature cpu_sig;
- int valid;
void *mc;
};
extern struct ucode_cpu_info ucode_cpu_info[];
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 366baa1..c205d37 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -156,8 +156,6 @@ static int apply_microcode_amd(int cpu)
printk(KERN_INFO "microcode: CPU%d: updated (new patch_level=0x%x)\n",
cpu, rev);
- uci->cpu_sig.rev = rev;
-
return 0;
}
@@ -249,14 +247,18 @@ static enum ucode_state
generic_load_microcode(int cpu, const u8 *data, size_t size)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+ struct cpu_signature cpu_sig;
const u8 *ucode_ptr = data;
- void *new_mc = NULL;
- void *mc;
- int new_rev = uci->cpu_sig.rev;
- unsigned int leftover;
+ void *new_mc = NULL, *mc;
+ unsigned int leftover, new_rev;
unsigned long offset;
enum ucode_state state = UCODE_OK;
+ if (collect_cpu_info_amd(cpu, &cpu_sig))
+ return UCODE_ERROR;
+
+ new_rev = cpu_sig.rev;
+
offset = install_equiv_cpu_table(ucode_ptr);
if (!offset) {
printk(KERN_ERR "microcode: failed to create "
@@ -293,7 +295,7 @@ generic_load_microcode(int cpu, const u8 *data, size_t size)
uci->mc = new_mc;
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);
+ cpu, new_rev, cpu_sig.rev);
} else {
vfree(new_mc);
state = UCODE_ERROR;
diff --git a/arch/x86/kernel/microcode_core.c b/arch/x86/kernel/microcode_core.c
index 378e9a8..b7ead3a 100644
--- a/arch/x86/kernel/microcode_core.c
+++ b/arch/x86/kernel/microcode_core.c
@@ -138,20 +138,6 @@ static int collect_cpu_info_on_target(int cpu, struct cpu_signature *cpu_sig)
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;
};
@@ -182,12 +168,8 @@ static int do_microcode_update(const void __user *buf, size_t size)
int cpu;
for_each_online_cpu(cpu) {
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
enum ucode_state ustate;
- if (!uci->valid)
- continue;
-
ustate = microcode_ops->request_microcode_user(cpu, buf, size);
if (ustate == UCODE_ERROR) {
error = -1;
@@ -269,23 +251,16 @@ static struct platform_device *microcode_pdev;
static int reload_for_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- int err = 0;
+ enum ucode_state ustate;
mutex_lock(µcode_mutex);
- if (uci->valid) {
- enum ucode_state ustate;
- ustate = microcode_ops->request_microcode_fw(cpu, µcode_pdev->dev);
- if (ustate == UCODE_OK)
- apply_microcode_on_target(cpu);
- else
- if (ustate == UCODE_ERROR)
- err = -EINVAL;
- }
+ ustate = microcode_ops->request_microcode_fw(cpu, µcode_pdev->dev);
+ if (ustate == UCODE_OK)
+ apply_microcode_on_target(cpu);
mutex_unlock(µcode_mutex);
- return err;
+ return (ustate == UCODE_ERROR)? -EINVAL : 0;
}
static ssize_t reload_store(struct sys_device *dev,
@@ -317,17 +292,23 @@ 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 cpu_signature cpu_sig;
- return sprintf(buf, "0x%x\n", uci->cpu_sig.rev);
+ if (collect_cpu_info_on_target(dev->id, &cpu_sig))
+ return 0;
+
+ return sprintf(buf, "0x%x\n", cpu_sig.rev);
}
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 cpu_signature cpu_sig;
+
+ if (collect_cpu_info_on_target(dev->id, &cpu_sig))
+ return 0;
- return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
+ return sprintf(buf, "0x%x\n", cpu_sig.pf);
}
static SYSDEV_ATTR(reload, 0200, NULL, reload_store);
@@ -348,10 +329,7 @@ static struct attribute_group mc_attr_group = {
static void microcode_fini_cpu(int cpu)
{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
-
microcode_ops->microcode_fini_cpu(cpu);
- uci->valid = 0;
}
static enum ucode_state microcode_resume_cpu(int cpu)
@@ -369,10 +347,10 @@ static enum ucode_state microcode_resume_cpu(int cpu)
static enum ucode_state microcode_init_cpu(int cpu)
{
+ struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
enum ucode_state ustate;
- if (collect_cpu_info(cpu))
- return UCODE_ERROR;
+ memset(uci, 0, sizeof(*uci));
/* --dimm. Trigger a delayed update? */
if (system_state != SYSTEM_RUNNING)
@@ -388,19 +366,6 @@ static enum ucode_state microcode_init_cpu(int cpu)
return ustate;
}
-static enum ucode_state microcode_update_cpu(int cpu)
-{
- struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
- enum ucode_state ustate;
-
- if (uci->valid)
- ustate = microcode_resume_cpu(cpu);
- else
- ustate = microcode_init_cpu(cpu);
-
- return ustate;
-}
-
static int mc_sysdev_add(struct sys_device *sys_dev)
{
int err, cpu = sys_dev->id;
@@ -450,7 +415,7 @@ static int mc_sysdev_resume(struct sys_device *dev)
*/
WARN_ON(cpu != 0);
- if (uci->valid && uci->mc)
+ if (uci->mc)
microcode_ops->apply_microcode(cpu);
return 0;
@@ -472,7 +437,10 @@ mc_cpu_callback(struct notifier_block *nb, unsigned long action, void *hcpu)
switch (action) {
case CPU_ONLINE:
case CPU_ONLINE_FROZEN:
- microcode_update_cpu(cpu);
+ if (action == CPU_ONLINE_FROZEN)
+ microcode_resume_cpu(cpu);
+ else
+ microcode_init_cpu(cpu);
case CPU_DOWN_FAILED:
case CPU_DOWN_FAILED_FROZEN:
pr_debug("microcode: CPU%d added\n", cpu);
diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 0d334dd..6589765 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -339,8 +339,6 @@ static int apply_microcode(int cpu)
mc_intel->hdr.date >> 24,
(mc_intel->hdr.date >> 16) & 0xff);
- uci->cpu_sig.rev = val[1];
-
return 0;
}
@@ -348,11 +346,16 @@ 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;
+ struct cpu_signature cpu_sig;
u8 *ucode_ptr = data, *new_mc = NULL, *mc;
- int new_rev = uci->cpu_sig.rev;
- unsigned int leftover = size;
+ unsigned int leftover = size, new_rev;
enum ucode_state state = UCODE_OK;
+ if (collect_cpu_info(cpu, &cpu_sig))
+ return UCODE_ERROR;
+
+ new_rev = cpu_sig.rev;
+
while (leftover) {
struct microcode_header_intel mc_header;
unsigned int mc_size;
@@ -377,7 +380,7 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,
break;
}
- if (get_matching_microcode(&uci->cpu_sig, mc, new_rev)) {
+ if (get_matching_microcode(&cpu_sig, mc, new_rev)) {
if (new_mc)
vfree(new_mc);
new_rev = mc_header.rev;
@@ -407,7 +410,7 @@ static enum ucode_state 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);
+ cpu, new_rev, cpu_sig.rev);
out:
return state;
}
* dimm <[email protected]> wrote:
> [ resending ]
>
>
> Hi,
>
>
> this is in response to Mike's patch "Limit the number of microcode
> messages".
>
> What's about the following (yet preliminary and not thoroughly tested)
> approach?
>
> patch-1:
>
> simplify 'struct ucode_cpu_info' and related functional logic.
>
>
> patch-2:
>
> reduce a number of similar 'microcode version' messages by printing a
> single message for all cpus with equal microcode version, like:
>
> (1)
> [ 96.589437] microcode: original microcode versions...
> [ 96.589451] microcode: CPU0-1: sig=0x6f2, pf=0x20, revision=0x57
>
> (2)
> [ 96.603176] microcode: microcode versions after update...
> [ 96.603193] microcode: CPU0-1: sig=0x6f2, pf=0x20, revision=0x57
>
>
> The new approach is used in microcode_init() [ i.e. when loading a
> module ] and microcode_write(), that's when we update all the cpus at
> once.
>
> reload_for_cpu() and update-all-cpus-upon-resuming() use the old
> approach - a new microcode version is being reported upon applying it.
>
> The latter might employ the similar 'report-for-all' approach as above
> but that would somewhat complicate the logic. Anyways, there are plenty
> of per-cpu messages upon system resuming so having some more
> update-microcode related ones won't harm that muc, I guess :-)
Seems sensible to me.
Ingo
The patches don't properly work here.
(1) For instance I got following log entries when doing
suspend/resume, doing CPU offline/online test and reloading the
module:
microcode: original microcode versions...
microcode: CPU0-3: patch_level=0x1000065
platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
...
microcode: CPU0-1,3: patch_level=0x1000083
microcode: CPU2-3: patch_level=0x1000065
Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
The patch levels are:
# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000065
PATCH_LEVEL = 0x0000000001000065
(2) During suspend/resume the ucode is not updated:
hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000083
hadburg linux # pm-suspend
hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
PATCH_LEVEL = 0x0000000001000065
PATCH_LEVEL = 0x0000000001000065
PATCH_LEVEL = 0x0000000001000065
PATCH_LEVEL = 0x0000000001000065
That used to work w/o your patches. Didn't have time to look why this
is now failing. You've changed mc_cpu_callback() -- most likely that
is causing this regression.
Regards,
Andreas
2009/11/5 Andreas Herrmann <[email protected]>:
> The patches don't properly work here.
>
> (1) For instance I got following log entries when doing
> ? ?suspend/resume, doing CPU offline/online test and reloading the
> ? ?module:
To avoid possible misunderstandings, I'd like to clarify the output below.
>
> ?microcode: original microcode versions...
> ?microcode: CPU0-3: patch_level=0x1000065
So this is the 1st time you have loaded a module.
> ?platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
> ?...
> ?microcode: CPU0-1,3: patch_level=0x1000083
before or after loading a module? CPU2 is down, isn't it?
>
> ?microcode: CPU2-3: patch_level=0x1000065
same question as above. Here, either CPUs 0 and 1 are down or have a
different version. Both above messages don't make sense taken together
(CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
broken.
>
> ?Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>
> The patch levels are:
>
> ?# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
this is after your test has been stopped and all the CPUs are up, right?
>
> (2) During suspend/resume the ucode is not updated:
>
> ?hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?hadburg linux # pm-suspend
> ?hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
>
>
> That used to work w/o your patches. Didn't have time to look why this
> is now failing. You've changed mc_cpu_callback() -- most likely that
> is causing this regression.
Hmm, cpu-event-callbacks seem to be working on my (Intel) setup. I
have enabled pr_debug messages and also did a little trick to allow
ucode of the same version to be loaded (my cpu is of the recent ucode
by itself) and I can see cpu-callback events for both resuming and
cpu-up cases.
(firstly, upgraded with microcode_ctl as I only have a .dat file)
suspend-resume
...
[ 584.506371] microcode: CPU1 removed
[ 584.516018] microcode: CPU0 updated to revision 0x57, date = 2007-03-15
[ 584.597326] microcode: CPU1 updated upon resume
[ 584.597562] microcode: CPU1 updated to revision 0x57, date = 2007-03-15
[ 584.597565] microcode: CPU1 added
...
and now cpu1 : down -> up
[ 1616.932249] microcode: CPU1 removed
[ 1633.942502] platform microcode: firmware: requesting intel-ucode/06-0f-02
[ 1633.954638] microcode: data file intel-ucode/06-0f-02 load failed
[ 1633.954642] microcode: CPU1 added
as I understand, you don't see " platform microcode: firmware:
requesting intel-ucode" messages upon 'upping' a cpu, do you?
sure, my test is somewhat limited... anyway, first of all I'd like to
get a clear understanding of your logs. Thanks for yout test btw. :-))
>
>
> Regards,
> Andreas
>
-- Dmitry
On Thu, Nov 05, 2009 at 07:40:53PM +0100, Dmitry Adamushko wrote:
> 2009/11/5 Andreas Herrmann <[email protected]>:
> > The patches don't properly work here.
> >
> > (1) For instance I got following log entries when doing
> > ? ?suspend/resume, doing CPU offline/online test and reloading the
> > ? ?module:
>
> To avoid possible misunderstandings, I'd like to clarify the output below.
>
> > ?microcode: original microcode versions...
> > ?microcode: CPU0-3: patch_level=0x1000065
>
> So this is the 1st time you have loaded a module.
>
> > ?platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
> > ?...
> > ?microcode: CPU0-1,3: patch_level=0x1000083
>
> before or after loading a module? CPU2 is down, isn't it?
No, no CPU was offline at this moment. They all were brought back
online after some CPU hotplug and/or suspend/resume tests.
> > ?microcode: CPU2-3: patch_level=0x1000065
Both messages showed up after same ucode-update process.
> same question as above.
Same answer as above all CPUs are online.
> Here, either CPUs 0 and 1 are down or have a
> different version. Both above messages don't make sense taken together
See, and that's the problem.
> (CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
> broken.
I didn't check that yet.
> > ?Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >
> > The patch levels are:
> >
> > ?# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
>
> this is after your test has been stopped and all the CPUs are up, right?
Yes.
> > (2) During suspend/resume the ucode is not updated:
> >
> > ?hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> > ?hadburg linux # pm-suspend
> > ?hadburg linux # for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> > ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> >
> >
> > That used to work w/o your patches. Didn't have time to look why this
> > is now failing. You've changed mc_cpu_callback() -- most likely that
> > is causing this regression.
>
> Hmm, cpu-event-callbacks seem to be working on my (Intel) setup. I
> have enabled pr_debug messages and also did a little trick to allow
> ucode of the same version to be loaded (my cpu is of the recent ucode
> by itself) and I can see cpu-callback events for both resuming and
> cpu-up cases.
>
> (firstly, upgraded with microcode_ctl as I only have a .dat file)
>
> suspend-resume
> ...
> [ 584.506371] microcode: CPU1 removed
> [ 584.516018] microcode: CPU0 updated to revision 0x57, date = 2007-03-15
> [ 584.597326] microcode: CPU1 updated upon resume
> [ 584.597562] microcode: CPU1 updated to revision 0x57, date = 2007-03-15
> [ 584.597565] microcode: CPU1 added
> ...
>
> and now cpu1 : down -> up
>
> [ 1616.932249] microcode: CPU1 removed
> [ 1633.942502] platform microcode: firmware: requesting intel-ucode/06-0f-02
> [ 1633.954638] microcode: data file intel-ucode/06-0f-02 load failed
> [ 1633.954642] microcode: CPU1 added
>
>
> as I understand, you don't see " platform microcode: firmware:
> requesting intel-ucode" messages upon 'upping' a cpu, do you?
Sure, no intel-ucode messages as I tested with AMD CPUs ;-)
But otherwise no, no messages.
> sure, my test is somewhat limited... anyway, first of all I'd like to
> get a clear understanding of your logs. Thanks for yout test btw. :-))
I'll send you full logs asap.
Regards,
Andreas
2009/11/6 Andreas Herrmann <[email protected]>:
>>> [ ... ]
>> > ...
>> > microcode: CPU0-1,3: patch_level=0x1000083
>>
>> before or after loading a module? CPU2 is down, isn't it?
>
> No, no CPU was offline at this moment. They all were brought back
> online after some CPU hotplug and/or suspend/resume tests.
>
>> > microcode: CPU2-3: patch_level=0x1000065
>
> Both messages showed up after same ucode-update process.
>
>> same question as above.
>
> Same answer as above all CPUs are online.
>
>> Here, either CPUs 0 and 1 are down or have a
>> different version. Both above messages don't make sense taken together
>
> See, and that's the problem.
>
>> (CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
>> broken.
>
> I didn't check that yet.
Yeah, this behavior is likely due to a missing cpumask_clear() in
summarize_cpu_info().
should be as follows:
if (!alloc_cpumask_var(&cpulist, GFP_KERNEL))
return;
+ cpumask_clear(cpulist);
>> sure, my test is somewhat limited... anyway, first of all I'd like to
>> get a clear understanding of your logs. Thanks for yout test btw. :-))
>
> I'll send you full logs asap.
Thanks. Maybe it's something about a particular sequence of actions
that triggers this behavior. Or was it reproducible with the very
first pm-suspend invocation after "modprobe microcode.ko"?
>
> Regards,
> Andreas
>
-- Dmitry
On Fri, Nov 06, 2009 at 01:56:31PM +0100, Dmitry Adamushko wrote:
> 2009/11/6 Andreas Herrmann <[email protected]>:
<snip>
> >> (CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
> >> broken.
> >
> > I didn't check that yet.
>
> Yeah, this behavior is likely due to a missing cpumask_clear() in
> summarize_cpu_info().
Yeah, that fixes the wrong messages.
The other problem of not-updated CPU microcode after suspend/resume persists.
> should be as follows:
>
> if (!alloc_cpumask_var(&cpulist, GFP_KERNEL))
> return;
>
> + cpumask_clear(cpulist);
Better use zalloc_cpumask instead of alloc/clear.
> >> sure, my test is somewhat limited... anyway, first of all I'd like to
> >> get a clear understanding of your logs. Thanks for yout test btw. :-))
> >
> > I'll send you full logs asap.
>
> Thanks. Maybe it's something about a particular sequence of actions
> that triggers this behavior. Or was it reproducible with the very
> first pm-suspend invocation after "modprobe microcode.ko"?
The sequence is:
1. loading microcode.ko
2. setting cpu2 offline
3. setting cpu2 online
4. suspend (pm-suspend)
5. resume
microcode of CPU2 is not updated:
# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000083
PATCH_LEVEL = 0x0000000001000065
PATCH_LEVEL = 0x0000000001000083
dmesg attached.
As I've said, that test used to pass with all CPUs updated to new
ucode in the past (at least that I think so ;-( -- but in contrast to
my previous mail this doesn't seem to be related to your patch. I
tested latest mainline and the test fails as well ... seems that I
need to do some debugging.
Regards,
Andreas
PS1: You should remove the needless newline from the patch level string:
static int version_snprintf(char *buf, int len, struct cpu_signature *csig)
{
- return snprintf(buf, len, "patch_level=0x%x\n", csig->rev);
+ return snprintf(buf, len, "patch_level=0x%x", csig->rev);
}
PS2: I plan to remove further needless messages from the amd ucode driver asap.
2009/11/6 Andreas Herrmann <[email protected]>:
> On Fri, Nov 06, 2009 at 01:56:31PM +0100, Dmitry Adamushko wrote:
>> 2009/11/6 Andreas Herrmann <[email protected]>:
>
> ? <snip>
>
>> >> (CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
>> >> broken.
>> >
>> > I didn't check that yet.
>>
>> Yeah, this behavior is likely due to a missing cpumask_clear() in
>> summarize_cpu_info().
>
> Yeah, that fixes the wrong messages.
> The other problem of not-updated CPU microcode after suspend/resume persists.
>
>> should be as follows:
>>
>> ? ? ? if (!alloc_cpumask_var(&cpulist, GFP_KERNEL))
>> ? ? ? ? ? ? ? return;
>>
>> + ? ?cpumask_clear(cpulist);
>
> Better use zalloc_cpumask instead of alloc/clear.
ok.
>
>> >> sure, my test is somewhat limited... anyway, first of all I'd like to
>> >> get a clear understanding of your logs. Thanks for yout test btw. :-))
>> >
>> > I'll send you full logs asap.
>>
>> Thanks. Maybe it's something about a particular sequence of actions
>> that triggers this behavior. Or was it reproducible with the very
>> first pm-suspend invocation after "modprobe microcode.ko"?
>
> The sequence is:
>
> 1. loading microcode.ko
> 2. setting cpu2 offline
> 3. setting cpu2 online
> 4. suspend (pm-suspend)
> 5. resume
>
> microcode of CPU2 is not updated:
>
> ?# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
>
> dmesg attached.
It looks like the microcode of CPU2 was not updated at step (3) [ and
not cached in uci->mc so that there was nothing to be loaded at resume
time later on ].
...
platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
microcode: size 1936, total_size 960
microcode: CPU2: patch mismatch (processor_rev_id: 1020, equiv_cpu_id: 1022)
microcode: size 968, total_size 960
PM: Syncing filesystems ... done.
...
These messages are from ->request_microcode_fw() but then there is
nothing from ->apply_microcode(). I'd expect to see "microcode: CPU%d:
updated (new patch_level=0x%x)".
So either request_fw() -> generic_load_microcode() somehow fails to
find/cache a ucode (while it could do this at microcode-load time) or
apply_microcode_on_target() -> smp_call_function_single() fails in
this context. I made a test (some changes to load a cached ->mc at
cpu-online time) to verify the latter hypothesis and it didn't reveal
any problems or it requires some special conditions (also my kernel is
-rc5+).
>
> As I've said, that test used to pass with all CPUs updated to new
> ucode in the past (at least that I think so ;-( -- but in contrast to
> my previous mail this doesn't seem to be related to your patch. I
> tested latest mainline and the test fails as well ... seems that I
> need to do some debugging.
Ok. Then by instrumenting ->request_microcode_fs() and
apply_microcode_on_target() we would get a hint on what's wrong.
>
>
> Regards,
> Andreas
>
> PS1: You should remove the needless newline from the patch level string:
>
> ?static int version_snprintf(char *buf, int len, struct cpu_signature *csig)
> ?{
> - ? ? ? return snprintf(buf, len, "patch_level=0x%x\n", csig->rev);
> + ? ? ? return snprintf(buf, len, "patch_level=0x%x", csig->rev);
> ?}
ack.
-- Dmitry
2009/11/7 Dmitry Adamushko <[email protected]>:
> 2009/11/6 Andreas Herrmann <[email protected]>:
>> On Fri, Nov 06, 2009 at 01:56:31PM +0100, Dmitry Adamushko wrote:
>>> 2009/11/6 Andreas Herrmann <[email protected]>:
>>
>> ? <snip>
>>
>>> >> (CPU3 belongs to both sets) unless summarize_cpu_info() is utterly
>>> >> broken.
>>> >
>>> > I didn't check that yet.
>>>
>>> Yeah, this behavior is likely due to a missing cpumask_clear() in
>>> summarize_cpu_info().
>>
>> Yeah, that fixes the wrong messages.
>> The other problem of not-updated CPU microcode after suspend/resume persists.
>>
>>> should be as follows:
>>>
>>> ? ? ? if (!alloc_cpumask_var(&cpulist, GFP_KERNEL))
>>> ? ? ? ? ? ? ? return;
>>>
>>> + ? ?cpumask_clear(cpulist);
>>
>> Better use zalloc_cpumask instead of alloc/clear.
>
> ok.
>
>>
>>> >> sure, my test is somewhat limited... anyway, first of all I'd like to
>>> >> get a clear understanding of your logs. Thanks for yout test btw. :-))
>>> >
>>> > I'll send you full logs asap.
>>>
>>> Thanks. Maybe it's something about a particular sequence of actions
>>> that triggers this behavior. Or was it reproducible with the very
>>> first pm-suspend invocation after "modprobe microcode.ko"?
>>
>> The sequence is:
>>
>> 1. loading microcode.ko
>> 2. setting cpu2 offline
>> 3. setting cpu2 online
>> 4. suspend (pm-suspend)
>> 5. resume
>>
>> microcode of CPU2 is not updated:
>>
>> ?# for i in `seq 0 3`; do lsmsr -c $i PATCH_LEVEL; done
>> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
>> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
>> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000065
>> ?PATCH_LEVEL ? ? ? ? ?= 0x0000000001000083
>>
>> dmesg attached.
>
> It looks like the microcode of CPU2 was not updated at step (3) [ and
> not cached in uci->mc so that there was nothing to be loaded at resume
> time later on ].
>
> ...
> platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
> microcode: size 1936, total_size 960
> microcode: CPU2: patch mismatch (processor_rev_id: 1020, equiv_cpu_id: 1022)
> microcode: size 968, total_size 960
> PM: Syncing filesystems ... done.
> ...
>
> These messages are from ->request_microcode_fw() but then there is
> nothing from ->apply_microcode(). I'd expect to see "microcode: CPU%d:
> updated (new patch_level=0x%x)".
>
> So either request_fw() -> generic_load_microcode() somehow fails to
> find/cache a ucode (while it could do this at microcode-load time) or
> apply_microcode_on_target() -> smp_call_function_single() fails in
> this context. I made a test (some changes to load a cached ->mc at
> cpu-online time) to verify the latter hypothesis and it didn't reveal
> any problems or it requires some special conditions (also my kernel is
> -rc5+).
Andreas,
any progress with this issue? You mentioned that the problem is also
reproducible without my patches, right?
--Dmitry
On Wed, Nov 11, 2009 at 05:07:22PM +0100, Dmitry Adamushko wrote:
> Andreas,
>
>
> any progress with this issue?
Yes
> You mentioned that the problem is also reproducible without my
> patches, right?
... and yes.
Fixed with
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=commitdiff;h=9f15226e75583547aaf542c6be4bdac1060dd425
Andreas
-tip testing found the following bug - there's a _long_ boot delay of
58.6 seconds if the CPU family is not supported:
[ 1.421761] calling microcode_init+0x0/0x137 @ 1
[ 1.426532] platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
[ 61.433126] microcode: failed to load file amd-ucode/microcode_amd.bin
[ 61.439682] microcode: CPU0: AMD CPU family 0xf not supported
[ 61.445441] microcode: CPU1: AMD CPU family 0xf not supported
[ 61.451273] Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
[ 61.459116] initcall microcode_init+0x0/0x137 returned 0 after 58625622 usecs
Where does this delay come from?
Ingo
2009/11/12 Ingo Molnar <[email protected]>:
>
> -tip testing found the following bug - there's a _long_ boot delay of
> 58.6 seconds if the CPU family is not supported:
>
> [ 1.421761] calling microcode_init+0x0/0x137 @ 1
> [ 1.426532] platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
> [ 61.433126] microcode: failed to load file amd-ucode/microcode_amd.bin
> [ 61.439682] microcode: CPU0: AMD CPU family 0xf not supported
> [ 61.445441] microcode: CPU1: AMD CPU family 0xf not supported
> [ 61.451273] Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> [ 61.459116] initcall microcode_init+0x0/0x137 returned 0 after 58625622 usecs
>
> Where does this delay come from?
My guess is that it's comming from
static int loading_timeout = 60; /* In seconds */
drivers/base/firmware_class.c
given that you seem to have MICROCODE build in kernel, so this patch
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=commit;h=d1c84f79a6ba992dc01e312c44a21496303874d6
will result in sending a request for a firmware image to user-space
(unless that firmware image is also built-in into the kernel) and
user-space has not started yet.
If so, copying the following block from microcode_cpu_init() into
init_microcode_amd() will help.
/* Trigger a delayed update? */
if (system_state != SYSTEM_RUNNING)
return UCODE_NFOUND;
>
> Ingo
>
-- Dmitry
2009/11/12 Dmitry Adamushko <[email protected]>:
> 2009/11/12 Ingo Molnar <[email protected]>:
>>
>> -tip testing found the following bug - there's a _long_ boot delay of
>> 58.6 seconds if the CPU family is not supported:
>>
>> [ 1.421761] calling microcode_init+0x0/0x137 @ 1
>> [ 1.426532] platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
>> [ 61.433126] microcode: failed to load file amd-ucode/microcode_amd.bin
>> [ 61.439682] microcode: CPU0: AMD CPU family 0xf not supported
>> [ 61.445441] microcode: CPU1: AMD CPU family 0xf not supported
>> [ 61.451273] Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>> [ 61.459116] initcall microcode_init+0x0/0x137 returned 0 after 58625622 usecs
>>
>> Where does this delay come from?
>
> My guess is that it's comming from
>
> static int loading_timeout = 60; /* In seconds */
>
> drivers/base/firmware_class.c
>
> given that you seem to have MICROCODE build in kernel, so this patch
> http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=commit;h=d1c84f79a6ba992dc01e312c44a21496303874d6
>
> will result in sending a request for a firmware image to user-space
> (unless that firmware image is also built-in into the kernel) and
> user-space has not started yet.
btw., it doesn't make sense for request_firmware() to even try this if
the system_state != SYSTEM_RUNNING and current == 'init' (it'd perhaps
make some sense if it's been done in a context of another task -- like
in case of a parallel boot).
And perhaps it just makes sense for microcode to use request_firmware_nowait().
-- Dmitry
On Thu, Nov 12, 2009 at 01:06:36PM +0100, Dmitry Adamushko wrote:
> 2009/11/12 Dmitry Adamushko <[email protected]>:
> > 2009/11/12 Ingo Molnar <[email protected]>:
> >>
> >> -tip testing found the following bug - there's a _long_ boot delay of
> >> 58.6 seconds if the CPU family is not supported:
> >>
> >> [ 1.421761] calling microcode_init+0x0/0x137 @ 1
> >> [ 1.426532] platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
> >> [ 61.433126] microcode: failed to load file amd-ucode/microcode_amd.bin
> >> [ 61.439682] microcode: CPU0: AMD CPU family 0xf not supported
> >> [ 61.445441] microcode: CPU1: AMD CPU family 0xf not supported
> >> [ 61.451273] Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
> >> [ 61.459116] initcall microcode_init+0x0/0x137 returned 0 after 58625622 usecs
> >>
> >> Where does this delay come from?
> >
> > My guess is that it's comming from
> >
> > static int loading_timeout = 60; /* In seconds */
> >
> > drivers/base/firmware_class.c
> >
> > given that you seem to have MICROCODE build in kernel, so this patch
> > http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=commit;h=d1c84f79a6ba992dc01e312c44a21496303874d6
> >
> > will result in sending a request for a firmware image to user-space
> > (unless that firmware image is also built-in into the kernel) and
> > user-space has not started yet.
>
> btw., it doesn't make sense for request_firmware() to even try this if
> the system_state != SYSTEM_RUNNING and current == 'init' (it'd perhaps
> make some sense if it's been done in a context of another task -- like
> in case of a parallel boot).
> And perhaps it just makes sense for microcode to use request_firmware_nowait().
That would be asynchronous.
I think I should ensure that microcode_amd.c is compiled into
microcode.o if and only if its built as module. microcode_amd.c
supports only the firmware interface.
Thus I suggest to add below.
Regards,
Andreas
----
>From 99cd1e170a30ea81164fd13333a5e5bb9587e4e8 Mon Sep 17 00:00:00 2001
From: Andreas Herrmann <[email protected]>
Date: Thu, 12 Nov 2009 16:08:38 +0100
Subject: [PATCH] x86, ucode-amd: Provide it only if microcode is compiled as module
microcode_amd.c supports only the firmware interface. Thus it depends
on the udev firmware helper. As we won't compile the micorode patches
into the kernel it also doesn't make sense to compile microcode_amd.c
into kernel.
This also ensures that loading an updated AMD microcode patch
container file is always possible via
Signed-off-by: Andreas Herrmann <[email protected]>
---
arch/x86/Kconfig | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 17abcfa..0559ca3 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -959,7 +959,7 @@ config MICROCODE_INTEL
config MICROCODE_AMD
bool "AMD microcode patch loading support"
- depends on MICROCODE
+ depends on MICROCODE=m
select FW_LOADER
---help---
If you select this option, microcode patch loading support for AMD
--
1.6.5.2
2009/11/12 Andreas Herrmann <[email protected]>:
> On Thu, Nov 12, 2009 at 01:06:36PM +0100, Dmitry Adamushko wrote:
>> 2009/11/12 Dmitry Adamushko <[email protected]>:
>> > 2009/11/12 Ingo Molnar <[email protected]>:
>> >>
>> >> -tip testing found the following bug - there's a _long_ boot delay of
>> >> 58.6 seconds if the CPU family is not supported:
>> >>
>> >> [ 1.421761] calling microcode_init+0x0/0x137 @ 1
>> >> [ 1.426532] platform microcode: firmware: requesting amd-ucode/microcode_amd.bin
>> >> [ 61.433126] microcode: failed to load file amd-ucode/microcode_amd.bin
>> >> [ 61.439682] microcode: CPU0: AMD CPU family 0xf not supported
>> >> [ 61.445441] microcode: CPU1: AMD CPU family 0xf not supported
>> >> [ 61.451273] Microcode Update Driver: v2.00 <[email protected]>, Peter Oruba
>> >> [ 61.459116] initcall microcode_init+0x0/0x137 returned 0 after 58625622 usecs
>> >>
>> >> Where does this delay come from?
>> >
>> > My guess is that it's comming from
>> >
>> > static int loading_timeout = 60; /* In seconds */
>> >
>> > drivers/base/firmware_class.c
>> >
>> > given that you seem to have MICROCODE build in kernel, so this patch
>> > http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=commit;h=d1c84f79a6ba992dc01e312c44a21496303874d6
>> >
>> > will result in sending a request for a firmware image to user-space
>> > (unless that firmware image is also built-in into the kernel) and
>> > user-space has not started yet.
>>
>> btw., it doesn't make sense for request_firmware() to even try this if
>> the system_state != SYSTEM_RUNNING and current == 'init' (it'd perhaps
>> make some sense if it's been done in a context of another task -- like
>> in case of a parallel boot).
>
>> And perhaps it just makes sense for microcode to use request_firmware_nowait().
>
> That would be asynchronous.
What I had in mind is as follows:
request_firmware_nowait() sends an async request which can be
preserved (and this is an assumption -- I haven't really verified it
yet) until some latter stage when user-space has been started and is
capable of handling (cached) firmware-load requests. I may be (and
perhaps I'm) wrong with the above assumption and the solution is
either never build such a module into the kernel or only do it with
built-in firmware blobs.
-- Dmitry
On Thu, Nov 12, 2009 at 04:48:34PM +0100, Dmitry Adamushko wrote:
> request_firmware_nowait() sends an async request which can be
> preserved (and this is an assumption -- I haven't really verified it
> yet) until some latter stage when user-space has been started and is
> capable of handling (cached) firmware-load requests. I may be (and
> perhaps I'm) wrong with the above assumption and the solution is
> either never build such a module into the kernel or only do it with
> built-in firmware blobs.
I don't think built-in blobs is the way we want to go here - in that
case updating the microcode would require rebuilding the kernel, which
is a clear overkill and exactly the opposite of what we should be doing.
Imagine a big supercomputer consisting of several thousand nodes, all
with identical CPUs. Now, everytime there's microcode patch available,
you have to reboot all those machines after having distributed the
updated kernel images just so that all nodes have their microcode
updated. Many admins would go: "Hmm, no!"
What actually got somehow dropped from Andreas' patch and which we
talked about and agreed upon earlier is that the best thing to do would
be to do
$ rmmod microcode
$ modprobe microcode
after having copied the new ucode patch to /lib/firmware without
disturbing the machine execution.
The async _nowait() version sounds good but in that case you're still
going to need to trigger the microcode update somehow (and AFAIK there's
no mechanism for that yet.) So reloading the module is the easiest thing
and it doesn't need any code changes except the Kconfig oneliner.
Hmm...
--
Regards/Gruss,
Boris.
Operating | Advanced Micro Devices GmbH
System | Karl-Hammerschmidt-Str. 34, 85609 Dornach b. M?nchen, Germany
Research | Gesch?ftsf?hrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis M?nchen
(OSRC) | Registergericht M?nchen, HRB Nr. 43632
... to avoid useless trial to load firmware on systems with
unsupported AMD CPUs.
Signed-off-by: Andreas Herrmann <[email protected]>
---
arch/x86/kernel/microcode_amd.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
Patch is against tip/master.
Regards,
Andreas
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 26e33bd..d0bb5ad 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -34,6 +34,7 @@ MODULE_LICENSE("GPL v2");
#define UCODE_UCODE_TYPE 0x00000001
const struct firmware *firmware;
+static int supported_cpu;
struct equiv_cpu_entry {
u32 installed_cpu;
@@ -73,15 +74,12 @@ static struct equiv_cpu_entry *equiv_cpu_table;
static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
{
- struct cpuinfo_x86 *c = &cpu_data(cpu);
u32 dummy;
- memset(csig, 0, sizeof(*csig));
- if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
- pr_warning("microcode: CPU%d: AMD CPU family 0x%x not "
- "supported\n", cpu, c->x86);
+ if (!supported_cpu)
return -1;
- }
+
+ memset(csig, 0, sizeof(*csig));
rdmsr(MSR_AMD64_PATCH_LEVEL, csig->rev, dummy);
pr_info("microcode: CPU%d: patch_level=0x%x\n", cpu, csig->rev);
return 0;
@@ -331,6 +329,17 @@ static void microcode_fini_cpu_amd(int cpu)
void init_microcode_amd(struct device *device)
{
const char *fw_name = "amd-ucode/microcode_amd.bin";
+ struct cpuinfo_x86 *c = &boot_cpu_data;
+
+ BUG_ON(c->x86_vendor != X86_VENDOR_AMD);
+
+ if (c->x86 < 0x10) {
+ pr_warning("microcode: AMD CPU family 0x%x not supported\n",
+ c->x86);
+ return;
+ }
+ supported_cpu = 1;
+
if (request_firmware(&firmware, fw_name, device))
pr_err("microcode: failed to load file %s\n", fw_name);
}
--
1.6.5.2
Commit-ID: 8cc2361bd00e87aab2827a3996a71fe9b2c9f9c4
Gitweb: http://git.kernel.org/tip/8cc2361bd00e87aab2827a3996a71fe9b2c9f9c4
Author: Andreas Herrmann <[email protected]>
AuthorDate: Tue, 17 Nov 2009 08:06:38 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Tue, 17 Nov 2009 10:10:40 +0100
x86: ucode-amd: Move family check to microcde_amd.c's init function
... to avoid useless trial to load firmware on systems with
unsupported AMD CPUs.
Signed-off-by: Andreas Herrmann <[email protected]>
Cc: Dmitry Adamushko <[email protected]>
Cc: Mike Travis <[email protected]>
Cc: Tigran Aivazian <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Andreas Mohr <[email protected]>
Cc: Jack Steiner <[email protected]>
LKML-Reference: <[email protected]>
[ v2: changed BUG_ON() to WARN_ON() ]
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/microcode_amd.c | 21 +++++++++++++++------
1 files changed, 15 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 26e33bd..63123d9 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -34,6 +34,7 @@ MODULE_LICENSE("GPL v2");
#define UCODE_UCODE_TYPE 0x00000001
const struct firmware *firmware;
+static int supported_cpu;
struct equiv_cpu_entry {
u32 installed_cpu;
@@ -73,15 +74,12 @@ static struct equiv_cpu_entry *equiv_cpu_table;
static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
{
- struct cpuinfo_x86 *c = &cpu_data(cpu);
u32 dummy;
- memset(csig, 0, sizeof(*csig));
- if (c->x86_vendor != X86_VENDOR_AMD || c->x86 < 0x10) {
- pr_warning("microcode: CPU%d: AMD CPU family 0x%x not "
- "supported\n", cpu, c->x86);
+ if (!supported_cpu)
return -1;
- }
+
+ memset(csig, 0, sizeof(*csig));
rdmsr(MSR_AMD64_PATCH_LEVEL, csig->rev, dummy);
pr_info("microcode: CPU%d: patch_level=0x%x\n", cpu, csig->rev);
return 0;
@@ -331,6 +329,17 @@ static void microcode_fini_cpu_amd(int cpu)
void init_microcode_amd(struct device *device)
{
const char *fw_name = "amd-ucode/microcode_amd.bin";
+ struct cpuinfo_x86 *c = &boot_cpu_data;
+
+ WARN_ON(c->x86_vendor != X86_VENDOR_AMD);
+
+ if (c->x86 < 0x10) {
+ pr_warning("microcode: AMD CPU family 0x%x not supported\n",
+ c->x86);
+ return;
+ }
+ supported_cpu = 1;
+
if (request_firmware(&firmware, fw_name, device))
pr_err("microcode: failed to load file %s\n", fw_name);
}