Re: [PATCH v2 02/12] cmd: bootefi: re-organize do_bootefi()

Heinrich Schuchardt
2023-12-17T12:03:54Z

On 12/17/23 13:00, Heinrich Schuchardt wrote:
> On 11/21/23 02:29, AKASHI Takahiro wrote:
>> Replicate some code and re-organize do_bootefi() into three cases, which
>> will be carved out as independent functions in the next two commits.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>   cmd/Kconfig          | 15 ++++++--
>>   cmd/bootefi.c        | 82 ++++++++++++++++++++++++++++++--------------
>>   include/efi_loader.h |  2 --
>>   3 files changed, 69 insertions(+), 30 deletions(-)
>>
>> diff --git a/cmd/Kconfig b/cmd/Kconfig
>> index 6f636155e5b6..4cf9a210c4a1 100644
>> --- a/cmd/Kconfig
>> +++ b/cmd/Kconfig
>> @@ -362,9 +362,19 @@ config CMD_BOOTEFI
>>       help
>>         Boot an EFI image from memory.
>>
>> +if CMD_BOOTEFI
>> +config CMD_BOOTEFI_BINARY
>> +    bool "Allow booting an EFI binary directly"
>> +    depends on BOOTEFI_BOOTMGR
>> +    default y
>> +    help
>> +      Select this option to enable direct execution of binary at
>> 'bootefi'.
>> +      This subcommand will allow you to load the UEFI binary using
>> +      other U-Boot commands or external methods and then run it.
>> +
>>   config CMD_BOOTEFI_BOOTMGR
>>       bool "UEFI Boot Manager command"
>> -    depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
>> +    depends on BOOTEFI_BOOTMGR
>>       default y
>>       help
>>         Select this option to enable the 'bootmgr' subcommand of
>> 'bootefi'.
>> @@ -373,7 +383,7 @@ config CMD_BOOTEFI_BOOTMGR
>>
>>   config CMD_BOOTEFI_HELLO_COMPILE
>>       bool "Compile a standard EFI hello world binary for testing"
>> -    depends on CMD_BOOTEFI && !CPU_V7M
>> +    depends on !CPU_V7M
>
> CMD_BOOTEFI_HELLO_COMPILE makes no sense without a command to run the
> hello command. The CPU_V7M dependency is not needed as described in

Missed that you pulled that CMD_BOOTEFI into the if above.

>
> [PATCH 1/1] cmd: CONFIG_CMD_BOOTEFI_HELLO_COMPILE dependencies
> https://lore.kernel.org/u-boot/20231217114457.70128-1-heinrich.schuchardt@canonical.com/T/#u
>
> Best regards
>
> Heinrich
>
>>       default y
>>       help
>>         This compiles a standard EFI hello world application with
>> U-Boot so
>> @@ -395,6 +405,7 @@ config CMD_BOOTEFI_HELLO
>>         up EFI support on a new architecture.
>>
>>   source lib/efi_selftest/Kconfig
>> +endif
>>
>>   config CMD_BOOTMENU
>>       bool "bootmenu"
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 190ccba260e0..e9e5ab67a1f5 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -503,7 +503,6 @@ out:
>>       return (ret != EFI_SUCCESS) ? ret : ret2;
>>   }
>>
>> -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>   static efi_status_t bootefi_run_prepare(const char *load_options_path,
>>           struct efi_device_path *device_path,
>>           struct efi_device_path *image_path,
>> @@ -593,7 +592,6 @@ static int do_efi_selftest(void)
>>
>>       return ret != EFI_SUCCESS;
>>   }
>> -#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>>
>>   /**
>>    * do_bootefi() - execute `bootefi` command
>> @@ -615,14 +613,6 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>>       if (argc < 2)
>>           return CMD_RET_USAGE;
>>
>> -    /* Initialize EFI drivers */
>> -    ret = efi_init_obj_list();
>> -    if (ret != EFI_SUCCESS) {
>> -        log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
>> -            ret & ~EFI_ERROR_MASK);
>> -        return CMD_RET_FAILURE;
>> -    }
>> -
>>       if (argc > 2) {
>>           uintptr_t fdt_addr;
>>
>> @@ -631,29 +621,54 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>>       } else {
>>           fdt = EFI_FDT_USE_INTERNAL;
>>       }
>> -    ret = efi_install_fdt(fdt);
>> -    if (ret == EFI_INVALID_PARAMETER)
>> -        return CMD_RET_USAGE;
>> -    else if (ret != EFI_SUCCESS)
>> -        return CMD_RET_FAILURE;
>>
>> -    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
>> -        if (!strcmp(argv[1], "bootmgr"))
>> -            return do_efibootmgr();
>> +    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) &&
>> +        !strcmp(argv[1], "bootmgr")) {
>> +        /* Initialize EFI drivers */
>> +        ret = efi_init_obj_list();
>> +        if (ret != EFI_SUCCESS) {
>> +            log_err("Error: Cannot initialize UEFI sub-system, r =
>> %lu\n",
>> +                ret & ~EFI_ERROR_MASK);
>> +            return CMD_RET_FAILURE;
>> +        }
>> +
>> +        ret = efi_install_fdt(fdt);
>> +        if (ret == EFI_INVALID_PARAMETER)
>> +            return CMD_RET_USAGE;
>> +        else if (ret != EFI_SUCCESS)
>> +            return CMD_RET_FAILURE;
>> +
>> +        return do_efibootmgr();
>>       }
>> -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> -    if (!strcmp(argv[1], "selftest"))
>> +
>> +    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_SELFTEST) &&
>> +        !strcmp(argv[1], "selftest")) {
>> +        /* Initialize EFI drivers */
>> +        ret = efi_init_obj_list();
>> +        if (ret != EFI_SUCCESS) {
>> +            log_err("Error: Cannot initialize UEFI sub-system, r =
>> %lu\n",
>> +                ret & ~EFI_ERROR_MASK);
>> +            return CMD_RET_FAILURE;
>> +        }
>> +
>> +        ret = efi_install_fdt(fdt);
>> +        if (ret == EFI_INVALID_PARAMETER)
>> +            return CMD_RET_USAGE;
>> +        else if (ret != EFI_SUCCESS)
>> +            return CMD_RET_FAILURE;
>> +
>>           return do_efi_selftest();
>> -#endif
>> +    }
>>
>> -#ifdef CONFIG_CMD_BOOTEFI_HELLO
>> -    if (!strcmp(argv[1], "hello")) {
>> +    if (!IS_ENABLED(CONFIG_CMD_BOOTEFI_BINARY))
>> +        return CMD_RET_SUCCESS;
>> +
>> +    if (IS_ENABLED(CONFIG_CMD_BOOTEFI_HELLO) &&
>> +        !strcmp(argv[1], "hello")) {
>>           image_buf = __efi_helloworld_begin;
>>           size = __efi_helloworld_end - __efi_helloworld_begin;
>>           efi_clear_bootdev();
>> -    } else
>> -#endif
>> -    {
>> +    } else {
>>           addr = strtoul(argv[1], NULL, 16);
>>           /* Check that a numeric value was passed */
>>           if (!addr)
>> @@ -675,6 +690,21 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>>               size = image_size;
>>           }
>>       }
>> +
>> +    /* Initialize EFI drivers */
>> +    ret = efi_init_obj_list();
>> +    if (ret != EFI_SUCCESS) {
>> +        log_err("Error: Cannot initialize UEFI sub-system, r = %lu\n",
>> +            ret & ~EFI_ERROR_MASK);
>> +        return CMD_RET_FAILURE;
>> +    }
>> +
>> +    ret = efi_install_fdt(fdt);
>> +    if (ret == EFI_INVALID_PARAMETER)
>> +        return CMD_RET_USAGE;
>> +    else if (ret != EFI_SUCCESS)
>> +        return CMD_RET_FAILURE;
>> +
>>       ret = efi_run_image(image_buf, size);
>>
>>       if (ret != EFI_SUCCESS)
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 664dae28f882..44436d346286 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -879,14 +879,12 @@ efi_status_t __efi_runtime EFIAPI efi_get_time(
>>CMD_BOOTEFI
>>   efi_status_t __efi_runtime EFIAPI efi_set_time(struct efi_time *time);
>>
>> -#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>   /*
>>    * Entry point for the tests of the EFI API.
>>    * It is called by 'bootefi selftest'
>>    */
>>   efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle,
>>                    struct efi_system_table *systab);
>> -#endif
>>
>>   efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
>>                        const efi_guid_t *vendor, u32 *attributes,
>