[coreboot] [RFC] v3: Stack switching abstraction for C7 and later Intel processors
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Tue Oct 14 19:57:14 CEST 2008
On 14.10.2008 18:55, ron minnich wrote:
> 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()
>
Works for me.
>>> 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.
>
Agreed. It was just a skeleton to show others the intended control flow.
>> 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.
>
Good. A manual code review of global variable additions will work nicely
to avoid that.
>>> 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 :-)
>
Oh, I'd also see it go away rather sooner than later.
>>> @@ -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.
>
Hm yes, something along these lines. Which leads to an interesting
question: Where do we want the stack if we have to move it? Top of memory?
>> Part of your comment ("passed an on-stack variable") has been obsoleted
>> by your code which can work without that.
>>
>
> oops.
>
I always take it as a good sign if the code is ahead of the comments.
>>> + * 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.
>
Yes, absolutely. I had such code in my tree before I first submitted the
framework, but then I discovered that it wouldn't work always.
> 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 :-)
>
I can mail a new version to the list in ~2 hours. If that's OK for you,
I'll go ahead.
Regards
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list