Use assembly sequences to enable caches. #234
Conversation
|
r? @adamgreig (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
S-waiting-on-review
|
I'm not clear on why Edit: Ah, I misinterpreted the results; I had assumed the assembly source files were built rather than checked in as static libraries. Will fix. |
|
Only the x86 target runs the Our CI is using ARM's |
|
I am using binutils 2.34-1 (the -1 being an Arch package rev) |
|
Is the decision to use pre-unified syntax in the asm files deliberate? It means that the asm files and the inline-asm blocks use different syntax. (I'm having to go digging to remember how to express a reg-reg-immed orr in pre-unified mnemonics.) If not, I will add the |
|
I don't see any reason not to use unified syntax here, I assume it was an oversight originally. The replication between the asm files and the inline asm blocks is the only way we've found to provide asm to stable users while still offering inline asm for nightly users, but I don't think there's any need for inline asm for these cache management operations, so we could only ship the prebuilt objects here. If we end up including all the other maintenance operations I think there would be enough assembly to tip the balance towards only using the prebuilt objects; as-is it's short enough that I think we might as well include both. Hopefully soon Rust will include stable inline assembly and eventually we can remove the duplication. On the critical section: do you think there's any situation where users are likely to be worried about the overhead of entering a new critical section, justifying a version of the function that takes an |
woot, updated
Right. I'd do the same thing in your place. I wish that
Well, that simplifies things! :-)
This may be a failure of imagination, but...no, I don't think that's likely. I haven't quantified the overhead on the M7 (which is where caches start to become commonplace) but I would be surprised if anyone noticed. So I will add the instructions and make the code correct. |
src/peripheral/scb.rs
Outdated
| crate::asm::dsb(); | ||
| crate::asm::isb(); | ||
| // NOTE(unsafe): Having &mut self means no ISR, for example, is going to | ||
| // try to alter this register at the same time. |
There was a problem hiding this comment.
The safety is provided by the assembly function entering a critical section, so it doesn't matter that we had a unique reference to self anyway. Perhaps this comment and the one below could be combined into a single comment that the asm routine contains a critical section so its access to SCB cannot be raced and it contains the required barriers for cache maintenance operation (and same for icache).
In fact, both methods could be made static now (albeit a breaking change) as neither require access to SCB any more.
There was a problem hiding this comment.
I can merge the comments.
Making the methods static is a design decision not related to the implementation -- it could have been done before, since the asm code is doing the equivalent of unsafe { &*SCB::ptr() }. So it comes down to your preference: should any code anywhere be able to turn on caches as "ambient authority," without having been handed an enabling token? It feels inconsistent with the rest of the API, which tends to really want you to have been passed a phantom peripheral representation to do things.
In either case, I'd prefer to get this fix available without bumping major versions, so making the operations static could wait?
There was a problem hiding this comment.
(For example, the data cache invalidation/cleaning operations are necessary in cases where you have a CPU with caches, and another device, sharing control of a block of memory, e.g. for DMA, if you haven't simply disabled caching for that section of memory. Right now, the only code with access to these operations must have exclusive control over the entire SCB. Assuming this was deliberate, the cache enable operations are strictly less common than other maintenance operations, and should probably not be the first or only ones to become static if we want to static some things.)
There was a problem hiding this comment.
I would say our current requirement to have the peripheral token exists in order to use Rust's lifetime system to ensure exclusive access to the peripheral in aid of preventing data races in read-modify-write operations, rather than as a means of safety-enforced access control to choose what code can make system modifications, if that makes sense. We have a number of static methods already and in general they're more ergonomic.
I completely take your point that clean and invalidate ops are the ones that really want to be static though (currently I have to unsafely create an SCB to use them since it's not practical to pass the only SCB instance into e.g. an Ethernet driver). Perhaps let's leave these operations as they are for now and consider making the majority of cache control static later.
Currently our master branch already contains breaking changes to our latest release, but I'm happy to issue a patch release on the current 0.6 series containing this and other non-breaking bug fixes.
See rust-embedded#232, which this partially fixes -- there's still the question of taking an interrupt in the midst of these sequences.
|
Build succeeded: |
246: Always link pre-built asm, required for new cache management functions r=therealprof a=adamgreig In #234 we added new `__enable_icache` and `__enable_dcache` assembly routines, but without providing an inline assembly version, to reduce duplication and since there wasn't an expected performance impact. However, our build.rs currently only links the pre-built object if the `inline-asm` feature is disabled, which means currently you can't call `enable_icache()` and use `inline-asm` at the same time. This PR makes us always link against the pre-built objects (for thumb targets) even if `inline-asm` is used; the pre-built object would only be used for the cache management routines at present but we may want to put more routines into the assembly blob only in the future. Closes #245. Co-authored-by: Adam Greig <[email protected]>
239: Make `ExceptionFrame`s fields private r=korken89 a=jonas-schievink Closes #234 ~~I can also add the `unsafe` setters, but they don't have any use right now.~~ (added them in order to not regress available operations on `ExceptionFrame`) Co-authored-by: Jonas Schievink <[email protected]>
See #232, which this partially fixes -- there's still the question of
taking an interrupt in the midst of these sequences.