Skip to content

Conversation

@TakuyaMiyasita
Copy link
Contributor

@TakuyaMiyasita TakuyaMiyasita commented Nov 15, 2024

Summary

Some armv7-m-based SoCs do not work with atomic instructions, even though armv7-m supports them.
(About these SoC, please refer 4cec713#commitcomment-148460811)

To avoid using atomic instructions generated by gcc, CONFIG_LIBC_ARCH_ATOMIC is newly introduced with which arch_atomic.c is linked explicitly.

However, the function names need to be changed to avoid build errors, since the functions described in stdatomic.h are gcc built-in and inlined when the code is compiled.

About libcxx with CONFIG_LIBC_ARCH_ATOMIC, it still does not work. It is also needed to call nx_atomic_ ver instead of __atomic ver in libcxx/include/__atomic/cxx_atomic_lmpl.h.

Impact

ARCH_CHIP_CXD32XX

Testing

  • ARCH_CHIP_CXD32XX : The build is passed and ostest is passed, but the board is out-of-tree.
  • raspberrypi-pico:nsh : The build is passed.
  • spresense:smp : The build is passed.
  • rv-virt:libcxx : The build is passed.
  • esp32c3-generic:nsh : The build is passed.
  • esp32h2-devkit:nsh : The build is passed.
  • esp32s3-devkit:cxx : The build is passed.

tool-chain version

arm-none-eabi-gcc (Arm GNU Toolchain 13.2.rel1 (Build arm-13.7)) 13.2.1 20231009
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
riscv-none-elf-gcc (xPack GNU RISC-V Embedded GCC x86_64) 13.2.0
Copyright (C) 2023 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
xtensa-esp32s3-elf-gcc (crosstool-NG esp-12.2.0_20230208) 12.2.0
Copyright (C) 2022 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Some armv7-m-based SoCs do not work with atomic instructions,
even though armv7-m supports them.

To avoid using atomic instructions generated by gcc,
CONFIG_LIBC_ARCH_ATOMIC is newly introduced with which
arch_atomic.c is linked explicitly.

However, the function names need to be changed to avoid
build errors, since the functions described in stdatomic.h
are gcc built-in and inlined when the code is compiled.

About libcxx with CONFIG_LIBC_ARCH_ATOMIC, it still
does not work. It is also needed to call nx_atomic_ ver
instead of __atomic ver in
libcxx/include/__atomic/cxx_atomic_lmpl.h.

Signed-off-by: Takuya Miyasita <Takuya.Miyashita@sony.com>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 15, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 15, 2024

[Experimental Bot, please feedback here]

The PR summary provided is insufficient and lacks crucial information required by the NuttX guidelines. Here's a breakdown of the issues and a suggested improvement:

Missing/Insufficient Information:

  • Summary - Why is this change necessary? The summary mentions some ARMv7-M SoCs don't work with atomic instructions despite supporting them. This needs more detail. Why don't they work? Is it a silicon bug? A compiler issue? What are the specific symptoms of failure? What problems does this PR solve for users?
  • Summary - What changes are being made? The summary is vague. Mentioning CONFIG_LIBC_ARCH_ATOMIC is introduced and function names changed is not enough. Which functions are renamed? How does this new config option affect the build? How does it interact with existing configurations? Be specific.
  • Summary - How does the change work? Explain at a high level how introducing this config option and renaming functions resolves the atomic instruction issue.
  • Impact - Is this a new feature? Yes, CONFIG_LIBC_ARCH_ATOMIC is a new configuration option. This needs to be explicitly stated.
  • Impact - User impact? While the PR states ARCH_CHIP_CXD32XX is impacted, it doesn't say how. Will users need to enable this new config option? Will it be enabled by default for CXD32XX? What changes are required in user applications? Provide clear instructions for users of affected architectures.
  • Impact - Build impact? Yes, there's a build impact. Describe it in more detail. Adding a new Kconfig option changes the build process. How are dependencies handled? How is arch_atomic.c now included in the build based on this config option?
  • Impact - Hardware impact? Yes, there is an impact. Specify which architectures/boards are directly affected. While you list several in the testing section, explicitly state them in the Impact section. Explain how they are affected.
  • Impact - Documentation impact? Does this change require documentation updates? If so, confirm that they are included in the PR. If not, explain why.
  • Impact - Security impact? Even if there's no security impact, explicitly state "NO".
  • Impact - Compatibility impact? Explicitly state whether there are any backward compatibility issues.
  • Testing - "Works as intended" is too vague. What was the intended outcome? What tests were performed to validate it? The listed test configurations are helpful, but the logs are missing. Provide relevant log snippets demonstrating the issue before the change and the successful operation after the change.

Suggested Improved Summary:

Summary

This PR addresses an issue where atomic instructions fail on certain ARMv7-M SoCs (specifically, family X, model Y, etc. - list affected SoCs), despite being architecturally supported. This failure manifests as [describe the specific symptoms of the failure, e.g., data corruption, system crashes, etc.]. The root cause is [explain the root cause - silicon errata, compiler bug, etc.].

To resolve this, a new Kconfig option CONFIG_LIBC_ARCH_ATOMIC is introduced. Enabling this option forces the use of software-emulated atomic operations provided by arch_atomic.c, bypassing the faulty hardware instructions. This requires renaming the affected functions in arch_atomic.c from __atomic_* to nx_atomic_* to avoid conflicts with GCC built-in atomics. This also addresses compatibility issues with libcxx by using nx_atomic_* versions in libcxx/include/__atomic/cxx_atomic_lmpl.h when CONFIG_LIBC_ARCH_ATOMIC is enabled.

Fixes [NuttX Issue # if applicable].

Impact

  • New Feature: YES. CONFIG_LIBC_ARCH_ATOMIC configuration option is added.
  • User Impact: YES. Users with affected SoCs (specifically, list them) must enable CONFIG_LIBC_ARCH_ATOMIC in their .config file to utilize the software atomic emulation. [Provide clear instructions on how to enable the option and any potential impact on application code].
  • Build Impact: YES. The build system now conditionally compiles and links arch_atomic.c based on the CONFIG_LIBC_ARCH_ATOMIC setting. [Explain any Kconfig dependencies added.]
  • Hardware Impact: YES. This change specifically targets [list affected architectures and boards]. Enabling the new config option provides a workaround for the atomic instruction issue on these platforms.
  • Documentation Impact: YES/NO. [Explain what documentation is updated/added or why no documentation update is needed.]
  • Security Impact: NO.
  • Compatibility Impact: [Explain any potential backward compatibility concerns or explicitly state NO.]
  • Other Considerations: [Mention any other relevant points, such as future work or related issues.]

Testing

I confirm that changes are verified on a local setup and work as intended. The intended outcome was to enable correct operation of atomic instructions on the affected platforms by utilizing software emulation. The following tests were performed:

  • Build Host: [OS, CPU, Compiler details]
  • Targets:
    • ARCH_CHIP_CXD32XX:nsh (Build passes, ostest passes). Note: This board is out-of-tree.
    • ... (Other tested targets)

Testing Logs (Before Change - ARCH_CHIP_CXD32XX):

[Relevant log snippets demonstrating the failure, e.g., data corruption, incorrect values]

Testing Logs (After Change - ARCH_CHIP_CXD32XX):

[Relevant log snippets demonstrating the corrected behavior, e.g., correct data values, successful operation]

...(Repeat log examples for other tested platforms, showing the problem before and the solution after).

By providing more detail and clarity, the PR will be much easier for reviewers to understand and approve. Remember to always provide specific information and demonstrate the problem and solution with concrete examples.

@xiaoxiang781216
Copy link
Contributor

@tmedicci @masayuki2009 please verify this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Area: OS Components OS Components issues Size: L The size of the change in this PR is large

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants