* Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID
2009-08-08 17:53 ` Ingo Molnar
@ 2009-08-08 18:19 ` Ondrej Zary
2009-08-09 1:28 ` H. Peter Anvin
2009-08-09 17:45 ` [PATCH v2] " Ondrej Zary
2 siblings, 0 replies; 6+ messages in thread
From: Ondrej Zary @ 2009-08-08 18:19 UTC (permalink / raw)
To: Ingo Molnar; +Cc: tglx, mingo, hpa, x86, linux-kernel
On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:
> * Ondrej Zary <linux@rainbow-software.org> wrote:
> > Kernel is broken for x86 CPUs without CPUID since 2.6.28. It
> > crashes with NULL pointer dereference in identify_cpu():
> >
> > 766 generic_identify(c);
> > 767
> > 768--> if (this_cpu->c_identify)
> > 769 this_cpu->c_identify(c);
> >
> > this_cpu is NULL. This is because it's only initialized in
> > get_cpu_vendor() function, which is not called if the CPU has no
> > CPUID instruction.
>
> indeed ...
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
> > 05:05:27.000000000 +0200 +++
> > linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
> > 18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
> >
> > static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
> > {
> > + this_cpu = &default_cpu;
> > c->extended_cpuid_level = 0;
> >
> > if (!have_cpuid_p())
>
> How about initializing this_cpu instead, via:
>
> static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
>
> ?
That might be even better - looks cleaner. I can't test it right now as I
don't have the hardware here. I'll test it tomorrow (unless someone will be
faster).
--
Ondrej Zary
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID
2009-08-08 17:53 ` Ingo Molnar
2009-08-08 18:19 ` Ondrej Zary
@ 2009-08-09 1:28 ` H. Peter Anvin
2009-08-09 21:12 ` Ondrej Zary
2009-08-09 17:45 ` [PATCH v2] " Ondrej Zary
2 siblings, 1 reply; 6+ messages in thread
From: H. Peter Anvin @ 2009-08-09 1:28 UTC (permalink / raw)
To: Ingo Molnar; +Cc: Ondrej Zary, tglx, mingo, x86, linux-kernel
On 08/08/2009 10:53 AM, Ingo Molnar wrote:
>
> indeed ...
>
>> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
>>
>> --- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10 05:05:27.000000000 +0200
>> +++ linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08 18:00:21.000000000 +0200
>> @@ -699,6 +699,7 @@
>>
>> static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
>> {
>> + this_cpu = &default_cpu;
>> c->extended_cpuid_level = 0;
>>
>> if (!have_cpuid_p())
>
> How about initializing this_cpu instead, via:
>
> static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
>
The whole this_cpu hack is scary as all hell... although it's probably
OK on a technicality, it takes what is properly a per-cpu attribute and
turns it into a static global.
We *should* be able to initialize the APs (at least) in parallel, and
although there probably aren't any systems in the field which don't have
duplicate vendors, it is at least theoretically possible to have
combinations of CPUID and non-CPUID processors in the same systems.
As such, it really would be better if this_cpu was changed to be passed
as return values and on the stack (as appropriate). However, that is
not 2.6.31 material, and as such doing the static initialization would
be okay.
Ondrej, would you be interested in doing a "fullblown" patch for this?
-hpa
--
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel. I don't speak on their behalf.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID
2009-08-09 1:28 ` H. Peter Anvin
@ 2009-08-09 21:12 ` Ondrej Zary
0 siblings, 0 replies; 6+ messages in thread
From: Ondrej Zary @ 2009-08-09 21:12 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: Ingo Molnar, tglx, mingo, x86, linux-kernel
On Sunday 09 August 2009 03:28:10 H. Peter Anvin wrote:
> On 08/08/2009 10:53 AM, Ingo Molnar wrote:
> > indeed ...
> >
> >> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >>
> >> --- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
> >> 05:05:27.000000000 +0200 +++
> >> linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
> >> 18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
> >>
> >> static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
> >> {
> >> + this_cpu = &default_cpu;
> >> c->extended_cpuid_level = 0;
> >>
> >> if (!have_cpuid_p())
> >
> > How about initializing this_cpu instead, via:
> >
> > static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
>
> The whole this_cpu hack is scary as all hell... although it's probably
> OK on a technicality, it takes what is properly a per-cpu attribute and
> turns it into a static global.
>
> We *should* be able to initialize the APs (at least) in parallel, and
> although there probably aren't any systems in the field which don't have
> duplicate vendors, it is at least theoretically possible to have
> combinations of CPUID and non-CPUID processors in the same systems.
>
> As such, it really would be better if this_cpu was changed to be passed
> as return values and on the stack (as appropriate). However, that is
> not 2.6.31 material, and as such doing the static initialization would
> be okay.
>
> Ondrej, would you be interested in doing a "fullblown" patch for this?
That would be too much for me. I know basically nothing about this
initialization code.
> -hpa
--
Ondrej Zary
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] OOPS in identify_cpu() on CPUs without CPUID
2009-08-08 17:53 ` Ingo Molnar
2009-08-08 18:19 ` Ondrej Zary
2009-08-09 1:28 ` H. Peter Anvin
@ 2009-08-09 17:45 ` Ondrej Zary
2 siblings, 0 replies; 6+ messages in thread
From: Ondrej Zary @ 2009-08-09 17:45 UTC (permalink / raw)
To: Ingo Molnar; +Cc: tglx, mingo, hpa, x86, linux-kernel
On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:
> * Ondrej Zary <linux@rainbow-software.org> wrote:
> > Kernel is broken for x86 CPUs without CPUID since 2.6.28. It
> > crashes with NULL pointer dereference in identify_cpu():
> >
> > 766 generic_identify(c);
> > 767
> > 768--> if (this_cpu->c_identify)
> > 769 this_cpu->c_identify(c);
> >
> > this_cpu is NULL. This is because it's only initialized in
> > get_cpu_vendor() function, which is not called if the CPU has no
> > CPUID instruction.
>
> indeed ...
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
> > 05:05:27.000000000 +0200 +++
> > linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
> > 18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
> >
> > static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
> > {
> > + this_cpu = &default_cpu;
> > c->extended_cpuid_level = 0;
> >
> > if (!have_cpuid_p())
>
> How about initializing this_cpu instead, via:
>
> static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
>
> ?
> Ingo
The new patch is not very nice as default_cpu has to be moved. Works fine.
Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL
pointer dereference in identify_cpu():
766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);
this_cpu is NULL. This is because it's only initialized in get_cpu_vendor()
function, which is not called if the CPU has no CPUID instruction.
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
--- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10 05:05:27.000000000 +0200
+++ linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-09 13:33:04.000000000 +0200
@@ -58,7 +58,30 @@
alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
}
-static const struct cpu_dev *this_cpu __cpuinitdata;
+static void __cpuinit default_init(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_X86_64
+ display_cacheinfo(c);
+#else
+ /* Not much we can do here... */
+ /* Check if at least it has cpuid */
+ if (c->cpuid_level == -1) {
+ /* No cpuid. It must be an ancient CPU */
+ if (c->x86 == 4)
+ strcpy(c->x86_model_id, "486");
+ else if (c->x86 == 3)
+ strcpy(c->x86_model_id, "386");
+ }
+#endif
+}
+
+static const struct cpu_dev __cpuinitconst default_cpu = {
+ .c_init = default_init,
+ .c_vendor = "Unknown",
+ .c_x86_vendor = X86_VENDOR_UNKNOWN,
+};
+
+static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
DEFINE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page) = { .gdt = {
#ifdef CONFIG_X86_64
@@ -330,29 +353,6 @@
static const struct cpu_dev *__cpuinitdata cpu_devs[X86_VENDOR_NUM] = {};
-static void __cpuinit default_init(struct cpuinfo_x86 *c)
-{
-#ifdef CONFIG_X86_64
- display_cacheinfo(c);
-#else
- /* Not much we can do here... */
- /* Check if at least it has cpuid */
- if (c->cpuid_level == -1) {
- /* No cpuid. It must be an ancient CPU */
- if (c->x86 == 4)
- strcpy(c->x86_model_id, "486");
- else if (c->x86 == 3)
- strcpy(c->x86_model_id, "386");
- }
-#endif
-}
-
-static const struct cpu_dev __cpuinitconst default_cpu = {
- .c_init = default_init,
- .c_vendor = "Unknown",
- .c_x86_vendor = X86_VENDOR_UNKNOWN,
-};
-
static void __cpuinit get_model_name(struct cpuinfo_x86 *c)
{
unsigned int *v;
--
Ondrej Zary
^ permalink raw reply [flat|nested] 6+ messages in thread