[coreboot] Re: libpayload CBFS API deprecation; please port payloads to new API
Hi Julius,
> All payloads that use the CBFS API in libpayload should please upgrade
to the new API.
Thanks for the notification, BOOTBOOT has been modified to use the new CBFS API.
> If there are no objections I'd suggest that we target the official
deprecation and removal of the old API for the release after next,
i.e. 4.18 around the end of this year.
No objections on our part, the new API works great! So I guess only
FILO remained.
Cheers,
bzt
On 05/04/2022, Julius Werner wrote:
> The libpayload CBFS stack ("libcbfs") has recently been rewritten from
> scratch in order to eliminate a bunch of very old warts and better
> match the new CBFS API in coreboot
> (https://review.coreboot.org/59497). For the time being, the old
> libcbfs code remains side-by-side to the new one so that the old API
> functions still work for existing payloads. However, we would like to
> remove this code eventually as it still presents a maintenance burden.
>
> All payloads that use the CBFS API in libpayload should please upgrade
> to the new API. Among the payloads hooked up to the coreboot build
> system, I think this only affects depthcharge (we already ported that
> one), BOOTBOOT and FILO. See
> https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/payloads/libpayload/include/cbfs.h
> for available interfaces (they match the coreboot API which is
> documented in more detail in
> https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/include/cbfs.h).
> If you have a use case that cannot be implemented with this interface
> please let me know.
>
> If there are no objections I'd suggest that we target the official
> deprecation and removal of the old API for the release after next,
> i.e. 4.18 around the end of this year.
>
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Documentation update
Hi everyone, Is there any chance to get this trivial commit merged? https://review.coreboot.org/c/coreboot/+/45829 I've added description of missing payloads and links to Documentation/payloads.md Low priority, but would be nice to have. Thanks, bzt ___ 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]
> 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]
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 wha
[coreboot] Re: System gcc requirements
Hi, If I may express my thoughts on this... > Yeah, I really don't understand your concern here. Having an undocumented (or silently installed, therefore unexpected) dependency is undesirable (especially for a firmware), no question about that. > You seem to imply that this may cause some kind of security or availability > problem Can you guarantee that a silently installed submodule's repo won't get hacked and replaced with malicious code for example? We have seen that happening with other repos already (github, sourceforge and other webhosts too). The fewer dependency you have, the less are the chances for a vulnerability or sechole, simple as that. But the main problem here I think is, following the tutorial https://doc.coreboot.org/tutorial/part1.html does not work, because it results in compilation errors (due to missing dependencies). This is clearly bad and must be resolved. It's not documented, but you MUST install the vboot submodule (which supposed to be OPTIONAL according to Kconfig) to get it work. Right now it is not possible to compile the vanilla coreboot source without vboot at all. I agree with Peter, this is unacceptable. About the solution: I think the best would be to use ifdef guards in the C code (both for coreboot and cbfstools), that's what the precompiler is for, after all. There's already a CONFIG_VBOOT_LIB config option, so why isn't it used? I see no reasonable excuse for that. I also think that "default n" should be added to https://github.com/coreboot/coreboot/blob/master/src/security/vboot/Kconfig However if Kconfig isn't wanted for any reason, it's pretty simple to add "ifneq ($(wildcard ../3rdparty/vboot/somefile*),)" to the Makefile. This way if the vboot submodule is cloned, then coreboot and cbfstool would be automatically compiled with support for it (without a Kconfig option). But for people who just clone the base repo, there would be no compilation errors. Everybody happy, problem solved. IMHO, bzt On 11/17/20, Julius Werner wrote: >> One is what you describe - a generic utility supporting everything >> that gets installed into say /usr/local/bin for lots of different >> invocations to do lots of different things with lots of different >> coreboot images. >> >> Two is specifically what is required to complete the configured build. >> >> It's certainly useful, and I argue highly desirable, to consider >> these two cases distinct. > > Yes, those are different cases and they could use differently > configured builds of cbfstool. Whether that's desirable is a different > question. Adding a configuration system to cbfstool would come with a > lot of maintenance overhead -- rather than just making sure the tool > as a whole keeps working, we'd have to ensure all possible > configurations keep working independently. The way cbfstool is > currently written doesn't really lend itself to easily split out all > these kinds of optional things. You're asking for a lot of work, > basically, and I fail to see the benefit on the other side. The > current monolithic design shouldn't really cause problems or drawbacks > to anyone (and quite frankly, I think it doesn't -- as mentioned > before I think the issues discussed in this thread don't really have > much to do with how big cbfstool is, and it doesn't really seem like a > good "solution" to the HOSTCC problem to just allow switches to stop > building parts that happened to cause compiler problems in this > specific instance. Sooner or later some compiler-specific code would > be added to a core function that you can't configure out). > >> Even worse is that you find it acceptable, or desirable, that a submodule >> is silently pulled in during the build. That may be typical for web >> development, but nothing that should be proliferated. > > Yeah, I really don't understand your concern here. Is there maybe some > confusion about how Git submodules work? You seem to imply that this > may cause some kind of security or availability problem where there is > none: submodule code is mirrored on and downloaded from coreboot.org, > same as the main repository. The currently active submodule repository > HEAD is stored in the main coreboot repository (as a Git SHA), so you > will always get exactly the version of the code you're supposed to > have, and updating that is a step that is manually committed to the > main coreboot repository. Submodules cannot suddenly become > unavailable or taken over by malicious actors due to actions outside > coreboot.org's control -- Git is not node.js. If you're just someone > building code from source in your local checkout, it really makes zero > practical difference
[coreboot] Re: Question about PR
Thank you! Good to know! I think the problem was I never edit the commit message. I always use "-m" because I'm more of a command line guy. I'll change my habit! Sorry again about the accidental extra PR. Cheers, bzt On 9/18/20, Peter Stuge wrote: > bzt wrote: >> After Jenkins run, it reported an empty line at the end of one file. >> I've fixed that, commit+push again. I though that's the normal flow >> for fixing. But it created a new PR: >> https://review.coreboot.org/c/coreboot/+/45483 >> >> And this time it gives me "Merge conflict" error (obviously). How am >> I supposed to fix a PR? > > By amending the original commit locally, keeping the original Change-Id > line in the commit message, and pushing again. > > "Amend" is a specific git operation which replaces one commit with > another commit with different (usually additional) changes. > > Rather than running 'git commit' to create a second commit on top of > the first, you can run 'git commit --amend' to create a new commit > that replaces the first, with the newly created commit adding more > stuff to (or removing an empty line from) the first. > > Git lets you edit the commit message for the new commit as usual, but > defaults to suggest the commit message that you created for the > original commit, and that should already include the Change-Id line. > > If you are careful to retain that line, Gerrit then knows that this > new commit is a fixed version of the previous change. > > > Kind regards > > //Peter > > PS: To fix this situation at f238414, you could run git rebase -i 8d1b6be^ > and change "commit" in the f238414 line to "fixup", then save and exit. > Git should then combine the two commits into one, as had you amended. > ___ > coreboot mailing list -- coreboot@coreboot.org > To unsubscribe send an email to coreboot-le...@coreboot.org > ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Question about PR
Thank you very much! I've tried "git push origin HEAD:refs/for/master" but didn't work. I've decided to start everything over, cloned the repo again, etc. I was able to push this time, however it is not clear why, since the two configurations are identical, look: $ diff coreboot-old/.git/config coreboot/.git/config $ Anyway, the push was successful. https://review.coreboot.org/c/coreboot/+/45482 Now I have a different problem. Sorry, I'm used to github, I'm new to gerrit. After Jenkins run, it reported an empty line at the end of one file. I've fixed that, commit+push again. I though that's the normal flow for fixing. But it created a new PR: https://review.coreboot.org/c/coreboot/+/45483 And this time it gives me "Merge conflict" error (obviously). How am I supposed to fix a PR? I guessed it's better to ask before I try anything else. I'm really sorry for these questions and the extra PR! I looked for a delete button or something but couldn't find any. Cheers, bzt On 9/17/20, Angel Pons wrote: > Hi, > > On Thu, Sep 17, 2020 at 2:50 PM Michal Zygowski > wrote: >> >> Hi, >> >> Please check out also this guide: >> https://www.coreboot.org/Git#Pushing_changes >> >> you need to tell git where to push: `HEAD:refs/for/master`. It seems the >> guide on https://doc.coreboot.org/tutorial/part2.html is missing one >> crucial step: >> >> `git config remote.origin.push HEAD:refs/for/master` > > This shouldn't be needed after running `make gitconfig`. > >> You don't need any particular rights to push. You have two options to >> authorize: >> >> 1. SSH key (add SSH key to gerrit account and configure git remote for >> SSH or simply clone with SSH like here >> https://www.coreboot.org/Git#Accessing_the_repository) >> 2. HTTP password. If you cloned the repo by HTTP(S) then you should be >> asked for password. You can generate it on your gerrit account. >> >> Even if you skip the git config commands, `git push origin >> HEAD:refs/for/master` should push your commit(s) you have added on top >> of your local master branch to gerrit. They will be public. if you >> append %private at the end of the command, it will be private. If you >> append %wip it will be marked as work in progress. >> >> Of course we can't see it if it is private. You would have to add >> reviewers or people on CC. > > You can also `unmark private` on the change. This way, everyone can > take a look. Note that private changes can't be submitted normally. > > https://gerritcodereview-test.gsrc.io/marking-a-change-as-private.html > >> Who to add as reviewer? It depends what the patch does. You may suggest >> reviewers by looking at MAINTAINERS file in the repo which contains the >> people who are more familiar with given part of coreboot source and can >> provide good reviews. >> >> How to add reviewer? If your press reply button above the commit message >> on gerrit (when displaying your patch) a window will pop up. You may >> skip writing any message. Just click in the row with reviewers (where >> Add reviewer is written) and start typing. Auto completion should give >> you some results. Type by name, nick or email of the reviewer. >> >> Best regards, >> >> -- >> Michał Żygowski >> Firmware Engineer >> https://3mdeb.com | @3mdeb_com >> >> On 17.09.2020 16:36, bzt wrote: >> > Hi, >> > >> > I'd like to commit a patch to coreboot. I've followed the tutorials on >> > https://doc.coreboot.org/tutorial/part2.html >> > >> > I've set up gerrit account, etc. created a local repo, configured git >> > for submit, set up change-id hook, etc. etc. etc. However at step 4a, >> > "git push", I got an error message from the server about missing >> > "Push" rights and to contact the administrator. How can I do that? >> > >> > I was able to push the commit as a private patch: >> > https://review.coreboot.org/c/coreboot/+/45480 >> > >> > I'm not sure if you can see this url, or is this for my user only. >> > I guess now I should add a reviewer, but how and who? Or how can I get >> > a "Push" right? > > I can't see it. You can `unmark private` on the change so that > everyone can see it. > >> > Thanks for your help, >> > bzt >> > ___ >> > coreboot mailing list -- coreboot@coreboot.org >> > To unsubscribe send an email to coreboot-le...@coreboot.org >> ___ >> coreboot mailing list -- coreboot@coreboot.org >> To unsubscribe send an email to coreboot-le...@coreboot.org > > Best regards, > Angel > ___ > coreboot mailing list -- coreboot@coreboot.org > To unsubscribe send an email to coreboot-le...@coreboot.org > ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Question about PR
Hi, I'd like to commit a patch to coreboot. I've followed the tutorials on https://doc.coreboot.org/tutorial/part2.html I've set up gerrit account, etc. created a local repo, configured git for submit, set up change-id hook, etc. etc. etc. However at step 4a, "git push", I got an error message from the server about missing "Push" rights and to contact the administrator. How can I do that? I was able to push the commit as a private patch: https://review.coreboot.org/c/coreboot/+/45480 I'm not sure if you can see this url, or is this for my user only. I guess now I should add a reviewer, but how and who? Or how can I get a "Push" right? Thanks for your help, bzt ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
