Heinrich,
On Fri, Mar 01, 2019 at 10:22:13AM +0900, AKASHI Takahiro wrote:
> On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
> > On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
> > > On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
> > >> On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> > >>> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
> > >>>> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> > >>>>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> > >>>>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> > >>>>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> > >>>>>>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> > >>>>>>>>> "run -e" allows for executing EFI application with a specific
> > >>>>>>>>> "BootXXXX"
> > >>>>>>>>> variable. If no "BootXXXX" is specified or "BootOrder" is
> > >>>>>>>>> specified,
> > >>>>>>>>> it tries to run an EFI application specified in the order of
> > >>>>>>>>> "bootOrder."
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>> If we cannot specify the device tree what would be the use of this
> > >>>>>>>> for
> > >>>>>>>> ARM processors?
> > >>>>>>>
> > >>>>>>> I don't get your point. What's the matter with device tree?
> > >>>>>>
> > >>>>>> To boot an ARM board on Linux or BSD you need a device tree.
> > >>>>>
> > >>>>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> > >>>>> he suggested that we should not allow users to specify fdt at a
> > >>>>> command line.
> > >>>>> I believe that it would apply to my case above.
> > >>>>>
> > >>>>> IMO, we should always provide system's fdt to EFI applications.
> > >>>>
> > >>>> With current Linux development practice this unfortunately does not
> > >>>> work. Linux device trees sometimes see incompatible changes between
> > >>>> versions. Booting may fail with a device tree that is either too old or
> > >>>> too new for your Linux version.
> > >>>>
> > >>>> E.g. for the Odroid C2 some reserved memory regions were removed from
> > >>>> the device tree and replaced by a logic that determines them on the fly
> > >>>> due to changes in ARM trusted firmware location.
> > >>>>
> > >>>> The Wandboard rev B1 device tree was moved to a new file when a new
> > >>>> board revision appeared and the new revision changed the old file
> > >>>> (sic).
> > >>>>
> > >>>> U-Boot is also not perfect at keeping its device tree in sync with the
> > >>>> newest Linux device tree.
> > >>>
> > >>> Why don't you use "fdt" command in that case?
> > >>> IMO, we don't need <fdt> argument at bootefi (and run -e).
> > >>> Obviously, I have one assumption that we need change the code
> > >>> to utilize "fdtaddr" variable in do_bootefi().
> > >>
> > >> Such a change would mean that after an upgrade of U-Boot all boards
> > >> running on Suse and Fedora suddenly will not boot again.
> > >
> > > Why do you think so?
> > > Unless people intentionally run "fdt" command before bootefi,
> > > the system will behave in the exact same way.
> > >
> > > How many people really expect that, in the case below,
> > > => load ... <addr> <file>
> > > => fdt addr <addr>
> > > => bootefi bootmgr
> > > bootefi will start EFI application *without* fdt?
> > >
> > > -Takahiro Akashi
> >
> > Your previous mail sounded to me as if you wanted to drop the
> > possibility to specify an FDT address in the bootefi command. But maybe
> > I got you wrong.
>
> No, even in v2, I didn't change any semantics in do_bootefi()
> and hence bootefi command.
>
> > If your idea is that we should use the address specified in command fdt
> > and $fdtcontroladdr as fallbacks if no FDT address is specified, that is
> > another story.
>
> Exactly for "run -e" case (and probably a new command, efibootmgr, too).
>
> The only concern is an incompatibility with distro_bootcmd.
> It has yet another logic regarding fdt.
I changed my mind a bit.
I dropped "run -e" patch, and posted "BootNext" patch on its own.
I'd like to focus more on re-working do_bootefi().
See my next RFC.
Thanks,
-Takahiro Akashi
> -Takahiro Akashi
>
> > Best regards
> >
> > Heinrich
> >
> > >
> > >> We should not change existing commands in an incompatible way.
> > >>
> > >>>
> > >>> Under the current implementation, a similar behavior is achieved
> > >>> only via distro_bootcmd. IMO, this should be corrected.
> > >>> If you agree, I will add another patch to my current patchset
> > >>> for this purpose.
> > >>
> > >> I suggest to drop patch 5/5 from your series.
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>>
> > >>>>>
> > >>>>>> So run -e Boot0001 will not allow you to boot into Linux because it
> > >>>>>> does
> > >>>>>> not specify a device tree.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> Why do you add the option to run and not to bootefi?
> > >>>>>>>>
> > >>>>>>>> You already introduced the capability to set BootNext. Why isn't
> > >>>>>>>> that
> > >>>>>>>> enough?
> > >>>>>>>
> > >>>>>>> Simple.
> > >>>>>>>
> > >>>>>>> => run -e Boot00>
> > >>>>>>> versus
> > >>>>>>>
> > >>>>>>> => efidebug boot next 1
> > >>>>>>> => bootefi bootmgr
> > >>>>>>
> > >>>>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r
> > >>>>>> 0001'
> > >>>>>>
> > >>>>>> So there is no need to go through efidebug.
> > >>>>>>
> > >>>>>> I think we should avoid alternative commands.
> > >>>>>
> > >>>>> As I said, I already removed this feature from bootefi.
> > >>>>>
> > >>>>>>>
> > >>>>>>> First of all, efidebug is only recognized as a *debugging* tool.
> > >>>>>>> I believe that the former syntax is more intuitive, and it looks
> > >>>>>>> a natural extension to "run" command semantics akin to "env -e".
> > >>>>>>>
> > >>>>>>> As a result, we don't have to know about bootefi for normal
> > >>>>>>> operations.
> > >>>>>>
> > >>>>>> But you have to know about 'run' which you might not need otherwise.
> > >>>>>
> > >>>>> "run" is much better known to U-Boot users than bootefi.
> > >>>>
> > >>>> Do you have a statistic ;)
> > >>>>
> > >>>> Up to now booting always required a command starting on boot...
> > >>>
> > >>> What I meant is that people need not learn more commands.
> > >>>
> > >>> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> > >>> # bootefi and making it as a single command. In that case,
> > >>> # bootefi does exist only for hello and selftest.
> > >>>
> > >>> -Takahiro Akashi
> > >>>
> > >>>>
> > >>>> Best regards
> > >>>>
> > >>>> Heinrich
> > >>>>
> > >>>>>
> > >>>>> Thanks,
> > >>>>> -Takahiro Akashi
> > >>>>>
> > >>>>>> Best regards
> > >>>>>>
> > >>>>>> Heinrich
> > >>>>>>
> > >>>>>>>
> > >>>>>>> Thanks,
> > >>>>>>> -Takahiro Akashi
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> Best regards
> > >>>>>>>>
> > >>>>>>>> Heinrich
> > >>>>>>>>
> > >>>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> > >>>>>>>>> ---
> > >>>>>>>>> cmd/bootefi.c | 31 +++++++++++++++++++++++++++++++
> > >>>>>>>>> cmd/nvedit.c | 9 ++++++++-
> > >>>>>>>>> common/cli.c | 10 ++++++++++
> > >>>>>>>>> include/command.h | 3 +++
> > >>>>>>>>> 4 files changed, 52 insertions(+), 1 deletion(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > >>>>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
> > >>>>>>>>> --- a/cmd/bootefi.c
> > >>>>>>>>> +++ b/cmd/bootefi.c
> > >>>>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int
> > >>>>>>>>> boot_id)
> > >>>>>>>>> return CMD_RET_SUCCESS;
> > >>>>>>>>> }
> > >>>>>>>>>
> > >>>>>>>>> +/* Called by "run" command */
> > >>>>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char *
> > >>>>>>>>> const argv[])
> > >>>>>>>>> +{
> > >>>>>>>>> + int boot_id = -1;
> > >>>>>>>>> + char *endp;
> > >>>>>>>>> +
> > >>>>>>>>> + if (argc > 2)
> > >>>>>>>>> + return CMD_RET_USAGE;
> > >>>>>>>>> +
> > >>>>>>>>> + if (argc == 2) {
> > >>>>>>>>> + if (!strcmp(argv[1], "BootOrder")) {
> > >>>>>>>>> + boot_id = -1;
> > >>>>>>>>> + } else if (!strncmp(argv[2], "Boot", 4)) {
> > >>>>>>>>> + boot_id = (int)simple_strtoul(&argv[2][4],
> > >>>>>>>>> &endp, 0);
> > >>>>>>>>> + if ((argv[2] + strlen(argv[2]) != endp) ||
> > >>>>>>>>> + boot_id > 0xffff)
> > >>>>>>>>> + return CMD_RET_USAGE;
> > >>>>>>>>> + } else {
> > >>>>>>>>> + return CMD_RET_USAGE;
> > >>>>>>>>> + }
> > >>>>>>>>> + }
> > >>>>>>>>> +
> > >>>>>>>>> + if (efi_init_obj_list())
> > >>>>>>>>> + return CMD_RET_FAILURE;
> > >>>>>>>>> +
> > >>>>>>>>> + if (efi_handle_fdt(NULL))
> > >>>>>>>>> + return CMD_RET_FAILURE;
> > >>>>>>>>> +
> > >>>>>>>>> + return do_bootefi_bootmgr_exec(boot_id);
> > >>>>>>>>> +}
> > >>>>>>>>> +
> > >>>>>>>>> /* Interpreter command to boot an arbitrary EFI image from
> > >>>>>>>>> memory */
> > >>>>>>>>> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char
> > >>>>>>>>> * const argv[])
> > >>>>>>>>> {
> > >>>>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > >>>>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
> > >>>>>>>>> --- a/cmd/nvedit.c
> > >>>>>>>>> +++ b/cmd/nvedit.c
> > >>>>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> > >>>>>>>>> run, CONFIG_SYS_MAXARGS, 1, do_run,
> > >>>>>>>>> "run commands in an environment variable",
> > >>>>>>>>> "var [...]\n"
> > >>>>>>>>> - " - run the commands in the environment variable(s) 'var'",
> > >>>>>>>>> + " - run the commands in the environment variable(s) 'var'"
> > >>>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> > >>>>>>>>> + "\n"
> > >>>>>>>>> + "run -e [BootXXXX]\n"
> > >>>>>>>>> + " - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > >>>>>>>>> +#else
> > >>>>>>>>> + ,
> > >>>>>>>>> +#endif
> > >>>>>>>>> var_complete
> > >>>>>>>>> );
> > >>>>>>>>> #endif
> > >>>>>>>>> diff --git a/common/cli.c b/common/cli.c
> > >>>>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
> > >>>>>>>>> --- a/common/cli.c
> > >>>>>>>>> +++ b/common/cli.c
> > >>>>>>>>> @@ -12,6 +12,7 @@
> > >>>>>>>>> #include <cli.h>
> > >>>>>>>>> #include <cli_hush.h>
> > >>>>>>>>> #include <console.h>
> > >>>>>>>>> +#include <efi_loader.h>
> > >>>>>>>>> #include <fdtdec.h>
> > >>>>>>>>> #include <malloc.h>
> > >>>>>>>>>
> > >>>>>>>>> @@ -125,6 +126,15 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int
> > >>>>>>>>> argc, char * const argv[])
> > >>>>>>>>> if (argc < 2)
> > >>>>>>>>> return CMD_RET_USAGE;
> > >>>>>>>>>
> > >>>>>>>>> +#ifdef CONFIG_CMD_BOOTEFI
> > >>>>>>>>> + if (!strcmp(argv[1], "-e")) {
> > >>>>>>>>> + argc--;
> > >>>>>>>>> + argv++;
> > >>>>>>>>> +
> > >>>>>>>>> + return do_bootefi_run(cmdtp, flag, argc, argv);
> > >>>>>>>>> + }
> > >>>>>>>>> +#endif
> > >>>>>>>>> +
> > >>>>>>>>> for (i = 1; i < argc; ++i) {
> > >>>>>>>>> char *arg;
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/include/command.h b/include/command.h
> > >>>>>>>>> index 200c7a5e9f4e..feddef300ccc 100644
> > >>>>>>>>> --- a/include/command.h
> > >>>>>>>>> +++ b/include/command.h
> > >>>>>>>>> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s cmd_tbl_t;
> > >>>>>>>>> #if defined(CONFIG_CMD_RUN)
> > >>>>>>>>> extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char *
> > >>>>>>>>> const argv[]);
> > >>>>>>>>> #endif
> > >>>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> > >>>>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc,
> > >>>>>>>>> char * const argv[]);
> > >>>>>>>>> +#endif
> > >>>>>>>>>
> > >>>>>>>>> /* common/command.c */
> > >>>>>>>>> int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t *
> > >>>>>>>>> cmdtp, int
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > >
> >
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
