Patchwork cbfstool "add-lzma" method

login
register
about
Submitter Kevin O'Connor
Date 2010-09-05 18:21:08
Message ID <20100905182108.GA32750@morn.localdomain>
Download mbox | patch
Permalink /patch/1867/
State Superseded
Headers show

Comments

Kevin O'Connor - 2010-09-05 18:21:08
This patch enhances cbfstool so that it can support "cbfstool ROM
add-lzma FILE NAME" calls.  This is useful for callers that wish to
add an lzma compressed raw file.

Right now, SeaBIOS supports lzma raw files for things like floppy
images.  Today, adding one of these files requires a two step process
- for example:

$ lzma -zc /path/to/myfloppy.img > myfloppy.img.lzma
$ cbfstool coreboot.rom add myfloppy.img.lzma floppyimg/MyFloppy.lzma raw

Unfortunately, various versions of "lzma" are quirky and the above can
be troublesome.  With the patch below, a user need only execute:

$ cbfstool coreboot.rom add-lzma myfloppy.img floppyimg/MyFloppy.lzma

Signed-off-by: Kevin O'Connor <kevin@koconnor.net>
Peter Stuge - 2010-09-05 18:26:41
Kevin O'Connor wrote:
> This patch enhances cbfstool so that it can support "cbfstool ROM
> add-lzma FILE NAME" calls.

It has slightly different semantics than plain add. I think an option
to enable compression would be nicer, so that there's just one
generic add command.


//Peter
Kevin O'Connor - 2010-09-05 21:34:07
On Sun, Sep 05, 2010 at 08:26:41PM +0200, Peter Stuge wrote:
> Kevin O'Connor wrote:
> > This patch enhances cbfstool so that it can support "cbfstool ROM
> > add-lzma FILE NAME" calls.
> 
> It has slightly different semantics than plain add. I think an option
> to enable compression would be nicer, so that there's just one
> generic add command.

There are three add commands right now: add, add-payload, add-stage.

The cbfstool code can't easily handle per-command options.  So, doing
something like "cbfstool ROM add -c lzma FILE NAME" wont be easy.

I can refactor the code so that "add" and "add-lzma" share the same
code.

Another possibility would be to add getopt to cbfstool and move
compression selection for all add commands to it.  For example,
something like "cbfstool -c lzma ROM add-payload FILE NAME".  However,
that would require changing the build to use the new form everywhere.

-Kevin
Patrick Georgi - 2010-09-06 09:40:10
Am 05.09.2010 23:34, schrieb Kevin O'Connor:
> Another possibility would be to add getopt to cbfstool and move
> compression selection for all add commands to it.  For example,
> something like "cbfstool -c lzma ROM add-payload FILE NAME".  However,
> that would require changing the build to use the new form everywhere.
We could move compression out of subheader fields into the regular file
header, so every file could be compressed (though for some it wouldn't
be useful, eg. romstages or EC roms), and then make this a global variable.

However, we had our reasons not to do this. Pick your poison


Patrick
Kevin O'Connor - 2010-09-06 15:27:06
On Mon, Sep 06, 2010 at 11:40:10AM +0200, Patrick Georgi wrote:
> We could move compression out of subheader fields into the regular file
> header, so every file could be compressed (though for some it wouldn't
> be useful, eg. romstages or EC roms), and then make this a global variable.
> 
> However, we had our reasons not to do this. Pick your poison

I wasn't looking to rework the CBFS format.  Right now, it can be a
pain for SeaBIOS users to add certain compressed files like floppies.
Today they need to do:

$ lzma -zc /path/to/myfloppy.img > myfloppy.img.lzma
$ cbfstool coreboot.rom add myfloppy.img.lzma floppyimg/MyFloppy.lzma raw

Unfortunately, not everyone has the "lzma" tool, and some distro
versions of that tool are quirky.

So, I'm looking for a helper to cbfstool to make adding compressed raw
files simpler for end users.

I've also been thinking of adding another helper - "add-string".  So a
user could do something like: "cbfstool ROM add-string '5500'
cfg/boot-menu-delay".  As I think this may be a straight forward way
to pass simple config items into SeaBIOS.

-Kevin
Stefan Reinauer - 2010-09-06 17:34:55
On 9/6/10 5:27 PM, Kevin O'Connor wrote:
> I've also been thinking of adding another helper - "add-string".  So a
> user could do something like: "cbfstool ROM add-string '5500'
> cfg/boot-menu-delay".  As I think this may be a straight forward way
> to pass simple config items into SeaBIOS.
What's a "string" supposed to be in that context?

Stefan
Patrick Georgi - 2010-09-06 17:36:28
Am 06.09.2010 19:34, schrieb Stefan Reinauer:
>  On 9/6/10 5:27 PM, Kevin O'Connor wrote:
>> I've also been thinking of adding another helper - "add-string".  So a
>> user could do something like: "cbfstool ROM add-string '5500'
>> cfg/boot-menu-delay".  As I think this may be a straight forward way
>> to pass simple config items into SeaBIOS.
> What's a "string" supposed to be in that context?
Sounds like a reinvention of our CMOS stuff...


Patrick
Kevin O'Connor - 2010-09-06 17:58:12
On Mon, Sep 06, 2010 at 07:34:55PM +0200, Stefan Reinauer wrote:
>  On 9/6/10 5:27 PM, Kevin O'Connor wrote:
> > I've also been thinking of adding another helper - "add-string".  So a
> > user could do something like: "cbfstool ROM add-string '5500'
> > cfg/boot-menu-delay".  As I think this may be a straight forward way
> > to pass simple config items into SeaBIOS.
> What's a "string" supposed to be in that context?

It would be passing in the file contents on the command-line instead
of in a file.  So, for example:

$ cbfstool ROM add-string '5500' cfg/boot-menu-delay

would be the equivalent of:

$ echo '5500' > myfile
$ cbfstool ROM add myfile cfg/boot-menu-delay

-Kevin
Kevin O'Connor - 2010-09-06 18:10:39
On Mon, Sep 06, 2010 at 07:36:28PM +0200, Patrick Georgi wrote:
> Am 06.09.2010 19:34, schrieb Stefan Reinauer:
> >  On 9/6/10 5:27 PM, Kevin O'Connor wrote:
> >> I've also been thinking of adding another helper - "add-string".  So a
> >> user could do something like: "cbfstool ROM add-string '5500'
> >> cfg/boot-menu-delay".  As I think this may be a straight forward way
> >> to pass simple config items into SeaBIOS.
> > What's a "string" supposed to be in that context?
> Sounds like a reinvention of our CMOS stuff...

It serves a similar role.  I don't much like CMOS because I've found
it flaky on boards.  I also want to be able to config boot order - for
example, a boot order might be something like
"cdrom,disk,pci00:13.1,floppy" for cdrom, hard drive, BEV on an
optionrom (eg, nic), and floppy.  Hard drive order might be like
"ata1-0,usb1234:4567,pci00:13.1/1" for an ata drive, USB drive, and a
BCV on an optionrom (eg, scsi drive).  I think encoding this into CMOS
would be a lot harder than encoding into a string in CBFS.

Anyway, this is just in the though phase.  I only raised it because I
thought it might be a sibling to the proposed "add-lzma".

-Kevin
Patrick Georgi - 2010-09-06 19:05:39
Am 06.09.2010 20:10, schrieb Kevin O'Connor:
> It serves a similar role.  I don't much like CMOS because I've found
> it flaky on boards.  I also want to be able to config boot order - for
> example, a boot order might be something like
> "cdrom,disk,pci00:13.1,floppy" for cdrom, hard drive, BEV on an
> optionrom (eg, nic), and floppy.  Hard drive order might be like
> "ata1-0,usb1234:4567,pci00:13.1/1" for an ata drive, USB drive, and a
> BCV on an optionrom (eg, scsi drive).  I think encoding this into CMOS
> would be a lot harder than encoding into a string in CBFS.
We use a string in CMOS for that on kontron/986lcd-m


> Anyway, this is just in the though phase.  I only raised it because I
> thought it might be a sibling to the proposed "add-lzma".
add-lzma somewhat breaks the CBFS design as is. hence the question how
this can be made consistent.


Patrick

Patch

Index: util/cbfstool/cbfs.h
===================================================================
--- util/cbfstool/cbfs.h	(revision 5777)
+++ util/cbfstool/cbfs.h	(working copy)
@@ -73,6 +73,7 @@ 
 #define CBFS_COMPONENT_OPTIONROM  0x30
 #define CBFS_COMPONENT_BOOTSPLASH 0x40
 #define CBFS_COMPONENT_RAW        0x50
+#define CBFS_COMPONENT_LZMA_RAW   0x40000050
 #define CBFS_COMPONENT_VSA        0x51
 #define CBFS_COMPONENT_MBI        0x52
 #define CBFS_COMPONENT_MICROCODE  0x53
Index: util/cbfstool/common.c
===================================================================
--- util/cbfstool/common.c	(revision 5777)
+++ util/cbfstool/common.c	(working copy)
@@ -142,6 +142,7 @@ 
 	{CBFS_COMPONENT_OPTIONROM, "optionrom"},
 	{CBFS_COMPONENT_BOOTSPLASH, "bootsplash"},
 	{CBFS_COMPONENT_RAW, "raw"},
+	{CBFS_COMPONENT_LZMA_RAW, "lzma-raw"},
 	{CBFS_COMPONENT_VSA, "vsa"},
 	{CBFS_COMPONENT_MBI, "mbi"},
 	{CBFS_COMPONENT_MICROCODE, "microcode"},
Index: util/cbfstool/cbfstool.c
===================================================================
--- util/cbfstool/cbfstool.c	(revision 5777)
+++ util/cbfstool/cbfstool.c	(working copy)
@@ -18,6 +18,7 @@ 
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA, 02110-1301 USA
  */
 
+#include <stdlib.h>
 #include <stdio.h>
 #include <stdint.h>
 #include <string.h>
@@ -28,6 +29,7 @@ 
 	CMD_ADD,
 	CMD_ADD_PAYLOAD,
 	CMD_ADD_STAGE,
+	CMD_ADD_LZMA,
 	CMD_CREATE,
 	CMD_LOCATE,
 	CMD_PRINT
@@ -187,6 +189,73 @@ 
 	return 0;
 }
 
+static int cbfs_add_lzma(int argc, char **argv)
+{
+	char *romname = argv[1];
+	char *cmd = argv[2];
+	void *rom = loadrom(romname);
+
+	if (rom == NULL) {
+		printf("Could not load ROM image '%s'.\n", romname);
+		return 1;
+	}
+
+	if (argc < 5) {
+		printf("not enough arguments to '%s'.\n", cmd);
+		return 1;
+	}
+
+	char *filename = argv[3];
+	char *cbfsname = argv[4];
+
+	uint32_t filesize = 0;
+	void *filedata = loadfile(filename, &filesize, 0, SEEK_SET);
+	if (filedata == NULL) {
+		printf("Could not load file '%s'.\n", filename);
+		return 1;
+	}
+
+	uint32_t base = 0;
+	void *cbfsfile = NULL;
+
+	uint32_t type = CBFS_COMPONENT_LZMA_RAW;
+	if (argc > 5) {
+		if (intfiletype(argv[5]) != ((uint64_t) - 1))
+			type = intfiletype(argv[5]);
+		else
+			type = strtoul(argv[5], NULL, 0);
+	}
+	if (argc > 6) {
+		base = strtoul(argv[6], NULL, 0);
+	}
+	uint32_t compresssize = filesize;
+	void *compressed_data;
+	for (;;) {
+		compressed_data = malloc(compresssize);
+		if (! compressed_data) {
+			printf("Could not allocate %d space\n", compresssize);
+			return 1;
+		}
+		uint32_t actualsize = compresssize;
+		extern void do_lzma_compress(char *in, int in_len,
+					     char *out, int *out_len);
+		do_lzma_compress(filedata, filesize, compressed_data,
+				 &compresssize);
+		if (compresssize <= actualsize)
+			break;
+		// Try again with required size.
+		free(compressed_data);
+	}
+
+	cbfsfile = create_cbfs_file(cbfsname, compressed_data,
+				    &compresssize, type, &base);
+	if (add_file_to_cbfs(cbfsfile, compresssize, base))
+		return 1;
+	if (writerom(romname, rom, romsize))
+		return 1;
+	return 0;
+}
+
 static int cbfs_create(int argc, char **argv)
 {
 	char *romname = argv[1];
@@ -249,6 +318,7 @@ 
 	{CMD_ADD, "add", cbfs_add},
 	{CMD_ADD_PAYLOAD, "add-payload", cbfs_add_payload},
 	{CMD_ADD_STAGE, "add-stage", cbfs_add_stage},
+	{CMD_ADD_LZMA, "add-lzma", cbfs_add_lzma},
 	{CMD_CREATE, "create", cbfs_create},
 	{CMD_LOCATE, "locate", cbfs_locate},
 	{CMD_PRINT, "print", cbfs_print}
@@ -265,6 +335,7 @@ 
 	     " add FILE NAME TYPE [base address]    Add a component\n"
 	     " add-payload FILE NAME [COMP] [base]  Add a payload to the ROM\n"
 	     " add-stage FILE NAME [COMP] [base]    Add a stage to the ROM\n"
+	     " add-lzma FILE NAME [TYPE] [base]     Lzma compress and add\n"
 	     " create SIZE BOOTBLOCK [ALIGN]        Create a ROM file\n"
 	     " locate FILE NAME ALIGN               Find a place for a file of that size\n"
 	     " print                                Show the contents of the ROM\n\n"