Hello Nico Huber, Patrick Georgi,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42548
to review the following change.
Change subject: Add qc_blobs repository ......................................................................
Add qc_blobs repository
This patch adds a separate blobs repository for Qualcomm blobs, analogous to the existing AMD blobs. Qualcomm's binary licenses allow files to be redistributed and used by anyone, but they explicitly require the user to agree to the license terms when just *downloading* the binary (even if they're not using them to build any firmware). Some community members do not like to have to agree to licenses for files they're not actually using, so we are keeping these files separate from the main blobs repository and adding an extra Kconfig to make sure the user is aware of and must explicitly agree to this before downloading these files.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I247746c1b633343064c9f32ef1556000475d6c4a --- M .gitmodules A 3rdparty/qc_blobs M Makefile.inc M src/Kconfig M util/abuild/abuild M util/release/build-release 6 files changed, 31 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/42548/1
diff --git a/.gitmodules b/.gitmodules index 9545bb6..bc4a6eb 100644 --- a/.gitmodules +++ b/.gitmodules @@ -46,3 +46,8 @@ path = 3rdparty/cmocka url = ../cmocka.git update = none +[submodule "3rdparty/qc_blobs"] + path = 3rdparty/qc_blobs + url = ../qc_blobs.git + update = none + ignore = dirty diff --git a/3rdparty/qc_blobs b/3rdparty/qc_blobs new file mode 160000 index 0000000..126fef6 --- /dev/null +++ b/3rdparty/qc_blobs @@ -0,0 +1 @@ +Subproject commit 126fef6b996237403039aa603945fc4caa75c8d6 diff --git a/Makefile.inc b/Makefile.inc index 86335d9..605913a 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -195,6 +195,9 @@ ifeq ($(CONFIG_USE_AMD_BLOBS),y) forgetthis:=$(if $(GIT),$(shell git submodule update --init --checkout 3rdparty/amd_blobs)) endif +ifeq ($(CONFIG_USE_QC_BLOBS),y) +forgetthis:=$(if $(GIT),$(shell git submodule update --init --checkout 3rdparty/qc_blobs)) +endif endif UPDATED_SUBMODULES:=1 COREBOOT_EXPORTS += UPDATED_SUBMODULES diff --git a/src/Kconfig b/src/Kconfig index 621a582..30ab68a 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -234,6 +234,26 @@ Note that for some products, omitting PSP, SMU images, or other items may result in a nonbooting coreboot.rom.
+config USE_QC_BLOBS + bool "Allow QC blobs repository (selecting this agrees to the license) + depends on USE_BLOBS + help + This draws in the qc_blobs repository, which contains binary files + distributed by Qualcomm that are required to build firmware for + certain Qualcomm SoCs (including QcLib, QC-SEC, qtiseclib and QUP + firmware). If you say Y here you are implicitly agreeing to the + license agreements of all files in this repository (which you can + browse at https://review.coreboot.org/cgit/qc_blobs.git/tree/ ). + + ******************************************************************* + PLEASE MAKE SURE YOU READ ALL 'LICENSE' FILES IN ALL SUBDIRECTORIES + OF THIS REPOSITORY AND AGREE TO THEIR TERMS BEFORE SELECTING THIS! + ******************************************************************* + + Not selecting this option means certain Qualcomm SoCs and related + mainboards cannot be built and will be hidden from the "Mainboards" + section. + config COVERAGE bool "Code coverage support" depends on COMPILER_GCC diff --git a/util/abuild/abuild b/util/abuild/abuild index 022567d..55d441f 100755 --- a/util/abuild/abuild +++ b/util/abuild/abuild @@ -714,7 +714,7 @@ shift;; -B|--blobs) shift customizing="${customizing}, blobs" - configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" + configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_USE_QC_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" ;; -A|--any-toolchain) shift customizing="${customizing}, any-toolchain" diff --git a/util/release/build-release b/util/release/build-release index 7ca6001..ce8e600 100755 --- a/util/release/build-release +++ b/util/release/build-release @@ -72,6 +72,7 @@ exclude_paths+="3rdparty/fsp " exclude_paths+="3rdparty/intel-microcode " exclude_paths+="3rdparty/amd_blobs " +exclude_paths+="3rdparty/qc_blobs " for i in ${exclude_paths}; do blobs_paths+="coreboot-${VERSION_NAME}/${i} " exclude_opts+="--exclude=coreboot-${VERSION_NAME}/${i} "
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42548/1/src/Kconfig@245 PS1, Line 245: license agreements of all files in this repository I understand this is not ideal and it would be better to host a single license somewhere like in the AMD case. I think effectively Qualcomm is planning to use the same license for all files here, but they can be a bit peculiar about how exactly they like license files and binaries arranged in the directory when they upload them. I'm going to see if I can get them to be okay with a single file and will then update this later, but for now let's assume it's going to be one license per distribution.
Hello build bot (Jenkins), Nico Huber, Martin Roth, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42548
to look at the new patch set (#2).
Change subject: Add qc_blobs repository ......................................................................
Add qc_blobs repository
This patch adds a separate blobs repository for Qualcomm blobs, analogous to the existing AMD blobs. Qualcomm's binary licenses allow files to be redistributed and used by anyone, but they explicitly require the user to agree to the license terms when just *downloading* the binary (even if they're not using them to build any firmware). Some community members do not like to have to agree to licenses for files they're not actually using, so we are keeping these files separate from the main blobs repository and adding an extra Kconfig to make sure the user is aware of and must explicitly agree to this before downloading these files.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I247746c1b633343064c9f32ef1556000475d6c4a --- M .gitmodules A 3rdparty/qc_blobs M Makefile.inc M src/Kconfig M util/abuild/abuild M util/release/build-release 6 files changed, 31 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/42548/2
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/2/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42548/2/src/Kconfig@246 PS2, Line 246: https://review.coreboot.org/cgit/qc_blobs.git/tree/LICENSE update: Qualcomm said they want a LICENSE file in every subdirectory to make sure there can be no ambiguity, but they agreed on using the same license everywhere and also put a copy of it at the toplevel, so we can just write it like this. The first blob patch is uploaded as CB:42707
Is it okay to just link to cgit here or do we need to mirror the license in the Documentation directory like for AMD?
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 2:
(1 comment)
*ping*
Hey Patrick, any more concerns here? If not, can I get a +2 (and on CB:42707 as well)?
https://review.coreboot.org/c/coreboot/+/42548/2/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42548/2/src/Kconfig@246 PS2, Line 246: https://review.coreboot.org/cgit/qc_blobs.git/tree/LICENSE
update: Qualcomm said they want a LICENSE file in every subdirectory to make sure there can be no am […]
Ack
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Add qc_blobs repository
This patch adds a separate blobs repository for Qualcomm blobs, analogous to the existing AMD blobs. Qualcomm's binary licenses allow files to be redistributed and used by anyone, but they explicitly require the user to agree to the license terms when just *downloading* the binary (even if they're not using them to build any firmware). Some community members do not like to have to agree to licenses for files they're not actually using, so we are keeping these files separate from the main blobs repository and adding an extra Kconfig to make sure the user is aware of and must explicitly agree to this before downloading these files.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I247746c1b633343064c9f32ef1556000475d6c4a Reviewed-on: https://review.coreboot.org/c/coreboot/+/42548 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Patrick Georgi pgeorgi@google.com --- M .gitmodules A 3rdparty/qc_blobs M Makefile.inc M src/Kconfig M util/abuild/abuild M util/release/build-release 6 files changed, 31 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Patrick Georgi: Looks good to me, approved
diff --git a/.gitmodules b/.gitmodules index 9545bb6..bc4a6eb 100644 --- a/.gitmodules +++ b/.gitmodules @@ -46,3 +46,8 @@ path = 3rdparty/cmocka url = ../cmocka.git update = none +[submodule "3rdparty/qc_blobs"] + path = 3rdparty/qc_blobs + url = ../qc_blobs.git + update = none + ignore = dirty diff --git a/3rdparty/qc_blobs b/3rdparty/qc_blobs new file mode 160000 index 0000000..126fef6 --- /dev/null +++ b/3rdparty/qc_blobs @@ -0,0 +1 @@ +Subproject commit 126fef6b996237403039aa603945fc4caa75c8d6 diff --git a/Makefile.inc b/Makefile.inc index bbb6685..89bb3e4 100644 --- a/Makefile.inc +++ b/Makefile.inc @@ -202,6 +202,9 @@ ifeq ($(CONFIG_USE_AMD_BLOBS),y) forgetthis:=$(if $(GIT),$(shell git submodule update --init --checkout 3rdparty/amd_blobs)) endif +ifeq ($(CONFIG_USE_QC_BLOBS),y) +forgetthis:=$(if $(GIT),$(shell git submodule update --init --checkout 3rdparty/qc_blobs)) +endif endif UPDATED_SUBMODULES:=1 COREBOOT_EXPORTS += UPDATED_SUBMODULES diff --git a/src/Kconfig b/src/Kconfig index f9c3e6a..f192776 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -234,6 +234,26 @@ Note that for some products, omitting PSP, SMU images, or other items may result in a nonbooting coreboot.rom.
+config USE_QC_BLOBS + bool "Allow QC blobs repository (selecting this agrees to the license!) + depends on USE_BLOBS + help + This draws in the qc_blobs repository, which contains binary files + distributed by Qualcomm that are required to build firmware for + certain Qualcomm SoCs (including QcLib, QC-SEC, qtiseclib and QUP + firmware). If you say Y here you are implicitly agreeing to the + Qualcomm license agreement which can be found at: + https://review.coreboot.org/cgit/qc_blobs.git/tree/LICENSE + + ***************************************************** + PLEASE MAKE SURE YOU READ AND AGREE TO ALL TERMS IN + ABOVE LICENSE AGREEMENT BEFORE SELECTING THIS OPTION! + ***************************************************** + + Not selecting this option means certain Qualcomm SoCs and related + mainboards cannot be built and will be hidden from the "Mainboards" + section. + config COVERAGE bool "Code coverage support" depends on COMPILER_GCC diff --git a/util/abuild/abuild b/util/abuild/abuild index 6e14765..903a569 100755 --- a/util/abuild/abuild +++ b/util/abuild/abuild @@ -714,7 +714,7 @@ shift;; -B|--blobs) shift customizing="${customizing}, blobs" - configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" + configoptions="${configoptions}CONFIG_USE_AMD_BLOBS=y\nCONFIG_USE_QC_BLOBS=y\nCONFIG_ADD_FSP_BINARIES=y\n" ;; -A|--any-toolchain) shift customizing="${customizing}, any-toolchain" diff --git a/util/release/build-release b/util/release/build-release index 7ca6001..ce8e600 100755 --- a/util/release/build-release +++ b/util/release/build-release @@ -72,6 +72,7 @@ exclude_paths+="3rdparty/fsp " exclude_paths+="3rdparty/intel-microcode " exclude_paths+="3rdparty/amd_blobs " +exclude_paths+="3rdparty/qc_blobs " for i in ${exclude_paths}; do blobs_paths+="coreboot-${VERSION_NAME}/${i} " exclude_opts+="--exclude=coreboot-${VERSION_NAME}/${i} "
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none Even with `update = none` it still gets cloned when one wants to update stale submodules with `git submodule --checkout`. Does somebody know a work around? It's pretty stressful because I'm constantly violating the most scary license around here.
If there is no better way, I'd prefer to remove it from the `.gitmodules`. One can still add it manually or with a script, AFAIK.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none
Even with `update = none` it still gets cloned when one wants to update […]
I assume you mean `git submodule update --checkout`? If I understand it correctly, the `--checkout` is only there to explicitly tell Git to ignore any `update = none` and treat them all as if they said `update = checkout`. Do you have to pass --checkout? If you don't want it to check out the repositories that are marked `update = none`, just running `git submodule update` should be the right thing.
Maybe the problem is that we have too many repositories marked `update = none` here? Some of them should really have no reason to be skipped and were probably just copy&pasted without knowing what it means. We should make sure only the controversial repositories are marked `update = none` so people don't get into the habit of running `git submodule update` with `--checkout`.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none
I assume you mean `git submodule update --checkout`? If I understand it correctly, the `--checkout` […]
Yes, `git submodule update --checkout`. It's also an alias in our gitconfig AFAIR.
I don't think what is controversial is a binary decision. We started to use `update = none` for any non-source repository. I wouldn't mind if we change that, but others might. And I would have no idea where to put `amd_blobs`. It also has a needs-to-be-read-first license, but compared to the `qc_blobs` license, it looks like day and night.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none
Yes, `git submodule update --checkout`. It's also an alias in our gitconfig […]
Hmm... well, I think it's hard to make everybody happy if there are so many different cases to cover. I don't think removing the repo entirely from .gitmodules is fair either because that does create a lot of hassle for the people who do need to work with it. Instead we should try to build something where people aren't lead to manually run update --checkout unless they really know what that does and really want all those blobs.
Can we maybe just add more .gitconfig aliases to handle your use case instead? I'm fine with changing the existing one to exclude qc_blobs if you want (personally, I don't use it... if other people do and want qc_blobs to be included, we can split it into `git sup` and `git sup-dangerous` or something like that). `git submodule status` prints all submodules, so with some cut and grep it shouldn't be hard to build commands that sync whichever subset you prefer.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none
Hmm... […]
Honestly, I don't care what the solution looks like. I just don't want to accidentally violate licenses.
`git submodule update --checkout` is a perfectly normal command and people who know it would likely miss it if they are told to use something else. I think the problem is that the questionable submodules are initialized in the first place and the easiest way to prevent that is to not have them in `.gitmodules`. If you prefer another solution, please go ahead.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42548 )
Change subject: Add qc_blobs repository ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules File .gitmodules:
https://review.coreboot.org/c/coreboot/+/42548/3/.gitmodules@52 PS3, Line 52: update = none
Honestly, I don't care what the solution looks like. I just […]
`git submodule update --checkout` is a command that tells Git to check out all submodules, including ones that are explicitly marked not to be checked out normally. If you don't want that to happen, I think "don't run that command" is the obvious solution. I don't think it's fair to ask for stuff that other people need to be removed entirely instead.
I'm not even sure why you're running that manually in the first place? The coreboot Makefile will already automatically check out the blob repositories that you allowed in menuconfig every time it is run. Does that not work for you? If you want a command that just checks out the "normal" blobs manually, like I said, we can add a Git alias for that. Or a Makefile target, if you prefer, so you can run 'make blobs' or something to check them out. None if this is hard to write, I can write it for you if you need me to, but I'm not exactly sure what you want in the first place (other than the solution that ruins other people's use cases).
-
Julius Werner (Code Review) -
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)