[LinuxBIOS] patch: extending LAR, and removing elf from linuxbios (it is not needed)
Peter Stuge
peter at stuge.se
Tue Aug 28 01:42:50 CEST 2007
On Mon, Aug 27, 2007 at 10:25:42AM -0700, ron minnich wrote:
> LAR is a very capable format. With two simple extensions, we can
> use LAR to replace all that we are using ELF for now.
Awesome!
Some comments on the patch, most are trivial:
> Index: include/lar.h
> ===================================================================
> --- include/lar.h (revision 464)
> +++ include/lar.h (working copy)
> @@ -54,7 +54,7 @@
>
> #define MAGIC "LARCHIVE"
> #define MAX_PATHLEN 1024
> -
> +/* NOTE -- This and the user-mode lar.h are NOT IN SYNC. Be careful. */
Good point. I think it would be desirable to change both util/lar and
include/lar in the same commit - or make the change backwards
compatible..
> struct lar_header {
> char magic[8];
> u32 len;
> @@ -62,6 +62,13 @@
> u32 checksum;
> u32 compchecksum;
> u32 offset;
> + u32 entry; /* we might need to make this u64 */
> + u32 loadaddress; /* ditto */
> + /* Compression:
> + * 0 = no compression
> + * 1 = lzma
> + * 2 = nrv2b
> + */
> u32 compression;
> };
..maybe by adding the new fields after compression instead, and
possibly by changing the magic.
> +++ mainboard/emulation/qemu-x86/initram.c (working copy)
> @@ -19,10 +19,12 @@
>
> #include <console.h>
>
> +int (*pk)(int msg_level, const char *fmt, ...) = printk;
> +
> int main(void)
> {
> - printk(BIOS_INFO, "RAM init code started.\n");
> - printk(BIOS_INFO, "Nothing to do.\n");
> + pk(BIOS_INFO, "RAM init code started.\n");
> + pk(BIOS_INFO, "Nothing to do.\n");
Huh? What's the idea with pk() ?
> +++ lib/lzmadecode.c (working copy)
> @@ -206,7 +206,6 @@
> RC_GET_BIT(probLit, symbol)
> }
> previousByte = (Byte)symbol;
> -
> outStream[nowPos++] = previousByte;
> if (state < 4) state = 0;
> else if (state < 10) state -= 3;
Maybe avoid unneeded changes? No big deal though.
> +++ lib/lar.c (working copy)
..
> @@ -52,19 +59,69 @@
> // FIXME: check checksum
>
> if (strcmp(fullname, filename) == 0) {
> + printk(BIOS_SPEW, "LAR: it matches %s @ %p\n", fullname, header);
More messages is always nice, but what is "it" ? :)
> +++ lib/stage2.c (working copy)
> @@ -96,8 +96,10 @@
>
> /* TODO: Add comment. */
> post_code(0x70);
> - write_tables();
> +#warning WRITE_TABLES IS FAILING -- FIX ME IT IS DISABLED
> +// write_tables();
Separate patch maybe? Or is it related to this lar change?
> +++ arch/x86/stage1.c (working copy)
..
> -#define UNCOMPRESS_AREA 0x60000
> +/* ah, well, what a mess! This is a hard code. FIX ME but how? By
> having a "bounding box" * for the payload, set in LAR, and just
> uncompressing PAST the payload */
> +#define UNCOMPRESS_AREA (0x400000)
Hm? Please explain this idea a bit more?
> + for(i = 0, entry = (void *)0; ;i++) {
> + char filename[64];
> + void *newentry;
> + sprintf(filename, "normal/payload%d", i);
> + archive.len = *(u32 *)0xfffffff4;
> + archive.start =(void *)(0UL-archive.len);
Is 0UL good also on 64bit? (Even if it's truncated and "saved" when
assigned to archive.start I'd prefer having the size explicitly.)
> +++ util/lar/lar.c (working copy)
> @@ -44,9 +45,14 @@
> static void usage(char *name)
> {
> printf("\nLAR - the LinuxBIOS Archiver " VERSION "\n" COPYRIGHT "\n\n"
> - "Usage: %s [-cxl] archive.lar [[[file1] file2] ...]\n\n", name);
> + "Usage: %s [-ecxl] archive.lar [[[file1] file2] ...]\n\n", name);
-e here but -p in code
> +++ util/lar/create.c (working copy)
..
> + source = fopen(name, "r");
"rb" to work on Windows as well, but maybe there are more serious
issues before lar builds there?
..
> + filebuf = readfile(name, &filelen);
..
> + if (parseelf() && iself(filebuf))
> + entrylen = outputelf(name,filebuf, filelen, thisalgo,archive);
> + else
> + entrylen = outputfile(name,filebuf, filelen, thisalgo,archive, 0, 0);
> free(filebuf);
malloc() in readfile() is never checked so both fread() in readfile()
and free() here can be called with filebuf==NULL.
> +++ util/lar/lar.h (working copy)
> @@ -57,6 +57,7 @@
>
> typedef uint32_t u32;
>
> +/* NOTE -- This and the linuxbios lar.h are NOT IN SYNC. Be careful. */
Sure? Same change in both files right?
> struct lar_header {
> char magic[8];
> u32 len;
> @@ -64,6 +65,8 @@
> u32 checksum;
> u32 compchecksum;
> u32 offset;
> + u32 entry; /* we might need to make this u64 */
> + u32 loadaddress; /* ditto */
Might as well make it u64 from the start then. :)
> +++ util/lar/Makefile (working copy)
> @@ -17,7 +17,8 @@
> ## along with this program; if not, write to the Free Software
> ## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
> ##
> -
> +CFLAGS+= -g
> +LDFLAGS += -g
..
> +HOSTCFLAGS+=-g
> +HOSTCXXFLAGS+=-g
Enable debugging by default everywhere? Was this on purpose?
> - $(Q)$(HOSTCXX) $(HOSTCXXFLAGS) -o $@ $(LAROBJ_ABS) $(COMPRESSOR)
> + $(Q)$(HOSTCXX) $(HOSTCXXFLAGS) -o $@ $(LAROBJ_ABS) $(COMPRESSOR)
Extra space.
//Peter
More information about the coreboot
mailing list