Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can stil be easily disabled.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/37972/1
diff --git a/src/Kconfig b/src/Kconfig index 762cf89..c89e49f 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -228,6 +228,7 @@
config USE_BLOBS bool "Allow use of binary-only repository" + default y help This draws in the blobs repository, which contains binary files that might be required for some chipsets or boards.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 1:
(1 comment)
I have very mixed feelings about this patch. On one hand, as you say, it better supports most of the coreboot users. The other side though is that we'd still like to encourage use of blob-free platforms. I thought Ron's idea of actually only showing blob free platforms by default was fantastic, but it caused other issues.
I guess the unfortunate reality of our current situation is that most modern platforms are going to require blobs. I think we're making progress in reversing that, but DAMN, it's slow.
https://review.coreboot.org/c/coreboot/+/37972/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37972/1//COMMIT_MSG@11 PS1, Line 11: stil still
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37972
to look at the new patch set (#2).
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can still be easily disabled.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/Kconfig 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/37972/2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 2:
(1 comment)
I have very mixed feelings about this patch. On one hand, as you say, it better supports most of the coreboot users. The other side though is that we'd still like to encourage use of blob-free platforms. I thought Ron's idea of actually only showing blob free platforms by default was fantastic, but it caused other issues.
I don't like that this is needed, but even building coreboot without microcode updates is a really bad idea. My main motivation for this user friendliness.
I guess the unfortunate reality of our current situation is that most modern platforms are going to require blobs. I think we're making progress in reversing that, but DAMN, it's slow.
yeah :/
It would probably be good to discuss this in the coreboot leadership meeting, but I won't be in that meeting on January 1st.
https://review.coreboot.org/c/coreboot/+/37972/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37972/1//COMMIT_MSG@11 PS1, Line 11: stil
still
Done
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/37972
to look at the new patch set (#3).
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can stil be easily disabled. With this abuild doesn't have to select USE_BLOBS any more, so what abuild tests becomes the coreboot default again.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/Kconfig M util/abuild/abuild 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/37972/3
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3: Code-Review+1
Patch Set 2:
(1 comment)
I have very mixed feelings about this patch. On one hand, as you say, it better supports most of the coreboot users. The other side though is that we'd still like to encourage use of blob-free platforms. I thought Ron's idea of actually only showing blob free platforms by default was fantastic, but it caused other issues.
I don't like that this is needed, but even building coreboot without microcode updates is a really bad idea. My main motivation for this user friendliness.
I guess the unfortunate reality of our current situation is that most modern platforms are going to require blobs. I think we're making progress in reversing that, but DAMN, it's slow.
yeah :/
It would probably be good to discuss this in the coreboot leadership meeting, but I won't be in that meeting on January 1st.
IMHO, I would like to have coreboot include microcode updates by default, but only those blobs by default.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3: Code-Review+2
I think for any reasonable encouragement of !USE_BLOBS, we need a more fine-grained distinction of blobs. For instance, microcode updates are optional on some platforms with some CPU steppings. So the boards don't actually "require" USE_BLOBS, but not adding the updates is just stupid. And microcode is out of coreboot's scope anyway, but covered by USE_BLOBS :-/
I would totally agree to hide all boards by default that require a blob that is run inside coreboot. But making it harder to use coreboot even on platform where the coreboot code is 100% open- source (just the CPUs microcode is not) seems wrong.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3: Code-Review+1
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3:
(2 comments)
Separating between microcode updates and other blobs sounds like a good idea.
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG@11 PS3, Line 11: stil still
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG@12 PS3, Line 12: With this abuild doesn't have to select USE_BLOBS any more, so what One blank line above to separate paragraphs.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3:
Separating between microcode updates and other blobs sounds like a good idea.
Yep, that would be good. Not sure when I'll get around to work on this patch again; somehow forgot about this one in the meantime
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3: -Code-Review
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 3:
(2 comments)
Updating this to keep the discussion going.
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG@11 PS3, Line 11: stil
still
Ack
https://review.coreboot.org/c/coreboot/+/37972/3//COMMIT_MSG@12 PS3, Line 12: With this abuild doesn't have to select USE_BLOBS any more, so what
One blank line above to separate paragraphs.
Ack
Christian Walter has uploaded a new patch set (#4) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can still be easily disabled.
With this abuild doesn't have to select USE_BLOBS any more, so what abuild tests becomes the coreboot default again.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/Kconfig M util/abuild/abuild 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/37972/4
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild File util/abuild/abuild:
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\nCONFIG_FSP_USE_REPO=y\n" How to handle USE_AMD_BLOBS here?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild File util/abuild/abuild:
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\nCONFIG_FSP_USE_REPO=y\n"
How to handle USE_AMD_BLOBS here?
Since it's not enabled by default, it should still be added here. I'd see AMD_BLOBS as an equivalent to FSP_BINARIES here
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 4: Code-Review+1
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild File util/abuild/abuild:
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\nCONFIG_FSP_USE_REPO=y\n"
Since it's not enabled by default, it should still be added here. […]
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 4:
(2 comments)
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild File util/abuild/abuild:
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: CONFIG_FSP_USE_REPO=y\n CONFIG_FSP_USE_REPO sneaked through the rebase, I guess.
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\nCONFIG_FSP_USE_REPO=y\n"
Ack
USE_AMD_BLOBS is actually USE_BLOBS with a license to ack before something is downloaded. As blob licenses go, it's not a bad one. But would still have to change if we wanted a default y there.
Christian Walter has uploaded a new patch set (#5) to the change originally created by Felix Held. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can still be easily disabled.
With this abuild doesn't have to select USE_BLOBS any more, so what abuild tests becomes the coreboot default again.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de --- M src/Kconfig M util/abuild/abuild 2 files changed, 2 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/37972/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 5: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild File util/abuild/abuild:
https://review.coreboot.org/c/coreboot/+/37972/4/util/abuild/abuild@723 PS4, Line 723: CONFIG_FSP_USE_REPO=y\n
CONFIG_FSP_USE_REPO sneaked through the rebase, I guess.
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37972 )
Change subject: src/Kconfig: enable USE_BLOBS by default ......................................................................
src/Kconfig: enable USE_BLOBS by default
To provide sane defaults for most of the user base, this patch switches on the USE_BLOBS option by default. Since it only changes the default, this behaviour can still be easily disabled.
With this abuild doesn't have to select USE_BLOBS any more, so what abuild tests becomes the coreboot default again.
Change-Id: Ia0632b9ae7a1f212a8640b3faec2695d17d238c5 Signed-off-by: Felix Held felix-coreboot@felixheld.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/37972 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/Kconfig M util/abuild/abuild 2 files changed, 2 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/Kconfig b/src/Kconfig index da21af1..cf4df18 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -226,6 +226,7 @@
config USE_BLOBS bool "Allow use of binary-only repository" + default y help This draws in the blobs repository, which contains binary files that might be required for some chipsets or boards. diff --git a/util/abuild/abuild b/util/abuild/abuild index f55dadc..3b86121 100755 --- a/util/abuild/abuild +++ b/util/abuild/abuild @@ -720,7 +720,7 @@ shift;; -B|--blobs) shift customizing="${customizing}, blobs" - configoptions="${configoptions}CONFIG_USE_BLOBS=y\nCONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" + configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" ;; -A|--any-toolchain) shift customizing="${customizing}, any-toolchain"