Change in ...coreboot[master]: Kconfig: Display a warning if the board is updated incorrectly
Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33641
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Kconfig: Display a warning if the board is updated incorrectly
Even though we have a comment to run make distclean before switching mainboards, people still ignore this and end up with confusing results.
To try to fix this, save the mainboard directory in a Kconfig symbol that will not get updated when the mainboard gets changed. This allows us to compare it to the actual mainboard directory which will be updated when the platform changes and put up an obnoxious warning in the mainboard directory. Add another warning in the main menu, because the initial warning is probably going to be ignored. The main menu points to some documentation on how to fix the problem.
Note that this will only catch issues going forward, and won't catch any already saved config that has a problem.
Signed-off-by: Martin Roth martinroth@chromium.org Change-Id: I2a1ccb62c7678f264015c87b9004be6a106f804a --- A Documentation/getting_started/fix_dot_config.md M src/Kconfig M src/mainboard/Kconfig 3 files changed, 78 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/33641/1
diff --git a/Documentation/getting_started/fix_dot_config.md b/Documentation/getting_started/fix_dot_config.md new file mode 100644 index 0000000..17d89d0 --- /dev/null +++ b/Documentation/getting_started/fix_dot_config.md @@ -0,0 +1,50 @@ +# How to repair your .config file + +## Overview +Many people rely on a saved .config file for building their +platform. This is reasonable, but can create unanticipated +problems. + +## TLDR +* Backup your .config: "cp .config .config_bak" +* Create the defconfig file: "make savedefconfig" +* Edit defconfig & delete unknown config lines: "vi defconfig" +* To just get rid of the warning about a bad .config, remove +* the CONFIG_MAINBOARD_CHECK line. +* The SMBIOS lines are probably incorrect as well. The ROM size +* is very suspect. Check the correct default for your platform. +* Save the defconfig +* Generate a new full config: +* "make -B defconfig KBUILD_DEFCONFIG=defconfig" + +## defconfig or "Mini config" +Instead of saving a "full" config, people should use a defconfig. +This is just the changes from a default coreboot configuration +file. It's significantly smaller, and when a new configuration option +gets added, it just takes the default value for that option when +expanded into a full .config. + +### Generating the defconfig file +To create the defconfig file, run "make savedefconfig". By default +this will generate the file in the root coreboot directory and call +it "defconfig". If there was already a file named "defconfig", this +will overwrite that file. + +### Contents of the defconfig file +As the defconfig file contains just the differences between the +platform and the default choices for the platform. For the current +default platform, the defconfig can actually be an empty file. + +Typically the defconfig will contain at least 2 lines - a +CONFIG_VENDOR_ line and a CONFIG_BOARD_ line. Again though, if you +are building the default board for any particular vendor, that line +may not be present. + +All additional lines in a defconfig file are options that the user +has specifically chosen to update in their config. + +### Generating a .config from a defconfig +To expand the defconfig so that can be used, run: + DFILE=defconfig; make -B defconfig KBUILD_DEFCONFIG=$DFILE + +You can point DFILE to any saved defconfig. diff --git a/src/Kconfig b/src/Kconfig index 72d826f..8eae329 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -16,6 +16,15 @@
mainmenu "coreboot configuration"
+if MAINBOARD_CHECK != MAINBOARD_DIR +comment "***********************************************************************" +comment "WARNING: Your config is using defaults from a different mainboard " +comment "than you currently have set. We'd recommend that you delete your " +comment ".config file and start over, but if you want to try to repair the " +comment "file, see Documentation/getting_started/fix_dot_config.md " +comment "***********************************************************************" +endif + menu "General setup"
config COREBOOT_BUILD @@ -1062,6 +1071,14 @@ mainboard code supports this. On supported Intel platforms this works by changing the settings in the descriptor.bin file.
+config MAINBOARD_CHECK + string "Mainboard dir - Do not change manually" + default MAINBOARD_DIR + help + Check the platform. If this symbol doesn't match the currently + selected mainboard directory, give the user warnings that they're + probably doing something wrong. + endmenu
diff --git a/src/mainboard/Kconfig b/src/mainboard/Kconfig index c88d317..d9216a9 100644 --- a/src/mainboard/Kconfig +++ b/src/mainboard/Kconfig @@ -1,5 +1,16 @@ comment "Important: Run 'make distclean' before switching boards"
+if MAINBOARD_CHECK != MAINBOARD_DIR +comment "***********************************************************************" +comment "WARNING: Your config is using defaults from a different mainboard " +comment "than you currently have set. We'd recommend that you delete your " +comment ".config file and start over. " +comment "* *" +comment "If you ignore this warning and build anyway, you will almost certainly " +comment "get a bad rom with undefined results. " +comment "***********************************************************************" +endif + choice prompt "Mainboard vendor" default VENDOR_EMULATION
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
I like the idea, though I have just done a quick read.
https://review.coreboot.org/#/c/33641/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33641/1//COMMIT_MSG@7 PS1, Line 7: warning Why a warning?
If there are not many false positives, I would suggest making this an error. Nobody should try to get past getting a build/boot error.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1:
We can also check this in the makefile and kill the build if that's desired.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1: Code-Review+1
We can also check this in the makefile and kill the build if that's desired.
I would prefer a Makefile solution, because we wouldn't need the fake Kconfig option. I just tried the following: moved the `include $(DOTCONFIG)` up out of the `ifeq ($(NOCOMPILE)...` then we could grep for the old setting in the new `.config` after each `make *config`.
I'm not 100% convinced by either solution. I don't like the fake Kconfig, but the Makefile code might get a little ugly.
In the Makefile case, we could even move the new, to be re- paired .config away and revert the changes?
Also noticed, the MAINBOARD_DIR might not be enough, a variant change could also create havoc.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33641/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33641/1/src/Kconfig@1075 PS1, Line 1075: string "Mainboard dir - Do not change manually" do we need a text description for this? If you remove it, the variable should be invisible
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33641/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/33641/1/src/Kconfig@1075 PS1, Line 1075: string "Mainboard dir - Do not change manually"
do we need a text description for this? If you remove it, the variable should be invisible
If it doesn't have a prompt, it will get updated automatically to the default value, and the whole process won't work.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Patch Set 1: Code-Review+2
Martin Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33641 )
Change subject: Kconfig: Display a warning if the board is updated incorrectly ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Martin Roth (Code Review)
-
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)