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

CONFIG_BOOTLOADER_SRAM_SIZE should not be defined by default #49484

Closed
quic-egmc opened this issue Aug 24, 2022 · 23 comments · Fixed by #60371
Closed

CONFIG_BOOTLOADER_SRAM_SIZE should not be defined by default #49484

quic-egmc opened this issue Aug 24, 2022 · 23 comments · Fixed by #60371
Assignees
Labels
area: ARM ARM (32-bit) Architecture bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug

Comments

@quic-egmc
Copy link
Contributor

The Kconfig option BOOTLOADER_SRAM_SIZE defaults to reserving 16KB if XIP is not defined and, strangely, only on ARM or Xtensa architectures.

The description states:
This option specifies the amount of SRAM (measure in kB) reserved for a bootloader image, when either: - the Zephyr image itself is to act as the bootloader, or - Zephyr is a !XIP image, which implicitly assumes existence of a bootloader that loads the Zephyr !XIP image onto SRAM.

but the implicit assumption that we want to reserve space for a bootloader is not valid for many platforms. Please make this flag explicit or dependent on something obvious. This default behavior results in wasted space and is non-obvious!

@quic-egmc quic-egmc added the bug The issue is a bug, or the PR is fixing a bug label Aug 24, 2022
@mmahadevan108 mmahadevan108 added the area: ARM ARM (32-bit) Architecture label Aug 30, 2022
@mmahadevan108 mmahadevan108 added the priority: low Low impact/importance bug label Aug 30, 2022
@mmahadevan108 mmahadevan108 self-assigned this Aug 30, 2022
@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@quic-egmc
Copy link
Contributor Author

@microbuilder what are the next steps here? I'm happy to provide more context and why this is necessary.

@github-actions
Copy link

github-actions bot commented Jan 1, 2023

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@microbuilder
Copy link
Member

@d3zd3z Mind taking a look at this?

@github-actions
Copy link

github-actions bot commented Mar 3, 2023

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Mar 3, 2023
@quic-egmc quic-egmc removed the Stale label Mar 3, 2023
@quic-egmc
Copy link
Contributor Author

@microbuilder @d3zd3z reminder please

@d3zd3z
Copy link
Collaborator

d3zd3z commented Mar 3, 2023

I'm not sure why it is defined on xtensa, either, since only arm even makes use of the symbol. 21628d4 added this, which allows a particular configuration with a bootloader that uses this reserved memory for its sram so that the image can use the rest of the sram. @benwrs do you have any insight on when this gets used, and if maybe we could make the configuration clearer?

@github-actions
Copy link

github-actions bot commented May 3, 2023

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@microbuilder
Copy link
Member

@benwrs @d3zd3z Ping?

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@mmahadevan108
Copy link
Collaborator

@benwrs @peter-mitsis, can you help look at this issue.

@mmahadevan108
Copy link
Collaborator

mmahadevan108 commented Jul 13, 2023

@quic-egmc but the implicit assumption that we want to reserve space for a bootloader is not valid for many platforms.

This SRAM space is reserved for the case only when Zephyr is configured to act as a bootloader and not for all bootloaders. I thought I will mention this incase it helps.

@microbuilder
Copy link
Member

This SRAM space is reserved for the case only when Zephyr is configured to act as a bootloader and not for all bootloaders. I thought I will mention this incase it helps.

Thanks for the context. I didn't understand the use case here.

@microbuilder
Copy link
Member

microbuilder commented Jul 13, 2023

This does feel very arbitrary:

config BOOTLOADER_SRAM_SIZE
	int "SRAM reserved for bootloader"
	default 16
	depends on !XIP || IS_BOOTLOADER
	depends on ARM || XTENSA
	help
	  This option specifies the amount of SRAM (measure in kB) reserved for
	  a bootloader image, when either:
	  - the Zephyr image itself is to act as the bootloader, or
	  - Zephyr is a !XIP image, which implicitly assumes existence of a
	  bootloader that loads the Zephyr !XIP image onto SRAM.

And given that using Zephyr AS the bootloader is a pretty specific edge case, this shouldn't be reserved by default in every case for Arm/Xtensa.

I'm not the right person to sort this out since I don't know the various use cases, but @d3zd3z or @peter-mitsis or @bendiscz If you have suggestions, this should be updated to reflect that it's an exception not the rule with Arm/Xtensa, etc.

@microbuilder
Copy link
Member

@nordicjm @carlescufi Do you know of use cases for using Zephyr itself as the bootloader with Nordic customers? Struggling to understand the context for this, since I've normally only seen MCUBoot or a HW bootloader used with Zephyr, not Zephyr as bootloader.

@microbuilder
Copy link
Member

I'd suggest this should default to 0 and in the edge case where it's needed, it can be set higher. I already see some boards in tree doing this:

# Override the setting in misc/Kconfig to allow full use of SRAM:
config BOOTLOADER_SRAM_SIZE
	default 0 if !XIP

Or:

CONFIG_BOOTLOADER_SRAM_SIZE=0

@microbuilder
Copy link
Member

microbuilder commented Jul 13, 2023

Sorry for all the messages, but ... the mystery that is CONFIG_IS_BOOTLOADER apparently goes pretty far back: #9566 ... and this feature was initially introduced in 2016: 21628d4

I'm going to flag this for review in the arch meeting since I don't know the context here, but we should sort it out.

@quic-egmc I'm not sure this is still important to you, but can you give me some context on how you're hitting this issue? It seems like this should only be enabled if CONFIG_IS_BOOTLOADER=y for your board. Or is !XIP plus ARM || XTENSA the problem?

Doing a quick test with a random Arm board, I'm not seeing the 16KB being assigned:

$ west build -p auto -b qemu_cortex_m0 samples/hello_world
...
Memory region         Used Size  Region Size  %age Used
           FLASH:       15518 B       256 KB      5.92%
             RAM:        3936 B        16 KB     24.02%
        IDT_LIST:          0 GB         2 KB      0.00%

If you can give me a build target where BOOTLOADER_SRAM_SIZE is being set, it would help.

EDIT: Some other useful context here, trying to clarify what this does: #6180 (comment)

@quic-egmc
Copy link
Contributor Author

this is going a long ways back so will have to dig deep.

I did a few experiments, namely to un-do our workaround which was to force CONFIG_BOOTLOADER_SRAM_SIZE=0 and I indeed see that our .config gets a default CONFIG_BOOTLOADER_SRAM_SIZE=16 even though we definitely not using Zephyr "as the bootloader".

Since the units of this option are KB, this is pretty significant.

Yes, CONFIG_ARM=y and CONFIG_XIP is not set.

You could argue that either the default value should be zero or that the depends on !XIP clause should be removed. This statement in Kconfig.zephyr:

	  - Zephyr is a !XIP image, which implicitly assumes existence of a
	  bootloader that loads the Zephyr !XIP image onto SRAM.

reaches an incorrect conclusion - just because somebody (or thing) is loading zephyr does not mean we want a reservation in the memory map.

@microbuilder
Copy link
Member

@quic-egmc Thanks for the reply, Eugene. I'll raise a PR tomorrow setting the default to 0, but I'd like to discuss the !XIP bit in the arch call to make sure I understand where this came from and why, and see how we can better document this. And also why it's only enabled for Arm and Xtensa.

@quic-egmc
Copy link
Contributor Author

I'm struggling to get CONFIG_XIP not set upstream versus on our internal projects....

@quic-egmc
Copy link
Contributor Author

Ok I found a real example in-tree. Look at the board bcm958402m2_m7_defconfig.

It disables XIP but does not override BOOTLOADER_SRAM_SIZE and in its .config it gets:

CONFIG_BOOTLOADER_SRAM_SIZE=16

and the resulting RAM usage map shows the total RAM reduced by 16K, at 496KB:

Memory region         Used Size  Region Size  %age Used
           FLASH:          0 GB         0 GB
             RAM:       24596 B       496 KB      4.84%
        IDT_LIST:          0 GB         2 KB      0.00%

now force CONFIG_BOOTLOADER_SRAM_SIZE to zero in boards/arm/bcm958402m2_m7/bcm958402m2_m7_defconfig and you instead see the total RAM size increased to 512KB:

Memory region         Used Size  Region Size  %age Used
           FLASH:          0 GB         0 GB
             RAM:       24596 B       512 KB      4.69%
        IDT_LIST:          0 GB         2 KB      0.00%

this stems from boards/arm/bcm958402m2_m7/bcm958402m2_m7_defconfig:
#define RAM_SIZE (CONFIG_SRAM_SIZE * 1K - CONFIG_BOOTLOADER_SRAM_SIZE * 1K)

@microbuilder
Copy link
Member

@quic-egmc I raised a PR setting the default to 0 (you're tagged for review), but if you're available and if this topic gets scheduled in next week's arch call, I'd be interested to discuss CONFIG_XIP and what you'd like to see changed upstream.

@carlescufi
Copy link
Member

@nordicjm @carlescufi Do you know of use cases for using Zephyr itself as the bootloader with Nordic customers? Struggling to understand the context for this, since I've normally only seen MCUBoot or a HW bootloader used with Zephyr, not Zephyr as bootloader.

Not directly, but MCUboot is a Zephyr application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: ARM ARM (32-bit) Architecture bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants