Bug#922251: live-build: support syslinux-efi as (additional) bootloader

classic Classic list List threaded Threaded
28 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

adrian15
El 08/04/19 a las 21:29, Matthijs Kooijman escribió:
> Hi Luca & others,
>
> I've been working on syslinux-efi support in the past weeks and just
> submitted a MR with a working implementation, along with some small
> bootloader-related cleanups and refactors:
>
> https://salsa.debian.org/live-team/live-build/merge_requests/19
Commenting below.
>
> In the end, I opted to implement syslinux-efi rather than make grub-efi
> work on hdd images, since that seemed easier and allows preserving the
> existing bootloader config files in our project. Getting grub-efi to
> work on hdd images might still be an interesting project that could be
> implemented alongside syslinux-efi support, though I do not have any
> specific purpose for it as of yet.
I implemented multi bootloader support back in the day and maybe
grub-efi, not sure about that last one.

I didn't implement the hdd part because I didn't know what was the
purpose of an hdd image and how I was supposed to test it.

I might take a look into your notes to implement grub-efi + secure boot
in hdd img but... this might be in 2021 XD . Too busy at the moment.

What's your use case? What do you need to build live-build based hdd
imgs? I'm curious.

> As predicted by others in this bug report, my work does not enable
> secure boot (which syslinux simply does not support), nor enable
> syslinux-efi support in iso/isohybrid images (since syslinux-efi does
> not support this, or at least it apparently does not work).

Nice. What you need to make sure is that you cannot choose syslinux-efi
when trying to build iso/isohybrid images.

> Gr.
>
> Matthijs

> 0) backward incompatible change
In the pull request there is a mention about a backward incompatible
change. Can you please describe in what that change consists?

> 1) Refactor some bootloader scripts (969cdf4e)

1.1) Nice idea the Get_Bootloader_Config_Dir function.
Have you made sure that $BOOTLOADER variable is not used anywhere else
in the code?
Maybe you should make that variable local.

1.2) The function Install_Bootloader_Files does not seem to be right.
If you are not going to reuse that function in more than one place (I'll
check your other commits later) you might consider not having that
function in the first place.

Even if the Install_Bootloader_Files function was a good idea it's not
well implemented commit-wise. You are performing two changes. One about
moving the functionality to a function. Another one is chaning the
comment about "two steps" to "three steps". It's hard to notice if you
changed something other than that comment on that commit.

I mean... this part should be splitted in two commits.

> 2) Reduce config duplication in syslinux variants (bccd127b)

I know it's better the way you merge files there but I think the
original idea was about having independent configuration files for each
different media.

I mean there were not meant to be merged together in the first place.

> 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)

3.1) Nice. I didn't know about those new syslinux architecture dependent
files. As per the wiki you link (
https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
) in the commit description I guess that even ldlinux.c32 wouldn't be
used but ldlinux.e64 instead.

3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?

> 4) Move syslinux modules into subdirectories (53901001)

Shouldn't those c32 modules be moved to a folder named c32-modules (or
c32mods for short name) instead?

Let's see how you name the UEFI modules folder later.

In "Add syslinux-efi bootloader support (a271f8f9)" you use modules32
folder name for some c32. Shouldn't you use modules32 as a folder name
in this "Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)" commit
in the first place?

I'm not sure about this one I might be missing something.

>  5) Add syslinux-efi bootloader support (a271f8f9)
5.1) You have got a nice bug on that code XD . Just after the Echo_error
statements about syslinux-efi not being supported there should be an
exit 1 statement.

Else you are just echoing a warning but not stopping the build.

5.2) Anyway I don't think I like this code at all. I mean... you are
supposed to create a new file named:
scripts/build/binary_syslinux-efi
and not hack around the existant one: binary_syslinux
.

I know it's difficult to work on a new binary file because of the way
binary_syslinux handles kernel images and renames them.
But I was able to handle that on the loopback.cfg binary file.

Probably the common code for generating cfg files should be on a common
binary file which it's only triggered if either binary_syslinux or
binary_syslinux-efi is being used as a bootloader.

5.3) I see you add a lot of c32 and efi as symbolic links to paths on
the system.
I'm not sure this is the right way of doing this.
Are other equivalent files already implemented like you are doing?
Or is it me finding it out for the first time.

5.4)
In "Move syslinux modules into subdirectories (53901001)" you are using:
"path modules"
here you are using:
"PATH /@EFI_DIR@/modules32"
.

This should be using either "path" in both files or "PATH" in both
files. As the rest of the syslinux commands are in non capital letters I
suggest using "path".

6) Only default to grub-efi for iso* images (118de99a)

Seems good to me.

7) Improve bootloader configuration checks (5fb9ab31)

I need to read through these changes another day.
Was live-build to be based on python given a compatibility matrix based
on nested arrays... this should be easier to implement.

8) Remove --templates from lb_config manpage (371892af)

As I said in another mail this option makes more sense in another pull
request but I guess you can leave it in this pull request for now.

9) Use syslinux-efi for hdd images by default (4e292ed0)

Seems good to me.

10) I think buxy implemented syslinux-efi support back in the day. I
guess it wasn't added to live-build because of reasons? Because grub-efi
enabled Secure Boot to work?

Anyways... Did you ever check buxy's work just in case it has something
that you can recycle?



I'll try to comment on "Improve bootloader configuration checks
(5fb9ab31)" on the next days. I need to be focused on this one :) .


Waiting for your feedback on the rest of the points. Hopefully we can
agree on the binary_syslinux-efi creation and other points :) .




adrian15
--
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/

Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Matthijs Kooijman-3
In reply to this post by adrian15
Hey Adrian,

[ About removing --templates from the manpage ]
> In that case IMO that commit should be in its own pull request and not
> the current one.
Done: https://salsa.debian.org/live-team/live-build/merge_requests/21

Gr.

Matthijs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Matthijs Kooijman-3
In reply to this post by Thierry-9
Hey Thierry,

> Is there a chance that this work will be part of buster live-build
> package, or is it too late already ?
I'm not the maintainer of live-build, but given the freeze state that
buster is in, I highly doubt this will make it into buster.

Gr.

Matthijs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Matthijs Kooijman-3
In reply to this post by adrian15
Hi Adrian,

thanks for your extensive review. I'll respond inline.

> I might take a look into your notes to implement grub-efi + secure boot
> in hdd img but... this might be in 2021 XD . Too busy at the moment.
Familiar sentiment. Would be nice to have it, though.

> What's your use case? What do you need to build live-build based hdd
> imgs? I'm curious.
I'm using live-build to build Webconverger (a kiosk OS booting into a
full-screen browser) and an image derived for that for Bizplay (a
digital signage system).

We need hdd images rather than isohybrid images, so the resulting
USB-disk can be written to. This is needed firstly to update the
bootloader configuration to change the kernel commandline (we use this
to customize the image, e.g. change the default homepage shown).
Secondly, the image uses git to manage the root filesystem and a running
image can update itself, but that again needs a writable filesystem.


> > As predicted by others in this bug report, my work does not enable
> > secure boot (which syslinux simply does not support), nor enable
> > syslinux-efi support in iso/isohybrid images (since syslinux-efi does
> > not support this, or at least it apparently does not work).
>
> Nice. What you need to make sure is that you cannot choose syslinux-efi
> when trying to build iso/isohybrid images.
Yeah, that's checked by the "Improve bootloader configuration checks"
commit.

> > 0) backward incompatible change
> In the pull request there is a mention about a backward incompatible
> change. Can you please describe in what that change consists?
I've added one commit with a NEWS file that describes this issue in a
bit more detail. Does that clarify sufficiently?

Note that while writing the NEWS entry, it seemed more confusing to have
a bootloaders/syslinux-common directory as well as a syslinux-common
Debian package, so I ended up modifying my commits to use
"syslinux-shared" rather than "syslinux-common" (I'll also use the
former below).

> > 1) Refactor some bootloader scripts (969cdf4e)
>
> 1.1) Nice idea the Get_Bootloader_Config_Dir function.
> Have you made sure that $BOOTLOADER variable is not used anywhere else
> in the code?
> Maybe you should make that variable local.
Good point. I guess I was following the style of other functions (that
dod not use local variables either), but that's no reason not to do it
here. I updated the PR with this change.

> 1.2) The function Install_Bootloader_Files does not seem to be right.
> If you are not going to reuse that function in more than one place (I'll
> check your other commits later) you might consider not having that
> function in the first place.
I am indeed planning to reuse the function. I've clarified this in the
commit message.


> Even if the Install_Bootloader_Files function was a good idea it's not
> well implemented commit-wise. You are performing two changes. One about
> moving the functionality to a function. Another one is chaning the
> comment about "two steps" to "three steps". It's hard to notice if you
> changed something other than that comment on that commit.
>
> I mean... this part should be splitted in two commits.
I'm usually a nitpick in this area, but here I decided for a bit more
simplicity. You're right, though. I ended up splitting this into three
commits, one for Get_Bootloader_Config_Dir, one for
Install_Bootloader_Files and one for the comment.

> > 2) Reduce config duplication in syslinux variants (bccd127b)
>
> I know it's better the way you merge files there but I think the
> original idea was about having independent configuration files for each
> different media.
>
> I mean there were not meant to be merged together in the first place.
The reason to do this, is that I want configuration files to be shared
between syslinux and syslinux-efi (which can co-exist in the same
image). If these would be duplicated everywhere, modifying the
bootloader config after an image is written to disk requires modifying
it in two places.

Note that syslinux-efi could still not have its own config files, but
refer to files contained in bootloaders/syslinux (without the need for
bootloaders/syslinux-shared), but then you can never have an image that
*only* contains syslinux-efi (which you can with the syslinux-shared
approach, since then you use syslinux-shared for the config and
syslinux-efi for the efi images).

> > 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
>
> 3.1) Nice. I didn't know about those new syslinux architecture dependent
> files. As per the wiki you link (
> https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
> ) in the commit description I guess that even ldlinux.c32 wouldn't be
> used but ldlinux.e64 instead.
Actually, ldlinux.e64 is only for EFI. This commit only touches
BIOS-related stuff. I'm not sure how "architecture dependent files" are
relevant here, since this commit just removes some files which are
really superfluous (since the 'syslinux' and 'extlinux' commands used to
install the bootloader in binary_hdd make sure to copy their own version
of these files into the image).

> 3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?
Yes, but that's a later commit.

> > 4) Move syslinux modules into subdirectories (53901001)
>
> Shouldn't those c32 modules be moved to a folder named c32-modules (or
> c32mods for short name) instead?
>
> Let's see how you name the UEFI modules folder later.
>
> In "Add syslinux-efi bootloader support (a271f8f9)" you use modules32
> folder name for some c32. Shouldn't you use modules32 as a folder name
> in this "Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)" commit
> in the first place?
>
> I'm not sure about this one I might be missing something.
I think the missing bit might be that
bootloaders/{syslinux,syslinux-shared} is installed into binary/boot,
while bootloaders/syslinux-efi is installed into binary/EFI/boot, so
these are distinct locations.

Also, EFI supports 32 and 64 bit (hence modules32 and modules64), but
BIOS is (by definition, I think) only 32-bit, so I just used "modules".

> >  5) Add syslinux-efi bootloader support (a271f8f9)
> 5.1) You have got a nice bug on that code XD . Just after the Echo_error
> statements about syslinux-efi not being supported there should be an
> exit 1 statement.
Thanks, fixed.

> 5.2) Anyway I don't think I like this code at all. I mean... you are
> supposed to create a new file named:
> scripts/build/binary_syslinux-efi
> and not hack around the existant one: binary_syslinux

That would make sense if I wanted syslinux-efi to be really indepenent,
but as I noted above, I wanted to make them share a single configuration
(and also allow syslinux-efi to be installed by itself). This seemed
like best way to achieve that, though alternative suggestions are
welcome :-)

I pushed a new version with the above changes to
https://salsa.debian.org/live-team/live-build/merge_requests/19

Gr.

Matthijs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

adrian15
El 16/04/19 a las 20:17, Matthijs Kooijman escribió:

> Hi Adrian,
>
> thanks for your extensive review. I'll respond inline.
>
>> I might take a look into your notes to implement grub-efi + secure boot
>> in hdd img but... this might be in 2021 XD . Too busy at the moment.
> Familiar sentiment. Would be nice to have it, though.
>
>> What's your use case? What do you need to build live-build based hdd
>> imgs? I'm curious.
> I'm using live-build to build Webconverger (a kiosk OS booting into a
> full-screen browser) and an image derived for that for Bizplay (a
> digital signage system).
>
> We need hdd images rather than isohybrid images, so the resulting
> USB-disk can be written to. This is needed firstly to update the
> bootloader configuration to change the kernel commandline (we use this
> to customize the image, e.g. change the default homepage shown).
> Secondly, the image uses git to manage the root filesystem and a running
> image can update itself, but that again needs a writable filesystem.
Ok, understood.

>>> As predicted by others in this bug report, my work does not enable
>>> secure boot (which syslinux simply does not support), nor enable
>>> syslinux-efi support in iso/isohybrid images (since syslinux-efi does
>>> not support this, or at least it apparently does not work).
>>
>> Nice. What you need to make sure is that you cannot choose syslinux-efi
>> when trying to build iso/isohybrid images.
> Yeah, that's checked by the "Improve bootloader configuration checks"
> commit.
Ok, I saw that.

>>> 0) backward incompatible change
>> In the pull request there is a mention about a backward incompatible
>> change. Can you please describe in what that change consists?
> I've added one commit with a NEWS file that describes this issue in a
> bit more detail. Does that clarify sufficiently?
>
> Note that while writing the NEWS entry, it seemed more confusing to have
> a bootloaders/syslinux-common directory as well as a syslinux-common
> Debian package, so I ended up modifying my commits to use
> "syslinux-shared" rather than "syslinux-common" (I'll also use the
> former below).
Ok, now 'Add NEWS entry about syslinux config refactoring'
(00eab3a7) makes clear what it's not backward compatible (under certain
circumstances).

It's ok for me.

>>> 1) Refactor some bootloader scripts (969cdf4e)
>>
>> 1.1) Nice idea the Get_Bootloader_Config_Dir function.
>> Have you made sure that $BOOTLOADER variable is not used anywhere else
>> in the code?
>> Maybe you should make that variable local.
> Good point. I guess I was following the style of other functions (that
> dod not use local variables either), but that's no reason not to do it
> here. I updated the PR with this change.
Nice.

>> 1.2) The function Install_Bootloader_Files does not seem to be right.
>> If you are not going to reuse that function in more than one place (I'll
>> check your other commits later) you might consider not having that
>> function in the first place.
> I am indeed planning to reuse the function. I've clarified this in the
> commit message.
Then it's ok I guess.

>> Even if the Install_Bootloader_Files function was a good idea it's not
>> well implemented commit-wise. You are performing two changes. One about
>> moving the functionality to a function. Another one is chaning the
>> comment about "two steps" to "three steps". It's hard to notice if you
>> changed something other than that comment on that commit.
>>
>> I mean... this part should be splitted in two commits.
> I'm usually a nitpick in this area, but here I decided for a bit more
> simplicity. You're right, though. I ended up splitting this into three
> commits, one for Get_Bootloader_Config_Dir, one for
> Install_Bootloader_Files and one for the comment.
Ok. Thank you.

>>> 2) Reduce config duplication in syslinux variants (bccd127b)
>>
>> I know it's better the way you merge files there but I think the
>> original idea was about having independent configuration files for each
>> different media.
>>
>> I mean there were not meant to be merged together in the first place.
> The reason to do this, is that I want configuration files to be shared
> between syslinux and syslinux-efi (which can co-exist in the same
> image). If these would be duplicated everywhere, modifying the
> bootloader config after an image is written to disk requires modifying
> it in two places.
>
> Note that syslinux-efi could still not have its own config files, but
> refer to files contained in bootloaders/syslinux (without the need for
> bootloaders/syslinux-shared), but then you can never have an image that
> *only* contains syslinux-efi (which you can with the syslinux-shared
> approach, since then you use syslinux-shared for the config and
> syslinux-efi for the efi images).
Ok, I guess it's fine. live-wrapper produced ISOs from official Debian
also have those common and shared cfg files I guess that stolen from the
way Ubuntu (and other distros) deal with this too.

>>> 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
>>
>> 3.1) Nice. I didn't know about those new syslinux architecture dependent
>> files. As per the wiki you link (
>> https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
>> ) in the commit description I guess that even ldlinux.c32 wouldn't be
>> used but ldlinux.e64 instead.
> Actually, ldlinux.e64 is only for EFI. This commit only touches
> BIOS-related stuff. I'm not sure how "architecture dependent files" are
> relevant here, since this commit just removes some files which are
> really superfluous (since the 'syslinux' and 'extlinux' commands used to
> install the bootloader in binary_hdd make sure to copy their own version
> of these files into the image).
I'm revisiting this commit and I'm not sure this is right thing to do.
If your pull request only affects syslinux-efi why do you even care
about 32bit code?

You are assuming that syslinux and extlinux commands are only used in
binary_hdd and by grepping the code it's seems to be true. So it's ok
with this assumption.

I guess that this directory was initially meant to be used as a template.
So it makes sense to keep those c32 files into their original place.
Just in case you want to override these files with your own custom c32
files.

Is it worth keeping those files there nowadays?
They are just symlinks after installing the package.

Well, I'm not sure about this one commit.

Anyway I think this commit should be on its own Merge Request because I
think adding syslinux-efi is going to work regardless of these files
getting into the final disk.

>> 3.2) BTW Do you support both EFI IA32 and EFI X64 or only EFI X64 ?
> Yes, but that's a later commit.
Ok.

>>> 4) Move syslinux modules into subdirectories (53901001)
>>
>> Shouldn't those c32 modules be moved to a folder named c32-modules (or
>> c32mods for short name) instead?
>>
>> Let's see how you name the UEFI modules folder later.
>>
>> In "Add syslinux-efi bootloader support (a271f8f9)" you use modules32
>> folder name for some c32. Shouldn't you use modules32 as a folder name
>> in this "Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)" commit
>> in the first place?
>>
>> I'm not sure about this one I might be missing something.
> I think the missing bit might be that
> bootloaders/{syslinux,syslinux-shared} is installed into binary/boot,
> while bootloaders/syslinux-efi is installed into binary/EFI/boot, so
> these are distinct locations.
You are right. This was the missing bit. I see.

> Also, EFI supports 32 and 64 bit (hence modules32 and modules64), but
> BIOS is (by definition, I think) only 32-bit, so I just used "modules".

Well, I think you should use something else.
modules32 is 9 characters long (not 8.3 compatible). What about module32
and module64? So that we can reuse the code in the iso side when
isolinux is updated to support hybrid isos in efi mode ?

>>>  5) Add syslinux-efi bootloader support (a271f8f9)
>> 5.1) You have got a nice bug on that code XD . Just after the Echo_error
>> statements about syslinux-efi not being supported there should be an
>> exit 1 statement.
> Thanks, fixed.
Nice.

>> 5.2) Anyway I don't think I like this code at all. I mean... you are
>> supposed to create a new file named:
>> scripts/build/binary_syslinux-efi
>> and not hack around the existant one: binary_syslinux
>
> That would make sense if I wanted syslinux-efi to be really indepenent,
> but as I noted above, I wanted to make them share a single configuration
> (and also allow syslinux-efi to be installed by itself). This seemed
> like best way to achieve that, though alternative suggestions are
> welcome :-)

Well, you could have a binary_syslinux_cfg file similar to the
binary_loopback_cfg one or maybe binary_shared_cfg.

>
> I pushed a new version with the above changes to
> https://salsa.debian.org/live-team/live-build/merge_requests/19

6.1)
PATH syslinux command is still being written in capital letters in
share/bootloaders/syslinux-efi/syslia32.cfg and
share/bootloaders/syslinux-efi/syslx64.cfg while it should be written in
non-capital letters.

6.2) What you should learn about the multibootloader support is that it
should let use as much as bootloaders as you want to.

So, you could have something like:

--bootloaders syslinux,syslinux-efi,grub-efi

which, of course it does not make sense because either syslinux-efi and
grub-efi would overwrite each ones code.

Anyway what I wanted to say here is that:

--bootloaders syslinux,syslinux-efi
and
--bootloaders syslinux-efi,syslinux

are not the same thing.

The first one (regarding binary_syslinux) should install special MBR
code so that BIOS can chainload into syslinx.
The second one (regarding binary_syslinux) should NOT install special
MBR code so that BIOS can chainload into syslinux. It should inhibit
itself because it is not the first bootloader.

Or maybe it should not inhibit but you can do it.

Well, I only had to deal with binary_iso when adding multi bootloaders
support so I modified the binary_hdd the minimum so that it worked with
the new variables.

So for the binary_iso part we used:
https://salsa.debian.org/live-team/live-build/blob/e4a4d4ae8d6379eae39eacb95486df1e5f028a69/scripts/build/binary_iso#L124-127
.

        if [ ${BOOTLOADER_NUMBER} -ge 2 ]
        then
                XORRISO_OPTIONS="${XORRISO_OPTIONS} -eltorito-alt-boot "
        fi

This means that an extra 'eltorito' is added, so that the new entry at
the isos can be recognised.
So that a BIOS firmware that supports eltorito specification can choose
between booting into the default torito entry or the non default one.

I'm not sure there's an equivalent thing that should be added to
binary_hdd now that we are at it.

Anyways... if we take a look at your untouched binary_hdd:
https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd
we can see that grub-pc is not supported. I guess I might add it in the
future.
But grub legacy is supported. (Well, it's not actually not supported.)
Assuming grub legacy are available at buster which I'm not totally sure.

Anyway... what I want to say is that you should be able to choose:

--bootloaders grub,syslinux-efi

or (if grub-pc was implemented there)

--bootloaders grub-pc,syslinux-efi

in a hdd target and 'syslinux' installation should be only triggered if
it's the first bootloader.

Well, that's exactly how binary_hdd works right now... although the
multi bootloader part should be improved to have something better than:

https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L60-86

All of this above is trying to improve multi bootloader support in the
binary_hdd part of live-build. Not sure if you should deal with this
prior to adding your code.

But if you want to take a look and tell us if binary_hdd should be
updated or not (in the end the efi installation is handled by
binary_syslinux-efi or binary_grub-efi in the filesystem level and not
by binary_hdd).


Having to deal with separate bootloaders, what they add or contribute
that's another reason why I prefer binary_syslinux and
binary_syslinux-efi being in different files.


6.3) Many of your commits seem to need a rebase into the current master
branch. Well, that's to be expected.


>
> Gr.
>
> Matthijs
>

adrian15
--
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/

Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

adrian15
In reply to this post by Matthijs Kooijman-3
El 16/04/19 a las 20:17, Matthijs Kooijman escribió:

> I pushed a new version with the above changes to
> https://salsa.debian.org/live-team/live-build/merge_requests/19
>
> Gr.
>
> Matthijs



7) Only default to grub-efi for iso* images (47d99222)
Looks good to me.
8) Use syslinux-efi for hdd images by default (658c0e2d)
Looks good to me.
9) Improve bootloader configuration checks (3e6645a2)
Well, the way how defaults.sh checks if anything is valid is kind of
antique.

In my opinnion binary_syslinux, binary_syslinux-efi, binary_grub-pc and
so on... should be able to be sourced and use sort of can_i_boot
function where you input architectures, filesystem and if the bootloader
is the first bootloader or not.

Then the binary_syslinux (or whatever) replies.

Anyways... that would be another improvement towards moving live-build
into a more Object-Oriented approach.

I haven't checked every possible combination you put there I guess it's ok.

So, once again looks good to me.


adrian15
--
Support free software. Donate to Super Grub Disk. Apoya el software
libre. Dona a Super Grub Disk. http://www.supergrubdisk.org/donate/

Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Matthijs Kooijman-3
In reply to this post by adrian15
Hi Adrian,

thanks again for your thorough review. I'm responding to both your
e-mails inline below.

> >>> 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
> >>
> >> 3.1) Nice. I didn't know about those new syslinux architecture dependent
> >> files. As per the wiki you link (
> >> https://wiki.syslinux.org/wiki/index.php?title=Library_modules#All_Syslinux_variants_need_an_additional_ldlinux_module
> >> ) in the commit description I guess that even ldlinux.c32 wouldn't be
> >> used but ldlinux.e64 instead.
> > Actually, ldlinux.e64 is only for EFI. This commit only touches
> > BIOS-related stuff. I'm not sure how "architecture dependent files" are
> > relevant here, since this commit just removes some files which are
> > really superfluous (since the 'syslinux' and 'extlinux' commands used to
> > install the bootloader in binary_hdd make sure to copy their own version
> > of these files into the image).
> I'm revisiting this commit and I'm not sure this is right thing to do.
> If your pull request only affects syslinux-efi why do you even care
> about 32bit code?
True, this was just a cleanup I came across. It is sort of related due
to the next commit that moves all c32 modules into a subdirectory. I
wondered whether to also move ldlinux.c32 and whether leaving it would
cause problems with the CWD change in the EFI boot. I then concluded the
file was not used at all and just sidestepped the issue by removing it
entirely.

I did one more test and noted that the existence of these files do not
cause problems for syslinux-efi. I'm not sure it it makes sense to keep
them or not, but dropping this commit from this MR for now seems fine
with me.

> > Also, EFI supports 32 and 64 bit (hence modules32 and modules64), but
> > BIOS is (by definition, I think) only 32-bit, so I just used "modules".
>
> Well, I think you should use something else.
> modules32 is 9 characters long (not 8.3 compatible). What about module32
> and module64? So that we can reuse the code in the iso side when
> isolinux is updated to support hybrid isos in efi mode ?
Good point, hadn't considered 8.3 compatibility. Singular "module32"
sounds a bit weird to me, but it is probably clearer than something like
"mods32" or even just "c32" (the latter seems reasonable, except that
the "c64" directory would then still contain ".c32" files since that's
what 64-bit syslinux-efi also uses...).

> >> 5.2) Anyway I don't think I like this code at all. I mean... you are
> >> supposed to create a new file named:
> >> scripts/build/binary_syslinux-efi
> >> and not hack around the existant one: binary_syslinux
> >
> > That would make sense if I wanted syslinux-efi to be really indepenent,
> > but as I noted above, I wanted to make them share a single configuration
> > (and also allow syslinux-efi to be installed by itself). This seemed
> > like best way to achieve that, though alternative suggestions are
> > welcome :-)
>
> Well, you could have a binary_syslinux_cfg file similar to the
> binary_loopback_cfg one or maybe binary_shared_cfg.
That would indeed make some sense. I had not really considered this
before, but did now. The problem with this approach is that both
binary_syslinux_cfg, binary_syslinux and binary_syslinux-efi would need
to duplicate code. At least they all need Install_Bootloader_Files,
which could be slightly generalized and moved into functions.

There is also a bunch of code which post-processes .cfg and splash.svg
files. This would be mostly needed in binary_syslinux_cfg only (since
the syslinux/syslinux-efi only contains a minimal config file that
includes the shared config file and needs minimal post-processing).

However, if binary_syslinux_cfg would install syslinux-shared and then
do all the post-processing, followed by binary_syslinux and/or
binary_syslinux-efi without any post-processing, you would:
 - lose the ability to override some the shared config files with
   bootloader-specific (e.g. syslinux/isolinux/extlinux)
 - lose backward-compatibility with existing live-build configs that do
   not have a syslinux-shared config directory but have all their config
   in the bootloader-specific directory (which is installed *after*
   configs are post-processed).

This can probably be fixed by further splitting the cfg step into an
install and post-processing step. You would then get:
 - binary_syslinux_cfg that installs config files verbatim (runs if any
   of syslinux/extlinux/isolinux/syslinux-efi is selected).
 - binary_syslinux that installs the modules and main config file for
   syslinux/isolinux/extlinux (runs if any of syslinux/extlinux/isolinux
   is selected, but not if only syslinux-efi is selected).
 - binary_syslinux-efi that installs the modules and main config file for
   syslinux-efi (runs if syslinux-efi is selected).
 - binary_syslinux_proces_cfg that post-processes all config files
   installed by previous steps (runs if any of
   syslinux/extlinux/isolinux/syslinux-efi is selected).

But I'm not so sure that this is really better than the current
single-script for everything approach (which essentially just does the
above four steps in a single script).

> 6.1)
> PATH syslinux command is still being written in capital letters in
> share/bootloaders/syslinux-efi/syslia32.cfg and
> share/bootloaders/syslinux-efi/syslx64.cfg while it should be written in
> non-capital letters.
Good catch, will fix that.

> 6.2) What you should learn about the multibootloader support is that it
> should let use as much as bootloaders as you want to.
>
> So, you could have something like:
>
> --bootloaders syslinux,syslinux-efi,grub-efi
>
> which, of course it does not make sense because either syslinux-efi and
> grub-efi would overwrite each ones code.
I see, and I think this still "works" with my MR.

> Anyway what I wanted to say here is that:
>
> --bootloaders syslinux,syslinux-efi
> and
> --bootloaders syslinux-efi,syslinux
>
> are not the same thing.
>
> The first one (regarding binary_syslinux) should install special MBR
> code so that BIOS can chainload into syslinx.
> The second one (regarding binary_syslinux) should NOT install special
> MBR code so that BIOS can chainload into syslinux. It should inhibit
> itself because it is not the first bootloader.
I'm not so convinved that this is how it should be. I guess the
complication comes from HDD images, as you mentioned. On ISOs you just
have multiple bootloader "slots" with one being the default. On a HDD
image, you sort of have 2 bootloader "slots": the BIOS MBR and the EFI
boot dir. As long as you select only one BIOS bootloader and one EFI
bootloader, they can (and probably should) nicely coexist. Once you
select more than one, they will likely overwrite each other, or perhaps
only the first one should take effect (the latter is how it works with
ISOs, I guess.).

I guess the interpretation of multiple bootloaders is currently
image-type-specific:
 - For iso images, all bootloaders are installed and the first one is
   made the default.
 - For hdd images before my MR, only the first bootloader is installed.
   Any extra bootloaders have their config files installed, but they are
   not installed into the MBR. EFI bootloaders are not supported for hdd
   images.
 - For hdd images with my MR applied, any BIOS bootloader is only
   installed if it is the first bootloader. Any EFI bootloader (which
   can only be syslinux-efi) is installed, regardless of its position.
   Multiple efi bootloaders would overwrite each other.

This might warrant an extra configuration check for hdd images perhaps,
that checks that no BIOS-bootloader appears in second or further
positions and only one EFI bootloader appears in the list? I did not add
anything like that, since there is no such check in place currently and
I did not want to further complicate the MR.

> Anyways... if we take a look at your untouched binary_hdd:
> https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd
> we can see that grub-pc is not supported. I guess I might add it in the
> future.
> But grub legacy is supported. (Well, it's not actually not supported.)
> Assuming grub legacy are available at buster which I'm not totally sure.
I actually think no grub is supported by hdd, the only grub code in
there is in a big FIXME:
https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L282-311

But this is probably beside the point.

> Anyway... what I want to say is that you should be able to choose:
>
> --bootloaders grub,syslinux-efi
>
> or (if grub-pc was implemented there)
>
> --bootloaders grub-pc,syslinux-efi
>
> in a hdd target and 'syslinux' installation should be only triggered if
> it's the first bootloader.
This is what the current MR implements. In this case, the
"syslinux-shared" configuration is installed, which is referenced by the
"syslinux-efi" installation.

See https://salsa.debian.org/matthijs-guest/live-build/blob/syslinux-efi/scripts/build/binary_syslinux#L165-171

>
> Well, that's exactly how binary_hdd works right now... although the
> multi bootloader part should be improved to have something better than:
>
> https://salsa.debian.org/live-team/live-build/blob/00eab3a77f3da176f3f0aa807b886206f8f0f0f1/scripts/build/binary_hdd#L60-86
In what sense would you want to improve this? It's not very pretty that
binary_hdd has to know about each bootloader, but since bootloader
installation in most cases needs details about the image type and
bootloader, some crossover is going to be present anyway.

Or did you see something else to improve?

> All of this above is trying to improve multi bootloader support in the
> binary_hdd part of live-build. Not sure if you should deal with this
> prior to adding your code.
>
> But if you want to take a look and tell us if binary_hdd should be
> updated or not (in the end the efi installation is handled by
> binary_syslinux-efi or binary_grub-efi in the filesystem level and not
> by binary_hdd).
I do not think any updates to binary_hdd are needed for efi support (and
any improvements I can see to binary_hdd are mostly unrelated to efi
support as well). Would that answer your question?

> 6.3) Many of your commits seem to need a rebase into the current master
> branch. Well, that's to be expected.
Yeah, I usually rebase whenever I push an update.

> 9) Improve bootloader configuration checks (3e6645a2)
> Well, the way how defaults.sh checks if anything is valid is kind of
> antique.
>
> In my opinnion binary_syslinux, binary_syslinux-efi, binary_grub-pc and
> so on... should be able to be sourced and use sort of can_i_boot
> function where you input architectures, filesystem and if the bootloader
> is the first bootloader or not.
>
> Then the binary_syslinux (or whatever) replies.
That would certainly be better, but also out of scope for this PR (and
probably an intrusive change as well...).

I'll make the changes mentioned above later (no more time now), but feel
free to already respond to the above in the meanwhile :-).

Gr.

Matthijs

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Bug#922251: live-build: support syslinux-efi as (additional) bootloader

Matthijs Kooijman-3
Hi Adrian,

I just updated the MR with the changes I discussed in my previous mail
(and also listed below). My previous mail also contained some responses
and questions, if you have some time to respond to those that would be
great.

> > >>> 3) Remove ldlinux.c32 for extlinux and syslinux (6fc68c1d)
I moved this into its own MR:
https://salsa.debian.org/live-team/live-build/merge_requests/26

> > Well, I think you should use something else.
> > modules32 is 9 characters long (not 8.3 compatible). What about module32
> > and module64? So that we can reuse the code in the iso side when
> > isolinux is updated to support hybrid isos in efi mode ?
> Good point, hadn't considered 8.3 compatibility. Singular "module32"
> sounds a bit weird to me, but it is probably clearer than something like
> "mods32" or even just "c32" (the latter seems reasonable, except that
> the "c64" directory would then still contain ".c32" files since that's
> what 64-bit syslinux-efi also uses...).
I changed to module32 and module64 now (also using module32 for the bios
modules, for consistency).

> > 6.1)
> > PATH syslinux command is still being written in capital letters in
> > share/bootloaders/syslinux-efi/syslia32.cfg and
> > share/bootloaders/syslinux-efi/syslx64.cfg while it should be written in
> > non-capital letters.
> Good catch, will fix that.
Fixed.

Gr.

Matthijs

signature.asc (849 bytes) Download Attachment
12