[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


ron



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

thanks

ron




More information about the coreboot mailing list