[PATCH 5/6] efidebug: add multiple device path instances on Boot####

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

On Fri, Mar 12, 2021 at 07:37:55AM +0200, Ilias Apalodimas wrote:
> Akashi-san,
> > On Fri, Mar 12, 2021 at 02:23:13PM +0900, AKASHI Takahiro wrote:
> > On Fri, Mar 12, 2021 at 06:55:57AM +0200, Ilias Apalodimas wrote:
> > > Akashi-san
> > > 
> > > > >  
> > > [...]
> > > > > +/**
> > > > > + * add_initrd_instance() - Append a device path to load_options pointing to an
> > > > > + *			   inirtd
> > > > > +	if (!initrd_dp) {
> > > > > +		printf("Cannot append media vendor device path path\n");
> > > > > +		goto out;
> > > > > +	}
> > > > > +	final_fp = efi_dp_concat(fp, initrd_dp);
> > > > > +	*fp_size = efi_dp_size(fp) + efi_dp_size(initrd_dp) +
> > > > > +		(2 * sizeof(struct efi_device_path));
> > > > > +
> > > > > +out:
> > > > > +	efi_free_pool(initrd_dp);
> > > > > +	efi_free_pool(tmp_dp);
> > > > > +	efi_free_pool(tmp_fp);
> > > > > +	return final_fp ? final_fp : ERR_PTR(-EINVAL);
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * do_efi_boot_add() - set UEFI load option
> > > > >   *
> > > > > @@ -806,7 +868,9 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,
> > > > >   *
> > > > >   * Implement efidebug "boot add" sub-command. Create or change UEFI load option.
> > > > >   *
> > > > > - *     efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
> > > > > + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] <file>
> > > > > + *                   -i <file> <interface2> <devnum2>[:<part>] <initrd>
> > > > > + *                   -s '<options>'
> > > > 
> > > > We discussed another syntax:
> > > > efidebug boot add <id> ...
> > > > efidebug boot add-initrd <id> <initrd path>
> > > > (Please don't care detailed syntax for now.)
> > > 
> > > Yep and I completely agree this is a better format but ...
> > > 
> > > > 
> > > > What is the difficulty that you have had to implement this type of
> > > > interface?
> > > 
> > > The problem is that the initrd and kernel eventually go into *one* Boot####
> > > variable.  So it's much easier to treat them in a single command as a bundle.
> > > 
> > > Otherwise you'll have to start adding checks on the initrd code to make
> > > sure the Boot### variable exists before you append an initrd.
> > > Then you have to deserialize the existing stored device path in Boot####,
> > > append the initrd and serialize it again (and last time I checked this is not
> > > as easy as it sounds).
> > 
> > If we take the format like:
> >   kernel path,end(0xff),
> >   VenMedia(INITRD),initrd1 path,end(0xff),
> >   VenMedia(INITRD),initrd2 path,end(0xff),
> > 
> > all that we have to do is
> > - deserialize the boot option variable,
> > - append initrd device path to a list (after kernel device path),
> > - serialize all the option together,
> 
> Sure but the serialize/deserialize is not as easy as it sounds and requires
> code as well for the optional data etc.

I don't still understand why it's not so easy.
Because I'm the author of that function?

> Again I am not saying it's not doable, or even more elegant.  I am saying the
> extra code doesn't seemt to worth the effort right now. Especially since we
> support a *single* initrd on the loading anyway and no dtbs.

Better user interface would pay the efforts of implementation.

> > 
> > Appending is quite simple, isn't it?
> > (because we will not have to parse a device path list.)
> > 
> 
> Yes and i've mentioned most of this on the mailing list. 
> We did choose the other format though...

Simply, who objected it?
I remember that Heinrich has a similar idea.
So who else?

> > > Also what happens if you edit a Boot#### option and that option has an initrd? 
> > If I were you,
> > 
> > > You have to pick up the existing initrd and move it over? Or do we silently 
> > > delete it?
> > > Something like:
> > > efidebug boot add 0001 
> > > efidebug boot add-initrd 0001 
> > > efidebug boot add 0001
> > 
> > even with the current implementation,
> > > efidebug boot add 0001 
> > > efidebug boot add 0001 
> > those sequence will overwrite the existing variable and,
> > deeleting initrd by the second "efidebug boot add" would make sense.
> 
> Yea but that feels 'natural' because it's a single command. Which is also the
> case with the current code. In multiple commands you wouldn't expect the
> initrd to away, unless you knew it was stored in a Boot option.

Well, if it's your concern, we can print a warning, say
  You're trying to rewrite BootXXXX variable (you will lost the existing data).
  Are you sure [y/n]?

But I don't think it's even worth doing so
because you can simply type "Ctrl-p" twice at the command line
if you want to run "efidebug boot add-initrd ..." again :)

-Takahiro Akashi


> 
> Thanks
> /Ilias