On 15.10.2008 17:31, ron minnich wrote: > On Wed, Oct 15, 2008 at 4:46 AM, Carl-Daniel Hailfinger > <[EMAIL PROTECTED]> wrote: > >> On 14.10.2008 21:15, Peter Stuge wrote: >> >>> Carl-Daniel Hailfinger wrote: >>> >>> >>>>>> I believe the stage0_main name is misleading. After all, stage0 >>>>>> is pure asm and lives in its own .S file. >>>>>> >>>>>> >>>>> let's call it stage1 then and main() >>>>> >>>>> >>>> Works for me. >>>> >>>> >>> I'm afraid I don't like that. >>> >>> Please suggest something that makes the timeline obvious. >>> I think we already have other problems like this in v3. >>> >>> I would be OK with adding phases to stage1 e.g. but I have also >>> contemplated flattening the stage/phase tree to only have stages and >>> no phases - though that doesn't have to happen right now. >>> >>> >> OK, so can we settle on phases? >> >> old -> new >> stage1_main before disable_car-> stage1_phase1 >> disable_car and stack switch -> stage1_phase2 >> stage1_main after stack switch -> stage1_phase3 >> > > sounds good. > > >> And yes, I'm aware that the patch below doesn't change the corresponding >> asm code yet. I'll work on that as soon as we have agreed on stage1_* >> naming. >> >> Signed-off-by: Carl-Daniel Hailfinger <[EMAIL PROTECTED]> >> >> Index: corebootv3-stackrebuild/arch/x86/stage1.c >> =================================================================== >> --- corebootv3-stackrebuild/arch/x86/stage1.c (Revision 925) >> +++ corebootv3-stackrebuild/arch/x86/stage1.c (Arbeitskopie) >> @@ -143,6 +143,9 @@ >> return ret; >> } >> >> +void stage1_phase2(void); >> +void __attribute__((stdcall)) stage1_phase3(void); >> + >> /** >> * This function is called from assembler code with its argument on the >> * stack. Force the compiler to generate always correct code for this case. >> @@ -154,29 +157,13 @@ >> * that we are restarting after some sort of reconfiguration. Note that we >> could use it on geode but >> * do not at present. >> */ >> -void __attribute__((stdcall)) stage1_main(u32 bist, u32 init_detected) >> +void __attribute__((stdcall)) stage1_phase1(u32 bist, u32 init_detected) >> { >> struct global_vars globvars; >> int ret; >> struct mem_file archive; >> - void *entry; >> struct node_core_id me; >> -#ifdef CONFIG_PAYLOAD_ELF_LOADER >> - struct mem_file result; >> - int elfboot_mem(struct lb_memory *mem, void *where, int size); >> >> - /* Why can't we statically init this hack? */ >> - unsigned char faker[64]; >> - struct lb_memory *mem = (struct lb_memory*) faker; >> - >> - mem->tag = LB_TAG_MEMORY; >> - mem->size = 28; >> - mem->map[0].start.lo = mem->map[0].start.hi = 0; >> - mem->map[0].size.lo = (32*1024*1024); >> - mem->map[0].size.hi = 0; >> - mem->map[0].type = LB_MEM_RAM; >> -#endif /* CONFIG_PAYLOAD_ELF_LOADER */ >> - >> post_code(POST_STAGE1_MAIN); >> >> /* before we do anything, we want to stop if we do not run >> @@ -234,6 +221,29 @@ >> >> printk(BIOS_DEBUG, "Done RAM init code\n"); >> >> + /* Switch the stack location from CAR to RAM, rebuild the stack, >> + * disable CAR and continue at stage1_phase3(). This is all wrapped >> in >> + * stage1_phase2() to make the code easier to follow. >> + * We will NEVER return. >> + */ >> + stage1_phase2(); >> + >> + /* If we reach this point, something went terribly wrong. */ >> + die("The world is broken.\n"); >> +} >> + >> +/** >> + * This function is called to take care of switching and rebuilding the >> stack >> + * so that we can cope with processors which don't support a CAR area at low >> + * addresses where CAR could be copied to RAM without problems. >> + * After the stack has been rebuilt, we switch the stack pointer to the new >> + * location, move the printk buffer and cotinue at stage1_phase3(). >> + * >> + * NOTE: switch_stack may need to be reimplemented in processor-specific >> asm. >> + * TODO: Reevaluate whether printk_buffer_move should come before >> disable_car() >> + */ >> +void stage1_phase2() >> +{ >> /* Turn off Cache-As-Ram */ >> disable_car(); >> >> @@ -241,7 +251,37 @@ >> /* Move the printk buffer to PRINTK_BUF_ADDR_RAM */ >> printk_buffer_move((void *)PRINTK_BUF_ADDR_RAM, PRINTK_BUF_SIZE_RAM); >> #endif >> + stage1_phase3(); >> +} >> > > per our earlier discussion, do you really want to try to call more > functions in stage1_phase2 after you've changed the stack, given > assumptions gcc may have made?
Absolutely not. You're right about this. However, right now my focus is on keeping changes isolated and I'd take care of this in a separate patch. > Or do you want the printk_buffer_move > in stage1_phase3? > I'd prefer to have printk_buffer_move as first instruction in stage1_phase2. > Either way, we have to move forward, let's see the assembly, I'll test > on geode to make sure there is no breakage, then we can move forward > for Corey. > I'll be back in ~3 hours. If you have acked this in the meanwhile, I will commit, otherwise I'll rework. Regards, Carl-Daniel -- http://www.hailfinger.org/ -- coreboot mailing list: coreboot@coreboot.org http://www.coreboot.org/mailman/listinfo/coreboot