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

-- 
http://www.hailfinger.org/