[PATCH] v3: Move stage1 global variable management from asm to C

[Carl-Daniel Hailfinger]

Move stage1 global variable management from asm to C. The stage0 asm
code now unconditionally pushes an empty pointer to the stack which is a
placeholder for the pointer to global variable storage. That pointer and
the global variable storage are initialized in global_vars_init().

Build tested on all targets, boot tested on Qemu.

NOTES:
- The code is not yet MP safe, but that's due to v3 not being MP safe in
general (and the comments contradict the code regarding MP features).
- K8 code now works by accident.
- Segher needs to review which attributes globvars needs to not be
optimized away.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-global_vars_in_c/include/console.h
===================================================================
--- corebootv3-global_vars_in_c/include/console.h	(Revision 782)
+++ corebootv3-global_vars_in_c/include/console.h	(Arbeitskopie)
@@ -60,10 +60,10 @@
 #endif

/*
- * If you change struct global_vars in any way, you have to fix all stage0 asm
- * code. The stage0 asm code modification is nontrivial (size of the struct,
- * alignment, initialization, order of struct members, initialization).
- * Depending on your compiler, real breakage may happen.
+ * struct global_vars is managed entirely from C code. Keep in mind that there
+ * is NO buffer at the end of the struct, so having zero-sized arrays at the
+ * end or similar stuff for which the compiler can't determine the final size
+ * will corrupt memory. If you don't try to be clever, everything will be fine.
  */
 struct global_vars {
 #ifdef CONFIG_CONSOLE_BUFFER
Index: corebootv3-global_vars_in_c/arch/x86/geodelx/stage0.S
===================================================================
--- corebootv3-global_vars_in_c/arch/x86/geodelx/stage0.S	(Revision 782)
+++ corebootv3-global_vars_in_c/arch/x86/geodelx/stage0.S	(Arbeitskopie)
@@ -361,13 +361,10 @@
    movw	%ax, %ss

lout:
-#ifdef CONFIG_CONSOLE_BUFFER
-	/* Store pointer to start of printk buffer, should really use
-	 * PRINTK_BUF_ADDR_CAR instead.
-	 */
-	movl    $CONFIG_CARBASE, %eax
-	pushl   %eax  /* printk buffer */
-#endif
+	/* Store zero for the pointer to the global variables. */
+	movl    $0, %eax
+	pushl   %eax
+
    /* Restore the BIST result. */
    movl	%ebp, %eax

Index: corebootv3-global_vars_in_c/arch/x86/stage0_i586.S
===================================================================
--- corebootv3-global_vars_in_c/arch/x86/stage0_i586.S	(Revision 782)
+++ corebootv3-global_vars_in_c/arch/x86/stage0_i586.S	(Arbeitskopie)
@@ -435,13 +435,10 @@
    movw    %ax, %ss

lout:
-#ifdef CONFIG_CONSOLE_BUFFER
-	/* Store pointer to start of printk buffer, should really use
-	 * PRINTK_BUF_ADDR_CAR instead.
-	 */
-	movl    $CONFIG_CARBASE, %eax
-	pushl 	%eax  /* printk buffer */
-#endif
+	/* Store zero for the pointer to the global variables. */
+	movl    $0, %eax
+	pushl   %eax
+
    /* Restore the BIST result */
    movl	%ebp, %eax
    /* We need to set ebp ? No need */
Index: corebootv3-global_vars_in_c/arch/x86/amd/stage0.S
===================================================================
--- corebootv3-global_vars_in_c/arch/x86/amd/stage0.S	(Revision 782)
+++ corebootv3-global_vars_in_c/arch/x86/amd/stage0.S	(Arbeitskopie)
@@ -23,7 +23,9 @@
 #define CacheBase CONFIG_CARBASE
 #define MEM_TOPK 2048

-/* leave some space for global variable to pass to RAM stage */
+/* Leave some space for a pointer to the global variables.
+ * This should most likely be 4.
+ */
 #define GlobalVarSize 32

#ifdef CONFIG_CPU_AMD_K10
Index: corebootv3-global_vars_in_c/arch/x86/stage1.c
===================================================================
--- corebootv3-global_vars_in_c/arch/x86/stage1.c	(Revision 783)
+++ corebootv3-global_vars_in_c/arch/x86/stage1.c	(Arbeitskopie)
@@ -82,9 +82,15 @@

struct global_vars *global_vars(void)
 {
-	return (struct global_vars *)(bottom_of_stack() - sizeof(struct global_vars));
+	return *(struct global_vars **)(bottom_of_stack() - sizeof(struct global_vars *));
 }

+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;
+}
+
 void dump_mem_range(int msg_level, unsigned char *buf, int size)
 {
    int i;
@@ -119,9 +125,14 @@
 /*
  * This function is called from assembler code with its argument on the
  * stack. Force the compiler to generate always correct code for this case.
+ * We have cache as ram running and can start executing code in C.
  */
 void __attribute__((stdcall)) stage1_main(u32 bist)
 {
+	/* FIXME: Which attributes does globvars need? GCC should not try to
+	 * be clever with it. __attribute__((used,externally_visible))?
+	 */
+	struct global_vars globvars;
    int ret;
    struct mem_file archive;
    void *entry;
@@ -150,10 +161,13 @@
    	stop_ap();
    }

-	// We have cache as ram running and can start executing code in C.
+	/* Initialize global variables before we can think of using them.
+	 * NEVER run this on an AP!
+	 */
+	global_vars_init(&globvars);

#ifdef CONFIG_CONSOLE_BUFFER
-	/* Initialize the printk buffer. */
+	/* Initialize the printk buffer. NEVER run this on an AP! */
    printk_buffer_init();
 #endif

[Carl-Daniel Hailfinger]

On 18.08.2008 16:00, Carl-Daniel Hailfinger wrote:
> Move stage1 global variable management from asm to C. The stage0 asm
> code now unconditionally pushes an empty pointer to the stack which is a
> placeholder for the pointer to global variable storage. That pointer and
> the global variable storage are initialized in global_vars_init().

Creating global variables is now extemely easy.
> Build tested on all targets, boot tested on Qemu.
>
> NOTES:
> The code is not yet MP safe, but that's due to v3 not being MP safe in
>
> general (and the comments contradict the code regarding MP features).
> K8 code now works by accident.
>
> Signed-off-by: Carl-Daniel Hailfinger

Acked-by: Segher Boessenkool 
(on IRC)

Committed in r785.

Regards,
Carl-Daniel

[PATCH] v3: Move stage1 global variable management from asm to C

[Segher Boessenkool]

> Segher needs to review which attributes globvars needs to not be
>
> optimized away.

It doesn't need any: if it's used, it won't be optimised away;
and if it isn't used, who cares :-)

There could be an issue if you hid some facts from the compiler,
but you didn't.

The only remaining issue is, does that struct actually get
allocated on the stack (and not in .data or similar)?  There is
no way to force this AFAIK: C doesn't even know what a stack is,
and there is no GCC extension for this.  But don't worry too
much about this, every compiler we use does in fact put this on
the stack.

Segher

[PATCH] v3: Move stage1 global variable management from asm to C

[Carl-Daniel Hailfinger]

On 20.08.2008 19:01, Segher Boessenkool wrote:
> > Segher needs to review which attributes globvars needs to not be
> >
> > optimized away.
>
> It doesn't need any: if it's used, it won't be optimised away;
> and if it isn't used, who cares :-)
>
> There could be an issue if you hid some facts from the compiler,
> but you didn't.

Thanks for checking this.
> The only remaining issue is, does that struct actually get
> allocated on the stack (and not in .data or similar)?  There is
> no way to force this AFAIK: C doesn't even know what a stack is,
> and there is no GCC extension for this.  But don't worry too
> much about this, every compiler we use does in fact put this on
> the stack.

I'm not too worried about that either. My v3 code correctness checker
complains if stage1 or initram use .bss or .data.

Regards,
Carl-Daniel