x86: AMD microcode patch loading v2 fixes

Fixed coding style issues.

2008/7/29 Peter Oruba <[email protected]>:

Quoted 1 line

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.

Quoted 16 lines · Dmitry Adamushko

2008/7/29 Peter Oruba <[email protected]>:

Quoted 2 lines

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

Quoted 7 lines

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

Quoted 14 lines · Dmitry Adamushko

2008/7/29 Peter Oruba <[email protected]>:

Quoted 1 line

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

Quoted 7 lines

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

Quoted 1 line

[ ... ]

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

2008/7/30 Peter Oruba <[email protected]>:

Quoted 6 lines

Quoted 1 line

[ ... ]

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?

Quoted 5 lines

Peter

--
Best regards,
Dmitry Adamushko

2008/7/30 Dmitry Adamushko <[email protected]>:

Quoted 21 lines

2008/7/30 Peter Oruba <[email protected]>:

Quoted 5 lines

Quoted 1 line

[ ... ]

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 [*].

Quoted 4 lines

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)

cpu.c 2008-07-30 12:32:02.000000000 +0200 renamed from cpu.c-old 2008-07-30 12:31:15.000000000 +0200 +2 -0

@@ -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);
 

Quoted 46 lines · Dmitry Adamushko

2008/7/30 Dmitry Adamushko <[email protected]>:

Quoted 21 lines

2008/7/30 Peter Oruba <[email protected]>:

Quoted 5 lines

Quoted 1 line

[ ... ]

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 [*].

Quoted 4 lines

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)

cpu.c 2008-07-30 12:32:02.000000000 +0200 renamed from cpu.c-old 2008-07-30 12:31:15.000000000 +0200 +2 -0

@@ -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);
 

cpu.c 2008-07-30 12:32:02.000000000 +0200 int __cpuinit cpu_up(unsigned int cpu) · line 390

Quoted 6 lines

cpu.c 2008-07-30 12:32:02.000000000 +0200 renamed from cpu.c-old 2008-07-30 12:31:15.000000000 +0200 +0 -3

-       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

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.

Quoted 1 line · Dmitry Adamushko

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.

2008/7/30 Peter Oruba <[email protected]>:

Quoted 5 lines

Quoted 1 line

[ ... ]

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 [*].

Quoted 6 lines · Dmitry Adamushko

2008/7/30 Peter Oruba <[email protected]>:

Quoted 5 lines

Quoted 1 line

[ ... ]

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.

Quoted 11 lines

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 ]

Quoted 4 lines

[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/7/30 Max Krasnyansky <[email protected]>:

Quoted 7 lines

Quoted 6 lines · Dmitry Adamushko

2008/7/30 Peter Oruba <[email protected]>:

Quoted 5 lines

Quoted 1 line

[ ... ]

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.

Quoted 2 lines

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 :-)

Quoted 14 lines

Quoted 14 lines

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 [*].

Quoted 4 lines

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

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,

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?

Fixed coding style issues.

Quoted 2 lines

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

Quoted 2 lines

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)?