[coreboot] v3 patch rm elfboot

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Thu Feb 14 19:29:41 CET 2008


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 <myles at pel.cs.byu.edu>
>   

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

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





More information about the coreboot mailing list