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

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

2009/4/24 Dmitry Adamushko <dmitry.adamushko@gmail.com>:
> 2009/4/24 Hugh Dickins <hugh@veritas.com>:
>> On Fri, 24 Apr 2009, Dmitry Adamushko wrote:
>>> 2009/4/23 Hugh Dickins <hugh@veritas.com>:
>>> >
>>> > 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?
>>
>> Good thinking, yes we can and do, unless I'm misinterpreting the
>> evidence.  Though P4 Xeon and Atom startup messages give the opposite
>> impression, claiming to update all cpus from lower revision, more
>> careful tests starting from "maxcpus=1" and then "echo 1 >online"
>> (which, unless you've fiddled around putting the microcode_ctl'ed
>> microcode.dat into /lib/firmware/intel-ucode/wherever, isn't able
>> to update at online time on Intel) shows that the later onlined
>> siblings already have the updated microcode applied to their
>> previously onlined siblings.  Which isn't surprising, but I'd
>> been lulled into thinking the opposite by the startup sequence.
>
> Ah, stupid me :-/ These differences in behavior during the startup and
> the later update reveal a real bug in my patch.
>
> this part:
>
> mutex_lock(&microcode_mutex);
> error = sysdev_driver_register(&cpu_sysdev_class, &mc_sysdev_driver);
> mutex_unlock(&microcode_mutex);
>
> sysdev_driver_register() calls mc_sysdev_driver's ->add() (which is
> mc_sysdev_add()) for each cpu in a loop. Obviously, "microcode_mutex"
> can't help to serialize these calls, oops. A very obvious thing but I
> missed it.

Well, never mind about this one. I might have really been in an
altered state of mind and now need to wait until it comes back to
(let's call it) normality.
All ucode operations are synchronous so obviously we don't need to
serialize actions that run sequentially one after another. Sorry for
the noise here.

But then I wonder why behavior (the fact that all threads seem to
upgrade to a newer version during the startup but they seem to already
be 'up-to-date' if onlined later) during the startup is different...
Too pity that I can't see it with my setups (heh, I perhaps could play
with it by actually downgrading cpus to older ucode).


-- 
Best regards,
Dmitry Adamushko