asm::delay() only blocks half the time intended on an STM32H7

[nkrackow]

The asm::delay() function delays a program only by half the time/CPU cycles intended on an STM32H7.
I have prepared a somewhat minimal example here. My CPU clock is at 400 MHz and I specify a delay of 4_000_000_000 cycles which should lead to a 10 second delay. The actual delay is 5 seconds for me.

I have also confirmed the same behavior in other programs on an STM32H7 with much shorter timescales and looking at IOs on a scope. The delay is always exactly half the time expected.

One guess is that this is due to the dual issue pipeline of the H7 eating up two instructions per cycle.

[nkrackow]

Oh I forgot to mention that I tried this with 0.7.4 and the current master. In my example repo I patch in the current master.

[adamgreig]

Hm, that's annoying, we deliberately double the number of cycles delay() blocks for in #312 to accommodate the dual issue pipeline of the Cortex-M7 (#236); in fact this led to complaints about delaying too long on Cortex-M3/4 in #325.

I vaguely recall checking this on an stm32h7 and getting the right result, I'll see if I can dig it out and investigate a bit further. I guess we could read CPUID at runtime to try and work out what CPU we're on, but even then things like whether execution is from flash or RAM or XIP (Q)SPI flash, whether i-cache is on, whether a flash accelerator is configured, etc etc etc will all heavily influence the number of cycles. The current calculation is a two-instruction loop and it runs half the specified number of cycles of iterations, i.e. it will retire as many instructions as requested delay.

It might be that the best fix we can do is make the documentation even more vague on precisely how many cycles of delay you'll get...

[nkrackow]

Sorry.. ;)
I mean please do double check my example or try with other code. I tired to exclude other variables where possible and had some discussion with @jordens but there might still be another explanation/condition causing this for me.

But since this actually broke my code which had to wait for an external device reset I think it's an important bug.

[jordens]

I suspect the only practical solution might indeed be to not touch the scaling factor anymore for behavioral stability/compatibility and make the docs say that this is an uncalibrated delay with both an an offset and a scaling factor that depend on cortex-m architecture and e.g. enabled cpu/code location/cache features.

[adamgreig]

I double checked this in #325 (comment) and indeed we currently only delay half the required time on CM7; the fix in #312 doesn't fully account for the core being able to issue both instructions in the loop at the same time.

I guess ideally we'd experiment a bit more about when a CM7 core will or won't dual-issue the loop, but if it seems like it does always manage it, I'd be inclined to try doing feature detection and delaying for about the right number of cycles for the current core, rather than being wrong on everything (as currently) or 3x worse on one core than another (if we fixed it for cm7).

[jordens]

I'd definitely prefer guaranteeing an error on the long delay side rather than trying "about right". For the frequent applications of e.g. guaranteeing datasheet timing a relative error of 3 is acceptable (if annoying) while 0.9 would be unacceptable.

[Indy2222]

We have encountered the same issue. There is a stack overflow ticket on the same behaviour https://stackoverflow.com/questions/74550647/arm-cortex-m7-assembly-timing-on-simple-delay-loop-how-to-explain-results

I think that branch folding/branch prediction speculatively pre-calculates the subtraction during the conditional branch. Combined with the superscalar Cortex-M7, only one CPU cycle is used during each iteration. A possible solution would be to place multiple subtractions (let's say 5) and then use BNB to make the timing as consistent as possible across different chips.

[cbiffle]

In case you're curious, I've done some reverse engineering of the Cortex-M7's dual issue capability here, after this bug in cortex_m::asm::delay broke some of our server firmware at Oxide.

The M7 will reliably issue a basic loop tail of the form

ADDR    subs rX, #1
ADDR+2  bne ADDR

...in one cycle per iteration, unless ADDR % 8 == 6. If the two instructions cross an 8-byte boundary, they issue separately. Can't tell you why, since ARM are annoying cagey about the M7's dual issue capability. But, this means that from build to build you can get different delays, depending on linker behavior, code size, etc.

We've gone through and doubled the parameter to delay on any code that might run on an M7. I think given the uncertainty of delay loops across microarchitectures and memory hierarchies, this might be the wrong primitive to provide in the cortex_m crate -- but at the very least it should probably stop dividing its parameter by 2 if it's intended to support M7.

[adamgreig]

Thanks for the report and details!

The problem is it's such a useful primitive to provide for very basic testing, but clearly one about which only the loosest of guarantees can be given. I suppose the answer is probably to bump it up to x loop iterations in the next release, document the behaviour more clearly including the architecture dependency, and go from there. I'm not convinced trying to do CPU feature detection really makes sense here but open to seeing what it would look like in practice if anyone wants to try.

[cbiffle]

Yeah, I considered proposing something with CPU feature detection, but... I mean there's only so far you can push that, and you can't (for instance) detect that this is a 3-wait-state flash or whatever.

So I think trying to keep it at 1+ cycle per iteration and documenting it as doing X iterations may be the best portable compromise.

[cbiffle]

This seems like a potentially breaking change, is there a process I would follow beyond sending a PR? I'm happy to try and fix it.