[coreboot] [RFC] v3: Stack switching abstraction for C7 and later Intel processors

[Carl-Daniel Hailfinger]

On 15.10.2008 17:31, ron minnich wrote:
> On Wed, Oct 15, 2008 at 4:46 AM, Carl-Daniel Hailfinger
> <c-d.hailfinger.devel.2006 at gmx.net> 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 <c-d.hailfinger.devel.2006 at gmx.net>
>>
>> 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/