`enable_icache` is wrong on M7 without `inline-asm` feature

[cbiffle]

This has been a fun one to track down.

tl;dr: if enabling instruction and/or data caches on your M7 is causing weird crashes, turn on the cortex_m inline-asm feature and make sure you're compiling at --release. Oh, and you're on nightly now. Sorry.

Details

I've got an STM32H7B3 here, and shortly after enabling the icache I would sometimes get hard faults that looked suspiciously like it trying to execute random data. (I'm doing this early enough in boot that I don't have configurable faults broken out, but they were either BF or MMF.)

The key here is sometimes -- some builds worked okay. Until I tried turning on dcache. Then things went squirrely.

It came down to: how many instructions did the compiler decide to insert between turning on the cache and the dsb / isb sequence? If the answer is not zero and contains a branch or memory access, weird things will happen.

The current implementation of enable_icache reads as follows:

    #[inline]
    pub fn enable_icache(&mut self) {
        // Don't do anything if I-cache is already enabled
        if Self::icache_enabled() {
            return;
        }

        // NOTE(unsafe): No races as all CBP registers are write-only and stateless
        let mut cbp = unsafe { CBP::new() };

        // Invalidate I-cache
        cbp.iciallu();

        // Enable I-cache
        // NOTE(unsafe): We have synchronised access by &mut self
        unsafe { self.ccr.modify(|r| r | SCB_CCR_IC_MASK) };

        crate::asm::dsb();
        crate::asm::isb();
    }

The tail end of that function is almost right.

• The modify will end in a str.
• The barriers will follow.

However,

• There's no use of compiler_fence or equivalent to prevent instructions from being rearranged above the barriers. (Perhaps we can assume this won't happen because they're extern "C"? I'm fairly certain I just saw it happen but I didn't save the listing.)
• dsb and isb default to being function calls

This means the actual dynamic instruction trace here is

str r1, [r0, #0]
bl __dsb
dsb sy
bx lr
bl __isb
isb sy
bx lr

In other words, we've got one branch in the pipeline before the write even takes effect, and no fewer than three before the isb causes a prefetch/pipeline flush. These instructions often execute incorrectly. (On a reasonably fast M7, "often" becomes "always" in my testing.)

Enabling the inline-asm feature fixes this by changing the dynamic trace to be what you'd expect:

str r1, [r0, #0]
dsb sy
isb sy

This is probably an issue with any existing code attempting to use isb to synchronize with memory map or cache changes. To do this correctly, one might consider making enable_icache (and enable_dcache though it crashes less reliably) contingent on the inline-asm feature, but that only produces the right instruction sequence at --release.

These routines probably need to be written in assembly to be correct on stable toolchains (and on debug builds on nightly toolchains, for that matter, to bind the compiler's hands). I haven't thought about the equivalent disable cases in detail, but they and the clean/invalidate routines probably bear review as well.

[jonas-schievink]

+1 for rewriting these routines (or at least the critical parts of them) in assembly, I don't think we can rely on LLVM to always produce the right instruction sequence here.

[adamgreig]

Ah, as soon as I saw the email subject I realised exactly what had happened. Thanks for the detailed report. On the whole you're right and this will need to be fixed by moving the routine to assembly. We're really desperate for proper thumb intrinsics! Or I guess the new inline assembly would also satisfy.

There's no use of compiler_fence or equivalent to prevent instructions from being rearranged above the barriers. (Perhaps we can assume this won't happen because they're extern "C"? I'm fairly certain I just saw it happen but I didn't save the listing.)

https://github.com/rust-embedded/cortex-m/blob/master/src/asm.rs#L221 ;)

As it happens the 'extern C' does add something with the effect of a memory clobber so should prevent reordering of memory access in the output, but an explicit fence wouldn't hurt. We had much the same issue recently around ordering entering/leaving a CriticalSection.

[adamgreig]

For the record this is the same problem people hit more often when using the externally linked 'intrinsics' to modify the stack pointer (e.g. to bootload) where the msp::write() call might modify the stack pointer and then pop some stuff off the stack, which ends as well as you'd expect and is also fixed by using inline-asm. It's a pain.

[cbiffle]

Any comment asking if a barrier is required should be replaced by a barrier. :-) You might lose some performance, but you're less likely to lose correctness.

I largely agree with @jonas-schievink, though I would go one step farther and suggest that dsb, dmb, and isb should be removed from the API. Any time you are writing code using explicit platform-dependent barrier instructions (as opposed to atomic types, or some library that contains barriers), you probably care about instruction reordering; Rust-level functions like this don't allow you to reason about it, and are almost impossible to use correctly, particularly across compiler versions. The current functions are footguns.

But if that seems too disruptive, then yes, going through every use and auditing why it's using a barrier, which ARM documentation it was following, whether the instruction order is actually significant, whether it's using the correct combination of machine and compiler fences, and then whether it needs to be rewritten in assembly anyway, would also work.

[adamgreig]

There are definitely use cases where dmb and dsb are required for correctness but it doesn't matter if it's a function call, for example interoperating with a DMA on a shared memory region. I wouldn't want to remove them from the API. I'm less certain about uses for isb() that don't care about it potentially turning into two branches. We should definitely document more carefully that the won't necessarily optimise into a single inline instruction.

I suspect every case using those barriers needs to prevent memory reordering across the barrier, but at present either the function call is an extern function call (which implies a memory clobber)* or has an explicit memory clobber in the inline asm, which will prevent memory reordering around the call*.

* gosh, i hope so, anyway

[cbiffle]

I'm less certain about uses for isb() that don't care about it potentially turning into two branches.

I'm pretty sure that's not a thing, except for code that is cargo-culting isb because it saw it elsewhere. Which is pretty common unfortunately. isb() delenda est.

[cbiffle]

Okay, I admit that I have found a case where isb() is not wrong: in a privileged-mode kernel, activating the MPU, and using isb() to ensure that the settings apply before switching to unprivileged code. It's only correct in that case because the kernel "knows" that its code is accessible both in the default map and after the MPU is enabled, so continuing to execute a pipeline(s) full of instructions from before the MPU got turned on is OK. (Though in practice, switching to unprivileged usually involves an exception return, which implies an isb on armv7-m.)

What about marking isb() unsafe? It is, technically, unsafe, in its current form -- my system was executing nonexistent instructions and stomping on memory before I fixed this.

[adamgreig]

"memory trouble? apply patented tincture __DSB(); __ISB(); topically as required"

What about marking isb() unsafe? It is, technically, unsafe, in its current form -- my system was executing nonexistent instructions and stomping on memory before I fixed this.

I think that's a bug in the isb() implementation rather than an incorrect safety contract; in principle I don't think executing isb should be able to cause memory unsafety (not executing it is what's potentially unsafe...).

How about:

• Add pre-built assembly snippet which enables icache and immediately runs dsb and isb; always use that in tail of enable_icache, resolving the immediate issue
• Document more clearly in all our pre-built "intrinsics" that calling them (except with release mode inline-asm) will result in a function call that may cause unwanted side effects
• The Cortex-M7 docs are quite explicit that "you must always place a DSB and ISB instruction sequence after a cache maintenance operation", so I don't think we should remove any; the question is more whether any of them require we make sure there's no extra branches involved first. I think that shouldn't affect the data cache, and doesn't cause problems when disabling the icache, in which case enable_icache is the only problem, but I'd welcome more eyes on that...
• We need to be very sure that calls to DSB and ISB cannot be reordered around; we could add a compiler_fence(Ordering::SeqCst) to these methods if we can't satisfy ourselves that the external function call and/or inline-asm-with-memory-clobber does the job. It's a shame that compiler_fence has a panic path though, I'm not sure under what conditions that is optimised out.

[cbiffle]

Add pre-built assembly snippet which enables icache and immediately runs dsb and isb; always use that in tail of enable_icache, resolving the immediate issue

sgtm.

Document more clearly in all our pre-built "intrinsics" that calling them (except with release mode inline-asm) will result in a function call that may cause unwanted side effects

I'm concerned that this doesn't proactively alert any current users that their code is wrong, the way a deprecation might. But, yes. This would be good.

The Cortex-M7 docs are quite explicit that "you must always place a DSB and ISB instruction sequence after a cache maintenance operation", so I don't think we should remove any

Yes, if you can cite docs, the barriers should be there. It looks like the cortex-m crate is using these barriers primarily in cache cases, so they are probably fine.

(I'll file a separate bug on how NVIC::mask ought to contain an isb.)

I think that shouldn't affect the data cache, and doesn't cause problems when disabling the icache, in which case enable_icache is the only problem, but I'd welcome more eyes on that...

I have definitely had intermittent failures when enabling the dcache (in fact, merely enabling the icache was not enough to trigger this in my code). I'm not sure if it was the dcache sequence or the interaction of the two, though. I'd use the asm snippet and the barriers in both cases.

In reading these cache maintenance functions -- are they written assuming they are executing in critical sections? Currently you could take an interrupt after enabling/disabling icache and before the barriers, which would be bad. (Worst case, the interrupt could have occurred shortly before the operation, and so the next instruction in the pipeline begins the interrupt handler.) Likewise between disabling dcache and cleaning.

These are privileged operations, so the standard cortex_m critical section should work (I'll file a separate bug about why I phrased that sentence like that).

It's a shame that compiler_fence has a panic path though, I'm not sure under what conditions that is optimised out.

Yeah, it really wants its Ordering as a const generic parameter, but that's of course not a thing yet.

[adamgreig]

re the docs, I'm looking at the Cortex-M7 User Guide, ARM DUI 0646B, pg296:

[adamgreig]

I'm concerned that this doesn't proactively alert any current users that their code is wrong, the way a deprecation might.

What would you replace all the instrinsics with, if deprecating them? Or are you only thinking about ISB/DSB (which we'd still need to offer somehow, of course)?

I have definitely had intermittent failures when enabling the dcache

It would be interesting to work out what's causing the failure here in case we need to put more things into the asm blob. Is it some interaction with the stack when dcache is getting turned on/off? Do we end up needing to do cache cleaning+disable all in asm in case a call hits the stack between disable and clean?

In reading these cache maintenance functions -- are they written assuming they are executing in critical sections?

The ARM CMSIS cache management methods do not enter a critical section or talk about them in the documentation, but clearly taking an interrupt in the middle of this sequence would be ruinous, so I wonder what's up with that. We could consider adding a critical section to all the cache management operations, but the overhead might be annoying in the more frequently executed methods.

[cbiffle]

What would you replace all the instrinsics with, if deprecating them? Or are you only thinking about ISB/DSB (which we'd still need to offer somehow, of course)?

Oh, sorry, I brought up deprecation as an example of how to communicate an issue to existing users. I don't know that you want to deprecate the existing intrinsics. Unless they prove unsalvageable of course.

Do we end up needing to do cache cleaning+disable all in asm in case a call hits the stack between disable and clean?

Probably. Any sequence provided as a literal assembly listing in either the v7-M ARM or the M4/M7 TRMs needs to be read very carefully before being rewritten in a higher-level language. I'd have to squint at cleaning and invalidation particularly.

The ARM CMSIS cache management methods do not enter a critical section or talk about them in the documentation, but clearly taking an interrupt in the middle of this sequence would be ruinous, so I wonder what's up with that.

Vendor code is often wrong. :-) I'm glad you are also seeing the issue there, I was concerned that I had missed something.

In a higher-level crate, if being in a critical section gives you a witness type, you could require that type to be passed in, proving a critical section exists, to avoid the overhead. (I felt like the interrupt::free function used to generate one of these, but I might be misremembering and thinking of some RTFM code.) In a lower-level crate, things probably just have to be unsafe with instructions that they need to be called without possibility of nonlocal control transfer to be used safely.

[adamgreig]

Probably. Any sequence provided as a literal assembly listing in either the v7-M ARM or the M4/M7 TRMs needs to be read very carefully before being rewritten in a higher-level language.

The CMSIS routines ARM recommends for cache management are fully in C so at least in principle I feel like we should be able to get Rust to do the right thing, the only caveat being the DSB/ISB intrinsics. I wonder if we can come to a clearer understanding of what precisely is causing the fault behaviour and so where a branch must be avoided and where it's acceptable; I'd rather not just rewrite the entire cache management in assembler just to be sure.

In a higher-level crate, if being in a critical section gives you a witness type, you could require that type to be passed in, proving a critical section exists, to avoid the overhead.

That's still the case; interrupt::free passes a &CriticalSection token to the closure, so we could implement unsafe underlying cache management operations and safe wrappers which require the critical section token and then call the unsafe methods.

[cbiffle]

The CMSIS routines ARM recommends for cache management are fully in C so at least in principle I feel like we should be able to get Rust to do the right thing, the only caveat being the DSB/ISB intrinsics.

This is a case where the correctness of the code, if written in a higher-level language, will require some understanding of compiler behavior.

• The C compilers ARM targets either implement __DSB and friends directly, or support forced-inlined functions with assembly bodies. So they're operating with the equivalent of the inline-asm feature.
• The C functions contain no abstractions or function calls, and are likely to be compiled as straight-line code even at -O0.

I agree that you could, in principle, write C code in Rust (i.e. avoiding register abstractions, using raw pointer writes) that would compile similarly in debug[1]. Would it be clearer/more maintainable/more obviously correct than assembly? Maybe! Can't rule it out until I've seen it, but I'd probably reach for assembly here for long-term stability.

[1]: note that I'm pretty sure I have observed the compiler out-lining core::ptr::write and friends at times, so that statement might not actually be true. This might have been a long time ago, or I might be misremembering.

The lack of a critical section requirement in the CMSIS code still appears to be a bug/omission in the CMSIS code. Based on past (and recent) experience I'd caution against assuming CMSIS code is correct without a deep understanding of how ARMCC processes it -- particularly in cases like this where the ARM recommendation interacts with vendor-implemented memory hierarchies and the like.

I wonder if we can come to a clearer understanding of what precisely is causing the fault behaviour and so where a branch must be avoided and where it's acceptable; I'd rather not just rewrite the entire cache management in assembler just to be sure.

I'll have a go at writing a reduction. But in the case of the basic cache maintenance operations, the ARM docs (which you excerpted) are pretty clear, and I'd caution against assuming that you can control instruction generation too precisely through several levels of indirection/compiler. It seems potentially fragile. (For instance, my inline-asm change only fixes things on --release, as far as I can tell, and that's only been tested on opt-level = "z" and codegen-units = 1 with lto.)

Re: witness types,

That's still the case

Oh good! So that would be an opportunity for a low-overhead safe API for unprivileged/monolithic apps.