Re: [patch 0/4] x86: AMD microcode patch loading v2 fixes

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

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