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

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

[Peter Oruba]

Fixed coding style issues.

[patch 1/4] x86: AMD microcode patch loader style corrections.

[Peter Oruba]

[-- Attachment #1: 0001-x86-AMD-microcode-patch-loader-style-corrections.patch --]
[-- Type: text/plain, Size: 1005 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_amd.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 07e52be..6789530 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -47,9 +47,9 @@ MODULE_LICENSE("GPL v2");
 #define UCODE_UCODE_TYPE           0x00000001
 
 #define UCODE_MAX_SIZE          (2048)
-#define DEFAULT_UCODE_DATASIZE	(896)	  /* 896 bytes */
-#define MC_HEADER_SIZE		(sizeof(struct microcode_header_amd))	  /* 64 bytes */
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE) /* 960 bytes */
+#define DEFAULT_UCODE_DATASIZE	(896)
+#define MC_HEADER_SIZE		(sizeof(struct microcode_header_amd))
+#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
 #define DWSIZE			(sizeof(u32))
 /* For now we support a fixed ucode total size only */
 #define get_totalsize(mc) \
-- 
1.5.4.5

[patch 2/4] x86: Intel microcode patch loader style corrections.

[Peter Oruba]

[-- Attachment #1: 0002-x86-Intel-microcode-patch-loader-style-corrections.patch --]
[-- Type: text/plain, Size: 2111 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_intel.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/microcode_intel.c b/arch/x86/kernel/microcode_intel.c
index 6da4a85..a61986e 100644
--- a/arch/x86/kernel/microcode_intel.c
+++ b/arch/x86/kernel/microcode_intel.c
@@ -55,8 +55,8 @@
  *		in a single CPU package.
  *	1.10	28 Feb 2002 Asit K Mallick <asit.k.mallick@intel.com> and
  *		Tigran Aivazian <tigran@veritas.com>,
- *		Serialize updates as required on HT processors due to speculative
- *		nature of implementation.
+ *		Serialize updates as required on HT processors due to
+ *		speculative nature of implementation.
  *	1.11	22 Mar 2002 Tigran Aivazian <tigran@veritas.com>
  *		Fix the panic when writing zero-length microcode chunk.
  *	1.12	29 Sep 2003 Nitin Kamble <nitin.a.kamble@intel.com>,
@@ -71,7 +71,7 @@
  *		Thanks to Stuart Swales for pointing out this bug.
  */
 
-//#define DEBUG /* pr_debug */
+/* #define DEBUG */ /* pr_debug */
 #include <linux/capability.h>
 #include <linux/kernel.h>
 #include <linux/init.h>
@@ -99,11 +99,11 @@ MODULE_DESCRIPTION("Microcode Update Driver");
 MODULE_AUTHOR("Tigran Aivazian <tigran@aivazian.fsnet.co.uk>");
 MODULE_LICENSE("GPL");
 
-#define DEFAULT_UCODE_DATASIZE 	(2000) 	  /* 2000 bytes */
-#define MC_HEADER_SIZE		(sizeof(struct microcode_header_intel)) /* 48 bytes */
-#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE) /* 2048 bytes */
-#define EXT_HEADER_SIZE		(sizeof(struct extended_sigtable)) /* 20 bytes */
-#define EXT_SIGNATURE_SIZE	(sizeof(struct extended_signature)) /* 12 bytes */
+#define DEFAULT_UCODE_DATASIZE 	(2000)
+#define MC_HEADER_SIZE		(sizeof(struct microcode_header_intel))
+#define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
+#define EXT_HEADER_SIZE		(sizeof(struct extended_sigtable))
+#define EXT_SIGNATURE_SIZE	(sizeof(struct extended_signature))
 #define DWSIZE			(sizeof(u32))
 #define get_totalsize(mc) \
 	(((struct microcode_intel *)mc)->hdr.totalsize ? \
-- 
1.5.4.5

[patch 3/4] x86: Moved function declarations out from AMD microcode patch loader to heade file.

[Peter Oruba]

[-- Attachment #1: 0003-x86-Moved-function-declarations-out-from-AMD-microc.patch --]
[-- Type: text/plain, Size: 678 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_amd.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 6789530..2b98e6a 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -56,9 +56,6 @@ MODULE_LICENSE("GPL v2");
 	((((struct microcode_amd *)mc)->hdr.mc_patch_data_len * 28) \
 	 + MC_HEADER_SIZE)
 
-extern int microcode_init(void *opaque, struct module *module);
-extern void microcode_exit(void);
-
 /* serialize access to the physical write */
 static DEFINE_SPINLOCK(microcode_update_lock);
 
-- 
1.5.4.5

[patch 4/4] x86: Minor pointer type cast in AMD microcode patch loader.

[Peter Oruba]

[-- Attachment #1: 0004-x86-Minor-pointer-type-cast-in-AMD-microcode-patch.patch --]
[-- Type: text/plain, Size: 610 bytes --]

Signed-off-by: Peter Oruba <peter.oruba@amd.com>
---
 arch/x86/kernel/microcode_amd.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 2b98e6a..33b2a21 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -394,7 +394,8 @@ static int cpu_request_microcode_amd(int cpu)
 		return error;
 	}
 
-	buf_pos = buf = firmware->data;
+	buf_pos = (unsigned int *)firmware->data;
+	buf = (void *)firmware->data;
 	size = firmware->size;
 
 	if (buf_pos[0] != UCODE_MAGIC) {
-- 
1.5.4.5

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

[Dmitry Adamushko]

2008/7/29 Peter Oruba <peter.oruba@amd.com>:
> 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

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

[Max Krasnyansky]

Dmitry Adamushko wrote:
> 2008/7/29 Peter Oruba <peter.oruba@amd.com>:
>> 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

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

[Dmitry Adamushko]

2008/7/29 Max Krasnyansky <maxk@qualcomm.com>:
> Dmitry Adamushko wrote:
>>
>> 2008/7/29 Peter Oruba <peter.oruba@amd.com>:
>>>
>>> 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

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

[Peter Oruba]

Max Krasnyansky schrieb:
> Dmitry Adamushko wrote:
>> 2008/7/29 Peter Oruba <peter.oruba@amd.com>:
>>> 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

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

[Dmitry Adamushko]

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?


>
> Peter
>

-- 
Best regards,
Dmitry Adamushko

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

[Dmitry Adamushko]

[-- Attachment #1: Type: text/plain, Size: 1986 bytes --]

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;


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



-- 
Best regards,
Dmitry Adamushko

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: move-cpu_set-cpu_active_map.patch --]
[-- Type: text/x-patch; name=move-cpu_set-cpu_active_map.patch, Size: 554 bytes --]

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

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

[Max Krasnyansky]

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. I'd say we fix the
microcode instead.

Max

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

[Dmitry Adamushko]

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

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

[Ingo Molnar]

* Dmitry Adamushko <dmitry.adamushko@gmail.com> 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

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

[Max Krasnyansky]

Ingo Molnar wrote:
> * Dmitry Adamushko <dmitry.adamushko@gmail.com> 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

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

[Max Krasnyansky]

Dmitry Adamushko wrote:
> 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.
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

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

[Dmitry Adamushko]

2008/7/30 Max Krasnyansky <maxk@qualcomm.com>:
>
> Dmitry Adamushko wrote:
>> 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.
> 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

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

[Max Krasnyansky]

Dmitry Adamushko wrote:
> 2008/7/30 Max Krasnyansky <maxk@qualcomm.com>:
>> Dmitry Adamushko wrote:
>>> 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.
>> 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

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

[Dmitry Adamushko]

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

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

[Peter Oruba]

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

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

[H. Peter Anvin]

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

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

[Ingo Molnar]

* Peter Oruba <peter.oruba@amd.com> 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

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

[Dmitry Adamushko]

2008/7/31 Ingo Molnar <mingo@elte.hu>:
>
> * Peter Oruba <peter.oruba@amd.com> 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

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

[Ingo Molnar]

* Dmitry Adamushko <dmitry.adamushko@gmail.com> 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