[Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Neal Frager via buildroot]

If a user requests a u-boot binary in elf format,
they may actually want the stripped u-boot.elf version.
This patch provides the stripped u-boot.elf binary.

Signed-off-by: Neal Frager <neal.frager@amd.com>
---
V1->V2:
  - reduced scope to only 64-bit ARM arch platforms
  - non-ARM platforms may not have a u-boot.elf by default
---
 boot/uboot/uboot.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 0439ec5e4b..61b3074163 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -54,6 +54,9 @@ endif
 
 ifeq ($(BR2_TARGET_UBOOT_FORMAT_ELF),y)
 UBOOT_BINS += u-boot
+ifeq ($(BR2_aarch64),y)
+UBOOT_BINS += u-boot.elf
+endif
 # To make elf usable for debuging on ARC use special target
 ifeq ($(BR2_arc),y)
 UBOOT_MAKE_TARGET += mdbtrick
-- 
2.17.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Luca Ceresoli via buildroot]

Hello Neal,

On Mon, 21 Nov 2022 22:36:11 -0700
Neal Frager <neal.frager@amd.com> wrote:

> If a user requests a u-boot binary in elf format,
> they may actually want the stripped u-boot.elf version.
> This patch provides the stripped u-boot.elf binary.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
> V1->V2:
>   - reduced scope to only 64-bit ARM arch platforms
>   - non-ARM platforms may not have a u-boot.elf by default
> ---
>  boot/uboot/uboot.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 0439ec5e4b..61b3074163 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -54,6 +54,9 @@ endif
>  
>  ifeq ($(BR2_TARGET_UBOOT_FORMAT_ELF),y)
>  UBOOT_BINS += u-boot
> +ifeq ($(BR2_aarch64),y)
> +UBOOT_BINS += u-boot.elf
> +endif

Is it really assured that all arm64 do build this file? And that
u-boot.elf is not useful for other architectures?

Looking at the U-Boot source code, it looks like this file is built if
CONFIG_REMAKE_ELF is enabled, which seems independent on the
architecture and happens for some non-arm64 defconfigs as well (e.g.
netgear_dgnd3700v2_ram_defconfig).

At first glance, it looks like we need (yet) another Kconfig knob in
Buildroot to manage this cleanly. Otherwise in
UBOOT_INSTALL_IMAGES_CMDS we might just install u-boot.elf if it is
present,skip it silently otherwise.

Not sure which solution is less appealing. :(

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Frager, Neal via buildroot]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

Hello Luca,

> If a user requests a u-boot binary in elf format, they may actually 
> want the stripped u-boot.elf version.
> This patch provides the stripped u-boot.elf binary.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
> V1->V2:
>   - reduced scope to only 64-bit ARM arch platforms
>   - non-ARM platforms may not have a u-boot.elf by default
> ---
>  boot/uboot/uboot.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 
> 0439ec5e4b..61b3074163 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -54,6 +54,9 @@ endif
>  
>  ifeq ($(BR2_TARGET_UBOOT_FORMAT_ELF),y)
>  UBOOT_BINS += u-boot
> +ifeq ($(BR2_aarch64),y)
> +UBOOT_BINS += u-boot.elf
> +endif

> Is it really assured that all arm64 do build this file? And that u-boot.elf is not useful for other architectures?

> Looking at the U-Boot source code, it looks like this file is built if CONFIG_REMAKE_ELF is enabled, which seems independent on the architecture and happens for some non-arm64 defconfigs as well (e.g.
> netgear_dgnd3700v2_ram_defconfig).

> At first glance, it looks like we need (yet) another Kconfig knob in Buildroot to manage this cleanly. Otherwise in UBOOT_INSTALL_IMAGES_CMDS we might just install u-boot.elf if it is present,skip it silently otherwise.

> Not sure which solution is less appealing. :(

Perhaps just checking every file for existence when running the loop below is the best option like below.  The idea is to keep this as simple as possible.  What do you think?

define UBOOT_INSTALL_IMAGES_CMDS
	$(foreach f,$(UBOOT_BINS), \
			$(if test -f $(@D)/$(f), \
			cp -dpf $(@D)/$(f) $(BINARIES_DIR))/
	)

Best regards,
Neal Frager
AMD

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 14644 bytes --]

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Thomas Petazzoni via buildroot]

Hello Neal,

On Tue, 22 Nov 2022 09:43:45 +0000
"Frager, Neal via buildroot" <buildroot@buildroot.org> wrote:

> Perhaps just checking every file for existence when running the loop below is the best option like below.  The idea is to keep this as simple as possible.  What do you think?
> 
> define UBOOT_INSTALL_IMAGES_CMDS
> 	$(foreach f,$(UBOOT_BINS), \
> 			$(if test -f $(@D)/$(f), \
> 			cp -dpf $(@D)/$(f) $(BINARIES_DIR))/
> 	)

I think we should change:

config BR2_TARGET_UBOOT_FORMAT_ELF
        bool "u-boot.elf"

to:

config BR2_TARGET_UBOOT_FORMAT_ELF
        bool "u-boot"

(because this is really what it is doing)

and then introduce:

config BR2_TARGET_UBOOT_FORMAT_ELF_STRIPPED
        bool "u-boot.elf"

which installs the stripped u-boot.elf file.

This way, BR2_TARGET_UBOOT_FORMAT_ELF_STRIPPED can be enabled on
configurations that need it, without breaking configurations that
provide u-boot but not u-boot.elf.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Luca Ceresoli via buildroot]

Hello Thomas,

On Tue, 22 Nov 2022 17:13:15 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello Neal,
> 
> On Tue, 22 Nov 2022 09:43:45 +0000
> "Frager, Neal via buildroot" <buildroot@buildroot.org> wrote:
> 
> > Perhaps just checking every file for existence when running the loop below is the best option like below.  The idea is to keep this as simple as possible.  What do you think?
> > 
> > define UBOOT_INSTALL_IMAGES_CMDS
> > 	$(foreach f,$(UBOOT_BINS), \
> > 			$(if test -f $(@D)/$(f), \
> > 			cp -dpf $(@D)/$(f) $(BINARIES_DIR))/
> > 	)  
> 
> I think we should change:
> 
> config BR2_TARGET_UBOOT_FORMAT_ELF
>         bool "u-boot.elf"
> 
> to:
> 
> config BR2_TARGET_UBOOT_FORMAT_ELF
>         bool "u-boot"
> 
> (because this is really what it is doing)

Right.

> 
> and then introduce:
> 
> config BR2_TARGET_UBOOT_FORMAT_ELF_STRIPPED
>         bool "u-boot.elf"
> 
> which installs the stripped u-boot.elf file.
> 
> This way, BR2_TARGET_UBOOT_FORMAT_ELF_STRIPPED can be enabled on
> configurations that need it, without breaking configurations that
> provide u-boot but not u-boot.elf.

I agree with the principle you are proposing.

However, according to the U-Boot source code, u-boot.elf appears as
u-boot.bin repackaged into an ELF file. Is "stripped" a correct word
for this?

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Thomas Petazzoni via buildroot]

On Tue, 22 Nov 2022 19:07:43 +0100
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> I agree with the principle you are proposing.
> 
> However, according to the U-Boot source code, u-boot.elf appears as
> u-boot.bin repackaged into an ELF file. Is "stripped" a correct word
> for this?

I used "stripped" because that's how Neal described this file.

Based on what you say, perhaps a better option name is:

BR2_TARGET_UBOOT_FORMAT_BIN_ELF
	bool "u-boot.elf"

Thoughts?

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Luca Ceresoli via buildroot]

Hi Thomas, Neal,

On Tue, 22 Nov 2022 21:47:13 +0100
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Tue, 22 Nov 2022 19:07:43 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > I agree with the principle you are proposing.
> > 
> > However, according to the U-Boot source code, u-boot.elf appears as
> > u-boot.bin repackaged into an ELF file. Is "stripped" a correct word
> > for this?  
> 
> I used "stripped" because that's how Neal described this file.
> 
> Based on what you say, perhaps a better option name is:
> 
> BR2_TARGET_UBOOT_FORMAT_BIN_ELF
> 	bool "u-boot.elf"
> 
> Thoughts?

Seems better.

Neal, adding a short help text would also be useful IMO, as this "bin
elf" Frankenstein creation is really non intuitive.

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Frager, Neal via buildroot]

[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]

Hi Thomas, Luca,

> On Tue, 22 Nov 2022 19:07:43 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > I agree with the principle you are proposing.
> > 
> > However, according to the U-Boot source code, u-boot.elf appears as 
> > u-boot.bin repackaged into an ELF file. Is "stripped" a correct word 
> > for this?
> 
> I used "stripped" because that's how Neal described this file.
> 
> Based on what you say, perhaps a better option name is:
> 
> BR2_TARGET_UBOOT_FORMAT_BIN_ELF
> 	bool "u-boot.elf"
> 
> Thoughts?

> Seems better.

> Neal, adding a short help text would also be useful IMO, as this "bin elf" Frankenstein creation is really non intuitive.

I am not sure creating a separate BR2 config is the right solution.  Both the u-boot and u-boot.elf file are in fact elf formatted files.  One just has all the debug symbols and one does not.

If we create a separate BR2 config, even with help text, I am not sure this would be so clean.  I was actually surprised when I used the BR2_TARGET_UBOOT_FORMAT_ELF config and did not get a u-boot.elf file.
I imagine many others are surprised too.

What do you think of this solution?  Basically, we check that the file exists before copying.  It is probably good to have this check anyway.

https://patchwork.ozlabs.org/project/buildroot/patch/20221122102025.62704-1-neal.frager@amd.com/

If you want, we could even add an else statement that prints a warning message about the expected u-boot file not being copied, so a user is warned about it at build time.

Thoughts?

Best regards,
Neal Frager
AMD




[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 15136 bytes --]

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Thomas Petazzoni via buildroot]

Hello Neal,

On Wed, 23 Nov 2022 00:09:39 +0000
"Frager, Neal" <neal.frager@amd.com> wrote:

> I am not sure creating a separate BR2 config is the right solution.
> Both the u-boot and u-boot.elf file are in fact elf formatted files.
> One just has all the debug symbols and one does not.

I don't understand why you think having a separate option is a problem.
We already have separate options for each file that can be installed by
U-Boot. Adding another option for another file is completely logical,
and actually much more logical than having a weird option that
magically installs 2 files - sometimes.

> What do you think of this solution?  Basically, we check that the
> file exists before copying.  It is probably good to have this check
> anyway.

Checks like this are actually not great, because you expect something
to happen, there is no error, but the thing you were expecting did not
happen. Clearly confusing for the user.

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/1] boot/uboot/uboot.mk: add stripped u-boot.elf support

[Frager, Neal via buildroot]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

Hi Thomas,

> I am not sure creating a separate BR2 config is the right solution.
> Both the u-boot and u-boot.elf file are in fact elf formatted files.
> One just has all the debug symbols and one does not.

> I don't understand why you think having a separate option is a problem.
> We already have separate options for each file that can be installed by U-Boot. Adding another option for another file is completely logical, and actually much more logical than having a weird option that magically installs 2 files - sometimes.

If adding another BR2 config for this is ok for you, it is ok for me.  I was just thinking it was strange to have two different commands for elf files.

To go along with Luca's point, I will add some helper text in the Config.in file, so it can help clarify which command is which for users.

> What do you think of this solution?  Basically, we check that the file 
> exists before copying.  It is probably good to have this check anyway.

> Checks like this are actually not great, because you expect something to happen, there is no error, but the thing you were expecting did not happen. Clearly confusing for the user.

Ok, no problem.  I will remove this for v4.

Thank you for your help and thoughtful review!

Best regards,
Neal Frager
AMD



[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 15164 bytes --]

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot