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

kern: suppress phantom SVCs on ARMv6-M #1932

Merged

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Nov 22, 2024

You might recognize this as #1134. Well, it's back. My fix for the problem on ARMv6-M was apparently never quite correct on M0+, because the core register we were relying on to detect and cancel a pending SVC -- SHCSR -- is implemented but only visible from the debugger. So when I tried it by hand in openocd, it worked... but from the kernel it silently fails.

ARM, you are not making friends with stuff like this.

I've worked out a different way to detect the "fault during SVC" case using the always-implemented ICSR register. That register doesn't provide any way to cancel the SVC, so I've added conditional code to the syscall entry path that can do it.

Fixes #1928.

@cbiffle cbiffle force-pushed the cbiffle/armv6m-phantom-svc-mitigation branch 4 times, most recently from 43df6fd to a139048 Compare November 22, 2024 23:11
@hawkw hawkw self-requested a review November 25, 2024 21:17
Comment on lines 1438 to 1450
let scb = unsafe { &*cortex_m::peripheral::SCB::PTR };
let icsr = scb.icsr.read();
let vectpending = (icsr >> 12) as u8;

// If we're in a hardfault (which we know, because it's the only fault
// on ARMv6M and we are in a fault handler) and an SVC is pending...
if vectpending == 11 {
crate::syscalls::EXPECT_PHANTOM_SYSCALL
.store(true, core::sync::atomic::Ordering::Relaxed);
}
Copy link
Member

Choose a reason for hiding this comment

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

so, this is a bit goofy: it looks like the cortex_m crate has a nice structured representation of VECTACTIVE block in ISCR, but absolutely no API for looking at VECTPENDING besides what you've done here. so, uh, i guess that's fine...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While they have the enum, I don't see how you'd actually use it. The ICSR accessor just returns a u32. The only method for parsing the bitfield seems to be a function for creating the enum from a u8, which given that it's a nine bit field is surprising. This looks half-implemented to me.

(Next obvious question: why am I treating it as a u8 here? It's arguably a little bit of a shortcut, but, all existing ARMv6-M implementations are limited to way fewer than 256 vectors, and I'm really just testing for vector 11. I'd be open to masking it and using a u16 instead if anyone feels strongly about it.)

Copy link
Member

Choose a reason for hiding this comment

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

a comment about the cast might be good enough here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On it

Comment on lines +78 to +79
// On certain architectures we risk receiving "phantom SVCs" assigned to
// tasks that are not, in fact, making system calls. As of this writing,
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to explain why these phantom syscalls might occur here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This module is attempting not to be ARM specific, so I'm not sure how I would do that. I could tell people to go read arch/arm_m.rs maybe?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's reasonable; I think it might be reasonable to say "on certain ARM architectures (see ...)" or something here. I'm just always leery of comments that say "$SPOOKY_THING might happen, but I won't tell you when and where it might happen"...

@cbiffle cbiffle force-pushed the cbiffle/armv6m-phantom-svc-mitigation branch from a139048 to a7f81e6 Compare November 25, 2024 21:58
You might recognize this as oxidecomputer#1134. Well, it's back. My fix for the
problem on ARMv6-M was apparently never quite correct on M0+, because
the core register we were relying on to detect and cancel a pending SVC
-- SHCSR -- is _implemented_ but _only visible from the debugger._ So
when I tried it by hand in openocd, it worked... but from the kernel it
silently fails.

ARM, you are not making friends with stuff like this.

I've worked out a different way to _detect_ the "fault during SVC" case
using the always-implemented ICSR register. That register doesn't
provide any way to _cancel_ the SVC, so I've added conditional code to
the syscall entry path that can do it.

Fixes oxidecomputer#1928.
@cbiffle cbiffle force-pushed the cbiffle/armv6m-phantom-svc-mitigation branch from a7f81e6 to af01fe5 Compare November 25, 2024 22:00
@cbiffle cbiffle enabled auto-merge (rebase) November 25, 2024 22:05
@cbiffle cbiffle merged commit 112c675 into oxidecomputer:master Nov 25, 2024
125 checks passed
@cbiffle cbiffle deleted the cbiffle/armv6m-phantom-svc-mitigation branch December 14, 2024 00:20
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.

ARMv6M still attributes faulting SVC to wrong task
3 participants