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

[ron minnich]

don't get too scared about the subject line.

LAR is a very capable format. With two simple extensions, we can use
LAR to replace all that we are using ELF for now. This change can
really make life better:
1. we can use streaming decompress instead of the current "uncompress
elf to memory and then copy segments" approach. So we can get rid of
THIS hardcode:
#define UNCOMPRESS_AREA (0x400000)
2. A simple lar l can show ALL segments, including payload segments
3. It's really easy to see where things will go in memory, and catch problems
4. We can figure out an ELF input file is bogus BEFORE we flash, not
AFTER we flash and try to boot it
5. did I mention streaming decompress?
6. We no longer have to worry about where we decompress the elf in
memory (this problem was causing trouble when the payload was a linux
kernel -- it was so big)
7. Since we have a load address, we can create this lar entry:
normal/cmdline
and specify that it be loaded at a place where linux will find it as
the cmdline.
8. The decision on whether to XIP can be made in the LAR entry, not in
hardcode. For example, if initram needs to be XIP, set the load
address to 0xffffffff. Done.

The change is simple. Add a load address and entry point to the lar
header. Extend the lar tool to parse the elf file and create multiple
lar segments. It looks like this:
  normal/payload0 (33192 bytes, lzma compressed to 18088 bytes @0x38
load @0x100000, entry 0x105258)
  normal/payload1 (72 bytes, lzma compressed to 47 bytes @0x4718 load
@0x1225a0, entry 0x105258)
  normal/option_table (932 bytes @0x4798 load @0, entry 0)
  normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
load @0, entry 0)
  normal/initram (4208 bytes @0x8828 load @0, entry 0)
  linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)

note that the payload is now payload0, payload1, etc. I've extended
linuxbios to look for these. Note that you can now see all the things
that get loaded ;they're no longer hidden in an ELF header somewhere.
Elf failures are gone!

Note that I've left legacy elf support in, for now, but recommend we
get rid of it as soon as possible.

patch attached. This is a first pass. lar.c needs some refactoring but
I want to get the comments going. You can now have a linux payload and
it will uncompress with no problems.

This has been tested with filo and BOCHS.

ron

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

[Carl-Daniel Hailfinger]

On 27.08.2007 19:25, ron minnich wrote:
> don't get too scared about the subject line.
>
> LAR is a very capable format. With two simple extensions, we can use
> LAR to replace all that we are using ELF for now. This change can
> really make life better:
> we can use streaming decompress instead of the current "uncompress
>
> elf to memory and then copy segments" approach.
> [...]
> 5. did I mention streaming decompress?

Streaming decompress for lzma (in the byte-wise decompress meaning)
needs quite a big buffer, so it is not attractive. However, if you mean
segment-wise decompress (if segments are compressed individually), that
would work fine.

Regards,
Carl-Daniel

[ron minnich]

On 8/27/07, Carl-Daniel Hailfinger  wrote:
> Streaming decompress for lzma (in the byte-wise decompress meaning)
> needs quite a big buffer, so it is not attractive. However, if you mean
> segment-wise decompress (if segments are compressed individually), that
> would work fine.

good point, what I mean is that we can decompress, segment-by-segment,
to the final destination for the segment. no intermediate copies
needed of the kind we have now with ELF.

I hope someone can review this. but it works.

ron

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

[Carl-Daniel Hailfinger]

On 27.08.2007 19:25, 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.
> [...]
> The change is simple. Add a load address and entry point to the lar
> header. Extend the lar tool to parse the elf file and create multiple
> lar segments.
> [...]
> patch attached. This is a first pass. lar.c needs some refactoring but
> I want to get the comments going. You can now have a linux payload and
> it will uncompress with no problems.

So you have an ELF-to-LAR converter. Nice. Can you convert the linux
payload back to ELF? And will that still boot?
At first, my questions above may sound a bit weird. However, if we have
a real two-way converter, we can look for possible shortcomings of the
new LAR extensions automatically and even get it into some sort of
automated regression testing.
Besides that, we could reuse a v3 (LAR) payload in v2 with such a
two-way converter.

Regards,
Carl-Daniel

[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

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

[Stefan Reinauer]

Peter Stuge wrote:
> > 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.)

There will not be a 64bit version of the above code within the next
decade. But who says never ;)

The size is explicit btw. The start address is not.

The above code would break on sc520, but that seems dead ;)

[Peter Stuge]

On Tue, Aug 28, 2007 at 01:55:30AM +0200, Stefan Reinauer wrote:
> Peter Stuge wrote:
> > > 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.)
>
> There will not be a 64bit version of the above code within the next
> decade. But who says never ;)

My point was that if this is built on amd64 0UL is 64 bit, right?
But archive.start is u32 - and we should be more explicit about what
we want to accomplish.

//Peter

[ron minnich]

On 8/29/07, Peter Stuge  wrote:
> My point was that if this is built on amd64 0UL is 64 bit, right?
> But archive.start is u32 - and we should be more explicit about what
> we want to accomplish.

I can test it at 64b tonight and see how it goes.

I'd like to have elf out of linuxbios by 11/1 or so.

thanks

ron

[Peter Stuge]

On Wed, Aug 29, 2007 at 02:47:48PM -0700, ron minnich wrote:
> On 8/29/07, Peter Stuge  wrote:
> > My point was that if this is built on amd64 0UL is 64 bit, right?
> > But archive.start is u32 - and we should be more explicit about
> > what we want to accomplish.
>
> I can test it at 64b tonight and see how it goes.

Great!
> I'd like to have elf out of linuxbios by 11/1 or so.

No objections here. :)

//Peter

[ron minnich]

64- bit version attached. Tested and working with qemu and filo.

ron

On 8/29/07, Peter Stuge  wrote:
> On Wed, Aug 29, 2007 at 02:47:48PM -0700, ron minnich wrote:
> > On 8/29/07, Peter Stuge  wrote:
> > > My point was that if this is built on amd64 0UL is 64 bit, right?
> > > But archive.start is u32 - and we should be more explicit about
> > > what we want to accomplish.
> >
> > I can test it at 64b tonight and see how it goes.
>
> Great!
> > I'd like to have elf out of linuxbios by 11/1 or so.
>
> No objections here. :)
>
> //Peter

[Peter Stuge]

On Wed, Aug 29, 2007 at 03:16:29PM -0700, ron minnich wrote:
> Make the load address and entry 64 bits.
> Remove a broken proto from a c file.
>
> The other changes are still out for an ACK, sorry, they were hard to
> break out.
> Signed-off-by: Ronald G. Minnich

Acked-by: Peter Stuge

[ron minnich]

On 8/29/07, Peter Stuge  wrote:
> On Wed, Aug 29, 2007 at 03:16:29PM -0700, ron minnich wrote:
> > Make the load address and entry 64 bits.
> > Remove a broken proto from a c file.
> >
> > The other changes are still out for an ACK, sorry, they were hard to
> > break out.
> > Signed-off-by: Ronald G. Minnich
>
> Acked-by: Peter Stuge

Committed revision 485.

[ron minnich]

Note new patch attached, in response to comments.

On 8/27/07, Peter Stuge  wrote:
> > 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..

I fixed that as per your comment, but it still can not be
backwards-compatible -- the size of the struct changes, and they are
packed in memory.
> > +++ 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() ?

Don't worry, this is a demo of combining execute-in-place AND PIC code
in initram. This is the best we have figured out so far. Let's leave
it in for now.
> > +++ 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.

Ah, that was a printk I put in and then took out. I will try to fix that. Fixed.
> > +++ 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" ? :)

It's related to a previous printk, like this:
LAR: Attempting to open 'normal/initram'.
LAR: Start 0xfff00000 len 0x100000
LAR: current filename is normal/payload
LAR: current filename is normal/option_table
LAR: current filename is normal/stage2
LAR: current filename is normal/initram
LAR: it matches normal/initram @ 0xfff088a0

It hangs together, just looks weird in the patch.
> > +++ 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?

Fixed. It was because bochs had the wrong memory size.
> > +++ 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?

This should die. It's related to the mess that we have with ELF files,
which I want to see dead.
> > 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.)

see Stefan's comment. Let's not worry about this.
> > +++ 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

it's -e now everywhere. Thanks for catching that.
> > +++ 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?

yes. Let's do that later.
> ..
> >         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.

Fixed.
> > +++ 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?

Fixed now. But we should have a common lar.h.
> > 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. :)

Let me think on that.
> > +++ 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?

removed. Fixed.
> > $(Q)$(HOSTCXX) $(HOSTCXXFLAGS) -o $@ $(LAROBJ_ABS) $(COMPRESSOR)
> > $(Q)$(HOSTCXX) $(HOSTCXXFLAGS)  -o $@ $(LAROBJ_ABS) $(COMPRESSOR)
>
> Extra space.

fixed.

new comment (included in file)
This patch is revised based on comments.

It also includes a demo of how to do a PIC initram, and:

EMERGENCY PATCH! Please see patch to lib/lar.c and include/lar.h for the
MAGIC constant. This fixes a bug I hit just now.

This patch also includes an EXPERT option for enabling no-ELF mode.
The system will default to old behaviour. See Kconfig in the root.

I still wish to kill ELF mode very soon, however.

LAR is a very capable format. With two simple extensions, we can use
LAR to replace all that we are using ELF for now. This change can
really make life better:
1. we can use streaming decompress instead of the current "uncompress
elf to memory and then copy segments" approach. So we can get rid of
THIS hardcode:
#define UNCOMPRESS_AREA (0x400000)
2. A simple lar l can show ALL segments, including payload segments
3. It's really easy to see where things will go in memory, and catch problems
4. We can figure out an ELF input file is bogus BEFORE we flash, not
AFTER we flash and try to boot it
5. did I mention streaming decompress?
6. We no longer have to worry about where we decompress the elf in
memory (this problem was causing trouble when the payload was a linux
kernel -- it was so big)
7. Since we have a load address, we can create this lar entry:
normal/cmdline
and specify that it be loaded at a place where linux will find it as
the cmdline.
8. The decision on whether to XIP can be made in the LAR entry, not in
hardcode. For example, if initram needs to be XIP, set the load
address to 0xffffffff. Done.

The change is simple. Add a load address and entry point to the lar
header. Extend the lar tool to parse the elf file and create multiple
lar segments. It looks like this:
 normal/payload0 (33192 bytes, lzma compressed to 18088 bytes @0x38
load @0x100000, entry 0x105258)
 normal/payload1 (72 bytes, lzma compressed to 47 bytes @0x4718 load
@0x1225a0, entry 0x105258)
 normal/option_table (932 bytes @0x4798 load @0, entry 0)
 normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
load @0, entry 0)
 normal/initram (4208 bytes @0x8828 load @0, entry 0)
 linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)

note that the payload is now payload0, payload1, etc. I've extended
linuxbios to look for these. Note that you can now see all the things
that get loaded ;they're no longer hidden in an ELF header somewhere.
Elf failures are gone!

Note that I've left legacy elf support in, for now, but recommend we
get rid of it as soon as possible.

patch attached. This is a first pass. lar.c needs some refactoring but
I want to get the cmdline going. You can now have a linux payload and
it will uncompress with no problems.

This has been tested with filo and BOCHS.

Signed-off-by: Ronald G. Minnich

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

[Stefan Reinauer]

* ron minnich  [070828 06:16]:
> ===================================================================
> --- lib/lar.c	(revision 480)
> +++ lib/lar.c	(working copy)
> @@ -42,9 +49,31 @@
>
> for (walk = archive->start;
>         (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> if (strcmp(walk, MAGIC) != 0)
> /* I am leaving this code in here because it is so dangerous. MAGIC is
>  * #define'd to a string. That string lives in data space. All of the 1M linuxbios 
>  * image is a LAR file. Therefore, this search can walk ALL of linuxbios. 
>  * IF the MAGIC string (in code space) just happens to be 16-byte aligned, 
>  * Then the strcmp will succeed, and you will match a non-LAR entry, 
>  * and you are screwed. can this happen? YES!
>  * LAR: Attempting to open 'fallback/initram'.
>  * LAR: Start 0xfff00000 len 0x100000
>  * LAR: current filename is normal/payload
>  * LAR: current filename is normal/option_table
>  * LAR: current filename is normal/stage2
>  * LAR: current filename is normal/initram
>  * LAR: current filename is R: it matches %s @ %p
>  * That garbage is there because the pointer is in the middle of a bunch 
>  * of non-null-terminated junk. The fix is easy, as you can see. 

I think the fix could be even simpler. Instead, if the first header is found,
the second header should be searched _after_ the end of the first file
in the LAR archive. Going through all of the ROM including the data
itself is plain stupid. I remember we did not do this in the beginning,
but we broke it since then.
> if (walk[0] != 'L')
> continue;
>
> if (strcmp(&walk[1], MAGIC) != 0)
> 	continue;
> printf("  -e pre-parse the payload ELF into LAR segments. Recommended\n\n");
> {"parseelf", 1, 0, 'p'},
> while ((opt = getopt_long(argc, argv, "acC:xls:b:vVh?",

--parseelf will not work like that.
> +/* NOTE -- This and the linuxbios lar.h are NOT IN SYNC. Be careful. */

What do you mean, by "not in sync"?

[ron minnich]

On 8/28/07, Stefan Reinauer  wrote:
> ron minnich  [070828 06:16]:
> > ===================================================================
> > --- lib/lar.c (revision 480)
> > +++ lib/lar.c (working copy)
> > @@ -42,9 +49,31 @@
> >   for (walk = archive->start;
> >        (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> >         if (strcmp(walk, MAGIC) != 0)
> >         /* I am leaving this code in here because it is so dangerous. MAGIC is
> >          * #define'd to a string. That string lives in data space. All of the 1M linuxbios
> >          * image is a LAR file. Therefore, this search can walk ALL of linuxbios.
> >          * IF the MAGIC string (in code space) just happens to be 16-byte aligned,
> >          * Then the strcmp will succeed, and you will match a non-LAR entry,
> >          * and you are screwed. can this happen? YES!
> >          * LAR: Attempting to open 'fallback/initram'.
> >          * LAR: Start 0xfff00000 len 0x100000
> >          * LAR: current filename is normal/payload
> >          * LAR: current filename is normal/option_table
> >          * LAR: current filename is normal/stage2
> >          * LAR: current filename is normal/initram
> >          * LAR: current filename is R: it matches %s @ %p
> >          * That garbage is there because the pointer is in the middle of a bunch
> >          * of non-null-terminated junk. The fix is easy, as you can see.
>
> I think the fix could be even simpler. Instead, if the first header is found,
> the second header should be searched _after_ the end of the first file
> in the LAR archive. Going through all of the ROM including the data
> itself is plain stupid. I remember we did not do this in the beginning,
> but we broke it since then.

The match appeared to be happening in the code space of linuxbios,
i.e. in the top 64k or so of rom. that string (MAGIC) was 16-bit
aligned. How can you fix that? The lar archive size is 0x100000 -- all
of FLASH.
> > printf("  -e pre-parse the payload ELF into LAR segments. Recommended\n\n");
> >         {"parseelf", 1, 0, 'p'},
> > while ((opt = getopt_long(argc, argv, "acC:xls:b:vVh?",
>
> --parseelf will not work like that.

Fixed. Patch follows in next message in reply to Carl-Daniel.

ron

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

[Stefan Reinauer]

* ron minnich  [070828 16:49]:
> > I think the fix could be even simpler. Instead, if the first header is found,
> > the second header should be searched _after_ the end of the first file
> > in the LAR archive. Going through all of the ROM including the data
> > itself is plain stupid. I remember we did not do this in the beginning,
> > but we broke it since then.
>
> The match appeared to be happening in the code space of linuxbios,
> i.e. in the top 64k or so of rom. that string (MAGIC) was 16-bit
> aligned. How can you fix that? The lar archive size is 0x100000 -- all
> of FLASH.

By walking the headers like a linked list, not the data like a brute
force search algorithm.

Stefan

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

[Carl-Daniel Hailfinger]

On 28.08.2007 16:49, ron minnich wrote:
> On 8/28/07, Stefan Reinauer  wrote:
> > ron minnich  [070828 06:16]:
> > > ===================================================================
> > > --- lib/lar.c (revision 480)
> > > +++ lib/lar.c (working copy)
> > > @@ -42,9 +49,31 @@
> > >   for (walk = archive->start;
> > >        (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> > >         if (strcmp(walk, MAGIC) != 0)
> > >         /* I am leaving this code in here because it is so dangerous. MAGIC is
> > >          * #define'd to a string. That string lives in data space. All of the 1M linuxbios
> > >          * image is a LAR file. Therefore, this search can walk ALL of linuxbios.
> > >          * IF the MAGIC string (in code space) just happens to be 16-byte aligned,
> > >          * Then the strcmp will succeed, and you will match a non-LAR entry,
> > >          * and you are screwed. can this happen? YES!
> > >          * LAR: Attempting to open 'fallback/initram'.
> > >          * LAR: Start 0xfff00000 len 0x100000
> > >          * LAR: current filename is normal/payload
> > >          * LAR: current filename is normal/option_table
> > >          * LAR: current filename is normal/stage2
> > >          * LAR: current filename is normal/initram
> > >          * LAR: current filename is R: it matches %s @ %p
> > >          * That garbage is there because the pointer is in the middle of a bunch
> > >          * of non-null-terminated junk. The fix is easy, as you can see.
> >
> > I think the fix could be even simpler. Instead, if the first header is found,
> > the second header should be searched _after_ the end of the first file
> > in the LAR archive. Going through all of the ROM including the data
> > itself is plain stupid. I remember we did not do this in the beginning,
> > but we broke it since then.
>
> The match appeared to be happening in the code space of linuxbios,
> i.e. in the top 64k or so of rom. that string (MAGIC) was 16-bit
> aligned. How can you fix that? The lar archive size is 0x100000 -- all
> of FLASH.

Can we change the MAGIC to some less likely string, perhaps even with
version info? A lot of filesystems have mixed-case magic strings, for
example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
going to include such a string by accident in his code.

Regards,
Carl-Daniel

[ron minnich]

On 8/28/07, Carl-Daniel Hailfinger  wrote:
> Can we change the MAGIC to some less likely string, perhaps even with
> version info? A lot of filesystems have mixed-case magic strings, for
> example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
> going to include such a string by accident in his code.

That's not the real problem. The real problem is that if you do this:
char *c = MAGIC;

Then that string is in the data space of linuxbois. If it ever gets to
be 16-byte aligned, the find_file in lar will think it has found  a
header. I just realized stefan's fix might solve the problem. But we
can add his fix later.

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 17:12, ron minnich wrote:
> On 8/28/07, Carl-Daniel Hailfinger  wrote:
> > Can we change the MAGIC to some less likely string, perhaps even with
> > version info? A lot of filesystems have mixed-case magic strings, for
> > example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
> > going to include such a string by accident in his code.
>
> That's not the real problem. The real problem is that if you do this:
> char *c = MAGIC;
>
> Then that string is in the data space of linuxbois. If it ever gets to
> be 16-byte aligned, the find_file in lar will think it has found  a
> header. I just realized stefan's fix might solve the problem. But we
> can add his fix later.

Can't we do that in an easier way? Idea:
struct {
    char misalign[1];
        char magic[8];
} lar_magic __attribute__ ((aligned(16))) = {
    0, "LARCHIVE"
};

That should guarantee the string to be always misaligned.

Regards,
Carl-Daniel

[ron minnich]

On 8/28/07, Carl-Daniel Hailfinger  wrote:
> Can't we do that in an easier way? Idea:
> struct {
>         char misalign[1];
>         char magic[8];
> } lar_magic __attribute__ ((aligned(16))) = {
>         0, "LARCHIVE"
> };
>
> That should guarantee the string to be always misaligned.

My personal preference is not to depend on this type of magic. The fix
I posted is really impossible to fail, and we want to guarantee no
failure in this important case.

Stefan's comment about not doing stupid brute force is a good point. I
am at fault for putting that in, but I did that because in the early
days there were bugs in linuxbios LAR parsing code ... we need to take
out the brute force.

thanks

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 17:29, ron minnich wrote:
> On 8/28/07, Carl-Daniel Hailfinger  wrote:
> > Can't we do that in an easier way? Idea:
> > struct {
> >         char misalign[1];
> >         char magic[8];
> > } lar_magic __attribute__ ((aligned(16))) = {
> >         0, "LARCHIVE"
> > };
> >
> > That should guarantee the string to be always misaligned.
>
> My personal preference is not to depend on this type of magic. The fix
> I posted is really impossible to fail, and we want to guarantee no
> failure in this important case.

If somewhere in a lar file the magic appears with correct alignment
(maybe due to a payload we don't control), your fix will fail as well.
However, my fix is not better in that case.
> Stefan's comment about not doing stupid brute force is a good point. I
> am at fault for putting that in, but I did that because in the early
> days there were bugs in linuxbios LAR parsing code ... we need to take
> out the brute force.

Yes, that would solve all problems.

Regards,
Carl-Daniel

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

[Uwe Hermann]

On Tue, Aug 28, 2007 at 04:59:58PM +0200, Carl-Daniel Hailfinger wrote:
> Can we change the MAGIC to some less likely string, perhaps even with
> version info? A lot of filesystems have mixed-case magic strings, for
> example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
> going to include such a string by accident in his code.

Yes, good idea IMO.

Maybe 'LAR Magic 0.01' or something similar?

Uwe.

[ron minnich]

On 8/28/07, Uwe Hermann  wrote:
> Yes, good idea IMO.
>
> Maybe 'LAR Magic 0.01' or something similar?

Again, given current brute force search, this won't help, since that
string will appear in the data segment. The most reliable fix is the
one I posted; the most proper fix is stefan's "let's stop being
stupid" which I will try to implement now.

ron

[ron minnich]

On 8/28/07, ron minnich  wrote:
> On 8/28/07, Uwe Hermann  wrote:
> > Yes, good idea IMO.
> >
> > Maybe 'LAR Magic 0.01' or something similar?
>
> Again, given current brute force search, this won't help, since that
> string will appear in the data segment. The most reliable fix is the
> one I posted; the most proper fix is stefan's "let's stop being
> stupid" which I will try to implement now.

OOOPS. We are being smart, it turns out:

header = (struct lar_header *)walk;
                fullname = walk + sizeof(struct lar_header);

printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
                // FIXME: check checksum

if (strcmp(fullname, filename) == 0) {
                        printk(BIOS_SPEW, "LAR: it matches %s @ %p\n",
fullname, header);
                        result->start = walk + ntohl(header->offset);
                        result->len = ntohl(header->len);
                        result->reallen = ntohl(header->reallen);
                        result->compression = ntohl(header->compression);
                        result->entry = (void *)ntohl(header->entry);
                        result->loadaddress = (void
*)ntohl(header->loadaddress);
                        printk(BIOS_SPEW, "start %p len %d reallen %d
compression %x entry %p loadaddress %p\n",
                                result->start, result->len,
result->reallen, result->compression, result->entry,
result->loadaddress);
                        return 0;
                }
                // skip file
                walk += (ntohl(header->len) + ntohl(header->offset) -
                        1) & 0xfffffff0;

But it still failed ...

hmm.

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 17:41, ron minnich wrote:
> On 8/28/07, ron minnich  wrote:
> > On 8/28/07, Uwe Hermann  wrote:
> > > Yes, good idea IMO.
> > >
> > > Maybe 'LAR Magic 0.01' or something similar?
> >
> > Again, given current brute force search, this won't help, since that
> > string will appear in the data segment. The most reliable fix is the
> > one I posted; the most proper fix is stefan's "let's stop being
> > stupid" which I will try to implement now.
>
> OOOPS. We are being smart, it turns out:

lib/lar.c and util/lar/example.c differ in subtle ways in find_file.
Inverted logic in one file, bogus calculations in the other one. We
might want to make sure they behave the same way.
>             header = (struct lar_header *)walk;
>             fullname = walk + sizeof(struct lar_header);
>
>             printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
>             // FIXME: check checksum
>
>             if (strcmp(fullname, filename) == 0) {
>                     printk(BIOS_SPEW, "LAR: it matches %s @ %p\n",
>
> fullname, header);
>                         result->start = walk + ntohl(header->offset);
>                         result->len = ntohl(header->len);
>                         result->reallen = ntohl(header->reallen);
>                         result->compression = ntohl(header->compression);
>                         result->entry = (void *)ntohl(header->entry);
>                         result->loadaddress = (void
> *)ntohl(header->loadaddress);
>                         printk(BIOS_SPEW, "start %p len %d reallen %d
> compression %x entry %p loadaddress %p\n",
>                                 result->start, result->len,
> result->reallen, result->compression, result->entry,
> result->loadaddress);
>                         return 0;
>                 }
>                 // skip file
>                 walk += (ntohl(header->len) + ntohl(header->offset) -
>                         1) & 0xfffffff0;

ARGH! Shouldn't that be

walk += (ntohl(header->len) + ntohl(header->offset) + 15) & 0xfffffff0;

Carl-Daniel

[ron minnich]

On 8/28/07, Carl-Daniel Hailfinger  wrote:
> lib/lar.c and util/lar/example.c differ in subtle ways in find_file.
> Inverted logic in one file, bogus calculations in the other one. We
> might want to make sure they behave the same way.
> >             header = (struct lar_header *)walk;
> >             fullname = walk + sizeof(struct lar_header);
> >
> >             printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
> >             // FIXME: check checksum
> >
> >             if (strcmp(fullname, filename) == 0) {
> >                     printk(BIOS_SPEW, "LAR: it matches %s @ %p\n",
> >
> > fullname, header);
> >                         result->start = walk + ntohl(header->offset);
> >                         result->len = ntohl(header->len);
> >                         result->reallen = ntohl(header->reallen);
> >                         result->compression = ntohl(header->compression);
> >                         result->entry = (void *)ntohl(header->entry);
> >                         result->loadaddress = (void
> > *)ntohl(header->loadaddress);
> >                         printk(BIOS_SPEW, "start %p len %d reallen %d
> > compression %x entry %p loadaddress %p\n",
> >                                 result->start, result->len,
> > result->reallen, result->compression, result->entry,
> > result->loadaddress);
> >                         return 0;
> >                 }
> >                 // skip file
> >                 walk += (ntohl(header->len) + ntohl(header->offset) -
> >                         1) & 0xfffffff0;
>
> ARGH! Shouldn't that be
>
> walk += (ntohl(header->len) + ntohl(header->offset) + 15) & 0xfffffff0;

OK, I added this to the code:
                if (strcmp(&walk[0], "LARCHIVE") != 0)
                        continue;
before the other fix I created. It dies:
LinuxBIOS-3.0.0 Tue Aug 28 08:21:43 PDT 2007 starting...
Choosing fallback boot.
LAR: Attempting to open 'fallback/initram'.
LAR: Start 0xfff00000 len 0x100000
LAR: search for normal/payload
LAR: search for normal/option_table
LAR: search for normal/stage2
LAR: search for normal/initram
LAR: search for %s @ %p

So I put in your fix (replace -1 with +15)
And it is worse:
LinuxBIOS-3.0.0 Tue Aug 28 08:21:43 PDT 2007 starting...
Choosing fallback boot.
LAR: Attempting to open 'fallback/initram'.
LAR: Start 0xfff00000 len 0x100000
LAR: search for normal/payload
LAR: search for normal/stage2
LAR: search for %s @ %p

So, how about we leave my patch in for now while I try to track this
nasty bug down?

thanks

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 18:19, ron minnich wrote:
> On 8/28/07, Carl-Daniel Hailfinger  wrote:
> > lib/lar.c and util/lar/example.c differ in subtle ways in find_file.
> > Inverted logic in one file, bogus calculations in the other one. We
> > might want to make sure they behave the same way.

This still applies. However, it can be fixed in another commit.
> > >             walk += (ntohl(header->len) + ntohl(header->offset) -
> > >                     1) & 0xfffffff0;
> >
> > ARGH! Shouldn't that be
> >
> > walk += (ntohl(header->len) + ntohl(header->offset) + 15) & 0xfffffff0;
>
> OK, I added this to the code:
>                 if (strcmp(&walk[0], "LARCHIVE") != 0)
>                         continue;
> before the other fix I created. It dies:
> LinuxBIOS-3.0.0 Tue Aug 28 08:21:43 PDT 2007 starting...
> Choosing fallback boot.
> LAR: Attempting to open 'fallback/initram'.
> LAR: Start 0xfff00000 len 0x100000
> LAR: search for normal/payload
> LAR: search for normal/option_table
> LAR: search for normal/stage2
> LAR: search for normal/initram
> LAR: search for %s @ %p
>
> So I put in your fix (replace -1 with +15)

Yes, I just reread that code and have to agree my +15 solution was not
the right one. Maybe we could add some debug printing to find out where
the code is looking for MAGIC, but I'd leave that to a later patch.
> And it is worse:
> LinuxBIOS-3.0.0 Tue Aug 28 08:21:43 PDT 2007 starting...
> Choosing fallback boot.
> LAR: Attempting to open 'fallback/initram'.
> LAR: Start 0xfff00000 len 0x100000
> LAR: search for normal/payload
> LAR: search for normal/stage2
> LAR: search for %s @ %p
>
> So, how about we leave my patch in for now while I try to track this
> nasty bug down?

Agreed.

Regards,
Carl-Daniel

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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [070828 18:08]:
> lib/lar.c and util/lar/example.c differ in subtle ways in find_file.
> Inverted logic in one file, bogus calculations in the other one. We
> might want to make sure they behave the same way.

I remember "fixing" something because the comparison would go wrong if a
file is not found in the archive and the last element in the archive
(bootblock) was encountered. The 32bit pointer would overrun from
0x100000000 to 0x00000000 so the exit condition would never bite.

I might have introduced something more stupid. Solving all the small
side cases suddenly became somewhat untrivial ;)

Stefan

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

[Jordan Crouse]

On 28/08/07 17:34 +0200, Uwe Hermann wrote:
> On Tue, Aug 28, 2007 at 04:59:58PM +0200, Carl-Daniel Hailfinger wrote:
> > Can we change the MAGIC to some less likely string, perhaps even with
> > version info? A lot of filesystems have mixed-case magic strings, for
> > example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
> > going to include such a string by accident in his code.
>
> Yes, good idea IMO.
>
> Maybe 'LAR Magic 0.01' or something similar?

I like it.  We should probably compress it down some, and turn the number 
into hex for maximum versioning, something like

LaRisCOol001  to LaRisCOolFFF

As long as we can match on the first section for identification and the 
second section for version, we'll be golden.

Jordan

[ron minnich]

Until I know exactly why that failure occurred, I'd like to leave my
emergency patch in. Esp. since it turns out we WERE being smart ...

thanks

ron

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

[Stefan Reinauer]

* ron minnich  [070828 17:43]:
> Until I know exactly why that failure occurred, I'd like to leave my
> emergency patch in. Esp. since it turns out we WERE being smart ...

thats fine as long as it's not committed like that.

We had so many workarounds in older versions because we did not
understand what was going on at a given time. Let's not get back there.

Stefan

[ron minnich]

Round 2.

I had not realized that lar had changed. So all my patches were
against the wrong files.

This code is against the right files in lar. I have also done a fair
amount of refactoring as I needed it.

don't get upset about the close of the fd right after mmap; it's legal
and even recommended.

Comments welcome as usual.

ron
p.s. I can not get the LARCHIVE problem to NOT happen now. So, until
we fix it right, let's take my wrong fix. Three of us have tried and
failed to fix this, so, let's assume it's not trivial.

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

[Carl-Daniel Hailfinger]

On 28.08.2007 20:41, ron minnich wrote:
> Round 2.
>
> I had not realized that lar had changed. So all my patches were
> against the wrong files.
>
> This code is against the right files in lar. I have also done a fair
> amount of refactoring as I needed it.
>
> don't get upset about the close of the fd right after mmap; it's legal
> and even recommended.
>
> Comments welcome as usual.
>
> ron
> p.s. I can not get the LARCHIVE problem to NOT happen now. So, until
> we fix it right, let's take my wrong fix. Three of us have tried and
> failed to fix this, so, let's assume it's not trivial.

I have added LARCHIVE debugging code to your patch below to find out why
and where the problem happens. Could you post the log of your patch with
my added debugging? I hope we can see an obvious fix through that.

Other than that, this patch is Acked by me, but please wait for an
additional ack from somebody else.

Carl-Daniel
> This patch is revised based on comments.
>
> It also includes a demo of how to do a PIC initram, and:
>
> EMERGENCY PATCH! Please see patch to lib/lar.c and include/lar.h for the 
> MAGIC constant. This fixes a bug I hit just now.
>
> This patch also includes an EXPERT option for enabling no-ELF mode. 
> The system will default to old behaviour. See Kconfig in the root.
>
> I still wish to kill ELF mode very soon, however.
>
> LAR is a very capable format. With two simple extensions, we can use
> LAR to replace all that we are using ELF for now. This change can
> really make life better:
> we can use streaming decompress instead of the current "uncompress
>
> elf to memory and then copy segments" approach. So we can get rid of
> THIS hardcode:
> #define UNCOMPRESS_AREA (0x400000)
> 2. A simple lar l can show ALL segments, including payload segments
> 3. It's really easy to see where things will go in memory, and catch problems
> 4. We can figure out an ELF input file is bogus BEFORE we flash, not
> AFTER we flash and try to boot it
> 5. did I mention streaming decompress?
> 6. We no longer have to worry about where we decompress the elf in
> memory (this problem was causing trouble when the payload was a linux
> kernel -- it was so big)
> 7. Since we have a load address, we can create this lar entry:
> normal/cmdline
> and specify that it be loaded at a place where linux will find it as
> the cmdline.
> 8. The decision on whether to XIP can be made in the LAR entry, not in
> hardcode. For example, if initram needs to be XIP, set the load
> address to 0xffffffff. Done.
>
> The change is simple. Add a load address and entry point to the lar
> header. Extend the lar tool to parse the elf file and create multiple
> lar segments. It looks like this:
>  normal/payload0 (33192 bytes, lzma compressed to 18088 bytes @0x38
> load @0x100000, entry 0x105258)
>  normal/payload1 (72 bytes, lzma compressed to 47 bytes @0x4718 load
> @0x1225a0, entry 0x105258)
>  normal/option_table (932 bytes @0x4798 load @0, entry 0)
>  normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
> load @0, entry 0)
>  normal/initram (4208 bytes @0x8828 load @0, entry 0)
>  linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)
>
> note that the payload is now payload/segment0, payload/segment1, etc. I've extended
> linuxbios to look for these. Note that you can now see all the things
> that get loaded ;they're no longer hidden in an ELF header somewhere.
> Elf failures are gone!
>
> Note that I've left legacy elf support in, for now, but recommend we
> get rid of it as soon as possible.
>
> patch attached. This is a first pass. lar.c needs some refactoring but
> I want to get the cmdline going. You can now have a linux payload and
> it will uncompress with no problems.
>
> This has been tested with filo and BOCHS.
>
> Signed-off-by: Ronald G. Minnich 
> Index: Kconfig
> ===================================================================
> --- Kconfig	(revision 480)
> +++ Kconfig	(working copy)
> @@ -53,6 +53,25 @@
>    help
>      Append an extra string to the end of the LinuxBIOS version.
>
> +config NOELF
> bool "Don't use ELF for payloads"
> depends EXPERT
> default n
> help
>  Until now, LinuxBIOS has used elf for the payload. There are many problems 
>  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, LinuxBIOS will direct lar to break each elf segment into a LAR 
>  entry. ELF will not be used at all. Note that (for now) LinuxBIOS is backward
>  compatible -- if you put an ELF payload in, LinuxBIOS can still parse it. We hope
>  to remove ELF entirely in the future. 
>
> config BEEPS
>    bool "Enable beeps upon certain LinuxBIOS events"
>    depends EXPERT
> Index: include/lar.h
> ===================================================================
> --- include/lar.h	(revision 480)
> +++ include/lar.h	(working copy)
> @@ -52,9 +52,10 @@
>
> #include <types.h>
>
> -#define MAGIC "LARCHIVE"
> +/* see note in lib/lar.c as to why this is ARCHIVE and not LARCHIVE */
> +#define MAGIC "ARCHIVE"
>  #define MAX_PATHLEN 1024
>
> +/* NOTE -- This and the user-mode lar.h are NOT IN SYNC. Be careful. */
>  struct lar_header {
>    char magic[8];
>    u32 len;
> @@ -62,7 +63,14 @@
>    u32 checksum;
>    u32 compchecksum;
>    u32 offset;
> /* Compression:
> * 0 = no compression
> * 1 = lzma
> * 2 = nrv2b
> */
> u32 compression;
> u32 entry;		/* we might need to make this u64 */
> u32 loadaddress; /* ditto */
>
> };
>
> struct mem_file {
> @@ -70,6 +78,8 @@
>    int len;
>    u32 reallen;
>    u32 compression;
> void *entry;
> void *loadaddress;
>
> };
>
> /* Prototypes. */
> @@ -77,5 +87,6 @@
>  int copy_file(struct mem_file *archive, char *filename, void *where);
>  int run_file(struct mem_file *archive, char *filename, void *where);
>  int execute_in_place(struct mem_file *archive, char *filename);
>
> +int run_address(void *f);
> +void *load_file(struct mem_file *archive, char *filename);
>  #endif /* LAR_H */
> Index: mainboard/emulation/qemu-x86/initram.c
> ===================================================================
> --- mainboard/emulation/qemu-x86/initram.c	(revision 480)
> +++ mainboard/emulation/qemu-x86/initram.c	(working copy)
> @@ -19,10 +19,17 @@
>
> #include <console.h>
>
> +/* This is how we force an absolute call when we are in PIC code (which this file is)
> If there is a nicer way to do this, I'd like to hear it. Ideally, we could tell
> gcc to force abs jumps on certain symbols.
> */
>
> +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");
>
> return 0;
>
> }
> Index: mainboard/emulation/qemu-x86/Makefile
> ===================================================================
> --- mainboard/emulation/qemu-x86/Makefile	(revision 480)
> +++ mainboard/emulation/qemu-x86/Makefile	(working copy)
> @@ -41,14 +41,17 @@
>  #
>
> INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> +$(obj)/mainboard/$(MAINBOARDDIR)/initram.o: $(src)/mainboard/$(MAINBOARDDIR)/initram.c
> cc -c $(INITCFLAGS)  -fPIE $(src)/mainboard/$(MAINBOARDDIR)/initram.c -o $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
>
> # These are possibly not permanent
> -INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
> +#INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
>
> $(obj)/linuxbios.initram: $(obj)/stage0.init $(obj)/stage0.o $(INITRAM_OBJ)
>    $(Q)# initram links against stage0
>    $(Q)printf "  LD      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(LD)  -Ttext 0x80000 $(INITRAM_OBJ) \
> --entry=main -o $(obj)/linuxbios.initram.o
> $(Q)$(LD)  $(INITRAM_OBJ) \
> --entry=main -R $(obj)/stage0.o -o $(obj)/linuxbios.initram.o
> $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) -O binary $(obj)/linuxbios.initram.o \
> $(obj)/linuxbios.initram
> Index: lib/lar.c
>
> --- lib/lar.c	(revision 480)
> +++ lib/lar.c	(working copy)
> @@ -31,6 +31,13 @@
>  #define ntohl(x) (x)
>  #endif
>
> +int run_address(void *f)
> +{
> int (*v) (void);
> v = f;
> return v();
>
> +}
>
> int find_file(struct mem_file *archive, char *filename, struct mem_file *result)
>  {
>    char *walk, *fullname;
> @@ -42,29 +49,103 @@
>
> for (walk = archive->start;
>         (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> if (strcmp(walk, MAGIC) != 0)
> /* I am leaving this code in here because it is so dangerous. MAGIC is
>  * #define'd to a string. That string lives in data space. All of the 1M linuxbios 
>  * image is a LAR file. Therefore, this search can walk ALL of linuxbios. 
>  * IF the MAGIC string (in code space) just happens to be 16-byte aligned, 
>  * Then the strcmp will succeed, and you will match a non-LAR entry, 
>  * and you are screwed. can this happen? YES!
>  * LAR: Attempting to open 'fallback/initram'.
>  * LAR: Start 0xfff00000 len 0x100000
>  * LAR: current filename is normal/payload
>  * LAR: current filename is normal/option_table
>  * LAR: current filename is normal/stage2
>  * LAR: current filename is normal/initram
>  * LAR: current filename is R: it matches %s @ %p
>  * That garbage is there because the pointer is in the middle of a bunch 
>  * of non-null-terminated junk. The fix is easy, as you can see. 
>  * if (strcmp(walk, MAGIC) != 0)
>  *	continue;
>  * And, yes, this did indeed fix the problem!
>  */
> if (walk[0] != 'L')
> continue;
>
> if (strcmp(&walk[1], MAGIC) != 0)
> 	continue;

Maybe change the code above to add add extra debugging code:
> printk(BIOS_SPEW, "LAR: looking for archive header at %p", walk);
> if (walk[0] != 'L') {
> 	printk(BIOS_SPEW, "LAR: no L magic at %p", walk);
> continue;
> }
>
> if (strcmp(&walk[1], MAGIC) != 0) {
> 	printk(BIOS_SPEW, "LAR: no ARCHIVE magic at %p", walk);
> 	continue;
> }
> printk(BIOS_SPEW, "LAR: full magic at %p", walk);
> header = (struct lar_header *)walk;
> fullname = walk + sizeof(struct lar_header);
>
> printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
> printk(BIOS_SPEW, "LAR: search for %s\n", fullname);
>
> // FIXME: check checksum
>
> if (strcmp(fullname, filename) == 0) {
> 	printk(BIOS_SPEW, "LAR: FOUND %s @ %p\n", fullname, header);
> result->start = walk + ntohl(header->offset);
> result->len = ntohl(header->len);
> result->reallen = ntohl(header->reallen);
> result->compression = ntohl(header->compression);
> 	result->entry = (void *)ntohl(header->entry);
> 	result->loadaddress = (void *)ntohl(header->loadaddress);
> 	printk(BIOS_SPEW, 
> 	"start %p len %d reallen %d compression %x entry %p loadaddress %p\n", 
> 		result->start, result->len, result->reallen, 
> 		result->compression, result->entry, result->loadaddress);
> return 0;
>
> }
> // skip file
> walk += (ntohl(header->len) + ntohl(header->offset) -
> 	1) & 0xfffffff0;
> }
>
> printk(BIOS_SPEW, "NO FILE FOUND\n");
> return 1;
>
> }
>
> +void *load_file(struct mem_file *archive, char *filename)
> +{
> int ret;
> struct mem_file result;
> void *where;
> void *entry;
> ret = find_file(archive, filename, &result);
> if (ret) {
> printk(BIOS_INFO, "LAR: load_file: No such file '%s'\n",
>        filename);
> return (void *)-1;
> }
> entry = result.entry;
> where = result.loadaddress;
> printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", result.compression);
> /* no compression */
> if (result.compression == 0) {
> memcpy(where, result.start, result.len);
> return entry;
> }
>
> +#ifdef CONFIG_COMPRESSION_LZMA
> /* lzma */
> unsigned long ulzma(unsigned char *src, unsigned char *dst);
> if (result.compression == 1) {
> ulzma(result.start, where);
> return entry;
> }
>
> +#endif
> +#ifdef CONFIG_COMPRESSION_NRV2B
> /* nrv2b */
> unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
> if (result.compression == 2) {
> int tmp;
> unrv2b(result.start, where, &tmp);
> return entry;
> }
>
> +#endif
> printk(BIOS_INFO, "LAR: Compression algorithm #%i not supported!\n", result.compression);
> return (void *)-1;
>
> +}
>
> +/* FIXME -- most of copy_file should be replaced by load_file */
>  int copy_file(struct mem_file *archive, char *filename, void *where)
>  {
>    int ret;
> @@ -85,7 +166,7 @@
>    }
>  #ifdef CONFIG_COMPRESSION_LZMA
>    /* lzma */
> unsigned long ulzma(unsigned char * src, unsigned char * dst);
> unsigned long ulzma(unsigned char *src, unsigned char *dst);
> if (result.compression == 1) {
> ulzma(result.start, where);
> return 0;
>
> @@ -93,7 +174,7 @@
>  #endif
>  #ifdef CONFIG_COMPRESSION_NRV2B
>    /* nrv2b */
> unsigned long unrv2b(u8 * src, u8 * dst, unsigned long *ilen_p);
> unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
> if (result.compression == 2) {
> int tmp;
> unrv2b(result.start, where, &tmp);
>
> @@ -113,6 +194,7 @@
>  {
>    int (*v) (void);
>    struct mem_file result;
>
> int ret;
>
> if ((u32) where != 0xFFFFFFFF) {
> if (copy_file(archive, filename, where)) {
>
> @@ -130,9 +212,11 @@
>    	}
>    	where = result.start;
>    }
> printk(BIOS_SPEW, "where is %p\n", where);
> v = where;
> return v();
> ret =  v();
> printk(BIOS_SPEW, "run_file returns with %d\n", ret);
> return ret;
>
> }
>
> /**
> Index: lib/stage2.c
> ===================================================================
> --- lib/stage2.c	(revision 480)
> +++ lib/stage2.c	(working copy)
> @@ -99,5 +99,6 @@
>    write_tables();
>    show_all_devs();
> printk(BIOS_SPEW, "STAGE2 NOW RETURNING\n");
> return 0;
>
> }
> Index: arch/x86/stage1.c
> ===================================================================
> --- arch/x86/stage1.c	(revision 480)
> +++ arch/x86/stage1.c	(working copy)
> @@ -22,12 +22,16 @@
>  #include <io.h>
>  #include <console.h>
>  #include <lar.h>
> +#include <string.h>
>  #include <tables.h>
>  #include <lib.h>
>  #include <mc146818rtc.h>
>  #include <post_code.h>
>
> -#define UNCOMPRESS_AREA 0x60000
> +/* ah, well, what a mess! This is a hard code. FIX ME but how?
> By getting rid of ELF ...
> */
>
> +#define UNCOMPRESS_AREA (0x400000)
>
> /* these prototypes should go into headers */
>  void uart_init(void);
> @@ -48,6 +52,24 @@
>    post_code(0xf2);
>  }
>
> +/* until we get rid of elf */
> +int legacy(struct mem_file *archive, char *name, void *where, struct lb_memory *mem)
> +{
> int ret;
> struct mem_file result;
> 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, result.reallen);
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> return -1;
>
> +}
>
> /*
> This function is called from assembler code whith its argument on the
> stack. Force the compiler to generate always correct code for this case.
>
> @@ -57,6 +79,8 @@
>    int ret;
>    struct mem_file archive, result;
>    int elfboot_mem(struct lb_memory *mem, void *where, int size);
>
> void *entry;
>
> int i;
>
> /* we can't statically init this hack. */
> unsigned char faker[64];
>
> @@ -144,20 +168,32 @@
>    printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> ret = find_file(&archive, "normal/payload", &result);
> if (ret) {
> printk(BIOS_ERR, "No such file '%s'.\n", "normal/payload");
> die("FATAL: No payload found.\n");
> if (! ret)
> legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
> /* new style lar boot. Install all the files in memory.
>  * By convention we take the entry point from the first 
>  * one. Look for a cmdline as well. 
>  */
> for(i = 0, entry = (void *)0; ;i++) {
> char filename[64];
> void *newentry;
> sprintf(filename, "normal/payload/segment%d", i);
> archive.len = *(u32 *)0xfffffff4;
> archive.start =(void *)(0UL-archive.len);
> newentry = load_file(&archive, filename);
> printk("newentry is %p\n", newentry);
> if (newentry == (void *)-1)
> 	break;
> if (! entry)
> 	entry = newentry;
> }
> ret = copy_file(&archive, "normal/payload", (void *)UNCOMPRESS_AREA);
> if (ret) {
> printk(BIOS_ERR, "'%s' found, but could not load it.\n", "normal/payload");
> die("FATAL: No usable payload found.\n");
> }
> printk(BIOS_SPEW, "all loaded, entry %p\n", entry);
> run_address(entry);
> ret =  elfboot_mem(mem, (void *)UNCOMPRESS_AREA, result.reallen);
> die("FATAL: No usable payload found.\n");
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> die ("FATAL: Last stage returned to LinuxBIOS.\n");
>
> }
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 480)
> +++ arch/x86/Makefile	(working copy)
> @@ -22,7 +22,7 @@
>  ifeq ($(CONFIG_ARCH_X86),y)
>
> INITCFLAGS := $(CFLAGS) -I$(src)/include/arch/x86 -I$(src)/include \
> -I$(obj) -fno-builtin
> -I$(obj) -fno-builtin
>
> SILENT := >/dev/null 2>&1
>
> @@ -78,7 +78,7 @@
>  endif
>    $(Q)printf "  LAR     $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)rm -f $(obj)/linuxbios.rom
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(COMPRESSFLAG) -c \
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -c \
> ../linuxbios.rom \
> $(LARFILES) \
> -s $(ROM_SIZE) -b $(obj)/linuxbios.bootblock
>
> @@ -122,6 +122,11 @@
>  endif
>  endif
>
> +ifeq ($(CONFIG_NOELF), y)
> PARSEELF = -e
>
> +else
> PARSEELF =
>
> +endif
>
> STAGE0_OBJ := $(patsubst %,$(obj)/lib/%,$(STAGE0_LIB_OBJ)) \
>          $(patsubst %,$(obj)/arch/x86/%,$(STAGE0_ARCH_X86_OBJ)) \
> @@ -143,6 +148,9 @@
>    $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)$(OBJCOPY) -O binary $(obj)/stage0.o $(obj)/stage0.init
> $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o $(obj)/stage0_link.o
> $(Q)printf "  TEST    $(subst $(shell pwd)/,,$(@))\n"
>  $(Q)test `wc -c < $(obj)/stage0.init` -gt 16128 && \
>   printf "Error. Bootblock got too big.\n" || true
> Index: util/lar/lar.c
>
> --- util/lar/lar.c	(revision 480)
> +++ util/lar/lar.c	(working copy)
> @@ -30,13 +30,14 @@
>  #include <sys/stat.h>
>  #include <sys/mman.h>
>
> +#include "lar.h"
>  #include "lib.h"
> -#include "lar.h"
>
> #define VERSION "v0.9.1"
>  #define COPYRIGHT "Copyright (C) 2006-2007 coresystems GmbH"
>
> static int isverbose = 0;
> +static int iselfparse = 0;
>  static long larsize = 0;
>  static char *bootblock = NULL;
>  enum compalgo algo = none;
> @@ -44,7 +45,7 @@
>  static void usage(char *name)
>  {
>    printf("\nLAR - the LinuxBIOS Archiver " VERSION "\n" COPYRIGHT "\n\n"
>       "Usage: %s [-cxal] archive.lar [[[file1] file2] ...]\n\n", name);
>       "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 -a myrom.lar foo blob:baz\n");
>
> @@ -66,13 +67,18 @@
>    printf("                \ta 'm' suffix to multiple the size by 1024*1024.\n");
>    printf("  -b [bootblock]\tSpecify the bootblock blob\n");
>    printf("  -C [lzma|nrv2b]\tSpecify the compression method to use\n\n");
>
> printf("  -e pre-parse the payload ELF into LAR segments. Recommended\n\n");
>
> printf("General options\n");
> printf("  -v\tEnable verbose mode\n");
> printf("  -V\tShow the version\n");
> printf("  -h\tShow this help\n");
> printf("\n");
>
> +}
>
> +int elfparse(void)
> +{
> return iselfparse;
>
> }
>
> /* Add a human touch to the LAR size by allowing suffixes:
> @@ -209,6 +215,7 @@
>    	{"list", 0, 0, 'l'},
>    	{"size", 1, 0, 's'},
>    	{"bootblock", 1, 0, 'b'},
> {"elfparse", 1, 0, 'e'},
> {"verbose", 0, 0, 'v'},
> {"version", 0, 0, 'V'},
> {"help", 0, 0, 'h'},
>
> @@ -220,7 +227,7 @@
>    	exit(1);
>    }
> while ((opt = getopt_long(argc, argv, "acC:xls:b:vVh?",
> while ((opt = getopt_long(argc, argv, "acC:xels:b:vVh?",
> 		  long_options, &option_index)) != EOF) {
> switch (opt) {
> case 'a':
>
> @@ -240,6 +247,9 @@
>    	case 'l':
>    		larmode = LIST;
>    		break;
> case 'e':
> 	iselfparse = 1;
> 	break;
> case 'x':
> 	larmode = EXTRACT;
> 	break;
> Index: util/lar/lar.h
>
> --- util/lar/lar.h	(revision 480)
> +++ util/lar/lar.h	(working copy)
> @@ -60,6 +60,7 @@
>  typedef uint32_t u32;
>  typedef uint8_t  u8;
>
> +/* NOTE -- This and the linuxbios lar.h are NOT IN SYNC. Be careful. */
>  struct lar_header {
>    char magic[8];
>    u32 len;
> @@ -73,6 +74,8 @@
>     * 2 = nrv2b
>     */
>    u32 compression;
> u32 entry;		/* we might need to make this u64 */
> u32 loadaddress; /* ditto */
>
> };
>
> /**\struct
> Index: util/lar/lib.h
> ===================================================================
> --- util/lar/lib.h	(revision 480)
> +++ util/lar/lib.h	(working copy)
> @@ -38,6 +38,7 @@
>
> /* prototypes for lar.c functions */
>  int verbose(void);
> +int elfparse(void);
>  long get_larsize(void);
>  char *get_bootblock(void);
>
> @@ -50,13 +51,24 @@
>  struct file *get_files(void);
>  void free_files(void);
>
> +/* Prototypes for ELF functions */
> +int iself(char *filebuf);
>
> +/* Prototypes for in-memory LAR operations */
> +int lar_process_name(char *name, char **pfilename, char **ppathname,
> enum compalgo *thisalgo);
>
> +u32 lar_compress(char *ptr, ssize_t size, char *temp, enum compalgo *thisalgo);
> +int lar_add_entry(struct lar *lar, char *pathname, void *data,
> u32 complen, u32 reallen, u32 loadaddress, u32 entry,
> enum compalgo thisalgo);
>
> /* Prototypes for the LAR I/O functions */
> +char *mapfile(char *filename, u32 *size);
>  struct lar * lar_new_archive(const char *archive, unsigned int size);
>  struct lar * lar_open_archive(const char *archive);
>  void lar_close_archive(struct lar *lar);
>
> void lar_list_files(struct lar *lar, struct file *files);
> -int lar_add_file(struct lar *lar, const char *name);
> +int lar_add_file(struct lar *lar, char *name);
>  int lar_add_bootblock(struct lar *lar, const char *bootblock);
>  int lar_extract_files(struct lar *lar, struct file *files);
> Index: util/lar/stream.c
>
> --- util/lar/stream.c	(revision 480)
> +++ util/lar/stream.c	(working copy)
> @@ -32,9 +32,10 @@
>  #include <sys/mman.h>
>  #include <netinet/in.h>
>  #include <libgen.h>
> +#include <elf.h>
>
> +#include "lar.h"
>  #include "lib.h"
> -#include "lar.h"
>
> /**
> \def err(fmt,args...)
>
> @@ -44,7 +45,126 @@
>
> extern enum compalgo algo;
>
> +/* ELF processing */
>  /**
> Given a ptr to data, determine if the data is an ELF image.
> @param filebuf pointer to the data
> @return 1 if ELF, 0 if not
> */
>
> +int iself(char *filebuf)
> +{
> Elf32_Ehdr *ehdr;
> /* validate elf header */
> ehdr = (Elf32_Ehdr *)filebuf;
> if (memcmp(ehdr->e_ident, ELFMAG, 4))
> return 0;
> return 1;
>
> +}
>
> +/**
> Output all the ELF segments for a given file
> @param lar The LAR Archoe
> @param name The LAR name
> @param filebuf The ELF file
> @param filelen Size of the ELF file
> @param algo The recommend compression algorithm
> Return 0 on success, -1 on failure
> */
>
> +int output_elf_segments(struct lar *lar, char *name, char *filebuf,
> 	int filelen, enum compalgo algo)
>
> +{
> int ret;
>    Elf32_Phdr *phdr;
> Elf32_Ehdr *ehdr;
> u32 entry;
> int i;
> int size;
> unsigned char *header;
> char ename[64];
> int headers;
> char *temp;
> enum compalgo thisalgo;
> u32 complen;
> /* Allocate a temporary buffer to compress into - this is unavoidable,
>   because we need to make sure that the compressed data will fit in
>   the LAR, and we won't know the size of the compressed data until
>   we actually compress it */
> temp = calloc(filelen, 1);
> if (temp == NULL) {
> err("Out of memory.\n");
> return -1;
> }
> /* validate elf header */
> ehdr = (Elf32_Ehdr *)filebuf;
> headers = ehdr->e_phnum;
> header  = (unsigned char *)ehdr;
> if (verbose())
> fprintf(stderr, "Type %d machine %d version %d entry %p phoff %d shoff %d flags %#x hsize %d phentsize %d phnum %d s_hentsize %d s_shnum %d \n", 
> ehdr->e_type,
> ehdr->e_machine,
> ehdr->e_version,
> (void *)ehdr->e_entry,
> ehdr->e_phoff,
> ehdr->e_shoff,
> ehdr->e_flags,
> ehdr->e_ehsize,
> ehdr->e_phentsize,
> ehdr->e_phnum,
> ehdr->e_shentsize,
> ehdr->e_shnum);
>    phdr = (Elf32_Phdr *)&(header[ehdr->e_phoff]);
> if (verbose())
> fprintf(stderr, "%s: header %p #headers %d\n", __FUNCTION__, ehdr, headers);
> entry = ehdr->e_entry;
> for(i = 0; i < headers; i++) {
> /* Ignore data that I don't need to handle */
> if (phdr[i].p_type != PT_LOAD) {
> 	if (verbose())
> 		fprintf(stderr, "Dropping non PT_LOAD segment\n");
> 	continue;
> }
> if (phdr[i].p_memsz == 0) {
> 	if (verbose())
> 		fprintf(stderr, "Dropping empty segment\n");
> 	continue;
> }
> thisalgo = algo;
> if (verbose())
> 	fprintf(stderr,  "New segment addr 0x%ulx size 0x%ulx offset 0x%ulx filesize 0x%ulx\n",
> 	(u32)phdr[i].p_paddr, (u32)phdr[i].p_memsz, 
> 	(u32)phdr[i].p_offset, (u32)phdr[i].p_filesz);
> /* Clean up the values */
> size = phdr[i].p_filesz;
> if (phdr[i].p_filesz > phdr[i].p_memsz)  {
> 	size = phdr[i].p_memsz;
> }
> if (verbose()) {
> 	fprintf(stderr, "(cleaned up) New segment addr %p size 0x%#x offset 0x%x\n",
> 		(void *)phdr[i].p_paddr, size, phdr[i].p_offset);
> 	fprintf(stderr, "Copy to %p from %p for %d bytes\n", 
> 		(unsigned char *)phdr[i].p_paddr, 
> 		&header[phdr[i].p_offset], size);
> 	fprintf(stderr, "entry %ux loadaddr %ux\n", 
> 		entry,  phdr[i].p_paddr);
> }
> 	/* ok, copy it out */
> sprintf(ename, "%s/segment%d", name, i);
> complen = lar_compress(filebuf, size, temp, &thisalgo);
> ret = lar_add_entry(lar, ename, &header[phdr[i].p_offset], 
> 		    complen, size, 
> 		    phdr[i].p_paddr, entry, thisalgo);
> }
> return 0;
>
> +out:
> return -1;
>
> +}
>
> +/**
> Given a size, return the offset of the bootblock (including the
> header)
> @param size Size of the LAR archive
>
> @@ -259,7 +379,7 @@
>    }
>
> return lar;
> err:
>
> +err:
>    lar_close_archive(lar);
>
> /* Don't leave a halfbaked LAR laying around */
> @@ -315,7 +435,7 @@
>
> return lar;
> err:
>
> +err:
>    lar_close_archive(lar);
>    return NULL;
>  }
> @@ -535,110 +655,139 @@
>  }
>
> /**
> Add a new file to the LAR archive
> @param lar The LAR archive to write into
> @param name The name of the file to add
> @return  0 on success, or -1 on failure
> Given a pathname in the form [option;]path, determine the file name,
> LAR path name, and compression algorithm.
> @param name name in the [option:][./]path form
> @param pfilename reference pointer to file name -- this is modified
> @param ppathname reference pointer to LAR path name -- this is modified
> @param thisalgo pointer to algorithm, which can be modified by path name
> @return 0 success, or -1 on failure (i.e. a bad name)
> */
>
> -int lar_add_file(struct lar *lar, const char *name)
> +int lar_process_name(char *name, char **pfilename, char **ppathname,
>      enum compalgo *thisalgo)
>
> {
> char *filename, *ptr, *temp;
> char *pathname;
> char *filename = name, *pathname = name;
> enum compalgo thisalgo;
> struct lar_header *header;
> u32 offset;
> int ret, fd, hlen;
> u32 complen;
> int pathlen;
> struct stat s;
> u32 *walk,  csum;
> /* Find the beginning of the available space in the LAR */
> offset = lar_empty_offset(lar);
> thisalgo = algo;
> filename = (char *) name;
> if (!strncmp(name, "nocompress:",11)) {
>   filename += 11;
> thisalgo = none;
> *thisalgo = none;
>
> }
>
> /* this is dangerous */
> if (filename[0] == '.' && filename[1] == '/')
> filename += 2;
>
> pathname = strchr(filename, ':');
>
> if (pathname != NULL) {
>  *pathname = '\0';
>  pathname++;
> *pathname = '\0';
> pathname++;
>
>  if (!strlen(pathname)) {
>    err("Invalid pathname specified.\n");
>    return -1;
>  }
> if (!strlen(pathname)) {
> 	err("Invalid pathname specified.\n");
> 	return -1;
> }
> }
> else
> pathname = filename;
> *pfilename = filename;
> *ppathname = pathname;
> return 0;
>
> +}
>
> +/**
> Given a pathname, open and mmap the file.
> @param filename
> @param size pointer to returned size
> @return ptr to mmap'ed area on success, or MAP_FAILED on failure
> */
>
> +char *mapfile(char *filename, u32 *size)
> +{
>
> int fd;
>
> struct stat s;
>
> char *ptr;
>
> /* Open the file */
>  fd = open(filename, O_RDONLY);
>
> if (fd == -1) {
>   err("Unable to open %s\n", filename);
> return -1;
> return MAP_FAILED;
>
> }
>
> if (fstat(fd, &s)) {
> err("Unable to stat the file %s\n", filename);
> close(fd);
> return -1;
> return MAP_FAILED;
> }
> /* Allocate a temporary buffer to compress into - this is unavoidable,
>   because we need to make sure that the compressed data will fit in
>   the LAR, and we won't know the size of the compressed data until
>   we actually compress it */
> temp = calloc(s.st_size, 1);
> if (temp == NULL) {
> err("Out of memory.\n");
> return -1;
> }
> ptr = mmap(0, s.st_size, PROT_READ, MAP_SHARED, fd, 0);
>
> close(fd);
>
> if (ptr == MAP_FAILED) {
> err("Unable to map %s\n", filename);
> close(fd);
> free(temp);
> return -1;
> return MAP_FAILED;
> }
> *size = s.st_size;
> return ptr;
>
> +}
>
> +/**
> Compress an area according to an algorithm. If the area grows,
> use no compression.
> @param ptr data to be compressed
> @param size size of the data
> @param temp destination of compressed data
> @param thisalgo pointer to algorithm -- this can be modified
> @return  size of compressed data
> */
>
> +u32 lar_compress(char *ptr, ssize_t size, char *temp, enum compalgo *thisalgo)
> +{
> u32 complen;
> compress_functions[*thisalgo](ptr, size, temp, &complen);
> /* Do the compression step */
> compress_functions[thisalgo](ptr, s.st_size, temp, &complen);
> if (complen >= s.st_size && (thisalgo != none)) {
> thisalgo = none;
> compress_functions[thisalgo](ptr, s.st_size, temp, &complen);
> if (complen >= size && (*thisalgo != none)) {
> *thisalgo = none;
> compress_functions[*thisalgo](ptr, size, temp, &complen);
> }
> return complen;
>
> +}
> munmap(ptr, s.st_size);
> close(fd);
>
> +/**
> Add a new entry to the LAR archive
> @param lar The LAR archive to write into
> @param pathname The name of the segment
> @param data The data for the segment
> @param complen The compressed length of the segment
> @param reallen The real (uncompressed) length of the segment
> @param loadaddress The load address of the segment
> @param entry The entry point of the segment
> @param thisalgo The compression algorithm
> @return  0 on success, or -1 on failure
> */
>
> +int lar_add_entry(struct lar *lar, char *pathname, void *data,
>   u32 complen, u32 reallen, u32 loadaddress, u32 entry, 
>   enum compalgo thisalgo)
>
> +{
> struct lar_header *header;
> int ret, hlen;
> int pathlen;
> u32 *walk,  csum;
> u32 offset;
> pathlen = strlen(pathname) + 1 > MAX_PATHLEN ? MAX_PATHLEN : strlen(pathname) + 1;
>
> /* Find the beginning of the available space in the LAR */
>
> offset = lar_empty_offset(lar);
>
> pathlen = strlen(pathname) + 1 > MAX_PATHLEN ?
> MAX_PATHLEN : strlen(pathname) + 1;
>
> /* Figure out how big our header will be */
>  hlen = sizeof(struct lar_header) + pathlen;
>  hlen = (hlen + 15) & 0xFFFFFFF0;
>
> if (offset + hlen + complen >= get_bootblock_offset(lar->size)) {
>   err("Not enough room in the LAR to add the file.\n");
> free(temp);
> return -1;
> }
>
> @@ -651,16 +800,17 @@
>
> memcpy(header, MAGIC, 8);
>    header->compression = htonl(thisalgo);
> header->reallen = htonl(s.st_size);
> header->reallen = htonl(reallen);
> header->len = htonl(complen);
> header->offset = htonl(hlen);
>
> header->loadaddress = htonl(loadaddress);
>
> header->entry = htonl(entry);
> /* Copy the path name */
> strncpy((char *) (lar->map + offset + sizeof(struct lar_header)),
> pathname, pathlen - 1);
>
> /* Copy in the data */
> memcpy(lar->map + (offset + hlen), temp, complen);
>
> memcpy(lar->map + (offset + hlen), data, complen);
>
> /* Figure out the checksum */
>
> @@ -671,7 +821,58 @@
>    	csum += ntohl(*walk);
>    }
>    header->checksum = htonl(csum);
> return 0;
>
> +}
>
> +/**
> Add a new file to the LAR archive
> @param lar The LAR archive to write into
> @param name The name of the file to add
> @return  0 on success, or -1 on failure
> */
>
> +int lar_add_file(struct lar *lar, char *name)
> +{
> char *filename, *ptr, *temp;
> char *pathname;
> enum compalgo thisalgo;
> struct lar_header *header;
> int ret, hlen;
> u32 complen;
> int pathlen;
> u32 size;
> thisalgo = algo;
> lar_process_name(name, &filename, &pathname, &thisalgo);
> ptr = mapfile(filename, &size);
> if (elfparse() && iself(ptr)) {
> output_elf_segments(lar, pathname, ptr, size, thisalgo);
> return 0;
> }
> /* This is legacy stuff. */
> /* Allocate a temporary buffer to compress into - this is unavoidable,
>   because we need to make sure that the compressed data will fit in
>   the LAR, and we won't know the size of the compressed data until
>   we actually compress it */
> temp = calloc(size, 1);
> if (temp == NULL) {
> err("Out of memory.\n");
> munmap(ptr, size);
> return -1;
> }
> complen = lar_compress(ptr, size, temp, &thisalgo);
> munmap(ptr, size);
> ret = lar_add_entry(lar, pathname, temp, complen, size, 0, 0, thisalgo);
> free(temp);
> return 0;
> return ret;
>
> }
> Index: util/lar/Makefile
> ===================================================================
> --- util/lar/Makefile	(revision 480)
> +++ util/lar/Makefile	(working copy)
> @@ -17,7 +17,6 @@
>  ## along with this program; if not, write to the Free Software
>  ## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
>  ##
>
> LAROBJ := lar.o stream.o lib.o
>
> LARDIR := lardir
> @@ -29,7 +28,6 @@
>  LARDIR     += nrv2bdir
>
> LAROBJ_ABS := $(patsubst %,$(obj)/util/lar/%,$(LAROBJ))
>
> lardir:
>    $(Q)printf "  BUILD   LAR\n"
>    $(Q)mkdir -p $(obj)/util/lar

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

[Stefan Reinauer]

* ron minnich  [070828 20:41]:
> p.s. I can not get the LARCHIVE problem to NOT happen now. So, until
> we fix it right, let's take my wrong fix. Three of us have tried and
> failed to fix this, so, let's assume it's not trivial.

NACK. I didn't even look into it yet. It won't become more trivial if we
start discussing this in 2 years, and we come up with "we don't remember
why we did it like that 2 years ago. There was some reason so we better
leave it as it is". This stuff really scared me in the past.

There is no such thing as a makeshift solution.

Please go on and commit your ELF-as-LAR stuff without this and the other
hack and keep them in your local tree so it works for you for now. I
will try to fix it as soon as I am through all of the GSoC evaluations.

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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [070828 16:59]:
> Can we change the MAGIC to some less likely string, perhaps even with
> version info? A lot of filesystems have mixed-case magic strings, for
> example ReiserFS has "ReIsErFs" or "ReIsEr2Fs" or "ReIsEr3Fs". Nobody is
> going to include such a string by accident in his code.

The chosen string LARCHIVE is already very unlikely. Any other string
would have been hit by Ron, since it was the lar handling code that
produced the string.

Instead of iterating over data, the code should iterate over the linked
list of archive headers. This would easily fix the problem.

Stefan

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

[Carl-Daniel Hailfinger]

On 28.08.2007 18:37, Stefan Reinauer wrote:
> Instead of iterating over data, the code should iterate over the linked
> list of archive headers. This would easily fix the problem.

It seems to do that (at least that's what the code looks like. Now we
just have to find out why iterating hits non-headers.

Regards,
Carl-Daniel

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

[Carl-Daniel Hailfinger]

On 27.08.2007 19:25, ron minnich wrote:
> Since we have a load address, we can create this lar entry:
>
> normal/cmdline
> and specify that it be loaded at a place where linux will find it as
> the cmdline.
>
> The change is simple. Add a load address and entry point to the lar
> header. Extend the lar tool to parse the elf file and create multiple
> lar segments. It looks like this:
>   normal/payload0 (33192 bytes, lzma compressed to 18088 bytes @0x38
> load @0x100000, entry 0x105258)
>   normal/payload1 (72 bytes, lzma compressed to 47 bytes @0x4718 load
> @0x1225a0, entry 0x105258)
>   normal/option_table (932 bytes @0x4798 load @0, entry 0)
>   normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
> load @0, entry 0)
>   normal/initram (4208 bytes @0x8828 load @0, entry 0)
>   linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)
>
> note that the payload is now payload0, payload1, etc. I've extended
> linuxbios to look for these. Note that you can now see all the things
> that get loaded ;they're no longer hidden in an ELF header somewhere.
> Elf failures are gone!

What about creating directories for each payload so we can differentiate
between multiple payloads (in the original sense)?

normal/payload0/segment0 (33192 bytes, lzma compressed to 18088 bytes
@0x38 load @0x100000, entry 0x105258)
  normal/payload0/segment1 (72 bytes, lzma compressed to 47 bytes
@0x4718 load @0x1225a0, entry 0x105258)
  normal/payload1/segment0 (xxx bytes, lzma compressed to yyy bytes
@0xfoo load @0xbar, entry 0xbaz)
  normal/option_table (932 bytes @0x4798 load @0, entry 0)
  normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
load @0, entry 0)
  normal/initram (4208 bytes @0x8828 load @0, entry 0)
  linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)

The normal/payload0/ directory would contain the ELF segments of the
first payload etc. The mapping between your proposal and mine would be:
Yours          <->      Mine
normal/payload0   normal/payload0/segment0
normal/payload1   normal/payload0/segment1
N/A               normal/payload1/segment0

What do you think?

Comments about the patch:
> --- include/lar.h	(revision 464)

This is against an old tree. lar has changed in between.
> +++ 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. */

Maybe also mention util/lar/README which is even less in sync.
> 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 we should introduce a major/minor version in the LAR header? Or a
different magic string for each revision?
Do we also need an alignment parameter in the header (like "free
placement, but please align to xx")?
> @@ -69,6 +76,8 @@
>    void *start;
>    int len;
>    u32 reallen;
> void * entry;
> void * loadaddress;

void *entry;
void *loadaddress;
> u32 compression;
>  };
>
> @@ -77,5 +86,6 @@
>  int copy_file(struct mem_file *archive, char *filename, void *where);
>  int run_file(struct mem_file *archive, char *filename, void *where);
>  int execute_in_place(struct mem_file *archive, char *filename);
>
> +int run_address(void *f);
> +void * load_file(struct mem_file *archive, char *filename);

void *load_file(...)
> #endif /* LAR_H */
> --- lib/lar.c	(revision 464)
> +++ 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);
> result->start = walk + ntohl(header->offset);
> result->len = ntohl(header->len);
> result->reallen = ntohl(header->reallen);
> result->compression = ntohl(header->compression);
> 	result->entry = (void *)ntohl(header->entry);
> 	result->loadaddress = (void *)ntohl(header->loadaddress);
> 	printk(BIOS_SPEW, "start %p len %d reallen %d compression %x entry %p loadaddress %p\n", 
> 		result->start, result->len, result->reallen, result->compression, result->entry, result->loadaddress);
> return 0;
> }
> // skip file
> walk += (ntohl(header->len) + ntohl(header->offset) -
> 	1) & 0xfffffff0;
> }
> printk(BIOS_SPEW, "NO FILE FOUND\n");
> return 1;
>
> }
>
> +void * load_file(struct mem_file *archive, char *filename)

void *load_file(...)
> +{
> int ret;
> struct mem_file result;
> void *where;
> void *entry;
> ret = find_file(archive, filename, &result);
> if (ret) {
> printk(BIOS_INFO, "LAR: load_file: No such file '%s'\n",
>        filename);
> return (void *)-1;
> }
> entry = result.entry;
> where = result.loadaddress;
> printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", result.compression);
> /* no compression */
> if (result.compression == 0) {
> memcpy(where, result.start, result.len);
> return entry;
> }
>
> +#ifdef CONFIG_COMPRESSION_LZMA
> /* lzma */
> unsigned long ulzma(unsigned char * src, unsigned char * dst);

dito.
> if (result.compression == 1) {
> ulzma(result.start, where);
> return entry;
> }
>
> +#endif
> +#ifdef CONFIG_COMPRESSION_NRV2B
> /* nrv2b */
> unsigned long unrv2b(u8 * src, u8 * dst, unsigned long *ilen_p);

dito.
> if (result.compression == 2) {
> int tmp;
> unrv2b(result.start, where, &tmp);
> return entry;
> }
>
> +#endif
> printk(BIOS_INFO, "LAR: Compression algorithm #%i not supported!\n", result.compression);
> return (void *)-1;
>
> +}
> --- mainboard/emulation/qemu-x86/initram.c	(revision 464)
> +++ 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");
>
> return 0;
>
> }

My eyes!
> --- mainboard/emulation/qemu-x86/Makefile	(revision 464)
> +++ mainboard/emulation/qemu-x86/Makefile	(working copy)
> @@ -41,14 +41,17 @@
>  #
>
> INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> +$(obj)/mainboard/$(MAINBOARDDIR)/initram.o: $(src)/mainboard/$(MAINBOARDDIR)/initram.c
> cc -c $(INITCFLAGS)  -fPIE $(src)/mainboard/$(MAINBOARDDIR)/initram.c -o $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
>
> # These are possibly not permanent
> -INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
> +#INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
>
> $(obj)/linuxbios.initram: $(obj)/stage0.init $(obj)/stage0.o $(INITRAM_OBJ)
>    $(Q)# initram links against stage0
>    $(Q)printf "  LD      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(LD)  -Ttext 0x80000 $(INITRAM_OBJ) \
> --entry=main -o $(obj)/linuxbios.initram.o
> $(Q)$(LD)  $(INITRAM_OBJ) \
> --entry=main -R $(obj)/stage0.o -o $(obj)/linuxbios.initram.o
> $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) -O binary $(obj)/linuxbios.initram.o \
> $(obj)/linuxbios.initram

Could you explain that one?
> Index: arch/x86/stage1.c
>
> --- arch/x86/stage1.c	(revision 464)
> +++ arch/x86/stage1.c	(working copy)
> @@ -144,20 +168,32 @@
>    printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> ret = find_file(&archive, "normal/payload", &result);
> if (ret) {
> printk(BIOS_ERR, "No such file '%s'.\n", "normal/payload");
> die("FATAL: No payload found.\n");
> if (! ret)
> legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
> /* new style lar boot. Install all the files in memory.
>  * By convention we take the entry point from the first 

Needs real documentation (LAR README maybe?), not just a code comment.
>  * one. Look for a cmdline as well. 

dito.
>  */
> 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);
> newentry = load_file(&archive, filename);
> printk("newentry is %p\n", newentry);
> if (newentry == (void *)-1)
> 	break;
> if (! entry)
> 	entry = newentry;
> }
> ret = copy_file(&archive, "normal/payload", (void *)UNCOMPRESS_AREA);
> if (ret) {
> printk(BIOS_ERR, "'%s' found, but could not load it.\n", "normal/payload");
> die("FATAL: No usable payload found.\n");
> }
> printk(BIOS_SPEW, "all loaded, entry %p\n", entry);
> run_address(entry);
> ret =  elfboot_mem(mem, (void *)UNCOMPRESS_AREA, result.reallen);
> die("FATAL: No usable payload found.\n");
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> die ("FATAL: Last stage returned to LinuxBIOS.\n");
>
> }
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 464)
> +++ arch/x86/Makefile	(working copy)
> @@ -22,7 +22,7 @@
>  ifeq ($(CONFIG_ARCH_X86),y)
>
> INITCFLAGS := $(CFLAGS) -I$(src)/include/arch/x86 -I$(src)/include \
> -I$(obj) -fno-builtin
> -I$(obj) -fno-builtin  

Superfluous whitespace introduction.
> SILENT := >/dev/null 2>&1
>
> @@ -77,7 +77,7 @@
>    fi
>  endif
>    $(Q)printf "  LAR     $(subst $(shell pwd)/,,$(@))\n"
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(COMPRESSFLAG) -c \
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar -p $(COMPRESSFLAG) -c \

lar -p option is not mentioned in the lar help message.
> ../linuxbios.rom \
> $(LARFILES) \
> -s $(ROM_SIZE) -b $(obj)/linuxbios.bootblock
>
> @@ -142,6 +142,9 @@
>    $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)$(OBJCOPY) -O binary $(obj)/stage0.o $(obj)/stage0.init
> $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o $(obj)/stage0_link.o
> $(Q)printf "  TEST    $(subst $(shell pwd)/,,$(@))\n"
>  $(Q)test `wc -c < $(obj)/stage0.init` -gt 16128 && \
>   printf "Error. Bootblock got too big.\n" || true

Please explain.
> --- util/lar/lar.c	(revision 464)

old revision, you will have to make sure the merge won't kill the
changes since 464.
> +++ util/lar/lar.c	(working copy)
> @@ -37,6 +37,7 @@
>  #define COPYRIGHT "Copyright (C) 2006-2007 coresystems GmbH"
>
> static int isverbose = 0;
> +static int isparseelf = 0;
>  static long larsize = 0;
>  static char *bootblock = NULL;
>  enum compalgo algo = none;
> @@ -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);

ecxl is missing a few options, among them p.
> }
>
> +int parseelf(void)
> +{
> return isparseelf;
>
> +}
>
> int verbose(void)
>  {
>    return isverbose;
> @@ -78,6 +84,7 @@
>    	{"list", 0, 0, 'l'},
>    	{"size", 1, 0, 's'},
>    	{"bootblock", 1, 0, 'b'},
> {"parseelf", 1, 0, 'p'},
> {"verbose", 0, 0, 'v'},
> {"version", 0, 0, 'V'},
> {"help", 0, 0, 'h'},
>
> @@ -89,7 +96,7 @@
>    	exit(1);
>    }
> while ((opt = getopt_long(argc, argv, "cC:xls:b:vVh?",
> while ((opt = getopt_long(argc, argv, "cC:xlps:b:vVh?",
> 		  long_options, &option_index)) != EOF) {
> switch (opt) {
> case 'c':
>
> @@ -106,6 +113,9 @@
>    	case 'l':
>    		larmode = LIST;
>    		break;
> case 'p':
> 	isparseelf = 1;
> 	break;
> case 'x':
> 	larmode = EXTRACT;
> 	break;
> --- util/lar/create.c	(revision 464)
> +++ util/lar/create.c	(working copy)
> @@ -49,6 +49,201 @@
>    out_len[0] = in_len;
>  }
>
> +char *readfile(char *name, int *plen)
> +{
> int ret, filelen;
> FILE *source;
> char *filebuf;
> struct stat statbuf;
> *plen = -1;
> ret = stat(name, &statbuf);
> if (ret) {
> fprintf(stderr, "No such file %s\n", name);
> exit(1);
> }
> filelen = statbuf.st_size;
> /* Read file into memory. */
> filebuf = malloc(filelen);
> source = fopen(name, "r");
> if (!source) {
> fprintf(stderr, "No such file %s\n", name);
> exit(1);
> }
> fread(filebuf, filelen, 1, source);
> fclose(source);
> *plen = filelen;
> return filebuf;
>
> +}
>
> +/* given an output file name, with data pointed to by filebuf, of size filelen,
> output it to the lar define by FILE *lar
> */
>
> +int
> +outputfile(char *name, char *filebuf, int filelen, enum compalgo algo, FILE *archive, u32 entry, u32 loadaddress)

int outputfile(...
> +{
> char *pathname;
> int i, ret;
> int diff = 0;
> int bb_header_len = 0;
> FILE *source;
> char *filetarget;
> u32 *walk;
> u32 csum;
> int pathlen, entrylen;
> u32 compfilelen;
> long currentsize = 0;
> struct lar_header *header;
> enum compalgo thisalgo;
> char *tempmem;
> thisalgo = algo;
> int tempsize = sizeof(struct lar_header) + MAX_PATHLEN + filelen + 16;
> tempmem = malloc(tempsize);
> if (!tempmem) {
> 	fprintf(stderr, "Out of memory.\n");
> 	return (-1);
> }
> memset(tempmem, 0, tempsize);
> header = (struct lar_header *)tempmem;
> pathname = tempmem + sizeof(struct lar_header);
> pathlen = snprintf(pathname, MAX_PATHLEN - 1, name) + 1;
> pathlen = (pathlen + 15) & 0xfffffff0;	/* Align to 16 bytes. */
> filetarget = pathname + pathlen;
> compress_functions[thisalgo](filebuf, filelen, filetarget,
> 			     &compfilelen);
> if ((compfilelen >= filelen) && (thisalgo != none)) {
> 	thisalgo = none;
> 	compress_functions[thisalgo](filebuf, filelen,
> 				     filetarget, &compfilelen);
> }
> /* Create correct header. */
> memcpy(header, MAGIC, 8);
> header->compression = htonl(thisalgo);
> header->entry = htonl(entry);
> header->loadaddress = htonl(loadaddress);
> header->reallen = htonl(filelen);
> header->len = htonl(compfilelen);
> header->offset = htonl(sizeof(struct lar_header) + pathlen);
> /* Calculate checksum. */
> csum = 0;
> for (walk = (u32 *) tempmem;
>      walk < (u32 *) (tempmem + compfilelen +
> 		     sizeof(struct lar_header) + pathlen);
>      walk++) {
> 	csum += ntohl(*walk);
> }
> header->checksum = htonl(csum);
> /* Write out entry to archive. */
> entrylen = (compfilelen + pathlen + sizeof(struct lar_header) +
> 	    15) & 0xfffffff0;
> fwrite(tempmem, entrylen, 1, archive);
> free(tempmem);
> return entrylen;
>
> +}
> [...]
>
> +int
> +outputelf(char *name, char *filebuf, int filelen, enum compalgo algo, FILE *archive)

dito.
> +{
> int index;
> --- util/lar/Makefile	(revision 464)
> +++ util/lar/Makefile	(working copy)
> @@ -33,14 +34,15 @@
>  endif
>
> LAROBJ_ABS := $(patsubst %,$(obj)/util/lar/%,$(LAROBJ))
>
> +HOSTCFLAGS+=-g
> +HOSTCXXFLAGS+=-g
>  lardir:
>    $(Q)printf "  BUILD   LAR\n"
>    $(Q)mkdir -p $(obj)/util/lar
>
> $(obj)/util/lar/lar: $(LARDIR) $(LAROBJ_ABS) $(COMPRESSOR)
>    $(Q)printf "  HOSTCXX $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(HOSTCXX) $(HOSTCXXFLAGS) -o $@ $(LAROBJ_ABS) $(COMPRESSOR)
> $(Q)$(HOSTCXX) $(HOSTCXXFLAGS)  -o $@ $(LAROBJ_ABS) $(COMPRESSOR)

Superfluous whitespace.
> $(obj)/util/lar/%.o: $(src)/util/lar/%.c
>    $(Q)printf "  HOSTCC  $(subst $(shell pwd)/,,$(@))\n"

Regards,
Carl-Daniel

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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [070828 01:54]:
> What about creating directories for each payload so we can differentiate
> between multiple payloads (in the original sense)?

Yes, I would prefer something like that, too
> normal/payload0/segment0 (33192 bytes, lzma compressed to 18088 bytes @0x38 load @0x100000, entry 0x105258)
>   normal/payload0/segment1 (72 bytes, lzma compressed to 47 bytes @0x4718 load @0x1225a0, entry 0x105258)
>   normal/payload1/segment0 (xxx bytes, lzma compressed to yyy bytes @0xfoo load @0xbar, entry 0xbaz)

Sounds reasonable and allows use of Uwe's multiple payloads in the same
breath.
> Maybe we should introduce a major/minor version in the LAR header? Or a
> different magic string for each revision?

I generally hate versions. Think features, not numbers. But since this
is probably not doable without significant overhead, I'd just go with a
normal version header that gets increased every time we change the
format. No need for major, minor, subminor, ...
> > void *start;
> >    int len;
> >    u32 reallen;
> > void * entry;
> > void * loadaddress;
>
> void *entry;
> void *loadaddress;

Not sure how these pointers sneaked in here. They do break portability
and cross compilability. Compiling LinuxBIOS on a 64bit host without
compiling lar 32bit is not possible with such things in the header.

I really don't like this.
> > #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");
> >
> > return 0;
> >
> > }
>
> My eyes!

:-) This is not supposed to go into the repo like that, but it fixes an
issue.

What was that linker auto rename trick that Marc mentioned recently?
> > INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> > +$(obj)/mainboard/$(MAINBOARDDIR)/initram.o: $(src)/mainboard/$(MAINBOARDDIR)/initram.c
> > cc -c $(INITCFLAGS)  -fPIE $(src)/mainboard/$(MAINBOARDDIR)/initram.c -o $(obj)/mainboard/$(MAINBOARDDIR)/initram.o

Rules per file are not exactly nice. Especially not when we start doing
non-trivial initram sequences.

Let's do this instead:

INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/PICOBJ/initram.o

and have a rule that handles all PICOBJ objects differently?
> > $(Q)$(LD)  -Ttext 0x80000 $(INITRAM_OBJ) \
> > --entry=main -o $(obj)/linuxbios.initram.o
> > $(Q)$(LD)  $(INITRAM_OBJ) \
> > --entry=main -R $(obj)/stage0.o -o $(obj)/linuxbios.initram.o
> > $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(OBJCOPY) -O binary $(obj)/linuxbios.initram.o \
> > $(obj)/linuxbios.initram
>
> Could you explain that one?

Initram was changed to link statically against printf a while ago. For
some reason this does not work because it craps out when jumping to the
serial_tx_byte (?) function.

Instead we decided the code has to become position independent. This
brings in another layer of problems we have to solve, like printk can't
just be printk like that. And linking the code at 0x80000 as we did
before creates a 4G file.

Stefan

[ron minnich]

On 8/28/07, Stefan Reinauer  wrote:
> Carl-Daniel Hailfinger  [070828 01:54]:
> > What about creating directories for each payload so we can differentiate
> > between multiple payloads (in the original sense)?
>
> Yes, I would prefer something like that, too

done.
normal/payload/segment0
normal/payload/segment1
etc.
> > Maybe we should introduce a major/minor version in the LAR header? Or a
> > different magic string for each revision?
>
> I generally hate versions. Think features, not numbers. But since this
> is probably not doable without significant overhead, I'd just go with a
> normal version header that gets increased every time we change the
> format. No need for major, minor, subminor, ...

but then you have to handle the versioning, and figure out what to do
if it does not match, and .... Let's not do this.
> > > void *start;
> > > int len;
> > > u32 reallen;
> > > void * entry;
> > > void * loadaddress;
> >
> > void *entry;
> > void *loadaddress;
>
> Not sure how these pointers sneaked in here. They do break portability
> and cross compilability. Compiling LinuxBIOS on a 64bit host without
> compiling lar 32bit is not possible with such things in the header.

I think you guys have missed the point. These are in the mem_file
struct, and that struct is by definition a per-machine struct. There
is NO cross-compatibility issue with mem_file -- see include/lar.h.
mem_file is not even defined in util/lar/lar.h. The lar utility
doesn't know what it is.
> What was that linker auto rename trick that Marc mentioned recently?

The naming is not the issue. The issue is getting gcc to do abs calls,
not relative calls, for some symbols.
> Let's do this instead:
>
> INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/PICOBJ/initram.o
>
> and have a rule that handles all PICOBJ objects differently?

As long as you agree to write it, I don't know how :-)

Or why not just PICFLAGS for pic objects?

New patch attached with new payload naming. But I am not changing the
void * you mentioned, because I don't think it's a problem (there was
already another void * in the mem_file struct before I got there!)

thanks

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 17:26, ron minnich wrote:
> On 8/28/07, Stefan Reinauer  wrote:
> > Carl-Daniel Hailfinger  [070828 01:54]:
> > > What about creating directories for each payload so we can differentiate
> > > between multiple payloads (in the original sense)?
> >
> > Yes, I would prefer something like that, too
>
> done.
> normal/payload/segment0
> normal/payload/segment1
> etc.

OK, so I was unclear. The idea was to number the payloads as well, not
only the segments.
> > > Maybe we should introduce a major/minor version in the LAR header? Or a
> > > different magic string for each revision?
> >
> > I generally hate versions. Think features, not numbers. But since this
> > is probably not doable without significant overhead, I'd just go with a
> > normal version header that gets increased every time we change the
> > format. No need for major, minor, subminor, ...
>
> but then you have to handle the versioning, and figure out what to do
> if it does not match, and .... Let's not do this.

We simply could reserve some space at the end of the header and use that
for future extensions without having to change a version number. Only if
the extension needs changes in earlier fields of the struct we would
have to change the MAGIC.
> > What was that linker auto rename trick that Marc mentioned recently?
>
> The naming is not the issue. The issue is getting gcc to do abs calls,
> not relative calls, for some symbols.

OK, but please include your explanation as code comment.
> This patch is revised based on comments.
>
> It also includes a demo of how to do a PIC initram, and:
>
> EMERGENCY PATCH! Please see patch to lib/lar.c and include/lar.h for the 
> MAGIC constant. This fixes a bug I hit just now.
>
> This patch also includes an EXPERT option for enabling no-ELF mode. 
> The system will default to old behaviour. See Kconfig in the root.
>
> I still wish to kill ELF mode very soon, however.
>
> LAR is a very capable format. With two simple extensions, we can use
> LAR to replace all that we are using ELF for now. This change can
> really make life better:
> we can use streaming decompress instead of the current "uncompress
>
> elf to memory and then copy segments" approach. So we can get rid of
> THIS hardcode:
> #define UNCOMPRESS_AREA (0x400000)
> 2. A simple lar l can show ALL segments, including payload segments
> 3. It's really easy to see where things will go in memory, and catch problems
> 4. We can figure out an ELF input file is bogus BEFORE we flash, not
> AFTER we flash and try to boot it
> 5. did I mention streaming decompress?
> 6. We no longer have to worry about where we decompress the elf in
> memory (this problem was causing trouble when the payload was a linux
> kernel -- it was so big)
> 7. Since we have a load address, we can create this lar entry:
> normal/cmdline
> and specify that it be loaded at a place where linux will find it as
> the cmdline.
> 8. The decision on whether to XIP can be made in the LAR entry, not in
> hardcode. For example, if initram needs to be XIP, set the load
> address to 0xffffffff. Done.
>
> The change is simple. Add a load address and entry point to the lar
> header. Extend the lar tool to parse the elf file and create multiple
> lar segments. It looks like this:
>  normal/payload/segment0 (33192 bytes, lzma compressed to 18088 bytes @0x38
> load @0x100000, entry 0x105258)
>  normal/payload/segment1 (72 bytes, lzma compressed to 47 bytes @0x4718 load
> @0x1225a0, entry 0x105258)
>  normal/option_table (932 bytes @0x4798 load @0, entry 0)
>  normal/stage2 (33308 bytes, lzma compressed to 15474 bytes @0x4b78
> load @0, entry 0)
>  normal/initram (4208 bytes @0x8828 load @0, entry 0)
>  linuxbios.bootblock (16384 bytes @0xfc000 load @0, entry 0)
>
> note that the payload is now payload0, payload1, etc. I've extended
> linuxbios to look for these. Note that you can now see all the things
> that get loaded ;they're no longer hidden in an ELF header somewhere.
> Elf failures are gone!

Please update the comment above.
> Note that I've left legacy elf support in, for now, but recommend we
> get rid of it as soon as possible.
>
> patch attached. This is a first pass. lar.c needs some refactoring but
> I want to get the cmdline going. You can now have a linux payload and
> it will uncompress with no problems.
>
> This has been tested with filo and BOCHS.
>
> Signed-off-by: Ronald G. Minnich 
> Index: Kconfig
> ===================================================================
> --- Kconfig	(revision 480)
> +++ Kconfig	(working copy)
> @@ -53,6 +53,25 @@
>    help
>      Append an extra string to the end of the LinuxBIOS version.
>
> +config NOELF
> bool "Don't use ELF for payloads"
> depends EXPERT
> default n
> help
>  Until now, LinuxBIOS has used elf for the payload. There are many problems 
>  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, LinuxBIOS will direct lar to break each elf segment into a LAR 
>  entry. ELF will not be used at all. Note that (for now) LinuxBIOS is backward
>  compatible -- if you put an ELF payload in, LinuxBIOS can still parse it. We hope
>  to remove ELF entirely in the future. 
>
> config BEEPS
>    bool "Enable beeps upon certain LinuxBIOS events"
>    depends EXPERT
> Index: include/lar.h
> ===================================================================
> --- include/lar.h	(revision 480)
> +++ include/lar.h	(working copy)
> @@ -52,9 +52,10 @@
>
> #include <types.h>
>
> -#define MAGIC "LARCHIVE"
> +/* see note in lib/lar.c as to why this is ARCHIVE and not LARCHIVE */
> +#define MAGIC "ARCHIVE"
>  #define MAX_PATHLEN 1024
>
> +/* NOTE -- This and the user-mode lar.h are NOT IN SYNC. Be careful. */
>  struct lar_header {
>    char magic[8];
>    u32 len;
> @@ -62,7 +63,14 @@
>    u32 checksum;
>    u32 compchecksum;
>    u32 offset;
> /* Compression:
> * 0 = no compression
> * 1 = lzma
> * 2 = nrv2b
> */
> u32 compression;
> u32 entry;		/* we might need to make this u64 */
> u32 loadaddress; /* ditto */
>
> };
>
> struct mem_file {
> @@ -70,6 +78,8 @@
>    int len;
>    u32 reallen;
>    u32 compression;
> void * entry;
> void * loadaddress;

Cosmetic (no space between * and name):
 void *entry;
 void *loadaddress;
> };
>
> /* Prototypes. */
> @@ -77,5 +87,6 @@
>  int copy_file(struct mem_file *archive, char *filename, void *where);
>  int run_file(struct mem_file *archive, char *filename, void *where);
>  int execute_in_place(struct mem_file *archive, char *filename);
>
> +int run_address(void *f);
> +void * load_file(struct mem_file *archive, char *filename);

Same cosmetic.
> #endif /* LAR_H */
> Index: mainboard/emulation/qemu-x86/initram.c
> ===================================================================
> --- mainboard/emulation/qemu-x86/initram.c	(revision 480)
> +++ mainboard/emulation/qemu-x86/initram.c	(working copy)
> @@ -19,10 +19,12 @@
>
> #include <console.h>
>
> +int (*pk)(int msg_level, const char *fmt, ...) = printk;

Can you add your explanation from above?

/* The issue is getting gcc to do abs calls, not relative calls, for
some symbols. */
> 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");
>
> return 0;
>
> }
> Index: mainboard/emulation/qemu-x86/Makefile
> ===================================================================
> --- mainboard/emulation/qemu-x86/Makefile	(revision 480)
> +++ mainboard/emulation/qemu-x86/Makefile	(working copy)
> @@ -41,14 +41,17 @@
>  #
>
> INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> +$(obj)/mainboard/$(MAINBOARDDIR)/initram.o: $(src)/mainboard/$(MAINBOARDDIR)/initram.c
> cc -c $(INITCFLAGS)  -fPIE $(src)/mainboard/$(MAINBOARDDIR)/initram.c -o $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
>
> # These are possibly not permanent
> -INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
> +#INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
>
> $(obj)/linuxbios.initram: $(obj)/stage0.init $(obj)/stage0.o $(INITRAM_OBJ)
>    $(Q)# initram links against stage0
>    $(Q)printf "  LD      $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(LD)  -Ttext 0x80000 $(INITRAM_OBJ) \
> --entry=main -o $(obj)/linuxbios.initram.o
> $(Q)$(LD)  $(INITRAM_OBJ) \
> --entry=main -R $(obj)/stage0.o -o $(obj)/linuxbios.initram.o
> $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) -O binary $(obj)/linuxbios.initram.o \
> $(obj)/linuxbios.initram
> Index: lib/lzmadecode.c
>
> --- lib/lzmadecode.c	(revision 480)
> +++ lib/lzmadecode.c	(working copy)
> @@ -206,7 +206,6 @@
>          RC_GET_BIT(probLit, symbol)
>        }
>        previousByte = (Byte)symbol;

Empty line removal. Was this intentional?
>    outStream[nowPos++] = previousByte;
>    if (state < 4) state = 0;
>    else if (state < 10) state -= 3;
> Index: lib/lar.c
>
> --- lib/lar.c	(revision 480)
> +++ lib/lar.c	(working copy)
> @@ -31,6 +31,13 @@
>  #define ntohl(x) (x)
>  #endif
>
> +int run_address(void *f)
> +{
> int (*v) (void);
> v = f;
> return v();
>
> +}
>
> int find_file(struct mem_file *archive, char *filename, struct mem_file *result)
>  {
>    char *walk, *fullname;
> @@ -42,9 +49,31 @@
>
> for (walk = archive->start;
>         (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> if (strcmp(walk, MAGIC) != 0)
> /* I am leaving this code in here because it is so dangerous. MAGIC is
>  * #define'd to a string. That string lives in data space. All of the 1M linuxbios 
>  * image is a LAR file. Therefore, this search can walk ALL of linuxbios. 
>  * IF the MAGIC string (in code space) just happens to be 16-byte aligned, 
>  * Then the strcmp will succeed, and you will match a non-LAR entry, 
>  * and you are screwed. can this happen? YES!
>  * LAR: Attempting to open 'fallback/initram'.
>  * LAR: Start 0xfff00000 len 0x100000
>  * LAR: current filename is normal/payload
>  * LAR: current filename is normal/option_table
>  * LAR: current filename is normal/stage2
>  * LAR: current filename is normal/initram
>  * LAR: current filename is R: it matches %s @ %p
>  * That garbage is there because the pointer is in the middle of a bunch 
>  * of non-null-terminated junk. The fix is easy, as you can see. 
>  * if (strcmp(walk, MAGIC) != 0)
>  *	continue;
>  * And, yes, this did indeed fix the problem!
>  */
> if (walk[0] != 'L')
> continue;
>
> if (strcmp(&walk[1], MAGIC) != 0)
> 	continue;

There has to be a cleaner trick. Just in case somebody has the expected
MAGIC string in an uncompressed lar file somewhere (other sources of
that are possible!), we still fail, but less often.
> header = (struct lar_header *)walk;
> fullname = walk + sizeof(struct lar_header);
>
> @@ -52,19 +81,69 @@
>    	// FIXME: check checksum
> if (strcmp(fullname, filename) == 0) {
> 	printk(BIOS_SPEW, "LAR: it matches %s @ %p\n", fullname, header);

replace "it" with "filename"?
> 	result->start = walk + ntohl(header->offset);
> 	result->len = ntohl(header->len);
> 	result->reallen = ntohl(header->reallen);
> 	result->compression = ntohl(header->compression);
> 	result->entry = (void *)ntohl(header->entry);
> 	result->loadaddress = (void *)ntohl(header->loadaddress);
> 	printk(BIOS_SPEW, "start %p len %d reallen %d compression %x entry %p loadaddress %p\n", 
> 		result->start, result->len, result->reallen, result->compression, result->entry, result->loadaddress);

Any chance to break that into multiple lines?
> 	return 0;
> }
> // skip file
> walk += (ntohl(header->len) + ntohl(header->offset) -
> 	1) & 0xfffffff0;
>
> }
> printk(BIOS_SPEW, "NO FILE FOUND\n");
> return 1;
>
> }
>
> +void * load_file(struct mem_file *archive, char *filename)

cosmetic: void *load_file(..)
> +{
> int ret;
> struct mem_file result;
> void *where;
> void *entry;
> ret = find_file(archive, filename, &result);
> if (ret) {
> printk(BIOS_INFO, "LAR: load_file: No such file '%s'\n",
>        filename);
> return (void *)-1;
> }
> entry = result.entry;
> where = result.loadaddress;
> printk(BIOS_SPEW, "LAR: Compression algorithm #%i used\n", result.compression);
> /* no compression */
> if (result.compression == 0) {
> memcpy(where, result.start, result.len);
> return entry;
> }
>
> +#ifdef CONFIG_COMPRESSION_LZMA
> /* lzma */
> unsigned long ulzma(unsigned char * src, unsigned char * dst);

cosmetic:
+	unsigned long ulzma(unsigned char *src, unsigned char *dst);
> if (result.compression == 1) {
> ulzma(result.start, where);
> return entry;
> }
>
> +#endif
> +#ifdef CONFIG_COMPRESSION_NRV2B
> /* nrv2b */
> unsigned long unrv2b(u8 * src, u8 * dst, unsigned long *ilen_p);

Cosmetic:
+	unsigned long unrv2b(u8 *src, u8 *dst, unsigned long *ilen_p);
> if (result.compression == 2) {
> int tmp;
> unrv2b(result.start, where, &tmp);
> return entry;
> }
>
> +#endif
> printk(BIOS_INFO, "LAR: Compression algorithm #%i not supported!\n", result.compression);
> return (void *)-1;
>
> +}
>
> +/* FIXME -- most of copy_file should be replaced by load_file */
>  int copy_file(struct mem_file *archive, char *filename, void *where)
>  {
>    int ret;
> @@ -113,6 +192,7 @@
>  {
>    int (*v) (void);
>    struct mem_file result;
>
> int ret;
>
> if ((u32) where != 0xFFFFFFFF) {
> if (copy_file(archive, filename, where)) {
>
> @@ -130,9 +210,11 @@
>    	}
>    	where = result.start;
>    }
> printk(BIOS_SPEW, "where is %p\n", where);
> v = where;
> return v();
> ret =  v();
> printk(BIOS_SPEW, "run_file returns with %d\n", ret);
> return ret;
>
> }
>
> /**
> Index: lib/stage2.c
> ===================================================================
> --- lib/stage2.c	(revision 480)
> +++ lib/stage2.c	(working copy)
> @@ -99,5 +99,6 @@
>    write_tables();
>    show_all_devs();
> printk(BIOS_SPEW, "STAGE2 NOW RETURNING\n");
> return 0;
>
> }
> Index: arch/x86/stage1.c
> ===================================================================
> --- arch/x86/stage1.c	(revision 480)
> +++ arch/x86/stage1.c	(working copy)
> @@ -22,12 +22,16 @@
>  #include <io.h>
>  #include <console.h>
>  #include <lar.h>
> +#include <string.h>
>  #include <tables.h>
>  #include <lib.h>
>  #include <mc146818rtc.h>
>  #include <post_code.h>
>
> -#define UNCOMPRESS_AREA 0x60000
> +/* ah, well, what a mess! This is a hard code. FIX ME but how?
> By getting rid of ELF ...
> */
>
> +#define UNCOMPRESS_AREA (0x400000)
>
> /* these prototypes should go into headers */
>  void uart_init(void);
> @@ -48,6 +52,24 @@
>    post_code(0xf2);
>  }
>
> +/* until we get rid of elf */
> +int legacy(struct mem_file *archive, char *name,  void *where, struct lb_memory *mem)

cosmetic whitespace:
+int legacy(struct mem_file *archive, char *name, void *where, struct
lb_memory *mem)
> +{
> int ret;
> struct mem_file result;
> 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, result.reallen);
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> return -1;
>
> +}
>
> /*
> This function is called from assembler code whith its argument on the
> stack. Force the compiler to generate always correct code for this case.
>
> @@ -57,6 +79,8 @@
>    int ret;
>    struct mem_file archive, result;
>    int elfboot_mem(struct lb_memory *mem, void *where, int size);
>
> void *entry;
>
> int i;
>
> /* we can't statically init this hack. */
> unsigned char faker[64];
>
> @@ -144,20 +168,32 @@
>    printk(BIOS_DEBUG, "Stage2 code done.\n");
>
> ret = find_file(&archive, "normal/payload", &result);
> if (ret) {
> printk(BIOS_ERR, "No such file '%s'.\n", "normal/payload");
> die("FATAL: No payload found.\n");
> if (! ret)
> legacy(&archive, "normal/payload", (void *)UNCOMPRESS_AREA, mem);
> /* new style lar boot. Install all the files in memory.
>  * By convention we take the entry point from the first 
>  * one. Look for a cmdline as well. 
>  */
> for(i = 0, entry = (void *)0; ;i++) {
> char filename[64];
> void *newentry;
> sprintf(filename, "normal/payload/segment%d", i);
> archive.len = *(u32 *)0xfffffff4;
> archive.start =(void *)(0UL-archive.len);
> newentry = load_file(&archive, filename);
> printk("newentry is %p\n", newentry);
> if (newentry == (void *)-1)
> 	break;
> if (! entry)
> 	entry = newentry;
> }
> ret = copy_file(&archive, "normal/payload", (void *)UNCOMPRESS_AREA);
> if (ret) {
> printk(BIOS_ERR, "'%s' found, but could not load it.\n", "normal/payload");
> die("FATAL: No usable payload found.\n");
> }
> printk(BIOS_SPEW, "all loaded, entry %p\n", entry);
> run_address(entry);
> ret =  elfboot_mem(mem, (void *)UNCOMPRESS_AREA, result.reallen);
> die("FATAL: No usable payload found.\n");
> printk(BIOS_ERR, "elfboot_mem returns %d\n", ret);
> die ("FATAL: Last stage returned to LinuxBIOS.\n");
>
> }
> Index: arch/x86/Makefile
>
> --- arch/x86/Makefile	(revision 480)
> +++ arch/x86/Makefile	(working copy)
> @@ -22,7 +22,7 @@
>  ifeq ($(CONFIG_ARCH_X86),y)
>
> INITCFLAGS := $(CFLAGS) -I$(src)/include/arch/x86 -I$(src)/include \
> -I$(obj) -fno-builtin
> -I$(obj) -fno-builtin
>
> SILENT := >/dev/null 2>&1
>
> @@ -78,7 +78,7 @@
>  endif
>    $(Q)printf "  LAR     $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)rm -f $(obj)/linuxbios.rom
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(COMPRESSFLAG) -c \
> $(Q)cd $(obj)/lar.tmp && ../util/lar/lar $(PARSEELF) $(COMPRESSFLAG) -c \
> ../linuxbios.rom \
> $(LARFILES) \
> -s $(ROM_SIZE) -b $(obj)/linuxbios.bootblock
>
> @@ -122,6 +122,11 @@
>  endif
>  endif
>
> +ifeq ($(CONFIG_NOELF), y)
> PARSEELF = -e
>
> +else
> PARSEELF =
>
> +endif
>
> STAGE0_OBJ := $(patsubst %,$(obj)/lib/%,$(STAGE0_LIB_OBJ)) \
>          $(patsubst %,$(obj)/arch/x86/%,$(STAGE0_ARCH_X86_OBJ)) \
> @@ -143,6 +148,9 @@
>    $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
>    $(Q)$(OBJCOPY) -O binary $(obj)/stage0.o $(obj)/stage0.init
> $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
> $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o $(obj)/stage0_link.o
> $(Q)printf "  TEST    $(subst $(shell pwd)/,,$(@))\n"
>  $(Q)test `wc -c < $(obj)/stage0.init` -gt 16128 && \
>   printf "Error. Bootblock got too big.\n" || true
> Index: util/lar/lar.c
>
> --- util/lar/lar.c	(revision 480)
> +++ util/lar/lar.c	(working copy)
> @@ -37,6 +37,7 @@
>  #define COPYRIGHT "Copyright (C) 2006-2007 coresystems GmbH"
>
> static int isverbose = 0;
> +static int iselfparse = 0;
>  static long larsize = 0;
>  static char *bootblock = NULL;
>  enum compalgo algo = none;
> @@ -44,7 +45,7 @@
>  static void usage(char *name)
>  {
>    printf("\nLAR - the LinuxBIOS Archiver " VERSION "\n" COPYRIGHT "\n\n"
>       "Usage: %s [-cxal] archive.lar [[[file1] file2] ...]\n\n", name);
>       "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 -a myrom.lar foo blob:baz\n");
>
> @@ -66,13 +67,18 @@
>    printf("                \ta 'm' suffix to multiple the size by 1024*1024.\n");
>    printf("  -b [bootblock]\tSpecify the bootblock blob\n");
>    printf("  -C [lzma|nrv2b]\tSpecify the compression method to use\n\n");
>
> printf("  -e pre-parse the payload ELF into LAR segments. Recommended\n\n");
>
> printf("General options\n");
> printf("  -v\tEnable verbose mode\n");
> printf("  -V\tShow the version\n");
> printf("  -h\tShow this help\n");
> printf("\n");
>
> +}
>
> +int elfparse(void)
> +{
> return iselfparse;
>
> }
>
> /* Add a human touch to the LAR size by allowing suffixes:
> @@ -209,6 +215,7 @@
>    	{"list", 0, 0, 'l'},
>    	{"size", 1, 0, 's'},
>    	{"bootblock", 1, 0, 'b'},
> {"elfparse", 1, 0, 'e'},
> {"verbose", 0, 0, 'v'},
> {"version", 0, 0, 'V'},
> {"help", 0, 0, 'h'},
>
> @@ -220,7 +227,7 @@
>    	exit(1);
>    }
> while ((opt = getopt_long(argc, argv, "acC:xls:b:vVh?",
> while ((opt = getopt_long(argc, argv, "acC:xels:b:vVh?",
> 		  long_options, &option_index)) != EOF) {
> switch (opt) {
> case 'a':
>
> @@ -240,6 +247,9 @@
>    	case 'l':
>    		larmode = LIST;
>    		break;
> case 'e':
> 	iselfparse = 1;
> 	break;
> case 'x':
> 	larmode = EXTRACT;
> 	break;
> Index: util/lar/lar.h
>
> --- util/lar/lar.h	(revision 480)
> +++ util/lar/lar.h	(working copy)
> @@ -60,6 +60,7 @@
>  typedef uint32_t u32;
>  typedef uint8_t  u8;
>
> +/* NOTE -- This and the linuxbios lar.h are NOT IN SYNC. Be careful. */
>  struct lar_header {
>    char magic[8];
>    u32 len;
> @@ -73,6 +74,8 @@
>     * 2 = nrv2b
>     */
>    u32 compression;
> u32 entry;		/* we might need to make this u64 */
> u32 loadaddress; /* ditto */
>
> };
>
> /**\struct
> Index: util/lar/lib.h
> ===================================================================
> --- util/lar/lib.h	(revision 480)
> +++ util/lar/lib.h	(working copy)
> @@ -38,6 +38,7 @@
>
> /* prototypes for lar.c functions */
>  int verbose(void);
> +int parseelf(void);
>  long get_larsize(void);
>  char *get_bootblock(void);
> Index: util/lar/Makefile
>
> --- util/lar/Makefile	(revision 480)
> +++ util/lar/Makefile	(working copy)
> @@ -17,7 +17,6 @@
>  ## along with this program; if not, write to the Free Software
>  ## Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
>  ##
>
> LAROBJ := lar.o stream.o lib.o
>
> LARDIR := lardir
> @@ -29,7 +28,6 @@
>  LARDIR     += nrv2bdir
>
> LAROBJ_ABS := $(patsubst %,$(obj)/util/lar/%,$(LAROBJ))
>
> lardir:
>    $(Q)printf "  BUILD   LAR\n"
>    $(Q)mkdir -p $(obj)/util/lar

[ron minnich]

On 8/28/07, Carl-Daniel Hailfinger  wrote:
> OK, so I was unclear. The idea was to number the payloads as well, not
> only the segments.

Let's hold off on that, it will require a lot of changes, and it is
not really what we planned. We have normal and fallback; there is
really nothing in the design for normal0, normal1, etc.
> We simply could reserve some space at the end of the header and use that
> for future extensions without having to change a version number. Only if
> the extension needs changes in earlier fields of the struct we would
> have to change the MAGIC.

It gets messy. We'd have to implement version # management in
lib/lar.c, and I think that's a different coding discussion which I
would rather not include in this patch.
> OK, but please include your explanation as code comment.

done.
> > note that the payload is now payload0, payload1, etc. I've extended
> > linuxbios to look for these. Note that you can now see all the things
> > that get loaded ;they're no longer hidden in an ELF header somewhere.
> > Elf failures are gone!
>
> Please update the comment above.

done
> Cosmetic (no space between * and name):
>  void *entry;
>  void *loadaddress;

fixed.
> > };
> >
> > /* Prototypes. */
> > @@ -77,5 +87,6 @@
> >  int copy_file(struct mem_file *archive, char *filename, void *where);
> >  int run_file(struct mem_file *archive, char *filename, void *where);
> >  int execute_in_place(struct mem_file *archive, char *filename);
> >
> > +int run_address(void *f);
> > +void * load_file(struct mem_file *archive, char *filename);
>
> Same cosmetic.

done
> Can you add your explanation from above?

done

===================================================================
> > --- lib/lzmadecode.c  (revision 480)
> > +++ lib/lzmadecode.c  (working copy)
> > @@ -206,7 +206,6 @@
> >          RC_GET_BIT(probLit, symbol)
> >        }
> >        previousByte = (Byte)symbol;
>
> Empty line removal. Was this intentional?

fixed.
> >         if (walk[0] != 'L')
> >                 continue;
> >
> >         if (strcmp(&walk[1], MAGIC) != 0)
> >                 continue;
>
> There has to be a cleaner trick. Just in case somebody has the expected
> MAGIC string in an uncompressed lar file somewhere (other sources of
> that are possible!), we still fail, but less often.

The trick here is that the 'L' is in code space. Yes, there has to be
a better way to do this.
> >           // FIXME: check checksum
> >
> >           if (strcmp(fullname, filename) == 0) {
> >                 printk(BIOS_SPEW, "LAR: it matches %s @ %p\n", fullname, header);
>
> replace "it" with "filename"?

I revised the print messages, see what you think.
> >                 printk(BIOS_SPEW, "start %p len %d reallen %d compression %x entry %p loadaddress %p\n",
> >                         result->start, result->len, result->reallen, result->compression, result->entry, result->loadaddress);
>
> Any chance to break that into multiple lines?

I did what I could, I added all your cosmetic changes too.

thanks, patch attached again

ron

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

[Carl-Daniel Hailfinger]

On 28.08.2007 18:11, ron minnich wrote:
> On 8/28/07, Carl-Daniel Hailfinger  wrote:
> > OK, so I was unclear. The idea was to number the payloads as well, not
> > only the segments.
>
> Let's hold off on that, it will require a lot of changes, and it is
> not really what we planned. We have normal and fallback; there is
> really nothing in the design for normal0, normal1, etc.

AFAIK Uwe has a patch doing exactly that.
> > We simply could reserve some space at the end of the header and use that
> > for future extensions without having to change a version number. Only if
> > the extension needs changes in earlier fields of the struct we would
> > have to change the MAGIC.
>
> It gets messy. We'd have to implement version # management in
> lib/lar.c, and I think that's a different coding discussion which I
> would rather not include in this patch.
> > OK, but please include your explanation as code comment.
>
> done.
> > > note that the payload is now payload0, payload1, etc. I've extended
> > > linuxbios to look for these. Note that you can now see all the things
> > > that get loaded ;they're no longer hidden in an ELF header somewhere.
> > > Elf failures are gone!
> >
> > Please update the comment above.
>
> done
> > Cosmetic (no space between * and name):
> >  void *entry;
> >  void *loadaddress;
>
> fixed.
> > > };
> > >
> > > /* Prototypes. */
> > > @@ -77,5 +87,6 @@
> > >  int copy_file(struct mem_file *archive, char *filename, void *where);
> > >  int run_file(struct mem_file *archive, char *filename, void *where);
> > >  int execute_in_place(struct mem_file *archive, char *filename);
> > >
> > > +int run_address(void *f);
> > > +void * load_file(struct mem_file *archive, char *filename);
> >
> > Same cosmetic.
>
> done
> > Can you add your explanation from above?
>
> done
>
> ===================================================================
> > > --- lib/lzmadecode.c  (revision 480)
> > > +++ lib/lzmadecode.c  (working copy)
> > > @@ -206,7 +206,6 @@
> > >          RC_GET_BIT(probLit, symbol)
> > >        }
> > >        previousByte = (Byte)symbol;
> >
> > Empty line removal. Was this intentional?
>
> fixed.
> > >         if (walk[0] != 'L')
> > >                 continue;
> > >
> > >         if (strcmp(&walk[1], MAGIC) != 0)
> > >                 continue;
> >
> > There has to be a cleaner trick. Just in case somebody has the expected
> > MAGIC string in an uncompressed lar file somewhere (other sources of
> > that are possible!), we still fail, but less often.
>
> The trick here is that the 'L' is in code space. Yes, there has to be
> a better way to do this.
> > >           // FIXME: check checksum
> > >
> > >           if (strcmp(fullname, filename) == 0) {
> > >                 printk(BIOS_SPEW, "LAR: it matches %s @ %p\n", fullname, header);
> >
> > replace "it" with "filename"?
>
> I revised the print messages, see what you think.
> > >                 printk(BIOS_SPEW, "start %p len %d reallen %d compression %x entry %p loadaddress %p\n",
> > >                         result->start, result->len, result->reallen, result->compression, result->entry, result->loadaddress);
> >
> > Any chance to break that into multiple lines?
>
> I did what I could, I added all your cosmetic changes too.
>
> thanks, patch attached again

I like it.
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel

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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [070828 18:27]:
> I like it.
> Acked-by: Carl-Daniel Hailfinger

Yes, me too. But please don't put in the pk() stuff yet. It's completely
unrelated to the replace ELF by LAR stuff. Atomic commits if possible.

[ron minnich]

R481 for LAR
R482 for arch/x86 mods

Attached is a patch for 'noelf mode' + a FIX that is clean for lib/lar.c.

signed-off in patch.

ron

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

[Carl-Daniel Hailfinger]

On 29.08.2007 17:32, ron minnich wrote:
> R481 for LAR
> R482 for arch/x86 mods
>
> Attached is a patch for 'noelf mode' + a FIX that is clean for lib/lar.c.
>
> signed-off in patch.
>
> ron
>
> This patch is for two things that are too hard to seperate, as they affect
> one common file.
>
> The first is the noelf option, which is a simple
> modification to look for payloads in the form
> payload/segmentX where X is a number.
>
> The second is the fix for improperly wrapping to 0 when
> searching the LAR.
>
> This has been tested with filo and BOCHS.
>
> Signed-off-by: Ronald G. Minnich

I like it. One minor comment:
> --- lib/lar.c	(revision 480)
> +++ lib/lar.c	(working copy)
> @@ -31,6 +31,13 @@
>  #define ntohl(x) (x)
>  #endif
>
> +int run_address(void *f)
> +{
> int (*v) (void);
> v = f;
> return v();
>
> +}
>
> int find_file(struct mem_file *archive, char *filename, struct mem_file *result)
>  {
>    char *walk, *fullname;
> @@ -40,31 +47,104 @@
>    printk(BIOS_SPEW, "LAR: Start %p len 0x%x\n", archive->start,
>           archive->len);
> /* Getting this for loop right is harder than it looks. All quantities are
>  * unsigned. The LAR stretches from (e.g.) 0xfff0000 for 0x100000 
>  * bytes, i.e. to address ZERO. 
>  * As a result, 'walk', can wrap to zero and keep going (this has been 
>  * seen in practice). Recall that these are unsigned; walk can 
>  * wrap to zero; so, question, when is walk less than any of these:
>  * archive->start
>  * Answer: once walk wraps to zero, it is < archive->start
>  * archive->start + archive->len
>  * archive->start + archive->len  - 1
>  * Answer: In the case that archive->start + archive->len == 0, ALWAYS!
>  * A lot of expressions have been tried and all have been wrong. 
>  * So what would work? Simple:
>  * test for walk < archive->start + archive->len - 1 to cover the case
>  *	that the archive does NOT occupy ALL of the top of memory and 
>  *	wrap to zero; 
>  * and test for walk >= archive->start, 
>  * to cover the case that you wrapped to zero. 
>  * Unsigned pointer arithmetic that wraps to zero can be messy.
>  */
> for (walk = archive->start;
>     (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
>     (walk < (char *)(archive->start + archive->len - 1)) && 
> 	(walk >= (char *)archive->start); walk += 16) {

What about
> for (walk = archive->start;
>     (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
>     (walk <= (char *)(archive->start + archive->len - sizeof(struct lar_header))) && 
> 	(walk >= (char *)archive->start); walk += 16) {

instead? Looking for a lar header 1 byte before the end of the archive
is not going to succeed anyway.
Should we additionally check at the beginning of the function that the
archive is at least as long as sizeof(struct lar_header)?
> if (strcmp(walk, MAGIC) != 0)
> 	continue;
>
> header = (struct lar_header *)walk;
> fullname = walk + sizeof(struct lar_header);
>
> printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
> printk(BIOS_SPEW, "LAR: search for %s\n", fullname);
>
> // FIXME: check checksum
>
> if (strcmp(fullname, filename) == 0) {
> 	printk(BIOS_SPEW, "LAR: CHECK %s @ %p\n", fullname, header);
> result->start = walk + ntohl(header->offset);
> result->len = ntohl(header->len);
> result->reallen = ntohl(header->reallen);
> result->compression = ntohl(header->compression);
> 	result->entry = (void *)ntohl(header->entry);
> 	result->loadaddress = (void *)ntohl(header->loadaddress);
> 	printk(BIOS_SPEW, 
> 	"start %p len %d reallen %d compression %x entry %p loadaddress %p\n", 
> 		result->start, result->len, result->reallen, 
> 		result->compression, result->entry, result->loadaddress);
> return 0;
>
> }
> // skip file
> walk += (ntohl(header->len) + ntohl(header->offset) -
> 	1) & 0xfffffff0;
> }
>
> printk(BIOS_SPEW, "NO FILE FOUND\n");
> return 1;
>
> }

Otherwise, this patch and included comments look really nice.

Acked-by: Carl-Daniel Hailfinger

[ron minnich]

On 8/29/07, Carl-Daniel Hailfinger  wrote:
> >   for (walk = archive->start;
> >      (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
> >      (walk <= (char *)(archive->start + archive->len - sizeof(struct lar_header))) &&
> >                 (walk >= (char *)archive->start); walk += 16) {
>
> instead? Looking for a lar header 1 byte before the end of the archive
> is not going to succeed anyway.

works for me.
> Should we additionally check at the beginning of the function that the
> archive is at least as long as sizeof(struct lar_header)?

Ah, well, we can do that, but ... what are we going to do at that
point? I'll put the check in with a warning, in case there is bogus
cruft, but I don't think we can give up at that point.

thanks

ron

[ron minnich]

Here's a proposed patch taking Carl-Daniel's comments into account.

ron

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

[Carl-Daniel Hailfinger]

On 29.08.2007 18:20, ron minnich wrote:
> This patch is for two things that are too hard to seperate, as they affect
> one common file.
>
> The first is the noelf option, which is a simple
> modification to look for payloads in the form
> payload/segmentX where X is a number.
>
> The second is the fix for improperly wrapping to 0 when
> searching the LAR.
>
> This has been tested with filo and BOCHS.
>
> Signed-off-by: Ronald G. Minnich

Acked-by: Carl-Daniel Hailfinger

[ron minnich]

Committed revision 484.

On 8/29/07, Carl-Daniel Hailfinger  wrote:
> On 29.08.2007 18:20, ron minnich wrote:
> > This patch is for two things that are too hard to seperate, as they affect
> > one common file.
> >
> > The first is the noelf option, which is a simple
> > modification to look for payloads in the form
> > payload/segmentX where X is a number.
> >
> > The second is the fix for improperly wrapping to 0 when
> > searching the LAR.
> >
> > This has been tested with filo and BOCHS.
> >
> > Signed-off-by: Ronald G. Minnich
>
> Acked-by: Carl-Daniel Hailfinger

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

[Uwe Hermann]

On Wed, Aug 29, 2007 at 09:20:05AM -0700, ron minnich wrote:
> Signed-off-by: Ronald G. Minnich ===================================================================

What's up with that "============" line?
> --- lib/lar.c	(revision 480)
> +++ lib/lar.c	(working copy)
> @@ -31,6 +31,13 @@
>  #define ntohl(x) (x)
>  #endif
>
> +int run_address(void *f)
> +{
> int (*v) (void);
> v = f;
> return v();
>
> +}

Add a doxygen comment please.
> int find_file(struct mem_file *archive, char *filename, struct mem_file *result)

Same here.

filename can be 'const char *filename' I guess.
> {
>    char *walk, *fullname;
> @@ -40,31 +47,108 @@
>    printk(BIOS_SPEW, "LAR: Start %p len 0x%x\n", archive->start,
>           archive->len);
> if (archive->len < sizeof(struct lar_header))
> printk(BIOS_ERR, "Error: truncated archive (%d bytes); minimum possible size is %d bytes\n", 
> 	archive->len, sizeof(struct lar_header));
> /* Getting this for loop right is harder than it looks. All quantities are
>  * unsigned. The LAR stretches from (e.g.) 0xfff0000 for 0x100000 
>  * bytes, i.e. to address ZERO. 
>  * As a result, 'walk', can wrap to zero and keep going (this has been 
>  * seen in practice). Recall that these are unsigned; walk can 
>  * wrap to zero; so, question, when is walk less than any of these:
>  * archive->start
>  * Answer: once walk wraps to zero, it is < archive->start
>  * archive->start + archive->len
>  * archive->start + archive->len  - 1
>  * Answer: In the case that archive->start + archive->len == 0, ALWAYS!
>  * A lot of expressions have been tried and all have been wrong. 
>  * So what would work? Simple:
>  * test for walk < archive->start + archive->len - 1 to cover the case
>  *	that the archive does NOT occupy ALL of the top of memory and 
>  *	wrap to zero; 
>  * and test for walk >= archive->start, 
>  * to cover the case that you wrapped to zero. 
>  * Unsigned pointer arithmetic that wraps to zero can be messy.
>  */
> for (walk = archive->start;
>     (walk - 1) < (char *)(archive->start + archive->len - 1 ); walk += 16) {
>     (walk < (char *)(archive->start + archive->len - sizeof(struct lar_header))) && 
> 	(walk >= (char *)archive->start); walk += 16) {
>
> if (strcmp(walk, MAGIC) != 0)
> 	continue;
>
> header = (struct lar_header *)walk;
> fullname = walk + sizeof(struct lar_header);
> printk(BIOS_SPEW, "LAR: current filename is %s\n", fullname);
> printk(BIOS_SPEW, "LAR: search for %s\n", fullname);
>
> // FIXME: check checksum
>
> if (strcmp(fullname, filename) == 0) {
> 	printk(BIOS_SPEW, "LAR: CHECK %s @ %p\n", fullname, header);
> result->start = walk + ntohl(header->offset);
> result->len = ntohl(header->len);
> result->reallen = ntohl(header->reallen);
> result->compression = ntohl(header->compression);
> 	result->entry = (void *)ntohl(header->entry);
> 	result->loadaddress = (void *)ntohl(header->loadaddress);

Just curious, is the cast to (void *) really needed?
> 	printk(BIOS_SPEW, 
> 	"start %p len %d reallen %d compression %x entry %p loadaddress %p\n", 
> 		result->start, result->len, result->reallen, 
> 		result->compression, result->entry, result->loadaddress);
> return 0;
> }
> // skip file
> walk += (ntohl(header->len) + ntohl(header->offset) -
> 	1) & 0xfffffff0;
> }
> printk(BIOS_SPEW, "NO FILE FOUND\n");
> return 1;
>
> }
>
> +void *load_file(struct mem_file *archive, char *filename)

Add doxygen comment, please.
> +{
> int ret;
> struct mem_file result;
> void *where;
> void *entry;
> ret = find_file(archive, filename, &result);
> if (ret) {
> printk(BIOS_INFO, "LAR: load_file: No such file '%s'\n",
>        filename);
> return (void *)-1;

Uh? What is '(void *)-1' supposed to be? Will that work? Is the cast needed?
> +/* FIXME -- most of copy_file should be replaced by load_file */
>  int copy_file(struct mem_file *archive, char *filename, void *where)
>  {
>    int ret;
> @@ -85,7 +169,7 @@
>    }
>  #ifdef CONFIG_COMPRESSION_LZMA
>    /* lzma */
> unsigned long ulzma(unsigned char * src, unsigned char * dst);
> unsigned long ulzma(unsigned char *src, unsigned char *dst);

ACK.
> @@ -130,9 +215,11 @@
>    	}
>    	where = result.start;
>    }
> printk(BIOS_SPEW, "where is %p\n", where);
> v = where;
> return v();
> ret =  v();

^^
Two spaces where only one should be.

Uwe.

[ron minnich]

Sorry, I saw the ack before I saw your mail. I will go back and fix up
per your comments.

Thanks

ron

[ron minnich]

On 8/29/07, Uwe Hermann  wrote:
> On Wed, Aug 29, 2007 at 09:20:05AM -0700, ron minnich wrote:
> > Signed-off-by: Ronald G. Minnich ===================================================================
>
> What's up with that "============" line?

my mistake.
> > --- lib/lar.c (revision 480)
> > +++ lib/lar.c (working copy)
> > @@ -31,6 +31,13 @@
> >  #define ntohl(x) (x)
> >  #endif
> >
> > +int run_address(void *f)
> > +{
> > int (*v) (void);
> > v = f;
> > return v();
> >
> > +}
>
> Add a doxygen comment please.

done. Please note that I did not write this stuff, there were never
comments in there :-)
> > int find_file(struct mem_file *archive, char *filename, struct mem_file *result)
>
> Same here.

done.
> filename can be 'const char *filename' I guess.

done. The next patch will include a patch to lar.h for this change.
> >                 result->entry = (void *)ntohl(header->entry);
> >                 result->loadaddress = (void *)ntohl(header->loadaddress);
>
> Just curious, is the cast to (void *) really needed?

yes. ntohl is a long. To get to void * you need the cast.
> > +void *load_file(struct mem_file *archive, char *filename)
>
> Add doxygen comment, please.

done
> >         return (void *)-1;
>
> Uh? What is '(void *)-1' supposed to be? Will that work? Is the cast needed?

(void *)-1 is a commonly used synonym for "bad address". It's the most
portable way to indicate a bad address.
> > printk(BIOS_SPEW, "where is %p\n", where);
> > v = where;
> > return v();
> > ret =  v();
>          ^^
>
> Two spaces where only one should be.

fixed.

new patch attached.

ron

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

[Stefan Reinauer]

* ron minnich  [070828 18:11]:
> On 8/28/07, Carl-Daniel Hailfinger  wrote:
> > OK, so I was unclear. The idea was to number the payloads as well, not
> > only the segments.
>
> Let's hold off on that, it will require a lot of changes, and it is
> not really what we planned. We have normal and fallback; there is
> really nothing in the design for normal0, normal1, etc.

We don't even have normal and fallback at the moment. And I doubt it
will come back, except artificially.

I think we have two choices:

* either fallback becomes _really_ stupid and just tries to load a
  payload via ymodem

* or fallback/normal is linuxbios and has nothing to do with the
  payload. So N linuxbios images (N=2.. normal and payload) will be able
  to load M (might be 2, but no requirement) payloads.
> > We simply could reserve some space at the end of the header and use that
> > for future extensions without having to change a version number. Only if
> > the extension needs changes in earlier fields of the struct we would
> > have to change the MAGIC.
>
> It gets messy. We'd have to implement version # management in
> lib/lar.c, and I think that's a different coding discussion which I
> would rather not include in this patch.

The simplest version management you could think of:

if (lar_version > my_version) {
        printf("Your image is too new. upgrade lar utility\n");
        exit(1);
}

if (lar_version > my_version) {
        printf("Your image is too old. downgrade lar utility\n");
        exit(1);
}

or even

if (lar_version != my_version) {
        printf("Your image version and lar utility version do not match. Change either\n");
        exit(1);
}

Anything beyound the above is a waste of precious time imho.
Let's not do migration paths for pre-pre-pre-pre-pre-production
software.

Stefan

[Peter Stuge]

On Tue, Aug 28, 2007 at 05:47:11PM +0200, Carl-Daniel Hailfinger wrote:
> > > > a major/minor version in the LAR header?
> > >
> > > I generally hate versions. Think features, not numbers.
> >
> > but then you have to handle the versioning, and figure out what
> > to do if it does not match, and .... Let's not do this.
>
> We simply could reserve some space at the end of the header and use
> that for future extensions without having to change a version
> number. Only if the extension needs changes in earlier fields of
> the struct we would have to change the MAGIC.

This works!

On Wed, Aug 29, 2007 at 10:04:26AM +0200, Stefan Reinauer wrote:
> Interoperability between different lar versions seems to be the
> quest for finding the best method of shooting yourself in the foot.
> Definitely a big issue and you can spend a looot of time on getting
> it done right.

It has surely been done many times before.

On Wed, Aug 29, 2007 at 10:12:24AM +0200, Stefan Reinauer wrote:
> if (lar_version > my_version) {
>         printf("Your image is too new. upgrade lar utility\n");
>
> if (lar_version > my_version) {
>         printf("Your image is too old. downgrade lar utility\n");
>
> if (lar_version != my_version) {
>         printf("Your image version and lar utility version do not match. Change either\n");
>
> Anything beyound the above is a waste of precious time imho.
> Let's not do migration paths for pre-pre-pre-pre-pre-production
> software.

Agreed - but the sooner we decide on a nice upwards compatible
protocol the sooner it becomes production software. ;)

Your suggestion is pretty similar to what BitTorrent uses and that
works fine. One bit per feature or even one bit to active a separate
extension protocol.

//Peter

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

[Stefan Reinauer]

* Carl-Daniel Hailfinger  [070828 17:47]:
> On 28.08.2007 17:26, ron minnich wrote:
> > On 8/28/07, Stefan Reinauer  wrote:
> > > Carl-Daniel Hailfinger  [070828 01:54]:
> > > > What about creating directories for each payload so we can differentiate
> > > > between multiple payloads (in the original sense)?
> > >
> > > Yes, I would prefer something like that, too
> >
> > done.
> > normal/payload/segment0
> > normal/payload/segment1
> > etc.
>
> OK, so I was unclear. The idea was to number the payloads as well, not
> only the segments.

I think that is something that Uwe's patch will solve?
> We simply could reserve some space at the end of the header and use that
> for future extensions without having to change a version number. Only if
> the extension needs changes in earlier fields of the struct we would
> have to change the MAGIC.

Since lar is part of LinuxBIOS v3, let's just use the lar version that
comes with it, and bundle the tool with the image in a tgz, or at least
mention the svn revision in the readme.

Interoperability between different lar versions seems to be the quest
for finding the best method of shooting yourself in the foot. Definitely
a big issue and you can spend a looot of time on getting it done right.

Stefan

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

[Stefan Reinauer]

* ron minnich  [070828 17:26]:
> > > void *entry;
> > > void *loadaddress;
> >
> > Not sure how these pointers sneaked in here. They do break portability
> > and cross compilability. Compiling LinuxBIOS on a 64bit host without
> > compiling lar 32bit is not possible with such things in the header.
>
> I think you guys have missed the point. These are in the mem_file
> struct, and that struct is by definition a per-machine struct. There
> is NO cross-compatibility issue with mem_file -- see include/lar.h.
> mem_file is not even defined in util/lar/lar.h. The lar utility
> doesn't know what it is.

Yes, sorry, I missed it.
> > What was that linker auto rename trick that Marc mentioned recently?
>
> The naming is not the issue. The issue is getting gcc to do abs calls,
> not relative calls, for some symbols.

Sure. But still the naming is an issue. The question is: How can we call
it printk on both sides of the rubicon. And having LD rename some of the
symbols for us seems like a simple solution.

Stefan

[ron minnich]

On 8/29/07, Stefan Reinauer  wrote:
> > > What was that linker auto rename trick that Marc mentioned recently?
> >
> > The naming is not the issue. The issue is getting gcc to do abs calls,
> > not relative calls, for some symbols.

I did the auto-rename trick. It is a simple objcopy with
--prefix-symbols and whatever you want -- e.g. stage0.

Then in initram you can do this (which I did):
int (*printk)(int level, char *fmt, ...) = stage0printk;

The problem comes in with includes. How do you declare printk? In
console.h it is this:
int printk(int level, char *fmt, ...)

So you get a compile time error. Do you change all the include files? uck.

The simplest, easiest way to make this go would be a pragma on certain
symbols that makes gcc do an absolute, not relative, call. is this
possible?

ron

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

[Stefan Reinauer]

ron minnich wrote:
> On 8/29/07, Stefan Reinauer  wrote:
> > > > What was that linker auto rename trick that Marc mentioned recently?
> > >
> > > The naming is not the issue. The issue is getting gcc to do abs calls,
> > > not relative calls, for some symbols.
>
> I did the auto-rename trick. It is a simple objcopy with
> --prefix-symbols and whatever you want -- e.g. stage0.
>
> Then in initram you can do this (which I did):
> int (*printk)(int level, char *fmt, ...) = stage0printk;
>
> The problem comes in with includes. How do you declare printk? In
> console.h it is this:

#ifndef STAGE0

int printk(int level, char *fmt, ...);

#else

int (*printk)(int level, char *fmt, ...) = stage0printk;

#endif

It even fails for those symbols that we don't explicitly want to export.
Pretty good I guess.
> So you get a compile time error. Do you change all the include files? uck.

No, only one header?!?
> The simplest, easiest way to make this go would be a pragma on certain
> symbols that makes gcc do an absolute, not relative, call. is this
> possible?

This sounds even better. No idea though.

[ron minnich]

symbol rename:

$(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
        $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o
$(obj)/stage0_link.o

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

[Uwe Hermann]

On Wed, Aug 29, 2007 at 08:01:31AM -0700, ron minnich wrote:
> symbol rename:
>     $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
>     $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o
>
> $(obj)/stage0_link.o

Personally I find this pretty scary stuff, so please make sure to
add very verbose documentation here as to why it's done and how it
works and which assumptions must be true for this to work (minimum
ld version or such things).

Uwe.

[ron minnich]

On 8/29/07, Uwe Hermann  wrote:
> On Wed, Aug 29, 2007 at 08:01:31AM -0700, ron minnich wrote:
> > symbol rename:
> >     $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
> >     $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o
> >
> > $(obj)/stage0_link.o
>
> Personally I find this pretty scary stuff, so please make sure to
> add very verbose documentation here as to why it's done and how it
> works and which assumptions must be true for this to work (minimum
> ld version or such things).

we're not doing this YET. So don't worry too much ... YET :-)

ron

[Peter Stuge]

On Wed, Aug 29, 2007 at 09:11:54AM -0700, ron minnich wrote:
> > > symbol rename:
> > >     $(Q)printf " OBJCOPY (stage0 link) $(subst $(shell pwd)/,,$(@))\n"
> > >     $(Q)$(OBJCOPY) --prefix-symbols=stage0 $(obj)/stage0.o
> > >
> > > $(obj)/stage0_link.o
> >
> > Personally I find this pretty scary stuff, so please make sure to
> > add very verbose documentation
>
> we're not doing this YET. So don't worry too much ... YET :-)

I like it. It's really rather simple;

stage0 is special, so we need a trick to be able to reuse code both
in stage0 and later stages.

//Peter

[ron minnich]

On 8/27/07, Carl-Daniel Hailfinger  wrote:
> What about creating directories for each payload so we can differentiate
> between multiple payloads (in the original sense)?
>
> normal/payload0/segment0 (33192 bytes, lzma compressed to 18088 bytes
> @0x38 load @0x100000, entry 0x105258)
>   normal/payload0/segment1 (72 bytes, lzma compressed to 47 bytes
> @0x4718 load @0x1225a0, entry 0x105258)

more like normal/payload/segment0
normal/payload/segment1

sure we can do this, I did not know how to make lar do it yet. I'll
see what I can do.
> Maybe we should introduce a major/minor version in the LAR header? Or a
> different magic string for each revision?

Please no. Let's get this right and let's try to avoid versioning
structures. It's nothing but a headache.
> Do we also need an alignment parameter in the header (like "free
> placement, but please align to xx")?

I think we should adopt the convention of page alignment and leave it at that.
> My eyes!

w.r.t. that comment on the pk( stuff in initram, this code is forcing
gcc to make abs jumps for the printk. Someone tell me how to do this
and that ugliness can go away. But we need PIC for initram in this
case and that's all I have been able to make
gcc do.
> > --- mainboard/emulation/qemu-x86/Makefile     (revision 464)
> > +++ mainboard/emulation/qemu-x86/Makefile     (working copy)
> > @@ -41,14 +41,17 @@
> >  #
> >
> > INITRAM_OBJ = $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> > +$(obj)/mainboard/$(MAINBOARDDIR)/initram.o: $(src)/mainboard/$(MAINBOARDDIR)/initram.c
> > cc -c $(INITCFLAGS)  -fPIE $(src)/mainboard/$(MAINBOARDDIR)/initram.c -o $(obj)/mainboard/$(MAINBOARDDIR)/initram.o
> >
> > # These are possibly not permanent
> > -INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
> > +#INITRAM_OBJ += $(obj)/lib/console.o $(obj)/lib/vtxprintf.o $(obj)/lib/uart8250.o $(obj)/arch/x86/post_code.o
> >
> > $(obj)/linuxbios.initram: $(obj)/stage0.init $(obj)/stage0.o $(INITRAM_OBJ)
> >       $(Q)# initram links against stage0
> >       $(Q)printf "  LD      $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(LD)  -Ttext 0x80000 $(INITRAM_OBJ) \
> >         --entry=main -o $(obj)/linuxbios.initram.o
> > $(Q)$(LD)  $(INITRAM_OBJ) \
> >         --entry=main -R $(obj)/stage0.o -o $(obj)/linuxbios.initram.o
> > $(Q)printf "  OBJCOPY $(subst $(shell pwd)/,,$(@))\n"
> > $(Q)$(OBJCOPY) -O binary $(obj)/linuxbios.initram.o \
> >         $(obj)/linuxbios.initram
>
> Could you explain that one?

It is linking the initram (which is PIC) against the stage 0 that
lives up at the top of flash.
> Needs real documentation (LAR README maybe?), not just a code comment.

once we get a commit of the basic idea, it will go in docs.
> >   $(Q)printf "  TEST    $(subst $(shell pwd)/,,$(@))\n"
> >   $(Q)test `wc -c < $(obj)/stage0.init` -gt 16128 && \
> >           printf "Error. Bootblock got too big.\n" || true
>
> Please explain.

this is stefan's stuff, stefan?
> old revision, you will have to make sure the merge won't kill the
> changes since 464.

I remerged in the next patch.

OK, new patch attached with fixes for all comments to date. I did an
svn up before this, and have built and tested with it.

ron