[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