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

[ron minnich]

On Tue, Oct 14, 2008 at 9:38 AM, Carl-Daniel Hailfinger
> 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()



>> I don't see a need for swtich_stack, since disable_car pretty much
>> does that now -- it copies stacks, and all it need do is change %esp.
>>
>
> switch_stack was intended to act as a placeholder for everything related
> to stack switching.

OK but just don't forget -- it's hard to call switch_stack. I think it
might as well remain where it is -- inline assembly.


> I have no strong opinion on the names of functions as long as we don't
> actively confuse developers.
>
>> We should move the LAR pointer into
>> global variables so it will be find-able after disable_car. We should
>> not call the LAR init twice.
>>
>
> Agreed. Can do. Though if we want to store anything in global variables,
> we have to be careful not to store pointers to something on the stack.

Good point. But I think we're safe on that issue.

>
>> The bottom_of_stack will now work for any stack, not just CAR
>> stack.Given a properly aligned stack
>> it will always work.
>>
>
> Sorry, it won't. Well, it might work by accident for some processors in
> some pieces of code, but it will definitely fail on real hardware
> somewhere before/in stage2. I just can't see how you manage to have the
> stack aligned at a 512k boundary between 640k and 1023k.

512k boundary? That code should work if we align to, e.g., an 8k
boundary with an 8k stack.
Do we ever need more than an 8k stack? I hope not :-)

> I took the liberty of diffing your code against svn HEAD.
>
>> Index: stage1.c
>> ===================================================================
>> --- stage1.c  (Revision 925)
>> +++ stage1.c  (Arbeitskopie)
>> @@ -31,13 +31,6 @@
>>  #include <cpu.h>
>>  #include <multiboot.h>
>>
>> -#ifdef CONFIG_PAYLOAD_ELF_LOADER
>> -/* ah, well, what a mess! This is a hard code. FIX ME but how?
>> - * By getting rid of ELF ...
>> - */
>> -#define UNCOMPRESS_AREA (0x400000)
>> -#endif /* CONFIG_PAYLOAD_ELF_LOADER */
>> -
>>  /* these prototypes should go into headers */
>>  void uart_init(void);
>>  void die(const char *msg);
>>
>
> Actually, getting rid of the ELF loader is not something I'm opposed to.
> But IIRC Stefan wants to be have LAR fixed first. My patch allows us to
> keep the code even in the stack switch case.

yeah, let's keep ELF, that's just my obsession with killing it :-)


>
>> @@ -67,14 +60,27 @@
>>
>>  }
>>
>> -/*
>> - * The name is slightly misleading because this is the initial stack pointer,
>> - * not the address of the first element on the stack.
>> +/**
>> + * returns bottom of stack, when passed an on-stack variable such as a
>> + * parameter or automatic. Stack base must be STACKSIZE aligned
>> + * and STACKSIZE must be a power of 2 for this to work.
>> + * Luckily these rules have always been true.
>>
>
> These rules have been violated on so many levels for quite some time or
> will need to be violated:
> - Fam10h AP stacks are byte-aligned, not even aligned to 16 bytes or so.
> - K8 and Fam10h BSP stacks have a maximum CAR size of 48k (not a power of 2)
> - We must use 48k CAR for newer multicore multiprocessor machines
> - Although we still can align BSP stacks to a 64k boundary, lzma
> decompression scratchpad and other stack hogs can cause the stack to
> grow larger than 64k, giving you a stack base location which is off by
> 64k or more.

oh. Wow. OK, let's go with a  different bottom_of_stack function. Did
not realize that LZMA
was so stack-hungry.

>
> What might work, though, is comparing the current stack location against
> the predefined values of CAR stack location and the computed value of
> RAM stack location.

yes.

If the stack is > CARBASE, then it is the CARBASE + CARSIZE - 4. If < carbase,
it is the RAMBASE + RAMSIZE - 4. Done. But let's make it flexible.

>
> Part of your comment ("passed an on-stack variable") has been obsoleted
> by your code which can work without that.

oops.

>
>> + * Recall that on stacks that grow down, "bottom of stack" is really
>> + * at "top of memory"!
>> + * This function should work for any stack -- CAR or non-CAR.
>> + *
>>   */
>>  void *bottom_of_stack(void)
>>  {
>> -     /* -4 because CONFIG_CARBASE + CONFIG_CARSIZE - 4 is initial %esp */
>> -     return (void *)(CONFIG_CARBASE + CONFIG_CARSIZE - 4);
>> +     /* this is weird, eh? Assigning something its own address.
>> +      * But it means that we have a %esp value in 'stack'
>> +      */
>> +     u32 stack = (u32) &stack;
>> +     stack &= STACKSIZE;
>> +     stack += STACKSIZE;
>> +     /* -4 because -4 is initial %esp */
>> +     stack -= 4;
>> +     return (void *) stack;
>>  }

ok, it won't work, but you have to admit it's kind of slick.


Do you want to take one more pass at this? Your ideas were more
attuned to hardware reality than mine. I think we're close and we need
to get Corey supported on via very soon :-)

ron