Re: [coreboot] ADLO for buildrom

[Myles Watson]

> -----Original Message-----
> From: Uwe Hermann [mailto:[e-mail]]
> Sent: Wednesday, April 30, 2008 3:01 PM
> To: Myles Watson
> Cc: Jordan Crouse; Coreboot
> Subject: Re: [coreboot] ADLO for buildrom
>
> On Wed, Apr 30, 2008 at 11:21:48AM -0600, Myles Watson wrote:
> > Here are two patches.  One adds a lightened ADLO to payloads/adlo.
> > The other adds ADLO to buildrom.
> >
> > The only non-specific change is an error if there is no payload
> > selected, since it causes the build to fail.
> >
> > Signed-off-by: Myles Watson
>
> Great work, thanks! Quick review below:

Sure.  Thanks for the comments.
> > Index: adlo/elf/elf-header-065kb.payload
> > Index: adlo/elf/elf-header-068kb.payload
>
> Not sure I like storing blobs in svn. How were they generated?

By hand with a hex editor.

If
> possible we should store the "source" or some scripts which generates
> them on-the-fly in svn? This won't hold up a commit though, can be
> done later. Hm, seems they're hand-crafted, but I'd still like a
> small script which "crafts" them better than storing blobs nobody
> can read or understand.

I'd love a script that crafts them.  Using readelf -a and trial and error
gets old.
> Also, v2's ADLO has three of those, what's the difference to these?

There used to be a 64K payload for the 16-bit BIOS and a 128K payload for
the 32-bit BIOS that added other features like ACPI.  Now the 32-bit BIOS
fits into 64K, so no need for the 128-bit ELF headers.  The reason there are
two is that I hope to be able to use kexec to load ADLO so that the kernel
can initialize all the hardware on the motherboard correctly.  Kexec can't
handle ELFs that have data that isn't page-aligned.  The 68K payload has
that 4K-aligned loader.
> > Index: adlo/loader.s
> >
> > --- adlo/loader.s	(revision 0)
> > +++ adlo/loader.s	(revision 0)
> > @@ -0,0 +1,467 @@
> > +;*****************************************************
> > +; $Id: loader.s,v 1.1 2002/11/25 02:07:53 rminnich Exp $
> > +;*****************************************************
>
> Hm, who wrote this code originally and what license applies?
>
> Juding from
>
> http://tracker.coreboot.org/trac/coreboot/log/trunk/LinuxBIOSv1/util/ADLO?
> rev=702
>
> http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/A
> DLO/README.1st?rev=702
>
> http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/A
> DLO/CAST?rev=702
> it seems the code was checked in by Ron, but actually implemented by
> Adam Sulmicki with the help of others:
>
> "Most of the analysis, design and implementation of the project was
> done by
>     me, Adam Sulmicki."
>
> Is this correct?

Good question.  I haven't been around this project that long.  Ron?

I'm hoping we'll replace it very soon, though.
> There's a copy of the GPLv2 in the original ADLO directory, so I think we
> can safely assume the code to be GPLv2, right?

I would assume that too.
> > Index: adlo/align.patch
> >
> > --- adlo/align.patch	(revision 0)
> > +++ adlo/align.patch	(revision 0)
> > @@ -0,0 +1,105 @@
> > +--- loader.s	2008-02-13 12:12:04.000000000 -0700
> > ++++ loader_4K.s	2008-04-11 15:57:01.000000000 -0600
> > +@@ -2,7 +2,7 @@
> > ; $Id: loader.s,v 1.1 2002/11/25 02:07:53 rminnich Exp $
> > ;*****************************************************
> > USE32
> >
> > +-; code it is loaded into memory at 0x7C00
> > ++; code it is loaded into memory at 0x7000
>
> Why? Please explain. Is this board-specific or payload-specific?
> Can it be a variable, selectable in a config file later?
> Surely in buildrom, but also for manual builds...

This is part of what should go away.  The BIOS has to get loaded at the
correct address for callbacks to work.  This loader needs to get loaded
somewhere else, then copy its payload to the right spot.  I'd love for the
loading part to get taken over by Coreboot.  Then we could just set up the
CMOS values separately and not depend on being loaded into a hard-coded
location in RAM.
> > Index: packages/adlo/adlo.mk
> >
> > --- packages/adlo/adlo.mk	(revision 0)
> > +++ packages/adlo/adlo.mk	(revision 0)
>
> [...]
> > +ifeq ($(CONFIG_VERBOSE),y)
> > +ADLO_FETCH_LOG=/dev/stdout
> > +ADLO_BUILD_LOG=/dev/stdout
> > +else
> > +ADLO_BUILD_LOG=$(ADLO_LOG_DIR)/build.log
> > +ADLO_FETCH_LOG=$(ADLO_LOG_DIR)/fetch.log
> > +endif
>
> Unrelated, but this stuff is pretty generic in all buildrom "packages",
> might be worth to move to a generic *.inc file later...

I agree.
> > +# Make sure we have the tools we need to accomplish this
> > +HAVE_AS86:=$(call find-tool,as86)
> >
> > +ifeq ($(HAVE_AS86),n)
> > +$(error To build ADLO, you need to install as86, available in dev86)
> > +endif
>
> This is needed for ADLO but not legacybios? Can we also change ADLO
> to not require it, or is that impossible?

I hope someone will do it soon.  It would take me some time, since I'm not
really familiar with assembly, but it really isn't too much code.
> > +ADLO_CONFIG=$(PACKAGE_DIR)/adlo/conf/defconfig
>
> There's no such file in your patch, unless I'm missing something.

Yep.  I'll remove that line.
> > +ADLO_TARBALL=adlo-svn-$(ADLO_TAG).tar.gz
> >
> > +$(SOURCE_DIR)/$(ADLO_TARBALL): | $(ADLO_LOG_DIR)
> > @ mkdir -p $(SOURCE_DIR)/adlo
> > @ $(BIN_DIR)/fetchsvn.sh $(ADLO_URL) $(SOURCE_DIR)/adlo \
> > $(ADLO_TAG) $(SOURCE_DIR)/$(ADLO_TARBALL) \
> > > $(ADLO_FETCH_LOG) 2>&1
> >
> > +$(ADLO_STAMP_DIR)/.unpacked: $(SOURCE_DIR)/$(ADLO_TARBALL) |
>
> $(ADLO_STAMP_DIR) $(ADLO_LOG_DIR) $(ADLO_DIR)
> > @ echo "Unpacking adlo..."
>
> Let's use "ADLO" in strings everywhere, as that seems to be the
> "official" name.

Sure.  Once we replace the loader there will be nothing left of the original
but the name, but the name's fine.
> > @ tar -C $(ADLO_DIR) -zxf $(SOURCE_DIR)/$(ADLO_TARBALL)
> > @ ln -s $(LEGACYBIOS_SRC_DIR)/out/ $(ADLO_DIR)/svn/legacybios
> > @ touch $@
> >
> > +$(ADLO_SRC_DIR)/adlo.elf: $(ADLO_STAMP_DIR)/.unpacked
>
> $(LEGACYBIOS_SRC_DIR)/out/rom.bin
> > @ echo "Building adlo..."
>
> See above.
> > @ make -C $(ADLO_SRC_DIR) > $(ADLO_BUILD_LOG) 2>&1
> >
> > +$(ADLO_STAMP_DIR)/.copied: $(ADLO_SRC_DIR)/adlo.elf
> > @ mkdir -p $(shell dirname $(PAYLOAD_ELF))
> > @ cp $(ADLO_SRC_DIR)/adlo.elf $(PAYLOAD_ELF)
> > @ touch $@
> >
> > +$(ADLO_STAMP_DIR) $(ADLO_LOG_DIR):
> > @ mkdir -p $@
> >
> > +adlo: $(ADLO_STAMP_DIR)/.copied
> >
> > +adlo-clean:
> > @ echo "Cleaning adlo..."
>
> Ditto.
> > @ rm -f $(ADLO_STAMP_DIR)/.copied
> >
> > +ifneq ($(wildcard $(ADLO_SRC_DIR)/Makefile),)
> > @ $(MAKE) -C $(ADLO_SRC_DIR) clean > /dev/null 2>&1
> >
> > +endif
> >
> > +adlo-distclean:
> > @ rm -rf $(ADLO_DIR)/*
> > Index: packages/legacybios/legacybios.mk
> >
> > --- packages/legacybios/legacybios.mk	(revision 0)
> > +++ packages/legacybios/legacybios.mk	(revision 0)
>
> [...]
> > +LEGACYBIOS_TARBALL=legacybios-$(LEGACYBIOS_TAG).tar.gz
> > +LEGACYBIOS_SOURCE=legacybios-$(LEGACYBIOS_TAG).tar.gz
>
> Why are both needed?
> > +ifeq ($(shell if [ -f $(PACKAGE_DIR)/legacybios/conf/customconfig--
>
> $(PAYLOAD)--$(COREBOOT_VENDOR)-$(COREBOOT_BOARD) ]; then echo 1; fi),1)
> > LEGACYBIOS_CONFIG = customconfig--$(PAYLOAD)--$(COREBOOT_VENDOR)-
>
> $(COREBOOT_BOARD)
> > +endif
> >
> > +$(SOURCE_DIR)/$(LEGACYBIOS_TARBALL):
> > @ mkdir -p $(SOURCE_DIR)
> > @ wget $(WGET_Q) -P $(SOURCE_DIR)
>
> $(LEGACYBIOS_URL)/$(LEGACYBIOS_SOURCE) --output-
> document=$(SOURCE_DIR)/$(LEGACYBIOS_TARBALL)
>
> Not too important, but I'd use -O instead of --output-document, it's
> shorter and the rest of the code also uses -O.

I'll change it.
> > +$(LEGACYBIOS_STAMP_DIR)/.unpacked: $(SOURCE_DIR)/$(LEGACYBIOS_TARBALL)
>
> | $(LEGACYBIOS_STAMP_DIR) $(LEGACYBIOS_DIR) $(LEGACYBIOS_LOG_DIR)
> > @ echo "Unpacking legacybios..."
> > @ tar -C $(LEGACYBIOS_DIR) -zxf $(SOURCE_DIR)/$(LEGACYBIOS_TARBALL)
> > @ touch $@
> >
> > +$(LEGACYBIOS_SRC_DIR)/out/rom.bin: $(LEGACYBIOS_STAMP_DIR)/.unpacked
> > @ echo "Building legacybios..."
> > @ echo $(LEGACYBIOS_SRC_DIR)
> > @ make -C $(LEGACYBIOS_SRC_DIR) > $(LEGACYBIOS_BUILD_LOG) 2>&1
>
> Doesn't work for me.
>
> Adding 'AVOIDCOMBINE=1' as per legacybios README helps, but there are
> still build failures on my box (x86, Linux):
>
> src/romlayout.S: Assembler messages:
> src/romlayout.S:427: Error: attempt to .org/.space backwards? (-1)
> src/romlayout.S:490: Fatal error: can't write out/romlayout16.o: Bad value
> make[1]: *** [out/romlayout16.o] Error 1
>
> Any ideas?
> > +$(LEGACYBIOS_STAMP_DIR) $(LEGACYBIOS_LOG_DIR):
> > @ mkdir -p $@
> >
> > +legacybios: $(LEGACYBIOS_SRC_DIR)/out/rom.bin
> >
> > +legacybios-clean:
> > @ echo "Cleaning legacybios..."
> > @ rm -f $(LEGACYBIOS_STAMP_DIR)/.copied
> >
> > +ifneq ($(wildcard $(LEGACYBIOS_SRC_DIR)/Makefile),)
> > @ $(MAKE) -C $(LEGACYBIOS_SRC_DIR) clean > /dev/null 2>&1
> >
> > +endif
> >
> > +legacybios-distclean:
> > @ rm -rf $(LEGACYBIOS_DIR)/*
> > Index: config/payloads/Config.in
> >
> > --- config/payloads/Config.in	(revision 170)
> > +++ config/payloads/Config.in	(working copy)
> > @@ -9,6 +9,9 @@
> >    help
> >      Buildrom can build a number of different payloads for the ROM
> >
> > +config PAYLOAD_ADLO
> > bool "ADLO (Bochs BIOS with a wrapper for Coreboot)"
>
> Maybe something like
>     bool "ADLO (x86 legacy BIOS on top of coreboot)"
>
> Or just "ADLO" and add more info the kconfig help.

ADLO is fine.  There wasn't help for any of the other payloads, so I didn't
add it for this one.

New patch attached.

Thanks,
Myles
> HTH, Uwe.
>
> http://www.hermann-uwe.de  | http://www.holsham-traders.de
> http://www.crazy-hacks.org | http://www.unmaintained-free-software.org