esp32s3: fix register-window corruption under interrupt load - #5600
Conversation
|
Size difference with the dev branch: Binary size differenceflash ram before after diff before after diff 19684 19684 0 0.00% 7052 7052 0 0.00% tinygo build -size short -o ./build/test.hex -target=feather-rp2040 ./examples/adafruit4650 63228 63228 0 0.00% 6788 6788 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adt7410/main.go 10024 10024 0 0.00% 5348 5348 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/adxl345/main.go 14628 14628 0 0.00% 7396 7396 0 0.00% tinygo build -size short -o ./build/test.hex -target=pybadge ./examples/amg88xx 10184 10184 0 0.00% 5348 5348 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/main.go 12592 12592 0 0.00% 7172 7172 0 0.00% tinygo build -size short -o ./build/test.hex -target=nano-33-ble ./examples/apds9960/proximity/main.go 11136 11136 0 0.00% 5360 5360 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/apa102/itsybitsy-m0/main.go 7588 7588 0 0.00% 2312 2312 0 0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/at24cx/main.go 9272 9272 0 0.00% 5340 5340 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bh1750/main.go 8660 8660 0 0.00% 5340 5340 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/blinkm/main.go 71408 71408 0 0.00% 3656 3656 0 0.00% tinygo build -size short -o ./build/test.hex -target=pinetime ./examples/bma42x/main.go 67120 67120 0 0.00% 6804 6804 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmi160/main.go 28952 28952 0 0.00% 5380 5380 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp180/main.go 65720 65720 0 0.00% 6828 6828 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/bmp280/main.go 13080 13080 0 0.00% 5412 5412 0 0.00% tinygo build -size short -o ./build/test.hex -target=trinket-m0 ./examples/bmp388/main.go 24024 24024 0 0.00% 6220 6220 0 0.00% tinygo build -size short -o ./build/test.hex -target=metro-rp2350 ./examples/bno08x/i2c/main.go 8008 8008 0 0.00% 3344 3344 0 0.00% tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/sram/main.go 22116 22116 0 0.00% 3548 3548 0 0.00% tinygo build -size short -o ./build/test.hex -target=bluepill ./examples/ds1307/time/main.go 30556 30556 0 0.00% 5576 5576 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/ds3231/alarms/main.go 44492 44492 0 0.00% 5576 5576 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/ds3231/basic/main.go 4592 4592 0 0.00% 2272 2272 0 0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/easystepper/main.go 71884 71884 0 0.00% 7588 7588 0 0.00% tinygo build -size short -o ./build/test.hex -target=itsybitsy-m0 ./examples/flash/console/spi 67996 67996 0 0.00% 9628 9628 0 0.00% tinygo build -size short -o ./build/test.hex -target=pyportal ./examples/flash/console/qspi 7128 7128 0 0.00% 2276 2276 0 0.00% tinygo build -size short -o ./build/test.hex -target=microbit ./examples/gc9a01/main.go 694072 694072 0 0.00% 131564 131564 0 0.00% |
|
let's see what copilot makes of the assembly :-) |
There was a problem hiding this comment.
Pull request overview
This PR hardens ESP32-S3 Xtensa windowed-ABI handling under heavy interrupt/task-switch load to prevent register-window corruption, which is required for stable BLE operation.
Changes:
- Updates
esp32s3.ldROM symbol definitions for Bluetooth/clock/interrupt usage and avoids defining C3-style BT hook symbols on S3. - Adjusts the level-1 interrupt/exception handler to reserve extra stack space to avoid clobbering the windowed-ABI save area and adds a dedicated MOVSP/alloca exception handler.
- Refines
tinygo_swapTaskto keep INTLEVEL masked across the stack switch and preserve the running frame’sWINDOWSTARTbit.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
targets/esp32s3.ld |
Adds/organizes ESP32-S3 ROM symbols needed for BLE and removes problematic hook symbol definitions. |
targets/esp32s3-interrupts.S |
Enlarges the exception frame to preserve the ABI save gap and adds MOVSP/alloca handling. |
src/internal/task/task_stack_esp32.S |
Prevents window-save-area corruption during task stack switching by adjusting interrupt masking and WINDOWSTART. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @@ -72,10 +72,13 @@ tinygo_swapTask: | |||
| // below, after the stack switch is done. This prevents stale overflow | |||
| // when the new goroutine's calls rotate back into these panes. | |||
| // Reload the saved PS, but do NOT restore it yet: everything from the stack | ||
| // switch below through the retw.n must stay masked. Once sp points at the | ||
| // new task's stack, the memory below sp is that task's save area -- the | ||
| // a0-a3 the retw.n is about to reload through a window underflow. An | ||
| // interrupt landing in that gap builds its frame there and overwrites them, | ||
| // and a garbage a0 makes the next retw an Illegal Instruction. |
| addi a0, a1, -96 // a0 = new frame pointer | ||
| // 64: SAR 68: EPC1 72: PS 76: WINDOWBASE 80: WINDOWSTART | ||
| // | ||
| // 128, not the 88 the layout needs: the 16 bytes below the interrupted SP |
470e417 to
59b5092
Compare
|
@dkegel-fastly just updated to accomodate the feedback items. |
|
Lots of comments with no explicit reference in them. Would you mind adding references? i.e: "we use ARM M9 Specification (https://arm.manuf.com/specm9) [...] as seen in section 3.9 blabla" and "we setup goroutine stack registers so they don't spill (See src/runtime/goroutine_stack.go). Else I have no way of checking validity without going through the same deep investigative process you've already done for this PR. |
59b5092 to
03a65af
Compare
|
Updated comments to reduce wordiness and add references. Very good call on that, @soypat |
| rsync | ||
| // Mask interrupts until WindowStart matches the new stack, or one lands in | ||
| // the gap and clobbers the a0-a3 that retw.n reloads. | ||
| // Xtensa ISA Reference Manual, RETW, section 8.3.253, p.565. |
There was a problem hiding this comment.
I'm having a hard time finding this reference. the ISA ref manual I have comes from here:https://0x04.net/~mwk/doc/xtensa.pdf#page=622.08. There are not 253 subsubsections. Page 565 talks of XORB instruction which does not seem to be the same thing this references. Please provide a source for the edition of the manual you used
There was a problem hiding this comment.
There was a problem hiding this comment.
Next time try the official docs maybe? I am not sure where that website obtained its PDF from
The ESP32 processor architecture (Xtensa) is from Tensilica (now called Cadence).
There was a problem hiding this comment.
I added a link to the official ISA docs and updated the PR accordingly.
There was a problem hiding this comment.
Official docs just don't appear in google 🤷 that's why I was asking where you were getting your info. We previously got our docs from this site (it's the site you just replaced in your most recent commit)
Remove the C3 bluetooth hook addresses from esp32s3.ld (on the S3 they point into the ROM md5/crc thunk table, and being bare assignments they also shadowed the blob's own definitions), keep the interrupt frame clear of the 16-byte windowed-ABI save area below SP, and make tinygo_swapTask hold INTLEVEL across the stack switch while keeping the running frame's WINDOWSTART bit set. Signed-off-by: deadprogram <ron@hybridgroup.com>
03a65af to
c97bbcc
Compare
|
Thank you for review @soypat now merging. |
|
Even though the PR only mentions esp32s3, this also solved severe crash problems on esp32 which started in #5510 but is now stable. |
Remove the C3 bluetooth hook addresses from esp32s3.ld (on the S3 they point into the ROM md5/crc thunk table, and being bare assignments they also shadowed the blob's own definitions), keep the interrupt frame clear of the 16-byte windowed-ABI save area below SP, and make tinygo_swapTask hold INTLEVEL across the stack switch while keeping the running frame's WINDOWSTART bit set.
Needed for BLE support on esp32s3.