[skeleton] ESP32: do not assume CAS, always use critical-section for parts of the address space #225
Conversation
| macro_rules! dispatch_impl { | ||
| ($self:expr, $atomic:expr, $fallback:expr) => { | ||
| { | ||
| let addr = $self as *const _ as usize; | ||
| if addr >= EXTERNAL_DATA_BUS_ADDRESS_RANGE.start && addr < EXTERNAL_DATA_BUS_ADDRESS_RANGE.end { | ||
| return $fallback; | ||
| } else { | ||
| return $atomic; | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
This is the core of the idea: the atomic types in this module wrap the critical-section based types, and will conditionally call either assembly code that uses the S32C1I instruction, or the inner methods.
| esp-println = { default-features = false, features = ["uart", "esp32s2"], git = "https://github.com/taiki-e/esp-hal.git", rev = "293a19f" } | ||
| esp-hal = { features = ["esp32s2"], git = "https://github.com/taiki-e/esp-hal.git", rev = "293a19f" } | ||
|
|
||
| [target.xtensa-esp32s3-none-elf.dependencies] |
There was a problem hiding this comment.
I would add tests for the ESP32, too, but they don't build with the fixed esp-hal revision with atomic CAS disabled.
taiki-e
left a comment
taiki-e
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'm basically on board with this approach.
I'm opening this PR because I'd like to make the necessary structural changes before bloating this up with a good bit of assembly.
I thought the implementation uses core::sync::atomic instead of a handwritten assembly.
If it is decided to disable atomic types in core::sync::atomic, assembly is indeed the only way, However, since the xtensa-*-espidf targets are not no-std, it's unclear whether that is realistic.
When writing assemblies, I think it's good to base them on atomic-maybe-uninit's, but for now, I think implementations using core::sync::atomic are also fine.
| "xtensa" => { | ||
| // https://github.com/rust-lang/rust/pull/93868 merged in Rust 1.60 (nightly-2022-02-13). | ||
| if is_allowed_feature("asm_experimental_arch") { | ||
| println!("cargo:rustc-cfg=portable_atomic_unstable_asm_experimental_arch"); |
There was a problem hiding this comment.
xtensa is tier 3 platform, so this is unneeded.
There was a problem hiding this comment.
I don't know if I understand what you're telling me here. Are you saying I can replace portable_atomic_unstable_asm_experimental_arch with target_arch = "xtensa" in code? I would prefer not adding more conditionals than I absolutely have to.
| pub(crate) fn is_lock_free() -> bool { | ||
| <crate::imp::interrupt::$atomic_type$(<$($generics)*>)?>::is_lock_free() | ||
| } | ||
| pub(crate) const IS_ALWAYS_LOCK_FREE: bool = <crate::imp::interrupt::$atomic_type$(<$($generics)*>)?>::IS_ALWAYS_LOCK_FREE; |
There was a problem hiding this comment.
I feel this should be always false.
There was a problem hiding this comment.
I'm not entirely sure why - whether the overall wrapped object is lock free depends on the inner implementation. If you consider the interrupt-free implementation for unsafe-assume-single-core to be lock-free, and we allow falling back to that implementation, this one will be lock-free.
|
|
||
| #[inline] | ||
| pub(crate) fn is_lock_free() -> bool { | ||
| <crate::imp::interrupt::$atomic_type$(<$($generics)*>)?>::is_lock_free() |
Co-authored-by: Taiki Endo <[email protected]>
taiki-e
force-pushed
the
main
branch
9 times, most recently
from
307c702 to
0fdd05b
Compare
2 participants
This is the skeleton of my proposal to close esp-rs/esp-hal#2027. These CPUs will need
critical-sectionto be enabled, but portable-atomic will implement the atomic CAS-based algorithms where they are working correctly. The CPU is detected from the target triple, and injected ascfg(portable_atomic_target_cpu = "...")so no cihp-specific feature or manual cfg is necessary.I'm opening this PR because I'd like to make the necessary structural changes before bloating this up with a good bit of assembly.
The test compiles with both the upstream targets, as well as the following custom JSON, as long as it's called
xtensa-esp32s3-none-elf.json.{ "arch": "xtensa", "atomic-cas": false, "cpu": "esp32s3", "crt-objects-fallback": "false", "data-layout": "e-m:e-p:32:32-v1:8:8-i64:64-i128:128-n32", "emit-debug-gdb-scripts": false, "linker": "xtensa-esp32s3-elf-gcc", "linker-flavor": "gnu-cc", "llvm-target": "xtensa-none-elf", "max-atomic-width": 32, "metadata": { "description": "Xtensa ESP32-S3", "host_tools": false, "std": false, "tier": 3 }, "panic-strategy": "abort", "relocation-model": "static", "target-pointer-width": "32", "vendor": "espressif" }