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

interrupt: Support no-std pre-v6 ARM targets #28

Merged
merged 2 commits into from
Aug 13, 2022
Merged

interrupt: Support no-std pre-v6 ARM targets #28

merged 2 commits into from
Aug 13, 2022

Conversation

taiki-e
Copy link
Owner

@taiki-e taiki-e commented Aug 9, 2022

Closes #26

This currently disables only IRQs. This is explained in the document on safety requirements:

  • On pre-v6 ARM, this currently disables only IRQs.
    Enabling this cfg in an environment where FIQs must also be disabled is also considered unsound.

I have a few questions:

@taiki-e taiki-e added the O-arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state label Aug 9, 2022
@taiki-e taiki-e force-pushed the armv4t-interrupt branch 2 times, most recently from 5937598 to b1ff9ec Compare August 13, 2022 11:58
@taiki-e
Copy link
Owner Author

taiki-e commented Aug 13, 2022

I think it is okay that we only disable the I bit because in some environments the F bit seems not to be able to be changed. Also, it seems that only IRQ is used in GBA.

@taiki-e taiki-e marked this pull request as ready for review August 13, 2022 14:36
@taiki-e
Copy link
Owner Author

taiki-e commented Aug 13, 2022

bors r+

Test (with mGBA) passed locally. (I also tested that the implementation around disabling interrupts is working using the gba crate example)

mgba

@bors
Copy link
Contributor

bors bot commented Aug 13, 2022

@bors bors bot merged commit 5511650 into main Aug 13, 2022
@bors bors bot deleted the armv4t-interrupt branch August 13, 2022 18:05
@Lokathor
Copy link

Your readme should likely note that this does not work in User mode.

@taiki-e
Copy link
Owner Author

taiki-e commented Aug 15, 2022

https://github.com/taiki-e/portable-atomic#optional-cfg

  • This uses privileged instructions to disable interrupts, so it usually doesn't work on unprivileged mode. Enabling this cfg in an environment where privileged instructions are not available is also usually considered unsound, although the details are system-dependent.

@Lokathor
Copy link

Oh, i only looked at the lines that the PR diff highlighted in green XD

Comment on lines +40 to +58
macro_rules! atomic_load {
($name:ident, $int_ty:ty, $atomic_ty:ident) => {
#[no_mangle]
#[cfg_attr(all(not(windows), not(target_vendor = "apple")), linkage = "weak")]
pub unsafe extern "C" fn $name(dst: *mut $int_ty, ordering: c_int) -> $int_ty {
unsafe {
let guard = FFIPanicGuard;
let res = mem::transmute(
(*dst.cast::<portable_atomic::$atomic_ty>()).load(c_load_ordering(ordering)),
);
mem::forget(guard);
res
}
}
};
}
atomic_load!(__atomic_load_1, u8, AtomicU8);
atomic_load!(__atomic_load_2, u16, AtomicU16);
atomic_load!(__atomic_load_4, u32, AtomicU32);
Copy link
Owner Author

@taiki-e taiki-e Aug 16, 2022

Choose a reason for hiding this comment

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

Btw, this is needed due to LLVM 15's change (rust-lang/rust#99668). Other GBA users seem to be affected as well (rust-lang/rust#100619). Passing +atomics-32 target feature is another way to solve this.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Filed the fix to upstream: rust-lang/rust#100621

@Jinxit
Copy link

Jinxit commented Aug 16, 2022

I just wanted to say that this is really cool and much appreciated!

As for running mGBA in "headless" mode, you can take a look at how agb compiles and uses its mgba-test-runner.

@taiki-e
Copy link
Owner Author

taiki-e commented Aug 16, 2022

Thanks @Jinxit! I will look into mgba-test-runner.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 17, 2022
Pass +atomics-32 feature for {arm,thumb}v4t-none-eabi

Similar to rust-lang@89582e8, but for ARMv4t.
Pre-v6 ARM target does not have atomic CAS, except for Linux and Android where atomic CAS is provided by compiler-builtins. So, there is a similar issue as thumbv6m.

I have confirmed that enabling the `atomics-32` target feature fixes the problem in the project affected by this issue. (taiki-e/portable-atomic#28 (comment))

Closes rust-lang#100619

r? `@nikic`
cc `@Lokathor`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 17, 2022
Pass +atomics-32 feature for {arm,thumb}v4t-none-eabi

Similar to rust-lang@89582e8, but for ARMv4t.
Pre-v6 ARM target does not have atomic CAS, except for Linux and Android where atomic CAS is provided by compiler-builtins. So, there is a similar issue as thumbv6m.

I have confirmed that enabling the `atomics-32` target feature fixes the problem in the project affected by this issue. (taiki-e/portable-atomic#28 (comment))

Closes rust-lang#100619

r? ``@nikic``
cc ``@Lokathor``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-arm Target: 32-bit Arm processors (armv6, armv7, thumb...), including 64-bit Arm in AArch32 state
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support atomic CAS on no-std pre-v6 ARM targets (e.g., thumbv4t-none-eabi)
3 participants