comments welcome. Please see notes in the patch.
ron
ron minnich wrote:
comments welcome. Please see notes in the patch.
Short version: Very nice!
I am not as interested in comments on this specific code (it needs cleanup) as I am in two questions:
I'll mention a few things that I would like to be included in that cleanup.
- can artec please test the current svn to make sure there is
nothing I have broken
Good point. Could be tested also on ALIX.
- Are the changes to lib/stage2.c ok
I think so.
Anyway, take a look. With luck, we have SMP on the kontron within the week; SMI follows, then ACPI, then maybe we can make v3 the preferred kontron software base.
It would be awesome!
+++ lib/stage2.c (working copy)
..
@@ -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);
The comment mentions phase 1 - but which phase 1 is that? Would help if it said cpu_phase1() or stage2_phase1() instead.
- movw $0x11, 0
Just curious, what do these movws to 0 do?
+++ arch/x86/intel/core2/init_cpus.c (working copy)
..
@@ -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);
Please make this debug output say ebx=%08x because even if that is redundant for the people looking at it right now, output like this can linger and anyone else may be confused.
@@ -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);
I guess this is missing ] after start_cpu_lock.
Overall great improvements!
//Peter
On Sat, Mar 7, 2009 at 2:05 AM, Peter Stuge peter@stuge.se wrote:
- movw $0x11, 0
Just curious, what do these movws to 0 do?
Glad you asked! These are how I got it debugged. Since we have another CPU (BSP) I had the BSP wait and print out the values. These are POST codes for AP startup! So BSP can now monitor AP progress with variables -- no more "worked"/"did not work" but "here's how far it got". The BSP has far more knowledge than in v2 of how far things got.
They movw to 0 going to be removed, I think; we can depend on secondary.S. Or should we? Should I leave them in?
Overall great improvements!
Thanks for the comments, I'm hoping you found the code understandable -- I am guessing you did?
Thanks for reading it!
ron
On 07.03.2009 16:37, ron minnich wrote:
On Sat, Mar 7, 2009 at 2:05 AM, Peter Stuge peter@stuge.se wrote:
movw $0x11, 0Just curious, what do these movws to 0 do?
Glad you asked! These are how I got it debugged. Since we have another CPU (BSP) I had the BSP wait and print out the values. These are POST codes for AP startup! So BSP can now monitor AP progress with variables -- no more "worked"/"did not work" but "here's how far it got". The BSP has far more knowledge than in v2 of how far things got.
They movw to 0 going to be removed, I think; we can depend on secondary.S. Or should we? Should I leave them in?
Please leave them in! More diagnostics are always welcome. If would be nice to be able to determine the AP core ID somehow from the AP POST code. You already use 4 bytes for that purpose. The first 2 bytes have a 0-extended POST code, the second 2 bytes have 0xbeef.
Regards, Carl-Daniel
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
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
On Tue, Mar 10, 2009 at 9:57 AM, ron minnich rminnich@gmail.com wrote:
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.
Were you going to do MTRRs in phase6?
Marc
On Tue, Mar 10, 2009 at 9:04 AM, Marc Jones marcj303@gmail.com wrote:
On Tue, Mar 10, 2009 at 9:57 AM, ron minnich rminnich@gmail.com wrote:
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.
Were you going to do MTRRs in phase6?
I would like to do them early. I don't see any reason not to do them in initram. That way the memory would run fast. Comments on that idea are welcome.
ron
On Tue, Mar 10, 2009 at 12:09 PM, ron minnich rminnich@gmail.com wrote:
On Tue, Mar 10, 2009 at 9:04 AM, Marc Jones marcj303@gmail.com wrote:
On Tue, Mar 10, 2009 at 9:57 AM, ron minnich rminnich@gmail.com wrote:
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.
Were you going to do MTRRs in phase6?
I would like to do them early. I don't see any reason not to do them in initram. That way the memory would run fast. Comments on that idea are welcome.
Yes, You should set the BSP at initram time. I meant which stage of AP init you would do it in.
Marc
On Tue, Mar 10, 2009 at 11:38 AM, Marc Jones marcj303@gmail.com wrote:
Yes, You should set the BSP at initram time. I meant which stage of AP init you would do it in.
no idea yet :-)
I am leaning toward cpu_phase1
ron
-
Carl-Daniel Hailfinger -
Marc Jones -
Peter Stuge -
ron minnich