Add flashmap (fmap) support to Flashrom

layout.c

Index: layout.c
diff --git a/layout.c b/layout.c
index 0dbb556c64ff8b8ceb74742ecf3ab1853a3fdb4f..ae4ef5752a625e8d78f3b1b7d91bbb89f565a9b8 100644
--- a/layout.c
+++ b/layout.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2005-2008 coresystems GmbH
  * (Written by Stefan Reinauer <stepan@coresystems.de> for coresystems GmbH)
+ * Portions (C) 2010 Google Inc.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -24,6 +25,7 @@
 #include <ctype.h>
 #include <limits.h>
 #include "flash.h"
+#include "fmap.h"
 #include "programmer.h"
 
 #if CONFIG_INTERNAL == 1
@@ -185,6 +187,31 @@ int read_romlayout(char *name)
 }
 #endif
 
+/* returns the number of entries added */
+static int add_fmap_entries(uint8_t *image, int size)
+{
+	int i;
+	struct fmap *fmap;
+	long int fmap_offset;
+
+	fmap_offset = fmap_find(image, size);

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

hm... shall we check the return value for fmap_find()?

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

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

+	fmap = (struct fmap *)(image + fmap_offset);;
+
+	for (i = 0; i < fmap->nareas; i++) {
+		rom_entries[romimages].start = fmap->areas[i].offset;
+		rom_entries[romimages].end = fmap->areas[i].offset +
+		                             fmap->areas[i].size;

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

should be .offset + .size "- 1" since .end is the last byte of partition.

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

yes

+		if (fmap->areas[i].flags & FMAP_AREA_RO)

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

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.

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

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...

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

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.
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.
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.

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

Agreed.

+			rom_entries[romimages].included = 0;
+		else
+			rom_entries[romimages].included = 1;
+		romimages++;

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

Shall we check the romimages if bigger than size of rom_entries?

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

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

+	}
+
+	return fmap->nareas;
+}
+
+
 int find_romentry(char *name)
 {
 	int i;
@@ -237,6 +264,9 @@ int handle_romentries(struct flashchip *flash, uint8_t *oldcontents, uint8_t *ne
 	int entry;
 	unsigned int size = flash->total_size * 1024;
 
+	/* add entries for non-readonly regions specified in flashmap */
+	add_fmap_entries(oldcontents, size);
+
 	/* If no layout file was specified or the layout file was empty, assume
 	 * that the user wants to flash the complete new image.
 	 */