[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