Messages in this thread | | | Date | Wed, 30 Jul 2008 20:38:28 +0200 | From | "Dmitry Adamushko" <> | Subject | Re: [patch 0/4] x86: AMD microcode patch loading v2 fixes |
| |
2008/7/30 Max Krasnyansky <maxk@qualcomm.com>: > > > Dmitry Adamushko wrote: >> 2008/7/30 Dmitry Adamushko <dmitry.adamushko@gmail.com>: >>> 2008/7/30 Peter Oruba <peter.oruba@amd.com>: >>>>> [ ... ] >>>> Since ucode updates may fix severe issues, it is supposed to happen as early >>>> as possible. If you re-plug your CPU into your socket, your BIOS also >>>> applies a ucode patch, but that won't necessarily be the latest and critical >>>> one. >>> Hum, let's say we don't do it from cpu-hotplug handlers [1] but from >>> start_secondary() before calling cpu_idle()? [*] >>> >>> This way, we do it before any other task may have a chance to run on a >>> cpu which is not a case with cpu-hotplug handlers >>> (and we don't mess-up with cpu-hotplug events :-) >>> >>> [ the drawback is that 'microcode' subsystem is not local to >>> microcode.c anymore ] >>> >>> [1] if we need a sync. operation in cpu-hotplug handlers and IPI is >>> not ok (say, we need to run in a sleepablel context) then perhaps it's >>> workqueues + wait_on_cpu_work(). But then it's not a bit later than >>> could have been with [*]. >>> >>> heh, this issue has already popped up in another thread so it should >>> be fixed asap, imho. >>> >>> Ingo, Peter? What would be the best way from your pov? >> >> or let's just use smth like a patch below so far: >> >> (non-white-space-damaged version is enclosed) >> >> --- kernel/cpu.c-old 2008-07-30 12:31:15.000000000 +0200 >> +++ kernel/cpu.c 2008-07-30 12:32:02.000000000 +0200 >> @@ -349,6 +349,8 @@ static int __cpuinit _cpu_up(unsigned in >> goto out_notify; >> BUG_ON(!cpu_online(cpu)); >> >> + cpu_set(cpu, cpu_active_map); >> + >> /* Now call notifier in preparation. */ >> raw_notifier_call_chain(&cpu_chain, CPU_ONLINE | mod, hcpu); >> >> @@ -383,9 +385,6 @@ int __cpuinit cpu_up(unsigned int cpu) >> >> err = _cpu_up(cpu, 0); >> >> - if (cpu_online(cpu)) >> - cpu_set(cpu, cpu_active_map); >> - >> out: >> cpu_maps_update_done(); >> return err; > > That was the first thing I thought of when you pointed out what the problem is > (ie when original bug report showed up). > But I immediately rejected the idea because it changes the rules of the game. > active bit is set even before the cpu is "truly" online.
hm, it depends on what is "truly" in this context :-) Tasks (kernel threads) may start running on this 'cpu' as a result of some CPU_ONLINE notifications (CPU_ONLINE notification kind of says "hey, this 'cpu' is online :-)
Sure, If we imagine that some CPU_ONLINE callbacks do additional initialization (e.g. load-balancer related) for 'cpu' and only after their completion the 'cpu' is "really" online (e.g. tasks can be migrated onto it).
I don't have a strong feeling here. I think it's just a matter of specifying the rules/interface.
> > I'd say we fix the microcode instead. >
Yeah, not that this use of set_cpus_allowed_ptr() in hotplug callbacks looks pretty to me (not that I'm saying I have a good taste though :-)
I've even suggested to consider doing microcode update somewhere earlier in start_secondary() (say, right before cpu_idle()). So it'd be done as ealry as possible + we don't mess up with cpu-hotplug events.
> > Max >
-- Best regards, Dmitry Adamushko
| |