The kernel-fitimage.bbclass is wrongly architected. It introduces a new optional entry 'fitImage' for the KERNEL_IMAGETYPES list. This list is also parsed by kernel.bbclass, which will also happily deploy fitImage-* files (that are not even FIT images but just contain the sub-image, i.e. the vmlinuz kernel itself.) Worse, when both kernel and kernel-fitimage classes are present, the final fitImage softlink did not end up being a FIT image, but it pointed to the vmlinuz binary created by kernel.bbclass. I am not the only one seeing this, I have seen users enter this or other problems: https://lists.yoctoproject.org/pipermail/yocto/2017-October/038437.html The work-around in that e-mail is telling much: we have introduced a new KERNEL_IMAGETYPES option that should not be handled by kernel.bbclass. There is no knowledge in one class whether to other class is also inherited from, nor should we want to have that. Back to basics: A flattened image tree image consists of one or more sub-images that can be of any type, and although often a kernel is included, this does not have to be the case in general. For example, it could pack three FPGA bitstreams for a Soc/FPGA and three matching DTB overlay files. So instead of the kernel-fitimage.bbclass we rather build a fit-image.bbclass from start that sits outside and on a higher level, that can pack any combination of other image types, including the typical case of a DTB, kernel and initramfs, but not make the class specific to this. The current kernel-fitimage class, once it sees it's own type, replaces it internally with a hard-coded default Linux image. There is not control over whether the uncompressed or compressed kernel must be packed. I have good reasons to need the uncompressed kernel. This bug report is here to gather insights on what fit-image.bbclass should do.
It would be great someone could work on this.
It would be great someone could work on this..
The architecture needs to be reconsidered. The proposal for an independent fit-image.bbclass sounds promising. In addition to the architectural problems already mentioned, the class has also become extremely complicated and unmaintainable because it supports many different variants, e.g. how to pack the kernel and the initramfs into the fitImage or how fitImages can be signed. Some concrete issues: * sstate does not work well if a fitImage contains an initramfs. The kernel gets re-built from scratch if the build runs from an empty TMPDIR: https://lists.openembedded.org/g/openembedded-core/message/203510 * Circular task dependencies when signing is enabled and UBOOT_ENV is used: https://lists.yoctoproject.org/g/yocto/message/64428 * Adding custom artifacts is not supported: https://lists.openembedded.org/g/openembedded-core/topic/109640118#msg208572 * A fitImage kernel is not available as a package, but all other kernel image types are. * The its file is generated by shell code. Switching to Python would simplify maintenance simply because of the indentation of spaces and tabs. Proposal for fixing and simplifying the implementation: * Start with a new fit-image.bbclass or move this one from meta-openebedded into oe-core: https://github.com/openembedded/meta-openembedded/blob/master/meta-oe/classes/fitimage.bbclass * Add a new linux-yocto-fitimage.bb recipe which uses the fit-image.bbclass to create the linux-yocto-fitimage package and deploys it also to DEPLOY_DIR. A clear separation of kernel build tasks (kernel.bbclass) and fitImage creation tasks (fit-image.bbclass + linux-yocto-fitimage.bb) is important. The linux-yocto-fitimage.bb must have standard tasks like do_package and do_deploy with standard sstate-cache usage. However, signing u-boot and kernel might cause some tricky dependencies which cannot be simplified. But understanding all these details needs a PoC implementation, I guess. * Add oe-selftests for the new linux-yocto-fitimage.bb which covers the use cases which are currently covered by https://git.yoctoproject.org/poky/tree/meta/lib/oeqa/selftest/cases/fitimage.py * Deprecate and finally remove the kernel-fitimage.bbclass which will probably also remove support for a few use cases with a bad cost-benefit ratio. For example combinations like KERNEL_IMAGETYPE(S) = "fitImage" and INITRAMFS_IMAGE_BUNDLE = '1' are hard to maintain because the fitImage generating code has to run from the kernel build folder which leads to issues with the sstate-cache and a lot of other corner cases.
I have run into the issue of a circular dependency when setting UBOOT_ENV and using a signed fitImage, described in the link by Adrian: https://lists.yoctoproject.org/g/yocto/message/64428 I have given it some thought and from my understanding (so far) the dependency on the linux kernel (from u-boot) is due to the fact that u-boot grabs the fitImage, re-signs it and inserts the public key in the u-boot device tree blob, the last two steps using the same command (mkimage). Following this it tries to verify the fitImage using the modified device tree blob to make sure it works. As far as I can tell the re-signing and verification are not really _required_ (but good for QA), so it should be possible to break that dependency loop by simply inserting the public key that we want to use in the u-boot device tree blob directly (using for example fdt_add_pubkey from uboot-tools), without re-signing/verifying the fitImage.
Seams to be related: https://bugzilla.yoctoproject.org/show_bug.cgi?id=15649
It does technically seem to work when I remove the u-boot->linux-kernel dependency and add the public key to the u-boot dtb using fdt_add_pubkey in uboot-sign.bbclass. One idea could be to do it like this, and then do the verification of the entire fitImage in kernel-fitimage.bbclass instead.
Here is my WIP branch: https://git.yoctoproject.org/poky-contrib/log/?h=adrianf/fitimage-improvements. The idea in a nutshell is as follows: 1. refactoring the its generator from the kernel-fitimage.bbclass in Python (This should make it more maintainable and extensible) 2. split out the its generator from kernel-fitimage.bbclass into a separate fit-image.bbclass (which can be used by the existing kernel-fitimage.bbclass). 3. write a new, simpler recipe that is independent of the kernel-fitimage.bbclass, similar to https://github.com/openembedded/meta-openembedded/blob/master/meta-oe/classes/fitimage.bbclass, that uses the its generator from the fit-image.bbclass as well as a recipe that generates the fitimage (as a package and as a deployment). 4. extend the new implementation with functions (and test cases) that are still needed, but do not add functions that cause problems like circular dependencies between u-boot and kernel or other issues. 5. if the new implementation turns out to be good enough, the kernel-fitimage.bbclass file can be deleted.
Related to the issues with uboot_signing: https://lists.openembedded.org/g/openembedded-core/topic/110557812#msg209708
This commit https://git.yoctoproject.org/poky/commit/?id=3fb215a3af242e2016a146739a69be746ab8b722 fixes the circular task dependency when enabling both UBOOT_SIGN_ENABLE and UBOOT_ENV and also use the kernel-fitimage.bbclass. This is a nice step towards solving this bug.
commit https://git.yoctoproject.org/poky/commit/?id=3fb215a3af242e2016a146739a69be746ab8b722 introduced a regression (also on the LTS branch). The discussion is here: https://lists.openembedded.org/g/openembedded-core/topic/111218371. Summary: The old and also documented behavior is: - FIT_SIGN_INDIVIDUAL="0": only the configurations are signed and the DTB contains one key. - FIT_SIGN_INDIVIDUAL=“1”: Both the configurations and the images are signed. There are two keys in the DTB. This means that both the configurations containing the signatures of the referenced images and the images themselves are signed, which is redundant. It is not clear why such a configuration should be useful, but it is also not clear why such a configuration should not be possible. There is also no mention of such a combination in the associated u-boot documentation: https://docs.u-boot.org/en/latest/usage/fit/signature.html. A fix is proposed here: https://lists.openembedded.org/g/openembedded-core/message/211761. This is another example that shows that we need better test coverage before we can change the implementation. If breaking changes are to be introduced, this should be done intentionally and with the necessary changes to the documentation. And such changes should not be ported back to the stable branches.
Thanks for working on this. Out of curiosity, was meta-oe implementation ever proposed to oe-core? I ended up here, because I think I might have another corner case (or something is wrong in my setup). I have a fit image with initramfs. The current documentation says if you wanna include initramfs into the kernel, then you have to set INITRAMFS_IMAGE_BUNDLE = "1" in your local.conf, meanwhile the kernel-fitimage.bbclass expects exactly the opposite (there is a test for "${INITRAMFS_IMAGE_BUNDLE}" != "1").
> Out of curiosity, was meta-oe implementation ever proposed to oe-core? As is common with complex and widely used open source projects such as OE/Yocto, it is relatively easy to quickly create a fork and hack a second implementation without automated testing and without worrying about backward compatibility. Unfortunately, this is usually counter-productive. Instead of reducing complexity and maintenance effort, such implementations lead to maintaining two half implementations with many problems instead of one that aims for perfection in the long run. What we need is a fix for OE core that handles the documented variables as backwards compatible as possible, not something completely new with no testing and no backward compatibility. So I would say hopefully not. > Regarding INITRAMFS_IMAGE_BUNDLE I'm pretty sure, that the INITRAMFS_IMAGE_BUNDLE variable behaves as documented. here is a test case with such a configuration: https://git.yoctoproject.org/poky/tree/meta/lib/oeqa/selftest/cases/fitimage.py#n842. But why do you use INITRAMFS_IMAGE_BUNDLE=1 at all? I would recommend not using it. To create a fitImage with the initramfs bundled into the kernel, you need the kernel build folder and the kernel makefile to create this bundle. In principle, this does not work properly with the sstate-cache. And this problem will probably not be solved in the future. I also see no advantage in this configuration if you have the fitImage as a container for several independent artifacts anyway.
> But why do you use INITRAMFS_IMAGE_BUNDLE=1 at all? I would recommend not using it. That's what I tried, but for some reason generated fit image did not boot. > [ 1.497576] /dev/root: Can't open blockdev > [ 1.500670] VFS: Cannot open root device "" or unknown-block(0,0): error -6 Again, maybe something wrong in my configuration / bootloader.
> Again, maybe something wrong in my configuration / bootloader. As I suspected, that was exactly the case (HW vendor did some hard-coded stuff in u-boot). > To create a fitImage with the initramfs bundled into the kernel, you need the kernel build folder and the kernel makefile to create this bundle. Yes, that works. I guess what causes a confusion though is that if you want to include initramfs with the fit image, then INITRAMFS_IMAGE_BUNDLE shouldn't be used. But with the legacy image format there's no other option, but to have it set. Perhaps we should improve documentation with that aspect in mind?
There is now a v1 patch series available: Cover letter: https://lists.openembedded.org/g/openembedded-core/message/215965 Patches: https://patchwork.yoctoproject.org/project/oe-core/list/?series=34205
(In reply to Vyacheslav Yurkov from comment #14) > > Again, maybe something wrong in my configuration / bootloader. > > As I suspected, that was exactly the case (HW vendor did some hard-coded > stuff in u-boot). > > > To create a fitImage with the initramfs bundled into the kernel, you need the kernel build folder and the kernel makefile to create this bundle. > > Yes, that works. I guess what causes a confusion though is that if you want > to include initramfs with the fit image, then INITRAMFS_IMAGE_BUNDLE > shouldn't be used. But with the legacy image format there's no other option, > but to have it set. Perhaps we should improve documentation with that aspect > in mind? hello, we are in a similar situation where we depend on INITRAMFS_IMAGE_BUNDLE today. i attempted making a FIT without it which only ends up booting the kernel without loading the initramfs. i am wondering, if you could share what you found about your version of u-boot? we have a not-insignificant number of devices in the field where it might be difficult to update u-boot to make this work, even if i found out what was wrong with it. we could keep a copy of the current classes that enable INITRAMFS_IMAGE_BUNDLE, but is there some other alternative to deleting it altogether upstream?
(In reply to Nick Owens from comment #16) > (In reply to Vyacheslav Yurkov from comment #14) > > > Again, maybe something wrong in my configuration / bootloader. > > > > As I suspected, that was exactly the case (HW vendor did some hard-coded > > stuff in u-boot). > > > > > To create a fitImage with the initramfs bundled into the kernel, you need the kernel build folder and the kernel makefile to create this bundle. > > > > Yes, that works. I guess what causes a confusion though is that if you want > > to include initramfs with the fit image, then INITRAMFS_IMAGE_BUNDLE > > shouldn't be used. But with the legacy image format there's no other option, > > but to have it set. Perhaps we should improve documentation with that aspect > > in mind? > > hello, > > we are in a similar situation where we depend on INITRAMFS_IMAGE_BUNDLE > today. i attempted making a FIT without it which only ends up booting the > kernel without loading the initramfs. i am wondering, if you could share > what you found about your version of u-boot? > > we have a not-insignificant number of devices in the field where it might be > difficult to update u-boot to make this work, even if i found out what was > wrong with it. we could keep a copy of the current classes that enable > INITRAMFS_IMAGE_BUNDLE, but is there some other alternative to deleting it > altogether upstream? Are you using a fitImage or a uImage? My intention with https://patchwork.yoctoproject.org/project/oe-core/list/?series=34205 is to remove the combination of building a kernel with INITRAMFS_IMAGE_BUNDLE and then adding the bundle to the fitImage. Using INITRAMFS_IMAGE_BUNDLE and adding the bundle to a uImage will still work. I doubt that anyone intentionally requires the INITRAMFS_IMAGE_BUNDLE + fitImage combination and cannot simply switch to a fitImage without INITRAMFS_IMAGE_BUNDLE. However, a broken U-Boot could be a reason for reconsidering this. But to be clear: We're talking about significant effort for the fitImage cleanup and additional effort to bring this "feature" back again. We're also discussing a use case that would only be needed to support a broken U-Boot. Furthermore, we're dealing with a use case that is inherently incompatible with using the sstate for a kernel + initramfs by design. From my point of view, there are many strong arguments for removing this and simplifying the fitImage implementation and usage. Are you sure you need this? Or would it be possible for you to test the patches or my branch? I could also offer to provide a backport to scartsgap if that would help. Thank you for your contribution to the discussion. It is much appreciated.
(In reply to Adrian Freihofer from comment #17) > (In reply to Nick Owens from comment #16) > > (In reply to Vyacheslav Yurkov from comment #14) > > > > Again, maybe something wrong in my configuration / bootloader. > > > > > > As I suspected, that was exactly the case (HW vendor did some hard-coded > > > stuff in u-boot). > > > > > > > To create a fitImage with the initramfs bundled into the kernel, you need the kernel build folder and the kernel makefile to create this bundle. > > > > > > Yes, that works. I guess what causes a confusion though is that if you want > > > to include initramfs with the fit image, then INITRAMFS_IMAGE_BUNDLE > > > shouldn't be used. But with the legacy image format there's no other option, > > > but to have it set. Perhaps we should improve documentation with that aspect > > > in mind? > > > > hello, > > > > we are in a similar situation where we depend on INITRAMFS_IMAGE_BUNDLE > > today. i attempted making a FIT without it which only ends up booting the > > kernel without loading the initramfs. i am wondering, if you could share > > what you found about your version of u-boot? > > > > we have a not-insignificant number of devices in the field where it might be > > difficult to update u-boot to make this work, even if i found out what was > > wrong with it. we could keep a copy of the current classes that enable > > INITRAMFS_IMAGE_BUNDLE, but is there some other alternative to deleting it > > altogether upstream? > > Are you using a fitImage or a uImage? yes, we use: INITRAMFS_IMAGE_BUNDLE = "1" INITRAMFS_FSTYPES = "cpio" KERNEL_IMAGETYPE = "fitImage" changing this to remove INITRAMFS_IMAGE_BUNDLE and INITRAMFS_FSTYPES = "cpio.gz" gives me a FIT with a separate (compressed) ramdisk image from the kernel as i think your intention is here, but unfortunately that separate image never loads - the kernel somehow ends up mounting and running init directly from the rootfs, which is incorrect for us since the initramfs should be setting up dm-verity :-/ > > My intention with > https://patchwork.yoctoproject.org/project/oe-core/list/?series=34205 is to > remove the combination of building a kernel with INITRAMFS_IMAGE_BUNDLE and > then adding the bundle to the fitImage. Using INITRAMFS_IMAGE_BUNDLE and > adding the bundle to a uImage will still work. > > I doubt that anyone intentionally requires the INITRAMFS_IMAGE_BUNDLE + > fitImage combination and cannot simply switch to a fitImage without > INITRAMFS_IMAGE_BUNDLE. However, a broken U-Boot could be a reason for > reconsidering this. But to be clear: We're talking about significant effort > for the fitImage cleanup and additional effort to bring this "feature" back > again. We're also discussing a use case that would only be needed to support > a broken U-Boot. Furthermore, we're dealing with a use case that is > inherently incompatible with using the sstate for a kernel + initramfs by > design. From my point of view, there are many strong arguments for removing > this and simplifying the fitImage implementation and usage. > > Are you sure you need this? Or would it be possible for you to test the > patches or my branch? I could also offer to provide a backport to scartsgap > if that would help. at this point, i'm not sure it was intentional, but more likely inherited from how things work from our vendor (qualcomm) and it's been this way for a long time. many different platform-specific u-boots have been shipped, increasing the pain to update it. i only tested one variant of hardware here, maybe others would work, but any broken ones would mean we have to retain the old behavior. we're still in the process of moving to scarthgap (where i tested this), so this would not end up affecting us immediately. i am not in a position to fix or update u-boot myself though, so it'd take convincing others to actually fix it. i think it's okay if upstream removes it, but we'd likely need to take the current classes into our tree and live with their other broken properties. :-( i will try to poke it some more, but since u-boot or the kernel don't even mention any attempt or failure at using the separate initramfs image, i'm left guessing right now. > > Thank you for your contribution to the discussion. It is much appreciated.
> yes, we use: > > INITRAMFS_IMAGE_BUNDLE = "1" > INITRAMFS_FSTYPES = "cpio" > KERNEL_IMAGETYPE = "fitImage" > > changing this to remove INITRAMFS_IMAGE_BUNDLE and INITRAMFS_FSTYPES = > "cpio.gz" gives me a FIT with a separate (compressed) ramdisk image from the > kernel as i think your intention is here, but unfortunately that separate > image never loads - the kernel somehow ends up mounting and running init > directly from the rootfs, which is incorrect for us since the initramfs > should be setting up dm-verity :-/ > The existing implementation is confusing. With INITRAMFS_IMAGE_BUNDLE = "0" and INITRAMFS_FSTYPES = "cpio.gz" two fitImages are built and deployed. One is a you say. The other one is bigger and should work as you need. Can you find a second fitImage in the deploy folder if you build it with INITRAMFS_IMAGE_BUNDLE = "0" and INITRAMFS_FSTYPES = "cpio.gz"? Does it work?
(In reply to Adrian Freihofer from comment #19) > > yes, we use: > > > > INITRAMFS_IMAGE_BUNDLE = "1" > > INITRAMFS_FSTYPES = "cpio" > > KERNEL_IMAGETYPE = "fitImage" > > > > changing this to remove INITRAMFS_IMAGE_BUNDLE and INITRAMFS_FSTYPES = > > "cpio.gz" gives me a FIT with a separate (compressed) ramdisk image from the > > kernel as i think your intention is here, but unfortunately that separate > > image never loads - the kernel somehow ends up mounting and running init > > directly from the rootfs, which is incorrect for us since the initramfs > > should be setting up dm-verity :-/ > > > The existing implementation is confusing. With INITRAMFS_IMAGE_BUNDLE = "0" > and INITRAMFS_FSTYPES = "cpio.gz" two fitImages are built and deployed. One > is a you say. The other one is bigger and should work as you need. Can you > find a second fitImage in the deploy folder if you build it with > INITRAMFS_IMAGE_BUNDLE = "0" and INITRAMFS_FSTYPES = "cpio.gz"? Does it work? well, good news and bad news. thank you for pointing this out, because you were right about the two different images. we have a class that puts the fit into our OTA artifact - it was indeed using the wrong symlink. i fixed that, but now u-boot hangs at Starting kernel ... the same thing happened before when there was some confusion about the compression of the kernel in the fit, but i fixed that too and now i don't know. i didn't really mean to turn this into a support case, but only to share that we do use this and changing it will impact us. thank you :-)
Created attachment 5117 [details] DTB overlays fix for FIT
Hi Adrian. Did you test your changes with external DTB overlays on top of the kernel DTB? I have made a hack for the old FIT implementation (DTBOs were broken there), do you mind extending your series?
Thank you for the hint. That's helpful. I was not sure how users are using the dtbo related code in the existing implementation. But the longer that I looked at that the more my conclusion was that something must be odd here. So I added a minimal test case to make it not even worse than it already is. Later on I would like to make it more useful e.g. by supporting configuration nodes which use a dtb + a dtbo image node for starting the kernel. but that's something which I did not yet look into the details. I guess the new Python code will make your patch even simpler. Do you agree with merging my series and the add a new version of your patch on top of it? Or so you see some issues with my patches?
> Do you agree with merging my series and the add a new version of your patch on top of it? Absolutely! I will be also happy to review it and test it as soon as I am back from my business trip - some time at June 6th.
This has been merged to master https://lists.openembedded.org/g/openembedded-core/message/217767 Documentation will follow soon: https://lists.yoctoproject.org/g/docs/message/7017
Let's assume this is fixed now. The FIT image related code has been completely rewritten. Also, the architecture is now different. If this is not the case, feel free to reopen this bug.