Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c
On Mon, Dec 25, 2023 at 10:17:06AM +0100, Heinrich Schuchardt wrote: > On 12/18/23 03:38, AKASHI Takahiro wrote: > > Some code moved from cmd/bootefi.c is actually necessary only for "bootefi > > " command (starting an image manually loaded by a user using U-Boot > > load commands or other methods (like JTAG debugger). > > > > The code will never been opted out as unused code by a compiler which > > doesn't know how EFI boot manager is implemented. So introduce a new > > configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out > > explicitly. > > We build with -ffunction-sections. The linker removes unreferenced > functions. Yes, I know but I also think it would be better in terms of readability and maintainability to add a new config option and separate EFI_BINARY_EXEC portion from BOOTEFI_BOOTMGR as these two functions share almost nothing (except efi_install_fdt()). > > > > Signed-off-by: AKASHI Takahiro > > --- > > boot/Kconfig | 4 +- > > cmd/Kconfig | 6 +- > > include/efi_loader.h | 28 +- > > lib/efi_loader/Kconfig | 9 + > > lib/efi_loader/efi_bootmgr.c | 493 -- > > lib/efi_loader/efi_device_path.c | 3 +- > > lib/efi_loader/efi_helper.c | 499 ++- > > We expect that after each patch we can compile the code. This requires > that the Makefile change is in the same patch as the creation of > efi_helper.c. Please remember that efi_helper.c is not a new file. If you like, as Simon suggested, I will move "499" lines of code into a new file, efi_boot.c and then add: obj-$(CONFIG_EFI_BINARY_EXEC) := efi_boot.o > > 7 files changed, 529 insertions(+), 513 deletions(-) > > > > diff --git a/boot/Kconfig b/boot/Kconfig > > index 987ca7314117..8ab7e6f63d34 100644 > > --- a/boot/Kconfig > > +++ b/boot/Kconfig > > @@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE > > > > config BOOTMETH_EFILOADER > > bool "Bootdev support for EFI boot" > > - depends on BOOTEFI_BOOTMGR > > + depends on EFI_BINARY_EXEC > > Why do we need a symbol CONFIG_EFI_BINARY_EXEC? CONFIG_EFI_LOADER=y > without the ability to execute an EFI binary makes no sense to me. It's up to users. It allows them to configure U-Boot with EFI_LOADER and EFI_BOOTMGR only. Thanks, -Takahiro Akashi > Best regards > > Heinrich > > > default y > > help > > Enables support for EFI boot using bootdevs. This makes the > > @@ -558,7 +558,7 @@ config BOOTMETH_DISTRO > > select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts > > select BOOTMETH_EXTLINUX # E.g. Debian uses these > > select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH > > - select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this > > + select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this > > > > config SPL_BOOTMETH_VBE > > bool "Bootdev support for Verified Boot for Embedded (SPL)" > > diff --git a/cmd/Kconfig b/cmd/Kconfig > > index 24bfbe505722..2c993496b70e 100644 > > --- a/cmd/Kconfig > > +++ b/cmd/Kconfig > > @@ -273,7 +273,7 @@ config CMD_BOOTMETH > > > > config BOOTM_EFI > > bool "Support booting UEFI FIT images" > > - depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT > > + depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT > > default y > > help > > Support booting UEFI FIT images via the bootm command. > > @@ -365,7 +365,7 @@ config CMD_BOOTEFI > > if CMD_BOOTEFI > > config CMD_BOOTEFI_BINARY > > bool "Allow booting an EFI binary directly" > > - depends on BOOTEFI_BOOTMGR > > + depends on EFI_BINARY_EXEC > > default y > > help > > Select this option to enable direct execution of binary at 'bootefi'. > > @@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE > > > > config CMD_BOOTEFI_HELLO > > bool "Allow booting a standard EFI hello world for testing" > > - depends on CMD_BOOTEFI_HELLO_COMPILE > > + depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE > > default y if CMD_BOOTEFI_SELFTEST > > help > > This adds a standard EFI hello world application to U-Boot so that > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index 34e7fbbf1840..484c9fad239f 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 > > len); > >* back to u-boot world > >*/ > > void efi_restore_gd(void); > > -/* Call this to unset the current device name */ > > -void efi_clear_bootdev(void); > > -/* Call this to set the current device name */ > > -void efi_set_bootdev(const char *dev, const char *devnr, const char *path, > > -void *buffer, size_t buffer_size); > > + > > /* Called by networking code to memorize the dhcp ack package */ > > void efi_net_set_dhcp_ack(void *pkt, int len); > > /* Print information about all loaded images */ > > @@ -116,10 +
Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c
On 12/18/23 03:38, AKASHI Takahiro wrote:
Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
" command (starting an image manually loaded by a user using U-Boot
load commands or other methods (like JTAG debugger).
The code will never been opted out as unused code by a compiler which
doesn't know how EFI boot manager is implemented. So introduce a new
configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
explicitly.
We build with -ffunction-sections. The linker removes unreferenced
functions.
Signed-off-by: AKASHI Takahiro
---
boot/Kconfig | 4 +-
cmd/Kconfig | 6 +-
include/efi_loader.h | 28 +-
lib/efi_loader/Kconfig | 9 +
lib/efi_loader/efi_bootmgr.c | 493 --
lib/efi_loader/efi_device_path.c | 3 +-
lib/efi_loader/efi_helper.c | 499 ++-
We expect that after each patch we can compile the code. This requires
that the Makefile change is in the same patch as the creation of
efi_helper.c.
7 files changed, 529 insertions(+), 513 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig
index 987ca7314117..8ab7e6f63d34 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE
config BOOTMETH_EFILOADER
bool "Bootdev support for EFI boot"
- depends on BOOTEFI_BOOTMGR
+ depends on EFI_BINARY_EXEC
Why do we need a symbol CONFIG_EFI_BINARY_EXEC? CONFIG_EFI_LOADER=y
without the ability to execute an EFI binary makes no sense to me.
Best regards
Heinrich
default y
help
Enables support for EFI boot using bootdevs. This makes the
@@ -558,7 +558,7 @@ config BOOTMETH_DISTRO
select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
select BOOTMETH_EXTLINUX # E.g. Debian uses these
select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
- select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this
+ select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
config SPL_BOOTMETH_VBE
bool "Bootdev support for Verified Boot for Embedded (SPL)"
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 24bfbe505722..2c993496b70e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -273,7 +273,7 @@ config CMD_BOOTMETH
config BOOTM_EFI
bool "Support booting UEFI FIT images"
- depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT
+ depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT
default y
help
Support booting UEFI FIT images via the bootm command.
@@ -365,7 +365,7 @@ config CMD_BOOTEFI
if CMD_BOOTEFI
config CMD_BOOTEFI_BINARY
bool "Allow booting an EFI binary directly"
- depends on BOOTEFI_BOOTMGR
+ depends on EFI_BINARY_EXEC
default y
help
Select this option to enable direct execution of binary at 'bootefi'.
@@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE
config CMD_BOOTEFI_HELLO
bool "Allow booting a standard EFI hello world for testing"
- depends on CMD_BOOTEFI_HELLO_COMPILE
+ depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE
default y if CMD_BOOTEFI_SELFTEST
help
This adds a standard EFI hello world application to U-Boot so that
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 34e7fbbf1840..484c9fad239f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
* back to u-boot world
*/
void efi_restore_gd(void);
-/* Call this to unset the current device name */
-void efi_clear_bootdev(void);
-/* Call this to set the current device name */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
-void *buffer, size_t buffer_size);
+
/* Called by networking code to memorize the dhcp ack package */
void efi_net_set_dhcp_ack(void *pkt, int len);
/* Print information about all loaded images */
@@ -116,10 +112,6 @@ static inline efi_status_t efi_add_runtime_mmio(void
*mmio_ptr, u64 len)
/* No loader configured, stub out EFI_ENTRY */
static inline void efi_restore_gd(void) { }
-static inline void efi_clear_bootdev(void) { }
-static inline void efi_set_bootdev(const char *dev, const char *devnr,
- const char *path, void *buffer,
- size_t buffer_size) { }
static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
static inline void efi_print_image_infos(void *pc) { }
static inline efi_status_t efi_launch_capsules(void)
@@ -129,6 +121,20 @@ static inline efi_status_t efi_launch_capsules(void)
#endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
+#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
+/* Call this to unset the current device name */
+void efi_clear_bootdev(void);
+/* Call this to set the current device name */
+void efi_set_bootde
Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c
On Mon, Dec 18, 2023 at 08:01:51AM -0700, Simon Glass wrote: > Hi AKASHI, > > On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro > wrote: > > > > Some code moved from cmd/bootefi.c is actually necessary only for "bootefi > > " command (starting an image manually loaded by a user using U-Boot > > load commands or other methods (like JTAG debugger). > > > > The code will never been opted out as unused code by a compiler which > > doesn't know how EFI boot manager is implemented. So introduce a new > > configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out > > explicitly. > > > > Signed-off-by: AKASHI Takahiro > > --- > > boot/Kconfig | 4 +- > > cmd/Kconfig | 6 +- > > include/efi_loader.h | 28 +- > > lib/efi_loader/Kconfig | 9 + > > lib/efi_loader/efi_bootmgr.c | 493 -- > > lib/efi_loader/efi_device_path.c | 3 +- > > lib/efi_loader/efi_helper.c | 499 ++- > > 7 files changed, 529 insertions(+), 513 deletions(-) > > 'helper' seems a bit vague to me. How about efi_boot.c ? Although I hesitated to add one more new file as we already have efi_boottime.c and efi_bootmgr.c, then efi_boot.c?, okay I will do that. -Takahiro Akashi > REgards, > Simon
Re: [PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c
Hi AKASHI, On Sun, 17 Dec 2023 at 19:39, AKASHI Takahiro wrote: > > Some code moved from cmd/bootefi.c is actually necessary only for "bootefi > " command (starting an image manually loaded by a user using U-Boot > load commands or other methods (like JTAG debugger). > > The code will never been opted out as unused code by a compiler which > doesn't know how EFI boot manager is implemented. So introduce a new > configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out > explicitly. > > Signed-off-by: AKASHI Takahiro > --- > boot/Kconfig | 4 +- > cmd/Kconfig | 6 +- > include/efi_loader.h | 28 +- > lib/efi_loader/Kconfig | 9 + > lib/efi_loader/efi_bootmgr.c | 493 -- > lib/efi_loader/efi_device_path.c | 3 +- > lib/efi_loader/efi_helper.c | 499 ++- > 7 files changed, 529 insertions(+), 513 deletions(-) 'helper' seems a bit vague to me. How about efi_boot.c ? REgards, Simon
[PATCH v3 1/4] efi_loader: split unrelated code from efi_bootmgr.c
Some code moved from cmd/bootefi.c is actually necessary only for "bootefi
" command (starting an image manually loaded by a user using U-Boot
load commands or other methods (like JTAG debugger).
The code will never been opted out as unused code by a compiler which
doesn't know how EFI boot manager is implemented. So introduce a new
configuration, CONFIG_EFI_BINARY_EXEC, to enforce theem opted out
explicitly.
Signed-off-by: AKASHI Takahiro
---
boot/Kconfig | 4 +-
cmd/Kconfig | 6 +-
include/efi_loader.h | 28 +-
lib/efi_loader/Kconfig | 9 +
lib/efi_loader/efi_bootmgr.c | 493 --
lib/efi_loader/efi_device_path.c | 3 +-
lib/efi_loader/efi_helper.c | 499 ++-
7 files changed, 529 insertions(+), 513 deletions(-)
diff --git a/boot/Kconfig b/boot/Kconfig
index 987ca7314117..8ab7e6f63d34 100644
--- a/boot/Kconfig
+++ b/boot/Kconfig
@@ -523,7 +523,7 @@ config BOOTMETH_EXTLINUX_PXE
config BOOTMETH_EFILOADER
bool "Bootdev support for EFI boot"
- depends on BOOTEFI_BOOTMGR
+ depends on EFI_BINARY_EXEC
default y
help
Enables support for EFI boot using bootdevs. This makes the
@@ -558,7 +558,7 @@ config BOOTMETH_DISTRO
select BOOTMETH_SCRIPT if CMDLINE # E.g. Armbian uses scripts
select BOOTMETH_EXTLINUX # E.g. Debian uses these
select BOOTMETH_EXTLINUX_PXE if CMD_PXE && CMD_NET && DM_ETH
- select BOOTMETH_EFILOADER if BOOTEFI_BOOTMGR # E.g. Ubuntu uses this
+ select BOOTMETH_EFILOADER if EFI_BINARY_EXEC # E.g. Ubuntu uses this
config SPL_BOOTMETH_VBE
bool "Bootdev support for Verified Boot for Embedded (SPL)"
diff --git a/cmd/Kconfig b/cmd/Kconfig
index 24bfbe505722..2c993496b70e 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -273,7 +273,7 @@ config CMD_BOOTMETH
config BOOTM_EFI
bool "Support booting UEFI FIT images"
- depends on BOOTEFI_BOOTMGR && CMD_BOOTM && FIT
+ depends on EFI_BINARY_EXEC && CMD_BOOTM && FIT
default y
help
Support booting UEFI FIT images via the bootm command.
@@ -365,7 +365,7 @@ config CMD_BOOTEFI
if CMD_BOOTEFI
config CMD_BOOTEFI_BINARY
bool "Allow booting an EFI binary directly"
- depends on BOOTEFI_BOOTMGR
+ depends on EFI_BINARY_EXEC
default y
help
Select this option to enable direct execution of binary at 'bootefi'.
@@ -395,7 +395,7 @@ config CMD_BOOTEFI_HELLO_COMPILE
config CMD_BOOTEFI_HELLO
bool "Allow booting a standard EFI hello world for testing"
- depends on CMD_BOOTEFI_HELLO_COMPILE
+ depends on CMD_BOOTEFI_BINARY && CMD_BOOTEFI_HELLO_COMPILE
default y if CMD_BOOTEFI_SELFTEST
help
This adds a standard EFI hello world application to U-Boot so that
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 34e7fbbf1840..484c9fad239f 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -90,11 +90,7 @@ efi_status_t efi_add_runtime_mmio(void *mmio_ptr, u64 len);
* back to u-boot world
*/
void efi_restore_gd(void);
-/* Call this to unset the current device name */
-void efi_clear_bootdev(void);
-/* Call this to set the current device name */
-void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
-void *buffer, size_t buffer_size);
+
/* Called by networking code to memorize the dhcp ack package */
void efi_net_set_dhcp_ack(void *pkt, int len);
/* Print information about all loaded images */
@@ -116,10 +112,6 @@ static inline efi_status_t efi_add_runtime_mmio(void
*mmio_ptr, u64 len)
/* No loader configured, stub out EFI_ENTRY */
static inline void efi_restore_gd(void) { }
-static inline void efi_clear_bootdev(void) { }
-static inline void efi_set_bootdev(const char *dev, const char *devnr,
- const char *path, void *buffer,
- size_t buffer_size) { }
static inline void efi_net_set_dhcp_ack(void *pkt, int len) { }
static inline void efi_print_image_infos(void *pc) { }
static inline efi_status_t efi_launch_capsules(void)
@@ -129,6 +121,20 @@ static inline efi_status_t efi_launch_capsules(void)
#endif /* CONFIG_IS_ENABLED(EFI_LOADER) */
+#if CONFIG_IS_ENABLED(EFI_BINARY_EXEC)
+/* Call this to unset the current device name */
+void efi_clear_bootdev(void);
+/* Call this to set the current device name */
+void efi_set_bootdev(const char *dev, const char *devnr, const char *path,
+void *buffer, size_t buffer_size);
+#else
+static inline void efi_clear_bootdev(void) { }
+
+static inline void efi_set_bootdev(const char *dev, const char *devnr,
+ const char *path, void *buffer,
+ size_t buffer_size) { }
+#endif
+
/* Maximum number of configuration tables */
#define EFI_MAX_CONFIGURATION_TABLES 1
