Add critical-section 1.0 implementation, fix multicore unsoundness. #110

Dirbaio · 2022-08-21T22:05:37Z

~~Requires #109~~

This adds a critical-section implementation for single-core chips, based on disabling all interrupts.

interrupt::free is is unsound on multicore systems because it only disables interrupts in the
current core. For multicore chips, a chip-specific critical section implementationis needed instead. Unsoundness is fixed by not returning the CriticalSection token.

This is a breaking change.

This is the riscv equivalent of rust-embedded/cortex-m#447 and rust-embedded/cortex-m#448

dkhayes117

Thank you! Looks good. but I will defer to disasm for final review.

Disasm · 2022-08-22T05:14:53Z

Cargo.toml

 [dependencies]
 bare-metal = "1.0.0"
 bit_field = "0.10.0"
+critical-section = "1.1.0"


Make it optional maybe?

It's required unconditionally by the singleton! macro

(and the macro can still be used if critical-section-single-core is not enabled, if the user supplies a c-s implementation from somewhere else, such as a chip-specific impl for a multicore chip, from its HAL crate)

Disasm · 2022-08-22T05:21:08Z

The goal is to release it in 0.8.x so critical-section becomes usable without having to wait for 0.9.

Next release will be "breaking" anyways because one breaking change is already merged. I'd prefer to have this change together with other critical-section-related changes.

Dirbaio · 2022-08-22T13:01:13Z

Next release will be "breaking" anyways because one breaking change is already merged. I'd prefer to have this change together with other critical-section-related changes.

Oh okay, hadn't noticed that. I've folded PR #111 into this one.

651: Use `critical_section` for `Peripherals::take`. r=therealprof a=reitermarkus - `cortex_m` rust-embedded/cortex-m#447 - `msp430` rust-embedded/msp430#13 - `riscv` rust-embedded/riscv#110 - `xtensa_lx` esp-rs/xtensa-lx#20, esp-rs/esp-hal#151 - `mips_mcu` kiffie/pic32-rs#5 Co-authored-by: Markus Reiter <me@reitermark.us>

almindor · 2022-10-12T16:01:45Z

@Dirbaio sorry I complicated this a bit, we released 0.9 due to a botched 0.8.1 (which contained some other breaking changes and we had to yank it). Could you please bump the changelog? I think that's the only blocker here.

Dirbaio · 2022-10-12T19:12:24Z

Rebased

almindor

LGTM, but I think we might want to look at core vs hart terminlogy and meaning before proceeding.

almindor · 2022-10-12T19:49:02Z

src/interrupt.rs


-/// Disables all interrupts
+/// Disables all interrupts in the current core.


I wonder if we should specify what core means in RISC-V context here. e.g. is it a hard or an actual core with its own fetching unit.

almindor · 2022-10-12T19:49:56Z

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

 ## [Unreleased]

+### Added
+
+- Added `critical-section-single-core` feature which provides an implementation for the `critical_section` crate for single-core systems, based on disabling all interrupts.


Same as below, I wonder if this should be critical-section-single-hart instead. TBF I'm not quite sure on this myself, I didn't use interrupts much yet, will review the spec a bit better.

Dirbaio · 2022-10-12T19:56:05Z

I'm afraid I don't know enough about riscv to know what's the difference between "core" and "hart" so I'll do whatever you guys choose :)

almindor · 2022-10-12T20:08:59Z

I'm afraid I don't know enough about riscv to know what's the difference between "core" and "hart" so I'll do whatever you guys choose :)

Heh sadly neither do I. My current understanding is that a core is a 1+ hard (hardware thread) that has its own fetch instructions mechanism. A hard is a separate execution environment but it does not include the fetcher.

Interrupts seem to be mixed between these two, some seem to be core-wide and some seem to be hard-specific but maybe I'm misunderstanding this. I'll try to get a better understanding first so we don't have to fix some UB later.

FawazTirmizi · 2022-10-12T22:02:08Z

In this case, I think hart is the correct term to use over core. The acquire() and release() functions disable/enable interrupts for the hart by flipping mstatus::mie.

Interrupts seem to be mixed between these two, some seem to be core-wide and some seem to be hard-specific but maybe I'm misunderstanding this.

As far as I am aware, there is nothing ratified that defines "core-wide" interrupts, although an interrupt controller like the PLIC could be configured to route a set of interrupts to all of the harts on a core. This eventually may not always be true, as Section 2 of the Smclic extension does say:

While the current CLIC provides only hart-local interrupt control, future additions might also support directing interrupts to harts within a core, hence the name ...

However, there isn't any further mention of this in the spec so far.

It's entirely possible that I've missed something here; there are a lot of specs relating to interrupts.

almindor · 2022-10-12T23:13:45Z

In this case, I think hart is the correct term to use over core. The acquire() and release() functions disable/enable interrupts for the hart by flipping mstatus::mie.

Interrupts seem to be mixed between these two, some seem to be core-wide and some seem to be hard-specific but maybe I'm misunderstanding this.

As far as I am aware, there is nothing ratified that defines "core-wide" interrupts, although an interrupt controller like the PLIC could be configured to route a set of interrupts to all of the harts on a core. This eventually may not always be true, as Section 2 of the Smclic extension does say:

While the current CLIC provides only hart-local interrupt control, future additions might also support directing interrupts to harts within a core, hence the name ...

However, there isn't any further mention of this in the spec so far.

It's entirely possible that I've missed something here; there are a lot of specs relating to interrupts.

I think you are correct, we should probably rename things to be hart specific.

This is unsound on multi-hart because it only disables interrupts in the current hart. For multi-hart chips, a chip-specific critical section implementation is needed instead. Unsoundness is fixed by not returning the `CriticalSection` token. This is a breaking change.

Dirbaio · 2022-10-12T23:38:19Z

Changed core -> hart.

almindor

Thank you!

almindor · 2022-10-13T00:42:40Z

bors r+

bors · 2022-10-13T00:46:36Z

Build succeeded:

110: fix: avoid rust-analyzer snake case warning r=almindor a=sethp It seems that rust-analyzer needs to operate over the expanded text of the proc macro (in order to e.g. support completion in the function body, see #11014 for way more details), so it "sees" the non-snake-case name emitted by riscv-rt's `entry` here. Without this change, rust-analyzer will show a "weak warning" on invocations of `#[entry]` with the text: ``` Function `__risc_v_rt__main` should have snake_case name, e.g. `__risc_v_rt_main` ``` Co-authored-by: sethp <seth@codecopse.net>

Dirbaio requested a review from a team as a code owner August 21, 2022 22:05

Dirbaio force-pushed the cs branch from 5f1949b to 8429f8c Compare August 21, 2022 22:16

This was referenced Aug 21, 2022

Fix interrupt::free() unsoundness on multicore systems. #111

Closed

Use critical-section 1.0 for locking. probe-rs/rtt-target#28

Merged

dkhayes117 reviewed Aug 21, 2022

View reviewed changes

dkhayes117 previously approved these changes Aug 21, 2022

View reviewed changes

Disasm reviewed Aug 22, 2022

View reviewed changes

Dirbaio dismissed dkhayes117’s stale review via 1b74874 August 22, 2022 13:01

Dirbaio force-pushed the cs branch from 8429f8c to 1b74874 Compare August 22, 2022 13:01

Dirbaio changed the title ~~Add implementation for critical-section 1.0 for single-core chips.~~ Add critical-section 1.0 implementation, fix multicore unsoundness. Aug 22, 2022

Dirbaio force-pushed the cs branch from 1b74874 to 15e3016 Compare August 22, 2022 18:27

reitermarkus mentioned this pull request Aug 24, 2022

Use critical_section for Peripherals::take. rust-embedded/svd2rust#651

Merged

Dirbaio force-pushed the cs branch from 15e3016 to 6d2ddc9 Compare October 12, 2022 19:12

almindor reviewed Oct 12, 2022

View reviewed changes

Dirbaio added 2 commits October 13, 2022 01:35

Add implementation for critical-section 1.0 for single-hart chips.

65db20e

Dirbaio force-pushed the cs branch from 6d2ddc9 to caec777 Compare October 12, 2022 23:37

almindor approved these changes Oct 13, 2022

View reviewed changes

bors bot merged commit d9c6076 into rust-embedded:master Oct 13, 2022

tana mentioned this pull request May 5, 2023

Use critical section instead of disabling interrupts rp-rs/rp2040-pac#72

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add critical-section 1.0 implementation, fix multicore unsoundness. #110

Add critical-section 1.0 implementation, fix multicore unsoundness. #110

Dirbaio commented Aug 21, 2022 •

edited

dkhayes117 left a comment

Disasm Aug 22, 2022

Dirbaio Aug 22, 2022

Disasm commented Aug 22, 2022

Dirbaio commented Aug 22, 2022 •

edited

almindor commented Oct 12, 2022

Dirbaio commented Oct 12, 2022

almindor left a comment

almindor Oct 12, 2022

almindor Oct 12, 2022

Dirbaio commented Oct 12, 2022

almindor commented Oct 12, 2022 •

edited

FawazTirmizi commented Oct 12, 2022

almindor commented Oct 12, 2022

Dirbaio commented Oct 12, 2022

almindor left a comment

almindor commented Oct 13, 2022

bors bot commented Oct 13, 2022


		/// Disables all interrupts
		/// Disables all interrupts in the current core.

Add critical-section 1.0 implementation, fix multicore unsoundness. #110

Add critical-section 1.0 implementation, fix multicore unsoundness. #110

Conversation

Dirbaio commented Aug 21, 2022 • edited

dkhayes117 left a comment

Choose a reason for hiding this comment

Disasm Aug 22, 2022

Choose a reason for hiding this comment

Dirbaio Aug 22, 2022

Choose a reason for hiding this comment

Disasm commented Aug 22, 2022

Dirbaio commented Aug 22, 2022 • edited

almindor commented Oct 12, 2022

Dirbaio commented Oct 12, 2022

almindor left a comment

Choose a reason for hiding this comment

almindor Oct 12, 2022

Choose a reason for hiding this comment

almindor Oct 12, 2022

Choose a reason for hiding this comment

Dirbaio commented Oct 12, 2022

almindor commented Oct 12, 2022 • edited

FawazTirmizi commented Oct 12, 2022

almindor commented Oct 12, 2022

Dirbaio commented Oct 12, 2022

almindor left a comment

Choose a reason for hiding this comment

almindor commented Oct 13, 2022

bors bot commented Oct 13, 2022

Dirbaio commented Aug 21, 2022 •

edited

Dirbaio commented Aug 22, 2022 •

edited

almindor commented Oct 12, 2022 •

edited