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.
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 17
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Mon, 23 Jul 2018 04:37:06 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
David Hendricks has uploaded a new patch set (#17) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Add support to get layout from fmap (e.g. coreboot rom)
Flashmap, or simply fmap, is a binary data format for describing
region offsets, sizes, and certain attributes and is widely used by
coreboot. This patch adds support for the fmap data format along with
--fmap and --fmap-file arguments.
Using --fmap will make flashrom to search the ROM content for fmap
data. Using --fmap-file will make flashrom search a supplied file
for fmap data.
An example of how to update the COREBOOT region of a ROM:
flashrom -p programmer --fmap -w coreboot.rom -i COREBOOT
flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT
The fmap functions are mostly copied from cbfstool.
Currently it is made mutually exclusive with --ifd while still
allowing --layout until we are more clever about this input.
Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
M Makefile
M cli_classic.c
M flashrom.8.tmpl
A fmap.c
A fmap.h
M libflashrom.c
M libflashrom.h
7 files changed, 545 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/23203/17
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 17
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Arthur Heymans 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 16:
(1 comment)
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 :/
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 16
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Sat, 21 Jul 2018 00:16:25 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
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 16:
(1 comment)
https://review.coreboot.org/#/c/23203/15/cli_classic.c
File cli_classic.c:
https://review.coreboot.org/#/c/23203/15/cli_classic.c@247
PS15, Line 247: cli_classic_abort_usage();
> So again (as said in patchset 10 already): […]
You are correct, it was I who missed something. I've added the additional checks in PS16. The newly introduced fmap integer will also be used for OPTION_FMAP in the next patch.
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 16
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 23:41:57 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
Hello Arthur Heymans, Paul Menzel, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/27480
to look at the new patch set (#6).
Change subject: Add ability to read fmap from ROM
......................................................................
Add ability to read fmap from ROM
This adds logic to search the ROM for the flashmap.
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
Change-Id: I83119528afeef7a864e7b774b7b7d60cb9750bb4
---
M cli_classic.c
1 file changed, 21 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/80/27480/6
--
To view, visit https://review.coreboot.org/27480
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I83119528afeef7a864e7b774b7b7d60cb9750bb4
Gerrit-Change-Number: 27480
Gerrit-PatchSet: 6
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
David Hendricks has uploaded a new patch set (#16) to the change originally created by Arthur Heymans. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom)
......................................................................
Add support to get layout from fmap (e.g. coreboot rom)
Using the --fmap-file argument one can give flashrom a layout in fmap
binary format like is found of coreboot roms (it will also look for
this on coreboot roms).
An example of how to use:
"flashrom -p programmer --fmap-file coreboot.rom -w coreboot.rom -i COREBOOT"
to only flash the COREBOOT fmap region.
The fmap functions are mostly copied from cbfstool.
Currently it is made mutually exclusive with --ifd while still
allowing --layout until we are more clever about this input.
Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Signed-off-by: Arthur Heymans <arthur(a)aheymans.xyz>
Signed-off-by: David Hendricks <dhendricks(a)fb.com>
---
M Makefile
M cli_classic.c
M flashrom.8.tmpl
A fmap.c
A fmap.h
M layout.c
M libflashrom.c
M libflashrom.h
8 files changed, 419 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/23203/16
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 16
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Werner Zeh 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 15:
(1 comment)
https://review.coreboot.org/#/c/23203/15/cli_classic.c
File cli_classic.c:
https://review.coreboot.org/#/c/23203/15/cli_classic.c@247
PS15, Line 247: case OPTION_FMAP_FILE:
So again (as said in patchset 10 already):
If you, for now, want to have --ifd and --fmap-file be exclusive you have to check in the IFD-case for fmapfile as the order on the parameters is not strictly enforced. I know that we need something smarter here one day but for now we have to care about it as the --ifd will win the race without even a warning
Or do I miss something?
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 15
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 05:13:06 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
David Hendricks has posted comments on this change. ( https://review.coreboot.org/27480 )
Change subject: Add ability to read fmap from ROM
......................................................................
Patch Set 5: Verified+1
PS5 updates the man page
--
To view, visit https://review.coreboot.org/27480
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I83119528afeef7a864e7b774b7b7d60cb9750bb4
Gerrit-Change-Number: 27480
Gerrit-PatchSet: 5
Gerrit-Owner: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Comment-Date: Fri, 20 Jul 2018 01:24:00 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: Yes
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 15:
PS15 updates the man page
--
To view, visit https://review.coreboot.org/23203
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12
Gerrit-Change-Number: 23203
Gerrit-PatchSet: 15
Gerrit-Owner: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Fri, 20 Jul 2018 01:23:44 +0000
Gerrit-HasComments: No
Gerrit-HasLabels: No