[coreboot] patch: working SMP startup for kontron/core2

ron minnich rminnich at gmail.com
Tue Mar 10 16:57:22 CET 2009

This is in response to the very good comments i got.

kontron is booting with this *with VGA*. Weirdly, serial and vga work
but not keyboard (!).

One issue is that the MTRRs are totally wrong, so that it runs slowly.
Three other issues:
- stack layout is wrong (see below)
- no SMI
- no ACPI


>>       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.

not at all! It's one of the few useful things you can do in C :-)

I'm leaving it in; it's too handy. Negative indices should not be
scary. You just have to be careful.

gcc should not scream warnings here because this is the kind of thing
C was designed for.

> 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.

yep, I got it upside down, fix will be in the next rev. I'd like to
get it in in this partial state for now.



More information about the coreboot mailing list