Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/lib/cbfs.c 1 file changed, 11 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/1
diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 1e8a93f..d81583c 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -62,6 +62,17 @@ }
int ret = cbfs_locate(fh, &rdev, name, type); + if (ret) { + + printk(BIOS_DEBUG, "CBFS_BOOT_LOCATE: Fall back to RO region\n"); + if (fmap_locate_area_as_rdev("COREBOOT", &rdev)) { + printk(BIOS_ALERT, "COREBOOT region not found while looking for %s\n", + name); + return ret; + } + ret = cbfs_locate(fh, &rdev, name, type); + } + if (!ret) if (vboot_measure_cbfs_hook(fh, name)) return -1;
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1: Code-Review-2
It needs at least proper documentation. I'm not sure if a fallback in this place is the correct way.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-2
It needs at least proper documentation. I'm not sure if a fallback in this place is the correct way.
I agree with you that the documentation is limited at this moment. To make sure I am doing the right things. I am planning to add inline documentation in the source. Is this sufficient or is there another place where a description should be added.
I think that this is the correct location for a file-based fallback. I can imagine that a fall back is not a valid option in other scenarios so I am considering to add a config option that is normally disabled so this feature can be enabled when desired.
I don't see a security issue with the solution. The cbfs_locate first tries to find the file in the selected region. If it can not find the file there it tries to use the RO region which is trusted. There no option to try the other (non trusted) RW region.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1: Code-Review-1
(2 comments)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions. The problem statement seems to be that one wants to put files in different CBFSes. Some of those files reside in RO while others may be in RW. But the desire is to locate the files using the same API regardless of which CBFS it is in.
Why not just target the specific region? cbfs_locate_file_in_region()
If anything, this fallback path should be guarded with a Kconfig option. On the Chromebook side of things we wouldn't want to implicitly pull things from different regions. Once we've steered to a specific CBFS region from vboot we don't want to implicitly go back to another region. We would only want to target different CBFS regions explicitly.
https://review.coreboot.org/c/coreboot/+/36545/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/1/src/lib/cbfs.c@67 PS1, Line 67: printk(BIOS_DEBUG, "CBFS_BOOT_LOCATE: Fall back to RO region\n"); ret = cbfs_locate_file_in_region(fh, "COREBOOT", name, type);
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
The problem statement seems to be that one wants to put files in different CBFSes. […]
+1 for Aaron's recommendation. Even if you really want a way to fallback, that should be done by a new API to make it explicit (especially there's clear reason for why fallback region name is "COREBOOT").
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
The problem statement seems to be that one wants to put files in different CBFSes. […]
I will definitely add a config option to make sure you don't run into problems.
Using cbfs_locate_file_in_region is not an option in our case. This would be fine if we only needed this for items only relevant to our mainboard but in our case, other files are concerned as well and changes would be needed in several parts of the tree.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
Using cbfs_locate_file_in_region is not an option in our case. This would be fine if we only needed this for items only relevant to our mainboard but in our case, other files are concerned as well and changes would be needed in several parts of the tree.
OK. I think you may be creating a hard to maintain situation, but it's your product offering.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
Using cbfs_locate_file_in_region is not an option in our case. […]
That sound like you want to put multiple files that are expected to be in the RW region into the RO region. Please give the use case or scenario you are trying to achieve. As RO isn't meant to be update-able, you end with a platform that can only update insignificant parts.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
That sound like you want to put multiple files that are expected to be in the RW region into the RO […]
First of all, I am aware that the RO area is intended to be fixed so the items that are there are not intended to be updated. This solution intends to make this a bit more flexible though.
The reason to even consider this is the fact that for the board we want to make maximum space available for the payload. In this case, the payload is most sensitive to changes over time. So this is priority #1 and development shouldn't be constrained when not needed. If there is an important reason it will be possible to put additional effort into this to reduce size.
So what we will be doing is start out by putting all items you would expect into the RO partition and not duplicate them into RW.
For some of the components like the logo, the microcode, and some customer-specific binaries this will mean they will only be there and will not be updated at all. (In this case, the OS image is controlled and will contain the microcode updates).
The other components, like romstage and ramstage, can still be updated when needed by putting them in the RW region. (If it's there the RO copy won't be used).
So what this solution provides for us is: 1) Additional space in flash because of "static" items that are not duplicated to RW regions 2) Additional space for the payload during the initial part of the lifetime of the product. When needed all components can still be updated as normal at the expense of additional effort to reduce payload size to make the updated components fit in RW.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
If you don't want to lock RO (make it read-only), you can achieve the same by simply not having a RW partitions at all. All components would reside in RO, you can still use the measured boot, have enough space for payloads and don't need to duplicate stages. The good thing is that is already supported by coreboot.
I would expect a minimum amount of items in the RO partition and all of them in RW to have them updated.
While technically correct I fear that without proper documentation (maybe even with proper documentation) it will be hard for the customer to figure out a reliable update scheme, as files are scattered all over the firmware image.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
The other components, like romstage and ramstage, can still be updated when needed by putting them in the RW region. (If it's there the RO copy won't be used).
vboot has tried something in that flavor - please look for RO_NORMAL preamble, although I'm not sure if it's still working today.
Even if not using RO_NORMAL, given you're targeting stage files, I think you should change where the stage files were loaded and add a fallback when some config is set?
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
First of all, I am aware that the RO area is intended to be fixed so the items that are there are no […]
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
Patch Set 1:
If you don't want to lock RO (make it read-only), you can achieve the same by simply not having a RW partitions at all. All components would reside in RO, you can still use the measured boot, have enough space for payloads and don't need to duplicate stages. The good thing is that is already supported by coreboot.
I would expect a minimum amount of items in the RO partition and all of them in RW to have them updated.
While technically correct I fear that without proper documentation (maybe even with proper documentation) it will be hard for the customer to figure out a reliable update scheme, as files are scattered all over the firmware image.
The issue is that we do want the RO region to be locked so your suggestion is not an option. We are using a single RO with a single RW in this case.
I agree with you that using a scheme like this will require additional attention. But the good thing is that it doesn't need to be complicated at all once there is sufficient space to fit the coreboot component alongside the payload.
Once this is the case all files can be located in the RW region and there won't be a difference with the "standard" situation at all. All this does is provide the option to use components that are present in the RO partition by simply removing them from the RW.
Regarding the documentation. I can provide documentation for this but at the moment it is not clear to me what to provide. Is this just documentation inside the code or should I provide documentation in the Documents directory as well?
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
Done
I touched done by mistake above. So please ignore that.
I had a look. The RO_NORMAL support isn't present in coreboot and the definition of VB2_FIRMWARE_PREAMBLE_USE_RO_NORMAL indicates this shouldn't be used.
Can you indicate why implementing an optional fallback mechanism is a problem? Basically this allows us to provide the functionality having a single change and a single option to enable/disable. Providing a fall back for every single file would require more changes at more locations to provide the same result.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
I touched done by mistake above. So please ignore that. […]
Once this is guarded by a Kconfig and properly documented in code and Documenation/ directory I'll remove my -2 vote.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
Once this is guarded by a Kconfig and properly documented in code and Documenation/ directory I'll r […]
I will add the documentation and add the Kconfig
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#2).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@71 PS2, Line 71: * This functionality is intended for very specific situations where flash space is a line over 96 characters
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@72 PS2, Line 72: * major concern. It should be used with care. It allows us to avoid duplicate files line over 96 characters
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@73 PS2, Line 73: * in the RO and RW partitions while still updateability in case this is required in a line over 96 characters
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
I will add the documentation and add the Kconfig
The documentation and Kconfig have been added. The Kconfig depends on the existance of a RW region.
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#3).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/3
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3: -Code-Review
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/3/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/3/src/lib/cbfs.c@71 PS3, Line 71: * This functionality is intended for very specific situations where flash space is line over 96 characters
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... File Documentation/security/vboot/index.md:
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... PS3, Line 189: **RO_REGION_ONLY** Space at end of line
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... File Documentation/security/vboot/index.md:
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... PS3, Line 189: **RO_REGION_ONLY**
Space at end of line
This is intended and required by the markdown language.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... File Documentation/security/vboot/index.md:
https://review.coreboot.org/c/coreboot/+/36545/3/Documentation/security/vboo... PS3, Line 189: **RO_REGION_ONLY**
This is intended and required by the markdown language.
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@71 PS2, Line 71: * This functionality is intended for very specific situations where flash space is a
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@72 PS2, Line 72: * major concern. It should be used with care. It allows us to avoid duplicate files
line over 96 characters
Done
https://review.coreboot.org/c/coreboot/+/36545/2/src/lib/cbfs.c@73 PS2, Line 73: * in the RO and RW partitions while still updateability in case this is required in a
line over 96 characters
Done
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#4).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/4/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/4/src/lib/cbfs.c@73 PS4, Line 73: * duplicate files in the RO and RW partitions while still updateability in trailing whitespace
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/3/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/3/src/lib/cbfs.c@71 PS3, Line 71: * This functionality is intended for very specific situations where flash space is
line over 96 characters
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36545/1//COMMIT_MSG@11 PS1, Line 11: static files that are not intended to be updated to the RW regions.
The documentation and Kconfig have been added. The Kconfig depends on the existance of a RW region.
Done
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/1/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/1/src/lib/cbfs.c@67 PS1, Line 67: printk(BIOS_DEBUG, "CBFS_BOOT_LOCATE: Fall back to RO region\n");
ret = cbfs_locate_file_in_region(fh, "COREBOOT", name, type);
Done
Hello Kyösti Mälkki, Patrick Rudolph, Aaron Durbin, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#5).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/5
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 5: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36545/5/src/security/vboot/Kconfig@... PS5, Line 229: bool "Enable CBFS fallback" Does this need to be a user visible kconfig option? Or can you select it from your mainboard products directly?
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#6).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 46 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/6
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 6: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/5/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36545/5/src/security/vboot/Kconfig@... PS5, Line 229: bool "Enable CBFS fallback"
Does this need to be a user visible kconfig option? Or can you select it from your mainboard product […]
I can select it from the mainboard directly. As far as I am concerned this only for some embedded systems anyhow. I will make it "invisible" assuming this is what you need to agree with this commit.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c@68 PS6, Line 68: /* When VBOOT_ENABLE_CBFS_FALLBACK is enabled and a file is not available in the Correct long-form multi-line comment style is
/* * text * text */
(FWIW, I'm not sure you need such a huge explanation inline here, explaining the details in the Kconfig help string should be good enough.)
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c@78 PS6, Line 78: CBFS_BOOT_LOCATE: nit: use __func__ if you want to output the function name, but the other code here doesn't do that either so I'm not sure why you'd want to do it here. Maybe print the file name instead? That could actually be useful for users...
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c@68 PS6, Line 68: /* When VBOOT_ENABLE_CBFS_FALLBACK is enabled and a file is not available in the
Correct long-form multi-line comment style is […]
I agree with you on the comment style. I normally use that one as well. I looked at the next bigger block (from line 136) ad adapted to that style expecting this is the method to use. Now reverted this.
I updated the comment a bit to make it less bulky. But didn't remove it completely. I assume this is fine with you.
Also updated the debug message as you indicated. Removed this BIG function name which was handy during debug but not that much now. I didn't remove it completely because I think it is important to know from the log that this feature kicked in.
https://review.coreboot.org/c/coreboot/+/36545/6/src/lib/cbfs.c@78 PS6, Line 78: CBFS_BOOT_LOCATE:
nit: use __func__ if you want to output the function name, but the other code here doesn't do that e […]
Done
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#7).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/7
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/7/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/7/src/lib/cbfs.c@68 PS7, Line 68: /* trailing whitespace
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36545/7/src/security/vboot/Kconfig@... PS7, Line 233: When this enabled cbfs_boot_locate will look for a file in the RO 'When this option is enabled,' of 'If this is set,'
Hello Kyösti Mälkki, Aaron Durbin, Patrick Rudolph, Julius Werner, Frans Hendriks, Stefan Reinauer, Hung-Te Lin, build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Daisuke Nojiri, Philipp Deppenwiese, Joel Kitching, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36545
to look at the new patch set (#8).
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/45/36545/8
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 8: Code-Review+1
(2 comments)
https://review.coreboot.org/c/coreboot/+/36545/7/src/lib/cbfs.c File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/36545/7/src/lib/cbfs.c@68 PS7, Line 68: /*
trailing whitespace
Done
https://review.coreboot.org/c/coreboot/+/36545/7/src/security/vboot/Kconfig File src/security/vboot/Kconfig:
https://review.coreboot.org/c/coreboot/+/36545/7/src/security/vboot/Kconfig@... PS7, Line 233: When this enabled cbfs_boot_locate will look for a file in the RO
'When this option is enabled,' of 'If this is set,'
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 8: Code-Review+2
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 8: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
lib/cbfs: Add fallback to RO region to cbfs_boot_locate
With this change cbfs_boot_locate will check the RO (COREBOOT) region if a file can not be found in the active RW region. By doing so it is not required to duplicate static files that are not intended to be updated to the RW regions.
The coreboot image can still be updated by adding the file to the RW region.
This change is intended to support VBOOT on systems with a small flash device.
BUG=N/A TEST=tested on facebook fbg1701
Change-Id: I81ceaf927280cef9a3f09621c796c451e9115211 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36545 Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/security/vboot/index.md M src/lib/cbfs.c M src/security/vboot/Kconfig 3 files changed, 45 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Wim Vervoorn: Looks good to me, but someone else must approve Frans Hendriks: Looks good to me, approved
diff --git a/Documentation/security/vboot/index.md b/Documentation/security/vboot/index.md index 9742089..400c2b5 100644 --- a/Documentation/security/vboot/index.md +++ b/Documentation/security/vboot/index.md @@ -186,6 +186,26 @@ enabling vboot causes the build script to add the read/write files into coreboot file systems in *FW_MAIN_A* and *FW_MAIN_B*.
+**RO_REGION_ONLY** + +The files added to this list will only be placed in the read-only region and +not into the read/write coreboot file systems in *FW_MAIN_A* and *FW_MAIN_B*. + +**VBOOT_ENABLE_CBFS_FALLBACK** + +Normally coreboot will use the active read/write coreboot file system for all +of it's file access when VBOOT is active and is not in recovery mode. + +When the `VBOOT_ENABLE_CBFS_FALLBACK` option is enabled the cbfs file system will +first try to locate a file in the active read/write file system. If the file +doesn't exist here the file system will try to locate the file in the read-only +file system. + +This option can be used to prevent duplication of static data. Files can be +removed from the read/write partitions by adding them to the `RO_REGION_ONLY` +config. If a file needs to be changed in a later stage simply remove it from +this list. + ***
## Signing the coreboot Image diff --git a/src/lib/cbfs.c b/src/lib/cbfs.c index 9ac1bc0..13b5afb 100644 --- a/src/lib/cbfs.c +++ b/src/lib/cbfs.c @@ -62,6 +62,22 @@ }
int ret = cbfs_locate(fh, &rdev, name, type); + + if (CONFIG(VBOOT_ENABLE_CBFS_FALLBACK) && ret) { + + /* + * When VBOOT_ENABLE_CBFS_FALLBACK is enabled and a file is not available in the + * active RW region, the RO (COREBOOT) region will be used to locate the file. + * + * This functionality makes it possible to avoid duplicate files in the RO + * and RW partitions while maintaining updateability. + * + * Files can be added to the RO_REGION_ONLY config option to use this feature. + */ + printk(BIOS_DEBUG, "Fall back to RO region for %s\n", name); + ret = cbfs_locate_file_in_region(fh, "COREBOOT", name, type); + } + if (!ret) if (vboot_measure_cbfs_hook(fh, name)) return -1; diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig index e3b8aa6..87bb80a 100644 --- a/src/security/vboot/Kconfig +++ b/src/security/vboot/Kconfig @@ -220,6 +220,15 @@ Add a space delimited list of filenames that should only be in the RO section.
+ +config VBOOT_ENABLE_CBFS_FALLBACK + bool + default n + depends on VBOOT_SLOTS_RW_A + help + When this option is enabled cbfs_boot_locate will look for a file in the RO + (COREBOOT) region if it isn't available in the active RW region. + menu "GBB configuration"
config GBB_HWID
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/9/Documentation/security/vboo... File Documentation/security/vboot/index.md:
https://review.coreboot.org/c/coreboot/+/36545/9/Documentation/security/vboo... PS9, Line 189: spaces
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36545 )
Change subject: lib/cbfs: Add fallback to RO region to cbfs_boot_locate ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36545/9/Documentation/security/vboo... File Documentation/security/vboot/index.md:
https://review.coreboot.org/c/coreboot/+/36545/9/Documentation/security/vboo... PS9, Line 189:
spaces
This is on purpose and required by the Sphinx markdown language. If these are not added the item will not be on a seperate line as I intended. I know it's confusing my editor didn't like it either.
-
Aaron Durbin (Code Review) -
Arthur Heymans (Code Review)
-
build bot (Jenkins) (Code Review)
-
Frans Hendriks (Code Review)
-
Hung-Te Lin (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Wim Vervoorn (Code Review)