Messages in this thread | | | Date | Wed, 30 Jul 2008 22:59:57 +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 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. > Sure. The question is would not workqueue be soon enough ? > I'd say it is given the non-deterministic CPU hotplug callback sequence.
Max, cpu-hotplug callbacks might have been not the best choice in the first place. So a comparison with them is not that relevant :-)
> >> 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 [*]. > Why would not IPI be ok ? From looking at the code all we have to do is to > factor request_firmware() out of the update path. So we'd do > collect_cpu_info() in the IPI, then do request_firwmare() inplace and then do > apply_microcode() in the IPI. ie The only thing that sleeps is request_firmware().
I think it's quite a complecated scheme. I still wonder whether e.g. start_secondary() - cpu_idle() would be a better place or we just move set_cpu(cpu, cpu_active_map) a bit :^)
But you know, at least short-term, it'd be nice if whoever might come up with any working solution. It's already -rc1 and this thing is still broken ;-)
btw., I've greped for "set_cpus_allowed_ptr()" and the following scheme seems to be quite wide-spread (didn't check all of them so maybe someone else does call it from cpu-hotplug notifications, heh)
cpus_allowed = current->cpus_allowed; set_cpus_allowed_ptr(current, cpus); // do_something set_cpus_allowed_ptr(current, &cpus_allowed);
but _not_ safely used indeed. argh
> > Max >
-- Best regards, Dmitry Adamushko
| |