Hi
Currently flashrom has support for 4 different mechanisms for providing layout, namely a plain layout file, on Intel platforms fetching the layout from IFD, using the coreboot specific FMAP layout either on file or on the flash. The way we handle those methods is awkward and does not scale very well. Adding new layout formats is quite awkward at the moment.
The following could be improved on: - Currently each methods of providing a layouts is mutually exclusive so care must be taken. This makes the code flow hard to read in the classic_cli.c code, since some methods for fetching the layout first need flash access, while others don't. - layout.c file has a global variable for holding the layout entries that sometimes methods append entries to, sometimes gets overwritten with a new layout. - The mutual exclusiveness of getting layouts sometimes stands in the way of implementing features that requires multiple methods. For instance if your Intel FD says a region is RO it makes not sense try to override that region if your programmer is an Intel southbridge. - the layout entries array is too small. Some google FMAPs have more entries than our current MAX. While increasing the MAX is no issue, having a layout that can simply grow on demand seems preferable. Using a linked list seems to be ideal for this. Appending entries to linked list is also easy as opposed to appending to arrays of region entries. - afaict our manual says included regions cannot overlap but this is not enforced. For read and erase this does not matter much, for writes this seems like a bad idea or at least needs to be very clear on region takes precedence.
So what I propose: - Use a single linked list for layouts (or regions in general) and pass the HEAD pointer around to append entries instead of using a global variable - make the methods of fetching the layout entries always append entries instead of outputting a new layout file. This could imply a libflashrom API change. (I don't have a strong opinion on this one as appending linked lists to each other is quite trivial) - Make methods for fetching layouts not exclusive, while checking that entries with the same name don't have different entries. E.g. if IFD says "bios" is 0x00000000:0x0020000 it should bail out if a layoutfile says something else. BTW with FMAP this can if desired also serve as a sanity check that the thing you are trying to flash also has the same FMAP as what resides on the flash. If this is implemented there is no real reason to block multiple layout files. - Fetch the layout entries in common places in cli_classic.c: layouts from files during the arg processing, layouts from flash where it is currently done (after programmer is ready) - Bail out on operations on overlapping regions, preferably in a function that appends layout entries, which can check for other things like double entries - get rid of the special case where no region + layout is included, but initialize the layout with a single entry spanning the whole flash. (get rid of struct single_layout) - Add some flags that can later be used to determine whether it is a good idea to try to write things, e.g. whether RO regions overlap with included regions on write operations.
So the layout.h would look somethings like the following:
=====layout.h===== struct romentry { chipoff_t start; chipoff_t end; bool included; bool write_possible; bool read_possible; char name[256]; };
/* Linked list entries */ struct flashrom_layout { struct romentry entry; struct flashrom_layout *next; };
/* No need for this anymore * struct single_layout { * struct flashrom_layout base; * struct romentry entry; * }; */
/* Linked list for include args. append when fetching layouts entries */ struct layout_include_args { char *name; struct layout_include_args *next; };
/* Deal with include args: * Add the included flag if everything is ok: no overlapping region, name * present in layout list. To be called after a layout entries are * gathered in the linked list. */ int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l); =====layout.h=====
So what do you think of this proposal?
Kind regards
Arthur Heymans
Hi Arthur,
On 05.02.19 20:01, Arthur Heymans wrote:
Hi
Currently flashrom has support for 4 different mechanisms for providing layout, namely a plain layout file, on Intel platforms fetching the layout from IFD, using the coreboot specific FMAP layout either on file or on the flash. The way we handle those methods is awkward and does not scale very well. Adding new layout formats is quite awkward at the moment.
The following could be improved on:
- Currently each methods of providing a layouts is mutually exclusive so
care must be taken. This makes the code flow hard to read in the classic_cli.c code, since some methods for fetching the layout first need flash access, while others don't.
I think we could fix the latter by moving all layout generation, even reading from plain files, after the flash-chip probing. I fear, I just didn't start this when adding IFD layout support because it was still a time of "touch the least to get your patch reviewed".
- layout.c file has a global variable for holding the layout entries
that sometimes methods append entries to, sometimes gets overwritten with a new layout.
- The mutual exclusiveness of getting layouts sometimes stands in the
way of implementing features that requires multiple methods. For instance if your Intel FD says a region is RO it makes not sense try to override that region if your programmer is an Intel southbridge.
- the layout entries array is too small. Some google FMAPs have more
entries than our current MAX. While increasing the MAX is no issue, having a layout that can simply grow on demand seems preferable. Using a linked list seems to be ideal for this. Appending entries to linked list is also easy as opposed to appending to arrays of region entries.
Agreed.
- afaict our manual says included regions cannot overlap but this is not
enforced. For read and erase this does not matter much, for writes this seems like a bad idea or at least needs to be very clear on region takes precedence.
It really only matters when you have conflicting data sources (e.g. dif- ferent files) for the overlapping regions.
So what I propose:
- Use a single linked list for layouts (or regions in general) and pass
the HEAD pointer around to append entries instead of using a global variable
- make the methods of fetching the layout entries always append entries
instead of outputting a new layout file. This could imply a libflashrom API change. (I don't have a strong opinion on this one as appending linked lists to each other is quite trivial)
We could just extend the API. e.g. for the current
flashrom_layout_read_from_ifd(struct flashrom_layout **, ...
add a
flashrom_layout_append_from_ifd(struct flashrom_layout *, ...
Along with some convenience functions, e.g.
flashrom_layout_merge(struct flashrom_layout *, struct flashrom_layout *); flashrom_layout_append(struct flashrom_layout *, const struct flashrom_layout *);
where the former would append and destroy the second layout and the latter would leave it be. Just a recent thought. Maybe we don't need this in the library API.
- Make methods for fetching layouts not exclusive, while checking that
entries with the same name don't have different entries. E.g. if IFD says "bios" is 0x00000000:0x0020000 it should bail out if a layoutfile says something else. BTW with FMAP this can if desired also serve as a sanity check that the thing you are trying to flash also has the same FMAP as what resides on the flash. If this is implemented there is no real reason to block multiple layout files.
I like the sanity-check idea. However, it should be optional as long as the resulting operation is unambiguous (i.e. no conflicting data sources for overlapping regions). It's quite possible to come across a board where an FMAP conflicts with the IFD (e.g. BIOS region is just a subset in FMAP). So I'd prefer a warning, plus optionally bail-out if the user said so.
This leaves the question how to identify regions with ambiguous names? Maybe keep a prefix string along each entry that identifies its source. So, if a region name is unambiguous or all entries agree on the layout, you could still just specify the region name `-i bios`. But if, for instance, FMAP and IFD disagree, you'd have to specify the prefix too `-i IFD/bios`.
Also maybe worth a prefix, layout hierarchies. FMAP actually provides hierarchical layouts. For coreboot's default-x86.fmd for instance, the CBFS would actually reside in `FMAP/BIOS/COREBOOT`.
- Fetch the layout entries in common places in cli_classic.c: layouts
from files during the arg processing, layouts from flash where it is currently done (after programmer is ready)
I'd prefer one single common place (see above).
- Bail out on operations on overlapping regions, preferably in a
function that appends layout entries, which can check for other things like double entries
This would require us to take into account which regions matter, e.g. what regions the user wants to use, because the hierarchical FMAP regions overlap by definition. It's doable. The alternative would be to sanity check on every operation that is going to actually work with the layout on data.
- get rid of the special case where no region + layout is included, but
initialize the layout with a single entry spanning the whole flash. (get rid of struct single_layout)
Couldn't agree more.
- Add some flags that can later be used to determine whether it is a
good idea to try to write things, e.g. whether RO regions overlap with included regions on write operations.
Ack.
So the layout.h would look somethings like the following:
=====layout.h===== struct romentry { chipoff_t start; chipoff_t end; bool included; bool write_possible; bool read_possible; char name[256];
I know 256B isn't that much, but maybe we should make it a pointer?
Plus `const char *source`, i.e. where the layout info was read from?
We'll need something here for the work in progress features to specify a data source. In current patches this is a string for a file name. I think it should be a pointer to a buffer instead. We might be able to get rid of `included`, it would implicitly be included if the pointer is set. But that's for another time, when the current code is cleaned up.
};
/* Linked list entries */ struct flashrom_layout { struct romentry entry; struct flashrom_layout *next; };
I'm not sure if we shouldn't include the `next` pointer into the origi- nal struct instead? Do we want to have a libflashrom-public struct? If not, we could just use a single one with the pointer included.
/* No need for this anymore
- struct single_layout {
- struct flashrom_layout base;
- struct romentry entry;
- }; */
/* Linked list for include args. append when fetching layouts entries */ struct layout_include_args { char *name; struct layout_include_args *next; };
/* Deal with include args:
- Add the included flag if everything is ok: no overlapping region, name
- present in layout list. To be called after a layout entries are
- gathered in the linked list.
*/ int process_include_args(const struct layout_include_args * const args, struct flashrom_layout *const l);
This is really just helper functionality for the cli code. I don't think it belongs here. Ideally, the cli would only use the libflashrom API. And it would be wise to design the API in a way that the cli and this helper code can work with it.
=====layout.h=====
So what do you think of this proposal?
Progress! Thanks for writing it up.
Nico
-
Arthur Heymans -
Nico Huber