Re: [coreboot] patch for global variables

[Carl-Daniel Hailfinger]

On 28.08.2008 00:48, ron minnich wrote:
> I responded as best I could.

The checkin you did in r828 slightly differs from the patch I acked. The
checkin has a very subtle stack corruption bug on Geode and generic
i586, maybe also on K8 (I need to recheck that).

Problems listed below in an excerpt of the unreviewed part of r828.
> Modified: coreboot-v3/arch/x86/stage1.c
>
> --- coreboot-v3/arch/x86/stage1.c	2008-08-27 22:29:38 UTC (rev 827)
> +++ coreboot-v3/arch/x86/stage1.c	2008-08-27 22:43:18 UTC (rev 828)
> @@ -138,12 +139,16 @@
>  }
>  #endif /* CONFIG_PAYLOAD_ELF_LOADER */
>
> -/*
> +/**
> 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.
> @param bist Built In Self Test value
> @param init_detected This (optionally set) value is used on some platforms (e.g. k8) to indicate
> 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)
> +void __attribute__((stdcall)) stage1_main(u32 bist, u32 init_detected)

You changed the calling convention of stage1_main (one additional
parameter) which means that accesses to init_detected on Geode and
generic i586 trigger a stack underflow because their stage0 asm has not
been adjusted. Very nasty.
> {
>    struct global_vars globvars;
>    int ret;
> @@ -178,6 +183,8 @@
>     * NEVER run this on an AP!
>     */
>    global_vars_init(&globvars);
> globvars.bist = bist;
> globvars.init_detected = init_detected;

And that one will break once the code is used in MP environments. Global
variables may only be changed via accessor functions. global_vars_init()
is the place to do that.

Thank you for pointing this problem out in practical use. Improved
documentation has been committed in r831.
> hardware_stage1();
> On Wed, Aug 27, 2008 at 2:46 PM, Carl-Daniel Hailfinger
>  wrote:
> > > Index: mainboard/amd/serengeti/mainboard.h
> > >
> > > --- mainboard/amd/serengeti/mainboard.h       (revision 826)
> > > +++ mainboard/amd/serengeti/mainboard.h       (working copy)
> > > @@ -31,3 +31,5 @@
> > >  #define HT_CHAIN_END_UNITID_BASE 0x6
> > >  #define SB_HT_CHAIN_ON_BUS0 2
> > >  #define SB_HT_CHAIN_UNITID_OFFSET_ONLY 1
> > > +#define SERIAL_DEV W83627HF_SP1
> > > +#define SERIAL_IOBASE 0x3f8
>
> I moved these to the stage1.c as they are clearly stage1 things, not
> mainboard.h things.
>
> Good catch.

The duplication remains a problem, but it's not your fault. We need a
dtc output mode which emits only #defines.
> > I trust you to have good reasons for rearranging the include files.
> > However, if our include files are indeed order sensitive, we ought to
> > fix them.
>
> I undid this change, sorry for it.

No problem.
> > > void hardware_stage1(void)
> > >  {
> > > void w83627hf_enable_serial(u8 dev, u8 serial, u16 iobase);
> >
> > That declaration should be in superio/winbond/w83627hf/w83627hf.h,
> > otherwise there's no reason to include it. Please remove the line above.
>
> It's a stage1 thing. Not removed it, corrections welcome.

I have to go over all that code anyway some time in the future, right
after we are able to create stage1 #defines from the DTS.
> > Same comment about include order.
>
> damn, I just realized I missed this one. We can fix it.
>
> I would actually like to have mainboard.h include all things known to
> be needed for the mainboard, it would reduce all this #include
> silliness.

Hm. I'm not sure whether that will lead to another #include hell.

Regards,
Carl-Daniel