Fix Xtensa big-endian instruction decoding

[cyrozap]

Fixes #8537

The Xtensa processor module was unable to correctly disassemble several instructions in big-endian mode due to bit-ordering issues. Essentially, because Xtensa reverses the order of the fields in big-endian instructions, not just the bytes, simply doing a 1:1 mapping of big-endian bits to little-endian bits as suggested in the comments is not enough. And this is because for certain instructions some of their fields are split into subfields, and the order of those subfields is always the same in the instruction word regardless of whether the instruction is big-endian or little-endian.

To fix this issue, first I went through all the instructions in the ISA manual one by one to identify instructions where subsets of bits in one or more instruction fields were used. Then I went through each of those instructions and corrected each of their field offsets, where necessary. Now there should be no instructions supported by this processor module that won't be decoded correctly in big-endian mode.

I've patched my locally-built version of Ghidra with these changes and have confirmed that the decoding issues I reported are now fixed.

[ghidra1]

@cyrozap Your changes appear inconsistent with the bit-reversal mapping that the Xtensa uses and we document within xtensaArch.sinc. This why we documented the LE to BE mapping and numbered our tokens with the LE bit numbers:

# little-endian -> big-endian 24-bit conversion chart
#|00|01|02|03|04|05|06|07|08|09|10|11|12|13|14|15|16|17|18|19|20|21|22|23|
#|23|22|21|20|19|18|17|16|15|14|13|12|11|10|09|08|07|06|05|04|03|02|01|00|

Your changes violate this mapping.

It is easy to fall prey to the binutils bug where the BE bit-encoding was broken for a period of time. I made this mistake a few years back when this bug was first introduced and had to revert my changes.

Xtensa uses bit-reversal rather than byte-reversal for instruction encoding when switching 
between big-endian (BE) and little-endian (LE) modes. The binutils bug involved mistakenly 
applying byte-reversal instead, and this error was introduced in version 2.36 and later 
corrected in version 2.39.

[cyrozap]

Your changes appear inconsistent with the bit-reversal mapping that the Xtensa uses and we document within xtensaArch.sinc. This why we documented the LE to BE mapping and numbered our tokens with the LE bit numbers:

# little-endian -> big-endian 24-bit conversion chart
#|00|01|02|03|04|05|06|07|08|09|10|11|12|13|14|15|16|17|18|19|20|21|22|23|
#|23|22|21|20|19|18|17|16|15|14|13|12|11|10|09|08|07|06|05|04|03|02|01|00|

Your changes violate this mapping.

Yes, my changes do violate this mapping, but the mapping is not how Xtensa big-endian instructions are encoded. This mapping is just a mapping of the big-endian bit numbering (used in the manual to describe the big-endian instruction formats) to the little-endian bit numbering used by Ghidra.

But this can be misleading when decoding individual bits in a subset of an instruction field, because Xtensa big-endian instructions are not simply bit-reversed versions of little-endian instructions. Rather, to swap between big-endian and little-endian instruction encodings, it's the order of the fields in the instruction word that are reversed, and then the order of the bytes of the instruction word in memory are also reversed.

This line takes care of the byte order swap, but we need to handle the field order swap in the token definitions. And while that code correctly handles the field order swap for instructions that use the whole fields, it does not correctly handle it when an instruction uses subsets of a field, because unlike what you might expect from the way the ISA manual is written, the order of the bits in each field in the instruction word is always the same regardless of the endianness of the processor.

For example, take a look at the encoding for SLLI. On page 610 of the ISA manual, the little-endian encoding of the instruction is shown. SLLI uses the RRR instruction format, described on page 656. Based on this information, you can see that in little-endian encoding, sa4 (the most-significant bit of the shift amount) is encoded in bit 20, which is the least-significant bit of op2. But when the field order is reversed for the big-endian encoding, because the bit order within each field is not reversed, sa4 should still be in the least-significant bit of op2, which in the big-endian word is bit 23, not bit 20. And when translating the big-endian bit numbering to the little-endian bit numbering used by Ghidra, sa4 is in bit 0 of the big-endian instruction word. This is why I changed u1_20 = (3,3) to u1_20 = (0,0) and u3_21_23 = (0,2) to u3_21_23 = (1,3).

It is easy to fall prey to the binutils bug where the BE bit-encoding was broken for a period of time. I made this mistake a few years back when this bug was first introduced and had to revert my changes.

Xtensa uses bit-reversal rather than byte-reversal for instruction encoding when switching 
between big-endian (BE) and little-endian (LE) modes. The binutils bug involved mistakenly 
applying byte-reversal instead, and this error was introduced in version 2.36 and later 
corrected in version 2.39.

As I stated previously, this is not entirely correct--Xtensa does not use bit-reversal, it uses field-reversal and byte-reversal. I think the confusion stems from the fact that, aside from using big-endian bit numbering, the ISA manual doesn't really make it obvious that the bit order within fields in an instruction word is always preserved. But when you realize that bit 0 of the big-endian instruction word is the most-significant bit and bit 23 is the least-significant bit, the bit ordering within each field becomes obvious--the lower the bit number, the more-significant the bit, and the higher the bit number, the less-significant the bit.

Also, I think I should note that I discovered this instruction decoding bug because I was trying to analyze a big-endian Xtensa binary in Ghidra, and while Ghidra was able to disassemble most instructions correctly, many instructions could not be disassembled at all. I knew the binary itself wasn't the problem because the binary ran 100% correctly on real hardware, so the problem had to be in Ghidra. And without the changes I've made in this PR, the binary cannot be disassembled correctly by Ghidra.

[ghidra1]

@cyrozap Could you please indicate what compiler toolchain and version was used to produce your test binary.

[ghidra1]

The documented formats within the Xtensa manual certainlydo not reflect conventional endianess byte-swap.
Xtensa ISA

[ghidra1]

I do agree that when defining a token field which corresponds to a specific bit within a format-defined field such as your sa4 example, we cannot apply the bit mapping which only applies to a fully intact format-defined field range.

[cyrozap]

Could you please indicate what compiler toolchain and version was used to produce your test binary.

The binary I'm analyzing is not something I built myself and there are no hints in the binary as to what toolchain was used to build it. If you're asking about the test binaries I included in #8537 that could not be disassembled without this change, those instructions were all copied byte-for-byte out of the binary I'm analyzing.

Here's some screenshots of code from the binary, disassembled by Ghidra (after being patched with the changes in this PR):

SLLI example:

MOVI.N example:

The documented formats within the Xtensa manual certainly do not reflect conventional endianess byte-swap.

Yeah, it's certainly very strange. I can only assume they made it this way to ensure that the major opcode is always in the first byte when read sequentially, since this is an ISA with mixed 16-bit and 24-bit instruction words. But yes it's not just a simple byte swap or bit reversal.

I do agree that when defining a token field which corresponds to a specific bit within a format-defined field such as your sa4 example, we cannot apply the bit mapping which only applies to a fully intact format-defined field range.

Yes, I think we might be able to make it more clear in the token definition by renaming the fields based on their field names (e.g., op0, op2, s, i, z, r, t, sa_3_0, sa_4, imm6_3_0, imm6_5_4, etc.) or based on the instruction format + field names (e.g., ri16_..., ri6_..., rrr_..., etc.), but I didn't want to risk breaking LE decoding with a big change like that, and I wanted to ensure that this PR would be straightforward to review and merge.

Currently, the only commit that could affect LE decoding at all is this one, which adds t2_4_5. I had to add the new field because I couldn't change u2_4_5 = (18,19), which is correct for some instructions.

[ghidra1]

I think we may need to do an overhaul on the token field naming to reflect <format-name>_<field>[_<field-lsb-pos>[_<bit-length>]] which may help with mapping and reduce confusion. Any use of a sub-field would need to be handled more carefully when mapping between LE and BE. We may be able to omit the format-name if field naming is consistent for a given instruction/token size. Examples:

rrr_imm16
rrr_op0
rrr_op0_0  (lsb)
rrr_op0_1_3   (upper 3-bits of 4-bit op0 field)

[cyrozap]

I think we may need to do an overhaul on the token field naming ...

Is that a blocker to getting this PR merged? Or can that renaming be done in another PR?

[GhidorahRex]

The PR is great as it is. We might have a separate task to update the token fields, but for now getting everything working correctly is a great addition.