Re: [LinuxBIOS] patch: extending LAR, and removing elf from linuxbios (it is not needed)

[Peter Stuge]

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