Add firmware image packing tool

Add firmware image packing tool

BUG=chromium-os:1302
TEST=manual

(cros) $ cd vboot_reference/utility/
(cros) $ cat > test_config <<EOF
KEYDIR = '/usr/share/vboot/devkeys/'
OUTPUT = 'image.bin'
SIZE = 1 << 20 # 1 MB
ENTRIES = [
  EntryFmap(name='FMAP',
        offset=0x00000000, length=0x00001000,
        ver_major=1, ver_minor=0, base=0x00000000, size=SIZE),
  EntryKeyBlock(name='Firmware A Key',
        offset=0x00010000, length=0x00010000,
        flags=FMAP_AREA_STATIC,
        keyblock=KEYDIR + 'firmware.keyblock',
        signprivate=KEYDIR + 'firmware_data_key.vbprivk',
        version=1,
        fv=INPUT_FILE,
        kernelkey=KEYDIR + 'kernel_subkey.vbpubk'),
  EntryBlob(name='Test Data',
        offset=0x00020000, length=0x000e0000,
        flags=FMAP_AREA_STATIC | FMAP_AREA_COMPRESSED,
        path=INPUT_FILE),
]
EOF
(cros) $ ./pack_firmware_image test_config INPUT_FILE=./pack_firmware_image; echo $?
0
(cros) $ dump_fmap image.bin
opened image.bin
hit at 0x00000000
fmap_signature   __FMAP__
fmap_version:    1.0
fmap_base:       0x0
fmap_size:       0x00100000 (1048576)
fmap_name:       FMAP
fmap_nareas:     2
area:            1
area_offset:     0x00010000
area_size:       0x00010000 (65536)
area_name:       Firmware A Key
area:            2
area_offset:     0x00020000
area_size:       0x000e0000 (917504)
area_name:       Test Data

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=b0310a7

[Hung-Te, 2011-01-04 08:39:02]

http://codereview.chromium.org/5985009/diff/1/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode7
utility/pack_image:7: import mmap
Any reason to use mmap? I think this script does not really need mmap - file i/o
+ python native array would work.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
The fmap has an python implementation, supporting both encode and decode. We
should reuse the official source as much as possible. In other words, "import
fmap" would be better instead of merging fmap python code in this script.

If you think the official one cannot support what you want to do, please send a
patch to David Hendricks.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode44
utility/pack_image:44: RE_DECIMAL = re.compile(r'^[1-9][0-9]*$')
int(a, 0) prevents the use of RE_HEXADECIMAL and RE_DECIMAL.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode47
utility/pack_image:47: 
Please make 2 blank lines for top level definitions (and for all over this
file).

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode181
utility/pack_image:181: if expr[0] == expr[-1] == '"' or expr[0] == expr[-1] ==
"'":
To prevent confusion and help readbility, please add the ()'s  for (expr1) or
(expr2).

Also, startswith and endswith are more readable than a[0] == a[-1] == 'blah'.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode188
utility/pack_image:188: return expr # if not a number, interpret as string
literals
I'd prefer a more strict syntax, ex: every string must be quoted, otherwise it's
a hex/decimal.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode197
utility/pack_image:197: # create image so that mmap will succeed
If you use simply buffer processing without mmap, there's no need for such trick
to touch file.

[Che-Liang Chiou, 2011-01-04 09:38:25]

Thanks for the comments.  Please see responds below.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode7
utility/pack_image:7: import mmap
On 2011/01/04 08:39:03, Hung-Te wrote:
> Any reason to use mmap? I think this script does not really need mmap - file
i/o
> + python native array would work.

Because it's supposedly faster.  Do you have strong reason against mmap?

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
On 2011/01/04 08:39:03, Hung-Te wrote:
> The fmap has an python implementation, supporting both encode and decode. We
> should reuse the official source as much as possible. In other words, "import
> fmap" would be better instead of merging fmap python code in this script.
> 
> If you think the official one cannot support what you want to do, please send
a
> patch to David Hendricks.

I agree, but it should be done in a separate CL because there might be objection
(though unlikely) on pulling David's fmap implementation into chromium os code
base.  I will add a TODO here and rewrite this after we can and does pull in the
fmap.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode44
utility/pack_image:44: RE_DECIMAL = re.compile(r'^[1-9][0-9]*$')
On 2011/01/04 08:39:03, Hung-Te wrote:
> int(a, 0) prevents the use of RE_HEXADECIMAL and RE_DECIMAL.

Done, but this would add one more try-catch block though.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode47
utility/pack_image:47: 
On 2011/01/04 08:39:03, Hung-Te wrote:
> Please make 2 blank lines for top level definitions (and for all over this
> file).

Done.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode181
utility/pack_image:181: if expr[0] == expr[-1] == '"' or expr[0] == expr[-1] ==
"'":
On 2011/01/04 08:39:03, Hung-Te wrote:
> To prevent confusion and help readbility, please add the ()'s  for (expr1) or
> (expr2).
> 
> Also, startswith and endswith are more readable than a[0] == a[-1] == 'blah'.

Done.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode188
utility/pack_image:188: return expr # if not a number, interpret as string
literals
On 2011/01/04 08:39:03, Hung-Te wrote:
> I'd prefer a more strict syntax, ex: every string must be quoted, otherwise
it's
> a hex/decimal.

That's not a good idea.  shell will unquote command line arguments before firing
processes.
$ echo -e 'import sys\nprint sys.argv' | python - "hello" "world"
['-', 'hello', 'world']

Or do you suggest we double-quote strings on command line?  I don't prefer this
way though.
$ echo -e 'import sys\nprint sys.argv' | python - "'hello'" "'world'"

[Hung-Te, 2011-01-04 10:05:23]

http://codereview.chromium.org/5985009/diff/1/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode7
utility/pack_image:7: import mmap
On 2011/01/04 09:38:25, Che-Liang Chiou wrote:
> > Any reason to use mmap? 
> Because it's supposedly faster.
  I think performance is not really critical for this script. mmap is more
implementation dependent and may have potential compatibility issues (at least,
forced you to make a hack by touching the file), so I'd prefer general file i/o
which is also easier for maintenance.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
On 2011/01/04 09:38:25, Che-Liang Chiou wrote:
> there might be objection (though unlikely) on
> pulling David's fmap implementation into chromium
> os code base.
   We already have that in autotest common lib folder.
   Unless you need lots of changes to the official fmap, otherwise I think we
should stop creating more fmap implementation variants.

   Maybe it's a good time to add flashmap as "host-depends" so that developers
can use it (both python and C library).

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode188
utility/pack_image:188: return expr # if not a number, interpret as string
literals
If you have only 3 parameters, maybe we should consider use getopt and list them
as --opt, instead of the format A=B.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
Please use "official" instead of "David's"

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode194
utility/pack_image:194: raise PackError('overlapped entries: [%08x:%08x],
[%08x:%08x]' %
You can use implicit multi-line expression, like
 ((a and b) or
  (b and c))

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode196
utility/pack_image:196: 
return expr[1:-1] ?
Otherwise it'll be converted to integer agian.

[gauravsh, 2011-01-04 21:14:38]

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode1
utility/pack_image:1: #!/usr/bin/env python2.6
make this version independent

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode3
utility/pack_image:3: # Copyright (c) 2010 The Chromium OS Authors. All rights
reserved.
2011

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode58
utility/pack_image:58: class Entry(dict):
general comment our python style - Chromium OS follows PEP-8 but with Mixed case
method and function names and 2 space instead of 4 space indent. Your indent is
fine, but the method names are not.

Please make the change for the entire file.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode74
utility/pack_image:74: return entry.offset <= self.offset < entry.offset +
entry.length or \
you can get rid of the \ and use implicit line breaking by parenthesizing it (as
Hung-Te suggested elsewhere). This will make this easier to read too.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode136
utility/pack_image:136: fvimg = mmap.mmap(f.fileno(), size, mmap.MAP_PRIVATE,
mmap.PROT_READ)
i am not sure the performance improvement from mmap is really needed here.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode165
utility/pack_image:165: stdout = None
this could be declared as a class variable right at the beginning of class
definition, you won't need the else block then.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode174
utility/pack_image:174: img = mmap.mmap(fd, 0, mmap.MAP_PRIVATE, mmap.PROT_READ)
in general, Google style recommends not using short forms for variable names
(i.e. image is better than img)

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode211
utility/pack_image:211: with open(output_path, 'w+b') as f:
again don't use 'f' here. Use 'file'. The only place where you short forms var
names are recommended are standard loop index variables like 'i'.

[Hung-Te, 2011-01-05 03:33:54]

I agree that we can temporary allow the duplication of fmap implementation in
this script (due to coding style, license, ebuild issue, ...), in order to
prevent blocking development of ARM platforms. Please try to make your interface
like the official version fmap so in the future it will be easy to migrate.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode1
utility/pack_image:1: #!/usr/bin/env python2.6
That reminds me something. Che-Liang used "with" which is supported since 2.6
(in 2.5, some special import is required).

Do we have some policy or coding style suggestion for such situation?

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode211
utility/pack_image:211: with open(output_path, 'w+b') as f:
P.S: 'file' is a special keyword in python -- some other things like image_file
would be even better.

[Che-Liang Chiou, 2011-01-05 08:01:54]

Thanks for the comments, Hung-Te and Gaurav.  Please see below.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode7
utility/pack_image:7: import mmap
On 2011/01/04 10:05:23, Hung-Te wrote:
> On 2011/01/04 09:38:25, Che-Liang Chiou wrote:
> > > Any reason to use mmap? 
> > Because it's supposedly faster.
>   I think performance is not really critical for this script. mmap is more
> implementation dependent and may have potential compatibility issues (at
least,
> forced you to make a hack by touching the file), so I'd prefer general file
i/o
> which is also easier for maintenance.
> 

Done.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
On 2011/01/04 10:05:23, Hung-Te wrote:
> On 2011/01/04 09:38:25, Che-Liang Chiou wrote:
> > there might be objection (though unlikely) on
> > pulling David's fmap implementation into chromium
> > os code base.
>    We already have that in autotest common lib folder.
>    Unless you need lots of changes to the official fmap, otherwise I think we
> should stop creating more fmap implementation variants.
> 
>    Maybe it's a good time to add flashmap as "host-depends" so that developers
> can use it (both python and C library).
>    

Done.

http://codereview.chromium.org/5985009/diff/1/utility/pack_image#newcode197
utility/pack_image:197: # create image so that mmap will succeed
On 2011/01/04 08:39:03, Hung-Te wrote:
> If you use simply buffer processing without mmap, there's no need for such
trick
> to touch file.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode1
utility/pack_image:1: #!/usr/bin/env python2.6
On 2011/01/05 03:33:54, Hung-Te wrote:
> That reminds me something. Che-Liang used "with" which is supported since 2.6
> (in 2.5, some special import is required).
> 
> Do we have some policy or coding style suggestion for such situation?

The coding style asks for the shebang line with a specific version. 
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Python...

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode3
utility/pack_image:3: # Copyright (c) 2010 The Chromium OS Authors. All rights
reserved.
On 2011/01/04 21:14:39, gauravsh wrote:
> 2011

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode15
utility/pack_image:15: FMAP_SIGNATURE = "__FMAP__"
On 2011/01/04 10:05:23, Hung-Te wrote:
> Please use "official" instead of "David's"

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode58
utility/pack_image:58: if f not in kwargs:
On 2011/01/04 21:14:39, gauravsh wrote:
> general comment our python style - Chromium OS follows PEP-8 but with Mixed
case
> method and function names and 2 space instead of 4 space indent. Your indent
is
> fine, but the method names are not.
> 
> Please make the change for the entire file.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode74
utility/pack_image:74: 
On 2011/01/04 21:14:39, gauravsh wrote:
> you can get rid of the \ and use implicit line breaking by parenthesizing it
(as
> Hung-Te suggested elsewhere). This will make this easier to read too.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode136
utility/pack_image:136: super(EntryKeyBlock, self).__init__(**kwargs)
On 2011/01/04 21:14:39, gauravsh wrote:
> i am not sure the performance improvement from mmap is really needed here.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode165
utility/pack_image:165: if size > self.length:
On 2011/01/04 21:14:39, gauravsh wrote:
> this could be declared as a class variable right at the beginning of class
> definition, you won't need the else block then.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode174
utility/pack_image:174: def parse_assignment(stmt):
On 2011/01/04 21:14:39, gauravsh wrote:
> in general, Google style recommends not using short forms for variable names
> (i.e. image is better than img)

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode194
utility/pack_image:194: raise PackError('overlapped entries: [%08x:%08x],
[%08x:%08x]' %
On 2011/01/04 10:05:23, Hung-Te wrote:
> You can use implicit multi-line expression, like
>  ((a and b) or
>   (b and c))

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode196
utility/pack_image:196: 
On 2011/01/04 10:05:23, Hung-Te wrote:
> return expr[1:-1] ?
> Otherwise it'll be converted to integer agian.

Done.

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode211
utility/pack_image:211: print >>sys.stderr, 'INFO: %s' % msg
On 2011/01/04 21:14:39, gauravsh wrote:
> again don't use 'f' here. Use 'file'. The only place where you short forms var
> names are recommended are standard loop index variables like 'i'. 

Done; avoid use 'file'.

[Hung-Te, 2011-01-05 10:21:00]

LGTM with some nits about indents.

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode77
utility/pack_image:77: self.offset <= entry.offset < self.offset + self.length)
please re-indent this line to match parentheses location

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode95
utility/pack_image:95: for name in FMAP_AREA_NAMES))
please re-indent this line to match parentheses location

[Hung-Te, 2011-01-05 10:28:27]

Wait, there's a final piece. The script file name should be
"pack_firmware_image.py" or some names with 'firmware' in it.

[gauravsh, 2011-01-06 02:06:45]

lgtm for my nits

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/8001/utility/pack_image#newcode1
utility/pack_image:1: #!/usr/bin/env python2.6
On 2011/01/05 08:01:55, Che-Liang Chiou wrote:
> On 2011/01/05 03:33:54, Hung-Te wrote:
> > That reminds me something. Che-Liang used "with" which is supported since
2.6
> > (in 2.5, some special import is required).
> > 
> > Do we have some policy or coding style suggestion for such situation?
> 
> The coding style asks for the shebang line with a specific version. 
>
http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Python...

Chromium OS follows PEP-8, not Google python style. But since PEP-8 doesn't say
anything about this, and you use 2.6 features, this is fine.

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode187
utility/pack_image:187: m = RE_ASSIGNMENT.match(stmt)
nit: use CamelCase for method names

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode188
utility/pack_image:188: if m is None:
same here

[Che-Liang Chiou, 2011-01-06 03:01:50]

Thanks for comments.  Also change its name to 'pack_firmware_image'

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image
File utility/pack_image (right):

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode77
utility/pack_image:77: self.offset <= entry.offset < self.offset + self.length)
On 2011/01/05 10:21:01, Hung-Te wrote:
> please re-indent this line to match parentheses location

Done.

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode95
utility/pack_image:95: for name in FMAP_AREA_NAMES))
On 2011/01/05 10:21:01, Hung-Te wrote:
> please re-indent this line to match parentheses location

Done.

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode187
utility/pack_image:187: def fmap_encode(obj):
On 2011/01/06 02:06:45, gauravsh wrote:
> nit: use CamelCase for method names

fmap_encode() is named for begin compatible with official flashmap
implementation's fmap_encode() function based on an offline discussion with
Hung-Te so that we can migrate to the official flashmap implementation.

http://codereview.chromium.org/5985009/diff/16001/utility/pack_image#newcode188
utility/pack_image:188: def _format_blob(format, names, obj):
On 2011/01/06 02:06:45, gauravsh wrote:
> same here

Done.