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

64 bit: Initial support for Linux x86-64 native. #19890

Closed
wants to merge 3 commits into from

Conversation

fzi-haxel
Copy link
Contributor

Contribution description

Initial support for 64 bit in the kernel, pkgs, examples, tests and the native board.

Why add 64 bit support?

As mentioned in the FAQ on www.riot-os.org, there are some benefits

  • Improve RIOT’s code base by fixing 64 bit related errors
  • Simplify toolchain setup for native
    • Users can compile native without having to install additional 32 bit libraries

The FAQ also mentions some cons

  • Effort / gain (as it’s unlikely we will support any other 64 bit platform, fixing potential 64 bit errors in RIOT is less beneficial)
    • The effort for basic 64 bit support was not very large. The kernel itself is quite well written, as it already supports different architectures. The biggest difference in 64 bit is that sizeof(void*) == sizeof(size_t) != sizeof(int), which leads to quite a few compiler warnings in printf formatting and casting between integer values and pointers. This is especially true for the tests. Other than that, and 64 bit support for the native board, the rest of the changes are fairly minor. I also tried to change as little as possible to avoid breaking compatibility with the other boards. See the commit messages for more details.
    • I agree that RIOT should focus on smaller architectures. However, I don't think this has to prevent RIOT from also supporting other 64 bit boards, such as riscv64-based ones.
  • Decreased 32 bit testing (unless we make 32 the default in which case the toolchain setup argument is diminished)
    • I definitely agree that 32 bit native should remain a default platform for testing, especially in the CI. But I don't see much of a downside if a user tests their application with 64 bit native (especially if it's not for a PR to the RIOT repo).

Native for Linux x86-64

For the initial version, instead of creating a separate native64 board, I decided to add the variable NATIVE_64BIT to the native board Makefiles. Setting the environment to NATIVE_64BIT=y will enable 64-bit mode for native, which is disabled by default.

While I don't necessarily think this is the best long-term solution, it does ensure that all tests run with the same configuration as the 32 bit version. A separate board would require a major refactoring of many tests, which often have different configurations in the Makefile, C code, and Python tests if the board is native, and I didn't want to increase the size of an already large pull request.

Almost all of the same features as the 32 bit version are supported by the 64 bit version, but the following are still missing

  • Architectures other than x86-64 or operating systems other than Linux
    • No FreeBSD support
    • No Aarch64 support
  • Rust support for x86-64

Some closing thoughts

This PR is a suggestion for initial 64 bit support, feel free to suggest any changes.
I thought about making several smaller PRs, but that would have made it difficult to test the 64 bit part.
So I decided to go for the big bang solution. If you have a better suggestion, I'm open to restructuring the PR.

DISCLAMER: While I did a more thorough review of the kernel, I did not do a thorough review of the entire code base. I mainly fixed compilation or testing errors that came up during testing. I am sure I have missed at least some DEBUG/printf formatting issues. There is also some optimization potential for 64 bit, but I focused on minimal changes.

Testing procedure

Just set the NATIVE_64BIT=y environment variable and compile something for native.

For example
NATIVE_64BIT=y ./dist/tools/compile_and_test_for_board/compile_and_test_for_board.py . native

NOTE:

  • Setting NATIVE_64BIT=y will skip the unsupported Rust examples and tests. Other than that, nothing else should be skipped.
  • 64 bit native may require additional dependencies. The CI is missing libsdl2-dev for 64 bit.

Issues/PRs references

See also #6603

@github-actions github-actions bot added Platform: native Platform: This PR/issue effects the native platform Area: network Area: Networking Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: pkg Area: External package ports Area: drivers Area: Device drivers Area: timers Area: timer subsystems Area: LoRa Area: LoRa radio support Area: boards Area: Board ports Area: OTA Area: Over-the-air updates Area: cpu Area: CPU/MCU ports Area: sys Area: System Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Aug 17, 2023
@benpicco benpicco requested a review from dylad August 21, 2023 11:06
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

If you move the preparatory patches (64 bit safety compile fixes) to a separate PR we can merge those quickly.

For changes to third party packages, please try to also make a PR upstream so we can drop the downstream patch next time we update the pkt version.

Then you'd only have changes to native left in this PR, which makes review easier (and reduces the chance of merge conflicts).

kv->len = kv_hdr.len;

- if (kv->len == ~0UL || kv->len > db_max_size(db) || kv->len < KV_HDR_DATA_SIZE) {
+ if (kv->len == UINT32_MAX || kv->len > db_max_size(db) || kv->len < KV_HDR_DATA_SIZE) {
Copy link
Contributor

Choose a reason for hiding this comment

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

upstream is pretty responsive when it comes to patches, you can just open a PR there and we can update the commit hash.

{
(void) ccnl;
- DEBUGMSG(DEBUG, "Received something of size %u for the application\n", c->pkt->contlen);
+ DEBUGMSG(DEBUG, "Received something of size %u for the application\n", (unsigned)c->pkt->contlen);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can also move that upstream

@OlegHahm
Copy link
Member

Is this based on #13009 or a complete rework?

@fzi-haxel
Copy link
Contributor Author

Is this based on #13009 or a complete rework?

No, it's technically based on this fork that we did a while ago.

@MrKevinWeiss
Copy link
Contributor

probably this won't make it into the release.

@@ -38,4 +44,6 @@ BOARD_INSUFFICIENT_MEMORY := \
stm32l0538-disco \
stm32mp157c-dk2 \
yunjia-nrf51822 \
waspmote-pro \
z1 \
Copy link
Member

Choose a reason for hiding this comment

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

How are these changes related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While replacing most of FEATURES_REQUIRED += arch_32bit with FEATURES_REQUIRED_ANY += arch_32bit|arch_64bit to make sure the same tests are run for 64bit, I found this one unnecessary.
So I deleted it instead of adding arch_64bit. But good point, this could also be its own PR.

@fzi-haxel
Copy link
Contributor Author

fzi-haxel commented Jan 15, 2024

I changed the PR to WiP and updated the commits so people can still test 64bit native.

Updates include:

  • Updated to current discussion in Preparations to make RIOT 64-bit ready #20154
  • Rebased to master
  • Some cleanup in the individual commits and commit messages
  • Added a temporary commit for the pkg/tinydtls patch, which will hopefully be merged upstream soon.

@github-actions github-actions bot removed Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: drivers Area: Device drivers Area: timers Area: timer subsystems Area: sys Area: System labels Jan 17, 2024
Fixed compilation errors. Mostly DEBUG/printf formatting and void pointer casting.

Other changes are:
* net/gnrc_sixlowpan_frag_*: Generalized packet size calculation
* cpu/native_backtrace: Reduced required backtrace size to 3 for 64-bit
* periph/flashpage: Simplified test
* unittests/tests-pktbuf: Generalized alignment
* sys/architecture: Extended test for 64-bit
Initial version to test 64 bit compatibility.

Instead of a separate board, 64 bit for Linux/x86_64 is enabled by setting the environment variable `NATIVE_64BIT=y` and compiling as usual.
While I personally prefer a separate `native64` board, this ensures that all tests run with the same configuration as the 32 bit version.
A separate board would require a major refactoring of many tests, which often have special behavior for the native board,
and I didn't want to increase the size of an already large pull request.

Not currently implemented:
* Architectures other than x86_64 or operating systems other than Linux
    * No FreeBSD support
    * No Aarch support
* Rust support for x86_64
@fzi-haxel
Copy link
Contributor Author

Closing this PR. The PRs above have been merged.

@fzi-haxel fzi-haxel closed this Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: cpu Area: CPU/MCU ports Area: doc Area: Documentation Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration Area: pkg Area: External package ports Area: tests Area: tests and testing framework Platform: native Platform: This PR/issue effects the native platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants