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

[ron minnich]

Note new patch attached, in response to comments.

On 8/27/07, Peter Stuge <peter at stuge.se> 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 <rminnich at gmail.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: noelf.diff
Type: text/x-patch
Size: 19883 bytes
Desc: not available
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070827/82e79961/attachment.diff>