[Buildroot] [PATCH v2 2/6] fs/iso9660: add support to Grub EFI bootloader in the image
Köry Maincent
kory.maincent at bootlin.com
Tue Sep 21 18:41:44 UTC 2021
Yann,
On Tue, 21 Sep 2021 17:36:09 +0200
"Yann E. MORIN" <yann.morin.1998 at free.fr> wrote:
> > --- a/fs/iso9660/Config.in
> > +++ b/fs/iso9660/Config.in
> > @@ -3,6 +3,7 @@ config BR2_TARGET_ROOTFS_ISO9660
> > depends on (BR2_i386 || BR2_x86_64)
> > depends on BR2_LINUX_KERNEL
> > depends on BR2_TARGET_GRUB2_I386_PC || \
> > + BR2_TARGET_GRUB2_I386_EFI || BR2_TARGET_GRUB2_X86_64_EFI
> > || \
>
> iso9660 already depends on (BR2_i386 || BR2_x86_64), and thanks to the
> select of a default platform for i386 and x86_64, we know that at least
> one of the three i386-pc, i386-efi, or x86_64-efi is set, so we can
> simplify the condition to just:
>
> depends on BR2_TARGET_GRUB2 || BR2_TARGET_SYSLINUX_ISOLINUX
>
> No?
Yes, nice catch!
> > config BR2_TARGET_ROOTFS_ISO9660_BOOT_MENU
> > string "Boot menu config file"
> > - default "fs/iso9660/grub.cfg" if BR2_TARGET_ROOTFS_ISO9660_GRUB2
> > + default "fs/iso9660/grub.cfg" if
> > BR2_TARGET_ROOTFS_ISO9660_GRUB2_PC || \
> > +
> > BR2_TARGET_ROOTFS_ISO9660_GRUB2_EFI
>
> I forgot to state so in the previous review, but I think it is more
> appropriate to duplicate the default for each case; that's nicer to
> read:
>
> default "fs/iso9660/grub.cfg" if BR2_TARGET_ROOTFS_ISO9660_GRUB2_PC
> default "fs/iso9660/grub.cfg" if BR2_TARGET_ROOTFS_ISO9660_GRUB2_EFI
>
> However, BR2_TARGET_ROOTFS_ISO9660_GRUB2_PC and
> BR2_TARGET_ROOTFS_ISO9660_GRUB2_EFI are defined nowhere...
>
> Did you mean BR2_TARGET_ROOTFS_ISO9660_BIOS_BOOTLOADER or
> BR2_TARGET_ROOTFS_ISO9660_EFI_BOOTLOADER by chance?
meh, how does these config are still there !!
> > -ifeq ($(BR2_TARGET_ROOTFS_ISO9660_GRUB2),y)
> > +ifeq ($(BR2_REPRODUCIBLE),y)
> > +ROOTFS_ISO9660_MKFS_OPTS = --invariant
>
> 'MKFS' is ambiguous: it seems to imply it applies to the iso9660 mkfs,
> while in fact it applies to the vfat mkfs. So, maybe (yes, bikesheding,
> and naming is hard): ROOTFS_ISO9660_VFAT_OPTS ?
Let's go for it.
> > +define ROOTFS_ISO9660_INSTALL_BOOTLOADER
> > + rm -rf $(ROOTFS_ISO9660_EFI_PARTITION_PATH)
> > + mkdir -p $(dir $(ROOTFS_ISO9660_EFI_PARTITION_PATH))
> > + dd if=/dev/zero of=$(ROOTFS_ISO9660_EFI_PARTITION_PATH) bs=1M
> > count=1
> > + $(HOST_DIR)/sbin/mkfs.vfat $(ROOTFS_ISO9660_MKFS_OPTS)
> > $(ROOTFS_ISO9660_EFI_PARTITION_PATH)
> > + $(HOST_DIR)/bin/mcopy -p -m -i
> > $(ROOTFS_ISO9660_EFI_PARTITION_PATH) -s \
> > + $(ROOTFS_ISO9660_EFI_PARTITION_CONTENT)/* ::/
>
> Ah, but the whole point of reproducible, and thus --invariant and -p -m,
> is that the generated filesystem is reproducible. Here, the VFAT fs
> itself is reproducible thanks to --invariant, but you copy a file with
> the date/time the build occurs, which by definition is not reproducible.
> You should touch the file to $(SOURCE_DATE_EPOCH) before copying into
> the vfat.
Ack
> > +ifeq ($(BR2_TARGET_ROOTFS_ISO9660_BIOS_BOOTLOADER),y)
> > +ROOTFS_ISO9660_OPTS += \
> > + -J \
> > + -R \
> > + -boot-load-size 4 \
> > + -boot-info-table \
> > + -no-emul-boot \
>
> --no-emul-boot is in both cases, so maybe it can be left in the constant
> options, in _CMDS below, no?
True it could. -no-emul-boot need to be set in both boot cases, if fact I have
added it like that to not need to separate it in the next patch. ;)
I will add it to constant options.
>
> > + -b $(ROOTFS_ISO9660_BOOT_IMAGE)
> > +else ifeq ($(BR2_TARGET_ROOTFS_ISO9660_EFI_BOOTLOADER),y)
>
> Note that BR2_TARGET_ROOTFS_ISO9660_BIOS_BOOTLOADER and
> BR2_TARGET_ROOTFS_ISO9660_EFI_BOOTLOADER are mutually exclusive, by
> definition, but we also know at least one will be set, also by
> definition.
>
> So, the else clause can be a simple one, without the ifeq.
doh! you raise a point. In fact in the current configuration they are not
mutually exclusive. I need to fix that with a Kconfig menu.
>
> /methink.
>
> > +ROOTFS_ISO9660_OPTS += \
> > + --efi-boot $(ROOTFS_ISO9660_EFI_PARTITION) \
> > + -no-emul-boot
>
> So with an EFI bootloader, we do not need to generate an ISO with Joliet
> and RockRidge externsions? Or is that implied by the mere fact that it
> is using an efi-boot?
No, you are right I should add them to constant options.
Thanks,
Regards,
Köry
More information about the buildroot
mailing list