Re: [flashrom] [PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Carl-Daniel Hailfinger]

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 
> Signed-off-by: Stefan Tauner
>
> Signed-off-by: Stefan Tauner

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

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Stefan Tauner]

On Sun, 15 Sep 2013 04:15:44 +0200
Carl-Daniel Hailfinger  wrote:
> 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 
> > Signed-off-by: Stefan Tauner
> >
> > Signed-off-by: Stefan Tauner
>
> 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.

hm. I have always read the -i as include, but in cli_classic it is
really --image. Let's change that to -i/--include <region>:<file>?
> 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?

My 2 cents: since we define the regions in a separate file I dont see a
problem with this at all. What would be gained by having another
"enable" flag or whatever?
> >       [\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".

I dont like the word ROM in there at all. It is actually untrue in
context of flashrom and the concept behind the whole layout thing (i.e.
address ranges with attributes) are not specific to them at all. IMHO
ROM layout implies some kind of chip-specific partitioning scheme.
Please give some rationale of your preferences.
> > .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).

That is actually not true. Giving the layout alone does not make
flashrom access only parts. Arguably "supporting" does not necessarily
imply that it *does* so, but I would rather not write it like that in
the manual.
> 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

So... "virtual file representing the flash chip contents" seems to be
the new wording for... that. We should try to come up with a shorter
way to refer to that. (VFRTFCC hm... nope. :)
> > +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.

Right, thanks.
> > .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/

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

You seem to imply that the -i parameters do only overwrite parts of the
file given by -w. While this is true for the current implementation,
David and I discussed about making the file parameter after -w
optional. Also, your sentence makes it sound as if the *file* is
modified instead of the flash chip.
> > .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.

thx
> > +.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."

Redundant. Bailing out is a valid implementation-dependent resolution
strategy ;) But I can add that for clarification...

Please let us decide on the exact names for images, layouts, layout
entries, their names etc. and their respective API parameters before
discussing any complete sentences in the manual.

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Carl-Daniel Hailfinger]

Am 15.09.2013 20:14 schrieb Stefan Tauner:
> On Sun, 15 Sep 2013 04:15:44 +0200
> Carl-Daniel Hailfinger  wrote:
> > 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 
> > > Signed-off-by: Stefan Tauner
> > >
> > > Signed-off-by: Stefan Tauner
> >
> > 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.
>
> hm. I have always read the -i as include, but in cli_classic it is
> really --image. Let's change that to -i/--include <region>:<file>?

Agreed. The time since we supported --include (with a totally different
meaning) is long enough to reuse that name.
> > 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?
>
> My 2 cents: since we define the regions in a separate file I dont see a
> problem with this at all. What would be gained by having another
> "enable" flag or whatever?
> > >       [\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".
>
> I dont like the word ROM in there at all. It is actually untrue in
> context of flashrom and the concept behind the whole layout thing (i.e.
> address ranges with attributes) are not specific to them at all. IMHO
> ROM layout implies some kind of chip-specific partitioning scheme.
> Please give some rationale of your preferences.

Think of "ROM" as generic name for an image file of the contents of some
flash/ROM chip. Just google for "ROMs" and you'll see what I mean.
> > > .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).
>
> That is actually not true. Giving the layout alone does not make
> flashrom access only parts. Arguably "supporting" does not necessarily
> imply that it *does* so, but I would rather not write it like that in
> the manual.

What about
"[...] (when a ROM layout file is given and certain other conditions are
met)."
> > 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
>
> So... "virtual file representing the flash chip contents" seems to be
> the new wording for... that. We should try to come up with a shorter
> way to refer to that. (VFRTFCC hm... nope. :)

"ROM image"? "ROM file"? I do prefer "ROM image".
Suggested explanation to be added somewhere in the man page:
"For historical reasons, the contents read from or written to a flash
chip are called ROM image. Addresses within a ROM image start at 0
regardless of the address where the flash chip is mapped in hardware."
> > > .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/
>
> ok
> > > +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.
>
> You seem to imply that the -i parameters do only overwrite parts of the
> file given by -w. While this is true for the current implementation,
> David and I discussed about making the file parameter after -w
> optional.

Ouch! I think that is a really really bad idea. I can see reasons to
avoid full image files, but it feels wrong to have undefined regions in
the image. Especially if writing fails and recovery is needed.
> Also, your sentence makes it sound as if the *file* is
> modified instead of the flash chip.

Yes, my bad. Let's finish the terminology discussion first.
> > > +.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."
>
> Redundant. Bailing out is a valid implementation-dependent resolution
> strategy ;) But I can add that for clarification...

"resolved" implied success for me.
> Please let us decide on the exact names for images, layouts, layout
> entries, their names etc. and their respective API parameters before
> discussing any complete sentences in the manual.

Agreed.

Regards,
Carl-Daniel

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Stefan Tauner]

On Mon, 16 Sep 2013 00:46:48 +0200
Carl-Daniel Hailfinger  wrote:
> Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > On Sun, 15 Sep 2013 04:15:44 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 12.09.2013 22:40 schrieb Stefan Tauner:
> > > > -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".
> >
> > I dont like the word ROM in there at all. It is actually untrue in
> > context of flashrom and the concept behind the whole layout thing (i.e.
> > address ranges with attributes) are not specific to them at all. IMHO
> > ROM layout implies some kind of chip-specific partitioning scheme.
> > Please give some rationale of your preferences.
>
> Think of "ROM" as generic name for an image file of the contents of some
> flash/ROM chip. Just google for "ROMs" and you'll see what I mean.

Fair enough, but that is even worse: there is no strict definition of
what a ROM is, how it is formatted and to be interpreted - actually
there are hundreds of formats out there. So what impression does a
reader get when reading "ROM layout"? I have no idea but it is
certainly far from being non-ambiguous.

Why do you want to include it rather than just use "layout" alone and
declare what we mean exactly? We need to do the later anyway including a
description of the format of the file. There is no benefit in adding
"ROM" then.

i'd go with
 - 'region' for an address range (and associated attributes)
 - 'layout' (alternatively 'flash layout' or 'flashrom layout') for the
   data structure/concept that defines regions and their attributes
   (only its name for now, arguable other attributes assigned by the
   command line (e.g. if a region should be used))
 - 'layout file' for a file containing a clear text description of a
   layout
 - 'layout entry' for a continuous, delimited part in a layout file
   that describes a region and its attributes (which is a bit fuzzy and
   overlapping with 'region', but I like to distinguish the thing in
   files and the thing in RAM)

Then there is the "image problem":
> > > > -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
> >
> > So... "virtual file representing the flash chip contents" seems to be
> > the new wording for... that. We should try to come up with a shorter
> > way to refer to that. (VFRTFCC hm... nope. :)
>
> "ROM image"? "ROM file"? I do prefer "ROM image".
> Suggested explanation to be added somewhere in the man page:
> "For historical reasons, the contents read from or written to a flash
> chip are called ROM image. Addresses within a ROM image start at 0
> regardless of the address where the flash chip is mapped in hardware."

If we can avoid using "ROM" in relation to layouts we should avoid it
here too IMO. The main problem here is to distinguish the VFRTFCC (aka
old_- and new_contents or the concept behind that), from the image files
of whole chips, and from the files used to supply/retrieve region
contents.

- no idea regarding VFRTFCC, maybe we can get away with paraphrasing it
 - image file for files containing the content of a whole flash chip
 - region file? like above but for address ranges/regions only

Till now I was thinking of image files as images reflecting the
contents of flash... but also if they only reflect a part of the flash,
i.e. if they are really <region files>. Using it for the "big" files
only may provoke confusion(?)
> > > > .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).
> >
> > That is actually not true. Giving the layout alone does not make
> > flashrom access only parts. Arguably "supporting" does not necessarily
> > imply that it *does* so, but I would rather not write it like that in
> > the manual.
>
> What about
> "[...] (when a ROM layout file is given and certain other conditions are
> met)."

Counterproposal:
flashrom supports reading/writing/erasing 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.
> > > > +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.
> >
> > You seem to imply that the -i parameters do only overwrite parts of the
> > file given by -w. While this is true for the current implementation,
> > David and I discussed about making the file parameter after -w
> > optional.
>
> Ouch! I think that is a really really bad idea. I can see reasons to
> avoid full image files, but it feels wrong to have undefined regions in
> the image. Especially if writing fails and recovery is needed.

There is no such thing as an undefined region (yet at least). For
address ranges for which no content is supplied by the user in the form
of an <image file> or a <region file>, the old content of the flash
chip is what should be there after any flashrom invocation. Cases of
"-w -i bla" where neither an <image file> nor a <region file> are given
need to be handled as errors of course.

But anyway, your sentence still implies that the file given in -w is
modified and that's what I was concerned about actually. No idea why I
brought up the file-less -w at all (anymore) sorry.

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Carl-Daniel Hailfinger]

Am 16.09.2013 02:40 schrieb Stefan Tauner:
> On Mon, 16 Sep 2013 00:46:48 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > > On Sun, 15 Sep 2013 04:15:44 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Am 12.09.2013 22:40 schrieb Stefan Tauner:
> > > > > -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".
> > >
> > > I dont like the word ROM in there at all. It is actually untrue in
> > > context of flashrom and the concept behind the whole layout thing (i.e.
> > > address ranges with attributes) are not specific to them at all. IMHO
> > > ROM layout implies some kind of chip-specific partitioning scheme.
> > > Please give some rationale of your preferences.
> >
> > Think of "ROM" as generic name for an image file of the contents of some
> > flash/ROM chip. Just google for "ROMs" and you'll see what I mean.
>
> Fair enough, but that is even worse: there is no strict definition of
> what a ROM is, how it is formatted and to be interpreted - actually
> there are hundreds of formats out there. So what impression does a
> reader get when reading "ROM layout"? I have no idea but it is
> certainly far from being non-ambiguous.
>
> Why do you want to include it rather than just use "layout" alone and
> declare what we mean exactly? We need to do the later anyway including a
> description of the format of the file. There is no benefit in adding
> "ROM" then.
>
> i'd go with
> 'region' for an address range (and associated attributes)
> 'layout' (alternatively 'flash layout' or 'flashrom layout') for the
> data structure/concept that defines regions and their attributes
> (only its name for now, arguable other attributes assigned by the
> command line (e.g. if a region should be used))
> 'layout file' for a file containing a clear text description of a
> layout
> 'layout entry' for a continuous, delimited part in a layout file
> that describes a region and its attributes (which is a bit fuzzy and
> overlapping with 'region', but I like to distinguish the thing in
> files and the thing in RAM)

Sounds good so far.
> Then there is the "image problem":
> > > > > -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
> > >
> > > So... "virtual file representing the flash chip contents" seems to be
> > > the new wording for... that. We should try to come up with a shorter
> > > way to refer to that. (VFRTFCC hm... nope. :)
> >
> > "ROM image"? "ROM file"? I do prefer "ROM image".
> > Suggested explanation to be added somewhere in the man page:
> > "For historical reasons, the contents read from or written to a flash
> > chip are called ROM image. Addresses within a ROM image start at 0
> > regardless of the address where the flash chip is mapped in hardware."
>
> If we can avoid using "ROM" in relation to layouts we should avoid it
> here too IMO. The main problem here is to distinguish the VFRTFCC (aka
> old_- and new_contents or the concept behind that), from the image files
> of whole chips, and from the files used to supply/retrieve region
> contents.
> no idea regarding VFRTFCC, maybe we can get away with paraphrasing it
> image file for files containing the content of a whole flash chip
> region file? like above but for address ranges/regions only
>
> Till now I was thinking of image files as images reflecting the
> contents of flash... but also if they only reflect a part of the flash,
> i.e. if they are really <region files>. Using it for the "big" files
> only may provoke confusion(?)

I still like "ROM image" for the complete image. That phrase is actually
usable both for the full on-disk image file ("ROM image file") as well
as the in-memory and possibly rebuilt/reassembled image ("in-memory ROM
image" or somesuch).
What about "partial image" or "region image" for files specified with
--include region:file?
> > > > > .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).
> > >
> > > That is actually not true. Giving the layout alone does not make
> > > flashrom access only parts. Arguably "supporting" does not necessarily
> > > imply that it *does* so, but I would rather not write it like that in
> > > the manual.
> >
> > What about
> > "[...] (when a ROM layout file is given and certain other conditions are
> > met)."
>
> Counterproposal:
> flashrom supports reading/writing/erasing 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.

Ack.
> > > > > +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.
> > >
> > > You seem to imply that the -i parameters do only overwrite parts of the
> > > file given by -w. While this is true for the current implementation,
> > > David and I discussed about making the file parameter after -w
> > > optional.
> >
> > Ouch! I think that is a really really bad idea. I can see reasons to
> > avoid full image files, but it feels wrong to have undefined regions in
> > the image. Especially if writing fails and recovery is needed.
>
> There is no such thing as an undefined region (yet at least). For
> address ranges for which no content is supplied by the user in the form
> of an <image file> or a <region file>, the old content of the flash
> chip is what should be there after any flashrom invocation. Cases of
> "-w -i bla" where neither an <image file> nor a <region file> are given
> need to be handled as errors of course.

That would make an implicit read from the flash chip before any write a
hard requirement. I hear people complaining from time to time about the
initial chip read in flashrom, and this won't make it better.

Besides that, writes where part of the image is read on-the-fly from the
flash chip directly before writing are not reproducible if an error
occurs. Think of the following scenario: Writing fails, and the flash
chip is now partially corrupted due to an erase command which affected a
larger region size than expected. As a result, parts of the implicitly
read region are now erased. Repeating the same write command with a
fixed flashrom version will _not_ yield the correct flash chip contents.
User despairs, smashes the computer and takes up a career in pottery. If
the image had been built completely from on-disk files (i.e. without
implicit chip reads), repeating the same write command would have
resulted in the correct flash chip contents.
> But anyway, your sentence still implies that the file given in -w is
> modified and that's what I was concerned about actually. No idea why I
> brought up the file-less -w at all (anymore) sorry.

I see. Not sure how to express that best. Next try:
This allows replacing the contents of one ROM image region 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.

There's another problem with the patch, though: Allowing region files
for read (instead of just for write) does violate the principle of least
surprise: Some users might expect regions to be written to separate
files on disk, i.e. having -r -i as reverse operation to -w -i. Right
now that assumption is not true.

Regards,
Carl-Daniel

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Stefan Tauner]

On Mon, 16 Sep 2013 03:36:47 +0200
Carl-Daniel Hailfinger  wrote:
> Am 16.09.2013 02:40 schrieb Stefan Tauner:
> > On Mon, 16 Sep 2013 00:46:48 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > > > On Sun, 15 Sep 2013 04:15:44 +0200
> > > > Carl-Daniel Hailfinger  wrote:
> > > > > Am 12.09.2013 22:40 schrieb Stefan Tauner:
> > Then there is the "image problem":
> > > > > > -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
> > > >
> > > > So... "virtual file representing the flash chip contents" seems to be
> > > > the new wording for... that. We should try to come up with a shorter
> > > > way to refer to that. (VFRTFCC hm... nope. :)
> > >
> > > "ROM image"? "ROM file"? I do prefer "ROM image".
> > > Suggested explanation to be added somewhere in the man page:
> > > "For historical reasons, the contents read from or written to a flash
> > > chip are called ROM image. Addresses within a ROM image start at 0
> > > regardless of the address where the flash chip is mapped in hardware."
> >
> > If we can avoid using "ROM" in relation to layouts we should avoid it
> > here too IMO. The main problem here is to distinguish the VFRTFCC (aka
> > old_- and new_contents or the concept behind that), from the image files
> > of whole chips, and from the files used to supply/retrieve region
> > contents.
> > no idea regarding VFRTFCC, maybe we can get away with paraphrasing it
> > image file for files containing the content of a whole flash chip
> > region file? like above but for address ranges/regions only
> >
> > Till now I was thinking of image files as images reflecting the
> > contents of flash... but also if they only reflect a part of the flash,
> > i.e. if they are really <region files>. Using it for the "big" files
> > only may provoke confusion(?)
>
> I still like "ROM image" for the complete image. That phrase is actually
> usable both for the full on-disk image file ("ROM image file") as well
> as the in-memory and possibly rebuilt/reassembled image ("in-memory ROM
> image" or somesuch).

That's the/my problem with the word image: It may be understood as
filesystem file too even if only the in-memory image is meant. Since
the filesystem file is not necessarily equal to the in-memory image (in
case of -i options being present), I would like to clearly distinguish
them.

Regarding ROM I have the same problems here as above in "ROM layout",
although here it is less severe, because it does not allow for so many
interpretations with the given context. So I think I could very well
live with "ROM image" for VFRTFCC and "ROM image file" for the files
named by -r/-w/-v parameters. I would still prefer an alternative if we
can come up with one that. chip (or flash) image (file), complete or
(full) image (file), chip/flash content (file), ...
> What about "partial image" or "region image" for files specified with
> --include region:file?

For those the image problem stated above is not so severe, but we
should distinguish them too to aid understanding of the differences of
the whole-chip images vs. files, else region image would be fine.
> > > > > > +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.
> > > >
> > > > You seem to imply that the -i parameters do only overwrite parts of the
> > > > file given by -w. While this is true for the current implementation,
> > > > David and I discussed about making the file parameter after -w
> > > > optional.
> > >
> > > Ouch! I think that is a really really bad idea. I can see reasons to
> > > avoid full image files, but it feels wrong to have undefined regions in
> > > the image. Especially if writing fails and recovery is needed.
> >
> > There is no such thing as an undefined region (yet at least). For
> > address ranges for which no content is supplied by the user in the form
> > of an <image file> or a <region file>, the old content of the flash
> > chip is what should be there after any flashrom invocation. Cases of
> > "-w -i bla" where neither an <image file> nor a <region file> are given
> > need to be handled as errors of course.
>
> That would make an implicit read from the flash chip before any write a
> hard requirement. I hear people complaining from time to time about the
> initial chip read in flashrom, and this won't make it better.

It does not make it more required than now. The absolute minimum is
that we know what any touched block should contain eventually before we
start. This does not allow for any overall diagnostics or recovery in
any case anyway, and the infrastructure patch that follows in this
patch set has this property too. You should not add a switch to dump the
cake and be surprised if no one is able to eat it after pressing the
button - meaning that we can not support fancy recovery and diagnostics
when the user wants to skip superfluous chip reads (pre-reads and
post-write verifications). That does not mean we should force him to,
nor that we should design flashrom's architecture for only one of the
two cases. Both leads to headaches when someone starts to disagree and
forks flashrom - think of multi-billion transnational companies. ;)
> Besides that, writes where part of the image is read on-the-fly from the
> flash chip directly before writing are not reproducible if an error
> occurs. Think of the following scenario: Writing fails, and the flash
> chip is now partially corrupted due to an erase command which affected a
> larger region size than expected. As a result, parts of the implicitly
> read region are now erased. Repeating the same write command with a
> fixed flashrom version will _not_ yield the correct flash chip contents.
> User despairs, smashes the computer and takes up a career in pottery. If
> the image had been built completely from on-disk files (i.e. without
> implicit chip reads), repeating the same write command would have
> resulted in the correct flash chip contents.

This is not at all related to what we are discussing. We do not write
out the read image yet in case of errors either (nor do we try to write
that back in emergencies). This should change anyway because in some
configurations we are already not able to recover - yes we can
reproduce the problem, but users still end up in pottery business from
time to time because of that (e.g. thinkpads with unrecoverable
unit-specific data in flash). Additionally we could then write out the
complete image flashrom tried to place into the flash chip to be able
to reproduce problems too. Please let us ignore that whole discussion
until we really have to have it. No one wants to build a wall (yet ;).
> > But anyway, your sentence still implies that the file given in -w is
> > modified and that's what I was concerned about actually. No idea why I
> > brought up the file-less -w at all (anymore) sorry.
>
> I see. Not sure how to express that best. Next try:
> This allows replacing the contents of one ROM image region 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.

Not sure if one would understand the subtle difference without knowing
it beforehand. I think the use of region there might even confuse more
than it helps. The context of this sentence is the explanation of
the :<file> parameter for --include:
The optional
.B file
parameter specifies the name of a file that is used to map the contents of that very region only.

What about this:
This allows the contents of a part of the $romimage to be overlayed with
content from another file when writing.

We need to change that part of the documentation anyway when we add -r
and -v support for --include.
> There's another problem with the patch, though: Allowing region files
> for read (instead of just for write) does violate the principle of least
> surprise: Some users might expect regions to be written to separate
> files on disk, i.e. having -r -i as reverse operation to -w -i. Right
> now that assumption is not true.

Hm, eventually that should be the case of course, i.e. -r rom.bin -i
boot:boot.bin should store the $regionimage of boot into boot.bin with
all the layout patches applied. I need to look at the code again to see
what would happen with this patch applied only.

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Carl-Daniel Hailfinger]

Am 16.09.2013 15:52 schrieb Stefan Tauner:
> On Mon, 16 Sep 2013 03:36:47 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 16.09.2013 02:40 schrieb Stefan Tauner:
> > > On Mon, 16 Sep 2013 00:46:48 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > > > > On Sun, 15 Sep 2013 04:15:44 +0200
> > > > > Carl-Daniel Hailfinger  wrote:
> > > > > > Am 12.09.2013 22:40 schrieb Stefan Tauner:
> > >
> > > Then there is the "image problem":
> > > > > > > -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
> > > > >
> > > > > So... "virtual file representing the flash chip contents" seems to be
> > > > > the new wording for... that. We should try to come up with a shorter
> > > > > way to refer to that. (VFRTFCC hm... nope. :)
> > > >
> > > > "ROM image"? "ROM file"? I do prefer "ROM image".
> > > > Suggested explanation to be added somewhere in the man page:
> > > > "For historical reasons, the contents read from or written to a flash
> > > > chip are called ROM image. Addresses within a ROM image start at 0
> > > > regardless of the address where the flash chip is mapped in hardware."
> > >
> > > If we can avoid using "ROM" in relation to layouts we should avoid it
> > > here too IMO. The main problem here is to distinguish the VFRTFCC (aka
> > > old_- and new_contents or the concept behind that), from the image files
> > > of whole chips, and from the files used to supply/retrieve region
> > > contents.
> > > no idea regarding VFRTFCC, maybe we can get away with paraphrasing it
> > > image file for files containing the content of a whole flash chip
> > > region file? like above but for address ranges/regions only
> > >
> > > Till now I was thinking of image files as images reflecting the
> > > contents of flash... but also if they only reflect a part of the flash,
> > > i.e. if they are really <region files>. Using it for the "big" files
> > > only may provoke confusion(?)
> >
> > I still like "ROM image" for the complete image. That phrase is actually
> > usable both for the full on-disk image file ("ROM image file") as well
> > as the in-memory and possibly rebuilt/reassembled image ("in-memory ROM
> > image" or somesuch).
>
> That's the/my problem with the word image: It may be understood as
> filesystem file too even if only the in-memory image is meant. Since
> the filesystem file is not necessarily equal to the in-memory image (in
> case of -i options being present), I would like to clearly distinguish
> them.

Understood.
> Regarding ROM I have the same problems here as above in "ROM layout",
> although here it is less severe, because it does not allow for so many
> interpretations with the given context. So I think I could very well
> live with "ROM image" for VFRTFCC and "ROM image file" for the files
> named by -r/-w/-v parameters. I would still prefer an alternative if we
> can come up with one that. chip (or flash) image (file), complete or
> (full) image (file), chip/flash content (file), ...

Maybe someone else can weigh in. Otherwise I'd say we use what we have
at this point in the discussion.
> > What about "partial image" or "region image" for files specified with
> > --include region:file?
>
> For those the image problem stated above is not so severe, but we
> should distinguish them too to aid understanding of the differences of
> the whole-chip images vs. files, else region image would be fine.

sub-image? chunk image? region image? partial image? Lots of possible
names come to mind. "region image file" has the advantage of making it
clear that we're dealing with a file, and that it has something to do
with regions.
> > > > > > > +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.
> > > > >
> > > > > You seem to imply that the -i parameters do only overwrite parts of the
> > > > > file given by -w. While this is true for the current implementation,
> > > > > David and I discussed about making the file parameter after -w
> > > > > optional.
> > > >
> > > > Ouch! I think that is a really really bad idea. I can see reasons to
> > > > avoid full image files, but it feels wrong to have undefined regions in
> > > > the image. Especially if writing fails and recovery is needed.
> > >
> > > There is no such thing as an undefined region (yet at least). For
> > > address ranges for which no content is supplied by the user in the form
> > > of an <image file> or a <region file>, the old content of the flash
> > > chip is what should be there after any flashrom invocation. Cases of
> > > "-w -i bla" where neither an <image file> nor a <region file> are given
> > > need to be handled as errors of course.
> >
> > That would make an implicit read from the flash chip before any write a
> > hard requirement. I hear people complaining from time to time about the
> > initial chip read in flashrom, and this won't make it better.
>
> It does not make it more required than now. The absolute minimum is
> that we know what any touched block should contain eventually before we
> start. This does not allow for any overall diagnostics or recovery in
> any case anyway, and the infrastructure patch that follows in this
> patch set has this property too. You should not add a switch to dump the
> cake and be surprised if no one is able to eat it after pressing the
> button - meaning that we can not support fancy recovery and diagnostics
> when the user wants to skip superfluous chip reads (pre-reads and
> post-write verifications). That does not mean we should force him to,
> nor that we should design flashrom's architecture for only one of the
> two cases. Both leads to headaches when someone starts to disagree and
> forks flashrom - think of multi-billion transnational companies. ;)

Heh.
> > Besides that, writes where part of the image is read on-the-fly from the
> > flash chip directly before writing are not reproducible if an error
> > occurs. Think of the following scenario: Writing fails, and the flash
> > chip is now partially corrupted due to an erase command which affected a
> > larger region size than expected. As a result, parts of the implicitly
> > read region are now erased. Repeating the same write command with a
> > fixed flashrom version will _not_ yield the correct flash chip contents.
> > User despairs, smashes the computer and takes up a career in pottery. If
> > the image had been built completely from on-disk files (i.e. without
> > implicit chip reads), repeating the same write command would have
> > resulted in the correct flash chip contents.
>
> This is not at all related to what we are discussing. We do not write
> out the read image yet in case of errors either (nor do we try to write
> that back in emergencies). This should change anyway because in some
> configurations we are already not able to recover - yes we can
> reproduce the problem, but users still end up in pottery business from
> time to time because of that (e.g. thinkpads with unrecoverable
> unit-specific data in flash). Additionally we could then write out the
> complete image flashrom tried to place into the flash chip to be able
> to reproduce problems too. Please let us ignore that whole discussion
> until we really have to have it. No one wants to build a wall (yet ;).

Agreed, let's postpone this.
> > > But anyway, your sentence still implies that the file given in -w is
> > > modified and that's what I was concerned about actually. No idea why I
> > > brought up the file-less -w at all (anymore) sorry.
> >
> > I see. Not sure how to express that best. Next try:
> > This allows replacing the contents of one ROM image region 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.
>
> Not sure if one would understand the subtle difference without knowing
> it beforehand. I think the use of region there might even confuse more
> than it helps. The context of this sentence is the explanation of
> the :<file> parameter for --include:
> The optional
> .B file
> parameter specifies the name of a file that is used to map the contents of that very region only.

That's also not really an optimal way to express the parameter. Strictly
speaking, the file is not used to map contents of the region, but the
other way round (mostly).
> What about this:
> This allows the contents of a part of the $romimage to be overlayed with
> content from another file when writing.

Mh. How about:
The optional file parameter causes the contents of this region to be
replaced by the contents of the file specified here.
> We need to change that part of the documentation anyway when we add -r
> and -v support for --include.

Right. This also means we have to disallow --include for -r until -r
support is in. Does it even make sense to support --include for -v?
> > There's another problem with the patch, though: Allowing region files
> > for read (instead of just for write) does violate the principle of least
> > surprise: Some users might expect regions to be written to separate
> > files on disk, i.e. having -r -i as reverse operation to -w -i. Right
> > now that assumption is not true.
>
> Hm, eventually that should be the case of course, i.e. -r rom.bin -i
> boot:boot.bin should store the $regionimage of boot into boot.bin with
> all the layout patches applied. I need to look at the code again to see
> what would happen with this patch applied only.

AFAICS with this patch applied, -r rom.bin -i boot:boot.bin will read
the whole chip, then replace region "boot" with the contents from
boot.bin, then write the resulting image into rom.bin.

Regards,
Carl-Daniel

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[Stefan Tauner]

On Tue, 17 Sep 2013 10:55:37 +0200
Carl-Daniel Hailfinger  wrote:
> Am 16.09.2013 15:52 schrieb Stefan Tauner:
> > On Mon, 16 Sep 2013 03:36:47 +0200
> > Carl-Daniel Hailfinger  wrote:
> > > Am 16.09.2013 02:40 schrieb Stefan Tauner:
> > > > On Mon, 16 Sep 2013 00:46:48 +0200
> > > > Carl-Daniel Hailfinger  wrote:
> > > > > Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > > > > > On Sun, 15 Sep 2013 04:15:44 +0200
> > > > > > Carl-Daniel Hailfinger  wrote:
> > > > > >
> > > > > > > Am 12.09.2013 22:40 schrieb Stefan Tauner:
> >
> > Regarding ROM I have the same problems here as above in "ROM layout",
> > although here it is less severe, because it does not allow for so many
> > interpretations with the given context. So I think I could very well
> > live with "ROM image" for VFRTFCC and "ROM image file" for the files
> > named by -r/-w/-v parameters. I would still prefer an alternative if we
> > can come up with one that. chip (or flash) image (file), complete or
> > (full) image (file), chip/flash content (file), ...
>
> Maybe someone else can weigh in. Otherwise I'd say we use what we have
> at this point in the discussion.

David?
> > > What about "partial image" or "region image" for files specified with
> > > --include region:file?
> >
> > For those the image problem stated above is not so severe, but we
> > should distinguish them too to aid understanding of the differences of
> > the whole-chip images vs. files, else region image would be fine.
>
> sub-image? chunk image? region image? partial image? Lots of possible
> names come to mind. "region image file" has the advantage of making it
> clear that we're dealing with a file, and that it has something to do
> with regions.

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.
> > > > But anyway, your sentence still implies that the file given in -w is
> > > > modified and that's what I was concerned about actually. No idea why I
> > > > brought up the file-less -w at all (anymore) sorry.
> > >
> > > I see. Not sure how to express that best. Next try:
> > > This allows replacing the contents of one ROM image region 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.
> >
> > Not sure if one would understand the subtle difference without knowing
> > it beforehand. I think the use of region there might even confuse more
> > than it helps. The context of this sentence is the explanation of
> > the :<file> parameter for --include:
> > The optional
> > .B file
> > parameter specifies the name of a file that is used to map the contents of that very region only.
>
> That's also not really an optimal way to express the parameter. Strictly
> speaking, the file is not used to map contents of the region, but the
> other way round (mostly).

Hm well, are memory mapped files a map of memory or is the memory a map
of the file...? For bijective maps (aka functions) there is usually a
trivial reverse map - so to me "file maps regions" and "region maps
file" are synonymous. NB: I don't use the preposition "to" at all, which
might give a hint of the direction of the function. But of course one
can read the subject and object of the sentence as direction hint too.
Being terse and unambiguous is hard :)
> > What about this:
> > This allows the contents of a part of the $romimage to be overlayed with
> > content from another file when writing.
>
> Mh. How about:
> The optional file parameter causes the contents of this region to be
> replaced by the contents of the file specified here.

That's fine for the current -w-only case and I'll add it to this patch,
but we will need a longer explanation for the future cases.
> > We need to change that part of the documentation anyway when we add -r
> > and -v support for --include.
>
> Right. This also means we have to disallow --include for -r until -r
> support is in. Does it even make sense to support --include for -v?

In general, sure why not? If you want to make sure that the bootblock
is fine only, or if you know only half the flash contains useful data
and dont care if the rest is erased or not...

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[David Hendricks]

On Tue, Sep 17, 2013 at 2:48 AM, Stefan Tauner <
[e-mail]> wrote:
> On Tue, 17 Sep 2013 10:55:37 +0200
> Carl-Daniel Hailfinger  wrote:
> > Am 16.09.2013 15:52 schrieb Stefan Tauner:
> > > On Mon, 16 Sep 2013 03:36:47 +0200
> > > Carl-Daniel Hailfinger  wrote:
> > > > Am 16.09.2013 02:40 schrieb Stefan Tauner:
> > > > > On Mon, 16 Sep 2013 00:46:48 +0200
> > > > > Carl-Daniel Hailfinger  wrote:
> > > > > > Am 15.09.2013 20:14 schrieb Stefan Tauner:
> > > > > > > On Sun, 15 Sep 2013 04:15:44 +0200
> > > > > > > Carl-Daniel Hailfinger  wrote:
> > > > > > >
> > > > > > >> Am 12.09.2013 22:40 schrieb Stefan Tauner:
> > >
> > > Regarding ROM I have the same problems here as above in "ROM layout",
> > > although here it is less severe, because it does not allow for so many
> > > interpretations with the given context. So I think I could very well
> > > live with "ROM image" for VFRTFCC and "ROM image file" for the files
> > > named by -r/-w/-v parameters. I would still prefer an alternative if we
> > > can come up with one that. chip (or flash) image (file), complete or
> > > (full) image (file), chip/flash content (file), ...
> >
> > Maybe someone else can weigh in. Otherwise I'd say we use what we have
> > at this point in the discussion.
>
> David?

Perhaps a glossary in the man page would be useful?

"Image" sounds good IMO, or least bad. I don't think English has a single
word that is perfect for this, so we'll just be careful about context. I
suggest using "image" to describe the software bits which compose a
complete object. It may have regions within it and its size might not match
the physical ROM size. A "ROM image" is an image which exists on ROM.
"Image file" is an image which exists as a file. We typically manipulate
images in memory so those qualifiers are not necessary until the image is
written to storage.

But please let's please avoid VFRTFCC :-)
> > > What about "partial image" or "region image" for files specified with
> > > > --include region:file?
> > >
> > > For those the image problem stated above is not so severe, but we
> > > should distinguish them too to aid understanding of the differences of
> > > the whole-chip images vs. files, else region image would be fine.
> >
> > sub-image? chunk image? region image? partial image? Lots of possible
> > names come to mind. "region image file" has the advantage of making it
> > clear that we're dealing with a file, and that it has something to do
> > with regions.
>
> 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.

[PATCH 2/4] layout: Add -i <image>[:<file>] support.

[David Hendricks]

On Sun, Sep 15, 2013 at 6:36 PM, Carl-Daniel Hailfinger <
[e-mail]> wrote:
> > > > > > +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.
> > > >
> > > > You seem to imply that the -i parameters do only overwrite parts of the
> > > > file given by -w. While this is true for the current implementation,
> > > > David and I discussed about making the file parameter after -w
> > > > optional.
> > >
> > > Ouch! I think that is a really really bad idea. I can see reasons to
> > > avoid full image files, but it feels wrong to have undefined regions in
> > > the image. Especially if writing fails and recovery is needed.
> >
> > There is no such thing as an undefined region (yet at least). For
> > address ranges for which no content is supplied by the user in the form
> > of an <image file> or a <region file>, the old content of the flash
> > chip is what should be there after any flashrom invocation. Cases of
> > "-w -i bla" where neither an <image file> nor a <region file> are given
> > need to be handled as errors of course.
>
> That would make an implicit read from the flash chip before any write a
> hard requirement. I hear people complaining from time to time about the
> initial chip read in flashrom, and this won't make it better.
>
> Besides that, writes where part of the image is read on-the-fly from the
> flash chip directly before writing are not reproducible if an error
> occurs. Think of the following scenario: Writing fails, and the flash
> chip is now partially corrupted due to an erase command which affected a
> larger region size than expected. As a result, parts of the implicitly
> read region are now erased. Repeating the same write command with a
> fixed flashrom version will _not_ yield the correct flash chip contents.
> User despairs, smashes the computer and takes up a career in pottery. If
> the image had been built completely from on-disk files (i.e. without
> implicit chip reads), repeating the same write command would have
> resulted in the correct flash chip contents.

The old contents of the flash are already read regardless. I don't see how
recovering from failure is an issue here, unless we change the default
behavior (I don't recommend doing that).

For chromeos we added a "--fast-verify" option to skip reading and
verifying non-included regions when doing particular writes. It's intended
for use in controlled circumstances (e.g. development with an external
programmer).
> > But anyway, your sentence still implies that the file given in -w is
> > modified and that's what I was concerned about actually. No idea why I
> > brought up the file-less -w at all (anymore) sorry.
>
> I see. Not sure how to express that best. Next try:
> This allows replacing the contents of one ROM image region 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.

I'd recommend stating this in concrete terms that translate directly to
command-line usage: The contents of any -i option takes priority over the
contents of the -w option.

BTW, I tried to distill some of these rules for syntax parsing @ line 686
here:
https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/master/c....

Feel free to borrow any wording if it helps.
> There's another problem with the patch, though: Allowing region files
> for read (instead of just for write) does violate the principle of least
> surprise: Some users might expect regions to be written to separate
> files on disk, i.e. having -r -i as reverse operation to -w -i. Right
> now that assumption is not true.

Why would a user provide a filename to a -i option along with -r if they
did not intend for a file to be written to disk?