Re: [coreboot] LAR TODO

[Carl-Daniel Hailfinger]

On 11.03.2008 21:52, Myles Watson wrote:
> > > > Myles? Could you prepare a patch which #ifdefs the code instead of
> > > > removing it?
>
> This patch is a hopefully less controversial version of a previous patch which
> removed the ELF loader from coreboot v3.  This adds a Kconfig option
> PAYLOAD_ELF_LOADER which builds the loader into v3.  In order to make it a
> little safer, I changed PAYLOAD_PREPARSE_ELF to PAYLOAD_NO_PREPARSE_ELF and
> made that option depend on PAYLOAD_ELF_LOADER so that no one adds an unparsed
> ELF without the loader.
>
> One part that was strange to me was that I first tried adding elfboot.o and
> archelfboot.o to the beginning of the list of object files.  This broke
> coreboot.  It still finished building, but would not boot on QEMU.  I was
> surprised that it broke it, but didn't investigate further.

Did you make distclean in between? v3 dependency handling is something
between screwed and nonexistent. It does work sometimes, though.
> Myles
>
> Signed-off-by: Myles Watson
> Index: Kconfig
>
> --- Kconfig	(revision 638)
> +++ Kconfig	(working copy)
> @@ -92,6 +92,12 @@
>
> menu "Payload"
>
> +config PAYLOAD_ELF_LOADER
> bool "Include ELF payload loader"
> default n
> help
>  This option allows an unparsed ELF paylaod to be added and loaded.
>
> choice
>          prompt "Payload type"
>          default PAYLOAD_NONE
> @@ -125,10 +131,10 @@
>    help
>      The path and filename of the ELF executable file to use as payload.
>
> -config PAYLOAD_PREPARSE_ELF
> bool "Pre-parse ELF file and convert ELF segments to LAR entries"
> depends PAYLOAD_ELF
> default y
>
> +config PAYLOAD_NO_PREPARSE_ELF
> bool "Add ELF without parsing and converting to LAR entries"
> depends PAYLOAD_ELF && PAYLOAD_ELF_LOADER
> 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
>
> @@ -142,7 +148,7 @@
>      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
>  Without 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 not backward compatible -- if you put an ELF
>    payload in, coreboot can not parse it. We hope to remove ELF
> Index: arch/x86/stage1.c
>
> --- arch/x86/stage1.c	(revision 638)
> +++ arch/x86/stage1.c	(working copy)
> @@ -28,10 +28,12 @@
>  #include <mc146818rtc.h>
>  #include <cpu.h>
>
> +#ifdef CONFIG_PAYLOAD_ELF_LOADER
>  /* ah, well, what a mess! This is a hard code. FIX ME but how?
> By getting rid of ELF ...
>
> */
>  #define UNCOMPRESS_AREA (0x400000)
> +#endif /* CONFIG_PAYLOAD_ELF_LOADER */
>
> /* these prototypes should go into headers */
>  void uart_init(void);
> @@ -86,6 +88,8 @@
>    return;
>  }
>
> +#ifdef CONFIG_PAYLOAD_ELF_LOADER
>  /* until we get rid of elf */
>  int legacy(struct mem_file *archive, char *name, void *where, struct lb_memory *mem)
>  {
> @@ -101,6 +105,7 @@
>    printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
>    return -1;
>  }
> +#endif /* CONFIG_PAYLOAD_ELF_LOADER */
>
> /*
> This function is called from assembler code with its argument on the
>
> @@ -110,7 +115,9 @@
>  {
>    int ret;
>    struct mem_file archive, result;
> +#ifdef CONFIG_PAYLOAD_ELF_LOADER
>    int elfboot_mem(struct lb_memory *mem, void *where, int size);
> +#endif /* CONFIG_PAYLOAD_ELF_LOADER */
>    void *entry;
>
> /* we can't statically init this hack. */
> @@ -202,9 +209,11 @@
>
> printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> +#ifdef CONFIG_PAYLOAD_ELF_LOADER
>    ret = find_file(&archive, "normal/payload", &result);
>    if (! ret)
>    	legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
> +#endif /* CONFIG_PAYLOAD_ELF_LOADER */
>
> entry = load_file_segments(&archive, "normal/payload");
>    if (entry != (void*)-1) {

OK so far.
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 638)
> +++ arch/x86/Makefile	(working copy)
> @@ -115,9 +115,20 @@
>  # 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 
> +STAGE0_ARCH_X86_OBJ  = stage1.o serial.o
>
> +ifeq ($(CONFIG_PAYLOAD_ELF_LOADER),y)
> +STAGE0_LIB_OBJ      += elfboot.o
> +STAGE0_ARCH_X86_OBJ += archelfboot.o
> +else
> +STAGE0_LIB_OBJ      +=
> +STAGE0_ARCH_X86_OBJ +=
> +endif
>
> +STAGE0_LIB_OBJ      += 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 += speaker.o \
>    	       udelay_io.o mc146818rtc.o post_code.o
>
> ifeq ($(CONFIG_CPU_I586),y)

Sorry, but the chunk above is a real mess. Can't you move the
conditional chunk to the end and avoid the else path completely?
> @@ -132,10 +143,10 @@
>  endif
>  endif
>
> -ifeq ($(CONFIG_PAYLOAD_PREPARSE_ELF), y)
> +ifeq ($(CONFIG_PAYLOAD_NO_PREPARSE_ELF), y)
> PARSEELF =
>
> +else
>    PARSEELF = -e
> -else
> PARSEELF =
>
> endif
>
> STAGE0_OBJ := $(patsubst %,$(obj)/lib/%,$(STAGE0_LIB_OBJ)) \

The patch would get my Ack except for the Makefile chunk I complained
about. Please rework that.

Regards,
Carl-Daniel