[coreboot] Re: Undepend on vboot [was: System gcc requirements]
> CONFIG_VBOOT enables vboot, the verified boot scheme. The issue here is the > submodule, which is drawn in through CONFIG_VBOOT_LIB. Why is this an issue? CONFIG_VBOOT_LIB isn't set either! Furthermore it is pretty clear to me that if CONFIG_VBOOT is not set, then no vboot related code should be generated (that also includes code ifdef guarded by CONFIG_VBOOT_LIB). The real problem here is, because of the poor code quality of vboot integration, it doesn't matter what you have in Kconfig, you cannot compile coreboot without the vboot submodule. That is the actual issue that needs to be addressed. > CBFS hashing (which has nothing to do with verified boot right now, although > that could change). If that's true, then why does cbfs_private.h include vb2_sha.h header at all? Let's just remove vboot dependency altogether, problem solved (see below). > that doesn't solve the problem that cbfstool has its own CBFS implementation > (so it also needs to be ifguarded that way, which is a bit annoying because > util/* doesn't use Kconfig at this time) Which could be simply solved by adding something like this to the Makefile (but see below): ifneq ($(wildcard ../3rdparty/vboot/*),) CFLAGS += -DCONFIG_VBOOT_LIB endif That's it! You're making this a lot more complicated than actually is. Don't overcomplicate things, look for the simplest solution! > I'd even accept a local hash implementation. Big plus one to this! An sha checksum code is pretty small (an sha265 implementation being ca. 50-60 SLoC, no more). And it *does not need* any maintenance (if it provides the correct results for the test vectors, there's no reason to modify it, ever). It is pretty obvious that it's a lot more complicated to maintain vboot integration than having a dependency-free cbfstool. I'm staring to have a feeling this isn't a technical issue but a political one, which I don't want to participate in. There are simple and easy solutions. I've recommended some, take it or leave it, but I'm out. Have a nice day bzt On 11/18/20, Patrick Georgi via coreboot wrote: > Am Mi., 18. Nov. 2020 um 22:03 Uhr schrieb Nico Huber : > >> The vboot dependency has been a PITA for a while. I'll happily accept >> patches that make it less of a pain even if that means a little more >> maintenance effort. I'd even accept a local hash implementation. > > That's an option. That isn't what was proposed though. The proposal was "I > don't need this, it annoys me, let's drop it". > > But I wonder, if that were a policy, would vboot have >> such implementations? I'm sure they weren't the first. Maybe there >> were even concerns about external code? >> > Suitable license (rules out everything GNU for GPL3+, OpenSSL + offspring > for their advertising clause or tomcrypt for not having a license), > somewhat recently maintained (rules out libtomcrypt and SPARK crypto), > suitable for embedded purposes (rules out Java implementations). Exactly > the issues coreboot would face when selecting an implementation to copy. > Just that by the time coreboot had to consider hashing data, vboot existed, > it ticked the right boxes, and some people with overlap to coreboot were > familiar with it. > > > Patrick > -- > Google Germany GmbH, ABC-Str. 19, 20354 Hamburg > Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: > Hamburg > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado > ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
Am Mi., 18. Nov. 2020 um 22:03 Uhr schrieb Nico Huber : > The vboot dependency has been a PITA for a while. I'll happily accept > patches that make it less of a pain even if that means a little more > maintenance effort. I'd even accept a local hash implementation. That's an option. That isn't what was proposed though. The proposal was "I don't need this, it annoys me, let's drop it". But I wonder, if that were a policy, would vboot have > such implementations? I'm sure they weren't the first. Maybe there > were even concerns about external code? > Suitable license (rules out everything GNU for GPL3+, OpenSSL + offspring for their advertising clause or tomcrypt for not having a license), somewhat recently maintained (rules out libtomcrypt and SPARK crypto), suitable for embedded purposes (rules out Java implementations). Exactly the issues coreboot would face when selecting an implementation to copy. Just that by the time coreboot had to consider hashing data, vboot existed, it ticked the right boxes, and some people with overlap to coreboot were familiar with it. Patrick -- Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
Am Mi., 18. Nov. 2020 um 22:15 Uhr schrieb bzt : > I believe you are both unnecessarily overcomplicate this. The way I > see it the only issue here is a few missing ifdef guards for > CONFIG_VBOOT in cbfs, that's all. Quite straightforward to solve. > CONFIG_VBOOT enables vboot, the verified boot scheme. The issue here is the submodule, which is drawn in through CONFIG_VBOOT_LIB. Besides vboot, other users of it are: the TPM drivers, Eltan's mboot, AMD PSP verstage, Intel CSE lite, and CBFS hashing (which has nothing to do with verified boot right now, although that could change). And even if "just ifdef stuff in CBFS with CONFIG_VBOOT_LIB" creates a working image, that doesn't solve the problem that cbfstool has its own CBFS implementation (so it also needs to be ifguarded that way, which is a bit annoying because util/* doesn't use Kconfig at this time), and with just "ifguarding things", there's some work left to do to handle "cbfstool coreboot.rom add -A sha256 ..." properly: should it error out generically (as if -A is unknown)? provide a useful error message? just ignore the flag? It's not quite that straightforward. Patrick -- Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
I think I should continue here, sorry that my previous response was to gcc requirements. David Hendricks wrote: > Can you be more specific? I ran thru the instructions and built for a qemu > target and did not run into any problems Here are the commands that I've used. Without submodules, coreboot won't compile. The only reason for this is the badly integrated vboot submodule, nothing else. All the other submodules are optional. $ git clone https://review.coreboot.org/coreboot $ cd coreboot $ make crossgcc-i386 CPUS=2 $ make -C payloads/coreinfo olddefconfig $ make -C payloads/coreinfo $ make menuconfig (here I've added coreinfo payload) $ make safedefconfig $ cat defconfig CONFIG_PAYLOAD_ELF=y CONFIG_PAYLOAD_FILE="payloads/coreinfo/build/coreinfo.elf" $ cat .config | grep VBOOT # CONFIG_VBOOT is not set CONFIG_VBOOT_VBNV_OFFSET=0x2c $ make (... removed for clearity ...) FMAP build/util/cbfstool/fmaptool -h build/fmap_config.h build/fmap.fmd build/fmap.fmap SUCCESS: Wrote 182 bytes to file 'build/fmap.fmap' (and generated header) The sections containing CBFSes are: COREBOOT CP bootblock/arch/x86/memlayout.ld In file included from src/arch/x86/memlayout.ld:3: src/include/memlayout.h:9:10: fatal error: vb2_constants.h: No such file or directory #include ^ compilation terminated. make: *** [Makefile:362: build/bootblock/arch/x86/memlayout.ld] Error 1 $ (here I've simply commented out the include inside memlayout.h for a quick test) $ make GENbuild.h CC bootblock/arch/x86/id.o CP bootblock/arch/x86/memlayout.ld CC bootblock/arch/x86/memmove.o CC bootblock/arch/x86/memset.o CC bootblock/arch/x86/mmap_boot.o CC bootblock/arch/x86/post.o CC bootblock/arch/x86/timestamp.o CC bootblock/commonlib/bsd/cbfs_private.o In file included from src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:8, from src/commonlib/bsd/cbfs_private.c:3: src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:7:10: fatal error: vb2_sha.h: No such file or directory #include ^~~ compilation terminated. make: *** [Makefile:362: build/bootblock/commonlib/bsd/cbfs_private.o] Error 1 $ I'd like put emphasis on this part: $ cat .config | grep VBOOT # CONFIG_VBOOT is not set and on the fact by removing the include from memlayout.h solved the issue and the compilation went a step further. This means that include clearly doesn't belong there. David Hendricks wrote: > As Julius mentioned this is possible but it's a fair bit of work with little > benefit. Patrick Georgi wrote: > I lined out how that could look like above. I believe you are both unnecessarily overcomplicate this. The way I see it the only issue here is a few missing ifdef guards for CONFIG_VBOOT in cbfs, that's all. Quite straightforward to solve. Forcing vboot on users is not the right answer, imho. Integrating vboot with proper ifdef guards is. But that's just my two cents. Have a nice day, bzt On 11/18/20, Patrick Georgi via coreboot wrote: > Am Di., 17. Nov. 2020 um 18:54 Uhr schrieb Peter Stuge : > >> Patrick Georgi via coreboot wrote: >> > coreboot doesn't, cbfstool does. >> >> If that were the case things would already be a lot better! >> >> Alas, coreboot unconditionally requires vboot in these files: >> > Oops, I forgot about those, you're right! > > So: we discussed that in today's meeting and had two take-aways: > > 1. people have issues with older host toolchains failing to build vboot. > We'll work on improving those scenarios. > > 2. We generally prefer to build our utilities fully featured to prevent > partial feature sets from popping up in installed binaries. > That said, if there were a patch that cleanly cuts out cbfs hashing support > from coreboot (and cbfstool used for building coreboot) based on a Kconfig > flag, we would consider it. > > "Cleanly" meaning: > - It needs to be optional > - The result needs to be maintainable. Things shouldn't break apart when > looking at the flag funny > - cbfstool should behave properly and reasonably when built without > hashing but the relevant options are used (that is: say that it's a > stripped down build, not just "command line error") > - cbfstool and cbfs in coreboot without those flags still need to be able > to deal with hash attributes sanely, that is, skip them safely. I don't > expect that to be an issue (the data structures are robust enough), but > something to keep in mind. > > Maybe we view Kconfig differently. I expect it to control not only the >> final build artefact but also the build process, meaning what gets built >> and what is needed for successful build. >> > I'm not entirely happy about the way we use Kconfig to enable ccache (to > pick an example) because IMHO changes to config.h should lead to a > different coreboot build (outside config.h). > I guess with this
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
Hello Patrick, I'm a bit concerned about the tone of your email. Maybe you were just reflecting the meeting literally. In that case, sorry in advance. In several spots you seem to imply that Peter would settle for less than what is expected. I have no idea where that comes from. Also, I don't remember a list of requirements when the vboot dependency was introduced. The vboot dependency has been a PITA for a while. I'll happily accept patches that make it less of a pain even if that means a little more maintenance effort. I'd even accept a local hash implementation. Some- where the argument was made that it would be bad to have additional implementations. But I wonder, if that were a policy, would vboot have such implementations? I'm sure they weren't the first. Maybe there were even concerns about external code? Nico ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
Am Di., 17. Nov. 2020 um 18:54 Uhr schrieb Peter Stuge : > Patrick Georgi via coreboot wrote: > > coreboot doesn't, cbfstool does. > > If that were the case things would already be a lot better! > > Alas, coreboot unconditionally requires vboot in these files: > Oops, I forgot about those, you're right! So: we discussed that in today's meeting and had two take-aways: 1. people have issues with older host toolchains failing to build vboot. We'll work on improving those scenarios. 2. We generally prefer to build our utilities fully featured to prevent partial feature sets from popping up in installed binaries. That said, if there were a patch that cleanly cuts out cbfs hashing support from coreboot (and cbfstool used for building coreboot) based on a Kconfig flag, we would consider it. "Cleanly" meaning: - It needs to be optional - The result needs to be maintainable. Things shouldn't break apart when looking at the flag funny - cbfstool should behave properly and reasonably when built without hashing but the relevant options are used (that is: say that it's a stripped down build, not just "command line error") - cbfstool and cbfs in coreboot without those flags still need to be able to deal with hash attributes sanely, that is, skip them safely. I don't expect that to be an issue (the data structures are robust enough), but something to keep in mind. Maybe we view Kconfig differently. I expect it to control not only the > final build artefact but also the build process, meaning what gets built > and what is needed for successful build. > I'm not entirely happy about the way we use Kconfig to enable ccache (to pick an example) because IMHO changes to config.h should lead to a different coreboot build (outside config.h). I guess with this "feature", the build would be different, so this satisfies my personal criterion. > Right, but maybe we at least agree that requiring some submodule(s) > for nothing is at a minimum unneccessary? > As I mentioned elsewhere, we could import vboot as a git subtree (even though I wouldn't recommend it). That changes next-to-nothing for users (but makes life hell for developers), but satisfies that criterion. So, why the hate on submodules? I don't want any submodules, so I go over the source and remove the > requirement. > I lined out how that could look like above. (Good) Patches accepted. Patrick -- Google Germany GmbH, ABC-Str. 19, 20354 Hamburg Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Undepend on vboot [was: System gcc requirements]
Patrick Georgi via coreboot wrote: > > It's absurd to me that coreboot would require any routines out of any > > submodule for a build which will not use those routines. > > coreboot doesn't, cbfstool does. If that were the case things would already be a lot better! Alas, coreboot unconditionally requires vboot in these files: Makefile.inc src/commonlib/bsd/cbfs_private.c src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h src/commonlib/cbfs.c src/commonlib/include/commonlib/cbfs.h src/include/memlayout.h src/lib/bootmode.c There are additional, more or less target-specific, dependencies in: src/drivers/intel/fsp2_0/memory_init.c src/soc/amd/picasso/psp_verstage/psp_verstage.c src/soc/intel/common/block/cse/cse_lite.c src/soc/qualcomm/common/qclib.c src/soc/rockchip/rk3288/crypto.c The hard requirements increase over time. Two years ago they were: Makefile.inc src/commonlib/cbfs.c src/commonlib/include/commonlib/cbfs.h src/lib/bootmode.c src/security/vboot/Makefile.inc src/southbridge/intel/common/pmbase.c In addition to that, since every coreboot build does require cbfstool (no problem per se) the indirect requirement through cbfstool also counts, as long as cbfstool can only be built to support everything. > > One purpose of Kconfig is to ensure that only what's neccessary gets built. > > But cbfstool isn't hooked up to Kconfig. Given that it's not part of the > final coreboot build, having extra stuff in cbfstool doesn't affect > coreboot in the slightest, so it's not clear to me that we should change > that. Maybe we view Kconfig differently. I expect it to control not only the final build artefact but also the build process, meaning what gets built and what is needed for successful build. > > It's wrong to pull in anything during build. I too am guilty of this > > by pushing for buildgcc, it would be good to improve that case too. > > Given your reference to buildgcc, I guess you mean "download"? In that > case: git clone --recurse-submodules and there's not a single extra > download going on at build time. Right, but maybe we at least agree that requiring some submodule(s) for nothing is at a minimum unneccessary? > It is because that's what consistently causes me extra work and > > frustration every time I want to build a minimal coreboot. > > git clone --recurse-submodules is extra work, really? I don't want any submodules, so I go over the source and remove the requirement. > > What some people always want isn't OK to require when other people do > > not want it. I think that's just lazy, and not the smart kind. :\ > > Some people do not want ramstage. Some people do not want blobs. Some > people do not want x86 support. They still carry the baggage when > downloading coreboot. Indeed I think that coreboot has a lot of unneccessary baggage, like this vboot requirement. It seems that it's rather one of a few cases of this type of problem, but unneccessary dependencies are never good. //Peter ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
