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

RISC-V: Make atomic emulation opt-in #904

Merged
merged 5 commits into from
Nov 10, 2023
Merged

RISC-V: Make atomic emulation opt-in #904

merged 5 commits into from
Nov 10, 2023

Conversation

bugadani
Copy link
Contributor

@bugadani bugadani commented Nov 6, 2023

Thank you!

Thank you for your contribution.
Please make sure that your submission includes the following:

Must

  • The code compiles without errors or warnings.
  • All examples work.
  • cargo fmt was run.
  • Your changes were added to the CHANGELOG.md in the proper section.
  • You updated existing examples or added examples (if applicable).
  • Added examples are checked in CI

Nice to have

  • You add a description of your work to this PR.
  • You added proper docs for your newly added features and code.

Let's try to make portable-atomic work by default and demote atomic emulation traps on risc-v. Xtensa is left untouched because, due to a happy accident, it works with atomic-polyfill and trying to remove emulation causes a bunch of problems in multiple crates.

I've updated CI so Clippy now runs on the target folder, using the correct target. This is required because otherwise portable-atomic doesn't allow enabling the unstable-assume-single-core feature - which is annoying, but fair when clippy wants to target the host architecture.

The current idea is, that neither atomic emulation, nor portable-atomic is enabled by default. The portable-atomic feature in esp-hal isn't very useful for RISC-V, but we may (barring ULP...) enable unsafe-assume-single-core or other default features. Besides that, I've kept the feature just to detect mutual exclusivity with the emulation trap. I may be overthinking this part, though, I'm not sure.

@bugadani bugadani force-pushed the atomic branch 6 times, most recently from b8a800a to 95f24bd Compare November 6, 2023 16:29
@bugadani bugadani changed the title Make atomic emulation opt-in RISC-V: Make atomic emulation opt-in Nov 6, 2023
@bugadani bugadani force-pushed the atomic branch 4 times, most recently from bace211 to 12aa3ba Compare November 6, 2023 20:13
toolchain: stable
components: clippy
- uses: Swatinem/rust-cache@v2

# Run clippy on all packages targeting RISC-V.
- name: clippy (esp-riscv-rt)
run: cargo +stable clippy --manifest-path=esp-riscv-rt/Cargo.toml -- -D warnings
run: cd esp-riscv-rt && cargo +stable clippy -- -D warnings
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NGL I like the idea of checking code using the host architecture less and less

@bugadani bugadani force-pushed the atomic branch 5 times, most recently from 7d408fd to 4e57335 Compare November 7, 2023 11:48
@bugadani bugadani marked this pull request as ready for review November 8, 2023 11:22
@bugadani bugadani force-pushed the atomic branch 5 times, most recently from 6887325 to e604bf8 Compare November 8, 2023 12:07
@bugadani bugadani marked this pull request as draft November 8, 2023 12:14
@bugadani bugadani force-pushed the atomic branch 2 times, most recently from e2119fd to 27783b1 Compare November 8, 2023 13:25
@bugadani bugadani marked this pull request as ready for review November 8, 2023 13:26
@bugadani bugadani force-pushed the atomic branch 2 times, most recently from cca3ad6 to 199b550 Compare November 8, 2023 13:57
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 8, 2023

I cannot comment on that line but in https://github.com/bugadani/esp-hal/blob/199b550774091788556b209e14b028259e983560/esp-hal-common/src/interrupt/riscv.rs#L467
we need to make sure this evaluates to false if we don't have atomic emulation enabled since otherwise we won't go into the defined exception handler

@bugadani bugadani force-pushed the atomic branch 2 times, most recently from 7ee8e7d to f4ac3ca Compare November 8, 2023 14:13
@bugadani
Copy link
Contributor Author

bugadani commented Nov 8, 2023

I cannot comment on that line but in https://github.com/bugadani/esp-hal/blob/199b550774091788556b209e14b028259e983560/esp-hal-common/src/interrupt/riscv.rs#L467 we need to make sure this evaluates to false if we don't have atomic emulation enabled since otherwise we won't go into the defined exception handler

Thanks, fixed (I think)

@bugadani bugadani force-pushed the atomic branch 3 times, most recently from 7afe9ae to e995570 Compare November 9, 2023 19:24
@bjoernQ
Copy link
Contributor

bjoernQ commented Nov 10, 2023

Looks good to me

One thing though (w/o atomic emulation)

warning: unused variable: `pc`
   --> C:\projects\review\bugadani\esp-hal\esp-hal-common\src\interrupt\riscv.rs:465:28
    |
465 | unsafe fn handle_exception(pc: usize, trap_frame: *mut TrapFrame) {
    |                            ^^ help: if this is intentional, prefix it with an underscore: `_pc`
    |
    = note: `#[warn(unused_variables)]` on by default

@bugadani
Copy link
Contributor Author

Looks good to me

One thing though (w/o atomic emulation)

warning: unused variable: `pc`
   --> C:\projects\review\bugadani\esp-hal\esp-hal-common\src\interrupt\riscv.rs:465:28
    |
465 | unsafe fn handle_exception(pc: usize, trap_frame: *mut TrapFrame) {
    |                            ^^ help: if this is intentional, prefix it with an underscore: `_pc`
    |
    = note: `#[warn(unused_variables)]` on by default

Thanks, I cleaned up and drive-by deleted the #[doc(hiddden)] which isn't needed on a private fn.

Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks for fighting your way through this! LGTM

@MabezDev MabezDev merged commit 280caad into esp-rs:main Nov 10, 2023
17 checks passed
@bugadani bugadani deleted the atomic branch November 10, 2023 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants