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.

Sign up for GitHub

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

Jump to bottom

Deprecate msp::write #297

Merged
merged 2 commits into from
Dec 3, 2020
Merged

Deprecate msp::write #297

merged 2 commits into from
Dec 3, 2020

Conversation

jonas-schievink
Copy link
Contributor

The function can not be used from Rust code, because it destroys the stack frame.

Users must use an assembly block (inline or out-of-line) that both sets MSP and performs a method call.

@jonas-schievink jonas-schievink requested a review from a team as a code owner November 25, 2020 22:29
@rust-highfive
Copy link

r? @therealprof

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m labels Nov 25, 2020
@adamgreig
Copy link
Member

I think we need to provide a usable alternative that works on stable before we deprecate this; I know I use it a bunch for either jumping to bootloaders or jumping to application code, and (as we keep finding out with cortex-m/-rt) it's a fair pain to include an assembly blob without using unstable inline-asm.

Perhaps that just needs to be the function we talked about that reads an SP and RV from a vector table and does the MSP write and jump, which is of course unsafe but might at least be sound so long as you give it the address of a valid vector table. That said, couldn't the same be true here so long as the user ensures immediately after updating MSP they jump into their new (divergent) function?

@jonas-schievink
Copy link
Contributor Author

I think having a function like fn bootstrap(stack_pointer: *mut u32, entry: fn() -> !) would be enough for that, right?

One problem is that that doesn't allow passing parameters, so maybe entry should take 4 u32 arguments?

Do we want to provide that in cortex-m or should all the assembly stubs be put into a cortex-m-asm crate, to be useful outside of bare-metal targets? (now that I think about it, such a crate would be a lot easier to push to 1.0 than the entirety of cortex-m)

@adamgreig
Copy link
Member

I think just fn bootload(vector_table: *const u32) would suffice for bootloading; the first entry in the table is always the new stack pointer and the second is always the new reset vector, in cortex-m. Saves the user having to transmute a raw pointer to a fn() -> ! themselves too. I guess the reason to pass arguments (or jump to something that's not a vector table) is OS implementations, perhaps they can have a separate method? I'm not as clear what they'd need, but it wouldn't hurt to support them too.

Putting these and the other asm routines into their own cortex-m-asm crate sounds great.

@jonas-schievink
Copy link
Contributor Author

RTOSes probably want to use PSP instead of MSP anyways, so +1 to just providing a bootloader-only method for now.

@adamgreig
Copy link
Member

The great thing about cortex-m-asm is even after 1.0 almost anything we'd want to do is add new methods, which aren't breaking changes anyway.

@therealprof
Copy link
Contributor

Sounds like a good plan. I'd prefer the name bootstrap over bootload in case we're bikeshedding.

@adamgreig adamgreig mentioned this pull request Dec 2, 2020
Merged
bors bot added a commit that referenced this pull request Dec 2, 2020
@bors @adamgreig
Merge #299
038396e 
299: Expose __syscall and add new bootstrap method r=jonas-schievink a=adamgreig

We added `__syscall` in the new inline asm, but did not expose it in the crate API, and the cortex-m-semihosting crate can't use it directly because the pre-built binaries would contain duplicate symbols (#271). This PR renames it to `__sh_syscall` (since we could imagine other different syscalls; this one is explicitly semihosting with the `bkpt 0xAB`) and exposes it in `cortex_m::asm::sh_syscall`.

This PR also adds the new methods discussed in #297 to permit sound bootstrapping, either from an MSP and RV or a vector table address.

Co-authored-by: Adam Greig <adam@adamgreig.com>
@adamgreig
Copy link
Member

I think we're good to merge this now. Could you add a changelog entry please?

adamgreig
adamgreig approved these changes Dec 3, 2020
View reviewed changes
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

bors r+

@adamgreig
Copy link
Member

(this looks fine to me and I think having the note about bootstrap in the changelog is OK, but just checking you deliberately reverted the deprecation message rather than a force-push accident?)

@bors
Copy link
Contributor

bors bot commented Dec 3, 2020

Build succeeded:

@bors bors bot merged commit d6fb22e into rust-embedded:master Dec 3, 2020
@jonas-schievink jonas-schievink deleted the deprecate-msp-write branch December 3, 2020 11:54
@jonas-schievink
Copy link
Contributor Author

(this looks fine to me and I think having the note about bootstrap in the changelog is OK, but just checking you deliberately reverted the deprecation message rather than a force-push accident?)

Oh, no, that was an accident. I didn't pull the commit fixing it.

@jonas-schievink jonas-schievink mentioned this pull request Dec 3, 2020
Merged
bors bot added a commit that referenced this pull request Dec 3, 2020
@bors @jonas-schievink
Merge #303
d007eb9 
303: Update msp::write deprecation message r=adamgreig a=jonas-schievink

Accidentally deleted from #297 during rebase

Co-authored-by: Jonas Schievink <jonasschievink@gmail.com>
@adamgreig adamgreig mentioned this pull request Jan 25, 2021
Merged
bors bot added a commit that referenced this pull request Jan 25, 2021
@bors @adamgreig
Merge #320
ad7e01b 
320: Prepare for v0.7.1 r=therealprof a=adamgreig

Includes:

* Deprecate msp::write #297
* New syscall and bootstrap ASM #299 
* More compiler fences #311 
* asm::delay timing fix #312 
* asm::delay clobber fix #317 
* LTO for ASM fix #318 

Doesn't include anything to help with #304 which might be nice to fix but can come in 0.7.2 and might need some time to think about.

Co-authored-by: Adam Greig <adam@adamgreig.com>
adamgreig pushed a commit that referenced this pull request Jan 12, 2022
@bors @hug-dev
Merge #297
7ac8f78 
297: Add a section to place the veneers in memory r=jonas-schievink a=hug-dev

The veneers are for now only generated by the Arm GNU linker when it
spots an entry function (one that was decorated with the
cmse_nonsecure_entry attribute).
Adding this section will allow to configure the SAU to make this section
Non-Secure Callable.

Doing tests locally I could not see any warnings if this section was empty so I think this is fine. It is highly specific to the GNU toolchain so maybe you would want some preprocessing directive and `cfg` options which I am happy to add.

There is documentation for the section name at the end of [this page](https://sourceware.org/binutils/docs/ld/ARM.html).

It needs to be aligned on 32 bytes as a requirement from the Security Attribute Unit.

Co-authored-by: Hugues de Valon <hugues.devalon@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamgreig adamgreig adamgreig approved these changes

Assignees

@therealprof therealprof

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-cortex-m
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jonas-schievink @rust-highfive @adamgreig @therealprof