zephyr_code_relocate() now supports the NOCOPY_DATA flag to inhibit data relocation

[rhaberkorn]

This helps when building firmwares for coprocessors, where the image is loaded completely into SRAM in Linux-land using the remoteproc framework.

For a motivation of this change, see #86530.

[github-actions]

Hello @rhaberkorn, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[rhaberkorn]

Force-pushed a new version with the changes requested by @JarmouniA.
I have rebuilt the documentation locally and it looks fine.

[JarmouniA]

We should add NOCOPY_DATA here, no?

zephyr/samples/application_development/code_relocation_nocopy/CMakeLists.txt

Line 12 in bec4a84

zephyr_code_relocate(FILES src/ext_code.c LOCATION EXTFLASH_TEXT NOCOPY)

for the sample to keep its current behavior.

[rhaberkorn]

We should add NOCOPY_DATA here, no?

zephyr/samples/application_development/code_relocation_nocopy/CMakeLists.txt

Line 12 in bec4a84

zephyr_code_relocate(FILES src/ext_code.c LOCATION EXTFLASH_TEXT NOCOPY)

for the sample to keep its current behavior.

No, the patch should not break any existing code. In particular, as far as I understand, this example is for typical execute-inplace-from-flash scenarios, i.e. modern MCU case where you can execute from the builtin flash. But you cannot write individual words in that flash, nor do you want to for most global variables, as it would wear out the flash. So on those devices, you do need to copy the .data segment from flash into SRAM.

Another question/bug might be that Zephyr does not seem to actually copy .data into SRAM if you relocate all segments (e.g. without the _TEXT suffix and without the new NOCOPY_DATA). This is how I actually noticed that something is missing. I wanted to relocate everything into SRAM. In my particular use case you don't actually have to copy .data, but it could still be broken on devices with real XIP flash.

[rhaberkorn]

As I was musing in a previous comment, perhaps we should simply apply NOCOPY to .data sections just like to .text:

diff --git a/scripts/build/gen_relocate_app.py b/scripts/build/gen_relocate_app.py
index 48ef8a4bf95..7778fd663bb 100755
--- a/scripts/build/gen_relocate_app.py
+++ b/scripts/build/gen_relocate_app.py
@@ -404,10 +404,10 @@ def generate_linker_script(linker_file, sram_data_linker_file, sram_bss_linker_f
             gen_string += MPU_RO_REGION_END.format(memory_type.lower())
 
         if region_is_default_ram(memory_type):
-            gen_string_sram_data += string_create_helper(SectionKind.DATA, memory_type, full_list_of_sections, 1, 1, phdrs)
+            gen_string_sram_data += string_create_helper(SectionKind.DATA, memory_type, full_list_of_sections, 1, is_copy, phdrs)
             gen_string_sram_bss += string_create_helper(SectionKind.BSS, memory_type, full_list_of_sections, 0, 1, phdrs)
         else:
-            gen_string += string_create_helper(SectionKind.DATA, memory_type, full_list_of_sections, 1, 1, phdrs)
+            gen_string += string_create_helper(SectionKind.DATA, memory_type, full_list_of_sections, 1, is_copy, phdrs)
             gen_string += string_create_helper(SectionKind.BSS, memory_type, full_list_of_sections, 0, 1, phdrs)
 
     # finally writing to the linker file

That's it, that will also do the trick even without adding any new zephyr_code_relocate() flags. .bss is already mapped properly (is_copy is apparently ignored if load_address_in_flash is 0). I don't see any benefit in keeping the old behavior, i.e. always ignoring NOCOPY for .data. If there is some kind of use case, somebody should speak out. Otherwise, I will perhaps close this PR and open another one with the much simpler patch above.

[tejlmand]

I don't see any benefit in keeping the old behavior, i.e. always ignoring NOCOPY for .data. If there is some kind of use case, somebody should speak out.

@carlocaione you authored the original NOCOPY feature here: d3203dc / PR##42859.
Can you please provide some feedback on this question ?

@rhaberkorn please take a look at the referenced commit / PR, as this provides details on the original use-case.

[rhaberkorn]

@rhaberkorn please take a look at the referenced commit / PR, as this provides details on the original use-case.

The original use case was for XIP flash, so @carlocaione didn't want to apply NOCOPY to .data sections. In the XIP scenario, you still have to copy .data to SRAM, as it must be mutable. The question is however why he didn't pass down the NOCOPY flag for .data sections in the first place. I at least don't see any reason for that.

[github-actions]

This pull request 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 pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

[cfriedt]

@carlocaione - please comment if you have some time.

It would be good to investigate the nocopy option highlighted above as well.

@rhaberkorn - can you make an alternate PR to explore that so we can compare the two?

[rhaberkorn]

@rhaberkorn - can you make an alternate PR to explore that so we can compare the two?

@cfriedt You mean a new PR with this patch?