GNOME Bugzilla – Bug 706370
main: Add U-Boot bootlader backend support
Last modified: 2013-08-20 17:59:57 UTC
Created attachment 252356 [details] [review] [PATCH 1/1] main: Add U-Boot bootlader backend support OSTree currently only supports booting from syslinux but it is designed to support other bootloader backends. This patch adds support to generate files that can be used by the Universal Bootloader (U-Boot). U-Boot allows to modify boards default boot commands by reading and executing a bootscript file or importing a plain text file that contains environment variables that could parameterize the boot command or a bootscript. OSTree generates a uEnv.txt file that contains booting information that is taken from Boot Loader Specification snippets files as defined in the new OSTree deployment model: https://wiki.gnome.org/OSTree/DeploymentModel2 On deploy or upgrade an uEnv.txt env var file is created in the path /boot/loader.${bootversion}/uEnv.txt. Also, a /boot/uEnv.txt symbolic link to loader/uEnv.txt is created so U-Boot can always import the file from a fixed path. Since U-Boot does not support a menu to list a set of Operative Systems, the most recent bootloader configuration from the list is used. To boot an OSTree using the generated uEnv.txt file, a board has to parameterize its default boot command using the following variables defined by OSTree: ${kernel_image}: path to the Linux kernel image ${ramdisk_image}: path to the initial ramdisk image ${bootargs}: parameters passed to the kernel command line Alternatively, for boards that don't support this scheme, a bootscript that overrides the default boot command can be used. An example of such a bootscript could be: setenv scriptaddr 40008000 setenv kernel_addr 0x40007000 setenv ramdisk_addr 0x42000000 ext2load mmc 0:1 ${scriptaddr} uEnv.txt env import -t ${scriptaddr} ${filesize} ext2load mmc 0:1 ${kernel_addr} ${kernel_image} ext2load mmc 0:1 ${ramdisk_addr} ${ramdisk_image} bootm ${kernel_addr} ${ramdisk_addr}
Review of attachment 252356 [details] [review]: Thanks for the extensive commit message! I appreciate the detail. Two comments: ::: src/ostree/ot-bootloader-uboot.c @@ +69,3 @@ + return FALSE; + + /* U-Boot doesn't support a menu so just pick the first one since the list is ordered */ That's unfortunate. But oh well, maybe u-boot can be improved later. The other option is to allow the user to pick in the initramfs, but that complicates things a lot...we'd need to merge /lib/modules in all trees, and ugh. @@ +93,3 @@ + +static char * +join_lines (GPtrArray *lines) Can you split this out into a helper somewhere rather than copying it? Maybe in ot-admin-util.c as ot_admin_join_config_lines(), and prototyped in ostree-admin-functions.h?
(In reply to comment #1) > Review of attachment 252356 [details] [review]: > > Thanks for the extensive commit message! I appreciate the detail. Two > comments: > Thanks a lot for your feedback! > ::: src/ostree/ot-bootloader-uboot.c > @@ +69,3 @@ > + return FALSE; > + > + /* U-Boot doesn't support a menu so just pick the first one since the list > is ordered */ > > That's unfortunate. But oh well, maybe u-boot can be improved later. > > The other option is to allow the user to pick in the initramfs, but that > complicates things a lot...we'd need to merge /lib/modules in all trees, and > ugh. > Indeed. It is also unfortunate that the Boot Loader Specification snippets don't have a "default" field or some hint on how to enumerate the boot loader entries. > @@ +93,3 @@ > + > +static char * > +join_lines (GPtrArray *lines) > > Can you split this out into a helper somewhere rather than copying it? Maybe > in ot-admin-util.c as ot_admin_join_config_lines(), and prototyped in > ostree-admin-functions.h? Ok, I'll split the helper function in a separate patch and post a v2 of the patch-set. Thanks a lot and best regards
(In reply to comment #2) > > Indeed. It is also unfortunate that the Boot Loader Specification snippets > don't have a "default" field or some hint on how to enumerate the boot loader > entries. After talking with Kay about this in person, the intended end game is that bootloaders parse /boot/loader/entries *on boot*, dynamically. But they should keep existing configuration files for things like which entry is the default, the splash screen, timeouts etc. So /boot/syslinux/syslinux.cfg would just be DEFAULT 0. The code I have in OSTree now is a short/medium term hack until someone (maybe eventually me) patches syslinux to support that. You might note that the code tries fairly hard to support merging any user edits to the syslinux config (like the DEFAULT). Since u-boot doesn't support multiple entries at all, this is less of an issue.
(In reply to comment #3) > (In reply to comment #2) > > > > Indeed. It is also unfortunate that the Boot Loader Specification snippets > > don't have a "default" field or some hint on how to enumerate the boot loader > > entries. > > After talking with Kay about this in person, the intended end game is that > bootloaders parse /boot/loader/entries *on boot*, dynamically. But they should > keep existing configuration files for things like which entry is the default, > the splash screen, timeouts etc. > > So /boot/syslinux/syslinux.cfg would just be DEFAULT 0. > I see, In fact this is exactly what the blscfg module does for Grub from what I've seen in the patch that adds Boot Loader Spec parsing support (parsing /boot/loader/entries on runtime and dynamically adding grub menu entries). I was wondering about the ordering in Grub but now thanks to your explanation I understand that something like set default=$foo is still needed in grub.cfg. > The code I have in OSTree now is a short/medium term hack until someone (maybe > eventually me) patches syslinux to support that. You might note that the code > tries fairly hard to support merging any user edits to the syslinux config > (like the DEFAULT). > > Since u-boot doesn't support multiple entries at all, this is less of an issue. This patch is also a workaround to allow booting boards that either ship with an U-Boot that can't be updated or use an archaic U-Boot from a vendor tree. Since most boards shipped with U-Boot allows reading a uEnv.txt or executing a bootscript. It would be nice to extend U-Boot to support parsing /boot/loader/entries and have multiple entries so the user can choose which OS to boot. I may eventually try to do this too.
Created attachment 252445 [details] [review] [PATCH 1/3] admin: add ot_admin_join_config_lines() helper function admin: add ot_admin_join_config_lines() helper function ot-bootloader-syslinux.c has a join_lines() function that is rather generic and can be used in other places. Let's add it as a helper function.
Created attachment 252446 [details] [review] [PATCH 2/3] main: use ot_admin_join_lines() helper in ot-bootloader-syslinux.c The join_lines() functions has been split out as a helper so let's use that instead of the private one.
Created attachment 252447 [details] [review] [PATCH 3/3] main: Add U-Boot bootlader backend support This is a v2 of the patch that addresses issues raised by Colin Walters. Changes since v1: - Split out join_lines() from ot-bootloader-syslinux.c as a helper function so it can be reused.
Comment on attachment 252445 [details] [review] [PATCH 1/3] admin: add ot_admin_join_config_lines() helper function This looks good, I'd prefer the 1st and second patches to be squashed though so there isn't a point of code duplication. I took care of this myself and will push.
Comment on attachment 252446 [details] [review] [PATCH 2/3] main: use ot_admin_join_lines() helper in ot-bootloader-syslinux.c Will be squashed.
Review of attachment 252447 [details] [review]: Looks good. One mechanical thing: I'd like for the commit messages to link back to the bug. See: https://live.gnome.org/GnomeLove/SubmittingPatches The advantage of this is it's a lot easier later to see the detailed discussion around a particular commit.
(In reply to comment #10) > Review of attachment 252447 [details] [review]: > > Looks good. One mechanical thing: I'd like for the commit messages to link > back to the bug. See: > > https://live.gnome.org/GnomeLove/SubmittingPatches > > The advantage of this is it's a lot easier later to see the detailed discussion > around a particular commit. Thanks a lot for your help and sorry for forgetting to include the link to the bug.