Messages in this thread | | | Date | Fri, 24 Apr 2009 00:16:35 +0200 | Subject | Re: [PATCH] x86 microcode: work_on_cpu and cleanup of the synchronization logic | From | Dmitry Adamushko <> |
| |
2009/4/23 Hugh Dickins <hugh@veritas.com>: > On Thu, 23 Apr 2009, Dmitry Adamushko wrote: >> [ ... ] >> - 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.
Thanks for the tests and review!
> [ ... ] > > 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.
Yeah. Then at least for the sake of consistency it makes sense to consider doing some kind of delayed update for CPU0 in this case. Will check.
>> >> 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?
Yes, *msr() calls are only done from functions that are now being called via smp_call_function_single(). The later seems to always do it with disabled interrupts. The only exception is mc_sysdev_resume() calling ->apply_microcode() directly but this one in turn is always called with disabled interrupts.
But now that you mentioned it I wonder if we may actually need a spinlock there... can we have multi-threaded cpus/cores with (all | some) shared msr registers?
> 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.
Indeed, will fix. Thanks!
> > Hugh >
-- Best regards, Dmitry Adamushko
| |