[flashrom] [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 <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:
>>>>> -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

-- 
http://www.hailfinger.org/