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 <[EMAIL PROTECTED]>
>
> 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/
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
