Re: [coreboot] v3 patch rm elfboot

[Carl-Daniel Hailfinger]

On 14.02.2008 19:15, Myles Watson wrote:
> This patch removes elfboot and references to ELF from v3.  Since lar
> parses ELF, there's no need to have v3 parse ELF.  The ELF files that
> v3 could parse should always have been a subset of the ones that lar
> could parse anyway.
>
> Myles
>
> Signed-off-by: Myles Watson

Patches which remove stuff are always very welcome. A few comments below.

If you commit, please make sure you remove the files with "svn rm"
instead of using patch to make them empty.
> Index: Kconfig
>
> --- Kconfig	(revision 597)
> +++ Kconfig	(working copy)
> @@ -96,29 +96,6 @@
>          prompt "Payload type"
>          default PAYLOAD_NONE
>
> -config PAYLOAD_PREPARSE_ELF

Isn't the removal of PAYLOAD_PREPARSE_ELF backwards? I would have
expected you to remove PAYLOAD_ELF.
> bool "Pre-parse ELF file and convert ELF segments to LAR entries"
> depends EXPERT
> default n
> help
>  Until now, coreboot has used ELF for the payload. There are many
>  problems with this, not least being the inefficiency -- the ELF has
>  to be decompressed to memory and then the segments have to be
>  copied. Plus, lar can't see the segments in the ELF -- to see all
>  segments, you have to extract the ELF and run readelf on it.
>  There are problems with collisions of the decompressed ELF
>  location in memory and the segment locations in memory.
>  Finally, validation of the ELF is done at run time, once you have
>  flashed the FLASH and rebooted the machine. Boot time is really
>  not the time you want to find out your ELF payload is broken.
>  With this option, coreboot will direct lar to break each ELF
>  segment into a LAR entry. ELF will not be used at all. Note that
>  (for now) coreboot is backward compatible -- if you put an ELF
>  payload in, coreboot can still parse it. We hope to remove ELF
>  entirely in the future.
>
> config PAYLOAD_ELF
>    bool "An ELF executable payload file"
>    help
> @@ -126,6 +103,9 @@
>      which coreboot should run as soon as the basic hardware
>      initialization is completed.
>  With this option, coreboot will direct lar to break each ELF
>  segment into a LAR entry. ELF will not be used at all in coreboot.
> You will be able to specify the location and file name of the
>    payload image later.
>
> @@ -143,7 +123,7 @@
>
> config PAYLOAD_FILE
>    string "Payload path and filename"
> depends PAYLOAD_ELF || PAYLOAD_PREPARSE_ELF
> depends PAYLOAD_ELF

See above.
> default "payload.elf"
>    help
>      The path and filename of the ELF executable file to use as payload.
> Index: include/post_code.h
> ===================================================================
> --- include/post_code.h	(revision 597)
> +++ include/post_code.h	(working copy)
> @@ -58,8 +58,5 @@
>  #define POST_STAGE2_PCISCANBUS_ENTER		0x24
>  #define POST_STAGE2_PCISCANBUS_DONEFORLOOP	0x25
>  #define POST_STAGE2_PCISCANBUS_EXIT		0x55
> -#define POST_ELFBOOT_JUMPING_TO_BOOTCODE	0xfe
> -#define POST_ELFBOOT_LOADER_STARTED		0xf8
> -#define POST_ELFBOOT_LOADER_IMAGE_FAILED	0xff
>
> #endif /* POST_CODE_H */
> Index: mainboard/emulation/qemu-x86/defconfig
> ===================================================================
> --- mainboard/emulation/qemu-x86/defconfig	(revision 597)
> +++ mainboard/emulation/qemu-x86/defconfig	(working copy)
> @@ -83,6 +83,5 @@
>  #
>  # Payload
>  #
> -# CONFIG_PAYLOAD_PREPARSE_ELF is not set
>  # CONFIG_PAYLOAD_ELF is not set
>  CONFIG_PAYLOAD_NONE=y
> Index: arch/x86/stage1.c
> ===================================================================
> --- arch/x86/stage1.c	(revision 597)
> +++ arch/x86/stage1.c	(working copy)
> @@ -29,7 +29,6 @@
>  #include <cpu.h>
>
> /* ah, well, what a mess! This is a hard code. FIX ME but how?
> By getting rid of ELF ...
> */
>
> #define UNCOMPRESS_AREA (0x400000)

I think you can remove the whole comment and UNCOMPRESS_AREA as well.
> @@ -86,22 +85,6 @@
>    return;
>  }
>
> -/* until we get rid of elf */
> -int legacy(struct mem_file *archive, char *name, void *where, struct lb_memory *mem)
> -{
> int ret;
> int elfboot_mem(struct lb_memory *mem, void *where, int size);
> ret = copy_file(archive, name, where);
> if (ret) {
> printk(BIOS_ERR, "'%s' found, but could not load it.\n", name);
> }
> ret =  elfboot_mem(mem, where, archive->reallen);
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> return -1;
>
> -}
>
> /*
> This function is called from assembler code with its argument on the
> stack. Force the compiler to generate always correct code for this case.
>
> @@ -110,7 +93,6 @@
>  {
>    int ret;
>    struct mem_file archive, result;
>
> int elfboot_mem(struct lb_memory *mem, void *where, int size);
> void *entry;
>
> /* we can't statically init this hack. */
>
> @@ -203,8 +185,6 @@
>    printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> ret = find_file(&archive, "normal/payload", &result);
>
> if (! ret)
> legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
>
> entry = load_file_segments(&archive, "normal/payload");
> if (entry != (void*)-1) {
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 597)
> +++ arch/x86/Makefile	(working copy)
> @@ -106,9 +106,9 @@
>  # initram module and the various stages and payload files.
>  #
>
> -STAGE0_LIB_OBJ       = uart8250.o mem.o elfboot.o lar.o delay.o vtxprintf.o \
> +STAGE0_LIB_OBJ       = uart8250.o mem.o lar.o delay.o vtxprintf.o \
>    	       vsprintf.o console.o string.o $(DECOMPRESSORS)
> -STAGE0_ARCH_X86_OBJ  = stage1.o serial.o archelfboot.o speaker.o \
> +STAGE0_ARCH_X86_OBJ  = stage1.o serial.o speaker.o \
>    	       udelay_io.o mc146818rtc.o post_code.o
>
> ifeq ($(CONFIG_CPU_I586),y)
> @@ -125,11 +125,7 @@
>
> # We now parse initram as ELF, so we need PARSEELF enabled unconditionally.
> -ifeq ($(CONFIG_PAYLOAD_PREPARSE_ELF), y)
> PARSEELF = -e
>
> -else
> PARSEELF = -e
>
> -endif
> +PARSEELF = -e

Why not remove PARSEELF altogether and hardcode -e for LAR?
> STAGE0_OBJ := $(patsubst %,$(obj)/lib/%,$(STAGE0_LIB_OBJ)) \
>          $(patsubst %,$(obj)/arch/x86/%,$(STAGE0_ARCH_X86_OBJ)) \

Regards,
Carl-Daniel