Fwd: [patch] tint payload update

[Marc Jones]

ping

---------- Forwarded message ----------
From: Marc Jones 
Date: Mon, Aug 23, 2010 at 6:37 PM
Subject: Re: [coreboot] Trouble compiling tint payload
To: Andrew Guertin 
Cc: [e-mail]

On Mon, Aug 23, 2010 at 12:08 PM, Marc Jones  wrote:
> On Mon, Aug 23, 2010 at 8:33 AM, Andrew Guertin  wrote:
> > I figured out the problem right after sending this: when I set $CC, it
> > overwrites the makefile variable that tells it to use lpgcc, so lpgcc isn't
> > getting used.
> >
> > I changed all occurrences of "CC" in the tint makefile to "MYCC", and it
> > compiled correctly.
> >
> > Is there a less hackish way I should solve it, and then submit a patch? Or
> > does someone who knows what they're doing better want to take care of it?
>
> Hi Andrew,
>
> I have some patches for tint to add xcompile and some other build
> changes to match what has gone into libpayload. I'll try to post them
> later today.

This patch is based on Uwe's, but patching a patch seems odd to me, so
here is a new patch that builds on the previous one. Start with a
clean tint0.03b and put it in the coreboot payloads directory. You no
longer need to build libpayload. The make handles it for you.

Add default libpayload build, xcompile, and lpgcc setup to tint.

Signed-off-by: Marc Jones

Let me know how it goes.

Marc

Fwd: [patch] tint payload update

[Myles Watson]

Picky observation: there are a couple of extra #if 0 #endif pairs.
Combining them would make the patch simpler.

@@ -337,7 +339,9 @@ static void getname (char *name)
         name[NAMELEN - 1] = '\0';
      }
 }
+#endif

+#if 0
 static void err1 ()
 {
    fprintf (stderr,"Error creating %s\n",scorefile);

I haven't tested it, but it looks fine.

Acked-by: Myles Watson

Thanks,
Myles

[Peter Stuge]

Marc Jones wrote:
> ping

..
> Add default libpayload build, xcompile, and lpgcc setup to tint.
>
> Signed-off-by: Marc Jones

Maybe that halt should be made into a sleep followed by returning to
e.g. SeaBIOS?

All those #if 0 are not so pretty.

Maybe some headers can be kept now with the libpayload changes in
that area.

And maybe the cpp:ed out code can be made #ifndef LIBPAYLOAD or so,
to get the changes into upstream.

I understand that you may not have touched any of this, Marc.

As for the build things, can we not get around stuffing all of that
into each payload?

//Peter

Fwd: [patch] tint payload update

[Marc Jones]

On Wed, Sep 15, 2010 at 12:55 PM, Peter Stuge  wrote:
> As for the build things, can we not get around stuffing all of that
> into each payload?

What do you mean? It seems to me that we want each payload responsible
for building its own version of libpayload.

Marc

Fwd: [patch] tint payload update

[Peter Stuge]

Marc Jones wrote:
> > As for the build things, can we not get around stuffing all of that
> > into each payload?
>
> What do you mean? It seems to me that we want each payload
> responsible for building its own version of libpayload.

I hadn't thought of that! Do we? Interesting.

In a way it makes sense because different payloads need different
parts of libpayload. But thinking of it as a replacement for glibc or
at least crt0 then it isn't so nice to have more than one. This is
how I've always thought of it until now.

But ok, say we want local libpayload per payload, then I think we
should still try to simplify things.

Maybe provide a payload.inc Makefile stub in libpayload which the
payload Makefile simply includes after setting a variable name or
two?

That's a different patch(set) though. :)

Acked-by: Peter Stuge

//Peter

Fwd: [patch] tint payload update

[Marc Jones]

On Wed, Sep 15, 2010 at 3:09 PM, Peter Stuge  wrote:
> Marc Jones wrote:
> > > As for the build things, can we not get around stuffing all of that
> > > into each payload?
> >
> > What do you mean? It seems to me that we want each payload
> > responsible for building its own version of libpayload.
>
> I hadn't thought of that! Do we? Interesting.
>
> In a way it makes sense because different payloads need different
> parts of libpayload. But thinking of it as a replacement for glibc or
> at least crt0 then it isn't so nice to have more than one. This is
> how I've always thought of it until now.
>
> But ok, say we want local libpayload per payload, then I think we
> should still try to simplify things.
>
> Maybe provide a payload.inc Makefile stub in libpayload which the
> payload Makefile simply includes after setting a variable name or
> two?
>
> That's a different patch(set) though. :)
>
> Acked-by: Peter Stuge
>
> //Peter

Thanks Myles and Peter.

r5816

Updating the wiki now.

Marc

Fwd: [patch] tint payload update

[Uwe Hermann]

On Thu, Sep 16, 2010 at 03:37:24PM -0600, Marc Jones wrote:
> On Wed, Sep 15, 2010 at 3:09 PM, Peter Stuge  wrote:
> > Marc Jones wrote:
> > > > As for the build things, can we not get around stuffing all of that
> > > > into each payload?
> > >
> > > What do you mean? It seems to me that we want each payload
> > > responsible for building its own version of libpayload.
> >
> > I hadn't thought of that! Do we? Interesting.
> >
> > In a way it makes sense because different payloads need different
> > parts of libpayload. But thinking of it as a replacement for glibc or
> > at least crt0 then it isn't so nice to have more than one. This is
> > how I've always thought of it until now.

There are pros and cons to both variants, but I think we cannot make a
fully generic libpayload build that gets installed somewhere and then
used by multiple payloads directly.

We could drop most "Add support for xyz" kconfig options from libpayload
that simply add new functions to the API and just always compile them in
directly, and let the linker do its magic to only link in those
functions the respective payloads actually uses.
This is possible (but not yet in the Makefiles I think).

However, there are various options (and more will follow) that modify
the _behavior_ of libpayload functions thus making it impossible to
satisfy all possible payloads from one libpayload build.

Example: There's a kconfig option which decides whether libpayload
printf()/putchar() etc. should print to both serial and VGA or only one
of them. The putchar() function is always compiled in, but it's
behaviour is different.
Another example: Some boards (VIA I think) need different CMOS/NVRAM
config port locations (0x72/0x73 vs. 0x74/0x75). And there are others.

So I think one libpayload build/checkout per payload will be indeed
required usually.
> > But ok, say we want local libpayload per payload, then I think we
> > should still try to simplify things.

Yes, definately.
> Updating the wiki now.

Marc, can you please add the full build instructions to the wiki page,
including how and where libpayload should be checked out
(or "svn export"ed) relative to tint, whether or not libpayload should
use the "make install" step before building tint etc. etc.

The current code in svn plus wiki instructions didn't work for me,
various issues it seems. First the "libpayloadbin" is never found (if
you do "make install" in the libpayload dir it installs into an
"install" subdir of the libpayload dir currently. Also, in some
situations lpgcc yields errors as $CC seems to be not set or similar
(probably due to incorrect placement of the libpayload dir?)

Maybe it's simpler to just assume a full coreboot checkout for building
payloads per default? I.e. in payloads/external/tint, we assume a
../../libpayload to be there and contain a built libpayload per default?

Of course, there should be an option to override this and build a
payload by itself with only a libpayload checkout (without the full
coreboot checkout).

Thanks, Uwe.

Fwd: [patch] tint payload update

[Marc Jones]

On Sun, Sep 19, 2010 at 4:47 PM, Uwe Hermann  wrote:
> Marc, can you please add the full build instructions to the wiki page,
> including how and where libpayload should be checked out
> (or "svn export"ed) relative to tint, whether or not libpayload should
> use the "make install" step before building tint etc. etc.
>
> The current code in svn plus wiki instructions didn't work for me,
> various issues it seems. First the "libpayloadbin" is never found (if
> you do "make install" in the libpayload dir it installs into an
> "install" subdir of the libpayload dir currently. Also, in some
> situations lpgcc yields errors as $CC seems to be not set or similar
> (probably due to incorrect placement of the libpayload dir?)
>
> Maybe it's simpler to just assume a full coreboot checkout for building
> payloads per default? I.e. in payloads/external/tint, we assume a
> ../../libpayload to be there and contain a built libpayload per default?
>
> Of course, there should be an option to override this and build a
> payload by itself with only a libpayload checkout (without the full
> coreboot checkout).

I had updated the instructions and then you changed them.  It is
automatic if you have tint in the payloads directory. The tint build
calls the libpayload make and installs in the tint directory.

Marc

Fwd: [patch] tint payload update

[Uwe Hermann]

On Mon, Sep 20, 2010 at 02:19:22PM -0600, Marc Jones wrote:
> I had updated the instructions and then you changed them.  It is
> automatic if you have tint in the payloads directory. The tint build
> calls the libpayload make and installs in the tint directory.

Ah, I probably missed that you should do

cd coreboot/payloads

instead of

cd coreboot/payloads/external/tint

as I would have expected.

But still, the build fails for me on a fresh checkout with your
instructions from the wiki (before I changed stuff there):

svn co svn://coreboot.org/coreboot/trunk coreboot
cd coreboot/payloads
wget http://ftp.debian.org/debian/pool/main/t/tint/tint_0.03b.tar.gz
tar xfvz tint_0.03b.tar.gz
cd tint-0.03b
svn export svn://coreboot.org/coreboot/trunk/payloads/external/tint/libpayload_tint.patch
patch -p1 < libpayload_tint.patch
make

Building libpayload @ ../libpayload.
make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
*** Default configuration is based on 'configs/defconfig'
*
* libpayload Configuration
*
*
* Generic Options
*
Experimental Options (EXPERIMENTAL) [N/y/?] n
[...]
make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
  CC      build/arch/i386/head.S.o
  CC      build/arch/i386/main.o
[...]
  CC      build/curses/colors.o
  AR      build/lib/libpayload.a
  CP      build/lib/i386/head.o
  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/lib
  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/include
  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/bin
make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
  LPCC      tint.o
basename: missing operand
Try `basename --help' for more information.
  LPCC      engine.o
basename: missing operand
Try `basename --help' for more information.
  LPCC      io.o
basename: missing operand
Try `basename --help' for more information.
io.c: In function ‘io_init’:
io.c:102: warning: array subscript is above array bounds
io.c:103: warning: array subscript is above array bounds
  LPCC      utils.o
basename: missing operand
Try `basename --help' for more information.
  LPCC      tint.elf
basename: missing operand
Try `basename --help' for more information.
make: only-keep-debug: Command not found
make: [tint.elf] Error 127 (ignored)
make: strip-debug: Command not found
make: [tint.elf] Error 127 (ignored)
/bin/sh: add-gnu-debuglink=tint.debug: not found
make: [tint.elf] Error 127 (ignored)

This is the problem where $CC is not being set for some reason and thus
"basename $CC" becomes just "basename", hence the error.

Uwe.

Fwd: [patch] tint payload update

[Marc Jones]

On Mon, Sep 20, 2010 at 5:01 PM, Uwe Hermann  wrote:
> On Mon, Sep 20, 2010 at 02:19:22PM -0600, Marc Jones wrote:
> > I had updated the instructions and then you changed them.  It is
> > automatic if you have tint in the payloads directory. The tint build
> > calls the libpayload make and installs in the tint directory.
>
> Ah, I probably missed that you should do
>
> cd coreboot/payloads
>
> instead of
>
> cd coreboot/payloads/external/tint
>
> as I would have expected.
>
> But still, the build fails for me on a fresh checkout with your
> instructions from the wiki (before I changed stuff there):
>
> svn co svn://coreboot.org/coreboot/trunk coreboot
> cd coreboot/payloads
> wget http://ftp.debian.org/debian/pool/main/t/tint/tint_0.03b.tar.gz
> tar xfvz tint_0.03b.tar.gz
> cd tint-0.03b
> svn export svn://coreboot.org/coreboot/trunk/payloads/external/tint/libpayload_tint.patch
> patch -p1 < libpayload_tint.patch
> make
>
> Building libpayload @ ../libpayload.
> make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
> make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
> make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
> *** Default configuration is based on 'configs/defconfig'
> libpayload Configuration
> Generic Options
>
> Experimental Options (EXPERIMENTAL) [N/y/?] n
> [...]
> make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
> make[1]: Entering directory `/home/uwe/coreboot/payloads/libpayload'
>  CC      build/arch/i386/head.S.o
>  CC      build/arch/i386/main.o
> [...]
>  CC      build/curses/colors.o
>  AR      build/lib/libpayload.a
>  CP      build/lib/i386/head.o
>  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/lib
>  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/include
>  INSTALL /home/uwe/coreboot/payloads/tint-0.03b/./libpayloadbin/libpayload/bin
> make[1]: Leaving directory `/home/uwe/coreboot/payloads/libpayload'
>  LPCC      tint.o
> basename: missing operand
> Try `basename --help' for more information.
>  LPCC      engine.o
> basename: missing operand
> Try `basename --help' for more information.
>  LPCC      io.o
> basename: missing operand
> Try `basename --help' for more information.
> io.c: In function ‘io_init’:
> io.c:102: warning: array subscript is above array bounds
> io.c:103: warning: array subscript is above array bounds
>  LPCC      utils.o
> basename: missing operand
> Try `basename --help' for more information.
>  LPCC      tint.elf
> basename: missing operand
> Try `basename --help' for more information.
> make: only-keep-debug: Command not found
> make: [tint.elf] Error 127 (ignored)
> make: strip-debug: Command not found
> make: [tint.elf] Error 127 (ignored)
> /bin/sh: add-gnu-debuglink=tint.debug: not found
> make: [tint.elf] Error 127 (ignored)

$CC should get set xcompile. What does your .xcompile have? I always
use xgcc so there could be a problem there.

Marc

Fwd: [patch] tint payload update

[Uwe Hermann]

On Mon, Sep 20, 2010 at 05:23:14PM -0600, Marc Jones wrote:
> $CC should get set xcompile. What does your .xcompile have? I always
> use xgcc so there could be a problem there.

I see one .xcompile in the coreboot/payloads/tint-0.03b directory after
a build attempt, which is an empty file. I use the stock gcc from Debian.

$ gcc --version
gcc (Debian 4.4.4-12) 4.4.5 20100902 (prerelease)

Something is going wrong with the invokation of xcompile.sh then? I
assume .xcompile should have some contents.

Uwe.

Fwd: [patch] tint payload update

[Marc Jones]

On Mon, Sep 20, 2010 at 6:24 PM, Uwe Hermann  wrote:
> On Mon, Sep 20, 2010 at 05:23:14PM -0600, Marc Jones wrote:
> > $CC should get set xcompile. What does your .xcompile have? I always
> > use xgcc so there could be a problem there.
>
> I see one .xcompile in the coreboot/payloads/tint-0.03b directory after
> a build attempt, which is an empty file. I use the stock gcc from Debian.
>
> $ gcc --version
> gcc (Debian 4.4.4-12) 4.4.5 20100902 (prerelease)
>
> Something is going wrong with the invokation of xcompile.sh then? I
> assume .xcompile should have some contents.

Yes, You should have something in .xcompile xgcc path or hostcc.

Marc