v3 patch rm elfboot

[Myles Watson]

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

[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

[ron minnich]

per stefan's comment, let's see how much space we save. This is
bootblock space, which could be significant, but can we see a number?

thanks

ron

[Stefan Reinauer]

ron minnich wrote:
> per stefan's comment, let's see how much space we save. This is
> bootblock space, which could be significant, but can we see a number?

Elfboot still uses bootblock space if it is disabled in Kconfig? That's 
definitely something we need to fix. That space is tight.

[ron minnich]

Stefan, one thing I don't understand, why does removing elfboot make
it so we can't unpack LAR?

ron

[Stefan Reinauer]

ron minnich wrote:
> Stefan, one thing I don't understand, why does removing elfboot make
> it so we can't unpack LAR?

Because the LAR header information is lost, things like the entry point.

You unpack it, repack it, and the segments become ordinary files with 
their lar headers containing bogus information.

So either we recreate an ELF file on unpack, or we dump the metadata to 
a MANIFEST file.

Or we forget about lars having the feature to be unpacked. Not sure that 
its ever needed, except when you want to migrate a payload from one 
image to another one.

Stefan.

[ron minnich]

On Thu, Feb 14, 2008 at 11:00 AM, Stefan Reinauer  wrote:
> You unpack it, repack it, and the segments become ordinary files with
>  their lar headers containing bogus information.

excellent point.
> So either we recreate an ELF file on unpack, or we dump the metadata to
>  a MANIFEST file.

Why not have a way to unpack and create LAR files? i.e. each file has
the LAR header in it.

ron

[Carl-Daniel Hailfinger]

On 14.02.2008 20:11, ron minnich wrote:
> On Thu, Feb 14, 2008 at 11:00 AM, Stefan Reinauer  wrote:
> > You unpack it, repack it, and the segments become ordinary files with
> >  their lar headers containing bogus information.
>
> excellent point.

Yes, unfortunately.
> > So either we recreate an ELF file on unpack, or we dump the metadata to
> >  a MANIFEST file.
>
> Why not have a way to unpack and create LAR files? i.e. each file has
> the LAR header in it.

Hm. It's not that hard to create a MANIFEST file. And the patch I sent
twice (which was NACKed twice) to specify LAR header contents on the LAR
command line would work perfectly together with the MANIFEST. No
information lost. I can resend the patch...

Regards,
Carl-Daniel

[ron minnich]

On Thu, Feb 14, 2008 at 2:28 PM, Carl-Daniel Hailfinger
> Hm. It's not that hard to create a MANIFEST file. And the patch I sent
>  twice (which was NACKed twice) to specify LAR header contents on the LAR
>  command line would work perfectly together with the MANIFEST. No
>  information lost. I can resend the patch...

I still think it's far simpler to have an option to extract LAR files
as LAR files.

ron

[Peter Stuge]

On Thu, Feb 14, 2008 at 02:30:48PM -0800, ron minnich wrote:
> > It's not that hard to create a MANIFEST file.
>
> I still think it's far simpler to have an option to extract LAR
> files as LAR files.

I would prefer ELF because it's more standard - if it can do all we
need.

//Peter

[ron minnich]

On Thu, Feb 14, 2008 at 4:23 PM, Peter Stuge  wrote:
> I would prefer ELF because it's more standard - if it can do all we
>  need.

All right, so we'll have LAR extract to elf. I don't think it's that hard.

ron

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> 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.

... as long as they do not remove functionality that is still used.

Please don't just remove this code. If you don't like to compile it in, 
create a config option to disable it. (There is such a config option 
already, so I really don't see the gain)

I say: NACK.

Why?

Because with this patch it is no longer possible to unpack a lar.

Please don't do stuff like that with levity.

Stefan

[Myles Watson]

> > Signed-off-by: Myles Watson <myles at pel.cs.byu.edu>
>
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Rev 118
Thanks,
Myles
> Regards,
> Carl-Daniel