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

[Stefan Tauner]

On Mon, 16 Sep 2013 03:36:47 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 16.09.2013 02:40 schrieb Stefan Tauner:
> > On Mon, 16 Sep 2013 00:46:48 +0200
> > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
> >
> >> Am 15.09.2013 20:14 schrieb Stefan Tauner:
> >>> On Sun, 15 Sep 2013 04:15:44 +0200
> >>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> 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.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner