2003-08-19 22:57:46

by Mikael Pettersson

[permalink] [raw]
Subject: [BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion

The box is UP with I/O-APIC, configured for !SMP, IO_APIC, !ACPI.
2.6.0-test3 worked fine, but 2.6.0-test3-bk7 panics and halts on
boot in arch/x86_64/kernel/io_apic.c:setup_ioapic_ids_from_mpc():

...
POSIX conformance testing by UNIFIX
ENABLING IO-APIC IRQs
BIOS bug, IO-APIC#0 ID 1 is already used!...
Kernel panic: Max APIC ID exceeded!

This happens because cpumask_t on UP implements "a set of CPUs"
as "a set is 0 or 1" (asm-generic/cpumask_up.h), while io_apic.c
actually wants "set of CPU (local APIC) and I/O-APIC IDs",
which obviously doesn't fit the "0 or 1" assumption.

As a quick-and-dirty test I changed linux/cpumask.h as follows

--- linux-2.6.0-test3-bk7/include/linux/cpumask.h.~1~ 2003-08-19 23:48:50.000000000 +0200
+++ linux-2.6.0-test3-bk7/include/linux/cpumask.h 2003-08-20 00:07:17.000000000 +0200
@@ -21,7 +21,7 @@
typedef unsigned long cpumask_t;
#endif

-#ifdef CONFIG_SMP
+#if defined(CONFIG_SMP) || defined(CONFIG_X86_IO_APIC)
#if NR_CPUS > BITS_PER_LONG
#include <asm-generic/cpumask_array.h>
#else

Since NR_CPUS==1 this makes UP_IOAPIC use cpumask_arith.h,
which is what the code used before the cpumask_t conversion.
With this change, the box boots Ok again.

(I believe this is the correct thing to do, except having
CONFIG_X86_IO_APIC in generic code isn't quite right.)

/Mikael


2003-08-19 23:39:21

by Andi Kleen

[permalink] [raw]
Subject: Re: [BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion

Mikael Pettersson <[email protected]> writes:

> --- linux-2.6.0-test3-bk7/include/linux/cpumask.h.~1~ 2003-08-19 23:48:50.000000000 +0200
> +++ linux-2.6.0-test3-bk7/include/linux/cpumask.h 2003-08-20 00:07:17.000000000 +0200
> @@ -21,7 +21,7 @@
> typedef unsigned long cpumask_t;
> #endif
>
> -#ifdef CONFIG_SMP
> +#if defined(CONFIG_SMP) || defined(CONFIG_X86_IO_APIC)
> #if NR_CPUS > BITS_PER_LONG
> #include <asm-generic/cpumask_array.h>
> #else
>
> Since NR_CPUS==1 this makes UP_IOAPIC use cpumask_arith.h,
> which is what the code used before the cpumask_t conversion.
> With this change, the box boots Ok again.

Nasty.

But why does i386/UP work?

>
> (I believe this is the correct thing to do, except having
> CONFIG_X86_IO_APIC in generic code isn't quite right.)

Better would be to undo the cpumask_t changes in io_apic.c
and go back to unsigned long masks there again.

Obviously a cpu mask is not the right data structure to manage APICs

Another way would be to do whatever i386 does to avoid the problem.
The IO-APIC code is unfortunately quite out of date/unsynced compared to i386,
maybe it just needs some bug fix ported over. I will check that later.

-Andi

2003-08-19 23:58:03

by Andi Kleen

[permalink] [raw]
Subject: Re: [BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion

> Odd; I have a UP IO-APIC ia32 box here and it appears to do okay; there
> is a question of sparse APIC ID's and APIC ID space needing to be
> independent of NR_CPUS handled in the ia32 code that isn't handled in
> the x86_64 code. It was handled for ia32 by using a bitmap of size
> MAX_APICS (physid_mask_t) instead of cpumask_t for the things, which
> appears to eliminate various special cases for xAPIC's too.

Ok, then the physid_mask_t code needs to be ported over to x86-64.

-Andi

2003-08-19 23:50:25

by William Lee Irwin III

[permalink] [raw]
Subject: Re: [BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion

Mikael Pettersson <[email protected]> writes:
> Nasty.
> But why does i386/UP work?

On Wed, Aug 20, 2003 at 01:39:10AM +0200, Andi Kleen wrote:
> > (I believe this is the correct thing to do, except having
> > CONFIG_X86_IO_APIC in generic code isn't quite right.)
> Better would be to undo the cpumask_t changes in io_apic.c
> and go back to unsigned long masks there again.
> Obviously a cpu mask is not the right data structure to manage APICs
> Another way would be to do whatever i386 does to avoid the problem.
> The IO-APIC code is unfortunately quite out of date/unsynced compared to i386,
> maybe it just needs some bug fix ported over. I will check that later.

Odd; I have a UP IO-APIC ia32 box here and it appears to do okay; there
is a question of sparse APIC ID's and APIC ID space needing to be
independent of NR_CPUS handled in the ia32 code that isn't handled in
the x86_64 code. It was handled for ia32 by using a bitmap of size
MAX_APICS (physid_mask_t) instead of cpumask_t for the things, which
appears to eliminate various special cases for xAPIC's too.


-- wli

2003-08-20 00:39:39

by Andi Kleen

[permalink] [raw]
Subject: [PATCH] physid_maskt for x86-64 was R[BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion


This patch fixes the UP boot problem in 2.6.0test3-bk7 for me. I just ported over
the physid_mask_t code from i386 for the IO-APIC ids. This avoids the problems
with the boolean cpumask on UP kernels.

Caveat: I only tested it with a UP kernel on a UP machine. SMP is untested.

-Andi


diff -u linux-2.5-amd64/arch/x86_64/kernel/apic.c-o linux-2.5-amd64/arch/x86_64/kernel/apic.c
--- linux-2.5-amd64/arch/x86_64/kernel/apic.c-o 2003-08-19 13:12:25.000000000 +0200
+++ linux-2.5-amd64/arch/x86_64/kernel/apic.c 2003-08-20 02:09:40.000000000 +0200
@@ -299,7 +299,7 @@
* This is meaningless in clustered apic mode, so we skip it.
*/
if (!clustered_apic_mode &&
- !cpu_isset(GET_APIC_ID(apic_read(APIC_ID)), phys_cpu_present_map))
+ !physid_isset(GET_APIC_ID(apic_read(APIC_ID)), phys_cpu_present_map))
BUG();

/*
@@ -993,7 +993,7 @@

connect_bsp_APIC();

- phys_cpu_present_map = cpumask_of_cpu(0);
+ phys_cpu_present_map = physid_mask_of_physid(0);
apic_write_around(APIC_ID, boot_cpu_id);

setup_local_APIC();
diff -u linux-2.5-amd64/arch/x86_64/kernel/smpboot.c-o linux-2.5-amd64/arch/x86_64/kernel/smpboot.c
--- linux-2.5-amd64/arch/x86_64/kernel/smpboot.c-o 2003-08-19 13:12:26.000000000 +0200
+++ linux-2.5-amd64/arch/x86_64/kernel/smpboot.c 2003-08-20 02:06:23.000000000 +0200
@@ -734,10 +734,10 @@
current_thread_info()->cpu = 0;
smp_tune_scheduling();

- if (!cpu_isset(hard_smp_processor_id(), phys_cpu_present_map)) {
+ if (!physid_isset(hard_smp_processor_id(), phys_cpu_present_map)) {
printk("weird, boot CPU (#%d) not listed by the BIOS.\n",
hard_smp_processor_id());
- cpu_set(hard_smp_processor_id(), phys_cpu_present_map);
+ physid_set(hard_smp_processor_id(), phys_cpu_present_map);
}

/*
@@ -748,7 +748,7 @@
printk(KERN_NOTICE "SMP motherboard not detected.\n");
io_apic_irqs = 0;
cpu_online_map = cpumask_of_cpu(0);
- phys_cpu_present_map = cpumask_of_cpu(0);
+ phys_cpu_present_map = physid_mask_of_physid(0);
if (APIC_init_uniprocessor())
printk(KERN_NOTICE "Local APIC not detected."
" Using dummy APIC emulation.\n");
@@ -759,10 +759,10 @@
* Should not be necessary because the MP table should list the boot
* CPU too, but we do it for the sake of robustness anyway.
*/
- if (!cpu_isset(boot_cpu_id, phys_cpu_present_map)) {
+ if (!physid_isset(boot_cpu_id, phys_cpu_present_map)) {
printk(KERN_NOTICE "weird, boot CPU (#%d) not listed by the BIOS.\n",
boot_cpu_id);
- cpu_set(hard_smp_processor_id(), phys_cpu_present_map);
+ physid_set(hard_smp_processor_id(), phys_cpu_present_map);
}

/*
@@ -774,7 +774,7 @@
printk(KERN_ERR "... forcing use of dummy APIC emulation. (tell your hw vendor)\n");
io_apic_irqs = 0;
cpu_online_map = cpumask_of_cpu(0);
- phys_cpu_present_map = cpumask_of_cpu(0);
+ phys_cpu_present_map = physid_mask_of_physid(0);
disable_apic = 1;
goto smp_done;
}
@@ -789,7 +789,7 @@
printk(KERN_INFO "SMP mode deactivated, forcing use of dummy APIC emulation.\n");
io_apic_irqs = 0;
cpu_online_map = cpumask_of_cpu(0);
- phys_cpu_present_map = cpumask_of_cpu(0);
+ phys_cpu_present_map = physid_mask_of_physid(0);
disable_apic = 1;
goto smp_done;
}
@@ -803,7 +803,7 @@
/*
* Now scan the CPU present map and fire up the other CPUs.
*/
- Dprintk("CPU present map: %lx\n", phys_cpu_present_map);
+ Dprintk("CPU present map: %lx\n", physids_coerce(phys_cpu_present_map));

for (apicid = 0; apicid < NR_CPUS; apicid++) {
/*
diff -u linux-2.5-amd64/arch/x86_64/kernel/mpparse.c-o linux-2.5-amd64/arch/x86_64/kernel/mpparse.c
--- linux-2.5-amd64/arch/x86_64/kernel/mpparse.c-o 2003-08-20 00:20:35.000000000 +0200
+++ linux-2.5-amd64/arch/x86_64/kernel/mpparse.c 2003-08-20 02:06:24.000000000 +0200
@@ -67,7 +67,7 @@
static unsigned int num_processors = 0;

/* Bitmask of physically existing CPUs */
-cpumask_t phys_cpu_present_map = CPU_MASK_NONE;
+physid_mask_t phys_cpu_present_map = PHYSID_MASK_NONE;

/* ACPI MADT entry parsing functions */
#ifdef CONFIG_ACPI_BOOT
@@ -126,7 +126,7 @@
}
ver = m->mpc_apicver;

- cpu_set(m->mpc_apicid, phys_cpu_present_map);
+ physid_set(m->mpc_apicid, phys_cpu_present_map);
/*
* Validate version
*/
diff -u linux-2.5-amd64/arch/x86_64/kernel/io_apic.c-o linux-2.5-amd64/arch/x86_64/kernel/io_apic.c
--- linux-2.5-amd64/arch/x86_64/kernel/io_apic.c-o 2003-08-20 00:20:35.000000000 +0200
+++ linux-2.5-amd64/arch/x86_64/kernel/io_apic.c 2003-08-20 02:10:08.000000000 +0200
@@ -1014,7 +1014,7 @@
static void __init setup_ioapic_ids_from_mpc (void)
{
union IO_APIC_reg_00 reg_00;
- cpumask_t phys_id_present_map = phys_cpu_present_map;
+ physid_mask_t phys_id_present_map = phys_cpu_present_map;
int apic;
int i;
unsigned char old_id;
@@ -1047,22 +1047,22 @@
* system must have a unique ID or we get lots of nice
* 'stuck on smp_invalidate_needed IPI wait' messages.
*/
- if (cpu_isset(mp_ioapics[apic].mpc_apicid, phys_id_present_map)) {
+ if (physid_isset(mp_ioapics[apic].mpc_apicid, phys_id_present_map)) {
printk(KERN_ERR "BIOS bug, IO-APIC#%d ID %d is already used!...\n",
apic, mp_ioapics[apic].mpc_apicid);
for (i = 0; i < 0xf; i++)
- if (!cpu_isset(i, phys_id_present_map))
+ if (!physid_isset(i, phys_id_present_map))
break;
if (i >= 0xf)
panic("Max APIC ID exceeded!\n");
printk(KERN_ERR "... fixing up to %d. (tell your hw vendor)\n",
i);
- cpu_set(i, phys_id_present_map);
+ physid_set(i, phys_id_present_map);
mp_ioapics[apic].mpc_apicid = i;
} else {
printk(KERN_INFO
"Using IO-APIC %d\n", mp_ioapics[apic].mpc_apicid);
- cpu_set(mp_ioapics[apic].mpc_apicid, phys_id_present_map);
+ physid_set(mp_ioapics[apic].mpc_apicid, phys_id_present_map);
}


@@ -1642,7 +1642,7 @@
int __init io_apic_get_unique_id (int ioapic, int apic_id)
{
union IO_APIC_reg_00 reg_00;
- static cpumask_t apic_id_map;
+ static physid_mask_t apic_id_map;
unsigned long flags;
int i = 0;

@@ -1655,7 +1655,7 @@
* advantage of new APIC bus architecture.
*/

- if (!cpus_empty(apic_id_map))
+ if (!physids_empty(apic_id_map))
apic_id_map = phys_cpu_present_map;

spin_lock_irqsave(&ioapic_lock, flags);
@@ -1672,10 +1672,10 @@
* Every APIC in a system must have a unique ID or we get lots of nice
* 'stuck on smp_invalidate_needed IPI wait' messages.
*/
- if (cpu_isset(apic_id, apic_id_map)) {
+ if (physid_isset(apic_id, apic_id_map)) {

for (i = 0; i < IO_APIC_MAX_ID; i++) {
- if (!cpu_isset(i, apic_id_map))
+ if (!physid_isset(i, apic_id_map))
break;
}

@@ -1688,7 +1688,7 @@
apic_id = i;
}

- cpu_set(apic_id, apic_id_map);
+ physid_set(apic_id, apic_id_map);

if (reg_00.bits.ID != apic_id) {
reg_00.bits.ID = apic_id;
diff -u linux-2.5-amd64/include/asm-x86_64/mpspec.h-o linux-2.5-amd64/include/asm-x86_64/mpspec.h
--- linux-2.5-amd64/include/asm-x86_64/mpspec.h-o 2003-08-19 13:19:52.000000000 +0200
+++ linux-2.5-amd64/include/asm-x86_64/mpspec.h 2003-08-20 01:59:16.000000000 +0200
@@ -169,7 +169,6 @@
extern cpumask_t mp_bus_to_cpumask [MAX_MP_BUSSES];

extern unsigned int boot_cpu_physical_apicid;
-extern cpumask_t phys_cpu_present_map;
extern int smp_found_config;
extern void find_smp_config (void);
extern void get_smp_config (void);
@@ -198,5 +197,49 @@

extern int using_apic_timer;

+#define PHYSID_ARRAY_SIZE BITS_TO_LONGS(MAX_APICS)
+
+struct physid_mask
+{
+ unsigned long mask[PHYSID_ARRAY_SIZE];
+};
+
+typedef struct physid_mask physid_mask_t;
+
+#define physid_set(physid, map) set_bit(physid, (map).mask)
+#define physid_clear(physid, map) clear_bit(physid, (map).mask)
+#define physid_isset(physid, map) test_bit(physid, (map).mask)
+#define physid_test_and_set(physid, map) test_and_set_bit(physid, (map).mask)
+
+#define physids_and(dst, src1, src2) bitmap_and((dst).mask, (src1).mask, (src2).mask, MAX_APICS)
+#define physids_or(dst, src1, src2) bitmap_or((dst).mask, (src1).mask, (src2).mask, MAX_APICS)
+#define physids_clear(map) bitmap_clear((map).mask, MAX_APICS)
+#define physids_complement(map) bitmap_complement((map).mask, MAX_APICS)
+#define physids_empty(map) bitmap_empty((map).mask, MAX_APICS)
+#define physids_equal(map1, map2) bitmap_equal((map1).mask, (map2).mask, MAX_APICS)
+#define physids_weight(map) bitmap_weight((map).mask, MAX_APICS)
+#define physids_shift_right(d, s, n) bitmap_shift_right((d).mask, (s).mask, n, MAX_APICS)
+#define physids_shift_left(d, s, n) bitmap_shift_left((d).mask, (s).mask, n, MAX_APICS)
+#define physids_coerce(map) ((map).mask[0])
+
+#define physids_promote(physids) \
+ ({ \
+ physid_mask_t __physid_mask = PHYSID_MASK_NONE; \
+ __physid_mask.mask[0] = physids; \
+ __physid_mask; \
+ })
+
+#define physid_mask_of_physid(physid) \
+ ({ \
+ physid_mask_t __physid_mask = PHYSID_MASK_NONE; \
+ physid_set(physid, __physid_mask); \
+ __physid_mask; \
+ })
+
+#define PHYSID_MASK_ALL { {[0 ... PHYSID_ARRAY_SIZE-1] = ~0UL} }
+#define PHYSID_MASK_NONE { {[0 ... PHYSID_ARRAY_SIZE-1] = 0UL} }
+
+extern physid_mask_t phys_cpu_present_map;
+
#endif

2003-08-23 21:51:43

by Mikael Pettersson

[permalink] [raw]
Subject: Re: [PATCH] physid_maskt for x86-64 was R[BUG][2.6.0-test3-bk7] x86-64 UP_IOAPIC panic caused by cpumask_t conversion

On Wed, 20 Aug 2003 02:39:29 +0200, Andi Kleen <[email protected]> wrote:
>This patch fixes the UP boot problem in 2.6.0test3-bk7 for me. I just ported over
>the physid_mask_t code from i386 for the IO-APIC ids. This avoids the problems
>with the boolean cpumask on UP kernels.
>
>Caveat: I only tested it with a UP kernel on a UP machine. SMP is untested.

Thanks. This fixed test3-bk7 & test4 for me.

/Mikael