[U-Boot] [PATCH 1/1] efi_loader: pass address to efi_install_fdt()
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@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) { 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); if (ret == EFI_INVALID_PARAMETER) return CMD_RET_USAGE; else if (ret != EFI_SUCCESS) -- 2.24.0
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@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.
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
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@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
On 03.12.19 09:08, Heinrich Schuchardt wrote:
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@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?
Mostly because I didn't look at the bigger picture, sorry :). Looking at all the options, your patch is probably the best solution. Sorry for the fuss.
Reviewed-by: Alexander Graf agraf@csgraf.de
Alex
On 03.12.19 11:26, Alexander Graf wrote:
On 03.12.19 09:08, Heinrich Schuchardt wrote:
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@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?
Mostly because I didn't look at the bigger picture, sorry :). Looking at all the options, your patch is probably the best solution. Sorry for the fuss.
Actually, thinking about it again, can we make the "Please fall back to internal device tree" token explicit instead of the implicit 0? It might be enough to just do something as simple as
#define EFI_FDT_USE_INTERNAL 0
and then use that in the if(!fdt_addr) line and in do_bootefi() instead. Then document it in the function header. That way we can make it an actual supported API that FIT image support can rely on.
Alex
Alex
On 12/3/19 10:31 AM, Alexander Graf wrote:
On 03.12.19 11:26, Alexander Graf wrote:
On 03.12.19 09:08, Heinrich Schuchardt wrote:
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@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?
Mostly because I didn't look at the bigger picture, sorry :). Looking at all the options, your patch is probably the best solution. Sorry for the fuss.
Actually, thinking about it again, can we make the "Please fall back to internal device tree" token explicit instead of the implicit 0? It might be enough to just do something as simple as
#define EFI_FDT_USE_INTERNAL 0
and then use that in the if(!fdt_addr) line and in do_bootefi() instead. Then document it in the function header. That way we can make it an actual supported API that FIT image support can rely on.
Hello Alex,
some boards have CONFIG_OF_BOARD=y (device tree provided by board at runtime) like qemu_arm64_defconfig and rpi_4_defconfig. I guess it would be preferable to use the device tree pointed to by 'fdt_addr' in this case instead of using the internal device tree indicated by 'fdtcontroladdr'.
According to doc/README.distro the mere existence of environment variable fdt_addr indicates that a hardware supplied device tree is available independent of CONFIG_OF_BOARD=y.
But some boards play it differently, e.g. include/configs/odroid.h. Here fdt_addr is set after loading a device tree successfully.
After all I think if no device tree address is passed to do_bootefi() and ACPI is not enabled we should first check if fdt_addr is defined and only if it is not defined fall back to fdtcontroladdr.
Best regards
Heinrich
participants (2)
-
Alexander Graf -
Heinrich Schuchardt