[U-Boot] [PATCH 1/1] efi_loader: pass address to efi_install_fdt()

[Heinrich Schuchardt]

On 12/3/19 7:37 AM, Alexander Graf wrote:
>
> On 03.12.19 08:27, Heinrich Schuchardt wrote:
>> As part of moving the parsing of command line arguments to do_bootefi()
>> call efi_install_fdt() with the address of the device tree instead of a
>> string.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>   cmd/bootefi.c | 30 ++++++++++++++++--------------
>>   1 file changed, 16 insertions(+), 14 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index f613cce7e2..3cf190889e 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -196,40 +196,37 @@ static void *get_config_table(const efi_guid_t
>> *guid)
>>   #endif /* !CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE) */
>>
>>   /**
>> - * efi_install_fdt() - install fdt passed by a command argument
>> + * efi_install_fdt() - install device tree
>>    *
>> - * If fdt_opt is available, the device tree located at that memory
>> address will
>> + * If fdt_addr is available, the device tree located at that memory
>> address will
>>    * will be installed as configuration table, otherwise the device
>> tree located
>>    * at the address indicated by environment variable fdtcontroladdr
>> will be used.
>>    *
>> - * On architectures (x86) using ACPI tables device trees shall not be
>> installed
>> - * as configuration table.
>> + * On architectures using ACPI tables device trees shall not be
>> installed as
>> + * configuration table.
>>    *
>> - * @fdt_opt:    pointer to argument
>> + * @fdt_addr:    address of device tree
>>    * Return:    status code
>>    */
>> -static efi_status_t efi_install_fdt(const char *fdt_opt)
>> +static efi_status_t efi_install_fdt(uintptr_t fdt_addr)
>>   {
>>       /*
>>        * The EBBR spec requires that we have either an FDT or an ACPI
>> table
>>        * but not both.
>>        */
>>   #if CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)
>> -    if (fdt_opt) {
>> +    if (fdt_addr) {
>
>
> Why check for fdt_addr != 0 here? Since you do the parsing outside of
> this function now, just make 0 a valid pointer and check for the
> validity outside of this function.

fdt_addr == 0 signals that U-Boot's internal device tree shall be used
for the UEFI sub-system.

Your suggested change would drop the capability to use the internal
device tree. Why would you want to do so?

---

The context is Christian's patch set

Add support for booting EFI FIT images
https://lists.denx.de/pipermail/u-boot/2019-November/391516.html

In his patch

https://lists.denx.de/pipermail/u-boot/2019-November/391518.html
bootm: Add a bootm command for type IH_OS_EFI

he converts addresses to strings and calls do_bootefi() which converts
these strings back to addresses. I would prefer to pass addresses directly.

Best regards

Heinrich

>
>
>>           printf("ERROR: can't have ACPI table and device tree.\n");
>>           return EFI_LOAD_ERROR;
>>       }
>>   #else
>> -    unsigned long fdt_addr;
>>       void *fdt;
>>       bootm_headers_t img = { 0 };
>>       efi_status_t ret;
>>
>> -    if (fdt_opt) {
>> -        fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
>> -        if (!fdt_addr)
>> -            return EFI_INVALID_PARAMETER;
>> -    } else {
>> +    if (!fdt_addr) {
>> +        const char* fdt_opt;
>> +
>>           /* Look for device tree that is already installed */
>>           if (get_config_table(&efi_guid_fdt))
>>               return EFI_SUCCESS;
>> @@ -556,6 +553,7 @@ static int do_efi_selftest(void)
>>   static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char *
>> const argv[])
>>   {
>>       efi_status_t ret;
>> +    uintptr_t fdt_addr;
>>
>>       if (argc < 2)
>>           return CMD_RET_USAGE;
>> @@ -568,7 +566,11 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag,
>> int argc, char * const argv[])
>>           return CMD_RET_FAILURE;
>>       }
>>
>> -    ret = efi_install_fdt(argc > 2 ? argv[2] : NULL);
>> +    if (argc > 2)
>> +        fdt_addr = simple_strtoul(argv[2], NULL, 16);
>> +    else
>> +        fdt_addr = 0UL;
>> +    ret = efi_install_fdt(fdt_addr);
>
>
> So here:
>
>
> if (fdt_addr)
>      efi_install_fdt(fdt_addr);
>
> And then you can remove all of the conditional code (and indentation) in
> efi_install_fdt() above.
>
>
> Alex
>
>
>