On Fri, Feb 22, 2008 at 3:21 PM, Carl-Daniel Hailfinger
<[EMAIL PROTECTED]> wrote:
>
> On 22.02.2008 22:08, Myles Watson wrote:
> >
> >> -----Original Message-----
> >> From: ron minnich [mailto:[EMAIL PROTECTED]
> >> Sent: Friday, February 22, 2008 2:00 PM
> >> To: Myles Watson
> >> Cc: Carl-Daniel Hailfinger; Coreboot
> >> Subject: Re: [coreboot] v3 config patch and lar patch
> >>
> >> On Fri, Feb 22, 2008 at 11:20 AM, Myles Watson <[EMAIL PROTECTED]> wrote:
> >>
> >>> > The patch seems to be OK. Please wait for another ack before commit,
> >>> > though.
> >>>
> >>> Stefan?
> >>>
> >>> This patch brings us back to where you can choose not to preparse the
> >>>
> >> ELF.
> >>
> >>
> >> hmm, I just realized if we have this, we're going to have to repair
> >> the coreboot elf parser ... it doesn't do bss correctly.
> >>
> >
> > Maybe I should make it so you have to be in expert mode to not parse the
> > ELF.
> >
>
> Add a dependency on BROKEN and stick a FIXME in the coreboot ELF parser.
> Whoever wants to use the code is free to fix it.
How about the middle ground for now and commit this patch, which
defaults to parsing the ELF, since that works. Someone who knows
where the ELF parser is broken should probably insert the FIXME or fix
it.
The only difference between this version and the last is that
PARSE_PAYLOAD_ELF defaults to y, and doesn't depend on EXPERT.
Myles
Signed-off-by: Myles Watson <[EMAIL PROTECTED]>
> Regards,
> Carl-Daniel
>
> --
> http://www.hailfinger.org/
>
>
Index: Kconfig
===================================================================
--- Kconfig (revision 616)
+++ Kconfig (working copy)
@@ -96,29 +96,6 @@
prompt "Payload type"
default PAYLOAD_NONE
-config PAYLOAD_PREPARSE_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
@@ -143,10 +120,33 @@
config PAYLOAD_FILE
string "Payload path and filename"
- depends PAYLOAD_ELF || PAYLOAD_PREPARSE_ELF
+ depends PAYLOAD_ELF
default "payload.elf"
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
+ 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 not backward compatible -- if you put an ELF
+ payload in, coreboot can not parse it. We hope to remove ELF
+ entirely in the future.
+
endmenu
Index: arch/x86/Makefile
===================================================================
--- arch/x86/Makefile (revision 616)
+++ arch/x86/Makefile (working copy)
@@ -36,10 +36,8 @@
ROM_SIZE := $(shell expr $(CONFIG_COREBOOT_ROMSIZE_KB) \* 1024)
-LARFILES := nocompress:normal/initram normal/stage2 nocompress:normal/option_table
-ifneq ($(CONFIG_PAYLOAD_NONE),y)
-LARFILES += normal/payload
-endif
+LARFILES_NOCOMPRESS := normal/initram normal/option_table
+LARFILES_COMPRESSIBLE := normal/stage2
DECOMPRESSORS :=
ifeq ($(CONFIG_COMPRESSION_LZMA),y)
@@ -64,24 +62,31 @@
$(Q)cp $(obj)/coreboot.initram $(obj)/lar.tmp/normal/initram
$(Q)cp $(obj)/coreboot.stage2 $(obj)/lar.tmp/normal/stage2
$(Q)cp $(obj)/option_table $(obj)/lar.tmp/normal/option_table
+ $(Q)printf " LAR $(subst $(shell pwd)/,,$(@))\n"
+ $(Q)rm -f $(obj)/coreboot.rom
+ $(Q)cd $(obj)/lar.tmp && \
+ ../util/lar/lar -e -c \
+ ../coreboot.rom \
+ $(LARFILES_NOCOMPRESS) \
+ -s $(ROM_SIZE) -b $(obj)/coreboot.bootblock
+ $(Q)cd $(obj)/lar.tmp && \
+ ../util/lar/lar -e $(COMPRESSFLAG) -a \
+ ../coreboot.rom \
+ $(LARFILES_COMPRESSIBLE)
ifeq ($(CONFIG_PAYLOAD_NONE),y)
$(Q)printf " PAYLOAD none (as specified by user)\n"
else
- $(Q)# TODO: Print sth. other than $(CONFIG_PAYLOAD_FILE) if compressed.
$(Q)if [ -r $(CONFIG_PAYLOAD_FILE) ]; then \
- printf " PAYLOAD $(CONFIG_PAYLOAD_FILE)\n"; \
+ printf " PAYLOAD $(CONFIG_PAYLOAD_FILE) $(COMPRESSFLAG)\n"; \
cp $(CONFIG_PAYLOAD_FILE) $(obj)/lar.tmp/normal/payload; \
else \
printf "Error: payload file '$(CONFIG_PAYLOAD_FILE)' not found.\n"; \
exit 1; \
fi
+ $(Q)cd $(obj)/lar.tmp && \
+ ../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -a \
+ ../coreboot.rom normal/payload;
endif
- $(Q)printf " LAR $(subst $(shell pwd)/,,$(@))\n"
- $(Q)rm -f $(obj)/coreboot.rom
- $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -c \
- ../coreboot.rom \
- $(LARFILES) \
- -s $(ROM_SIZE) -b $(obj)/coreboot.bootblock
$(Q)# QEMU wants bios.bin:
$(Q)# Run "qemu -L build/ -serial stdio -hda /dev/zero".
$(Q)printf " CP $(subst $(shell pwd)/,,$(obj)/bios.bin)\n"
@@ -123,12 +128,10 @@
endif
endif
-
-# We now parse initram as ELF, so we need PARSEELF enabled unconditionally.
ifeq ($(CONFIG_PAYLOAD_PREPARSE_ELF), y)
PARSEELF = -e
else
- PARSEELF = -e
+ PARSEELF =
endif
STAGE0_OBJ := $(patsubst %,$(obj)/lib/%,$(STAGE0_LIB_OBJ)) \
Index: util/lar/lar.c
===================================================================
--- util/lar/lar.c (revision 616)
+++ util/lar/lar.c (working copy)
@@ -47,15 +47,13 @@
printf("\nLAR - the Lightweight Archiver " VERSION "\n" COPYRIGHT "\n\n"
"Usage: %s [-ecxal] archive.lar [[[file1] file2] ...]\n\n", name);
printf("Examples:\n");
- printf(" lar -c -s 32k -b bootblock myrom.lar foo nocompress:bar\n");
+ printf(" lar -c -s 32k -b bootblock myrom.lar foo bar\n");
printf(" lar -a myrom.lar foo blob:baz\n");
printf(" lar -l myrom.lar\n\n");
printf("File names:\n");
printf(" Names specified in the create or add modes are formatted as\n");
- printf(" follows: [flags]:[filename]:[pathname].\n");
- printf(" * Flags are modifiers for the file. Valid flags:\n");
- printf(" nocompress\tDon't compress the file in the LAR\n");
+ printf(" follows: [filename]:[pathname].\n");
printf(" * Filename is the name of the file on disk. If no pathname\n");
printf(" is specified, then the filename will also be the path name\n");
printf(" used in the LAR.\n");
Index: util/lar/lib.c
===================================================================
--- util/lar/lib.c (revision 616)
+++ util/lar/lib.c (working copy)
@@ -230,24 +230,11 @@
{
struct stat filestat;
int ret = -1;
- char *realname;
char *c;
- if (strstr(name, "nocompress:") == name) {
- realname = strdup(name + 11);
- } else {
- realname = strdup(name);
- }
-
- if (realname == NULL) {
- fprintf(stderr, "Out of memory.\n");
- exit(1);
- }
-
/* printf("... add_files %s\n", name); */
- if (stat(realname, &filestat) == -1) {
+ if (stat(name, &filestat) == -1) {
fprintf(stderr, "Error getting file attributes of %s\n", name);
- free(realname);
return -1;
}
@@ -268,7 +255,7 @@
}
// Is it a directory?
if (S_ISDIR(filestat.st_mode)) {
- ret = handle_directory(realname);
+ ret = handle_directory(name);
}
// Is it a regular file?
if (S_ISREG(filestat.st_mode)) {
@@ -292,7 +279,6 @@
ret = 0;
}
- free(realname);
return ret;
}
Index: util/lar/stream.c
===================================================================
--- util/lar/stream.c (revision 616)
+++ util/lar/stream.c (working copy)
@@ -728,11 +728,6 @@
{
char *filename = name, *pathname = name;
- if (!strncmp(name, "nocompress:",11)) {
- filename += 11;
- *thisalgo = ALGO_NONE;
- }
-
/* this is dangerous */
if (filename[0] == '.' && filename[1] == '/')
filename += 2;
--
coreboot mailing list
[email protected]
http://www.coreboot.org/mailman/listinfo/coreboot
