[coreboot] ADLO for buildrom

Uwe Hermann uwe at hermann-uwe.de
Wed Apr 30 23:01:24 CEST 2008


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 <mylesgw at gmail.com>

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?rev=702
  http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/ADLO/README.1st?rev=702
  http://tracker.coreboot.org/trac/coreboot/browser/trunk/LinuxBIOSv1/util/ADLO/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?

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.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list