[RFC PATCH 3/3] efidebug: add multiple device path instances on Boot####

[Ilias Apalodimas]

On Thu, Jan 14, 2021 at 01:07:42PM +0100, Heinrich Schuchardt wrote:
> On 14.01.21 10:55, Ilias Apalodimas wrote:
> > On Thu, Jan 14, 2021 at 02:11:33PM +0900, AKASHI Takahiro wrote:
> >> Ilias,
> >>
> >> On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:
> >>> The UEFI spec allow a packed array of UEFI device paths in the
> >>> FilePathList[] of an EFI_LOAD_OPTION. The first file path must
> >>> describe the laoded image but the rest are OS specific.
> >>> Previous patches parse the device path and try to use the second
> >>> member of the array as an initrd. So let's modify efidebug slightly
> >>> and install the second file described in the command line as the
> >>> initrd device path.
> >>
> >> I have a concern about your proposed command line syntax.
> >> It takes a lot of parameters as a whole which makes it
> >> hard to understand it at a glance, easily overflowing
> >> the width of terminal window.
> >>
> >> It will even get worse if we want to take dtb as a third
> >> device path, and what if we want to specify dtb, but not initrd?
> >>
> >> Moreover, if we want to add support for non-linux executabes which
> >> utilize extra device paths (neither initrd nor dtb) in a boot
> >> load option, the syntax will be problematic.
> >>
> >
> > Maybe we should add explicit commands in efidebug then?
> > Something like:
> > efidebug initrd add 0002 virtio 1 initrd_file
> > efidebug dtb add 0002 virtio 1 dtb
> 
> Why "add"? If no file is provided, we could empty the device path.
> 
> All other boot related subcommands start with efidebug boot. So how about:
> 
> efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> efidebug boot initrd 00B1 mmc 0:1 initrd-5.99
> efidebug boot dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> efidebug boot order 00B1
> 
> What will happen if you pass the following command next:
> 
> efidebug boot add 00B1 'Linux' mmc 0:1 vmlinux-5.99.1 UUID=1234
> 
> Will initrd and and dtb be kept?

That's one of the reasons the RFC had everything in one line. You enforce less
options to the user, which imho is always good :).

> 
> What will happen if you start with dtb or with initrd and not with add?

IMHO we should return the usage in that case and explicitly request the boot
option to be set first. In principle the dtb and initrd are not load options. 
They are appended DTB instances in an existing load option. If you dont do 
'add' first, there's no device path to begin with.

So to answer your first question, in order to simplify the command line, I'd
suggest we overwrite the load_option when adding a new image. It's unlikely
the initrd will remain as is anyway, the dtb might remain the same, but it's
not worth the effort and complexity. You'll need a substantial amount of code 
parsing the DP instances and placing elements in top/middle/bottom etc.

> 
> A device path with kernel.efi, no initrd, and dtb would have an initrd
> device path with only an end node. Should we install a LOAD FILE2
> protocol in this case to disallow initrd on the command line?

If you do that the user won't be able to load an initrd, unless a fixup is
applied directly on the DTB. I'd avoid that, I would simply defer from
installing the protocol overall if an end node is discovered on the initrd.

> 
> The boot options are relevant for all users, while the rest of efidebug
> is more developers oriented. Should we separate the boot related
> commands from the rest of efidebug and build with the new command by
> default?
> 
> bootopt add 00B1 'Linux' mmc 0:1 vmlinux-5.99 UUID=1234
> bootopt initrd 00B1 mmc 0:1 initrd-5.99
> bootopt dtb mmc 00B1 0:1 /dtbs/5.99/vendor/board.dtb
> bootopt order 00B1

+1 on that, efidebug feels a bit weird for that.
This can go in on a different patchset I guess?
It would merely mean c/p the code in a different file and 
edit a few makesfiles?

Cheers
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> >
> > That would untangle the do_efi_boot_add() function, make our lives easier on
> > adding things like 'kernel <no initrd> valid dtb' and should be much easier
> > to use. The user will just have to make sure the boot order numbers match when
> > adding files
> >
> > [...]
> >
> > Cheers
> > /Ilias
> >
>