2008-07-29 15:44:34

by Peter Oruba

[permalink] [raw]
Subject: [patch 0/4] x86: AMD microcode patch loading v2 fixes

Fixed coding style issues.




2008-07-29 16:18:35

by Dmitry Adamushko

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

2008/7/29 Peter Oruba <[email protected]>:
> Fixed coding style issues.

I have a comment on the abstraction layer (microcode_ops).

[ Not that I've looked very carefully at it so far, nor I pretend to
be at-ease with this 'microcode' topic to make any design judgements
:-) ]

but would it be somehow possible to not have set_cpus_allowed_ptr()
code in arch-dependent parts? Let's say the mechanism of how to run
certain arch-specific code (and synchronization) on a given cpu should
be a prerogative of (and placed in) the generic part...

Note, this code will likely happily give you an oops if you run
cpu_down/up() ;-)

I also wondered, is there a requirement that when a new cpu is brought
up, microcode updates {should,must} be done as early as possible, say
before any tasks have a chance to run on it? Or can the update be a
bit delayed? e.g. we don't do it from cpu-hotplug handlers.


--
Best regards,
Dmitry Adamushko

2008-07-29 17:49:27

by Max Krasnyansky

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

Dmitry Adamushko wrote:
> 2008/7/29 Peter Oruba <[email protected]>:
>> Fixed coding style issues.
>
> I have a comment on the abstraction layer (microcode_ops).
>
> [ Not that I've looked very carefully at it so far, nor I pretend to
> be at-ease with this 'microcode' topic to make any design judgements
> :-) ]
>
> but would it be somehow possible to not have set_cpus_allowed_ptr()
> code in arch-dependent parts? Let's say the mechanism of how to run
> certain arch-specific code (and synchronization) on a given cpu should
> be a prerogative of (and placed in) the generic part...
>
> Note, this code will likely happily give you an oops if you run
> cpu_down/up() ;-)
>
> I also wondered, is there a requirement that when a new cpu is brought
> up, microcode updates {should,must} be done as early as possible, say
> before any tasks have a chance to run on it? Or can the update be a
> bit delayed? e.g. we don't do it from cpu-hotplug handlers.

Dmitry looks like you missed my email. I already asked that question and
proposed a couple of options (workqueue and ipi).
Tigran said that he does not know. Maybe Peter does.

My guess is that it does not have to be done synchronously and can be
delayed. There is no guaranty that microcode CPU hotplug handler will
run before all the other handlers. Which by definition means that some
things will start running on the cpu before the microcode is updated.
Hence we might as well do the update outside of the cpu hotplug path.

Max

2008-07-29 19:37:56

by Dmitry Adamushko

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

2008/7/29 Max Krasnyansky <[email protected]>:
> Dmitry Adamushko wrote:
>>
>> 2008/7/29 Peter Oruba <[email protected]>:
>>>
>>> Fixed coding style issues.
>>
>> I have a comment on the abstraction layer (microcode_ops).
>>
>> [ Not that I've looked very carefully at it so far, nor I pretend to
>> be at-ease with this 'microcode' topic to make any design judgements
>> :-) ]
>>
>> but would it be somehow possible to not have set_cpus_allowed_ptr()
>> code in arch-dependent parts? Let's say the mechanism of how to run
>> certain arch-specific code (and synchronization) on a given cpu should
>> be a prerogative of (and placed in) the generic part...
>>
>> Note, this code will likely happily give you an oops if you run
>> cpu_down/up() ;-)
>>
>> I also wondered, is there a requirement that when a new cpu is brought
>> up, microcode updates {should,must} be done as early as possible, say
>> before any tasks have a chance to run on it? Or can the update be a
>> bit delayed? e.g. we don't do it from cpu-hotplug handlers.
>
> Dmitry looks like you missed my email. I already asked that question and
> proposed a couple of options (workqueue and ipi).
> Tigran said that he does not know. Maybe Peter does.
>
> My guess is that it does not have to be done synchronously and can be
> delayed. There is no guaranty that microcode CPU hotplug handler will run
> before all the other handlers. Which by definition means that some things
> will start running on the cpu before the microcode is updated.

yes.

> Hence we
> might as well do the update outside of the cpu hotplug path.

but it doesn't necessarily mean that it was correct before (I mean,
maybe some assumptions were made about cpu-hotplug handlers :-)



>
> Max
>

--
Best regards,
Dmitry Adamushko

2008-07-30 09:28:48

by Peter Oruba

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


Max Krasnyansky schrieb:
> Dmitry Adamushko wrote:
>> 2008/7/29 Peter Oruba <[email protected]>:
>>> Fixed coding style issues.
>>
>> I have a comment on the abstraction layer (microcode_ops).
>>
>> [ Not that I've looked very carefully at it so far, nor I pretend to
>> be at-ease with this 'microcode' topic to make any design judgements
>> :-) ]
>>
>> but would it be somehow possible to not have set_cpus_allowed_ptr()
>> code in arch-dependent parts? Let's say the mechanism of how to run
>> certain arch-specific code (and synchronization) on a given cpu should
>> be a prerogative of (and placed in) the generic part...
>>
>> Note, this code will likely happily give you an oops if you run
>> cpu_down/up() ;-)
>>
>> I also wondered, is there a requirement that when a new cpu is brought
>> up, microcode updates {should,must} be done as early as possible, say
>> before any tasks have a chance to run on it? Or can the update be a
>> bit delayed? e.g. we don't do it from cpu-hotplug handlers.
>
> Dmitry looks like you missed my email. I already asked that question and
> proposed a couple of options (workqueue and ipi).
> Tigran said that he does not know. Maybe Peter does.
>
> My guess is that it does not have to be done synchronously and can be
> delayed. There is no guaranty that microcode CPU hotplug handler will
> run before all the other handlers. Which by definition means that some
> things will start running on the cpu before the microcode is updated.
> Hence we might as well do the update outside of the cpu hotplug path.
>
> Max
>
>
>
>

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.

Peter

--
| AMD Saxony Limited Liability Company & Co. KG
Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
System | Register Court Dresden: HRA 4896
Research | General Partner authorized to represent:
Center | AMD Saxony LLC (Wilmington, Delaware, US)
| General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

2008-07-30 09:57:34

by Dmitry Adamushko

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

2008/7/30 Peter Oruba <[email protected]>:
>> [ ... ]
>
> 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?


>
> Peter
>

--
Best regards,
Dmitry Adamushko

2008-07-30 10:34:17

by Dmitry Adamushko

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

2008/7/30 Dmitry Adamushko <[email protected]>:
> 2008/7/30 Peter Oruba <[email protected]>:
>>> [ ... ]
>>
>> 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;


>
>
>>
>> Peter
>>
>
> --
> Best regards,
> Dmitry Adamushko
>



--
Best regards,
Dmitry Adamushko


Attachments:
(No filename) (1.94 kB)
move-cpu_set-cpu_active_map.patch (554.00 B)
Download all attachments

2008-07-30 16:35:28

by Max Krasnyansky

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



Dmitry Adamushko wrote:
> 2008/7/30 Dmitry Adamushko <[email protected]>:
>> 2008/7/30 Peter Oruba <[email protected]>:
>>>> [ ... ]
>>> 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. I'd say we fix the
microcode instead.

Max


2008-07-30 16:54:22

by Max Krasnyansky

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



Dmitry Adamushko wrote:
> 2008/7/30 Peter Oruba <[email protected]>:
>>> [ ... ]
>> 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.

> 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().

Max

2008-07-30 18:38:42

by Dmitry Adamushko

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

2008/7/30 Max Krasnyansky <[email protected]>:
>
>
> Dmitry Adamushko wrote:
>> 2008/7/30 Dmitry Adamushko <[email protected]>:
>>> 2008/7/30 Peter Oruba <[email protected]>:
>>>>> [ ... ]
>>>> 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

2008-07-30 21:00:13

by Dmitry Adamushko

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

2008/7/30 Max Krasnyansky <[email protected]>:
>
> Dmitry Adamushko wrote:
>> 2008/7/30 Peter Oruba <[email protected]>:
>>>> [ ... ]
>>> 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

2008-07-31 21:25:20

by Ingo Molnar

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


* Dmitry Adamushko <[email protected]> wrote:

> 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.

i have no problems with that approach in principle. Microcode updates
are special and black magic things anyway.

Ingo

2008-07-31 21:27:29

by Ingo Molnar

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


* Peter Oruba <[email protected]> wrote:

> Fixed coding style issues.

applied to tip/x86/microcode - thanks Peter.

Will you take a shot at moving the driver to early init (and early SMP
init) code, as suggested by Dmitry?

Ingo

2008-07-31 22:02:39

by Dmitry Adamushko

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

2008/7/31 Ingo Molnar <[email protected]>:
>
> * Peter Oruba <[email protected]> wrote:
>
>> Fixed coding style issues.
>
> applied to tip/x86/microcode - thanks Peter.
>
> Will you take a shot at moving the driver to early init (and early SMP
> init) code, as suggested by Dmitry?

I have another fix for 'microcode' (one place may race vs.
sched_setaffinity()) and a few ideas/improvements to try. I would also
do this 'move-cpu-hotplug-activities-to-early-init' thing (but
perhaps, not earlier than on Saturday-Sunday) if Peter doesn't come up
with a patch by this time.

btw., is tip/x86/microcode a .27 material? I mean, if I do some work,
should I do it on top of the current linus-git or -tip (or both)?


>
> Ingo
>

--
Best regards,
Dmitry Adamushko

2008-07-31 22:12:35

by Ingo Molnar

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


* Dmitry Adamushko <[email protected]> wrote:

> > Will you take a shot at moving the driver to early init (and early
> > SMP init) code, as suggested by Dmitry?
>
> I have another fix for 'microcode' (one place may race vs.
> sched_setaffinity()) and a few ideas/improvements to try. I would also
> do this 'move-cpu-hotplug-activities-to-early-init' thing (but
> perhaps, not earlier than on Saturday-Sunday) if Peter doesn't come up
> with a patch by this time.
>
> btw., is tip/x86/microcode a .27 material? I mean, if I do some work,
> should I do it on top of the current linus-git or -tip (or both)?

it's not really .27 material, it was recently started. So for fixes i'd
suggest as-small-as-possible patches against -git. We'll sort out the
x86/microcode impact of that.

Ingo

2008-08-01 02:18:32

by Max Krasnyansky

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

Dmitry Adamushko wrote:
> 2008/7/30 Max Krasnyansky <[email protected]>:
>> Dmitry Adamushko wrote:
>>> 2008/7/30 Peter Oruba <[email protected]>:
>>>>> [ ... ]
>>>> 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 :-)

The reason I thought it's relevant is "hey it has worked before" :)
I mean it looks like people were happy with updating microcode from
hotplug. Also the original interface was driven entirely by the
userspace which tells me that timing of the microcode update was not
considered critical.

>>> 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 :^)
Sure. I'm ok with start_secondary() or whatever I was just saying that
IPI would work and yes maybe it's a bit more complicated.
btw I still think workqueue would work just fine.

> 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 ;-)
Agree. I was going to implement/test workqueue based solution but did/do
not have spare cycles (24x7 in the lab these days).

> 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
Uh, that's not good. We need to fix all that. I can think of a bunch of
interesting races. Like adding a process to a cpuset while it was doing
that "something" above.

Max


Max


Max

2008-08-01 02:23:20

by Max Krasnyansky

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

Ingo Molnar wrote:
> * Dmitry Adamushko <[email protected]> wrote:
>
>> 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.
>
> i have no problems with that approach in principle. Microcode updates
> are special and black magic things anyway.
>

I still think we should just do the update via schedule_work_on() and be
done with it. Maybe in theory it should be done as early as possible but
in practice hotplug handler -> workqueue will be soon enough, I think.

Max

2008-08-01 11:25:36

by Dmitry Adamushko

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

Tigran, Peter,


may a firmware package contain a few 'microcode' updates for a specific cpu?

And if so, does each of them provide independent 'errata' fixes? [*]

(or they are just different versions of the same self-consistent/full
'microcode' update and we may need to apply each of them just e.g.
because we can't jump from stepping X.1 to X.3 without applying X.2 in
between?

if it's [1], then I wonder why only a single 'microcode' update (which
has been previously cached in 'uci->mc') is being applied for the case
of system-wide resume (apply_microcode_check_cpu()). Don't we need to
go through the full cpu_request_microcode() cycle to consider all
updates?


TIA,

--
Best regards,
Dmitry Adamushko

2008-08-01 12:21:55

by Peter Oruba

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

Dmitry Adamushko schrieb:
> Tigran, Peter,
>
>
> may a firmware package contain a few 'microcode' updates for a specific cpu?
>
> And if so, does each of them provide independent 'errata' fixes? [*]
>
> (or they are just different versions of the same self-consistent/full
> 'microcode' update and we may need to apply each of them just e.g.
> because we can't jump from stepping X.1 to X.3 without applying X.2 in
> between?
>
> if it's [1], then I wonder why only a single 'microcode' update (which
> has been previously cached in 'uci->mc') is being applied for the case
> of system-wide resume (apply_microcode_check_cpu()). Don't we need to
> go through the full cpu_request_microcode() cycle to consider all
> updates?
>
>
> TIA,
>

Dmitry,

to answer your question for the AMD part: ucode patches are always
combo-patches. A ucode patch for a particular CPU revision solves all targeted
issues at once. There is no case in which two or more ucode patches would be
applied to a CPU.

Peter

--
| AMD Saxony Limited Liability Company & Co. KG
Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
System | Register Court Dresden: HRA 4896
Research | General Partner authorized to represent:
Center | AMD Saxony LLC (Wilmington, Delaware, US)
| General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy

2008-08-01 15:32:16

by H. Peter Anvin

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

Dmitry Adamushko wrote:
> Tigran, Peter,
>
> may a firmware package contain a few 'microcode' updates for a specific cpu?
>
> And if so, does each of them provide independent 'errata' fixes? [*]
>
> (or they are just different versions of the same self-consistent/full
> 'microcode' update and we may need to apply each of them just e.g.
> because we can't jump from stepping X.1 to X.3 without applying X.2 in
> between?
>
> if it's [1], then I wonder why only a single 'microcode' update (which
> has been previously cached in 'uci->mc') is being applied for the case
> of system-wide resume (apply_microcode_check_cpu()). Don't we need to
> go through the full cpu_request_microcode() cycle to consider all
> updates?
>

No, there is only ever one microcode; you only run the latest one.

-hpa