Re: [ RFC, PATCH - 1/2, v2 ] x86-microcode: refactor microcode output messages

[Dmitry Adamushko <dmitry.adamushko@gmail.com>]

2009/11/6 Andreas Herrmann <herrmann.der.user@googlemail.com>:
> On Fri, Nov 06, 2009 at 01:56:31PM +0100, Dmitry Adamushko wrote:
>> 2009/11/6 Andreas Herrmann <herrmann.der.user@googlemail.com>:
>
>   <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