Re: [coreboot] ADLO for buildrom

[Uwe Hermann]

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:
> 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? 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.

Also, v2's ADLO has three of those, what's the difference to these?
> 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?re...
  http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/ADL...
  http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/ADL...
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?

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?
> 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...
> 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...
> +# 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?
> +ADLO_CONFIG=$(PACKAGE_DIR)/adlo/conf/defconfig

There's no such file in your patch, unless I'm missing something.
> +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.
> @ 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.
> +$(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.

HTH, Uwe.