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