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