Update and improve cache operations. #192

adamgreig · 2020-01-15T23:36:44Z

I've implemented the proposed methods from #47 and marked all d-cache invalidation functions as unsafe. It's not unsafe to invalidate i-cache or branch predictor as they are read-only caches. The clean and clean+invalidate operations do not alter memory from the executing core's point of view so are also safe.

It wasn't possible to remove the requirement to pass in &mut CPUID as you require synchronized access to CPUID to read the number of sets and ways in the cache, which is required to fully clean or invalidate them, which is required to enable or disable them. So it goes.

Breaking change due to changing safety of d-cache invalidation functions.

Closes #47, #188. Breaking change due to changing safety of d-cache invalidation functions.

rust-highfive · 2020-01-15T23:36:55Z

r? @ithinuel

(rust_highfive has picked a reviewer for you, use r? to override)

jonas-schievink

Looks pretty good!

jonas-schievink · 2020-01-15T23:50:54Z

src/peripheral/scb.rs

+    /// recent writes to `obj` to be lost, potentially including the write that initialised it.
+    /// Therefore, this method may cause uninitialised memory or invalid values to be read,
+    /// resulting in undefined behaviour. You must ensure that main memory contains a valid and
+    /// initialised value for T before invalidating `obj`.


Isn't it unsound in general when the memory a &T points to is modified (unless T is an UnsafeCell)?

Do you think we should require &mut here instead of &, or just give a more dire warning about potential consequences?

&mut sounds like a good option, as long as it doesn't make the API unusable. Do you happen to have any real-world examples where this API would be used?

I've used the previous invalidate API in ethernet peripheral drivers where the descriptors and packet memory are shared between CPU and peripheral. In all of those instances the code had &mut access, though the receive buffers were in principle read-only.

The other use case I can see is sharing memory between cores on a multi-core device, e.g. the STM32H7 dual-core series have D-cache for the M7 core, so communicating with the M4 core could require cache management. Imagine a synchronisation primitive which needs to invalidate some interior memory to ensure it's reading a fresh value from the other core, and wants to be able to do this with only &self. But probably such a primitive should contain an UnsafeCell which can handle the interior mutability, and then it would still be able to pass &mut to the cache invalidation function.

On balance I think it's probably a more obvious API if it takes &mut to make it clear that the value might change, and in the rare case someone needs to access it with a non-mut, it's only a one line function that calls another public unsafe method anyway, so it's not that much extra effort. I'll update this PR.

adamgreig · 2020-01-16T11:43:30Z

A couple of other notes:

The comments refer to 32-byte alignment and length. Technically in ARMv7-M this length is core-specific and available from the DMinLine field of the CTR register. Currently, only Cortex-M7 cores can have a D-cache, and Cortex-M7 cores can only have 32-byte DMinLine. In the future ARM might release other cores with other cache sizes. I'll update the docstrings to clarify.
The above means we probably should not have an assert that the memory being invalidated is 32-byte aligned and a multiple of 32 bytes long. Which is a shame as those asserts would probably be quire useful and make this a less sharp API. Perhaps we could read the DMinLine register and check based on that, I'll investigate.

Please don't merge just yet.

adamgreig · 2020-02-04T00:39:35Z

I've updated the documentation to more clearly convey what's required and what will happen, updated the methods to take &mut where appropriate, and updated the underlying loops to use DminLine to obtain the cache size at runtime instead of hardcoding 32, although at the moment all released devices will use 32.

This is ready for review now.

jonas-schievink

Great work! I have only some documentation nits.

src/peripheral/scb.rs

src/peripheral/cpuid.rs

src/peripheral/scb.rs

Co-Authored-By: Jonas Schievink <[email protected]>

adamgreig · 2020-02-04T23:40:22Z

Thanks, I've applied suggestions and updated some other initialised 🇬🇧

jonas-schievink · 2020-02-04T23:48:40Z

Great, let's do this!

bors r+

bors · 2020-02-04T23:48:43Z

👎 Rejected by too few approved reviews

jonas-schievink

bors r+

192: Update and improve cache operations. r=jonas-schievink a=adamgreig Closes #47, #188. I've implemented the proposed methods from #47 and marked all d-cache invalidation functions as unsafe. It's not unsafe to invalidate i-cache or branch predictor as they are read-only caches. The clean and clean+invalidate operations do not alter memory from the executing core's point of view so are also safe. It wasn't possible to remove the requirement to pass in `&mut CPUID` as you require synchronized access to `CPUID` to read the number of sets and ways in the cache, which is required to fully clean or invalidate them, which is required to enable or disable them. So it goes. Breaking change due to changing safety of d-cache invalidation functions. Co-authored-by: Adam Greig <[email protected]>

bors · 2020-02-05T00:05:10Z

Build failed

continuous-integration/travis-ci/push

jonas-schievink · 2020-02-05T00:09:55Z

Apparently we're downloading stuff from Arm servers during CI. Not sure why but we shouldn't be doing that.

adamgreig · 2020-02-05T00:10:01Z

bors retry

…

192: Update and improve cache operations. r=jonas-schievink a=adamgreig Closes #47, #188. I've implemented the proposed methods from #47 and marked all d-cache invalidation functions as unsafe. It's not unsafe to invalidate i-cache or branch predictor as they are read-only caches. The clean and clean+invalidate operations do not alter memory from the executing core's point of view so are also safe. It wasn't possible to remove the requirement to pass in `&mut CPUID` as you require synchronized access to `CPUID` to read the number of sets and ways in the cache, which is required to fully clean or invalidate them, which is required to enable or disable them. So it goes. Breaking change due to changing safety of d-cache invalidation functions. Co-authored-by: Adam Greig <[email protected]>

adamgreig · 2020-02-05T00:12:23Z

We download the gcc toolchain which is used to verify the precompiled binary blobs are correct compared to the source, iirc.

…

On Wed, 5 Feb 2020, 00:09 Jonas Schievink, ***@***.***> wrote: Apparently we're downloading stuff from Arm servers during CI. Not sure why but we shouldn't be doing that. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#192?email_source=notifications&email_token=AAALQ4YKQ7345V63VXIDXILRBH7VHA5CNFSM4KHLQKG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKZVLBI#issuecomment-582178181>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALQ427ZFJOESIJ4OQBXWDRBH7VHANCNFSM4KHLQKGQ> .

jonas-schievink · 2020-02-05T00:16:19Z

Using apt-get is better for that since Travis caches it. I'll open an issue.

adamgreig · 2020-02-05T00:17:26Z

At the time I don't think a suitable version was available in apt and it still might not be. We could have Travis cache the downloaded toolchain, though.

…

On Wed, 5 Feb 2020, 00:16 Jonas Schievink, ***@***.***> wrote: Using apt-get is better for that since Travis caches it. I'll open an issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#192?email_source=notifications&email_token=AAALQ46FXWJKHYLJTOSA3QTRBIANJA5CNFSM4KHLQKG2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKZVZOY#issuecomment-582180027>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAALQ4ZGRH6657UABFACWDDRBIANJANCNFSM4KHLQKGQ> .

bors · 2020-02-05T00:24:02Z

Build succeeded

continuous-integration/travis-ci/push

192: add a .uninit section r=therealprof a=japaric that contains static variables that will not be initialized by the runtime this implements only the linker section described in RFC #116 so that downstream libraries / framework can leverage it without (a) forking this crate or (b) requiring the end user to pass additional linker scripts to the linker when building their crate. This commit doesn't add the user interface described in RFC #116; instead it documents how to use this linker section in the advanced section of the crate level documentation Co-authored-by: Jorge Aparicio <[email protected]>

prepare a new patch release that includes PRs #190 and #192

194: v0.6.9 r=adamgreig a=japaric prepare a new patch release that includes PRs #190 and #192 Co-authored-by: Jorge Aparicio <[email protected]>

Update and improve cache operations.

4b4f825

Closes #47, #188. Breaking change due to changing safety of d-cache invalidation functions.

adamgreig added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. breaking change labels Jan 15, 2020

adamgreig requested a review from a team as a code owner January 15, 2020 23:36

rust-highfive assigned ithinuel Jan 15, 2020

rust-highfive added the T-cortex-m label Jan 15, 2020

jonas-schievink reviewed Jan 15, 2020

View reviewed changes

Update documentation for cache functions; use dynamic cache line size

9fac3d9

jonas-schievink reviewed Feb 4, 2020

View reviewed changes

adamgreig and others added 2 commits February 4, 2020 23:36

Apply suggestions from code review

2608c23

Co-Authored-By: Jonas Schievink <[email protected]>

Add missing inline(always) and change more initialised->initialized

9ad4e10

jonas-schievink approved these changes Feb 4, 2020

View reviewed changes

bors bot merged commit 9ad4e10 into master Feb 5, 2020

bors bot deleted the cache-safety branch February 5, 2020 00:24

jonas-schievink mentioned this pull request Feb 5, 2020

"Invalidate Cache" operation is unsafe #188

Closed

adamgreig mentioned this pull request Jul 5, 2020

v0.6.3 release? #217

Closed

adamgreig pushed a commit that referenced this pull request Jan 12, 2022

v0.6.9

b98ee6c

prepare a new patch release that includes PRs #190 and #192

adamgreig pushed a commit that referenced this pull request Jan 12, 2022

Merge #194

7de67e8

194: v0.6.9 r=adamgreig a=japaric prepare a new patch release that includes PRs #190 and #192 Co-authored-by: Jorge Aparicio <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update and improve cache operations. #192

Update and improve cache operations. #192

adamgreig commented Jan 15, 2020

rust-highfive commented Jan 15, 2020

jonas-schievink left a comment

jonas-schievink Jan 15, 2020

adamgreig Jan 16, 2020

jonas-schievink Jan 16, 2020

adamgreig Jan 16, 2020

adamgreig commented Jan 16, 2020

adamgreig commented Feb 4, 2020

jonas-schievink left a comment

adamgreig commented Feb 4, 2020

jonas-schievink commented Feb 4, 2020

bors bot commented Feb 4, 2020

jonas-schievink left a comment

bors bot commented Feb 5, 2020

jonas-schievink commented Feb 5, 2020

adamgreig commented Feb 5, 2020 via email

adamgreig commented Feb 5, 2020 via email

jonas-schievink commented Feb 5, 2020

adamgreig commented Feb 5, 2020 via email

bors bot commented Feb 5, 2020

Update and improve cache operations. #192

Update and improve cache operations. #192

Conversation

adamgreig commented Jan 15, 2020

rust-highfive commented Jan 15, 2020

jonas-schievink left a comment

Choose a reason for hiding this comment

jonas-schievink Jan 15, 2020

Choose a reason for hiding this comment

adamgreig Jan 16, 2020

Choose a reason for hiding this comment

jonas-schievink Jan 16, 2020

Choose a reason for hiding this comment

adamgreig Jan 16, 2020

Choose a reason for hiding this comment

adamgreig commented Jan 16, 2020

adamgreig commented Feb 4, 2020

jonas-schievink left a comment

Choose a reason for hiding this comment

adamgreig commented Feb 4, 2020

jonas-schievink commented Feb 4, 2020

bors bot commented Feb 4, 2020

jonas-schievink left a comment

Choose a reason for hiding this comment

bors bot commented Feb 5, 2020

Build failed

jonas-schievink commented Feb 5, 2020

adamgreig commented Feb 5, 2020 via email

adamgreig commented Feb 5, 2020 via email

jonas-schievink commented Feb 5, 2020

adamgreig commented Feb 5, 2020 via email

bors bot commented Feb 5, 2020

Build succeeded