[BUG] DSP panic with Zephyr on Intel MTL, regression 27th June

Describe the bug
A DSP panic started showing up in CI runs on 27th June. No individual PR merged recently shows this in test results, so suspcion is a combination of PRs merged. Current suspect:

Both passed tested independently, but in together there seem to be DSP panics.

To Reproduce
https://sof-ci.01.org/sofpr/PR9267/build6051/devicetest/index.html

Reproduction Rate
Very high (most PR CI runs on 27th June)

Expected behavior
No DSP panics

Impact
Blocking CI

Environment
https://sof-ci.01.org/sofpr/PR9265/build6047/devicetest/index.html
https://sof-ci.01.org/sofpr/PR9267/build6051/devicetest/index.html

Also seen in:

git bisect reliably brought to an SOF commit, that obviously cannot be the real reason - in a sense that that commit only adds 2 new functions without calling them and without touching a single line of runnable code, so it's purely modifying the build layout.
The crash happens at https://github.com/zephyrproject-rtos/zephyr/blob/b82b5b0734c30490368e21627423f10036487343/soc/intel/intel_adsp/ace/power_down.S#L64
That location is trying to lock in cache a variable on caller's stack https://github.com/zephyrproject-rtos/zephyr/blob/b82b5b0734c30490368e21627423f10036487343/soc/intel/intel_adsp/ace/power.c#L342 which all looks valid. However, it causes an exception. As a WA moving that variable to .bss by making it static seems to eliminate the problem.

@ceolin

@ceolin any ideas?

What does the assembly dump look like for pm_state_set and power_down?

One thing to clean up in power_down assembly is window exceptions. You're called in a context where register windowing is enabled, which means that any access to registers other than A0-A3 can in principle trap. And the first use of A11+ (the last window, after which no window exceptions will occur) doesn't happen until after you've locked three data and four instruction lines into the cache. I don't see why that's illegal, but if I wanted to place bets on "how to exercise weird core behavior", this would be on the list. Strongly suggest "pre-spilling" all registers by e.g. putting a and a15, a15, a15 # force window exceptions right after the ENTRY instruction.

Also, I note there are two "MOVI" pseudo instructions after you start locking cache lines which are going to end up pointing into an arbitrary literals location. You need an explicit L32R to be sure it lands in valid memory, not a compiler-generated MOVI.

Also, I note there are two "MOVI" pseudo instructions after you start locking cache lines

Oooh, and there are lots more in asm_memory_management.h in contexts where you've already started shutting down memory! I'm going to place my chips on this as the culprit. I give it 60%+ odds.

Someone needs to comb through these files and make sure there's no "MOVI" usage (which again to be clear: is only a CPU instruction for small values, for big ones the compiler gets fancy and emits a .literals record for the linker to find and place). I mean, maybe I'll lose the bet and these will turn out to all be valid/non-loading variants. But still I think style would demand this be cleaned up.

@andyross

Also, I note there are two "MOVI" pseudo instructions after you start locking cache lines

Oooh, and there are lots more in asm_memory_management.h in contexts where you've already started shutting down memory! I'm going to place my chips on this as the culprit. I give it 60%+ odds.

Someone needs to comb through these files and make sure there's no "MOVI" usage (which again to be clear: is only a CPU instruction for small values, for big ones the compiler gets fancy and emits a .literals record for the linker to find and place). I mean, maybe I'll lose the bet and these will turn out to all be valid/non-loading variants. But still I think style would demand this be cleaned up.

@andyross ouch, yes, that sounds like a problem. Thanks for finding it! Now somebody just needs to fix it...

@andyross

Also, I note there are two "MOVI" pseudo instructions after you start locking cache lines which are going to end up pointing into an arbitrary literals location. You need an explicit L32R to be sure it lands in valid memory, not a compiler-generated MOVI.

@andyross I looked again at these. And TBH I don't see a problem with those specific ones. Here are the lines we're talking about:
https://github.com/zephyrproject-rtos/zephyr/blob/cfbe2adabc511663776642616cdc75510db882d3/soc/intel/intel_adsp/ace/power_down.S#L53-L61
Firstly, they just lock some cache lines, all the memory is still powered on. The problem would occur if we tried to access RAM after it's powered off, correct? So, those locations before powering down RAM should be safe? Same about these two lines https://github.com/zephyrproject-rtos/zephyr/blob/cfbe2adabc511663776642616cdc75510db882d3/soc/intel/intel_adsp/ace/power_down.S#L67-L68

Oooh, and there are lots more in asm_memory_management.h in contexts where you've already started shutting down memory! I'm going to place my chips on this as the culprit. I give it 60%+ odds.

These ones - yes, agree, look dangerous...

@andyross

These ones - yes, agree, look dangerous...

@andyross @teburd that was a nice theory, but it doesn't seem to help: zephyrproject-rtos/zephyr#75174 doesn't fix the problem. What exactly did we bet, Andy? ;-)

and a new attempt to fix it zephyrproject-rtos/zephyr#78057

Posted this in the PR by accident, copying here:

FWIW: I still bet that if you whiteboxed a rig to enumerate all the dcache indexes and check how many of them can be pinned, we'd discover that something in the boot ROM or elsewhere has left a line pinned accidentally, preventing more pinning at that index, and we're just hitting that by bad luck in the linker.

Would be non-trivial assembly to write, and tedious to debug as the only feedback is a panic, but not "hard" hopefully.

@andyross

@andyross I think what I've tested that back when debugging this last time by doing either one or both of the following things:

trying to lock (and unlock) cache lines at all offsets to cover the entire associativity set - all succeeded
by trying to speculatively unlock the entire data cache by using DIU

diff --git a/soc/intel/intel_adsp/ace/power.c b/soc/intel/intel_adsp/ace/power.c
index 20efc8e6f97..61830897397 100644
--- a/soc/intel/intel_adsp/ace/power.c
+++ b/soc/intel/intel_adsp/ace/power.c
@@ -46,6 +46,9 @@ __imr void power_init(void)
 	cache_data_flush_range((__sparse_force void *)
 			sys_cache_cached_ptr_get(&adsp_pending_buffer),
 			sizeof(adsp_pending_buffer));
+	/* unlock entire data cache */
+	for (unsigned int i = 0; i < 16 * 1024; i += 64)
+		__asm__ __volatile__("diu %0, 0" : "r"(0xa0000000 + i));
 #endif /* CONFIG_SOC_INTEL_ACE15_MTPM */
 }

Neither had helped

@lyakh

@lyakh my read is there are three ways, so you need to prove you can lock two lines at each index to saturate. IIRC there are two separate dcache regions in the code in question, right? So if those overlap in index and happen to collide with something already in the cache, then we'd be in a situation where we're trying to lock the same index in all three ways, which is illegal.

Maybe. :)

@andyross

@andyross sure, but as I said - I also tried unlocking all cache lines and it didn't help either

@lyakh

@lyakh is everything working now? Can we close this issue?

@wszypelt

@wszypelt the last words are "... and it didn't help either"

?

@wszypelt

@wszypelt the last words are "... and it didn't help either"

?

@marc-hb that was a reply to a specific question, not a statement about the state of this bug

@lyakh

@lyakh is everything working now? Can we close this issue?

@wszypelt yes, so far zephyrproject-rtos/zephyr#78283 has fixed it

intel_adsp/ace: power: pad the hpsram_mask passed to power_down

Navigation Menu

Repository navigation

[BUG] DSP panic with Zephyr on Intel MTL, regression 27th June #9268

33 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[BUG] DSP panic with Zephyr on Intel MTL, regression 27th June #9268

Description

Activity

kv2019i commented on Jun 27, 2024

lyakh commented on Jun 27, 2024

lyakh commented on Jun 27, 2024

lyakh commented on Jun 27, 2024

teburd commented on Jun 27, 2024

andyross commented on Jun 27, 2024

andyross commented on Jun 27, 2024

lyakh commented on Jun 28, 2024

lyakh commented on Jun 28, 2024

lyakh commented on Jun 28, 2024

33 remaining items

lyakh commented on Sep 5, 2024

andyross commented on Sep 5, 2024

lyakh commented on Sep 6, 2024

andyross commented on Sep 7, 2024

lyakh commented on Sep 9, 2024

wszypelt commented on Sep 20, 2024

marc-hb commented on Sep 20, 2024

lyakh commented on Sep 23, 2024

lyakh commented on Sep 23, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions