"Invalidate Cache" operation is unsafe #188
Comments
It might also be that a clean is unsound too, I want to think about this a little more.
Given the planned upcoming breaking changes and I think the relatively uncommon use of the cache operations this is my preferred option.
These operations are peripheral registers in Cortex-M, with clean+invalidate being distinct from invalidate - sometimes you need one and sometimes you need the other. The methods in SCB mirror those provided in CMSIS. You very often need to invalidate without cleaning if you know another observer has written to the main memory and your cache is outdated, since a clean+invalidate would overwrite whatever the observer had written. Clean+invalidate is pretty much just used for disabling the cache, when you want to ensure all updates are written out and then the item is removed from the cache. I definitely think we should keep all the methods. |
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]>
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]>
|
Fixed by #192, closing |
…jects Tested with 1.34.0 and 1.38.0 and careful inspection of the linker map generated on the previously failing https://github.com/rust-lang/rust/files/3722440/minimal-rust-lld-issue.zip Closes #188 (I believe) Closes rust-lang/rust#65391 Signed-off-by: Daniel Egger <[email protected]>
207: Rejig link.x to include more lables to help the linker lay out the ob… r=thejpster a=therealprof …jects Tested with 1.34.0 and 1.38.0 and careful inspection of the linker map generated on the previously failing https://github.com/rust-lang/rust/files/3722440/minimal-rust-lld-issue.zip Closes #188 (I believe) Closes rust-lang/rust#65391 Signed-off-by: Daniel Egger <[email protected]> Co-authored-by: Daniel Egger <[email protected]>
The optional Cortex-M cache exposes "clean" and "invalidate" operations. As pointed out by @adamgreig in #47 (comment), performing only the "invalidate" operation without a preceding "clean" is unsound.
This operation is currently exposed via safe interfaces:
CBPSCBWe should either deprecate the methods and add unsafe replacements, or make the methods
unsafein a breaking change without deprecation period (this avoids having to find new names).In my opinion there is little to no value in exposing only "invalidate" since it hard to use correctly and has little to no benefits over "clean + invalidate", so removing it is also an option.
The text was updated successfully, but these errors were encountered: