[coreboot] [PATCH] v3: introduce generic global variable storage

[Carl-Daniel Hailfinger]

On 10.08.2008 14:03, Carl-Daniel Hailfinger wrote:
> On 10.08.2008 03:49, ron minnich wrote:
>   
>> On Sat, Aug 9, 2008 at 5:45 PM, Carl-Daniel Hailfinger wrote:
>>   
>>     
>>> +struct global_vars {
>>> +#ifdef CONFIG_CONSOLE_BUFFER
>>> +       struct printk_buffer *printk_buffer;
>>> +#endif
>>> +};
>>>     
>>>       
>> I think you should always leave the struct the same size and let it
>> have struct members that are in some cases unused.
>>   
>>     
>
> If we do that, we also have to keep the definition of struct
> printk_buffer outside #ifdef CONFIG_CONSOLE_BUFFER and that is not
> really clean. But I see your point there.
>   

We can postpone that decision. The important thing is to create a
generic infrastructure, which my patch does.

>> hmm. Why not when we allocate stack have a constant that defines a
>> 'base of stack' area that is some fixed size, which matches struct
>> global_vars in size? just random thoughts.  
>>     
>
> I could move the area allocation to be local to stage1_main. That makes
> some code more complicated, but individual stage0 asm files can be left
> alone when changing the global var struct.
> However I'd like a compiler expert to tell me that gcc will NEVER
> optimize away or reuse that stack allocation (local variable
> "global_buffer") even if that variable is completely unused in the
> respective function.
>
> int stage1_main()
> {
> 	char global_buffer[1234];
> 	some_function();
> 	some_more_stuff;
> 	return 0;
> }
>   

Maybe I misunderstood your proposal. Where do you want to allocate that
area? In stage0 (like my current code does) or in stage1 (my code
proposal above)?

>> This is close but Peter's comment is important.  
>>     
>
> I'll push my explanation from my mail answering him into the code.
>   

Added a code comment in the right place.

New iteration. Regardless of the route we decide to pursue, this patch
is needed anyway because it creates the generic infrastructure and
provides much-needed abstraction. Adapting the new code to different
storage concepts later will be a simple one-liner.

Regards,
Carl-Daniel

Introduce a generic global variable storage mechanism and switch the
printk buffer management to it.

Build tested and boot tested and result tested on Qemu.

Adding a new global variable is not as easy as it looks, but the
comments in the code should be good enough to tell you how.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: corebootv3-global_vars/include/console.h
===================================================================
--- corebootv3-global_vars/include/console.h	(Revision 730)
+++ corebootv3-global_vars/include/console.h	(Arbeitskopie)
@@ -60,6 +60,18 @@
 };
 #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 {
+#ifdef CONFIG_CONSOLE_BUFFER
+	struct printk_buffer *printk_buffer;
+#endif
+};
+
 SHARED_WITH_ATTRIBUTES(printk, int, __attribute__((format (printf, 2, 3))),
 					int msg_level, const char *fmt, ...);
 SHARED(banner, void, int msg_level, const char *msg);
Index: corebootv3-global_vars/include/arch/x86/cpu.h
===================================================================
--- corebootv3-global_vars/include/arch/x86/cpu.h	(Revision 730)
+++ corebootv3-global_vars/include/arch/x86/cpu.h	(Arbeitskopie)
@@ -198,6 +198,7 @@
 }
 
 SHARED(bottom_of_stack, void *, void);
+SHARED(global_vars, struct global_vars *, void);
 
 #ifdef CONFIG_CONSOLE_BUFFER
 #define PRINTK_BUF_SIZE_CAR (CONFIG_CARSIZE / 2)
Index: corebootv3-global_vars/lib/console.c
===================================================================
--- corebootv3-global_vars/lib/console.c	(Revision 730)
+++ corebootv3-global_vars/lib/console.c	(Arbeitskopie)
@@ -35,13 +35,16 @@
 }
 
 #ifdef CONFIG_CONSOLE_BUFFER
+struct printk_buffer *printk_buffer_addr(void)
+{
+	return global_vars()->printk_buffer;
+}
+
 void printk_buffer_move(void *newaddr, int newsize)
 {
-	struct printk_buffer **p;
 	struct printk_buffer *oldbuf, *newbuf;
 	int copylen;
-	p = bottom_of_stack();
-	oldbuf = *p;
+	oldbuf = printk_buffer_addr();
 	newbuf = newaddr;
 	newbuf->len = newsize;
 	newbuf->readoffset = 0;
@@ -68,17 +71,10 @@
 			&oldbuf->buffer[0], copylen);
 		newbuf->writeoffset += copylen;
 	}
-	*p = newbuf;
+	global_vars()->printk_buffer = newbuf;
 	return;
 }
 
-struct printk_buffer *printk_buffer_addr(void)
-{
-	struct printk_buffer **p;
-	p = bottom_of_stack();
-	return *p;
-}
-
 void printk_buffer_init(void)
 {
 	struct printk_buffer *buf = printk_buffer_addr();
Index: corebootv3-global_vars/arch/x86/stage1.c
===================================================================
--- corebootv3-global_vars/arch/x86/stage1.c	(Revision 730)
+++ corebootv3-global_vars/arch/x86/stage1.c	(Arbeitskopie)
@@ -70,12 +70,21 @@
 
 }
 
+/*
+ * The name is slightly misleading because this is the initial stack pointer,
+ * not the address of the first element on the stack.
+ */
 void *bottom_of_stack(void)
 {
-	/* -4-4 because CONFIG_CARBASE + CONFIG_CARSIZE - 4 is initial %esp */
-	return (void *)(CONFIG_CARBASE + CONFIG_CARSIZE - 4 - 4);
+	/* -4 because CONFIG_CARBASE + CONFIG_CARSIZE - 4 is initial %esp */
+	return (void *)(CONFIG_CARBASE + CONFIG_CARSIZE - 4);
 }
 
+struct global_vars *global_vars(void)
+{
+	return (struct global_vars *)(bottom_of_stack() - sizeof(struct global_vars));
+}
+
 void dump_mem_range(int msg_level, unsigned char *buf, int size)
 {
 	int i;


-- 
http://www.hailfinger.org/