From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Date: Fri, 12 Mar 2021 07:37:55 +0200
Subject: [PATCH 5/6] efidebug: add multiple device path instances on
 Boot####
In-Reply-To: <20210312052313.GE15112@laputa>
References: <20210305222303.2866284-1-ilias.apalodimas@linaro.org>
 <20210305222303.2866284-5-ilias.apalodimas@linaro.org>
 <20210312044456.GC15112@laputa> <YEr0XcpqVD0NyZXm@apalos.home>
 <20210312052313.GE15112@laputa>
Message-ID: <YEr+M0SfK3oQCo46@apalos.home>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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

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

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


Thanks
/Ilias