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

Add function that enables FPU for cortex-m4 MCUs #1010

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@eupn
Copy link
Contributor

eupn commented Jun 20, 2018

Pull Request Overview

This pull request adds a function that enables Cortex-M4's Floating Point Unit (FPU).

As said in the User Guide, here and also here, firmware must manually enable FPU to perform floating-point operations, so it would be nice if that function is available to user.

Testing Strategy

This pull request was tested by code compilation.

TODO or Help Wanted

This pull request adds a function that still needs a some #[cfg()] gate to make sure this function available to use only on Cortex-M4 MCUs, since scb.rs module is moved from cortex-m4 into generic cortex-m module.

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make formatall.

eupn and others added some commits Jun 20, 2018

Add function that enables FPU for cortex-m4 MCUs
As said in the [User Guide](http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0553a/BEHBJHIG.html), firmware must manually enable FPU to perform floating-point operations, so it would be nice if that function is available to user.

@bradjc bradjc added the P-Upkeep label Jun 20, 2018

@ppannuto
Copy link
Member

ppannuto left a comment

Following those links, it looks like there are some more changes. For example, I believe that with the FPU enabled interrupt register stacking changes to save FPU registers as well, which means all the context switch code will have to be FPU-aware.

/// Enables Floating Point Unit. Section 4.6.6.
/// http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0553a/BEHBJHIG.html
/// http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dai0298a/CEGJEIGH.html
pub unsafe fn enable_fpu() {

This comment has been minimized.

@ppannuto

ppannuto Jun 20, 2018

Member

Is there a hardware register we can read to validate that a FPU is present? I would prefer this throw an exception / error / etc if there isn't an FPU

This comment has been minimized.

@alevy

alevy Jun 20, 2018

Member

what about feature gating it at compile time? So it just wouldn't be available to call on a non-floating point enabled MCU?

This comment has been minimized.

@eupn

eupn Jun 20, 2018

Contributor

This is certainly should be feature-gated since there's no way to check FPU presence using hardware registers/flags.

This comment has been minimized.

@ppannuto

ppannuto Jun 20, 2018

Member

That'd be fine too, though there's probably a bigger architectural discussion we need to have surrounding how to manage such cfgs for varying chips

@ppannuto ppannuto added P-Significant and removed P-Upkeep labels Jun 20, 2018

@eupn

This comment has been minimized.

Copy link
Contributor

eupn commented Jun 20, 2018

I actually wondering, since there were no code that enables FPU anywhere, all floating-point operations were done in software even though code is running on FPU-powered Cortex-M4 chips or on some chips like SAM4L or nRF52 FPU is enabled by-default?

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jun 20, 2018

Summarizing my thoughts on this and @ppannuto's comments:

I basically agree with @ppannuto that this is running up against some of the rough edges in Tock, particularly around cross-arch portability. It's something we should resolve in general.

I think for this feature, it's a reasonable place to accrue some engineering debt. Basically I say we this works properly, test it, etc, and do things like feature gate the FPU code such that there is some way to remove them at compile time. But we can let it be a bit messy.

That's my reaction anyway.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 20, 2018

As a nonbeliever in #ifdef, why do need to feature gate anything? Why not just not call enable_fpu if you shouldn't be calling it?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jun 20, 2018

👍 for one-off feature gate in the short term.

But, I do think this needs to consider the rest of the system implications -- which may just be context switching, but definitely includes context switching -- before it's ready to merge.

@bradjc
Copy link
Contributor

bradjc left a comment

This seems like a function that might be useful someday, and is (as far as I can tell) correctly implemented (even though it won't work until #1011 is merged).

Are there other comments in the remainder of the 7 day window?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jun 25, 2018

Unless GitHub is failing to show new commits to me or something, I don't think either of my concerns have been addressed:

  1. This needs to be feature-gated.

  2. This needs to include code to fixup context switch handling when the FPU is enabled.

This is not ready for merge.

@ppannuto ppannuto added the blocked label Jun 25, 2018

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 25, 2018

Why does it need to be featured gated? What is wrong with just not calling code that you shouldn't? For example, those registers in the SCB are already defined and there, but the m0 platform just doesn't use them.

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jun 25, 2018

Why does it need to be featured gated? What is wrong with just not calling code that you shouldn't?

I think that's pretty reductionist; why not just write code without bugs?

In general, one of the principle features of Tock/Rust that I value is the ability to catch a greater percentage of mistakes at compile-time rather than runtime. I think this is an excellent example of functionality that necessarily cannot be dynamic (a given chip either has an FPU or it doesn't) and Tock should make it impossible to try to enable the FPU on hardware that doesn't have it.


From an out-of-band discussion, it was suggested that this shouldn't necessarily block on fixing syscalls, as maybe someone would like FPU support and isn't using userland. But I think to support that argument, I would like to see an example of something calling this function. As it stands, if someone playing around with Tock called this function, userland would break in fairly cryptic fashion with very little introspection into why* (and somewhat far removed from this function call as the error would not appear until the next context switch).

Thank you @eupn for the start, I think this is a good contribution, unfortunately I think it's also a dangerous one that needs to be a little more complete before we can pull it in.

*Probably a HardFault, but without reasoning too deeply, I think the effect would actually be to return with a wrong set of values in the core registers (a few bytes further up the stack), which could possibly do other really trippy things like return to a different function (!).

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jun 27, 2018

I haven't had a chance to do this yet, but what I think we want is to create a new crate cortex-m4f that exports everything the cortex-m4 crate does and in addition this function. That way we don't need a feature gate as it will only be available to platforms that have a fpu. The other cortex-m crates would not export this function so there is no chance it can get called.

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Dec 9, 2018

Closing this for now. Now that we have a UserspaceKernelBoundary abstraction it should be doable to implement the logic required to switch between kernel and user mode on the floating point cortex-m devices. If someone wants to implement that we will revive this PR.

@bradjc bradjc closed this Dec 9, 2018

@bradjc bradjc referenced this pull request Dec 9, 2018

Open

Tracking: Support Cortex-M with Floating Point #1246

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment