[rfc-patch, bugfix] x86-microcode

[rfc-patch, bugfix] x86-microcode

[Dmitry Adamushko]

Hi,


[ consider it a pre-release and RFC... I'm a bit in hurry now and just send what I have got by this moment.
Although, I expect it to be workable ]


this change is supposed to fix bug#11197 (note, its name "Oops in microcode sysfs registration" is misleading)

The problem description can be found here:
http://www.ussg.iu.edu/hypermail/linux/kernel/0807.3/3791.html
or
http://lkml.org/lkml/2008/7/24/260

perhaps it does look quite bulky for -rc, although it's mainly move-redesign-some-bits of the code and
I tried to preserve the original logic (even if it looked like a possible optimizations might had been applied)
as much as possible.

The basic idea is that we introduce another mechanism to run ucode-updates on a target cpu
and replace set_cpus_allowed_ptr() in (1) cpu-hotplug events and (2) module load.


[1/2] x86-microcode: generic updates

Basically, it introduces microcode_update_cpu() which can be run either from start_secondary()
(perhaps via a function pointer) or scheduled via keventd ([2/2]) and reworks the logic of cpu-hotplug events.

[2/2] x86-microcode: do updates via workqueue


More testing is necessary. I tested without ucode-package (so only generic machinery) and for
- load/unload module;
- cpu-hotplug (so it doesn't give an oops anymore)

hm, suspend/resume seems to be broken even without the 'microcode' module (will check the date of my previous kernel).


--Dmitry

Re: [rfc-patch, bugfix] x86-microcode

[Max Krasnyansky]

Dmitry Adamushko wrote:
> Hi,
> 
> 
> [ consider it a pre-release and RFC... I'm a bit in hurry now and just send what I have got by this moment.
> Although, I expect it to be workable ]
> 
> 
> this change is supposed to fix bug#11197 (note, its name "Oops in microcode sysfs registration" is misleading)
> 
> The problem description can be found here:
> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.3/3791.html
> or
> http://lkml.org/lkml/2008/7/24/260
> 
> perhaps it does look quite bulky for -rc, although it's mainly move-redesign-some-bits of the code and
> I tried to preserve the original logic (even if it looked like a possible optimizations might had been applied)
> as much as possible.
> 
> The basic idea is that we introduce another mechanism to run ucode-updates on a target cpu
> and replace set_cpus_allowed_ptr() in (1) cpu-hotplug events and (2) module load.
> 
> 
> [1/2] x86-microcode: generic updates
> 
> Basically, it introduces microcode_update_cpu() which can be run either from start_secondary()
> (perhaps via a function pointer) or scheduled via keventd ([2/2]) and reworks the logic of cpu-hotplug events.
> 
> [2/2] x86-microcode: do updates via workqueue

Looks good to me. You did not change the old interface which still does
set_cpus_allowed_ptr() (ie racy against sched_setaffinity()) but it's not in
hotplug path and we can take care of that later.

> More testing is necessary. I tested without ucode-package (so only generic machinery) and for
> - load/unload module;
> - cpu-hotplug (so it doesn't give an oops anymore)
Test here too (dell latitude d620 - core2 duo). I have correct ucode files and
stuff. My cpu does not seem to need any updates so the actual update does not
get applied but everything else (workqueue, request firmware, etc) is working
as expected.

> hm, suspend/resume seems to be broken even without the 'microcode' module (will check the date of my previous kernel).
Works very here.

btw I do not think it's to bulky for -rc. Seems like a very straightforward fixes.

Max

Re: [rfc-patch, bugfix] x86-microcode

[Dmitry Adamushko]

2008/8/4 Max Krasnyansky <maxk@qualcomm.com>:
> Dmitry Adamushko wrote:
>> Hi,
>>
>>
>> [ consider it a pre-release and RFC... I'm a bit in hurry now and just send what I have got by this moment.
>> Although, I expect it to be workable ]
>>
>>
>> this change is supposed to fix bug#11197 (note, its name "Oops in microcode sysfs registration" is misleading)
>>
>> The problem description can be found here:
>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.3/3791.html
>> or
>> http://lkml.org/lkml/2008/7/24/260
>>
>> perhaps it does look quite bulky for -rc, although it's mainly move-redesign-some-bits of the code and
>> I tried to preserve the original logic (even if it looked like a possible optimizations might had been applied)
>> as much as possible.
>>
>> The basic idea is that we introduce another mechanism to run ucode-updates on a target cpu
>> and replace set_cpus_allowed_ptr() in (1) cpu-hotplug events and (2) module load.
>>
>>
>> [1/2] x86-microcode: generic updates
>>
>> Basically, it introduces microcode_update_cpu() which can be run either from start_secondary()
>> (perhaps via a function pointer) or scheduled via keventd ([2/2]) and reworks the logic of cpu-hotplug events.
>>
>> [2/2] x86-microcode: do updates via workqueue
>
> Looks good to me. You did not change the old interface which still does
> set_cpus_allowed_ptr() (ie racy against sched_setaffinity())

ah, right. I've only fixed reload_store() vs. cpu_down().

sched_setaffinity() calls get_online_cpus() so a pair of
cpu_maps_update_begin() + cpu_hotplug_begin() would need to be used by
the following code:

old_mask = p->cpus_allowed;
set_cpus_allowed_ptr(p, new_mask);
// do_something
set_cpus_allowed_ptr(p, old_mask);

or we'd introduce another mutex to be used in both cases...  but I
think we just can get rid of most of use-cases (if not all) by
replacing them with schedule_work_on().


>
> Max
>

-- 
Best regards,
Dmitry Adamushko

Re: [rfc-patch, bugfix] x86-microcode

[Max Krasnyansky]

Dmitry Adamushko wrote:
> 2008/8/4 Max Krasnyansky <maxk@qualcomm.com>:
>> Dmitry Adamushko wrote:
>>> Hi,
>>>
>>>
>>> [ consider it a pre-release and RFC... I'm a bit in hurry now and just send what I have got by this moment.
>>> Although, I expect it to be workable ]
>>>
>>>
>>> this change is supposed to fix bug#11197 (note, its name "Oops in microcode sysfs registration" is misleading)
>>>
>>> The problem description can be found here:
>>> http://www.ussg.iu.edu/hypermail/linux/kernel/0807.3/3791.html
>>> or
>>> http://lkml.org/lkml/2008/7/24/260
>>>
>>> perhaps it does look quite bulky for -rc, although it's mainly move-redesign-some-bits of the code and
>>> I tried to preserve the original logic (even if it looked like a possible optimizations might had been applied)
>>> as much as possible.
>>>
>>> The basic idea is that we introduce another mechanism to run ucode-updates on a target cpu
>>> and replace set_cpus_allowed_ptr() in (1) cpu-hotplug events and (2) module load.
>>>
>>>
>>> [1/2] x86-microcode: generic updates
>>>
>>> Basically, it introduces microcode_update_cpu() which can be run either from start_secondary()
>>> (perhaps via a function pointer) or scheduled via keventd ([2/2]) and reworks the logic of cpu-hotplug events.
>>>
>>> [2/2] x86-microcode: do updates via workqueue
>> Looks good to me. You did not change the old interface which still does
>> set_cpus_allowed_ptr() (ie racy against sched_setaffinity())
> 
> ah, right. I've only fixed reload_store() vs. cpu_down().
> 
> sched_setaffinity() calls get_online_cpus() so a pair of
> cpu_maps_update_begin() + cpu_hotplug_begin() would need to be used by
> the following code:
> 
> old_mask = p->cpus_allowed;
> set_cpus_allowed_ptr(p, new_mask);
> // do_something
> set_cpus_allowed_ptr(p, old_mask);
> 
> or we'd introduce another mutex to be used in both cases...  but I
> think we just can get rid of most of use-cases (if not all) by
> replacing them with schedule_work_on().
Agree. That's I suggested we do it later (ie convert those to use
schedule_work_on()).

Max