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

[Hugh Dickins <hugh@veritas.com>]

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

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

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

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

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

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

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

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

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

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

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

A couple of things that worried me.

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

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

Hugh