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
