Skip to contentSkip to conversation

x86/apic changes for v2.6.38

GIT PULL Ingo Molnar 6 participants 15 messages Jan 6, 2011 – Jan 12, 2011

Subsystems (3)
x86 ACPI x86 MM
lore
Share & tools
lore

Ingo Molnar

author
#0 lore raw

Linus,

Please pull the latest x86-apic-for-linus git tree from:

git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git x86-apic-for-linus

out-of-topic modifications in x86-apic-for-linus:
-------------------------------------------------
drivers/acpi/numa.c # d3bd058: x86, acpi: Parse all SRAT cpu ent

Thanks,

Ingo

------------------>
Sebastian Andrzej Siewior (1):
x86: io_apic: Split setup_ioapic_ids_from_mpc()

Thomas Gleixner (3):
x86: Allow platforms to force enable apic
x86: apic: Move probe_nr_irqs_gsi() into ioapic_init_mappings()
x86: io_apic: Fix CONFIG_X86_IO_APIC=n breakage

Yinghai Lu (3):
x86, acpi: Add MAX_LOCAL_APIC for 32bit
x86, acpi: Parse all SRAT cpu entries even above the cpu number limitation
x86: Fix APIC ID sizing bug on larger systems, clean up MAX_APICS confusion

 arch/x86/include/asm/apic.h            |    1 +
 arch/x86/include/asm/apicdef.h         |    1 +
 arch/x86/include/asm/io_apic.h         |    8 +--
 arch/x86/include/asm/mpspec.h          |   31 ++++-------
 arch/x86/include/asm/mpspec_def.h      |    7 ---
 arch/x86/kernel/acpi/boot.c            |   11 +++-
 arch/x86/kernel/apic/apic.c            |   89 ++++++++++++++++++++------------
 arch/x86/kernel/apic/io_apic.c         |   37 ++++++++-----
 arch/x86/kernel/setup.c                |    5 +--
 arch/x86/mm/srat_32.c                  |    1 +
 arch/x86/mm/srat_64.c                  |   10 ++++
 arch/x86/platform/sfi/sfi.c            |    4 +-
 arch/x86/platform/visws/visws_quirks.c |    2 +-
 drivers/acpi/numa.c                    |   14 ++++-
 14 files changed, 129 insertions(+), 92 deletions(-)
arch/x86/include/asm/apic.h +1 -0
index f6ce0bd..cf12007 100644
@@ -238,6 +238,7 @@ extern void setup_boot_APIC_clock(void);
 extern void setup_secondary_APIC_clock(void);
 extern int APIC_init_uniprocessor(void);
 extern void enable_NMI_through_LVT0(void);
+extern int apic_force_enable(void);
 
 /*
  * On 32bit this is mach-xxx local
arch/x86/include/asm/apicdef.h +1 -0
index a859ca4..47a30ff 100644
@@ -145,6 +145,7 @@
 
 #ifdef CONFIG_X86_32
 # define MAX_IO_APICS 64
+# define MAX_LOCAL_APIC 256
 #else
 # define MAX_IO_APICS 128
 # define MAX_LOCAL_APIC 32768
arch/x86/include/asm/io_apic.h +3 -5
index a6b28d0..0c5ca4e 100644
@@ -159,7 +159,7 @@ struct io_apic_irq_attr;
 extern int io_apic_set_pci_routing(struct device *dev, int irq,
 		 struct io_apic_irq_attr *irq_attr);
 void setup_IO_APIC_irq_extra(u32 gsi);
-extern void ioapic_init_mappings(void);
+extern void ioapic_and_gsi_init(void);
 extern void ioapic_insert_resources(void);
 
 extern struct IO_APIC_route_entry **alloc_ioapic_entries(void);

@@ -168,10 +168,9 @@ extern int save_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries);
 extern void mask_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries);
 extern int restore_IO_APIC_setup(struct IO_APIC_route_entry **ioapic_entries);
 
-extern void probe_nr_irqs_gsi(void);
 extern int get_nr_irqs_gsi(void);
-
 extern void setup_ioapic_ids_from_mpc(void);
+extern void setup_ioapic_ids_from_mpc_nocheck(void);
 
 struct mp_ioapic_gsi{
 	u32 gsi_base;

@@ -189,9 +188,8 @@ extern void __init pre_init_apic_IRQ0(void);
 #define io_apic_assign_pci_irqs 0
 #define setup_ioapic_ids_from_mpc x86_init_noop
 static const int timer_through_8259 = 0;
-static inline void ioapic_init_mappings(void)	{ }
+static inline void ioapic_and_gsi_init(void) { }
 static inline void ioapic_insert_resources(void) { }
-static inline void probe_nr_irqs_gsi(void)	{ }
 #define gsi_top (NR_IRQS_LEGACY)
 static inline int mp_find_ioapic(u32 gsi) { return 0; }
arch/x86/include/asm/mpspec.h +12 -19
index c82868e..0c90dd9 100644
@@ -5,8 +5,9 @@
 
 #include <asm/mpspec_def.h>
 #include <asm/x86_init.h>
+#include <asm/apicdef.h>
 
-extern int apic_version[MAX_APICS];
+extern int apic_version[];
 extern int pic_mode;
 
 #ifdef CONFIG_X86_32

@@ -107,7 +108,7 @@ extern int mp_register_gsi(struct device *dev, u32 gsi, int edge_level,
 				 int active_high_low);
 #endif /* CONFIG_ACPI */
 
-#define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_APICS)
+#define PHYSID_ARRAY_SIZE	BITS_TO_LONGS(MAX_LOCAL_APIC)
 
 struct physid_mask {
 	unsigned long mask[PHYSID_ARRAY_SIZE];

@@ -122,31 +123,31 @@ typedef struct physid_mask physid_mask_t;
 	test_and_set_bit(physid, (map).mask)
 
 #define physids_and(dst, src1, src2)					\
-	bitmap_and((dst).mask, (src1).mask, (src2).mask, MAX_APICS)
+	bitmap_and((dst).mask, (src1).mask, (src2).mask, MAX_LOCAL_APIC)
 
 #define physids_or(dst, src1, src2)					\
-	bitmap_or((dst).mask, (src1).mask, (src2).mask, MAX_APICS)
+	bitmap_or((dst).mask, (src1).mask, (src2).mask, MAX_LOCAL_APIC)
 
 #define physids_clear(map)					\
-	bitmap_zero((map).mask, MAX_APICS)
+	bitmap_zero((map).mask, MAX_LOCAL_APIC)
 
 #define physids_complement(dst, src)				\
-	bitmap_complement((dst).mask, (src).mask, MAX_APICS)
+	bitmap_complement((dst).mask, (src).mask, MAX_LOCAL_APIC)
 
 #define physids_empty(map)					\
-	bitmap_empty((map).mask, MAX_APICS)
+	bitmap_empty((map).mask, MAX_LOCAL_APIC)
 
 #define physids_equal(map1, map2)				\
-	bitmap_equal((map1).mask, (map2).mask, MAX_APICS)
+	bitmap_equal((map1).mask, (map2).mask, MAX_LOCAL_APIC)
 
 #define physids_weight(map)					\
-	bitmap_weight((map).mask, MAX_APICS)
+	bitmap_weight((map).mask, MAX_LOCAL_APIC)
 
 #define physids_shift_right(d, s, n)				\
-	bitmap_shift_right((d).mask, (s).mask, n, MAX_APICS)
+	bitmap_shift_right((d).mask, (s).mask, n, MAX_LOCAL_APIC)
 
 #define physids_shift_left(d, s, n)				\
-	bitmap_shift_left((d).mask, (s).mask, n, MAX_APICS)
+	bitmap_shift_left((d).mask, (s).mask, n, MAX_LOCAL_APIC)
 
 static inline unsigned long physids_coerce(physid_mask_t *map)
 {

@@ -159,14 +160,6 @@ static inline void physids_promote(unsigned long physids, physid_mask_t *map)
 	map->mask[0] = physids;
 }
 
-/* Note: will create very large stack frames if physid_mask_t is big */
-#define physid_mask_of_physid(physid)					\
-	({								\
-		physid_mask_t __physid_mask = PHYSID_MASK_NONE;		\
-		physid_set(physid, __physid_mask);			\
-		__physid_mask;						\
-	})
-
 static inline void physid_set_mask_of_physid(int physid, physid_mask_t *map)
 {
 	physids_clear(*map);
arch/x86/include/asm/mpspec_def.h +0 -7
index 4a7f96d..c0a955a 100644
@@ -15,13 +15,6 @@
 
 #ifdef CONFIG_X86_32
 # define MAX_MPC_ENTRY 1024
-# define MAX_APICS      256
-#else
-# if NR_CPUS <= 255
-#  define MAX_APICS     255
-# else
-#  define MAX_APICS   32768
-# endif
 #endif
 
 /* Intel MP Floating Pointer Structure */
arch/x86/kernel/acpi/boot.c +8 -3
index 71232b9..17c8090 100644
@@ -198,6 +198,11 @@ static void __cpuinit acpi_register_lapic(int id, u8 enabled)
 {
 	unsigned int ver = 0;
 
+	if (id >= (MAX_LOCAL_APIC-1)) {
+		printk(KERN_INFO PREFIX "skipped apicid that is too big\n");
+		return;
+	}
+
 	if (!enabled) {
 		++disabled_cpus;
 		return;

@@ -910,13 +915,13 @@ static int __init acpi_parse_madt_lapic_entries(void)
 	acpi_register_lapic_address(acpi_lapic_addr);
 
 	count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_SAPIC,
-				      acpi_parse_sapic, MAX_APICS);
+				      acpi_parse_sapic, MAX_LOCAL_APIC);
 
 	if (!count) {
 		x2count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_X2APIC,
-						acpi_parse_x2apic, MAX_APICS);
+					acpi_parse_x2apic, MAX_LOCAL_APIC);
 		count = acpi_table_parse_madt(ACPI_MADT_TYPE_LOCAL_APIC,
-					      acpi_parse_lapic, MAX_APICS);
+					acpi_parse_lapic, MAX_LOCAL_APIC);
 	}
 	if (!count && !x2count) {
 		printk(KERN_ERR PREFIX "No LAPIC entries present\n");
arch/x86/kernel/apic/apic.c +55 -34

113 lines not on this page · raw message

arch/x86/kernel/apic/io_apic.c +22 -15
index fadcd74..2fc696e 100644
@@ -1934,8 +1934,7 @@ void disable_IO_APIC(void)
  *
  * by Matt Domsch <[email protected]>  Tue Dec 21 12:25:05 CST 1999
  */
-
-void __init setup_ioapic_ids_from_mpc(void)
+void __init setup_ioapic_ids_from_mpc_nocheck(void)
 {
 	union IO_APIC_reg_00 reg_00;
 	physid_mask_t phys_id_present_map;

@@ -1944,15 +1943,6 @@ void __init setup_ioapic_ids_from_mpc(void)
 	unsigned char old_id;
 	unsigned long flags;
 
-	if (acpi_ioapic)
-		return;
-	/*
-	 * Don't check I/O APIC IDs for xAPIC systems.  They have
-	 * no meaning without the serial APIC bus.
-	 */
-	if (!(boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
-		|| APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
-		return;
 	/*
 	 * This is broken; anything with a real cpu count has to
 	 * circumvent this idiocy regardless.

@@ -2006,7 +1996,6 @@ void __init setup_ioapic_ids_from_mpc(void)
 			physids_or(phys_id_present_map, phys_id_present_map, tmp);
 		}
 
-
 		/*
 		 * We need to adjust the IRQ routing table
 		 * if the ID changed.

@@ -2042,6 +2031,21 @@ void __init setup_ioapic_ids_from_mpc(void)
 			apic_printk(APIC_VERBOSE, " ok.\n");
 	}
 }
+
+void __init setup_ioapic_ids_from_mpc(void)
+{
+
+	if (acpi_ioapic)
+		return;
+	/*
+	 * Don't check I/O APIC IDs for xAPIC systems.  They have
+	 * no meaning without the serial APIC bus.
+	 */
+	if (!(boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
+		|| APIC_XAPIC(apic_version[boot_cpu_physical_apicid]))
+		return;
+	setup_ioapic_ids_from_mpc_nocheck();
+}
 #endif
 
 int no_timer_check __initdata;

@@ -3639,7 +3643,7 @@ int __init io_apic_get_redir_entries (int ioapic)
 	return reg_01.bits.entries + 1;
 }
 
-void __init probe_nr_irqs_gsi(void)
+static void __init probe_nr_irqs_gsi(void)
 {
 	int nr;
 

@@ -3956,7 +3960,7 @@ static struct resource * __init ioapic_setup_resources(int nr_ioapics)
 	return res;
 }
 
-void __init ioapic_init_mappings(void)
+void __init ioapic_and_gsi_init(void)
 {
 	unsigned long ioapic_phys, idx = FIX_IO_APIC_BASE_0;
 	struct resource *ioapic_res;

@@ -3994,6 +3998,8 @@ fake_ioapic_page:
 		ioapic_res->end = ioapic_phys + IO_APIC_SLOT_SIZE - 1;
 		ioapic_res++;
 	}
+
+	probe_nr_irqs_gsi();
 }
 
 void __init ioapic_insert_resources(void)

@@ -4103,7 +4109,8 @@ void __init pre_init_apic_IRQ0(void)
 
 	printk(KERN_INFO "Early APIC setup for system timer0\n");
 #ifndef CONFIG_SMP
-	phys_cpu_present_map = physid_mask_of_physid(boot_cpu_physical_apicid);
+	physid_set_mask_of_physid(boot_cpu_physical_apicid,
+					 &phys_cpu_present_map);
 #endif
 	/* Make sure the irq descriptor is set up */
 	cfg = alloc_irq_and_cfg_at(0, 0);
arch/x86/kernel/setup.c +1 -4
index a0f52af..a25ce88 100644
@@ -1045,10 +1045,7 @@ void __init setup_arch(char **cmdline_p)
 #endif
 
 	init_apic_mappings();
-	ioapic_init_mappings();
-
-	/* need to wait for io_apic is mapped */
-	probe_nr_irqs_gsi();
+	ioapic_and_gsi_init();
 
 	kvm_guest_init();
arch/x86/mm/srat_32.c +1 -0
index a17dffd..f164345 100644
@@ -92,6 +92,7 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *cpu_affinity)
 	/* mark this node as "seen" in node bitmap */
 	BMAP_SET(pxm_bitmap, cpu_affinity->proximity_domain_lo);
 
+	/* don't need to check apic_id here, because it is always 8 bits */
 	apicid_to_pxm[cpu_affinity->apic_id] = cpu_affinity->proximity_domain_lo;
 
 	printk(KERN_DEBUG "CPU %02x in proximity domain %02x\n",
arch/x86/mm/srat_64.c +10 -0
index a35cb9d..171a0aa 100644
@@ -134,6 +134,10 @@ acpi_numa_x2apic_affinity_init(struct acpi_srat_x2apic_cpu_affinity *pa)
 	}
 
 	apic_id = pa->apic_id;
+	if (apic_id >= MAX_LOCAL_APIC) {
+		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%04x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		return;
+	}
 	apicid_to_node[apic_id] = node;
 	node_set(node, cpu_nodes_parsed);
 	acpi_numa = 1;

@@ -168,6 +172,12 @@ acpi_numa_processor_affinity_init(struct acpi_srat_cpu_affinity *pa)
 		apic_id = (pa->apic_id << 8) | pa->local_sapic_eid;
 	else
 		apic_id = pa->apic_id;
+
+	if (apic_id >= MAX_LOCAL_APIC) {
+		printk(KERN_INFO "SRAT: PXM %u -> APIC 0x%02x -> Node %u skipped apicid that is too big\n", pxm, apic_id, node);
+		return;
+	}
+
 	apicid_to_node[apic_id] = node;
 	node_set(node, cpu_nodes_parsed);
 	acpi_numa = 1;
arch/x86/platform/sfi/sfi.c +2 -2
index dd4c281..ca54875 100644
@@ -48,9 +48,9 @@ static void __init mp_sfi_register_lapic_address(unsigned long address)
 /* All CPUs enumerated by SFI must be present and enabled */
 static void __cpuinit mp_sfi_register_lapic(u8 id)
 {
-	if (MAX_APICS - id <= 0) {
+	if (MAX_LOCAL_APIC - id <= 0) {
 		pr_warning("Processor #%d invalid (max %d)\n",
-			id, MAX_APICS);
+			id, MAX_LOCAL_APIC);
 		return;
 	}
arch/x86/platform/visws/visws_quirks.c +1 -1
index 3371bd0..6320376 100644
@@ -171,7 +171,7 @@ static void __init MP_processor_info(struct mpc_cpu *m)
 	ver = m->apicver;
 	if ((ver >= 0x14 && m->apicid >= 0xff) || m->apicid >= 0xf) {
 		printk(KERN_ERR "Processor #%d INVALID. (Max ID: %d).\n",
-			m->apicid, MAX_APICS);
+			m->apicid, MAX_LOCAL_APIC);
 		return;
 	}
drivers/acpi/numa.c +12 -2
index 5718566..d9926af 100644
@@ -275,13 +275,23 @@ acpi_table_parse_srat(enum acpi_srat_type id,
 int __init acpi_numa_init(void)
 {
 	int ret = 0;
+	int nr_cpu_entries = nr_cpu_ids;
+
+#ifdef CONFIG_X86
+	/*
+	 * Should not limit number with cpu num that is from NR_CPUS or nr_cpus=
+	 * SRAT cpu entries could have different order with that in MADT.
+	 * So go over all cpu entries in SRAT to get apicid to node mapping.
+	 */
+	nr_cpu_entries = MAX_LOCAL_APIC;
+#endif
 
 	/* SRAT: Static Resource Affinity Table */
 	if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY,
-				     acpi_parse_x2apic_affinity, nr_cpu_ids);
+				     acpi_parse_x2apic_affinity, nr_cpu_entries);
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
-				     acpi_parse_processor_affinity, nr_cpu_ids);
+				     acpi_parse_processor_affinity, nr_cpu_entries);
 		ret = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
 					    acpi_parse_memory_affinity,
 					    NR_NODE_MEMBLKS);
#1 lore raw
Quoted 4 lines · Ingo Molnar

out-of-topic modifications in x86-apic-for-linus:
-------------------------------------------------
drivers/acpi/numa.c                # d3bd058: x86, acpi: Parse all SRAT cpu ent

Why is this x86-only? Aren't the same issues possible elsewhere?

Sure, ACPI is only used on x86 and ia64, and likely nobody cares about ia64, but that CONFIG_X86 still looks odd.

Linus

reviewer
#2 lore raw
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Ingo Molnar

out-of-topic modifications in x86-apic-for-linus:
-------------------------------------------------
drivers/acpi/numa.c # d3bd058: x86, acpi: Parse all SRAT cpu ent

Why is this x86-only? Aren't the same issues possible elsewhere?

Sure, ACPI is only used on x86 and ia64, and likely nobody cares about ia64, but that CONFIG_X86 still looks odd.

Tony, as representative of the (ACPI - x86) universe, could you comment?
I don't know if this is applicable on IA64.

-hpa

#3 lore raw
Quoted 1 line · H. Peter Anvin

 I don't know if this is applicable on IA64.

I think that ia64 might need something like this too. If I understand the change correctly, it makes sure that we read all entries for cpus from the SRAT table, so that we can figure out the topology for any cpus that we decide to use (which may be at arbitrary positions in the SRAT table as there is no guarantee that SRAT entries are in the same order as MADT entries ... we might try to boot the first four cpus we find in MADT, but those might be the last four entries in SRAT.)

It looks like this could be more simply achieved in an architecture neutral way by just passing "0" as the last argument to acpi_table_parse_srat() ... which will look at every entry in SRAT (this is not exactly the same ... but it seems unlikely that SRAT will have more than 32768 processor entries ... which you are apparently willing to allow for with the MAX_LOCAL_APIC value.)

-Tony

#4 lore raw

Taking a slight tangent ... it would seem that nr_cpus/maxcpus are woefully inadequate on NUMA systems. If I have a four node NUMA machine and I boot with maxcpus=4 ... I probably want one cpu on each node. I might want all four of the cpus from the same node (so I can check memory allocation from cpu-less nodes?). What I almost definitely don't want is the first four cpus that the BIOS listed in the MADT (which could be scattered in any way across nodes).

-Tony

#5 lore raw
In reply to Tony Luck

Untested on X86. Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

---

I left the comment ... though it might need some tweaking as it is kind of tenuously connected to the '0' argument passed to acpi_table_parse_srat.

drivers/acpi/numa.c +2 -6
index d9926af..5eb25eb 100644
@@ -275,23 +275,19 @@ acpi_table_parse_srat(enum acpi_srat_type id,
 int __init acpi_numa_init(void)
 {
 	int ret = 0;
-	int nr_cpu_entries = nr_cpu_ids;
 
-#ifdef CONFIG_X86
 	/*
 	 * Should not limit number with cpu num that is from NR_CPUS or nr_cpus=
 	 * SRAT cpu entries could have different order with that in MADT.
 	 * So go over all cpu entries in SRAT to get apicid to node mapping.
 	 */
-	nr_cpu_entries = MAX_LOCAL_APIC;
-#endif
 
 	/* SRAT: Static Resource Affinity Table */
 	if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY,
-				     acpi_parse_x2apic_affinity, nr_cpu_entries);
+				     acpi_parse_x2apic_affinity, 0);
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
-				     acpi_parse_processor_affinity, nr_cpu_entries);
+				     acpi_parse_processor_affinity, 0);
 		ret = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
 					    acpi_parse_memory_affinity,
 					    NR_NODE_MEMBLKS);
#6 lore raw
Quoted 4 lines · Tony

Untested on X86.  Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

Looks much better to me, and gets rid of that silly #ifdef that I reacted to.

But no hurry, this looks low-priority. So let's make this go through the x86 tree and get merged later, after it has passed whatever normal tests that the -tip tree goes through. Ok?

Linus

#7 lore raw
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Tony

Untested on X86.  Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

Looks much better to me, and gets rid of that silly #ifdef that I reacted to.

But no hurry, this looks low-priority. So let's make this go through the x86 tree and get merged later, after it has passed whatever normal tests that the -tip tree goes through. Ok?

yes. x86 is safe. We have apicid_to_node[] array, and already check apic id with MAX_LOCAL_APIC
in x86 version acpi_numa_processor_affinity_init().

looks ia64 is using struct array with cpu idx for mapping

        node_cpuid[srat_num_cpus].phys_id =
            (pa->apic_id << 8) | (pa->local_sapic_eid);
        /* nid should be overridden as logical node id later */
        node_cpuid[srat_num_cpus].nid = pxm;
        cpu_set(srat_num_cpus, early_cpu_possible_map);
        srat_num_cpus++;

but it does not check the boundary of that array...

struct node_cpuid_s node_cpuid[NR_CPUS]..

so if some one try to boot kernel with small NR_CPUS on bigger IA64 system, could get some variables node_cpuid[]...

Assume Tony will have another patch for IA64 to check that before apply this patch.

sth like

arch/ia64/kernel/acpi.c +2 -0
Index: linux-2.6/arch/ia64/kernel/acpi.c
===================================================================
@@ -477,6 +479,11 @@ acpi_numa_processor_affinity_init(struct
 	if (!(pa->flags & ACPI_SRAT_CPU_ENABLED))
 		return;
 
+	if (srat_num_cpus >= NR_CPUS) {
+		printk(KERN_WARNING "NR_CPUS (%d) is too small, not all cpus used.

%d\n", NR_CPUS);
+ return;
+ }
+
pxm = get_processor_proximity_domain(pa);

/* record this node in proximity bitmap */

Thanks

Yinghai

#8 lore raw
Quoted 8 lines · Yinghai Lu

looks ia64 is using struct array with cpu idx for mapping

       node_cpuid[srat_num_cpus].phys_id = (pa->apic_id << 8) | (pa->local_sapic_eid); /* nid should be overridden as logical node id later */ node_cpuid[srat_num_cpus].nid = pxm; cpu_set(srat_num_cpus, early_cpu_possible_map); srat_num_cpus++;

Good point.

Quoted 6 lines

+       if (srat_num_cpus >= NR_CPUS) {
+               printk(KERN_WARNING "NR_CPUS (%d) is too small, not all cpus used. %d\n", NR_CPUS);
+               return;
+       }
+

Looks ok - but printk_once() might be better so we don't flood the console if someone tries running an NR_CPUS=16 kernel on a monster SGI machine.

-Tony

#9 lore raw
In reply to Yinghai Lu
Quoted 44 lines · On Thu, 6 Jan 2011 16:17:10 -0800 Yinghai Lu wrote:
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Tony

Untested on X86.  Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

Looks much better to me, and gets rid of that silly #ifdef that I reacted to.

But no hurry, this looks low-priority. So let's make this go through the x86 tree and get merged later, after it has passed whatever normal tests that the -tip tree goes through. Ok?

yes. x86 is safe. We have apicid_to_node[] array, and already check apic id with MAX_LOCAL_APIC
in x86 version acpi_numa_processor_affinity_init().

looks ia64 is using struct array with cpu idx for mapping

        node_cpuid[srat_num_cpus].phys_id =
            (pa->apic_id << 8) | (pa->local_sapic_eid);
        /* nid should be overridden as logical node id later */
        node_cpuid[srat_num_cpus].nid = pxm;
        cpu_set(srat_num_cpus, early_cpu_possible_map);
        srat_num_cpus++;

but it does not check the boundary of that array...

struct node_cpuid_s node_cpuid[NR_CPUS]..

so if some one try to boot kernel with small NR_CPUS on bigger IA64 system, could get some variables node_cpuid[]...

Assume Tony will have another patch for IA64 to check that before apply this patch.

sth like

Index: linux-2.6/arch/ia64/kernel/acpi.c
===================================================================

arch/ia64/kernel/acpi.c acpi_numa_processor_affinity_init(struct · line 484

Quoted 6 lines
arch/ia64/kernel/acpi.c +2 -0
@@ -477,6 +479,11 @@ acpi_numa_processor_affinity_init(struct
 	if (!(pa->flags & ACPI_SRAT_CPU_ENABLED))
 		return;
 
+	if (srat_num_cpus >= NR_CPUS) {
+		printk(KERN_WARNING "NR_CPUS (%d) is too small, not all cpus used.
 %d\n", NR_CPUS);

CPUs used.

Don't need %d 2 times.

Quoted 6 lines
arch/ia64/kernel/acpi.c +3 -0
+		return;
+	}
+
 	pxm = get_processor_proximity_domain(pa);

/* record this node in proximity bitmap */

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

#10 lore raw
Quoted 49 lines · Randy Dunlap
Quoted 47 lines · On Thu, 6 Jan 2011 16:17:10 -0800 Yinghai Lu wrote:
Quoted 11 lines · Linus Torvalds
Quoted 4 lines · Tony

Untested on X86.  Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

Looks much better to me, and gets rid of that silly #ifdef that I reacted to.

But no hurry, this looks low-priority. So let's make this go through the x86 tree and get merged later, after it has passed whatever normal tests that the -tip tree goes through. Ok?

yes. x86 is safe. We have apicid_to_node[] array, and already check apic id with MAX_LOCAL_APIC
in x86 version acpi_numa_processor_affinity_init().

looks ia64 is using struct array with cpu idx for mapping

        node_cpuid[srat_num_cpus].phys_id = (pa->apic_id << 8) | (pa->local_sapic_eid); /* nid should be overridden as logical node id later */ node_cpuid[srat_num_cpus].nid = pxm; cpu_set(srat_num_cpus, early_cpu_possible_map); srat_num_cpus++;

but it does not check the boundary of that array...

struct node_cpuid_s node_cpuid[NR_CPUS]..

so if some one try to boot kernel with small NR_CPUS on bigger IA64 system, could get some variables node_cpuid[]...

Assume Tony will have another patch for IA64 to check that before apply this patch.

sth like

Index: linux-2.6/arch/ia64/kernel/acpi.c
===================================================================

arch/ia64/kernel/acpi.c +0 -0
@@ -477,6 +479,11 @@ acpi_numa_processor_affinity_init(struct
       if (!(pa->flags & ACPI_SRAT_CPU_ENABLED))
               return;
Quoted 8 lines

arch/ia64/kernel/acpi.c acpi_numa_processor_affinity_init(struct

Quoted 3 lines
arch/ia64/kernel/acpi.c +2 -0
+     if (srat_num_cpus >= NR_CPUS) {
+             printk(KERN_WARNING "NR_CPUS (%d) is too small, not all cpus used.
 %d\n", NR_CPUS);

                                                                       CPUs used.

Don't need %d 2 times.

right.

#11 lore raw
In reply to Yinghai Lu
Quoted 2 lines · Yinghai Lu

Assume Tony will have another patch for IA64 to check that before apply this patch.

Ingo,

Ok, I sent (a slightly modified version of) this patch up to Linus as part of the ia64 merge window pull ... so it is safe to update drivers/acpi/numa.c with a patch that will parse all the SRAT cpu entries down to ia64 now.

-Tony

Pasted version of commit - which gmail will munge ...

commit 05f2f274c8a8747bbfb13ac8ee0c27d5f2ad8510

Author: Tony Luck <[email protected]>
Date:   Fri Jan 7 09:11:55 2011 -0800

[IA64] Avoid array overflow if there are too many cpus in SRAT table

acpi_numa_init() has to parse the whole SRAT table, even if the kernel wants to limit the number of cpus it will use (because the ones it is going to use may be described by entries at the end of the SRAT table). Avoid overflowing the node_cpuid array.

Reported-by Yinghai Lu <[email protected]>
Signed-off-by Tony Luck <[email protected]>
arch/ia64/kernel/acpi.c +0 -0
index c6c90f3..7b897b7 100644
@@ -477,6 +477,12 @@ acpi_numa_processor_affinity_init(struct

acpi_srat_cpu_affinity *pa)

        if (!(pa->flags & ACPI_SRAT_CPU_ENABLED))
                return;

+ if (srat_num_cpus >= ARRAY_SIZE(node_cpuid)) {
+ printk_once(KERN_WARNING
+ "node_cpuid[%d] is too small, may not be
able to use all cpus\n",
+ ARRAY_SIZE(node_cpuid));
+ return;
+ }

        pxm = get_processor_proximity_domain(pa);

        /* record this node in proximity bitmap */
author
#12 lore raw
Quoted 10 lines · Tony Luck
Quoted 2 lines · Yinghai Lu

Assume Tony will have another patch for IA64 to check that before apply this patch.

Ingo,

Ok, I sent (a slightly modified version of) this patch up to Linus as part of the ia64 merge window pull ... so it is safe to update drivers/acpi/numa.c with a patch that will parse all the SRAT cpu entries down to ia64 now.

Thanks Tony!

Since the original commit d3bd058 is already upstream i suspect we should be fine all around, right?

Ingo

#13 lore raw
Quoted 14 lines · Ingo Molnar
Quoted 10 lines · Tony Luck
Quoted 2 lines · Yinghai Lu

Assume Tony will have another patch for IA64 to check that before apply this patch.

Ingo,

Ok, I sent (a slightly modified version of) this patch up to Linus as part of the ia64 merge window pull ... so it is safe to update drivers/acpi/numa.c with a patch that will parse all the SRAT cpu entries down to ia64 now.

Quoted 4 lines

Thanks Tony!

Since the original commit d3bd058 is already upstream i suspect we should be fine all around, right?

he mean this one:

Untested on X86. Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>

---

I left the comment ... though it might need some tweaking as it is kind of tenuously connected to the '0' argument passed to acpi_table_parse_srat.

drivers/acpi/numa.c +2 -6
index d9926af..5eb25eb 100644
@@ -275,23 +275,19 @@ acpi_table_parse_srat(enum acpi_srat_type id,
 int __init acpi_numa_init(void)
 {
 	int ret = 0;
-	int nr_cpu_entries = nr_cpu_ids;
 
-#ifdef CONFIG_X86
 	/*
 	 * Should not limit number with cpu num that is from NR_CPUS or nr_cpus=
 	 * SRAT cpu entries could have different order with that in MADT.
 	 * So go over all cpu entries in SRAT to get apicid to node mapping.
 	 */
-	nr_cpu_entries = MAX_LOCAL_APIC;
-#endif
 
 	/* SRAT: Static Resource Affinity Table */
 	if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY,
-				     acpi_parse_x2apic_affinity, nr_cpu_entries);
+				     acpi_parse_x2apic_affinity, 0);
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
-				     acpi_parse_processor_affinity, nr_cpu_entries);
+				     acpi_parse_processor_affinity, 0);
 		ret = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
 					    acpi_parse_memory_affinity,
 					    NR_NODE_MEMBLKS);
#14 lore raw

[tip:x86/urgent] x86, ia64, acpi: Clean up x86-ism in drivers/acpi/numa.c

Commit-ID:  9378b63ccb32b9c071dab155c96357ad1e52a709
Gitweb:     http://git.kernel.org/tip/9378b63ccb32b9c071dab155c96357ad1e52a709
Author:     Tony Luck <[email protected]>
AuthorDate: Wed, 12 Jan 2011 00:50:37 -0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 12 Jan 2011 12:15:09 +0100

x86, ia64, acpi: Clean up x86-ism in drivers/acpi/numa.c

As pointed out by Linus CONFIG_X86 in drivers/acpi/numa.c is ugly.

Builds and boots on ia64 (both normally and with maxcpus=8 to limit the number of cpus).

Signed-off-by Tony Luck <[email protected]>
Acked-by Yinghai Lu <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Wu Fengguang <[email protected]>
Cc Bjorn Helgaas <[email protected]>
Cc Len Brown <[email protected]>

LKML-Reference: <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>
---
 drivers/acpi/numa.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
drivers/acpi/numa.c +2 -6
index d9926af..5eb25eb 100644
@@ -275,23 +275,19 @@ acpi_table_parse_srat(enum acpi_srat_type id,
 int __init acpi_numa_init(void)
 {
 	int ret = 0;
-	int nr_cpu_entries = nr_cpu_ids;
 
-#ifdef CONFIG_X86
 	/*
 	 * Should not limit number with cpu num that is from NR_CPUS or nr_cpus=
 	 * SRAT cpu entries could have different order with that in MADT.
 	 * So go over all cpu entries in SRAT to get apicid to node mapping.
 	 */
-	nr_cpu_entries = MAX_LOCAL_APIC;
-#endif
 
 	/* SRAT: Static Resource Affinity Table */
 	if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_X2APIC_CPU_AFFINITY,
-				     acpi_parse_x2apic_affinity, nr_cpu_entries);
+				     acpi_parse_x2apic_affinity, 0);
 		acpi_table_parse_srat(ACPI_SRAT_TYPE_CPU_AFFINITY,
-				     acpi_parse_processor_affinity, nr_cpu_entries);
+				     acpi_parse_processor_affinity, 0);
 		ret = acpi_table_parse_srat(ACPI_SRAT_TYPE_MEMORY_AFFINITY,
 					    acpi_parse_memory_affinity,
 					    NR_NODE_MEMBLKS);