[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 18:38:22 CEST 2008
On 14.10.2008 17:43, ron minnich wrote:
> Here is a version I put together yesterday as a straw main.
>
Nice. I'll comment inline.
> summary:
>
> stage1_main is now split into stage0_main and main(). stage0_main runs
> up to and including initram. It then calls disable_car.
>
I believe the stage0_main name is misleading. After all, stage0 is pure
asm and lives in its own .S file.
> disable_car does what it does now:
> copy CAR stack to ram stack, disable car,
> BUT:
> instead of a ret, it does a ljmp to main.
>
We agree on the concept of calling/jumping to the second part of the old
stage1_main().
> very little of disable_car has to change since we already copy stack
> to stack. We do need a new
> constant, RAM_STACK, or some such, so we know where stack goes in RAM.
>
Yes.
> 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.
> main is main. It calls everything else.
> I don't like names like xxx_eary and xxx_late-- they don't convey enough
> information. main() calls stage2 and payload and, later, code to load
> microcode (needed on k10) and possibly
> code to set up the resource maps (k8 and k10).
>
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.
> 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.
> This will fix issues with via and intel.
>
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.
> @@ -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.
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.
Part of your comment ("passed an on-stack variable") has been obsoleted
by your code which can work without that.
> + * 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;
> }
>
> struct global_vars *global_vars(void)
> @@ -85,7 +91,7 @@
> void global_vars_init(struct global_vars *globvars)
> {
> memset(globvars, 0, sizeof(struct global_vars));
> - *(struct global_vars **)(bottom_of_stack() - sizeof(struct global_vars *)) = globvars;
> + *(struct global_vars **)(bottom_of_stack(vars) - sizeof(struct global_vars *)) = globvars;
> #ifdef CONFIG_CONSOLE_BUFFER
> /* Initialize the printk buffer. */
> printk_buffer_init();
> @@ -117,25 +123,6 @@
> return ret;
> }
>
> -#ifdef CONFIG_PAYLOAD_ELF_LOADER
> -/* until we get rid of elf */
> -int legacy(struct mem_file *archive, char *name, void *where, struct lb_memory *mem)
> -{
> - int ret;
> - int elfboot_mem(struct lb_memory *mem, void *where, int size);
> - ret = copy_file(archive, name, where);
> - if (ret) {
> - printk(BIOS_ERR, "'%s' found, but could not load it.\n", name);
> - }
> -
> - ret = elfboot_mem(mem, where, archive->reallen);
> -
> - printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> - return -1;
> -}
> -#endif /* CONFIG_PAYLOAD_ELF_LOADER */
> -
> -
> static int run_address_multiboot(void *f)
> {
> int ret, dummy;
> @@ -154,31 +141,15 @@
> * 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)) stage0_main(u32 bist, u32 init_detected)
>
stage0 is asm. This is stage1_main() if anything.
> {
> 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);
> + post_code(POST_STAGE0_MAIN);
>
> - /* 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
> * on the bootstrap processor.
> * stop_ap is responsible for NOT stopping the BSP
> @@ -235,8 +206,18 @@
> printk(BIOS_DEBUG, "Done RAM init code\n");
>
> /* Turn off Cache-As-Ram */
> + /* as a side effect this calls main() */
> + /* and copies global_vars from the CAR stack to the RAM stack */
> disable_car();
>
> +}
> +
> +/** this is called from disable_car. It calls stage2 (which returns to it)
> + * and hence is not called 'stage1'
> + */
> +void __attribute__((stdcall)) main(u32 bist, u32 init_detected)
> +{
> +
> #ifdef CONFIG_CONSOLE_BUFFER
> /* Move the printk buffer to PRINTK_BUF_ADDR_RAM */
> printk_buffer_move((void *)PRINTK_BUF_ADDR_RAM, PRINTK_BUF_SIZE_RAM);
> @@ -251,12 +232,6 @@
>
> printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> -#ifdef CONFIG_PAYLOAD_ELF_LOADER
> - ret = find_file(&archive, "normal/payload", &result);
> - if (! ret)
> - legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
> -#endif /* CONFIG_PAYLOAD_ELF_LOADER */
> -
> entry = load_file_segments(&archive, "normal/payload");
> if (entry != (void*)-1) {
> /* Final coreboot call before handing off to the payload. */
> @@ -269,3 +244,4 @@
> }
>
>
> +
>
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the coreboot
mailing list