[PATCH] v3: introduce generic global variable storage

[Carl-Daniel Hailfinger]

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.

Please note the following pitfalls:
- Adding a new global variable is not as easy as it looks.
- Don't try to add one without consulting me.

Signed-off-by: Carl-Daniel Hailfinger

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,12 @@
 };
 #endif

+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;

[Peter Stuge]

On Sun, Aug 10, 2008 at 02:45:24AM +0200, Carl-Daniel Hailfinger wrote:
> Please note the following pitfalls:
> Adding a new global variable is not as easy as it looks.
> Don't try to add one without consulting me.

I'm afraid the last pitfall is a blocker for me. Please document the
real problem either in the commit message or preferably in code near
where I would be tempted to add more variables.

Thank you.

//Peter

[Carl-Daniel Hailfinger]

On 10.08.2008 02:55, Peter Stuge wrote:
> On Sun, Aug 10, 2008 at 02:45:24AM +0200, Carl-Daniel Hailfinger wrote:
> > Please note the following pitfalls:
> > Adding a new global variable is not as easy as it looks.
> > Don't try to add one without consulting me.
>
> I'm afraid the last pitfall is a blocker for me. Please document the
> real problem either in the commit message or preferably in code near
> where I would be tempted to add more variables.

Short version: You need to fix all stage0 asm code if you add a new
variable. And the stage0 asm code modification is nontrivial (size of
the struct, alignment, initialization, order of struct members).
Depending on your compiler, real breakage may happen.
> Thank you.

I hope the short explanation above is good enough for the moment.

Regards,
Carl-Daniel

[ron minnich]

On Sat, Aug 9, 2008 at 5:45 PM, Carl-Daniel Hailfinger
> +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.

Why not have, e.g,  a page sized global area and allocate out of it?
Hmm. we just recreated malloc.

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.

This is close but Peter's comment is important.

ron

[Carl-Daniel Hailfinger]

Hi Segher,

can you answer the question near the end of this mail with your gcc hat
on? Thanks.

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.
> Why not have, e.g,  a page sized global area and allocate out of it?

Please NO! One page (4k) is really beyond what we need and with a
minimum CAR size of 4k, we will have exactly zero stack left for
function calls.
> Hmm. we just recreated malloc.

You're free to use malloc in stage2 and beyond, but please keep that
complexity out of stage1. malloc becomes ugly really fast if it can fail
easily due to size constraints.
> 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;
}
> This is close but Peter's comment is important.

I'll push my explanation from my mail answering him into the code.

Regards,
Carl-Daniel

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > Hmm. we just recreated malloc.
>
> You're free to use malloc in stage2 and beyond, but please keep that
> complexity out of stage1. malloc becomes ugly really fast if it can fail
> easily due to size constraints.

malloc was removed from v3 for good reasons. Do not put it back.

Stefan

[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

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;

[Carl-Daniel Hailfinger]

Ping?

Getting this in is both bugfix and preparation for more goodness.

Regards,
Carl-Daniel

On 10.08.2008 18:00, Carl-Daniel Hailfinger wrote:
> 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
> 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;

[ron minnich]

Plug it in and let's try it out.

Acked-by: Ronald G. Minnich

[Carl-Daniel Hailfinger]

On 14.08.2008 05:45, ron minnich wrote:
> Plug it in and let's try it out.
>
> Acked-by: Ronald G. Minnich

Thanks, r760.

Regards,
Carl-Daniel