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

Stop using r0 crate for user programs. #101

Merged
merged 2 commits into from
Dec 11, 2020
Merged

Stop using r0 crate for user programs. #101

merged 2 commits into from
Dec 11, 2020

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Dec 10, 2020

r0 is likely unsound. This replaces the data/bss init with an assembly
routine that is guaranteed not to get mangled by the compiler.

Context:
rust-lang/unsafe-code-guidelines#259

@cbiffle
Copy link
Collaborator Author

cbiffle commented Dec 11, 2020

I've validated this most recent version on at least the STM32H7, so I reckon this is ready for review


1: cmp r2, r0 @ has dest reached the upper bound?
bne 2b @ if not, repeat

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the existing r0 crate there's a compiler fence between init_data and zero_bss. I don't think we need to keep that since there should be no aliasing between data and bss so it should be fine to do a single call at the end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, compiler_fence is only important when you're using a compiler -- it stops LLVM from moving loads and stores across that point, among other things. In our case, in assembly, there's no risk of our instructions getting moved.

Though your link reminded me that I did not implement r0's pre_init hook. I should make sure we're not relying on that anywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Conclusion: we are totally relying on it, and so I have subtly broken initialization on the more complex chips...but in a way that passed the tests, interestingly. Hooray "manufacturer says this behavior is undefined!"

Will fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FALSE ALARM. This is for user programs. They don't use pre_init and have never actually had access to it. I was thinking of the kernel. Nevermind. :-)

r0 is likely unsound. This replaces the data/bss init with an assembly
routine that is guaranteed not to get mangled by the compiler.

Context:
rust-lang/unsafe-code-guidelines#259
@cbiffle cbiffle merged commit f38a339 into master Dec 11, 2020
@cbiffle cbiffle deleted the r0-soundness branch December 11, 2020 22:24
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.

2 participants