[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linuxbios
Subject:    Re: [coreboot] MP table multicore patch
From:       Stefan Reinauer <stepan () coresystems ! de>
Date:       2010-02-16 19:02:13
Message-ID: 4B7AEBB5.9070704 () coresystems ! de
[Download RAW message or body]

On 2/16/10 5:11 AM, Timothy Pearson wrote:
> Here is a cleaned up and tested version of the SMP APIC autodetect patch.
>
> Signed-off-by: Timothy Pearson <tpearson@raptorengineeringinc.com>
>
> ---
>
> It would of course be helpful to attach the patch.  My Webmail client
> keeps eating it...
>
> Timothy Pearson
> Raptor Engineering
>   


Do you have a comparison mptable as generated by the old and the new
version on your system?

And, can you describe high level, what the patch changes? It looks to me
as if you are recursing through the tree instead of just walking the
"all_devices" list.
So this implies that you don't catch all devices when running through
all_devices. This sounds like a problem in the resource allocator and
maybe it should be fixed there instead?

The code as it is runs in roughly O(n^3) and on top of that calls itself
which is a little bit scary.
But, looking at it a bit closer I see that the outer loop with p_it does
nothing. I dropped the outer loop and moved the lapic stuff to the inner
loop  where it is used, except passing it around on the stack. It will
probably mean 3 more cpuid calls and 3 more lapic reads as opposed to
heavy stack usage, but I think the stack usage has been more troublesome
in the past..

Please let me know what you think...

Stefan

["smp_fix_clean-v2.diff" (text/plain)]

Index: src/arch/i386/smp/mpspec.c
===================================================================
--- src/arch/i386/smp/mpspec.c	(revision 5127)
+++ src/arch/i386/smp/mpspec.c	(working copy)
@@ -91,6 +91,52 @@
 	smp_add_mpc_entry(mc, sizeof(*mpc));
 }
 
+void smp_scan_for_apics(struct mp_config_table *mc, struct device *parent)
+{
+	int c_it;
+	struct device *child;
+	/* Scan all links of the root node for APIC clusters and APICs */
+	for(c_it=0; c_it < parent->links; c_it++) {
+		for (child = parent->link[c_it].children; child; child = child->sibling) {
+			/* Is this an APIC? */
+			if (child->path.type == DEVICE_PATH_APIC) {
+				int boot_apic_id;
+				unsigned apic_version;
+				unsigned long cpu_flag;
+				unsigned cpu_features;
+				unsigned cpu_feature_flags;
+				struct cpuid_result result;
+
+				if (!child->enabled)
+					continue;
+
+				/* Found an APIC, add it to the MP table */
+				boot_apic_id = lapicid();
+				apic_version = lapic_read(LAPIC_LVR) & 0xff;
+
+				result = cpuid(1);
+				cpu_features = result.eax;
+				cpu_feature_flags = result.edx;
+
+				cpu_flag = MPC_CPU_ENABLED;
+				if (boot_apic_id == child->path.apic.apic_id) {
+					cpu_flag = MPC_CPU_ENABLED | MPC_CPU_BOOTPROCESSOR;
+				}
+				smp_write_processor(mc, 
+					child->path.apic.apic_id, apic_version,
+					cpu_flag, cpu_features, cpu_feature_flags
+				);
+			}
+
+			// Or an APIC cluster?
+			if (child->path.type == DEVICE_PATH_APIC_CLUSTER) {
+				// Found an APIC cluster, scan it for APICs
+				smp_scan_for_apics(mc, child);
+			}
+		}
+	}
+}
+
 /* If we assume a symmetric processor configuration we can
  * get all of the information we need to write the processor
  * entry from the bootstrap processor.
@@ -100,37 +146,8 @@
  */
 void smp_write_processors(struct mp_config_table *mc)
 {
-	int boot_apic_id;
-	unsigned apic_version;
-	unsigned cpu_features;
-	unsigned cpu_feature_flags;
-	struct cpuid_result result;
-	device_t cpu;
-	
-	boot_apic_id = lapicid();
-	apic_version = lapic_read(LAPIC_LVR) & 0xff;
-	result = cpuid(1);
-	cpu_features = result.eax;
-	cpu_feature_flags = result.edx;
-	for(cpu = all_devices; cpu; cpu = cpu->next) {
-		unsigned long cpu_flag;
-		if ((cpu->path.type != DEVICE_PATH_APIC) || 
-			(cpu->bus->dev->path.type != DEVICE_PATH_APIC_CLUSTER))
-		{
-			continue;
-		}
-		if (!cpu->enabled) {
-			continue;
-		}
-		cpu_flag = MPC_CPU_ENABLED;
-		if (boot_apic_id == cpu->path.apic.apic_id) {
-			cpu_flag = MPC_CPU_ENABLED | MPC_CPU_BOOTPROCESSOR;
-		}
-		smp_write_processor(mc, 
-			cpu->path.apic.apic_id, apic_version,
-			cpu_flag, cpu_features, cpu_feature_flags
-		);
-	}
+	// Scan the root node for APIC clusters and APICs
+	smp_scan_for_apics(mc, all_devices);
 }
 
 void smp_write_bus(struct mp_config_table *mc,
@@ -179,7 +196,6 @@
 #ifdef DEBUG_MPTABLE
 	printk_debug("add intsrc srcbus 0x%x srcbusirq 0x%x, dstapic 0x%x, dstirq 0x%x\n",
 				srcbus, srcbusirq, dstapic, dstirq);
-	hexdump(__func__, mpc, sizeof(*mpc));
 #endif
 }
 


-- 
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

[prev in list] [next in list] [prev in thread] [next in thread]