David Hendricks has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom) ......................................................................
Patch Set 17:
(7 comments)
Patch Set 16:
(6 comments)
It seems a little weird to have the bulk of the implementation of fmap-from-rom here while only adding the CLI option in the next patch. Maybe it's a good idea to squash both?
Agreed.
https://review.coreboot.org/#/c/23203/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/23203/16//COMMIT_MSG@7 PS16, Line 7: Add support to get layout from fmap (e.g. coreboot rom)
Needs to be updated to mention fmap from rom functionality implementation.
Done
https://review.coreboot.org/#/c/23203/16/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/16/fmap.h@1 PS16, Line 1: /*
Probably needs license from cbfstool here
Done
https://review.coreboot.org/#/c/23203/16/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/16/fmap.c@1 PS16, Line 1: /* Copyright 2015, Google Inc.
probably needs licence from cbfstool.
Done
https://review.coreboot.org/#/c/23203/16/layout.c File layout.c:
https://review.coreboot.org/#/c/23203/16/layout.c@22 PS16, Line 22: #include "flash.h"
Added headers not needed anymore?
Done
https://review.coreboot.org/#/c/23203/16/layout.c@104 PS16, Line 104: /* returns the index of the entry (or a negative value if it is not found) */
accidental newline?
Done
https://review.coreboot.org/#/c/23203/16/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/16/libflashrom.c@456 PS16, Line 456: fmap_bsearch
This is no good for concatenated chips which can occur on Intel hardware :/
Why is that? I don't usually work with concatenated chips...
https://review.coreboot.org/#/c/23203/16/libflashrom.c@462 PS16, Line 462: offset
On Intel targets you probably only want and in some reasons even only can, if for instance ME region […]
OK. I've also updated the public interface and CLI code to pass in offset and length. I broke apart the interface since it seemed clumsy to have filenames and offset/length arguments in the function signature.
To address the problem you mention, I think we need to do a few things: 1. Add region attributes to struct romentry which we can use for things like access permissions. 2. Merge your linked list patch so we can incorporate layout info from multiple sources. 3. Always read IFD when it's known to exist (e.g. all modern Intel-based systems) and set region attributes as needed. 4. Add an error policy handling (something like https://gerrit.chromium.org/gerrit/12117 and https://gerrit.chromium.org/gerrit/46207). For example, in the ME region case we know that access to the ME region is blocked, so if the user is trying to read/write content then we know to err out but if we're just looking for an fmap then we can ignore it and keep searching.
I'd still like to see this patch get merged soon as it addresses some immediate issues, but we should try to make layouts easier and more useful by doing these other things too.
-
David Hendricks (Code Review)