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

Generate atomic module outside of MSP430 #693

Merged
merged 12 commits into from
Dec 25, 2022

Conversation

YuhanLiin
Copy link
Contributor

Currently the --nightly flag generates atomic register modification code for MSP430. This PR expands this code generation to all architectures.

@rust-highfive
Copy link

r? @reitermarkus

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Nov 1, 2022
@burrbull
Copy link
Member

burrbull commented Nov 1, 2022

  1. It is better to rename flag to atomics (with note in docs that it requires nightly)
  2. The trait is implemented for u8 and u16 only. Other architectures usually support bigger registers.

@burrbull
Copy link
Member

burrbull commented Nov 2, 2022

We don't use signed integers as raw registers.

what about riscv? they can be with/without atomic extention or can have 64 bits

@YuhanLiin
Copy link
Contributor Author

I didn't know we even supported 64-bit registers. I think I can detect it using target_ptr_width.

I'm not sure how to detect the atomic extension. Maybe I can mention it in the documentation and tell users not to use the atomic flag without the extension.

@adamgreig
Copy link
Member

I left this comment on the previous PR but it's probably more useful here:

I haven't compared them in detail, but for other targets it might make more sense to use atomic-polyfill which builds on critical-section and is quite widely used in other embedded Rust projects. portable-atomic can only use a global disable-interrupts to simulate CAS operations (when enabled with --cfg portable_atomic_unsafe_assume_single_core), while atomic-polyfill uses critical-section which allows pluggable implementations (and is used in svd2rust for Peripherals::take()).

At the moment I don't think atomic-polyfill supports MSP430 at all, so it's not an option here (yet?).

Maybe it's possible to add msp430 to atomic-polyfill so one crate can be used by all architectures (@Dirbaio?).

That said, are you sure this is a valid way to operate on MMIO? At least in ARMv7-M, the spec says "LDREX and STREX operations must be performed only on memory with the Normal memory attribute", which doesn't include Device memory for MMIO. In practice on Cortex-M4 it seems like the usual implementation of the global monitor doesn't care about the memory type, so it may end up working despite the spec. The accesses also don't get marked as volatile, so their interaction with the Rust memory model is a bit harder to be sure about, and the compiler might be within its rights to completely optimise out some accesses, either now or in the future. There's some ongoing discussion in these issues.

Overall I think this probably needs a bit more thought before it's generated for other platforms, around whether it even makes sense to polyfill atomics on platforms without them (armv6-m for example) and whether it's even legitimate to just stick some AtomicU32 types onto the MMIO memory map.

@YuhanLiin
Copy link
Contributor Author

For this PR I'm aiming to only add atomic register modification for platforms with instruction-level atomic support. This is why I disabled the fallback feature in portable-atomic. If a target requires a polyfill for atomic operations then the generate atomics code won't even compile, since atomic operations won't be supported by portable-atomic for that platform. This guarantees that the generated atomic code is low-cost.

Are the concerns about MMIO memory type still relevant in this case? I'm not familiar with ARM so I don't know. As for volatile accesses, according to this issue Rust doesn't support memory accesses with both volatile and atomic semantics, though I believe it's achievable using inline ASM with the right flags (like in msp430-atomics).

@YuhanLiin
Copy link
Contributor Author

Ok it turns out that portable-atomics doesn't actually use ASM instructions for most MSP430 atomic operations, but instead uses the interrupt-disable method. This is only an issue for MSP430 and AVR (I don't think we support this one), so it seems like we still need msp430-atomic after all.

@burrbull
Copy link
Member

burrbull commented Nov 4, 2022

@adamgreig what should we do with this PR? Merge or make it draft?

@YuhanLiin YuhanLiin marked this pull request as draft November 4, 2022 14:47
@YuhanLiin
Copy link
Contributor Author

I'm gonna make it into a draft and wait for the next release of msp430-atomic

@YuhanLiin YuhanLiin marked this pull request as ready for review December 25, 2022 02:55
@YuhanLiin
Copy link
Contributor Author

I modified the atomic code to match the newest version of portable-atomic, which supports single-instruction atomics on MSP430. This is now ready for review.

@burrbull
Copy link
Member

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Build succeeded:

@bors bors bot merged commit f206126 into rust-embedded:master Dec 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants