Change in coreboot[master]: The .config is overwritten when added to coreboot.rom via CBFS at the...

[Jeremy Jackson (Code Review)]

Jeremy Jackson has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: The .config is overwritten when added to coreboot.rom via CBFS at the end  of the build (if you choose that option)... should this be a separate file  for each of fallback and normal?  Since there can be two different versions  and/or configurations, would fallback/config and normal/config make more  sense?  Also the "revision" file might benefit from similar treatment.
......................................................................

The .config is overwritten when added to coreboot.rom via CBFS at the end
 of the build (if you choose that option)... should this be a separate file
 for each of fallback and normal?  Since there can be two different versions
 and/or configurations, would fallback/config and normal/config make more
 sense?  Also the "revision" file might benefit from similar treatment.

Signed-off-by: Jeremy Jackson 
Change-Id: I3e904e44137f731f239f51dbf224965f33838e19
---
M Makefile.inc
1 file changed, 6 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/42028/1

diff --git a/Makefile.inc b/Makefile.inc
index 86467a6..b547f11 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -1195,13 +1195,13 @@
 vgaroms/seavgabios.bin-file := $(CONFIG_PAYLOAD_VGABIOS_FILE)
 vgaroms/seavgabios.bin-type := raw

-cbfs-files-$(CONFIG_INCLUDE_CONFIG_FILE) += config
-config-file := $(DOTCONFIG):defconfig
-config-type := raw
+cbfs-files-$(CONFIG_INCLUDE_CONFIG_FILE) += $(CONFIG_CBFS_PREFIX)/config
+$(CONFIG_CBFS_PREFIX)/config-file := $(DOTCONFIG):defconfig
+$(CONFIG_CBFS_PREFIX)/config-type := raw

-cbfs-files-$(CONFIG_INCLUDE_CONFIG_FILE) += revision
-revision-file := $(obj)/build.h
-revision-type := raw
+cbfs-files-$(CONFIG_INCLUDE_CONFIG_FILE) += $(CONFIG_CBFS_PREFIX)/revision
+$(CONFIG_CBFS_PREFIX)/revision-file := $(obj)/build.h
+$(CONFIG_CBFS_PREFIX)/revision-type := raw

BOOTSPLASH_SUFFIX=$(suffix $(call strip_quotes,$(CONFIG_BOOTSPLASH_FILE)))
 cbfs-files-$(CONFIG_BOOTSPLASH_IMAGE) += bootsplash$(BOOTSPLASH_SUFFIX)

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: The .config is overwritten when added to coreboot.rom via CBFS at the end  of the build (if you choose that option)... should this be a separate file  for each of fallback and normal?  Since there can be two different versions  and/or configurations, would fallback/config and normal/config make more  sense?  Also the "revision" file might benefit from similar treatment.
......................................................................

Patch Set 1:

Nice. Welcome to coreboot and thank you for the patch.

It’d be great if you could amend the git commit message to follow the common style.

$ git commit --amend
    $ git push

… should be all you need.

[1]: https://doc.coreboot.org/tutorial/part2.html#step-4a-use-the-command-line-to...
[2]: https://chris.beams.io/posts/git-commit/

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Jeremy Jackson (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42028

to look at the new patch set (#2).

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Create separate .config files in CBFS for fallback/normal prefixes

When building using CONFIG_UPDATE_IMAGE=y, the previous copy of
.config is overwritten when adding files to coreboot.rom CBFS via
cbfstool at the end of the build.  This should be a separate file to
track the origin if each prefix (fallback and normal) separately,
since there can be two different versions and/or configurations. Also
the "revision" file benefits from the same treatment.

Signed-off-by: Jeremy Jackson 
Change-Id: I3e904e44137f731f239f51dbf224965f33838e19
---
M Makefile.inc
1 file changed, 6 insertions(+), 6 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/42028/2

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 2:

I guess `util/board_status/board_status.sh` also needs to be updated to consider the new location (use `fallback/config` should be enough). As board status is more or less tied to the revision (it could be copied, but let’s just say, we do not support that), backwards compatibility is not necessary in my opinion.

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 2: Code-Review-1

Other programs including coreboot itself rely on "config" and "revision" without a prefix.

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Jeremy Jackson (Code Review)]

Jeremy Jackson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 2:
> Patch Set 2: Code-Review-1
>
> Other programs including coreboot itself rely on "config" and "revision" without a prefix.

I have checked access to CBFS from the following APIs:

soc/intel/common/block/cpu/car/cache_as_ram_fsp.S
commonlib/cbfs.c
lib/cbfs.c cbfs.*load.*()

but did not find any uses of "config" or "revision".  I could argue that only one of fallback or normal gets these files, and that therefore the other could get a prefix, but I would rather understand any consumers of these files from CBFS to see if this proposal still makes sense.

Can anyone point me to consumers of these files?

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2: Code-Review-1
> >
> > Other programs including coreboot itself rely on "config" and "revision" without a prefix.
>
> I have checked access to CBFS from the following APIs:
>
> soc/intel/common/block/cpu/car/cache_as_ram_fsp.S
> commonlib/cbfs.c
> lib/cbfs.c cbfs.*load.*()
>
> but did not find any uses of "config" or "revision".  I could argue that only one of fallback or normal gets these files, and that therefore the other could get a prefix, but I would rather understand any consumers of these files from CBFS to see if this proposal still makes sense.
>
> Can anyone point me to consumers of these files?

I found it used in src/mainboard/facebook/fbg1701/board_mboot.h and util/board_status/board_status.sh. It's possible some external programs rely on it, so you'd better send out a warning on the mailing list.

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Jeremy Jackson (Code Review)]

Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Arthur Heymans,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/42028

to look at the new patch set (#3).

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Create separate .config files in CBFS for fallback/normal prefixes

When building using CONFIG_UPDATE_IMAGE=y, the previous copy of
.config is overwritten when adding files to coreboot.rom CBFS via
cbfstool at the end of the build.  This should be a separate file to
track the origin if each prefix (fallback and normal) separately, since
there can be two different versions and/or configurations. Also the
"revision" file benefits from the same treatment.

The original config and revision files are retained for backwards
compatibility, if not updating an existing image.  This also serves to
record these files for the bootblock.

Signed-off-by: Jeremy Jackson 
Change-Id: I3e904e44137f731f239f51dbf224965f33838e19
---
M Makefile.inc
M payloads/external/Makefile.inc
M src/mainboard/facebook/fbg1701/board_mboot.h
M util/board_status/board_status.sh
4 files changed, 21 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/28/42028/3

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/42028/3/util/board_status/board_sta... 
File util/board_status/board_status.sh:

https://review.coreboot.org/c/coreboot/+/42028/3/util/board_status/board_sta... 
PS3, Line 313: $cbfstool_cmd "$COREBOOT_IMAGE" extract -n fallback/config -f "${tmpdir}/config.txt" >/dev/null 2>&1
line over 96 characters

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3:

OT: I wonder how the board status script `util/board_status/board_status.sh` could detect, what prefix was used for booting. Can that be determined from the shell?

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Jeremy Jackson (Code Review)]

Jeremy Jackson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3:
> Patch Set 3:
>
> OT: I wonder how the board status script `util/board_status/board_status.sh` could detect, what prefix was used for booting. Can that be determined from the shell?

That is on my mind as well.  After reading about FMAP regions and vboot implementation, I think the full scope should be, how to determine what region (vboot case currently), prefix, and bootblock (Intel top-swap case) was booted from.  This patch is meant to address the related question of "what was the build configuration of" what was booted.

Knowing what was booted from helps board_status.sh, and enables testing using alternate bootblock and normal prefix, leaving default top bootblock and fallback prefix for recovery.

I think other areas may benefit from knowing what was booted from, starting with the bootblock itsself.  I see the top-swap feature as a valueable tool.  Using knowledge of what bootblock is in use (CMOS top-swap bit BUCTS), may replace or be used in combination with the CMOS bit for normal/fallback selection.

Additionally, knowing what *wasn't* booted and the configuration(s) may enable things like fwupd or other means of automated updates.

I don't have a patch ready, so what's the best way to move this discussion to a new thread?  ML or make up a trivial patch to start a gerrit thread?

Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42028 )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Patch Set 3: Code-Review+1

[S] Change in coreboot[master]: Create separate .config files in CBFS for fallback/normal prefixes

[Martin L Roth (Code Review)]

Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/42028?usp=email )

Change subject: Create separate .config files in CBFS for fallback/normal prefixes
......................................................................

Abandoned

This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.