-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: esp-qcom-image: convert to OE-core uki.bbclass #138
Conversation
classes/uki-esp-image.bbclass
Outdated
if [ -n "${KERNEL_CMDLINE_EXTRA}" ]; then | ||
UKI_CMDLINE="$UKI_CMDLINE ${KERNEL_CMDLINE_EXTRA}" | ||
fi | ||
echo "options $UKI_CMDLINE" >> ${IMAGE_ROOTFS}${ESPFOLDER}/loader/entries/boot.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this override or extend the command line in the UKI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a build perspective, it will extend what is passed into UKI_CMDLINE. From a boot perspective, the order seems to be:
- The provided cmdline in boot.conf replaces
- The provided cmdline inside the UKI replaces
- A compiled-in cmdline in the *Image
The systemd-boot documentation is ambiguous, the most definitive discussions seems to be here: 1. A compiled-in cmdline in the *Image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This behavior depends if secure boot is enabled as well.
classes/uki-esp-image.bbclass
Outdated
install -m 0644 ${DEPLOY_DIR_IMAGE}/${ESPINITRD} ${IMAGE_ROOTFS}${ESPFOLDER} | ||
echo "initrd /${ESPINITRD}" >> ${IMAGE_ROOTFS}${ESPFOLDER}/loader/entries/boot.conf | ||
fi | ||
echo "devicetree /${KERNEL_DEVICETREE}" >> ${IMAGE_ROOTFS}${ESPFOLDER}/loader/entries/boot.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally feel that specifying extra DT, initrd or kernel command line defeats the purpose of the UKI in some way. Ideally it should be a single and the only file that is being loaded by the bootloader. For example, loader.conf
can not be signed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but the aim for this PR is to match what the WIC plugin does, so we have an actual, bootable example to show why this is unsuited for the use cases we want to support. Having said that, this should be the highest quality implementation that matches that plugin!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my 2c would be to drop the initrd
and devicetree
from the beginning (and maybe the commandline as it is so unobvios). In the end, the major obstacle with WIC is in the image format, not in some other property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it is better to just support type 2 entries instead (https://uapi-group.org/specifications/specs/boot_loader_specification/#type-2-efi-unified-kernel-images), with everything included as part of the UKI.
This PR is at the stage where it boots on the RB3 board with the vision kit attached and the bbclass doesn't make my eyes bleed. It does need a bit of cleanup and much, much better error checking and logging. |
I am confused as to why we need ufi-esp-image class at all. we are now generating an EFI boot loader spec entry which we were not using before. why do we need it now? We do not use WIC since we manage our partitions/GPT separately, and the ESP image/partition (VFAT) is built by esp-qcom-image anyway. I must be missing something. |
We are currently using a home-grown way of creating UKIs, OE-core recently added a bbclass to do that. We want to see if we can use the OE-core bbclass instead of our recipe, the OE-core bbclass doesn't package the UKI, it only puts it in deploy, which is why we need this PR to get the UKI into our boot partition. If OE-core gains a way to package the generated UKI, we won't need this anymore. |
I think it might make sense to squash both commits. It will make code flow more obvious. |
I understand our home grown uki method, and the oe core class. But I did not understand why we needed another 'plumbing' class (uki-esp-image). Is it such a bad thing that the UKI is deployed? that seems to make sense. Why do we need to package the UKI? |
If UKI is packaged, then building the ESP image becomes as easy as selecting packages to install. Fetching UKI from deploy dir is more fun, it is necessary to find the image, etc. |
@@ -1,14 +1,20 @@ | |||
DESCRIPTION = "EFI System Partition Image to boot Qualcomm boards" | |||
|
|||
PACKAGE_INSTALL = " \ | |||
linux-qcom-uki \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that in our current UKI (we build with linux-qcom-uki) we do not set an initrd, while I think there is one by default with the upstream uki class. It's a boot behavior change that we need to be aware of. We might want an initrd, but we should not make this change without an explicit commit/discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initrd or initramfs? They are subtly different in how the / mountpoint is pivoted or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we use none of them right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to use initrd with meta-qcom-distro, but here I don't think it is needed / used.
The OE-core class puts the UKI in deploy, nothing more, it doesn't package it nor does it put it in staging. Aside from WIC, there is no way to include it in an image without plumbing as we have in this PR. |
Agreed, the split is mostly an artefact from the meta-qcom/meta-qcom-hwe split, I'll squash appropriately before removing the WIP status. |
This version of systemd-boot doesn't like type 2:
It doesn't say anything amount missing PE sections, but that is something I'm going to check: whether EDIT: oops, that 'ESP/Linux' is not right! |
We do not have these files in the UKI we generate so far. Not sure it makes a difference. just pointing it out. |
After fixing the ESP vs EFI folder typo:
And that fails to boot:
|
In our current builds, we do not add a DTB in the UKI, we rely on the DTB from the firmware. it looks like you are trying to use an upstream DTB here, and the firmware is failing to do its fixups. |
So, it seems, there are two issues:
BTW: is the EFI registering |
This class uses the OE-core uki.bbclass infrastructure to assemble an ESP image with systemd-boot + UKI. It tried to closely mimic the behaviour of the WIC UKI plugin, but since WIC doesn't supported full disk, partition-less images, this needs its own bbclass. Systemd-boot is installed through package management, the UKI is picked up from deploy. A systemd-boot config and boot entry config are generated based on the BSP metadata. Signed-off-by: Koen Kooi <[email protected]>
Replace homegrown linux-qcom-uki with uki.bbclass. Signed-off-by: Koen Kooi <[email protected]>
Signed-off-by: Koen Kooi <[email protected]>
Signed-off-by: Koen Kooi <[email protected]>
Signed-off-by: Koen Kooi <[email protected]>
Signed-off-by: Koen Kooi <[email protected]>
Signed-off-by: Koen Kooi <[email protected]>
Unsetting KERNEL_DEVICETREE makes the kernel boot again, but it fails to mount the rootfs on UFS. I suspect the cmdline isn't quite right. |
I think that's the initramfs which you have in this PR, which we don't. Or did you remove it already? |
The log for the EFI+systemd-boot+linux bit:
And the cmdline it is using:
|
I did not, or rather, cannot, without patching uki.bbclass in OE-core. I'm looking at that right now. |
This is a bit unexpected because the dtb used is already coming from upstream, but loaded via the dtb partition instead. Should work the same when loaded by firmware or loaded by UEFI.
Yeah, IIRC it is using EFI_DT_FIXUP_PROTOCOL. |
What we have ^ |
Signed-off-by: Koen Kooi <[email protected]>
Yeah, setting |
So far I've encountered the following situations:
|
Yeah, this is unexpected, needs validation with the 1.3 fw. |
Please move this PR to https://github.com/qualcomm-linux/meta-qcom |
Moved to qualcomm-linux/meta-qcom#722 |
This new class uses the OE-core uki.bbclass infrastructure to assemble an
ESP image with systemd-boot + UKI. It tries to closely mimic the
behaviour of the WIC UKI plugin, but since WIC doesn't supported full
disk, partition-less images, this needs its own bbclass.
Systemd-boot is installed through package management, the UKI is picked
up from deploy. A systemd-boot config and boot entry config are
generated based on the BSP metadata.
Signed-off-by: Koen Kooi [email protected]
Fixes qualcomm-linux/meta-qcom#707