zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.88k stars 6.63k forks source link

Linker sections based on devicetree partitions #31073

Open JordanYates opened 3 years ago

JordanYates commented 3 years ago

Is your enhancement proposal related to a problem?

I am in the process of writing a custom NVS-like filesystem that can be initialized at compile time. Compile time initialization is required as values are expected to be many KB large (Compiled in values are expected to never be deleted). Storing them once in .rodata and then saving them again to the NVS on init is not feasible (In extreme cases it simply wouldn't fit).

For a NVS filesystem to be useful, it must be located at the same address across application upgrades. Declaring variables into a fixed memory address (for compile time initialization) requires the use of linker regions.

Zephyr already provides a mechanism for partitioning flash space, the aptly named fixed-partitions. The natural solution is to leverage the flash partitions to generate linker memory regions which describe a start address and size. The linker snippets framework could then be utilized to populate the sections appropriately.

Generating linker sections from flash partitions would be opt-in via a devicetree property.

Other problems resolved by this addition

Currently there is no protection on the flash partitions used for the Zephyr NVS subsystem. If the "storage" partition is declared to be 50% of flash, and the application fill 75% of flash, no warnings or errors will be generated. By giving the partition its own linker section, the linker will warn on this condition. As a side benefit, the space reserved for NVS also becomes visible when compiling.

FLASH memory region

When compiling a standalone application, the FLASH region consists of the entire memory space of the device. When compiling an application against a bootloader, the FLASH region shrinks to the boundaries defined by the zephyr,code-partition chosen node. After the proposed solution:

Standalone case: The FLASH region would automatically resize itself to be as large as possible without impinging on a region declared from devicetree. Bootloader case: Unchanged.

Describe the solution you'd like

After prototyping a solution, I believe there is only one 'user visible' change required to make this work.

A new optional property on the fixed-partitions child binding, provisionally named linker-region, that signifies a memory region should be generated for this provision.

The rest is just a new variant of FOR_EACH, some minor linker script changes, and a sprinkle of macrobatics. Below is an example output, note that the FLASH region size has decreased from 1MB.

&flash0 {
    /*
     * For more information, see:
     * https://docs.zephyrproject.org/latest/guides/dts/legacy-macros.html#legacy-flash-partitions
     */
    partitions {
        compatible = "fixed-partitions";
        #address-cells = <1>;
        #size-cells = <1>;

        boot_partition: partition@0 {
            label = "mcuboot";
            reg = <0x000000000 0x0000C000>;
        };
        slot0_partition: partition@c000 {
            label = "image-0";
            reg = <0x0000C000 0x00067000>;
        };
        slot1_partition: partition@73000 {
            label = "image-1";
            reg = <0x00073000 0x00067000>;
        };
        scratch_partition: partition@da000 {
            label = "image-scratch";
            reg = <0x000da000 0x0001e000>;
        };
        /*
         * Storage partition will be used by FCB/LittleFS/NVS
         * if enabled.
         */
        storage_partition: partition@f8000 {
            label = "storage";
            reg = <0x000f8000 0x00008000>;
                        linker-region;
        };
    };
};
[2/6] Linking C executable zephyr\zephyr_prebuilt.elf
Memory region         Used Size  Region Size  %age Used
           FLASH:      243404 B       992 KB     23.96%
            SRAM:       50748 B       256 KB     19.36%
        IDT_LIST:         216 B         2 KB     10.55%
         storage:          0 GB        32 KB      0.00%
[6/6] Linking C executable zephyr\zephyr.elf

Open Questions

As far as I know linker regions only make sense for onboard memory (Not sure about XIP). Should partitions that enable linker-region on other flash devices be ignored or error out?

Automatically resizing the FLASH region requires that custom partitions (or at least those enabling linker-region) are packed at the end of the usable ROM to avoid wasting space. I believe this is not a problem.

Describe alternatives you've considered

Hardcoding memory regions using the SECTIONS snippet. This will result in overlapping memory regions for standalone applications, and splits the memory definition of the device across several files.

Laczen commented 3 years ago

Hi @JordanYates, I have one major concern regarding this enhancement: the enhancement creates an image that consists of several regions that need to be uploaded separately when all partitions are enabled, how will this be handled ?

Regarding the other problem solved by this addition: this is not really a solution, when zephyr, code-partition is set to flash0 the complete flash is reserved for the code. If an area needs to be reserved for storage it should be declared. The correct way to do this would be to create a code partiton and a storage partition in the dts.

I don't fully understand the requirement for read-only variables to be used but they could be stored in a separate flash partition and read using the flash interface.

JordanYates commented 3 years ago

the enhancement creates an image that consists of several regions that need to be uploaded separately when all partitions are enabled, how will this be handled ?

The regions are simply an abstraction used by the linker. The only observable change in output images is that variables may be placed at different addresses (if the memory regions are used to store custom sections). Depending on the partitions/regions enabled, .bin files may have substantial padding, but that doesn't change the upload process.

If an area needs to be reserved for storage it should be declared. The correct way to do this would be to create a code partiton and a storage partition in the dts.

code and storage partitions are already being declared, they are just being ignored by the linker. A storage partition that is defined to contain 100% of flash, leaving no space for an application, will happily be ignored.

they could be stored in a separate flash partition and read using the flash interface.

This is the core of what this enhancement is addressing. It is currently impossible to generate an image that contains variables at a given offset due to the lack of support for custom linker memory regions.

zephyr, code-partition is set to flash0 the complete flash is reserved for the code

zephyr, code-partition is set to a partition (i.e. slot0_partition) not to the flash device. Furthermore, zephyr,code-partition is only used when compiling an application with a bootloader, otherwise partitions are ignored. When zephyr,code-partition is not being used, the assumption that the entirety of flash is available for code is incorrect. The devicetree already provides information that certain sections should not be used.

Laczen commented 3 years ago

the enhancement creates an image that consists of several regions that need to be uploaded separately when all partitions are enabled, how will this be handled ?

The regions are simply an abstraction used by the linker. The only observable change in output images is that variables may be placed at different addresses (if the memory regions are used to store custom sections). Depending on the partitions/regions enabled, .bin files may have substantial padding, but that doesn't change the upload process.

The padding will overwrite data that it should not overwrite.

If an area needs to be reserved for storage it should be declared. The correct way to do this would be to create a code partiton and a storage partition in the dts.

code and storage partitions are already being declared, they are just being ignored by the linker. A storage partition that is defined to contain 100% of flash, leaving no space for an application, will happily be ignored.

Correct, if you look at Kconfig.zephyr you will see that if USE_DT_CODE_PARTITION is not defined and FLASH_LOAD_SIZE and FLASH_LOAD_OFFSET are left to default the whole flash is reserved for code and the partitioning is completely ignored. If partitioning needs to be taken into account USE_DT_CODE_PARTITION is a correct way to do this. Your proposal is a different way to achieve that, and I am not against it, but there needs to be a way to handle the upload of separate sections.

they could be stored in a separate flash partition and read using the flash interface.

This is the core of what this enhancement is addressing. It is currently impossible to generate an image that contains variables at a given offset due to the lack of support for custom linker memory regions.

Agreed, but exactly this creates multiple regions in the image and associated problems.

zephyr, code-partition is set to flash0 the complete flash is reserved for the code

zephyr, code-partition is set to a partition (i.e. slot0_partition) not to the flash device. Furthermore, zephyr,code-partition is only used when compiling an application with a bootloader, otherwise partitions are ignored. When zephyr,code-partition is not being used, the assumption that the entirety of flash is available for code is incorrect. The devicetree already provides information that certain sections should not be used.

You are correct, my formulation was wrong, it should have been when USE_DT_CODE_PARTITION is not enabled the complete flash area is reserved for code. At the moment the devicetree contains no information about certain sections that should not be used, the property linker-region adds exactly this information.

JordanYates commented 3 years ago

The padding will overwrite data that it should not overwrite.

How will this occur? Padding by definition is only inserted at memory locations where no variables are present to reach addresses where more variables exist.

but there needs to be a way to handle the upload of separate sections.

Once you get to a .bin or .hex to upload the sections/memory regions are invisible. Is there something I'm missing here?

Agreed, but exactly this creates multiple regions in the image and associated problems.

Sorry, what are the associated problems with multiple regions?

IMO partitioning needing to be taken into account and USE_DT_CODE_PARTITION are separate (although related) problems. The storage partition is the prime example, regardless of whether you want to constrain your code to some smaller partition (zephyr,code-partition), it is never a valid configuration for code to flow into the storage partition.

In short, I don't believe that CONFIG_USE_DT_CODE_PARTITION=n means that all partitions should be ignored. The symbol itself only says that when not selected, offset and size can be set manually. The proposed PR does not change this behavior. The behavior change is that it doesn't always select ROM_END when the values are left at their default values.

Laczen commented 3 years ago

The padding will overwrite data that it should not overwrite.

How will this occur? Padding by definition is only inserted at memory locations where no variables are present to reach addresses where more variables exist.

Let's take a look at a bin file, this is just a set of bytes that are programmed in the flash, there is no jumping or whatever included in a bin file. To jump over a certain area these bytes are filled with 0x00 or 0xff, there is no way to say in a bin file that you should ignore what is already in flash. When you program this the locations are overwritten with 0x00 or 0xff. You can make a small test for this by introducing a storage area (nvs or fcb or ...) before the linker-region area, if there is no overwrite the storage area should not be overwritten.

For a hex file it is different, a hex file contains information about address and value, so these can have a empty area.

IMO partitioning needing to be taken into account and USE_DT_CODE_PARTITION are separate (although related) problems. The storage partition is the prime example, regardless of whether you want to constrain your code to some smaller partition (zephyr,code-partition), it is never a valid configuration for code to flow into the storage partition.

I agree that the storage partition is special and that support is a bit flaky, however it is a partition like any other and should not be treated differently (if all other partitions are ignored, also storage should be ignored).

In short, I don't believe that CONFIG_USE_DT_CODE_PARTITION=n means that all partitions should be ignored. The symbol itself only says that when not selected, offset and size can be set manually. The proposed PR does not change this behavior. The behavior change is that it doesn't always select ROM_END when the values are left at their default values.

That is what USE_DT_CODE_PARTITION=ndoes when no FLASH_SIZE is specified. The correct way to define the partitions for a device where you would only have code and storage would be:

this will generate a image that does not flow into storage.

JordanYates commented 3 years ago

I see the potential problem with the .bin file, but at some point when messing with linker scripts its going to be 'user beware'. The only in-tree use of this I see is the storage partition, which in all boards I've looked at sit at the end of ROM, so it won't cause a problem by default. Its not like this problem doesn't exist currently as well, there is no way of ensuring that memory addresses aren't overwritten as code size grows.

storage may be special in-tree (although I suspect the TI_CCFG stuff could also be swapped to this), but there is nothing special about wanting to place variables at defined addresses in general. I guess the point of this enhancement is to specifically define some partitions as special and treat them differently accordingly.

Unfortunately your proposed solution doesn't play nicely with how bootloaders are enabled. Currently you can convert any application from standalone to bootloader by adding CONFIG_MCUBOOT=y. However AFAIK all the devicetree parsing has already occurred at this point. So the two options are: Leave .dts: Bootloader operation will protect storage, standalone wont Modify .dts: Bootloader won't compile, standalone will protect storage.

JordanYates commented 3 years ago

You can make a small test for this by introducing a storage area (nvs or fcb or ...) before the linker-region area, if there is no overwrite the storage area should not be overwritten.

I should note that the storage area will only be overwritten in this case if variables are actually initialized into the custom memory region. If it's only defined to protect it from being overwritten, no variables will be in that address space so the .bin won't reach that far.

Laczen commented 3 years ago

Unfortunately your proposed solution doesn't play nicely with how bootloaders are enabled. Currently you can convert any application from standalone to bootloader by adding CONFIG_MCUBOOT=y. However AFAIK all the devicetree parsing has already occurred at this point. So the two options are: Leave .dts: Bootloader operation will protect storage, standalone wont Modify .dts: Bootloader won't compile, standalone will protect storage.

It is easily done using a standalone.overlay. IMO the mcuboot partitioning should never have been in the dts and should have been separate overlay files, but hey this is how it is. The partitioning is there to take advantage of and for the storage it should be used properly, no need to create an extra thing to do the same.

JordanYates commented 3 years ago

It is easily done using a standalone.overlay.

How would such a solution look to get correct behavior for samples/subsys/nvs? Over 50 board.standalone.overlay files with the user having to select the right one with west build -b board zephyr/samples/subsys/nvs/ -DDTC_OVERLAY_FILE=board.standalone.overlay?

Regardless, none of this actually solves the original problem of, 'how do I place this variable at this address in the final image'. This requires custom memory regions in the linker, there are no alternatives. The storage partition change was a happy side benefit that is purely an improvement to everyone who completely ignores this new feature.

Laczen commented 3 years ago

Regardless, none of this actually solves the original problem of, 'how do I place this variable at this address in the final image'. This requires custom memory regions in the linker, there are no alternatives. The storage partition change was a happy side benefit that is purely an improvement to everyone who completely ignores this new feature.

Yes, lets get back on the main goal of the PR: on top of the problem of a bin file that would overwrite existing data, there is another problem: how will the method guarantee that the variables are always at the same location ?

But lets focus on the problem you are trying to solve:

JordanYates commented 3 years ago

how will the method guarantee that the variables are always at the same location ?

The purpose of a linker script is to control the location of variables. You guarantee addresses by using sections placed in memory regions. Refer to the linked PR where I add a test that hardcodes the variable address in the twister regex precisely because I can guarantee where they are.

The advantage of having the data in a different memory region is that you can guarantee the locations don't change across application updates. As stated in the issue description, my use case is to compile time instantiate a NVS like system. This is not possible if addresses change across application upgrades.

For another use case, consider the TI HAL layer. Currently linker.ld has several #ifdef CONFIG_HAS_TI_CCFG because this functionality does not currently exist. It needs to place a constant variable at a known location. See https://github.com/zephyrproject-rtos/hal_ti/blob/277d70a65ab14d46bf1ec0935cf9bb28bbaa8ab9/simplelink/source/ti/devices/cc13x2_cc26x2/startup_files/ccfg.c#L520 for this. The special casing for TI could be removed with this functionality.

Laczen commented 3 years ago

The purpose of a linker script is to control the location of variables. You guarantee addresses by using sections placed in memory regions. Refer to the linked PR where I add a test that hardcodes the variable address in the twister regex precisely because I can guarantee where they are.

Good, that this is covered.

The advantage of having the data in a different memory region is that you can guarantee the locations don't change across application updates. As stated in the issue description, my use case is to compile time instantiate a NVS like system. This is not possible if addresses change across application upgrades.

From the upgrade functionality I would conclude that you would like an image where the "region" is separated from the image, this would also solve the problem with the overwriting.

JordanYates commented 3 years ago

From the upgrade functionality I would conclude that you would like an image where the "region" is separated from the image, this would also solve the problem with the overwriting.

This is partly correct, I need both. The initial application flashed at production would contain a base set of values that would be contained in the output .hex/.bin, while future OTA images would not include any data in the memory region.

This is trivial to implement in application land with a Kconfig symbol (CONFIG_PROD_IMAGE or similar) that controls whether those base variables exist in the application image. The memory region would still exist in the OTA image (protecting against code overflowing into the region).

Laczen commented 3 years ago

From the upgrade functionality I would conclude that you would like an image where the "region" is separated from the image, this would also solve the problem with the overwriting.

This is partly correct, I need both. The initial application flashed at production would contain a base set of values that would be contained in the output .hex/.bin, while future OTA images would not include any data in the memory region.

If you have the possibility to separate them you don't need both, you can decide just not to download/upgrade the "region".

It is possible to separate the "region" using objcopy, but I don't know if it is possible to generate an image without the "region", but I guess this should also be possible. This is a question for the tools guys.

This is trivial to implement in application land with a Kconfig symbol (CONFIG_PROD_IMAGE or similar) that controls whether those base variables exist in the application image. The memory region would still exist in the OTA image (protecting against code overflowing into the region).

I don't think the memory region would exist in the OTA image, addresses pointing to it would. Using addresses pointing to the storage area would be another method to achieve the same. I didn't go through your PR , but maybe it is possible to do the following:

For standard OTA: generate without the symbol (use this also for the production image). For Production set the symbol and extract the region.

JordanYates commented 3 years ago

All of this is getting into the specifics of how I will use this functionality, rather than the functionality itself.

I need a way to store variables at a given address, which requires custom memory regions in the linker. Devicetree provides a method of partitioning flash, and devicetree is available in the linker scripts, so the path of least resistance appeared to be the linker-region solution I implemented.

I believe your only outstanding concern is the possibility of .bin files overwriting previously flashed data. Given that the conditions under which that can happen must be explicitly configured by the application developer, I believe sufficient warning can be provided in the documentation to alleviate the concern. We provide the flash_erase API even though a user could erase all of ROM, I would place this in a similar category.

JordanYates commented 3 years ago

You are correct about using objcopy as a better solution than the CONFIG_PROD_IMAGE however. Compile the application with the base variables in, and use objcopy --remove-section .my_custom_nvs zephyr.elf to generate the OTA version without it.

Laczen commented 3 years ago

@JordanYates, maybe unrelated, it is possible to generate a "preloaded" storage partition using a native_posix executable. It is some utility I created long ago and can be found at: https://github.com/Laczen/zephyrmodules/utils/native_posix/settings_preload/

zephyrbot commented 9 months ago

Hi @tejlmand,

This issue, marked as an Enhancement, was opened a while ago and did not get any traction. Please confirm the issue is correctly assigned and re-assign it otherwise.

Please take a moment to review if the issue is still relevant to the project. If it is, please provide feedback and direction on how to move forward. If it is not, has already been addressed, is a duplicate, or is no longer relevant, please close it with a short comment explaining the reason.

@JordanYates you are also encouraged to help moving this issue forward by providing additional information and confirming this request/issue is still relevant to you.

Thanks!