David Hendricks would like Louis Yung-Chieh Lo to review this change.
make args for -r/-w/-v non-positional and optional
Ported from chromiumos:
https://chromium-review.googlesource.com/#/c/60515/
This makes a filename following -r, -w, and -v operations non-
positional. The first argument that does not begin with a hyphen (-)
is the filename for -r/-w/-v. If no such argument exists, then
-r/-w/-v options apply to files specified for individually included
regions via -i.
This has a few side-effects:
1. It allows us to omit the ROM-sized filename entirely when a
filename is provided for a particular region when using -i. For
example, "flashrom -i FOO:foo.bin -r".
2. It allows, for better or worse, the filename be specified anywhere
on the command line after the program name. Our scripts and docs
should still specify the file after -r/-w/-v for clarity.
For our purposes, if a filename is given for every included region
(-i region:filename) then the user does not need to specify a filename
after -r/-w/-v. However, if any -i option does not specify a filename,
then a file must be specified after -r/-w/-v.
The syntax will be backwards compatible for now so that one can still
mix -i options with and without the added filename specifier for each
region.
BUG=chromium:263495
BRANCH=none
TEST=manual (see notes below)
1. Write random data to RW_UNUSED region (on snow in this case)
without requiring an argument to -w. See that only RW_UNUSED
is erased and written, and that verify works:
dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc
flashrom -p host -i RW_UNUSED:rw_unused.bin -w -V
flashrom -p host -i RW_UNUSED:rw_unused.bin -v -V
2. Same as above, but specify a dummy file to test syntax
backwards compatibility:
dd if=/dev/urandom of=rw_unused.bin bs=1k count=1 conv=notrunc
dd if=/dev/urandom of=random_4M.bin bs=1M count=4
flashrom -p host -i RW_UNUSED:rw_unused.bin -w random_4M.bin -V
flashrom -p host -i RW_UNUSED:rw_unused.bin -v random_4M.bin -V
3. Test that dumping RW_UNUSED and GBB regions without -r arg dumps
two files and that they can be used to verify the content:
flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r
flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v
4. Same as above, but with dummy file:
flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -r x.bin
flashrom -p host -i RW_UNUSED:rw_unused.bin -i GBB:gbb.bin -v x.bin
Change-Id: Iefbcb7dc4fefe26f5afd1292dfd5c1687fa62803
Reviewed-on: https://gerrit.chromium.org/gerrit/60515
Reviewed-by: Yung-Chieh Lo <yjlou@chromium.org>
Commit-Queue: David Hendricks <dhendrix@chromium.org>
Tested-by: David Hendricks <dhendrix@chromium.org>
---
M cli_classic.c
M flashrom.8.tmpl
M flashrom.c
M layout.c
M layout.h
5 files changed, 232 insertions(+), 32 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/22/23022/1
diff --git a/cli_classic.c b/cli_classic.c
index c1481ec..9939b98 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -111,12 +111,12 @@
enum programmer prog = PROGRAMMER_INVALID;
int ret = 0;
- static const char optstring[] = "r:Rw:v:nNVEfc:l:i:p:Lzho:";
+ static const char optstring[] = "rRwvnNVEfc:l:i:p:Lzho:";
static const struct option long_options[] = {
- {"read", 1, NULL, 'r'},
- {"write", 1, NULL, 'w'},
+ {"read", 0, NULL, 'r'},
+ {"write", 0, NULL, 'w'},
{"erase", 0, NULL, 'E'},
- {"verify", 1, NULL, 'v'},
+ {"verify", 0, NULL, 'v'},
{"noverify", 0, NULL, 'n'},
{"noverify-all", 0, NULL, 'N'},
{"chip", 1, NULL, 'c'},
@@ -163,7 +163,6 @@
"specified. Aborting.\n");
cli_classic_abort_usage();
}
- filename = strdup(optarg);
read_it = 1;
break;
case 'w':
@@ -172,7 +171,6 @@
"specified. Aborting.\n");
cli_classic_abort_usage();
}
- filename = strdup(optarg);
write_it = 1;
break;
case 'v':
@@ -186,7 +184,6 @@
fprintf(stderr, "--verify and --noverify are mutually exclusive. Aborting.\n");
cli_classic_abort_usage();
}
- filename = strdup(optarg);
verify_it = 1;
break;
case 'n':
@@ -344,12 +341,12 @@
}
}
- if (optind < argc) {
- fprintf(stderr, "Error: Extra parameter found.\n");
- cli_classic_abort_usage();
+ if (read_it || write_it || verify_it) {
+ if (argv[optind])
+ filename = strdup(argv[optind]);
}
- if ((read_it | write_it | verify_it) && check_filename(filename, "image")) {
+ if ((read_it | write_it | verify_it) && filename && check_filename(filename, "image")) {
cli_classic_abort_usage();
}
if (layoutfile && check_filename(layoutfile, "layout")) {
@@ -560,6 +557,71 @@
flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_AFTER_WRITE, !dont_verify_it);
flashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, !dont_verify_all);
+ /*
+ * Common rules for -r/-w/-v syntax parsing:
+ * - If no filename is specified at all, quit.
+ * - If no filename is specified for -r/-w/-v, but files are specified
+ * for -i, then the number of file arguments for -i options must be
+ * equal to the total number of -i options.
+ *
+ * Rules for reading:
+ * - If files are specified for -i args but not -r, do partial reads for
+ * each -i arg, creating a new file for each region. Each -i option
+ * must specify a filename.
+ * - If filenames are specified for -r and -i args, then:
+ * - Do partial read for each -i arg, creating a new file for
+ * each region where a filename is provided (-i region:filename).
+ * - Create a ROM-sized file with partially filled content. For each
+ * -i arg, fill the corresponding offset with content from ROM.
+ *
+ * Rules for writing and verifying:
+ * - If files are specified for both -w/-v and -i args, -i files take
+ * priority. (Note: We determined this was the most useful syntax for
+ * chromium.org's flashrom after some discussion. Upstream may wish
+ * to quit in this case due to ambiguity).
+ * See: http://crbug.com/263495.
+ * - If file is specified for -w/-v and no files are specified with -i
+ * args, then the file is to be used for writing/verifying the entire
+ * ROM.
+ * - If files are specified for -i args but not -w, do partial writes
+ * for each -i arg. Likewise for -v and -i args. All -i args must
+ * supply a filename. Any omission is considered ambiguous.
+ * - Regions with a filename associated must not overlap. This is also
+ * considered ambiguous. Note: This is checked later since it requires
+ * processing the layout/fmap first.
+ */
+ if (read_it || write_it || verify_it) {
+ char op = 0;
+
+ if (read_it)
+ op = 'r';
+ else if (write_it)
+ op = 'w';
+ else if (verify_it)
+ op = 'v';
+
+ if (!filename) {
+ if (layout->num_entries == 0) {
+ msg_gerr("Error: No file specified for -%c.\n", op);
+ ret = 1;
+ goto out_shutdown;
+ }
+
+ for (i = 0; i < layout->num_entries; i++) {
+ if (!layout->entries[i].included)
+ continue;
+ char *const file = layout->entries[i].file;
+ if (!file || !strlen(file)) {
+ msg_gerr("Error: Missing filename for region \"%s\"\n", file);
+ ret = 1;
+ }
+ }
+
+ if (ret)
+ goto out_shutdown;
+ }
+ }
+
/* FIXME: We should issue an unconditional chip reset here. This can be
* done once we have a .reset function in struct flashchip.
* Give the chip time to settle.
diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl
index 6a05e8a..6e1ba7b 100644
--- a/flashrom.8.tmpl
+++ b/flashrom.8.tmpl
@@ -46,7 +46,7 @@
.SH SYNOPSIS
.B flashrom \fR[\fB\-h\fR|\fB\-R\fR|\fB\-L\fR|\fB\-z\fR|\
\fB\-p\fR <programmername>[:<parameters>]
- [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] \
+ [\fB\-E\fR|\fB\-r\fR [file]|\fB\-w\fR [file]|\fB\-v\fR <file>] \
[\fB\-c\fR <chipname>]
[(\fB\-l\fR <file>|\fB\-\-ifd\fR) [\fB\-i\fR <image>[:<file>]]] \
[\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR]]
@@ -81,17 +81,29 @@
.B -p/--programmer
option to be used (please see below).
.TP
-.B "\-r, \-\-read <file>"
-Read flash ROM contents and save them into the given
-.BR <file> .
-If the file already exists, it will be overwritten.
+.B "\-r, \-\-read [file]"
+Read flash ROM contents. If
+.BR [file]
+is specified ROM contents will be saved to it. If the file already exists, it
+will be overwritten.
+.sp
+Alternatively,
+.B "\-i"
+may be used to output files for specific regions only.
.TP
-.B "\-w, \-\-write <file>"
-Write
-.B <file>
-into flash ROM. This will first automatically
+.B "\-w, \-\-write [file]"
+Write contents to ROM. If
+.B [file]
+is specified then its contents will be written to the ROM. See
+.B "\-i"
+option for information about using
+.B "\-i"
+with
+.B "\-w."
+.sp
+Write operations will erase
.B erase
-the chip, then write to it.
+necessary regions of chip before writing to it.
.sp
In the process the chip is also read several times. First an in-memory backup
is made for disaster recovery and to be able to skip regions that are
@@ -240,7 +252,7 @@
into region-sized files
.BR "foo.bin " and " bar.bin" ", run:
.sp
-.B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -r rom.bin
+.B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -r
.sp
To write files
.BR "foo.bin " and " bar.bin"
@@ -249,7 +261,7 @@
.BR <layout>
to the ROM, run:
.sp
-.B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -w rom.bin
+.B " flashrom \-p prog \-l <layout> \-i foo:foo.bin -i bar:bar.bin -w
.TP
.B "\-L, \-\-list\-supported"
List the flash chips, chipsets, mainboards, and external programmers
diff --git a/flashrom.c b/flashrom.c
index 4f50c54..5035f83 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -1356,6 +1356,75 @@
#endif
}
+static int read_buf_from_include_args(const struct flashctx *const flash, unsigned char *buf, unsigned long size)
+{
+#ifdef __LIBPAYLOAD__
+ msg_gerr("Error: No file I/O support in libpayload\n");
+ return 1;
+#else
+ int ret = 0, i;
+ const struct flashrom_layout *const layout = get_layout(flash);
+ const struct romentry *entry;
+
+ /*
+ * Content will be read from -i args, so they must not overlap since
+ * we need to know exactly what content to write to the ROM.
+ */
+ if (included_regions_overlap(layout)) {
+ msg_gerr("Error: Included regions must not overlap when writing.\n");
+ return 1;
+ }
+
+ /* sanity check region boundaries before accessing the chip */
+ for (i = 0; i < layout->num_entries; i++) {
+ entry = &layout->entries[i];
+ if (!entry->included)
+ continue;
+
+ if ((entry->start > size) || (entry->end > size)) {
+ msg_gerr("Region \"%s\" exceeds the chip size.\n", entry->name);
+ return 1;
+ }
+ }
+
+ for (i = 0; i < layout->num_entries; i++) {
+ FILE *image;
+
+ entry = &layout->entries[i];
+ chipoff_t len = entry->end - entry->start + 1;
+
+ /* TODO(dhendrix) make sure layout->entries[i].file is NULL for non-included regions */
+ if (entry->included && entry->file) {
+ if ((image = fopen(entry->file, "rb")) == NULL) {
+ msg_gerr("Error: opening file \"%s\" failed: %s\n", entry->file, strerror(errno));
+ return 1;
+ }
+ } else {
+ continue;
+ }
+
+ struct stat image_stat;
+ if (fstat(fileno(image), &image_stat) != 0) {
+ msg_gerr("Error: getting metadata of file \"%s\" failed: %s\n", entry->file, strerror(errno));
+ ret = 1;
+ goto out;
+ }
+
+ unsigned long numbytes = fread(buf + entry->start, 1, len, image);
+ fclose(image);
+ if (numbytes != len) {
+ msg_gerr("Error: Failed to read file \"%s\". Got %ld bytes, "
+ "wanted %u!\n", entry->file, numbytes, len);
+ ret = 1;
+ break;
+ }
+
+ }
+out:
+ return ret;
+#endif
+}
+
/**
* @brief Writes included layout regions into buffer(s)
*
@@ -1441,15 +1510,19 @@
size_t i;
for (i = 0; i < layout->num_entries; i++) {
- if (!layout->entries[i].included)
- continue;
- if (!layout->entries[i].file)
+ if (!layout->entries[i].included || !layout->entries[i].file)
continue;
const chipoff_t region_start = layout->entries[i].start;
- const chipsize_t region_len = layout->entries[i].end - layout->entries[i].start + 1;
+ const chipoff_t region_end = layout->entries[i].end;
+ const chipsize_t region_len = region_end - region_start + 1;
const char *region_file = layout->entries[i].file;
+ if (i == 0)
+ msg_gdbg("\n"); /* avoid printing in caller's output */
+ msg_gdbg("Writing region %s [0x%06x:0x%06x] to file \"%s\"\n",
+ layout->entries[i].name, region_start, region_end, region_file);
+
ret = do_write_buf_to_file(buf + region_start, region_len, region_file);
if (ret)
goto out;
@@ -1466,7 +1539,6 @@
#endif
}
-
static int read_by_layout(struct flashctx *, uint8_t *);
int read_flash_to_file(struct flashctx *flash, const char *filename)
{
@@ -2598,8 +2670,18 @@
goto _free_ret;
}
- if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
- goto _free_ret;
+ /*
+ * This must be done before any files specified by -i arguments are
+ * processed and merged into newcontents since -i files take priority.
+ * See http://crbug.com/263495.
+ */
+ if (filename) {
+ if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
+ goto _free_ret;
+ } else {
+ if (read_buf_from_include_args(flash, newcontents, flash_size))
+ goto _free_ret;
+ }
ret = flashrom_image_write(flash, newcontents, flash_size);
@@ -2619,8 +2701,13 @@
goto _free_ret;
}
- if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
- goto _free_ret;
+ if (filename) {
+ if (read_buf_from_file(newcontents, flash_size, filename, "the flash chip's size"))
+ goto _free_ret;
+ } else {
+ if (read_buf_from_include_args(flash, newcontents, flash_size))
+ goto _free_ret;
+ }
ret = flashrom_image_verify(flash, newcontents, flash_size);
diff --git a/layout.c b/layout.c
index 4ae4a69..3493624 100644
--- a/layout.c
+++ b/layout.c
@@ -237,6 +237,44 @@
return 0;
}
+/* returns boolean 1 if any regions overlap, 0 otherwise */
+int included_regions_overlap(const struct flashrom_layout *const l)
+{
+ int i, overlap_detected = 0;
+
+ for (i = 0; i < l->num_entries; i++) {
+ int j;
+
+ if (!l->entries[i].included)
+ continue;
+
+ for (j = 0; j < l->num_entries; j++) {
+ if (!l->entries[j].included)
+ continue;
+
+ if (i == j)
+ continue;
+
+ if (l->entries[i].start > l->entries[j].end)
+ continue;
+
+ if (l->entries[i].end < l->entries[j].start)
+ continue;
+
+ msg_gdbg("Regions %s [0x%08x-0x%08x] and "
+ "%s [0x%08x-0x%08x] overlap\n",
+ l->entries[i].name, l->entries[i].start,
+ l->entries[i].end, l->entries[j].name,
+ l->entries[j].start, l->entries[j].end);
+ overlap_detected = 1;
+ goto out;
+ }
+
+ }
+out:
+ return overlap_detected;
+}
+
void layout_cleanup(void)
{
int i;
diff --git a/layout.h b/layout.h
index d1c793f..f401ef2 100644
--- a/layout.h
+++ b/layout.h
@@ -63,5 +63,6 @@
const struct flashrom_layout *get_layout(const struct flashrom_flashctx *const flashctx);
int process_include_args(struct flashrom_layout *);
+int included_regions_overlap(const struct flashrom_layout *const flashrom_layout);
#endif /* !__LAYOUT_H__ */
To view, visit change 23022. To unsubscribe, or for help writing mail filters, visit settings.