On 07.03.2009 07:05, ron minnich wrote:
Index: lib/stage2.c
--- lib/stage2.c (revision 1143) +++ lib/stage2.c (working copy) @@ -27,7 +27,22 @@ #include <console.h> #include <device/device.h> #include <tables.h> +#include <globalvars.h>
+unsigned int __attribute__((weak)) cpu_phase1(unsigned int coldboot,
struct sys_info *sysinfo)+{
- printk(BIOS_SPEW, "cpu_phase1: nothing to do\n");
You may want to print the content of the coldboot param here.
- return 0;
+}
+unsigned int __attribute__((weak)) cpu_phase2(unsigned int coldboot,
struct sys_info *sysinfo)+{
- printk(BIOS_SPEW, "cpu_phase2: nothing to do\n");
Dito.
- return 0;
+}
/**
- Main function of the DRAM part of coreboot.
@@ -44,8 +59,12 @@ void *stage2(void) { void *mbi;
- struct sys_info *sysinfo;
- int is_coldboot(void);
This prototype should live in a header file.
- struct global_vars *global_vars(void);
This prototype is in globalvars.h IIRC, so it can be removed here.
post_code(POST_STAGE2_BEGIN);
sysinfo = &(global_vars()->sys_info);
cpu_phase1(is_coldboot(), sysinfo); dev_init();
/* Phase 1 was console init and making printk work. Both functions are
@@ -85,6 +104,11 @@ dev_phase6(); show_all_devs(BIOS_DEBUG, "After phase 6.");
- /* final cleanup: do any remaining CPU setup. This can include memory
* init, or not, depending on the CPU; it may have been done in phase 1.*/- cpu_phase2(is_coldboot(), sysinfo);
- /* Write tables to pass information to the payloads. */ post_code(POST_STAGE2_WRITE_TABLES); mbi = write_tables();
Index: arch/x86/secondary.S
--- arch/x86/secondary.S (revision 1143) +++ arch/x86/secondary.S (working copy) @@ -62,24 +62,28 @@ movw $8, 0 movl %eax, %cr0 movw $9, 0
hlt
/* tested to this point but not past it */
/* I am pretty sure this just jumps back into
- ROM; it's an abs jump
*/
data32 ljmp $0x10, $secondary32
- data32 ljmp $0x8, $secondary32
The code line above is a bit confusing and the comment does not really explain what happens.
movw $0xa, 0 1: .code32 secondary32:
- hlt
- movw $0x18, %ax
- movw $0x11, 0
- movw $0x10, %ax
+// movw $0x12, 0 movw %ax, %ds
- movw $0x13, 0 movw %ax, %es
+// movw $0x14, 0 movw %ax, %ss +// movw $0x15, 0 movw %ax, %fs +// movw $0x16, 0 movw %ax, %gs
- movw $0x17, 0
Can you either activate all movw $something, 0 or deactivate all of them?
/* Load the Interrupt descriptor table */ lidt idtarg @@ -87,6 +91,8 @@ /* Set the stack pointer */ movl -4(%ebx),%esp movl $0, -4(%ebx) call secondary_cpu_init 1: hlt @@ -108,14 +114,6 @@ /* selgdt 0x10, flat data segment */ .word 0xffff, 0x0000 .byte 0x00, 0x93, 0xcf, 0x00
- /* selgdt 0x18, flat code segment for CAR */
- .word 0xffff, 0x0000
- .byte 0x00, 0x9b, 0xcf, 0x00
- /* selgdt 0x20, flat data segment for CAR */
- .word 0xffff, 0x0000
- .byte 0x00, 0x93, 0xcf, 0x00
gdt_end:
Index: arch/x86/intel/core2/init_cpus.c
--- arch/x86/intel/core2/init_cpus.c (revision 1143) +++ arch/x86/intel/core2/init_cpus.c (working copy) @@ -21,7 +21,6 @@
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
*/ -#include <mainboard.h> #include <types.h> #include <lib.h> #include <console.h> @@ -73,6 +72,7 @@ int nodes, siblings; result = cpuid(1); /* See how many sibling cpus we have */
- printk(BIOS_DEBUG, "cpuid(1) %x\n", result.ebx);
Maybe mention that you're printing ebx.
siblings = (result.ebx >> 16) & 0xff; if (siblings < 1) { siblings = 1; @@ -204,6 +204,9 @@ * Starting actual IPI sequence... */
- printk(BIOS_SPEW, "Before Startup.apicid %ld\n", apicid);
- printk(BIOS_SPEW, "Before Startup.sb[0] %p @0 %p\n",
(void *) secondary_base[0], (void *) *(u32 *) 0);
I'm not sure why you're casting *(u32 *)0 to (void *) here. If you change the printk format to %08lx, you can avoid the cast and improve readability. Like this: printk(BIOS_SPEW, "Before Startup.sb[0] %p @0 %08lx\n", (void *) secondary_base[0], *(u32 *) 0);
printk(BIOS_SPEW, "Asserting INIT.\n");
/* @@ -321,7 +324,11 @@ if (send_status || accept_status) break; }
- printk(BIOS_SPEW, "udelay(1000000)\n");
- udelay(1000000); printk(BIOS_SPEW, "After Startup.sb[-1] %p\n", (void *) secondary_base[-1]);
No offense, but doesn't gcc scream warnings all over the place here? Do you really want the memory contents of secondary_base[-1] or do you want &secondary_base[-1] instead? Hm. Looking at the code again, the -1 seems to be OK, but negative array indices are somewhere between clever and scary.
- printk(BIOS_SPEW, "After Startup.sb[0] %p @0 %p\n",
(void *) secondary_base[0], (void *) *(u32 *) 0);
Second cast of the contents of the zero address to void * is unneeded.
if (send_status) printk(BIOS_WARNING, "APIC never delivered???\n"); if (accept_status) @@ -377,18 +384,25 @@ stackmem->stacks[index].post = 0; stackmem->stacks[index].active_cpus = active_cpus; stackmem->stacks[index].start_cpu_lock = start_cpu_lock;
printk(BIOS_SPEW, "stack[index, apicid, post, active_cpus, start_cpu_lock = [%lx, %x, %d, %p, %p]\n", index, apicid, 0, active_cpus, start_cpu_lock); /* Advertise the new stack to start_cpu */ printk(BIOS_SPEW, "Set stack @ %p to %p\n", &secondary_base[-1], (void *)stack_end); secondary_base[-1] = stack_end;
/* Start the cpu */ result = lapic_start_cpu(apicid, secondary_base);
printk(BIOS_SPEW, "we think we started it. The stack value is 0x%p (should be 0)\n", (void *)secondary_base[-1]);
if (result) {
printk(BIOS_SPEW, "Spinning on post which is now 0x%x\n",stackmem->stacks[index].post);result = 0; /* Wait 1s or until the new the new cpu calls in */
for(count = 0; count < 100000 ; count++) {if (stackmem->stacks[index].post) {
for(count = 0; count < 1000000 ; count++) {printk(BIOS_SPEW,"BSP post 0x%x\n",stackmem->stacks[index].post);if (stackmem->stacks[index].post >= AP_STOP_OK) { result = 1; break; }@@ -476,9 +490,12 @@ struct atomic *active_cpus, struct spinlock *start_cpu_lock) { +printk(BIOS_SPEW, "secondary start\n"); post = AP_START; +printk(BIOS_SPEW, "secondary post %d\n", post); atomic_inc(active_cpus); post = AP_ACTIVEUP; +printk(BIOS_SPEW, "secondary post %d\n", post); if (SERIAL_CPU_INIT && (CONFIG_MAX_PHYSICAL_CPUS > 2)) spin_lock(start_cpu_lock); post = AP_LOCKED; @@ -559,7 +576,7 @@
- @param sysinfo The sys_info pointer
- @returns the BSP APIC ID
*/ -unsigned int init_cpus(unsigned cpu_init_detectedx, +unsigned int cpu_phase1(unsigned cpu_init_detectedx, struct sys_info *sysinfo) { /* Number of cpus that are currently running in coreboot */
There is something about stack setup which smells just wrong. Code excerpts follow.
struct stack { u32 post; u32 index; u32 apicid; struct atomic *active_cpus; struct spinlock *start_cpu_lock; u32 callerpc; u32 data[16384/sizeof(u32) - 7]; }; [...] stack_end = (u32)&stackmem->stacks[index].data;
stack_end is now the address of where downwards grow of the stack must end, NOT the top of a pristine stack aka initial stack pointer.
void __attribute__((regparm(0))) secondary_cpu_init( u32 post, u32 index, u32 apicid, struct atomic *active_cpus, struct spinlock *start_cpu_lock) { [...]
I guess the function signature is intended to match struct stack, but if that is the case, you will overflow the stack. The data member of struct stack is at the wrong end of the struct.
Regards, Carl-Daniel