xtensa: add SALT/SALTU opcodes to esp32/esp32s3 core - #41
Open
hinaultd wants to merge 2 commits into
Open
Conversation
The generic instruction semantics for SALT (Set-A-Less-Than) and SALTU (Set-A-Less-Than-Unsigned) already exist in translate.c (core_ops[], .name = "salt"/"saltu", translate_salt), but the esp32 core opcode table (core-esp32/xtensa-modules.c.inc, reused as-is by core-esp32s3.c) never declares these opcodes, so they are never recognized by the decoder for this core and any code using them aborts with "unrecognized opcode in slot 0". Real-world impact: xtensa-esp32s3-elf-gcc emits SALTU/SALT for the common range-check idiom (low <= p < high), e.g. in the ESP-IDF esp_ptr_executable/esp_ptr_in_dram family of helpers. Any ESP32 or ESP32-S3 firmware exercising these helpers hits the unrecognized opcode abort. This adds the missing table entries (iclass, iclass args, opcode enum values, name-to-iclass mapping, encode functions, and the Slot_inst_decode branches for op1==2, op2==6/7) mirroring the existing entries already present for other cores that support this instruction (e.g. core-de233_fpu), and slots them into the existing op1==2 decode branch used by ANDB/ORB/XORB/MULL/QUOU/etc, which already handles every other opcode in that group. Verified: the esp32/esp32s3 xtensa-modules.c.inc changes compile cleanly with the existing CMake build (-DTARGET_ARCH=xtensa).
xtensa_modules.isa_internal (the top-level ISA struct at the end of xtensa-modules.c.inc) carries the opcodes/iclasses array lengths as hardcoded literals (390/513), not ARRAY_SIZE() expressions. Adding the salt/saltu opcode and iclass entries without bumping these two numbers left them at their old counts, which config->opcode_ops[] (malloc'd from that same count in helper.c's init_libisa) is sized from. The two new opcode IDs then indexed past the end of that allocation, reading garbage XtensaOpcodeOps pointers, observed as a bogus non-zero ops->coprocessor value that spuriously raised a COPROCESSOR_DISABLED exception every time SALT/SALTU executed, instead of running them as plain ALU instructions. Bumped iclasses 390 -> 391 and opcodes 513 -> 515 to match the one new iclass and two new opcodes added by the previous commit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The generic instruction semantics for
SALT(Set-A-Less-Than) andSALTU(Set-A-Less-Than-Unsigned) already exist in
translate.c(core_ops[],.name = "salt"/"saltu",translate_salt), but the esp32 core opcode table(
arch/xtensa/core-esp32/xtensa-modules.c.inc, reused as-is bycore-esp32s3.c) never declares these opcodes, so they are never recognizedby the decoder for this core, and any code using them aborts with
unrecognized opcode in slot 0.Real-world impact:
xtensa-esp32s3-elf-gccemitsSALTU/SALTfor thecommon range-check idiom (
low <= p < high), e.g. in the ESP-IDFesp_ptr_executable/esp_ptr_in_dramfamily of helpers. Any ESP32 orESP32-S3 firmware exercising these helpers hits the unrecognized-opcode
abort under Renode.
What this adds
Mirrors the entries already present for other cores that support this
instruction (e.g.
core-de233_fpu), slotted into the existingop1==2decode branch used by
ANDB/ORB/XORB/MULL/QUOU/etc., which alreadyhandles every other opcode in that group:
Iclass_xt_iclass_salt_args[](3 args:arrout,ars/artin)iclasses[]entry +ICLASS_xt_iclass_saltenum valueOpcode_salt_Slot_inst_encode/Opcode_saltu_Slot_inst_encode(encodefunctions, bit patterns identical to the ones already used by cores that
support this opcode — it is a standard/core Xtensa RRR-format opcode, not
a config-specific extension)
Opcode_salt_encode_fns[]/Opcode_saltu_encode_fns[]opcodes[]name-table entries ("salt","saltu")OPCODE_SALT/OPCODE_SALTUenum valuesSlot_inst_decodebranches (op2==6->OPCODE_SALTU,op2==7->OPCODE_SALT) in the existingop1==2blockTesting
Compiles cleanly with the existing CMake build
(
cmake -DTARGET_ARCH=xtensa -DCMAKE_BUILD_TYPE=Release ..), and correctlydecodes/executes
saltu a8, a8, a9instructions from a real ESP32-S3firmware ELF (traced via
esp_ptr_executable) that previously aborted withunrecognized opcode in slot 0 (pc = 0x403814f0).I was not able to do a full end-to-end runtime verification against a
locally-built Renode, since the native library ABI expected by my local
Renode install did not match what a from-source tlib build produces
(missing unrelated symbols from a newer/private build) — that is an
environment/build-pinning issue on my end, unrelated to this specific
change. The opcode-table addition itself is otherwise a self-contained,
mechanical patch following the exact pattern already used by other cores
in this same file.