Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: banner: Allow for external boot banner #72400

Closed
wants to merge 3 commits into from

Conversation

nordicjm
Copy link
Collaborator

@nordicjm nordicjm commented May 7, 2024

Adds a new Kconfig CONFIG_BOOT_BANNER_EXTERNAL_IMPLEMENTATION which
allows for applications/modules to implement their own boot banner

Also includes fix for MCUboot

Background:

MCUboot has had a custom boot banner added which replaces the zephyr one to include information on the MCUboot build, with a normal zephyr build this works fine due to the linker dropping the original function and using the one that MCUboot has, but with llext enabled in the build, the build fails because the function is defined multiple times. This resolves the issue by making the zephyr one weak which allows for applications to replace it with an alternative implementation

  • Release notes addition

@nordicjm nordicjm requested review from pillo79, teburd, lyakh and tejlmand May 7, 2024 07:28
@zephyrbot zephyrbot added Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. area: Kernel labels May 7, 2024
tejlmand
tejlmand previously approved these changes May 7, 2024
pillo79
pillo79 previously approved these changes May 7, 2024
simhein
simhein previously approved these changes May 7, 2024
tejlmand pushed a commit to tejlmand/fw-nrfconnect-zephyr-1 that referenced this pull request May 7, 2024
Makes the boot banner function weak, this resolves an issue when
building with llext enabled which uses different build options
than a normal zephyr build

Upstream PR: zephyrproject-rtos/zephyr#72400

Signed-off-by: Jamie McCrae <[email protected]>
Copy link
Collaborator

@katgiadla katgiadla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it works on-target.

@nashif
Copy link
Member

nashif commented May 7, 2024

do not like this approach of adding weak symbols so that can be impemented somewhere else. Basically what this mean is an introduction of an adhoc interface or API. We have been trying to minimize the use of weak symbols, because of how unreliable they are when multiple symbols are defined.

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No particular complaint about weak symbols, I think they're a fine tool in the right contexts.

But here? I mean, we already allow the banner to be configured. We already allow the banner to be disabled. We already allow apps to register early SYS_INIT() hooks to print their own banners[1] in any way they want.

What's the value in adding another dial to an already complicated whizgig? Who needs this? Frankly I'd argue that the only real value to a banner is that it's "standard" and can be reliably used to identify Zephyr apps across platforms and versions.

Not worth a -1. But I guess maybe I'm saying I'd prefer to see features removed from this area vs. added.

[1] Even before the official one. Ours is actually quite late in the startup sequence; it happens out of the main thread after all the kernel init is already done.

@nashif nashif removed the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label May 7, 2024
Copy link
Member

@stephanosio stephanosio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MCUboot has had a custom boot banner added which replaces the zephyr one

If so, why would you not just set CONFIG_BOOT_BANNER=n and let MCUboot do its thing?

@nordicjm
Copy link
Collaborator Author

nordicjm commented May 8, 2024

MCUboot has had a custom boot banner added which replaces the zephyr one

If so, why would you not just set CONFIG_BOOT_BANNER=n and let MCUboot do its thing?

If someone wants a boot banner, it makes sense to use a single option, if people have existing applications then updating zephyr would have no impact for them, if they have the boot banner off for their build then nothing will change, adding a new option then means suddenly everyone has it enabled. I don't see the point in having multiple options for the same thing, and what if someone enables them both, the output would just be weird.

do not like this approach of adding weak symbols so that can be impemented somewhere else. Basically what this mean is an introduction of an adhoc interface or API. We have been trying to minimize the use of weak symbols, because of how unreliable they are when multiple symbols are defined.

So would you be opposed to having this defined as an API with a Kconfig to suppress the output so that applications can provide their own function?

nordicjm added a commit to nrfconnect/sdk-zephyr that referenced this pull request May 8, 2024
Makes the boot banner function weak, this resolves an issue when
building with llext enabled which uses different build options
than a normal zephyr build

Upstream PR: zephyrproject-rtos/zephyr#72400

Signed-off-by: Jamie McCrae <[email protected]>
@carlescufi carlescufi requested review from andyross and stephanosio May 8, 2024 10:35
@nordicjm
Copy link
Collaborator Author

nordicjm commented May 8, 2024

I have added a banner API and adjusted MCUboot to use this

@nordicjm nordicjm added the DNM This PR should not be merged (Do Not Merge) label May 8, 2024
@zephyrbot zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label May 8, 2024
@nordicjm nordicjm added the DNM This PR should not be merged (Do Not Merge) label May 8, 2024
nordicjm added 3 commits May 8, 2024 13:19
Adds a new Kconfig CONFIG_BOOT_BANNER_EXTERNAL_IMPLEMENTATION which
allows for applications/modules to implement their own boot banner

Signed-off-by: Jamie McCrae <[email protected]>
Update Zephyr fork of MCUboot to revision:
  TODO

Brings following Zephyr relevant fixes:
  - 3240371d zephyr: Update boot banner implementation

Signed-off-by: Jamie McCrae <[email protected]>
Adds release notes on the new external boot banner functionality

Signed-off-by: Jamie McCrae <[email protected]>
@zephyrbot zephyrbot added Release Notes To be mentioned in the release notes and removed DNM This PR should not be merged (Do Not Merge) labels May 8, 2024
@nordicjm nordicjm added the DNM This PR should not be merged (Do Not Merge) label May 8, 2024
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think this is sort of silly, given that a printk() out of a SYS_INIT() hook is absolutely isomorphic to a BOOT_BANNER_EXTERNAL_IMPLEMENTATION.

But no -1, this is obviously harmless. I'm going to remove myself from the bikeshed now. :)

@nashif
Copy link
Member

nashif commented May 8, 2024

one could also do it without all the changes to zephyr...

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index c550ab461cb..2effc992235 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,6 +5,17 @@
  */

 #include <stdio.h>
+#include <zephyr/kernel.h>
+#include <zephyr/init.h>
+#include <zephyr/device.h>
+
+static int custom_boot_banner(void)
+{
+       printk("*** HELLO WORLD APP BANNER v1.0 ***\n");
+       return 0;
+}
+
+

 int main(void)
 {
@@ -12,3 +23,7 @@ int main(void)

        return 0;
 }
+
+
+
+SYS_INIT(custom_boot_banner, APPLICATION, 0);

resulting in:

*** Booting Zephyr OS build v3.6.99-3532-gd77dc62d196b ***
*** HELLO WORLD APP BANNER v1.0 ***
Hello World! qemu_x86/atom

@henrikbrixandersen
Copy link
Member

once could also do it without all the changes to zephyr...

This is pretty much what we do for our downstream applications.

@nordicjm
Copy link
Collaborator Author

nordicjm commented May 8, 2024

once could also do it without all the changes to zephyr...

diff --git a/samples/hello_world/src/main.c b/samples/hello_world/src/main.c
index c550ab461cb..2effc992235 100644
--- a/samples/hello_world/src/main.c
+++ b/samples/hello_world/src/main.c
@@ -5,6 +5,17 @@
  */

 #include <stdio.h>
+#include <zephyr/kernel.h>
+#include <zephyr/init.h>
+#include <zephyr/device.h>
+
+static int custom_boot_banner(void)
+{
+       printk("*** HELLO WORLD APP BANNER v1.0 ***\n");
+       return 0;
+}
+
+

 int main(void)
 {
@@ -12,3 +23,7 @@ int main(void)

        return 0;
 }
+
+
+
+SYS_INIT(custom_boot_banner, APPLICATION, 0);

resulting in:

*** Booting Zephyr OS build v3.6.99-3532-gd77dc62d196b ***
*** HELLO WORLD APP BANNER v1.0 ***
Hello World! qemu_x86/atom

Except that doesn't work for this usecase as mentioned earlier:

If someone wants a boot banner, it makes sense to use a single option, if people have existing applications then updating zephyr would have no impact for them, if they have the boot banner off for their build then nothing will change, adding a new option then means suddenly everyone has it enabled. I don't see the point in having multiple options for the same thing, and what if someone enables them both, the output would just be weird.

@nashif
Copy link
Member

nashif commented May 8, 2024

Except that doesn't work for this usecase as mentioned earlier:

If someone wants a boot banner, it makes sense to use a single option, if people have existing applications then updating zephyr would have no impact for them, if they have the boot banner off for their build then nothing will change, adding a new option then means suddenly everyone has it enabled. I don't see the point in having multiple options for the same thing, and what if someone enables them both, the output would just be weird.

hmm, I am not sure I understand the usecase here. The code is not adding any options, the app will decide what it wants to print regardless of any options related to boot banner coming from zephyr.

@nordicjm
Copy link
Collaborator Author

nordicjm commented May 8, 2024

Allowing it to be controlled using the same existing Kconfig, e.g.:

menuconfig MCUBOOT_BOOT_BANNER
	bool "Use MCUboot boot banner"
	depends on BOOT_BANNER
	depends on "$(APP_VERSION_EXTENDED_STRING)" != ""
	select BOOT_BANNER_EXTERNAL_IMPLEMENTATION
	default y

Instead of adding a new Kconfig, and this disables the normal boot banner output because it includes a modified version of it in the output:

	    *** Booting MCUboot v2.0.0-72-g8c0e36c88663 ***
	    *** Using Zephyr OS build v3.6.0-2607-gd0be2010c31f ***

You can't have a Kconfig de-select something, so the only way to add another one would be to have depends on !BOOT_BANNER so it can never be active - this is more of an issue when this is used by modules

@nashif
Copy link
Member

nashif commented May 10, 2024

Allowing it to be controlled using the same existing Kconfig, e.g.:

MCUBOOT_BOOT_BANNER can then control the custom banner you can introduce in mcuboot using something similar to what I have in my example. You do not need to use the zephyr built-in banner

@nordicjm
Copy link
Collaborator Author

Allowing it to be controlled using the same existing Kconfig, e.g.:

MCUBOOT_BOOT_BANNER can then control the custom banner you can introduce in mcuboot using something similar to what I have in my example. You do not need to use the zephyr built-in banner

As said above:

You can't have a Kconfig de-select something, so the only way to add another one would be to have depends on !BOOT_BANNER so it can never be active - this is more of an issue when this is used by modules

And this makes things worse because then everyone out of tree that is using MCUboot that has set CONFIG_BOOT_BANNER=n now suddenly has a new boot banner appear

@nordicjm nordicjm added the Architecture Review Discussion in the Architecture WG required label May 13, 2024
@carlescufi
Copy link
Member

Architecture WG:

  • The approach to override boot_banner from the Zephyr has multiple objections
  • An alternative approach is proposed, with the use of configdefault for modules that wish to override the Zephyr banner:
In the module's Kconfig file:

configdefault BOOT_BANNER
   default n
   
config MODULE_BOOT_BANNER
   default y
   

CONFIG_BOOT_BANNER=y
*** Booting Zephyr OS build v3.6.0-2607-gd0be2010c31f ***

CONFIG_MODULE_BOOT_BANNER=y
*** Booting Module v2.0.0-72-g8c0e36c88663 ***
if (! CONFIG_BOOT_BANNER)
print *** Using Zephyr OS build v3.6.0-2607-gd0be2010c31f ***
endif

@@ -427,7 +425,10 @@ static void bg_thread_main(void *unused1, void *unused2, void *unused3)
#if defined(CONFIG_STACK_POINTER_RANDOM) && (CONFIG_STACK_POINTER_RANDOM != 0)
z_stack_adjust_initialized = 1;
#endif /* CONFIG_STACK_POINTER_RANDOM */

#if defined(CONFIG_BOOT_BANNER)
Copy link
Member

@nashif nashif May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boot_banner should also run when CONFIG_BOOT_BANNER=n to handle the delayed boot option which does not depend on the boot banner.

@nordicjm nordicjm closed this Jun 12, 2024
anangl pushed a commit to anangl/sdk-zephyr that referenced this pull request Jul 1, 2024
Makes the boot banner function weak, this resolves an issue when
building with llext enabled which uses different build options
than a normal zephyr build

Upstream PR: zephyrproject-rtos/zephyr#72400

Signed-off-by: Jamie McCrae <[email protected]>
(cherry picked from commit d22aede)
@nordicjm nordicjm deleted the weakbanner branch July 12, 2024 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Architecture Review Discussion in the Architecture WG required area: Kernel DNM This PR should not be merged (Do Not Merge) manifest manifest-mcuboot Release Notes To be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.