Add flashmap (fmap) support to Flashrom

Add flashmap (fmap) support to Flashrom

This adds flashmap parsing to Flashrom. If an fmap data structure is found, entries will be added to flashrom's internal rom layout.

BUG=chromium-os:115
TEST=Tested on Alex

Testing methodology is similar to what was used in the -i argument patch (git hash d0ea9e), except now fmap data was present in the original bios image:
- Partial write test (automated script)

- Manual partial reads test by reading 4K chunks from ROM and comparing against original BIOS image file and also inspecting the chip-sized file.

- Ensure final chip-size image is generated properly by omitting :file from one of the -i arguments.

- Full read if no -i options are used when fmap is present

- Entire chip written if no -i option is specified and fmap is present. Same holds true if a layout file is specified by no regions are included. A command like "flashrom -w bios.bin" or "flashrom -l layout.txt -w bios.bin" will consistently write the full image to ROM.

- Specifying a non-existent region caused flashrom to fail (expected result).

TODO: Automate all of the above.

[Louis, 2010-12-30 02:57:08]

Thanks for adding this sweet function for user. I'd like to see this convenient
feature in flashrom ASAP. Please see my inlined comments. :-)

http://codereview.chromium.org/6025013/diff/1/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/1/fmap.c#newcode59
fmap.c:59: for (offset = 0; offset < image_len; offset += strides[i]) {
; offset + sizeof(sig) <= image_len;

http://codereview.chromium.org/6025013/diff/1/fmap.c#newcode67
fmap.c:67: if (offset < image_len)
if (offset + sizeof(sig) <= image_len)

http://codereview.chromium.org/6025013/diff/1/fmap.h
File fmap.h (right):

http://codereview.chromium.org/6025013/diff/1/fmap.h#newcode55
fmap.h:55: struct crypto_algo;	/* forward declaration */
remove this until we need it?

http://codereview.chromium.org/6025013/diff/1/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode197
layout.c:197: fmap_offset = fmap_find(image, size);
hm... shall we check the return value for fmap_find()?

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode203
layout.c:203: fmap->areas[i].size;
should be .offset + .size "- 1" since .end is the last byte of partition.

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode204
layout.c:204: if (fmap->areas[i].flags & FMAP_AREA_RO)
hm... does this imply we CANNOT update the whole image when factory?

I would suggest we keep .included = 0 at initial. Then set to 1 if user
specifies -i.

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode208
layout.c:208: romimages++;
Shall we check the romimages if bigger than size of rom_entries?

[dhendrix, 2010-12-31 00:47:02]

Thanks for the review! The CL definitely needs further testing and consideration
for various usage models.

http://codereview.chromium.org/6025013/diff/1/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/1/fmap.c#newcode59
fmap.c:59: for (offset = 0; offset < image_len; offset += strides[i]) {
On 2010/12/30 02:57:08, Louis wrote:
> ; offset + sizeof(sig) <= image_len;

good catch -- done

http://codereview.chromium.org/6025013/diff/1/fmap.c#newcode67
fmap.c:67: if (offset < image_len)
On 2010/12/30 02:57:08, Louis wrote:
> if (offset + sizeof(sig) <= image_len)

Done.

http://codereview.chromium.org/6025013/diff/1/fmap.h
File fmap.h (right):

http://codereview.chromium.org/6025013/diff/1/fmap.h#newcode55
fmap.h:55: struct crypto_algo;	/* forward declaration */
On 2010/12/30 02:57:08, Louis wrote:
> remove this until we need it?

Done.

http://codereview.chromium.org/6025013/diff/1/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode197
layout.c:197: fmap_offset = fmap_find(image, size);
On 2010/12/30 02:57:08, Louis wrote:
> hm... shall we check the return value for fmap_find()?

Yes, definitely! I had only tested using BIOSes with valid fmap structures, so I
didn't hit that condition.

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode203
layout.c:203: fmap->areas[i].size;
On 2010/12/30 02:57:08, Louis wrote:
> should be .offset + .size "- 1" since .end is the last byte of partition.

yes

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode204
layout.c:204: if (fmap->areas[i].flags & FMAP_AREA_RO)
On 2010/12/30 02:57:08, Louis wrote:
> hm... does this imply we CANNOT update the whole image when factory?
> 
> I would suggest we keep .included = 0 at initial. Then set to 1 if user
> specifies -i.

Correct. Using this approach, the way to override the firmware's built-in fmap
would be to use a layout file. So to write the whole ROM, you'd need something
like:

1. echo "0:`flashrom -p internal:bus=spi --get-size` entire_rom" > layout.txt
2. flashrom -p internal:bus=spi -l layout.txt -i entire_rom -w romimage.bin

Your approach has an advantage in the sense that fmap regions will be treated
the same way as layout regions. However, normal firmware updates will require
that the user include each writeable region as a -i option. This is 4 regions on
the current Mario BIOS, and I fear that they may be labeled differently or
changed in future BIOSes.

Both approaches have some significant benefits/disadvantages... Hmmm...

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode208
layout.c:208: romimages++;
On 2010/12/30 02:57:08, Louis wrote:
> Shall we check the romimages if bigger than size of rom_entries?

Yes. I added a check near the top of this loop.

btw -- I just realized that we may need more romimage regions, so I sent an
orthogonal CL hoping that we never exceed MAX_ROMLAYOUT:
http://codereview.chromium.org/6001009

[dhendrix, 2010-12-31 02:37:39]

fyi -- i have done some testing using modified a Mario BIOS image to mark some
regions as "ro" in the fmap data struct. The modified image is @
www/~dhendrix/flashrom_fmap_test_mario.bin .

To test the image, erase the ROM first (-E), then write with -w, then read back
with -r to examine the result.

For testing purposes only, I have patch with some helpful debug statements. We
can try some ideas with the "included" rom images by updating the #if 0
statements left in add_fmap_entries().

My initial results show two things:
1. Using the first approach works as expected. Regions marked as "ro" are not
written.
2. Since "oldcontents" is passed into add_fmap_entries, if the ROM begins in a
blank state or does not contain a valid FMAP structure (ie if vanilla H2O is
used) then the entire ROM will be written. Perhaps this means that the factory
install issue is not a problem after all :-)

[Louis, 2010-12-31 09:13:30]

Thanks for replying. Please kindly see my comments inlined.  :-)

http://codereview.chromium.org/6025013/diff/1/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode204
layout.c:204: if (fmap->areas[i].flags & FMAP_AREA_RO)
On 2010/12/31 00:47:02, dhendrix wrote:
> On 2010/12/30 02:57:08, Louis wrote:
> > hm... does this imply we CANNOT update the whole image when factory?
> > 
> > I would suggest we keep .included = 0 at initial. Then set to 1 if user
> > specifies -i.
> 
> Correct. Using this approach, the way to override the firmware's built-in fmap
> would be to use a layout file. So to write the whole ROM, you'd need something
> like:
> 
> 1. echo "0:`flashrom -p internal:bus=spi --get-size` entire_rom" > layout.txt
> 2. flashrom -p internal:bus=spi -l layout.txt -i entire_rom -w romimage.bin

This changes the behavior too much and becomes more complicated if I just want
to overwrite a H2C bios. I personally don't like this solution.

> 
> Your approach has an advantage in the sense that fmap regions will be treated
> the same way as layout regions. However, normal firmware updates will require
> that the user include each writeable region as a -i option. This is 4 regions
on
> the current Mario BIOS, and I fear that they may be labeled differently or
> changed in future BIOSes.
Specifying -i is always needed because we only update one pair of {FW, sig} at a
time. The firmware updater never overwrites RW A/B at the same time when AU.
> 
> Both approaches have some significant benefits/disadvantages... Hmmm...

I'd like to suggest keeping original convention. Backward compatible: we don't
have to change existing callers (testing code, updater, ...). We earn the good
experience because we don't have to create layout file when fmap is present.

http://codereview.chromium.org/6025013/diff/11001/layout.c#newcode209
layout.c:209: }
remember do this before copy:
memset(rom_entries[romimages].name, 0, sizeof(rom_entries[romimages].name));

[stefanreinauer, 2011-03-04 00:04:17]

LGTM

[dhendrix, 2011-03-04 00:06:17]

Let's revisit this now that we have an actual usage case. Stefan is re-working
some installer scripts, and would like for Flashrom to handle the FMAP logic.

We agree that the usage case you suggest is the correct approach. We will keep
it backward compatible, and not include any FMAP entries by default.

http://codereview.chromium.org/6025013/diff/1/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/1/layout.c#newcode204
layout.c:204: if (fmap->areas[i].flags & FMAP_AREA_RO)
On 2010/12/31 09:13:30, Louis wrote:
> On 2010/12/31 00:47:02, dhendrix wrote:
> > On 2010/12/30 02:57:08, Louis wrote:
> > > hm... does this imply we CANNOT update the whole image when factory?
> > > 
> > > I would suggest we keep .included = 0 at initial. Then set to 1 if user
> > > specifies -i.
> > 
> > Correct. Using this approach, the way to override the firmware's built-in
fmap
> > would be to use a layout file. So to write the whole ROM, you'd need
something
> > like:
> > 
> > 1. echo "0:`flashrom -p internal:bus=spi --get-size` entire_rom" >
layout.txt
> > 2. flashrom -p internal:bus=spi -l layout.txt -i entire_rom -w romimage.bin
> 
> This changes the behavior too much and becomes more complicated if I just want
> to overwrite a H2C bios. I personally don't like this solution.
> 
> > 
> > Your approach has an advantage in the sense that fmap regions will be
treated
> > the same way as layout regions. However, normal firmware updates will
require
> > that the user include each writeable region as a -i option. This is 4
regions
> on
> > the current Mario BIOS, and I fear that they may be labeled differently or
> > changed in future BIOSes.
> Specifying -i is always needed because we only update one pair of {FW, sig} at
a
> time. The firmware updater never overwrites RW A/B at the same time when AU.
> > 
> > Both approaches have some significant benefits/disadvantages... Hmmm...
> 
> I'd like to suggest keeping original convention. Backward compatible: we don't
> have to change existing callers (testing code, updater, ...). We earn the good
> experience because we don't have to create layout file when fmap is present.

Agreed.

http://codereview.chromium.org/6025013/diff/11001/layout.c#newcode209
layout.c:209: }
On 2010/12/31 09:13:30, Louis wrote:
> remember do this before copy:
> memset(rom_entries[romimages].name, 0, sizeof(rom_entries[romimages].name));

Done.

[Louis, 2011-03-04 00:40:59]

Time flies. The requirement for flashrom changed a little. Now we need a fast
read of flashrom for OOBE (to read VPD data). So I made some comments to speed
up the fmap search. This CL is good to me. If you like, you can submit first,
then I will follow up the speed up part. Good work for this sweet function,
flashrom clients will love it.

http://codereview.chromium.org/6025013/diff/24007/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode43
fmap.c:43: long int fmap_find(const uint8_t *image, size_t image_len)
To find out where the fmap is in the fastest way, I'd like to suggest the
following algo:
1. check crossystem/fmap  (code may be not ready yet)
2. pass *flash into this function, rather than *image. So that we can jump every
64KB, 4KB... etc without reading whole flash content which takes about 5 secs.

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode47
fmap.c:47: unsigned int strides[] = { 64, 4 };
Smart!  How about adding 64KB and 4KB, which are the usually cases we align to. 

Please also consider remove 4 (or change to 16) to avoid long loop when fmap is
not existed.

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode57
fmap.c:57: */
Shall we implement a bit array to indicate the address we have checked based on
the fact that the numbers in strides[] are usually multiples? sample code is
like:

visited = malloc(flash->size / min_strides / 8);

for each strides {
  for (offset = 0; ....) {
    if (!(visited[offset/min_strides/8] & (1<<((offset/min_strides)%8)))) {
         uint8_t buf[sizeof(sig)];
         flash->read(offset, sizeof(buf), buf);
         if (!memcmp(buf, &sig, sizeof(sig)) goto fmap_find_exit;
         visited[offset/min_strides/8] |= (1<<((offset/min_strides)%8));
     }
  }
}

http://codereview.chromium.org/6025013/diff/24007/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/24007/layout.c#newcode225
layout.c:225: msg_gdbg("%s(): added region \"%s\" as %sincluded, offset: 0x%08x,
size: 0x%08x\n",
nit: exceeds 80 columns.

http://codereview.chromium.org/6025013/diff/24007/layout.c#newcode291
layout.c:291: add_fmap_entries(oldcontents, size);
I thought add_fmap_entries() should be called around read_romlayout()?

[dhendrix, 2011-03-04 08:04:46]

http://codereview.chromium.org/6025013/diff/24007/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode43
fmap.c:43: long int fmap_find(const uint8_t *image, size_t image_len)
On 2011/03/04 00:40:59, Louis wrote:
> To find out where the fmap is in the fastest way, I'd like to suggest the
> following algo:
> 1. check crossystem/fmap  (code may be not ready yet)
> 2. pass *flash into this function, rather than *image. So that we can jump
every
> 64KB, 4KB... etc without reading whole flash content which takes about 5 secs.

I'm not sure how 1) works. I like the idea in 2). I'll implement that after we
submit this CL.

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode47
fmap.c:47: unsigned int strides[] = { 64, 4 };
On 2011/03/04 00:40:59, Louis wrote:
> Smart!  How about adding 64KB and 4KB, which are the usually cases we align
to. 
> 
> Please also consider remove 4 (or change to 16) to avoid long loop when fmap
is
> not existed.

Done.

http://codereview.chromium.org/6025013/diff/24007/fmap.c#newcode57
fmap.c:57: */
On 2011/03/04 00:40:59, Louis wrote:
> Shall we implement a bit array to indicate the address we have checked based
on
> the fact that the numbers in strides[] are usually multiples? sample code is
> like:
> 
> visited = malloc(flash->size / min_strides / 8);
> 
> for each strides {
>   for (offset = 0; ....) {
>     if (!(visited[offset/min_strides/8] & (1<<((offset/min_strides)%8)))) {
>          uint8_t buf[sizeof(sig)];
>          flash->read(offset, sizeof(buf), buf);
>          if (!memcmp(buf, &sig, sizeof(sig)) goto fmap_find_exit;
>          visited[offset/min_strides/8] |= (1<<((offset/min_strides)%8));
>      }
>   }
> }

done using the mod trick :-)

http://codereview.chromium.org/6025013/diff/24007/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/24007/layout.c#newcode225
layout.c:225: msg_gdbg("%s(): added region \"%s\" as %sincluded, offset: 0x%08x,
size: 0x%08x\n",
On 2011/03/04 00:40:59, Louis wrote:
> nit: exceeds 80 columns.

good catch -- fixed.

http://codereview.chromium.org/6025013/diff/24007/layout.c#newcode291
layout.c:291: add_fmap_entries(oldcontents, size);
On 2011/03/04 00:40:59, Louis wrote:
> I thought add_fmap_entries() should be called around read_romlayout()?

When responding to this comment, I realized that we need to do this
differently... add_fmap_entries() should be called from doit() along with the
new -i argument processing function.

[Louis, 2011-03-04 11:01:13]

LGTM except few minor issues. Please feel free to submit after refine them and
fully test.

Please also modify the BUG and TEST field. Remember to note what combinations
you have tested in TEST field. I'd like to suggest testing like following
sequence:

1. H2C BIOS: flashrom -w H2O.fd
2. H2O BIOS: flashrom -l -i  (partial read/write)
3. H2O BIOS: flashrom -w H2C.fd
4. H2C BIOS: flashrom -i     (partial read/write)
5. H2C BIOS: flashrom -l -i  (partial read/write)
6. H2C BIOS: flashrom -l     (without -i, -w actually does nothing, -r does do
full read)
7. more tests are welcome. :-)

http://codereview.chromium.org/6025013/diff/30003/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/30003/fmap.c#newcode69
fmap.c:69: if (offset % strides[i-1] == 0)
Awesome! Much better than my stupid bitarray approach.

http://codereview.chromium.org/6025013/diff/30003/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode235
layout.c:235: 
strcpy(rom_entries[romimages].file, "");

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode236
layout.c:236: msg_gdbg("%s(): added region \"%s\" as %sincluded, offset: 0x%08x,
"
nit: 
exceed 80 columns.

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode238
layout.c:238: rom_entries[romimages].name,
add [].file?

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode336
layout.c:336: /* add entries for non-readonly regions specified in flashmap */
Why do we limit to "non-readonly" regions?

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode407
layout.c:407: * that the user wants to flash the complete new image.
This is my bad. I forgot to refine this comment. Could you kindly do it for me?
Thanks.

[dhendrix, 2011-03-04 21:46:33]

http://codereview.chromium.org/6025013/diff/30003/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode235
layout.c:235: 
On 2011/03/04 11:01:13, Louis wrote:
> strcpy(rom_entries[romimages].file, "");

Done.

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode236
layout.c:236: msg_gdbg("%s(): added region \"%s\" as %sincluded, offset: 0x%08x,
"
On 2011/03/04 11:01:13, Louis wrote:
> nit: 
> exceed 80 columns.

fixed. thanks for catching that.

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode238
layout.c:238: rom_entries[romimages].name,
On 2011/03/04 11:01:13, Louis wrote:
> add [].file?

Done.

I also updated this debug string to omit the function name (since it's long and
clutters the output).

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode336
layout.c:336: /* add entries for non-readonly regions specified in flashmap */
On 2011/03/04 11:01:13, Louis wrote:
> Why do we limit to "non-readonly" regions?

I believe that is an obsolete comment, so I have removed it.

The first version of this patch changed default behavior so that non-readonly
regions were included automatically:
+		if (fmap->areas[i].flags & FMAP_AREA_RO)
+			rom_entries[romimages].included = 0;
+		else
+			rom_entries[romimages].included = 1;

We recently decided that your suggestion to maintain backwards compatibility is
the better approach :-)

http://codereview.chromium.org/6025013/diff/30003/layout.c#newcode407
layout.c:407: * that the user wants to flash the complete new image.
On 2011/03/04 11:01:13, Louis wrote:
> This is my bad. I forgot to refine this comment. Could you kindly do it for
me?
> Thanks.

Heh, I also noticed that this comment needs to be updated, but didn't want to
introduce more noise into this CL.

Let's update the comment in another CL.

[Louis, 2011-03-04 23:33:16]

Did you accidentally include code from CL 6625011?

http://codereview.chromium.org/6025013/diff/31010/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/31010/layout.c#newcode52
layout.c:52: static char *include_args[MAX_ROMLAYOUT];
Did you accidentally include code from CL 6625011?

http://codereview.chromium.org/6025013/diff/31010/layout.c#newcode384
layout.c:384: /* add entries for non-readonly regions specified in flashmap */
Oooops. I guess you forgot to remove this comment.  :-)

[dhendrix, 2011-03-05 00:25:30]

One more big iteration on this CL :-) Now add_fmap_entries is called from
doit(). The fmap_find() function has been refactored so now it reads the ROM
directly instead of using a full BIOS image. Also, this patch makes fmap work
with partial reads.

Unfortunately on Alex the FMAP structure is located in kind of a weird location.
So it still takes a while to find it. Maybe we can change that in the next BIOS
release.

I'll add some testing notes to the CL description shortly.

http://codereview.chromium.org/6025013/diff/31010/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/31010/layout.c#newcode52
layout.c:52: static char *include_args[MAX_ROMLAYOUT];
On 2011/03/04 23:33:16, Louis wrote:
> Did you accidentally include code from CL 6625011?

I think that is due to git... I have this patch set up in a branch that tracks
the other CL, so "git cl" might think that the change belongs in both patches.

http://codereview.chromium.org/6025013/diff/31010/layout.c#newcode384
layout.c:384: /* add entries for non-readonly regions specified in flashmap */
On 2011/03/04 23:33:16, Louis wrote:
> Oooops. I guess you forgot to remove this comment.  :-)

Fixed (note: this call and the updated comment are in doit() in flashrom.c now)

[Louis, 2011-03-05 01:05:01]

SuperB. I like your fmap->read() way! Some suggestion inlined.

Could you use 'time flashrom' to measure the reading time on Alex? I am curious
how fast compared to original design?

http://codereview.chromium.org/6025013/diff/34007/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/34007/fmap.c#newcode50
fmap.c:50: unsigned int strides[] = { 64 * 1024, 4 * 1024, 64 };
Feel free to add more granularities here since we have your awesome mod trick.
:-)

Actually the most optimized strides sequence would look like this:
  { flash_size / 2, ..., 1M, 512K, 256K, ..., sizeof(siganature)}
See below for sample code.

http://codereview.chromium.org/6025013/diff/34007/fmap.c#newcode67
fmap.c:67: for (offset = 0;
hehehe... why not try the following way based on the assumption that RO VPD
usually rely on top of BIOS image.

for (stride = flash->total_size * 1024 / 2;
     stride >= sizeof(sig);
     stride >>= 1) {

    for (offset = flash->total_size * 1024 - strides[i];
         offset >= 0;
         offset -= stride) {

         // mod trick here

    }
}

http://codereview.chromium.org/6025013/diff/34007/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/34007/layout.c#newcode260
layout.c:260: 
buf must be freed.

[dhendrix, 2011-03-05 02:34:14]

http://codereview.chromium.org/6025013/diff/34007/fmap.c
File fmap.c (right):

http://codereview.chromium.org/6025013/diff/34007/fmap.c#newcode50
fmap.c:50: unsigned int strides[] = { 64 * 1024, 4 * 1024, 64 };
On 2011/03/05 01:05:01, Louis wrote:
> Feel free to add more granularities here since we have your awesome mod trick.
> :-)
> 
> Actually the most optimized strides sequence would look like this:
>   { flash_size / 2, ..., 1M, 512K, 256K, ..., sizeof(siganature)}
> See below for sample code.

That's a good idea, though I don't think it will help much for the current ROM
images since the FMAP is in a (multiple of 64, but not 128, 1024, etc...).

http://codereview.chromium.org/6025013/diff/34007/fmap.c#newcode67
fmap.c:67: for (offset = 0;
On 2011/03/05 01:05:01, Louis wrote:
> hehehe... why not try the following way based on the assumption that RO VPD
> usually rely on top of BIOS image.
> 
> for (stride = flash->total_size * 1024 / 2;
>      stride >= sizeof(sig);
>      stride >>= 1) {
> 
>     for (offset = flash->total_size * 1024 - strides[i];
>          offset >= 0;
>          offset -= stride) {
> 
>          // mod trick here
> 
>     }
> }

Good thinking. I only incorporated the suggestion to start from top-of-ROM so
far, though. This dramatically sped up my test for reading the RO_VPD region:
1. Old approach which read whole ROM first: 4.998s
2. New approach which uses flash->read() in this function: 4.437s
3. Your approach which starts seeking at top-of-ROM: 1.751s.

http://codereview.chromium.org/6025013/diff/34007/layout.c
File layout.c (right):

http://codereview.chromium.org/6025013/diff/34007/layout.c#newcode260
layout.c:260: 
On 2011/03/05 01:05:01, Louis wrote:
> buf must be freed.

d'oh! good catch. I also added free(buf) before line 228 (in case of error).

[dhendrix, 2011-03-05 02:43:56]

I tweaked fmap_find() a little further. Now it only reads 8 bytes for the
signature on every stride, rather than reading 56 bytes for the main fmap
structure.

This brought the time down from 1.753s to 1.273s :-)

[Louis, 2011-03-06 09:34:51]

Amazing. Now we reduce from 4.998s --> 1.273sec. Pretty impressive.  :-)

By the way, Stefan reminds us that once fmap is supported, full write should be
successful by the following command:

  flashrom -w bios.bin

My suggested test command 1 should catch out this issue. Let's fix. An idea is
that we add a counter for -i argument, if no -i is specified, we set all regions
to included=1.

On 2011/03/05 02:43:56, dhendrix wrote:
> I tweaked fmap_find() a little further. Now it only reads 8 bytes for the
> signature on every stride, rather than reading 56 bytes for the main fmap
> structure.
> 
> This brought the time down from 1.753s to 1.273s :-)

[Stefan Reinauer, 2011-03-07 18:58:58]

Alternatively, we could ignore .included if the counter is zero. Whatever is
easier.

[dhendrix, 2011-03-08 03:34:37]

On 2011/03/07 18:58:58, Stefan Reinauer wrote:
> Alternatively, we could ignore .included if the counter is zero. Whatever is
> easier.

Yep, good call. Only now that we have the patch for adding -i arguments to an
array, we can check if (!include_args[0]).

Preliminary results look good. I'll post some testing results soon.

[dhendrix, 2011-03-08 05:13:31]

On 2011/03/08 03:34:37, dhendrix wrote:
> On 2011/03/07 18:58:58, Stefan Reinauer wrote:
> > Alternatively, we could ignore .included if the counter is zero. Whatever is
> > easier.
> 
> Yep, good call. Only now that we have the patch for adding -i arguments to an
> array, we can check if (!include_args[0]).
> 
> Preliminary results look good. I'll post some testing results soon.

Okay, I finally finished all the tests and everything looked good! See the
updated CL description for some details. I don't want to drag this CL on any
longer, especially since Stefan needs it, so I'll go ahead and commit it based
on earlier feedback.

There are a couple outstanding issues:
- The strides used for searching for the fmap signature in ROM should be
automatically generated, based on the algorithm Louis described earlier.

- I tried using the FMAP file in sysfs that is exported via ACPI in Chrome OS,
but hit a situation where the file read back a weird value. We should add
support for using that file once we figure out exactly how...