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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Thu, Jan 14, 2021 at 02:36:23PM +0200, Ilias Apalodimas wrote:
> 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

I have had the same idea in my mind.

> > 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

My long-standing hope was to step up the stage of "efidebug" from
"debug" to "normal" command. Initially, I intended to implement this
command as an alternative of EDK2's efishell (or poorman's shell),
so most of the sub commands have a similar syntax and output formats
to efisehell defined in EFI specification.
But this idea was rejected by Alex :)

I agree that "boot" sub command is a good candidate of a standalone command.
Or alternatively, we may want to add options to bootefi command.

> +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?

Anyhow, when you go this way, please don't forget to update pytest scripts
since efidebug is also used in secure boot/capsule update tests.

-Takahiro Akashi


> 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
> > >
> >