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