September 2013 - flashrom - mail.coreboot.org

[PATCH 2/2] layout: Better file parsing.
by Stefan Tauner Sept. 18, 2013

Sept. 18, 2013

A complete rewrite of the layout file parser: - safe (unlike before) - support for comments - supports including/sourcing of other layout files (relative to the including file as well as via absolute paths) up to predefined nesting level - checks for duplicate region names - can handle region names and layout file names with spaces etc correctly - way better diagnostics - no more character arrays in struct romentry_t but pointers only To help with migrating legacy layout files add a script for converting them to format 2. Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> --- layout.c | 305 +++++++++++++++++++++++++++++++++------- util/convert_layout_v1_to_v2.sh | 79 +++++++++++ 2 files changed, 331 insertions(+), 53 deletions(-) create mode 100755 util/convert_layout_v1_to_v2.sh diff --git a/layout.c b/layout.c index 4d4f35c..eb4ec07 100644 --- a/layout.c +++ b/layout.c @@ -22,17 +22,23 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <ctype.h> +#include <errno.h> #include <limits.h> #include "flash.h" #include "programmer.h" #define MAX_ROMLAYOUT 32 +#define MAX_ENTRY_LEN 1024 +#define WHITESPACE_CHARS " \t" +#define INCLUDE_INSTR "source" +#define MAX_NESTING_LVL 5 typedef struct { unsigned int start; unsigned int end; unsigned int included; - char name[256]; + char *name; char *file; } romentry_t; @@ -45,58 +51,264 @@ static int num_rom_entries = 0; /* the number of valid rom_entries */ static char *include_args[MAX_ROMLAYOUT]; static int num_include_args = 0; /* the number of valid include_args. */ -#ifndef __LIBPAYLOAD__ -int read_romlayout(char *name) +/* returns the index of the entry (or a negative value if it is not found) */ +static int find_romentry(char *name) { - FILE *romlayout; - char tempstr[256]; int i; + msg_gspew("Looking for region \"%s\"... ", name); + for (i = 0; i < num_rom_entries; i++) { + if (strcmp(rom_entries[i].name, name) == 0) { + msg_gspew("found.\n"); + return i; + } + } + msg_gspew("not found.\n"); + return -1; +} - romlayout = fopen(name, "r"); +/* FIXME: While libpayload has no file I/O itself, code using libflashrom could still provide layout information + * obtained by other means like user input or by fetching it from somewhere else. Therefore the parsing code + * should be separated from the file reading code eventually. */ +#ifndef __LIBPAYLOAD__ +/** Parse one line in a layout file. + * @param file_name The name of the file this line originates from. + * @param entry If not NULL fill it with the parsed data, else just detect errors and print diagnostics. + * @return -1 on error, + * 0 if the line could be parsed into a layout entry succesfully, + * 1 if a file was successfully sourced. + */ +static int parse_entry(char *file_name, char *buf, romentry_t *entry) +{ + msg_gdbg2("String to parse: \"%s\".\n", buf); + + /* Skip all white space in the beginning. */ + char *tmp_str = buf + strspn(buf, WHITESPACE_CHARS); + char *endptr; + + /* Check for include command. */ + if (strncmp(tmp_str, INCLUDE_INSTR, strlen(INCLUDE_INSTR)) == 0) { + tmp_str += strlen(INCLUDE_INSTR); + tmp_str += strspn(tmp_str, WHITESPACE_CHARS); + if (unquote_string(&tmp_str, NULL, WHITESPACE_CHARS) != 0) { + msg_gerr("Error parsing version 2 layout entry: Could not find file name in \"%s\".\n", + buf); + return -1; + } + msg_gspew("Source command found with filename \"%s\".\n", tmp_str); + + static unsigned int nesting_lvl = 0; + if (nesting_lvl >= MAX_NESTING_LVL) { + msg_gerr("Error: Nesting level exeeded limit of %u.\n", MAX_NESTING_LVL); + msg_gerr("Unable to import \"%s\" in layout file \"%s\".\n", tmp_str, file_name); + return -1; + } - if (!romlayout) { - msg_gerr("ERROR: Could not open layout file (%s).\n", - name); + nesting_lvl++; + int ret; + /* If a relative path is given, append it to the dirname of the current file. */ + if (*tmp_str != '/') { + /* We need space for: dirname of file_name, '/' , the file name in tmp_strand and '\0'. + * Since the dirname of file_name is shorter than file_name this is more than enough: */ + char *path = malloc(strlen(file_name) + strlen(tmp_str) + 2); + if (path == NULL) { + msg_gerr("Out of memory!\n"); + return -1; + } + strcpy(path, file_name); + + /* A less insane but incomplete dirname implementation... */ + endptr = strrchr(path, '/'); + if (endptr != NULL) { + endptr[0] = '/'; + endptr[1] = '\0'; + } else { + /* This is safe because the original file name was at least one char. */ + path[0] = '.'; + path[1] = '/'; + path[2] = '\0'; + } + strcat(path, tmp_str); + ret = read_romlayout(path); + free(path); + } else + ret = read_romlayout(tmp_str); + nesting_lvl--; + return ret >= 0 ? 1 : -1; /* Only return values < 0 are errors. */ + } + + errno = 0; + long tmp_long = strtol(tmp_str, &endptr, 0); + if (errno != 0 || endptr == tmp_str || tmp_long < 0 || tmp_long > UINT32_MAX) { + msg_gerr("Error parsing version 2 layout entry: Could not convert start address in \"%s\".\n", + buf); return -1; } + uint32_t start = tmp_long; - while (!feof(romlayout)) { - char *tstr1, *tstr2; + tmp_str = endptr + strspn(endptr, WHITESPACE_CHARS); + if (*tmp_str != ':') { + msg_gerr("Error parsing version 2 layout entry: Address separator does not follow start " + "address in \"%s\".\n", buf); + return -1; + } + tmp_str++; - if (num_rom_entries >= MAX_ROMLAYOUT) { - msg_gerr("Maximum number of entries (%i) in layout file reached.\n", MAX_ROMLAYOUT); - return 1; + errno = 0; + tmp_long = strtol(tmp_str, &endptr, 0); + if (errno != 0 || endptr == tmp_str || tmp_long < 0 || tmp_long > UINT32_MAX) { + msg_gerr("Error parsing version 2 layout entry: Could not convert end address in \"%s\"\n", + buf); + return -1; + } + uint32_t end = tmp_long; + + size_t skip = strspn(endptr, WHITESPACE_CHARS); + if (skip == 0) { + msg_gerr("Error parsing version 2 layout entry: End address is not followed by white space in " + "\"%s\"\n", buf); + return -1; + } + + tmp_str = endptr + skip; + /* The region name is either enclosed by quotes or ends with the first whitespace. */ + if (unquote_string(&tmp_str, &endptr, WHITESPACE_CHARS) != 0) { + msg_gerr("Error parsing version 2 layout entry: Could not find region name in \"%s\".\n", buf); + return -1; + } + + msg_gdbg("Parsed entry: 0x%08x - 0x%08x named \"%s\"\n", start, end, tmp_str); + + if (start >= end) { + msg_gerr("Error parsing version 2 layout entry: Length of region \"%s\" is not positive.\n", + tmp_str); + return -1; + } + + if (find_romentry(tmp_str) >= 0) { + msg_gerr("Error parsing version 2 layout entry: Region name \"%s\" used multiple times.\n", + tmp_str); + return -1; + } + + endptr += strspn(endptr, WHITESPACE_CHARS); + if (strlen(endptr) != 0) + msg_gerr("Warning: Region name \"%s\" is not followed by white space only.\n", tmp_str); + + if (entry != NULL) { + entry->name = strdup(tmp_str); + if (entry->name == NULL) { + msg_gerr("Out of memory!\n"); + return -1; } - if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name)) - continue; -#if 0 - // fscanf does not like arbitrary comments like that :( later - if (tempstr[0] == '#') { - continue; + + entry->start = start; + entry->end = end; + entry->included = 0; + entry->file = NULL; + } + return 0; +} + +/* Scan the first line for the determinant version comment and parse it, or assume it is version 1. */ +static int detect_layout_version(FILE *romlayout) +{ + int c; + do { /* Skip white space */ + c = fgetc(romlayout); + if (c == EOF) + return -1; + } while (isblank(c)); + ungetc(c, romlayout); + + const char* vcomment = "# flashrom layout v"; + char buf[strlen(vcomment) + 1]; /* comment + \0 */ + if (fgets(buf, sizeof(buf), romlayout) == NULL) + return -1; + if (strcmp(vcomment, buf) != 0) + return 1; + int version; + if (fscanf(romlayout, "%d", &version) != 1) + return -1; + if (version < 2) { + msg_gwarn("Warning: Layout file declares itself to be version %d, but self delcaration has\n" + "only been possible since version 2. Continuing anyway.\n", version); + } + return version; +} + +int read_romlayout(char *name) +{ + FILE *romlayout = fopen(name, "r"); + if (romlayout == NULL) { + msg_gerr("ERROR: Could not open layout file \"%s\".\n", name); + return -1; + } + + const int version = detect_layout_version(romlayout); + if (version < 0) { + msg_gerr("Could not determine version of layout file \"%s\".\n", name); + fclose(romlayout); + return 1; + } + if (version != 2) { + msg_gerr("Layout file version %d is not supported in this version of flashrom.\n", version); + fclose(romlayout); + return 1; + } + rewind(romlayout); + + msg_gdbg("Parsing layout file \"%s\" according to version %d.\n", name, version); + int linecnt = 0; + while (!feof(romlayout)) { + char buf[MAX_ENTRY_LEN]; + char *curchar = buf; + linecnt++; + msg_gspew("Parsing line %d of \"%s\".\n", linecnt, name); + + while (true) { + /* Make sure that we ignore various newline sequences by checking for \r too. + * NB: This might introduce empty lines. */ + char c = fgetc(romlayout); + if (c == '#') { + do { /* Skip characters in comments */ + c = fgetc(romlayout); + } while (c != EOF && c != '\n' && c != '\r'); + continue; + } + if (c == EOF || c == '\n' || c == '\r') { + *curchar = '\0'; + break; + } + if (curchar == &buf[MAX_ENTRY_LEN - 1]) { + msg_gerr("Line %d of layout file \"%s\" is longer than the allowed %d chars.\n", + linecnt, name, MAX_ENTRY_LEN); + fclose(romlayout); + return 1; + } + *curchar = c; + curchar++; } -#endif - tstr1 = strtok(tempstr, ":"); - tstr2 = strtok(NULL, ":"); - if (!tstr1 || !tstr2) { - msg_gerr("Error parsing layout file. Offending string: \"%s\"\n", tempstr); + + /* Skip all whitespace or empty lines */ + if (strspn(buf, WHITESPACE_CHARS) == strlen(buf)) + continue; + + romentry_t *entry = (num_rom_entries >= MAX_ROMLAYOUT) ? NULL : &rom_entries[num_rom_entries]; + int ret = parse_entry(name, buf, entry); + if (ret > 0) { fclose(romlayout); return 1; } - rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16); - rom_entries[num_rom_entries].end = strtol(tstr2, (char **)NULL, 16); - rom_entries[num_rom_entries].included = 0; - rom_entries[num_rom_entries].file = NULL; - num_rom_entries++; + /* Only 0 indicates the successfully parsing of an entry, others are errors or imports. */ + if (ret == 0) + num_rom_entries++; } - - for (i = 0; i < num_rom_entries; i++) { - msg_gdbg("romlayout %08x - %08x named %s\n", - rom_entries[i].start, - rom_entries[i].end, rom_entries[i].name); - } - fclose(romlayout); - + if (num_rom_entries >= MAX_ROMLAYOUT) { + msg_gerr("Found %d entries in layout file which is more than the %i allowed.\n", + num_rom_entries + 1, MAX_ROMLAYOUT); + return 1; + } return 0; } #endif @@ -135,21 +347,6 @@ int register_include_arg(char *name) return 0; } -/* returns the index of the entry (or a negative value if it is not found) */ -static int find_romentry(char *name) -{ - int i; - msg_gspew("Looking for region \"%s\"... ", name); - for (i = 0; i < num_rom_entries; i++) { - if (strcmp(rom_entries[i].name, name) == 0) { - msg_gspew("found.\n"); - return i; - } - } - msg_gspew("not found.\n"); - return -1; -} - /* process -i arguments * returns 0 to indicate success, >0 to indicate failure */ @@ -224,6 +421,8 @@ void layout_cleanup(void) num_include_args = 0; for (i = 0; i < num_rom_entries; i++) { + free(rom_entries[i].name); + rom_entries[i].name = NULL; free(rom_entries[i].file); rom_entries[i].file = NULL; rom_entries[i].included = 0; diff --git a/util/convert_layout_v1_to_v2.sh b/util/convert_layout_v1_to_v2.sh new file mode 100755 index 0000000..712de87 --- /dev/null +++ b/util/convert_layout_v1_to_v2.sh @@ -0,0 +1,79 @@ +#!/bin/sh +# +# Copyright 2013 Stefan Tauner +# +# 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 +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, +# MA 02110-1301, USA. +# +# +# +# This script converts legacy layout files as understood by flashrom up to version 0.9.7 to format version 2. +# It converts all files given as parameters in place and creates backups (with the suffix ".old") of the old +# contents unless --nobackup is given. +# +# It does... +# - check if the file exists and if its format is already in v2 format +# - prefix addresses with 0x if they have not been already +# - remove superfluous white space (i.e. more than one consecutive space) +# - remove white space from otherwise empty lines + +usage () +{ + echo "Usage: $0 [--nobackup] FILE..." + exit 1 +} + +if [ $# -eq 0 ]; then + usage +fi + +if [ $1 = "--nobackup" ]; then + sed_opt="-i" + shift + if [ $# -eq 0 ]; then + usage + fi +else + sed_opt="-i.old" +fi + +# Test if all files are really there before starting conversion +ret=0 +for f in "$@" ; do + if [ ! -e "$f" ]; then + echo "File not found: $f">&2 + ret=1 + fi +done +if [ "$ret" -ne 0 ]; then + echo "Aborting" + return 1 +fi + +for f in "$@" ; do + if grep -q 'flashrom layout v2\b' "$f" ; then + echo "File already in new format: $f" + continue + fi + + sed $sed_opt -e " + 1i # flashrom layout v2 + s/ *$0x\|$$[0-9a-fA-F][0-9a-fA-F]*$ *: *$0x\|$$[0-9a-fA-F][0-9a-fA-F]*$ */0x\2:0x\4 / + s/ */ / + s/^ *$// + " "$f" + echo "$f done" +done +echo "Done!" -- Kind regards, Stefan Tauner

1 0

[PATCH 1/2] layout: Add -i <region>[:<file>] support.
by Stefan Tauner Sept. 18, 2013

Sept. 18, 2013

Add an optional sub-parameter to the -i parameter to allow building the image to be written from multiple files. This will also allow regions to be read from flash and written to separate image files in a later patch. Existing function read_buf_from_file() is refined and reused to read the file. Document the whole layout handling including the new features a bit better and refine wording regarding files, images, layouts and regions as discussed. Also, finally introduce helpers.c: a module containing utility functions to be shared which do not fit elsewhere. The first function to be added is unquote_string() which is used throughout this patch. Abort for non-write operations if a layout file is given. based on chromiumos' d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 Signed-off-by: David Hendricks <dhendrix(a)chromium.org> Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> --- Makefile | 2 +- cli_classic.c | 15 +++++-- dummyflasher.c | 2 +- flash.h | 6 ++- flashrom.8.tmpl | 130 ++++++++++++++++++++++++++++++++++---------------------- flashrom.c | 20 +++++---- helpers.c | 74 ++++++++++++++++++++++++++++++++ layout.c | 70 ++++++++++++++++++++++-------- 8 files changed, 236 insertions(+), 83 deletions(-) create mode 100644 helpers.c diff --git a/Makefile b/Makefile index 346d46a..69e0b9c 100644 --- a/Makefile +++ b/Makefile @@ -338,7 +338,7 @@ CHIP_OBJS = jedec.o stm50.o w39.o w29ee011.o \ ############################################################################### # Library code. -LIB_OBJS = layout.o flashrom.o udelay.o programmer.o +LIB_OBJS = layout.o flashrom.o udelay.o programmer.o helpers.o ############################################################################### # Frontend related stuff. diff --git a/cli_classic.c b/cli_classic.c index 70bccb5..ed6d6aa 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -40,8 +40,8 @@ static void cli_classic_usage(const char *name) #if CONFIG_PRINT_WIKI == 1 "-z|" #endif - "-p <programmername>[:<parameters>] [-c <chipname>]\n" - "[-E|(-r|-w|-v) <file>] [-l <layoutfile> [-i <imagename>]...] [-n] [-f]]\n" + "-p <programmername>[:<parameter>[,<parameter>]...] [-c <chipname>]\n" + "[-E|(-r|-w|-v) <imagefile>] [-l <layoutfile> [-i <region>[:<regionfile>]...] [-n] [-f]]\n" "[-V[V[V]]] [-o <logfile>]\n\n", name); printf(" -h | --help print this help text\n" @@ -54,8 +54,9 @@ static void cli_classic_usage(const char *name) " -c | --chip <chipname> probe only for specified flash chip\n" " -f | --force force specific operations (see man page)\n" " -n | --noverify don't auto-verify\n" - " -l | --layout <layoutfile> read ROM layout from <layoutfile>\n" - " -i | --image <name> only flash image <name> from flash layout\n" + " -l | --layout <layoutfile> read layout from <layoutfile>\n" + " -i | --include <region>[<:file>] only flash image <region> from layout \n" + " (optionally with data from <file>)\n" " -o | --output <logfile> log output to <logfile>\n" " -L | --list-supported print supported devices\n" #if CONFIG_PRINT_WIKI == 1 @@ -370,6 +371,12 @@ int main(int argc, char *argv[]) ret = 1; goto out; } + if (layoutfile != NULL && !write_it) { + msg_gerr("Layout files are currently supported for write operations only.\n"); + ret = 1; + goto out; + } + if (process_include_args()) { ret = 1; goto out; diff --git a/dummyflasher.c b/dummyflasher.c index 9c0d868..90d2261 100644 --- a/dummyflasher.c +++ b/dummyflasher.c @@ -389,7 +389,7 @@ int dummy_init(void) msg_pdbg("matches.\n"); msg_pdbg("Reading %s\n", emu_persistent_image); read_buf_from_file(flashchip_contents, emu_chip_size, - emu_persistent_image); + emu_persistent_image, NULL); } else { msg_pdbg("doesn't match.\n"); } diff --git a/flash.h b/flash.h index 7b88477..b4946a0 100644 --- a/flash.h +++ b/flash.h @@ -253,9 +253,12 @@ void print_banner(void); void list_programmers_linebreak(int startcol, int cols, int paren); int selfcheck(void); int doit(struct flashctx *flash, int force, const char *filename, int read_it, int write_it, int erase_it, int verify_it); -int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename); +int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename, const char *size_msg); int write_buf_to_file(unsigned char *buf, unsigned long size, const char *filename); +/* helpers.c */ +int unquote_string(char **startp, char **endp, const char *delimiters); + enum test_state { OK = 0, NT = 1, /* Not tested */ @@ -321,6 +324,7 @@ int process_include_args(void); int read_romlayout(char *name); int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents); void layout_cleanup(void); +int build_new_image(struct flashctx *flash, int oldcontents_valid, uint8_t *oldcontents, uint8_t *newcontents); /* spi.c */ struct spi_command { diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index b507a97..0ccdcf1 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -3,10 +3,10 @@ flashrom \- detect, read, write, verify and erase flash chips .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\-p\fR <programmername>[:<parameter>[,<parameter>]...]] + [\fB\-E\fR|\fB\-r\fR <imagefile>|\fB\-w\fR <imagefile>|\fB\-v\fR <imagefile>] \ [\fB\-c\fR <chipname>] - [\fB\-l\fR <file> [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-f\fR]] + [\fB\-l\fR <layoutfile> [\fB\-i\fR <regionname>[:<regionfile>]]...] [\fB\-n\fR] [\fB\-f\fR]] [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] .SH DESCRIPTION .B flashrom @@ -38,14 +38,14 @@ before you try to write a new image. All operations involving any chip access (p .B -p/--programmer option to be used (please see below). .TP -.B "\-r, \-\-read <file>" +.B "\-r, \-\-read <imagefile>" Read flash ROM contents and save them into the given -.BR <file> . +.BR <imagefile> . If the file already exists, it will be overwritten. .TP -.B "\-w, \-\-write <file>" +.B "\-w, \-\-write <imagefile>" Write -.B <file> +.B <imagefile> into flash ROM. This will first automatically .B erase the chip, then write to it. @@ -65,14 +65,14 @@ recommended, you should only use it if you know what you are doing and if you feel that the time for verification takes too long. .sp Typical usage is: -.B "flashrom \-p prog \-n \-w <file>" +.B "flashrom \-p prog \-n \-w <imagefile>" .sp This option is only useful in combination with .BR \-\-write . .TP -.B "\-v, \-\-verify <file>" +.B "\-v, \-\-verify <imagefile>" Verify the flash ROM contents against the given -.BR <file> . +.BR <imagefile> . .TP .B "\-E, \-\-erase" Erase the flash ROM chip. @@ -102,46 +102,6 @@ size for the flash bus. .sp * Force write even if write is known bad. .TP -.B "\-l, \-\-layout <file>" -Read ROM layout from -.BR <file> . -.sp -flashrom supports ROM layouts. This allows you to flash certain parts of -the flash chip only. A ROM layout file contains multiple lines with the -following syntax: -.sp -.B " startaddr:endaddr imagename" -.sp -.BR "startaddr " "and " "endaddr " -are hexadecimal addresses within the ROM file and do not refer to any -physical address. Please note that using a 0x prefix for those hexadecimal -numbers is not necessary, but you can't specify decimal/octal numbers. -.BR "imagename " "is an arbitrary name for the region/image from" -.BR " startaddr " "to " "endaddr " "(both addresses included)." -.sp -Example: -.sp - 00000000:00008fff gfxrom - 00009000:0003ffff normal - 00040000:0007ffff fallback -.sp -If you only want to update the image named -.BR "normal " "in a ROM based on the layout above, run" -.sp -.B " flashrom \-p prog \-\-layout rom.layout \-\-image normal \-w some.rom" -.sp -To update only the images named -.BR "normal " "and " "fallback" ", run:" -.sp -.B " flashrom \-p prog \-l rom.layout \-i normal -i fallback \-w some.rom" -.sp -Overlapping sections are not supported. -.TP -.B "\-i, \-\-image <imagename>" -Only flash region/image -.B <imagename> -from flash layout. -.TP .B "\-L, \-\-list\-supported" List the flash chips, chipsets, mainboards, and external programmers (including PCI, USB, parallel port, and serial port based devices) @@ -226,6 +186,76 @@ Some programmers have optional or mandatory parameters which are described in detail in the .B PROGRAMMER SPECIFIC INFO section. Support for some programmers can be disabled at compile time. +.TP +flashrom supports reading/writing/erasing/verifying flash chips in whole (default) or in part. To access only \ +parts of a chip one has to use layout files and respective arguments described below. +.TP +.B "\-l, \-\-layout <layoutfile>" +Read layout entries from +.BR <layoutfile> . +.sp +A layout file can contain comments which are +started by a pound sign (#) and causes all following characters on the same line to be ignored with one +exception: Every layout file +.B must +start with a comment in the following form to define the version of the file. Example for the current version 2: +.sp +.B " # flashrom layout v2" +.sp +Every other line may contain either a command to include another layout file, a layout entry or white space +(and an optional comment at the end). +.sp +To include another file you can use the +.sp +.B " source <path>" +.sp +syntax where +.B path +specifies the absolute or relative path to the file to be included. Relative paths are interpreted to be +relative to the file containing the include command. If the path contains spaces it has to be written in double +quotes, or else only the part before the first space will be used. +.sp +Each layout entry describes an address region of the flash chip and gives it a name (hereinafter referred to as +a region). One entry per line is allowed with the following syntax: +.sp +.B " startaddr:endaddr regionname" +.sp +.BR "startaddr " "and " "endaddr " +are addresses within the ROM image representing the flash ROM contents. They are interpreted in the 'usual' form +i.e.\ a leading 0 means octal, leading 0x or 0X means hexadecimal, everything else is just decimal. +.BR "regionname " "is the name for the region from " "startaddr " "to " "endaddr " "(both addresses included)." +If the name contains spaces it has to be written in double quotes, or else only the part before the first space +will be used. +.sp +Example content of file rom.layout: +.sp + # flashrom layout v2 + source /home/flashrom/include.layout + 0x00000000:0x00008fff "gfx rom" + 0x00009000:0x0003ffff normal + 0x00040000:0x0007ffff fallback +.sp +.TP +.B "\-i, \-\-include <name>[:<regionfile>]" +Work on the flash region +.B name +instead of the full address space if a layout file is given and parsed correctly. +Multiple such include parameters can be used to work on the union of different regions. +.sp +The optional +.B regionfile +parameter specifies the name of a file that is used to map the contents of that very region only. +The optional file parameter causes the contents of this region to be replaced by the contents of the file +specified here. +.sp +If you only want to update the regions named +.BR "normal " "and " "gfx rom " "in a ROM based on the layout mentioned above, run" +.sp +.B " flashrom \-p prog \-l rom.layout \-i normal -i ""gfx rom"" \-w some.rom" +.sp +Overlapping regions are resolved in an implementation-dependent manner (or may even yield an error) - do +.BR "not " "rely on it." +.sp .B "flashrom \-h" lists all supported programmers. .TP diff --git a/flashrom.c b/flashrom.c index 9169620..b98507c 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1165,8 +1165,7 @@ notfound: return chip - flashchips; } -int read_buf_from_file(unsigned char *buf, unsigned long size, - const char *filename) +int read_buf_from_file(unsigned char *buf, unsigned long size, const char *filename, const char *size_msg) { #ifdef __LIBPAYLOAD__ msg_gerr("Error: No file I/O support in libpayload\n"); @@ -1186,8 +1185,10 @@ int read_buf_from_file(unsigned char *buf, unsigned long size, return 1; } if (image_stat.st_size != size) { - msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%lu B)!\n", - (intmax_t)image_stat.st_size, size); + if (size_msg == NULL) + size_msg = "the expected size"; + msg_gerr("Error: Image size (%jd B) doesn't match %s (%lu B)!\n", + (intmax_t)image_stat.st_size, size_msg, size); fclose(image); return 1; } @@ -1962,7 +1963,7 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, } if (write_it || verify_it) { - if (read_buf_from_file(newcontents, size, filename)) { + if (read_buf_from_file(newcontents, size, filename, "the flash chip's size")) { ret = 1; goto out; } @@ -1995,9 +1996,12 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, } msg_cinfo("done.\n"); - // This should be moved into each flash part's code to do it - // cleanly. This does the job. - handle_romentries(flash, oldcontents, newcontents); + if (handle_romentries(flash, oldcontents, newcontents)) { + msg_gerr("Could not prepare the data to be written due to problems with the layout,\n" + "aborting.\n"); + ret = 1; + goto out; + } // //////////////////////////////////////////////////////////// diff --git a/helpers.c b/helpers.c new file mode 100644 index 0000000..ebb6f29 --- /dev/null +++ b/helpers.c @@ -0,0 +1,74 @@ +/* + * This file is part of the flashrom project. + * + * Copyright (C) 2013 Stefan Tauner + * + * 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 + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <string.h> +#include "flash.h" + +/** Parse a \em possibly quoted string. + * + * The function expects \a startp to point to the string to be parsed without a prefix. If the + * string starts with a quotation mark it looks for the second one and removes both in-place, if not then it + * looks for the first character contained in \a delimiters and ends the wanted string there by replacing the + * character with '\0'. If \a delimiters is NULL an empty set is assumed, hence the whole start string equals + * the wanted string. + * + * After returning \a startp will point to a string that is either the first quoted part of the + * original string with the quotation marks removed, or the first word of that string before any delimiter. + * If \a endp is not NULL it will be set to point to the first character after the parsed string, or to the + * '\0' at the end of the string pointed to by \a startp if there are no more subsequent characters. + * + * @param start Points to the input string. + * @param end Is set to the first char following the input string. + * @return -1 if a quotation mark is not matched, + * 0 on success, + * 1 if the parsed string is empty. + */ +int unquote_string(char **startp, char **endp, const char *delimiters) +{ + msg_gspew("unquoting \'%s\'\n", *startp); + char *end; + size_t len; + if (**startp == '"') { + (*startp)++; + len = strcspn(*startp, "\""); + if (*(*startp + len) != '"') + return -1; + + } else { + if (delimiters == NULL) + len = strlen(*startp); + else + len = strcspn(*startp, delimiters); + } + if (len == 0) { + *startp = '\0'; + return 1; + } + + end = *startp + len; + if (*end != '\0') { + *end = '\0'; + end++; + } + if (endp != NULL) + *endp = end; + msg_gspew("%s: start=\'%s\', end=\'%s\'\n", __func__, *startp, end); + return 0; +} diff --git a/layout.c b/layout.c index 86351b8..4d4f35c 100644 --- a/layout.c +++ b/layout.c @@ -33,6 +33,7 @@ typedef struct { unsigned int end; unsigned int included; char name[256]; + char *file; } romentry_t; /* rom_entries store the entries specified in a layout file and associated run-time data */ @@ -54,7 +55,7 @@ int read_romlayout(char *name) romlayout = fopen(name, "r"); if (!romlayout) { - msg_gerr("ERROR: Could not open ROM layout (%s).\n", + msg_gerr("ERROR: Could not open layout file (%s).\n", name); return -1; } @@ -63,8 +64,7 @@ int read_romlayout(char *name) char *tstr1, *tstr2; if (num_rom_entries >= MAX_ROMLAYOUT) { - msg_gerr("Maximum number of ROM images (%i) in layout " - "file reached.\n", MAX_ROMLAYOUT); + msg_gerr("Maximum number of entries (%i) in layout file reached.\n", MAX_ROMLAYOUT); return 1; } if (2 != fscanf(romlayout, "%s %s\n", tempstr, rom_entries[num_rom_entries].name)) @@ -85,6 +85,7 @@ int read_romlayout(char *name) rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16); rom_entries[num_rom_entries].end = strtol(tstr2, (char **)NULL, 16); rom_entries[num_rom_entries].included = 0; + rom_entries[num_rom_entries].file = NULL; num_rom_entries++; } @@ -138,14 +139,9 @@ int register_include_arg(char *name) static int find_romentry(char *name) { int i; - - if (num_rom_entries == 0) - return -1; - msg_gspew("Looking for region \"%s\"... ", name); for (i = 0; i < num_rom_entries; i++) { - if (!strcmp(rom_entries[i].name, name)) { - rom_entries[i].included = 1; + if (strcmp(rom_entries[i].name, name) == 0) { msg_gspew("found.\n"); return i; } @@ -167,25 +163,53 @@ int process_include_args(void) /* User has specified an area, but no layout file is loaded. */ if (num_rom_entries == 0) { - msg_gerr("Region requested (with -i \"%s\"), " - "but no layout data is available.\n", + msg_gerr("Region requested (with -i/--include \"%s\"),\n" + "but no layout data is available. To include one use the -l/--layout syntax).\n", include_args[0]); return 1; } for (i = 0; i < num_include_args; i++) { - if (find_romentry(include_args[i]) < 0) { - msg_gerr("Invalid region specified: \"%s\".\n", - include_args[i]); + char *file; + char *name = include_args[i]; + int ret = unquote_string(&name, &file, ":"); /* -i <region>[:<file>] */ + if (ret != 0) { + msg_gerr("Invalid include argument specified: \"%s\".\n", name); return 1; } + int idx = find_romentry(name); + if (idx < 0) { + msg_gerr("Invalid region name specified: \"%s\".\n", name); + return 1; + } + rom_entries[idx].included = 1; found++; + + if (file[0] == ':') { + file++; + /* The remaining characters are interpreted as possible quoted filename. */ + ret = unquote_string(&file, NULL, NULL); + if (ret != 0) { + msg_gerr("Invalid region file name specified: \"%s\".\n", file); + return 1; + } +#ifdef __LIBPAYLOAD__ + msg_gerr("Error: No file I/O support in libpayload\n"); + return 1; +#else + file = strdup(file); + if (file == NULL) { + msg_gerr("Out of memory!\n"); + return 1; + } + rom_entries[idx].file = file; +#endif + } } - msg_ginfo("Using region%s: \"%s\"", num_include_args > 1 ? "s" : "", - include_args[0]); - for (i = 1; i < num_include_args; i++) - msg_ginfo(", \"%s\"", include_args[i]); + msg_ginfo("Using region%s: \"%s\"", num_rom_entries > 1 ? "s" : "", rom_entries[0].name); + for (i = 1; i < num_rom_entries; i++) + msg_ginfo(", \"%s\"", rom_entries[i].name); msg_ginfo(".\n"); return 0; } @@ -200,6 +224,8 @@ void layout_cleanup(void) num_include_args = 0; for (i = 0; i < num_rom_entries; i++) { + free(rom_entries[i].file); + rom_entries[i].file = NULL; rom_entries[i].included = 0; } num_rom_entries = 0; @@ -259,6 +285,14 @@ int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_ if (entry->start > start) memcpy(newcontents + start, oldcontents + start, entry->start - start); + /* If a file name is specified for this region, read the file contents and + * overwrite @newcontents in the range specified by @entry. */ + if (entry->file != NULL) { + if (read_buf_from_file(newcontents + entry->start, entry->end - entry->start + 1, + entry->file, "the region's size") != 0) + return 1; + } + /* Skip to location after current romentry. */ start = entry->end + 1; /* Catch overflow. */ -- Kind regards, Stefan Tauner

1 0

[PATCH 0/2] layout patches 3.1 (first half)
by Stefan Tauner Sept. 18, 2013

Sept. 18, 2013

Apart from the unquoting disput and possible wording problems, I hope this is closer to what we want. The removal of layout 1 parsing support did not really remove much code... I made a serious mistake when refining the whole patch set (and merged a few patches in there sometime ago without noticing before reworking a lot of stuff), so I hope I have cleaned that up correctly. If you see code in there that seems to have nothing to do with the actual patch then it is that - not insanity (neither on your nor my side). The unquoting function is moved to helpers.c and gets used in the -i patch already, because it makes sense there for really odd file names. It might also make sense for ROM file or log file names, I havent checked yet. Let's decide first if we want the function in general and which features it should have before anything else please. Finally, I refuse any responsibility for the new file exist check in convert_layout_v1_to_v2.sh, that's carldani's fault alone I just obeyed :P Stefan Tauner (2): layout: Add -i <region>[:<file>] support. layout: Better file parsing. Makefile | 2 +- cli_classic.c | 15 +- dummyflasher.c | 2 +- flash.h | 6 +- flashrom.8.tmpl | 130 ++++++++------ flashrom.c | 20 ++- helpers.c | 74 ++++++++ layout.c | 367 ++++++++++++++++++++++++++++++++-------- util/convert_layout_v1_to_v2.sh | 79 +++++++++ 9 files changed, 563 insertions(+), 132 deletions(-) create mode 100644 helpers.c create mode 100755 util/convert_layout_v1_to_v2.sh -- Kind regards, Stefan Tauner

1 0

Re: [flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.
by Carl-Daniel Hailfinger Sept. 17, 2013

Sept. 17, 2013

Am 12.09.2013 22:40 schrieb Stefan Tauner: > Add an optional sub-parameter to the -i parameter to allow building the > image to be written from multiple files. This will also allow regions to > be read from flash and written to separate image files in a later patch. > Document the whole layout handling including the new features a bit better. > > based on chromiumos' > d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 > Signed-off-by: David Hendricks <dhendrix(a)chromium.org> > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> > > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> Double signoff. > --- > flashrom.8.tmpl | 47 +++++++++++++-------------- > flashrom.c | 8 +++-- > layout.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++------- > 3 files changed, 116 insertions(+), 37 deletions(-) See my other mail for the man page review. > diff --git a/flashrom.c b/flashrom.c > index a00347e..cd15de7 100644 > --- a/flashrom.c > +++ b/flashrom.c > @@ -1993,9 +1993,11 @@ int doit(struct flashctx *flash, int force, const char *filename, int read_it, > } > msg_cinfo("done.\n"); > > - // This should be moved into each flash part's code to do it > - // cleanly. This does the job. > - handle_romentries(flash, oldcontents, newcontents); > + if (handle_romentries(flash, oldcontents, newcontents)) { Missing ret=1 > + msg_gerr("Could not prepare the data to be written due to problems with the layout,\n" > + "aborting.\n"); > + goto out; > + } > > // //////////////////////////////////////////////////////////// > > diff --git a/layout.c b/layout.c > index 86351b8..20fe303 100644 > --- a/layout.c > +++ b/layout.c > @@ -33,6 +38,7 @@ typedef struct { > unsigned int end; > unsigned int included; > char name[256]; > + char *file; > } romentry_t; > > /* rom_entries store the entries specified in a layout file and associated run-time data */ > @@ -85,6 +91,7 @@ int read_romlayout(char *name) > rom_entries[num_rom_entries].start = strtol(tstr1, (char **)NULL, 16); > rom_entries[num_rom_entries].end = strtol(tstr2, (char **)NULL, 16); That code wasn't changed, but should we use strtoul here instead? > rom_entries[num_rom_entries].included = 0; > + rom_entries[num_rom_entries].file = NULL; > num_rom_entries++; > } > > @@ -167,19 +169,36 @@ int process_include_args(void) > > /* User has specified an area, but no layout file is loaded. */ > if (num_rom_entries == 0) { > - msg_gerr("Region requested (with -i \"%s\"), " > - "but no layout data is available.\n", > + msg_gerr("Region requested (with -i/--image \"%s\"),\n" > + "but no layout data is available. To include one use the -l/--layout syntax).\n", > include_args[0]); > return 1; > } > > for (i = 0; i < num_include_args; i++) { > - if (find_romentry(include_args[i]) < 0) { > - msg_gerr("Invalid region specified: \"%s\".\n", > - include_args[i]); > + char *name = strtok(include_args[i], ":"); /* -i <image>[:<file>] */ Side note: We should state in the man page that ":" is not a valid character for region names. > + int idx = find_romentry(name); > + if (idx < 0) { > + msg_gerr("Invalid region specified: \"%s\".\n", include_args[i]); > return 1; > } > + rom_entries[idx].included = 1; > found++; > + > + char *file = strtok(NULL, ""); /* remaining non-zero length token or NULL */ > + if (file != NULL) { > +#ifdef __LIBPAYLOAD__ > + msg_gerr("Error: No file I/O support in libpayload\n"); > + return 1; > +#else > + file = strdup(file); > + if (file == NULL) { > + msg_gerr("Out of memory!\n"); > + return 1; > + } > + rom_entries[idx].file = file; > +#endif > + } > } > > msg_ginfo("Using region%s: \"%s\"", num_include_args > 1 ? "s" : "", > @@ -232,6 +253,57 @@ romentry_t *get_next_included_romentry(unsigned int start) > return best_entry; > } > > +/* If a file name is specified for this region, read the file contents and > + * overwrite @newcontents in the range specified by @entry. */ > +static int read_content_from_file(romentry_t *entry, uint8_t *newcontents) This whole function is a broken copypaste from read_buf_from_file() in flashrom.c. Please use the original function, and add additional parameters to it if needed. > +{ > + char *file = entry->file; > + if (file == NULL) > + return 0; > + > +#ifdef __LIBPAYLOAD__ > + msg_gerr("Error: No file I/O support in libpayload\n"); > + return 1; > +#else > + int len = entry->end - entry->start + 1; > + FILE *fp; > + if ((fp = fopen(file, "rb")) == NULL) { > + msg_gerr("Error: Opening layout image file \"%s\" failed: %s\n", file, strerror(errno)); "layout image"? Sorry, that name won't fly. > + return 1; > + } > + > + struct stat file_stat; > + if (fstat(fileno(fp), &file_stat) != 0) { > + msg_gerr("Error: Getting metadata of layout image file \"%s\" failed: %s\n", file, strerror(errno)); > + fclose(fp); > + return 1; > + } > + if (file_stat.st_size != len) { > + msg_gerr("Error: Image size (%jd B) doesn't match the flash chip's size (%d B)!\n", Somebody forgot to change the text here. > + (intmax_t)file_stat.st_size, len); > + fclose(fp); > + return 1; > + } > + > + int numbytes = fread(newcontents + entry->start, 1, len, fp); > + if (ferror(fp)) { > + msg_gerr("Error: Reading layout image file \"%s\" failed: %s\n", file, strerror(errno)); > + fclose(fp); > + return 1; > + } > + if (fclose(fp)) { > + msg_gerr("Error: Closing layout image file \"%s\" failed: %s\n", file, strerror(errno)); > + return 1; > + } > + if (numbytes != len) { > + msg_gerr("Error: Failed to read layout image file \"%s\" completely.\n" > + "Got %d bytes, wanted %d!\n", file, numbytes, len); > + return 1; > + } > + return 0; > +#endif > +} > + > int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_t *newcontents) > { > unsigned int start = 0; > @@ -259,6 +331,10 @@ int handle_romentries(const struct flashctx *flash, uint8_t *oldcontents, uint8_ > if (entry->start > start) > memcpy(newcontents + start, oldcontents + start, > entry->start - start); > + /* For included region, copy from file if specified. */ > + if (read_content_from_file(entry, newcontents) != 0) > + return 1; I don't know if this behaviour is completely intended. AFAICS the following command line flashrom -r read.rom -l layout.txt -i region1:region1.bin will read the full flash chip, then replace the contents of region1 in the flash image with the contents of region1.bin, then write the combined image to disk. I don't have a problem with that, but I think it warrants thinking about it. > + > /* Skip to location after current romentry. */ > start = entry->end + 1; > /* Catch overflow. */ Looks good otherwise. Regards, Carl-Daniel -- http://www.hailfinger.org/

2 2

Re: [flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.
by Stefan Tauner Sept. 17, 2013

Sept. 17, 2013

On Tue, 17 Sep 2013 12:55:38 -0700 David Hendricks <dhendrix(a)google.com> wrote: > > As I tried to explain, I have no problem with the "region image" apart > > from the image/file ambiguity. "region image file" is totally fine with > > me. > > > > Be careful not to conflate the concept of an image with individual regions > (or maybe I already have). A region is simply an address range inside of an > image. A region is an address range inside a chip-size/full/ROM image. The thing that gets a name by the use of a layout file and which is selected by that name with an -i parameter. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner

1 0

Re: [flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.
by Carl-Daniel Hailfinger Sept. 17, 2013

Sept. 17, 2013

Am 12.09.2013 22:40 schrieb Stefan Tauner: > Add an optional sub-parameter to the -i parameter to allow building the > image to be written from multiple files. This will also allow regions to > be read from flash and written to separate image files in a later patch. > Document the whole layout handling including the new features a bit better. > > based on chromiumos' > d0ea9ed71e7f86bb8e8db2ca7c32a96de25343d8 > Signed-off-by: David Hendricks <dhendrix(a)chromium.org> > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> > > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> First review round, man page only. I want to get our terms right before the existing confusion gets worse. Code review follows in a separate mail. > --- > flashrom.8.tmpl | 47 +++++++++++++-------------- > flashrom.c | 8 +++-- > layout.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++------- > 3 files changed, 116 insertions(+), 37 deletions(-) > > diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl > index 5ede423..b1371e4 100644 > --- a/flashrom.8.tmpl > +++ b/flashrom.8.tmpl > @@ -6,7 +6,7 @@ flashrom \- detect, read, write, verify and erase flash chips > \fB\-p\fR <programmername>[:<parameters>] > [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] \ > [\fB\-c\fR <chipname>] > - [\fB\-l\fR <file> [\fB\-i\fR <image>]] [\fB\-n\fR] [\fB\-f\fR]] > + [\fB\-l\fR <file> [\fB\-i\fR <image>[:<file>]]...] [\fB\-n\fR] [\fB\-f\fR]] The naming conflict between "image" (region) and "image file" (the file we write to or read from the flash chip) is very unfortunate. Unfortunately, we also lack good free single letters for a better name. While --region (instead of --image) would be a really good choice for the long option name, -r (read) and -R (revision/version) are already used and -i is an admittedly difficult association for --region. The new syntax also has one catch: You can't specify a separate file for a region without having that region actively written/read. While this won't be a problem for ordinary users, it might pose a problem for vendors like Google who want to use flashrom inside their firmware update process. Such update processes (at least when I look at the processes of other vendors) often do specify all files to source an image from, but they desire to select the to-be-written parts of the flash separately. Not sure if there is any practical relevance to this observation, though. David? > [\fB\-V\fR[\fBV\fR[\fBV\fR]]] [\fB-o\fR <logfile>] > .SH DESCRIPTION > .B flashrom > @@ -103,44 +103,45 @@ size for the flash bus. > * Force write even if write is known bad. > .TP > .B "\-l, \-\-layout <file>" > -Read ROM layout from > +Read layout entries from Do we want to call them "ROM layout", "flash layout" or simply "layout"? My preferences are 1. "ROM layout" 2. "flash layout". > .BR <file> . > .sp > -flashrom supports ROM layouts. This allows you to flash certain parts of > -the flash chip only. A ROM layout file contains multiple lines with the > -following syntax: > +A layout entry describes an address region of the flash chip and gives it a name. This allows to access certain > +parts of the flash chip only. A layout file can contain multiple entries one per line with the following syntax: I am not a native speaker, but the sentences above look odd. Suggested rewording : flashrom supports reading/writing/erasing flash chips in whole (default) or in part (when a ROM layout is given). Layouts are stored in layout files. A layout entry describes an address region/range in the virtual file representing the flash chip contents and assigns a name to that region. A layout file consists of one or more lines with the following syntax: > .sp > -.B " startaddr:endaddr imagename" > +.B " startaddr:endaddr regionname" > .sp > .BR "startaddr " "and " "endaddr " > -are hexadecimal addresses within the ROM file and do not refer to any > -physical address. Please note that using a 0x prefix for those hexadecimal > +are hexadecimal addresses within the ROM file representing the flash ROM contents. are hexadecimal addresses within the virtual file representing the flash chip contents > +Please note that using a 0x prefix for those hexadecimal > numbers is not necessary, but you can't specify decimal/octal numbers. > -.BR "imagename " "is an arbitrary name for the region/image from" > -.BR " startaddr " "to " "endaddr " "(both addresses included)." > +.BR "regionname " "is an arbitrary name for the region from " > +.BR "startaddr " "to " "endaddr " "(both addresses included)." > .sp > -Example: > +Example content of file rom.layout: > .sp > 00000000:00008fff gfxrom > 00009000:0003ffff normal > 00040000:0007ffff fallback We should use 0x prefixes in the example. That helps readability. > .sp > -If you only want to update the image named > -.BR "normal " "in a ROM based on the layout above, run" > -.sp > -.B " flashrom \-p prog \-\-layout rom.layout \-\-image normal \-w some.rom" > +.TP > +.B "\-i, \-\-image <name>[:<file>]" > +Work on the flash region > +.B name > +instead of the full address space if a layout file is given and parsed correctly. > +Multiple such image parameters can be used to include the union of different regions. s/include/work on/ > +The optional > +.B file > +parameter specifies the name of an image file that is used to map the contents of that very region only. "image file" naming again. > +This is useful for example if you want to write only a part of the flash chip and leave everything else alone > +without preparing an image of the complete chip manually: Uhh... sounds denglish. Suggested rewording: This allows replacing part of a ROM file with contents from another file before writing the resulting contents to the flash chip, eliminating the separate step of manually assembling a combined ROM image. > .sp > -To update only the images named > -.BR "normal " "and " "fallback" ", run:" > +If you only want to update the image named Plural from "images" was removed by accident. > +.BR "normal " "and " "fallback " "in a ROM based on the layout mentioned above, run" > .sp > .B " flashrom \-p prog \-l rom.layout \-i normal -i fallback \-w some.rom" > .sp > -Overlapping sections are not supported. > -.TP > -.B "\-i, \-\-image <imagename>" > -Only flash region/image > -.B <imagename> > -from flash layout. > +.RB "Overlapping sections are resolved in an implementation-dependent manner - do " "not " "rely on it." I reserve the right to have flashrom error out on overlapping regions unless you can convince me such an error would be a bad idea. Suggested rewording: .RB "Overlapping region definitions are resolved in an implementation-dependent manner or may yield an error - do " "not " "rely on any observed flashrom behaviour." > .TP > .B "\-L, \-\-list\-supported" > List the flash chips, chipsets, mainboards, and external programmers Regards, Carl-Daniel -- http://www.hailfinger.org/

3 9

Re: [flashrom] [PATCH 4/4] layout: Better file parsing.
by Carl-Daniel Hailfinger Sept. 17, 2013

Sept. 17, 2013

[Incomplete review, more follows later. Please already respond to this so I can decide how to proceed with the review.] Am 12.09.2013 22:40 schrieb Stefan Tauner: > A complete rewrite of the layout file parser: > - safe (unlike before) > - support for comments > - supports including/sourcing of other layout files (relative to the including > file as well as via absolute paths) > - checks for duplicate region names (breaks inclusion loops too) > - can handle region names and layout file names with spaces correctly > - way better diagnostics > - no more character arrays in struct romentry_t but pointers only > > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> General comments about the format: Can we change the first line slightly to # flashrom layout v2 and also mandate that region names may neither contain whitespace nor ":" now "#", please? I'd also like to drop support for v1 layouts as part of the switchover. There is a conversion script and thus no reason to support the old interface any more. The command line invocation changed anyway, so nobody will be able to reuse old commands, and then there's no point to reuse old layout files. This should reduce the amount of code needed. > flashrom.8.tmpl | 50 +++++--- > layout.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 331 insertions(+), 67 deletions(-) > > diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl > index b1371e4..8793e91 100644 > --- a/flashrom.8.tmpl > +++ b/flashrom.8.tmpl > @@ -106,23 +106,46 @@ size for the flash bus. > Read layout entries from > .BR <file> . > .sp > -A layout entry describes an address region of the flash chip and gives it a name. This allows to access certain > -parts of the flash chip only. A layout file can contain multiple entries one per line with the following syntax: > +A layout file is useful for interacting with parts of a flash chip only. It can contain comments which are > +started by a pound sign (#) and causes all following characters on the same line to be ignored with one > +exception: Every layout file > +.B must > +start with a comment in the following form to define the version of the file. Example for the current version 2: > +.sp > +.B " # flashrom layout 2" > +.sp > +Every other line may contain either a command to include another layout file, a layout entry or white space > +(and an optional comment). > +.sp > +To include another file you can use the > +.sp > +.B " source <path>" > +.sp > +syntax where > +.B path > +specifies the absolute or relative path to the file to be included. Relative paths are interpreted to be > +relative to the file containing the include command. If the path contains spaces it has to be written in double > +quotes, or else only the part before the first space will be used. Ouch! Can we please either enforce double-quotes everywhere or nowhere? > +.sp > +Each layout entry describes an address region of the flash chip and gives it a name. One entry per line is > +allowed with the following syntax: > .sp > .B " startaddr:endaddr regionname" > .sp > .BR "startaddr " "and " "endaddr " > -are hexadecimal addresses within the ROM file representing the flash ROM contents. > -Please note that using a 0x prefix for those hexadecimal > -numbers is not necessary, but you can't specify decimal/octal numbers. > -.BR "regionname " "is an arbitrary name for the region from " > -.BR "startaddr " "to " "endaddr " "(both addresses included)." > +are addresses within the ROM file representing the flash ROM contents. They are interpreted in the 'usual' form > +i.e.\ a leading 0 means octal, leading 0x or 0X means hexadecimal, everything else is just decimal. Octal support is a really bad idea IMHO. Especially when people try to align numbers by adding leading zeros. > +.BR "regionname " "is the name for the region from " "startaddr " "to " "endaddr " "(both addresses included)." > +If the name contains spaces it has to be written in double quotes, or else only the part before the first space > +will be used. > .sp > Example content of file rom.layout: > .sp > - 00000000:00008fff gfxrom > - 00009000:0003ffff normal > - 00040000:0007ffff fallback > + # flashrom layout 2 > + source /home/flashrom/include.layout > + 0x00000000:0x00008fff "gfx rom" > + 0x00009000:0x0003ffff normal > + 0x00040000:0x0007ffff fallback > .sp > .TP > .B "\-i, \-\-image <name>[:<file>]" > @@ -136,10 +159,11 @@ parameter specifies the name of an image file that is used to map the contents o > This is useful for example if you want to write only a part of the flash chip and leave everything else alone > without preparing an image of the complete chip manually: > .sp > -If you only want to update the image named > -.BR "normal " "and " "fallback " "in a ROM based on the layout mentioned above, run" > +If you only want to update the regions named > +.BR "normal " "and " "gfx rom " "in a ROM based on the layout mentioned above, run" > .sp > -.B " flashrom \-p prog \-l rom.layout \-i normal -i fallback \-w some.rom" > + > +.B " flashrom \-p prog \-l rom.layout \-i normal -i ""gfx rom"" \-w some.rom" > .sp > .RB "Overlapping sections are resolved in an implementation-dependent manner - do " "not " "rely on it." > .TP BIG FAT NOTE: The review below is of a patch which had the deleted lines stripped out to improve readability (the patch was almost unreadable due to code moves). > diff --git a/layout.c b/layout.c > index 20fe303..5b4bee4 100644 > --- a/layout.c > +++ b/layout.c > @@ -22,9 +22,11 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <ctype.h> > #include <limits.h> > #include <errno.h> > #ifndef __LIBPAYLOAD__ > +#include <libgen.h> Not available on Windows AFAICS. > #include <fcntl.h> > #include <sys/stat.h> > #endif > @@ -32,12 +34,15 @@ > #include "programmer.h" > > #define MAX_ROMLAYOUT 32 > +#define MAX_ENTRY_LEN 1024 > +#define WHITESPACE_CHARS " \t" > +#define INCLUDE_INSTR "source" > > typedef struct { > unsigned int start; > unsigned int end; > unsigned int included; > - char name[256]; > + char *name; > char *file; > } romentry_t; > > @@ -50,59 +55,307 @@ static int num_rom_entries = 0; /* the number of valid rom_entries */ > static char *include_args[MAX_ROMLAYOUT]; > static int num_include_args = 0; /* the number of valid include_args. */ > > +/* returns the index of the entry (or a negative value if it is not found) */ > +static int find_romentry(char *name) > { > int i; > + msg_gspew("Looking for region \"%s\"... ", name); > + for (i = 0; i < num_rom_entries; i++) { > + if (strcmp(rom_entries[i].name, name) == 0) { > + msg_gspew("found.\n"); > + return i; > + } > + } > + msg_gspew("not found.\n"); > + return -1; > +} > > +/* FIXME: While libpayload has no file I/O itself, code using libflashrom could still provide layout information > + * obtained by other means like user input or by fetching it from somewhere else. Therefore the parsing code > + * should be separated from the file reading code eventually. */ > +#ifndef __LIBPAYLOAD__ > +/** Parse a \em possibly quoted string. > + * > + * The function expects \a startp to point to the string to be parsed without leading white space. If the > + * string starts with a quotation mark it looks for the second one and removes both in-place, if not then it > + * looks for the first white space character and uses that as delimiter of the wanted string. > + * After returning \a startp will point to a string that is either the original string with the quotation marks > + * removed, or the first word of that string before a white space. > + * If \a endp is not NULL it will be set to point to the first character after the parsed string, which might > + * well be the '\0' at the end of the string pointed to by \a startp if there are no more characters. > + * > + * @param start Points to the input string. > + * @param end Is set to the first char following the input string. > + * @return 0 on success, 1 if the parsed string is empty or a quotation mark is not matched. > + */ > +static int unquote_string(char **startp, char **endp) If we disallow spaces in region names and if we make include file names either always-quoted or never-quoted, this complete function can die. > +{ > + char *end; > + size_t len; > + if (**startp == '"') { > + (*startp)++; > + len = strcspn(*startp, "\""); > + } else { > + len = strcspn(*startp, WHITESPACE_CHARS); > + } > + if (len == 0) > + return 1; > > + end = *startp + len; > + if (*end != '\0') { > + *end = '\0'; > + end++; > } > + if (endp != NULL) > + *endp = end; > + msg_gspew("%s: start=\"%s\", end=\"%s\"\n", __func__, *startp, end); > + return 0; > +} > > +/** Parse one line in a layout file. > + * @param file_name The name of the file this line originates from. > + * @param file_version The version of the layout specification to be used. Only v2, please. > + * @param entry If not NULL fill it with the parsed data, else just detect errors and print diagnostics. > + * @return 1 on error, > + * 0 if the line could be parsed into a layout entry succesfully, > + * -1 if a file was successfully sourced. Very odd choice of return codes. Negative values are usually associated with errors. -- http://www.hailfinger.org/

2 3

Here's my flashrom-V to help support my motherboard/chipset/flash chip s-io...
by Joshua Gnitecki Sept. 15, 2013

Sept. 15, 2013

[root@manjaro joshuag]# flashrom --programmer internal --chip W25Q80.V -V flashrom v0.9.7-r1711 on Linux 3.4.61-1-MANJARO (x86_64) flashrom is free software, get the source code at http://www.flashrom.org flashrom was built with libpci 3.2.0, GCC 4.8.1 20130725 (prerelease), little endian Command line (5 args): flashrom --programmer internal --chip W25Q80.V -V Calibrating delay loop... OS timer resolution is 1 usecs, 1405M loops per second, 10 myus = 10 us, 100 myus = 100 us, 1000 myus = 999 us, 10000 myus = 9998 us, 4 myus = 4 us, OK. Initializing internal programmer No coreboot table found. DMI string system-manufacturer: "Hewlett-Packard" DMI string system-product-name: "s5500z" DMI string system-version: " " DMI string baseboard-manufacturer: "PEGATRON CORPORATION" DMI string baseboard-product-name: "2A99" DMI string baseboard-version: "6.01" DMI string chassis-type: "Desktop" Found chipset "NVIDIA MCP61" with PCI ID 10de:03e0. Enabling flash write... This chipset is not really supported yet. Guesswork... ISA/LPC bridge reg 0x8a contents: 0x40, bit 6 is 1, bit 5 is 0 Flash bus type is SPI SPI on this chipset is WIP. Please report any success or failure by mailing us the verbose output to flashrom(a)flashrom.org, thanks! Found SMBus device 10de:03eb at 00:01:1 MCP SPI BAR is at 0xfec80000 Mapping NVIDIA MCP6x SPI at 0x00000000fec80000, unaligned size 0x544. SPI control is 0x0002, req=0, gnt=0 Please send the output of "flashrom -V" to flashrom(a)flashrom.org with your board name: flashrom -V as the subject to help us finish support for your chipset. Thanks. OK. The following protocols are supported: SPI. Probing for Winbond W25Q80.V, 1024 kB: probe_spi_rdid_generic: id1 0xef, id2 0x4014 Found Winbond flash chip "W25Q80.V" (1024 kB, SPI) at physical address 0xfff00000. Chip status register is 0x00. This chip may contain one-time programmable memory. flashrom cannot read and may never be able to write it, hence it may not be able to completely clone the contents of this chip (see man page for details). No operations were specified. -- I'm jmon...live in Austin, TX.

1 0

2A99 Motherboard flashrom -V puked-out!
by Joshua Gnitecki Sept. 15, 2013

Sept. 15, 2013

Here's my flashrom-V to help support my motherboard/chipset/flash chip s-io... [root@manjaro joshuag]# flashrom --programmer internal --chip W25Q80.V -V flashrom v0.9.7-r1711 on Linux 3.4.61-1-MANJARO (x86_64) flashrom is free software, get the source code at http://www.flashrom.org flashrom was built with libpci 3.2.0, GCC 4.8.1 20130725 (prerelease), little endian Command line (5 args): flashrom --programmer internal --chip W25Q80.V -V Calibrating delay loop... OS timer resolution is 1 usecs, 1405M loops per second, 10 myus = 10 us, 100 myus = 100 us, 1000 myus = 999 us, 10000 myus = 9998 us, 4 myus = 4 us, OK. Initializing internal programmer No coreboot table found. DMI string system-manufacturer: "Hewlett-Packard" DMI string system-product-name: "s5500z" DMI string system-version: " " DMI string baseboard-manufacturer: "PEGATRON CORPORATION" DMI string baseboard-product-name: "2A99" DMI string baseboard-version: "6.01" DMI string chassis-type: "Desktop" Found chipset "NVIDIA MCP61" with PCI ID 10de:03e0. Enabling flash write... This chipset is not really supported yet. Guesswork... ISA/LPC bridge reg 0x8a contents: 0x40, bit 6 is 1, bit 5 is 0 Flash bus type is SPI SPI on this chipset is WIP. Please report any success or failure by mailing us the verbose output to flashrom(a)flashrom.org, thanks! Found SMBus device 10de:03eb at 00:01:1 MCP SPI BAR is at 0xfec80000 Mapping NVIDIA MCP6x SPI at 0x00000000fec80000, unaligned size 0x544. SPI control is 0x0002, req=0, gnt=0 Please send the output of "flashrom -V" to flashrom(a)flashrom.org with your board name: flashrom -V as the subject to help us finish support for your chipset. Thanks. OK. The following protocols are supported: SPI. Probing for Winbond W25Q80.V, 1024 kB: probe_spi_rdid_generic: id1 0xef, id2 0x4014 Found Winbond flash chip "W25Q80.V" (1024 kB, SPI) at physical address 0xfff00000. Chip status register is 0x00. This chip may contain one-time programmable memory. flashrom cannot read and may never be able to write it, hence it may not be able to completely clone the contents of this chip (see man page for details). No operations were specified. -- I'm jmon...live in Austin, TX.

1 0

Re: [flashrom] [PATCH 3/4] util: add script for converting legacy layout files to format 2.
by Carl-Daniel Hailfinger Sept. 15, 2013

Sept. 15, 2013

Am 12.09.2013 22:40 schrieb Stefan Tauner: > Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> Is this patch (3/4) targeting the layout files used by flashrom after 4/4 in this series is in? If yes, we should switch order between 3/4 and 4/4. Review follows later. Regards, Carl-Daniel -- http://www.hailfinger.org/

2 1