On 9/11/20 3:41 AM, Peter Maydell wrote:
>> + /* Detect store by reading the instruction at the program counter. */
>> + uint32_t insn = *(uint32_t *)pc;
>> + switch(insn>>29) {
>> + case 0x5:
>> + switch((insn>>26) & 0x7) {
>
> Here we mask to get a 3-bit field...
>
>> + case 0x0: /* SB */
>> + case 0x1: /* SH */
>> + case 0x2: /* SWL */
>> + case 0x3: /* SW */
>> + case 0x4: /* SDL */
>> + case 0x5: /* SDR */
>> + case 0x6: /* SWR */
>> + case 0x7: /* CACHE */
>> + is_write = 1;
>
> ...but here all 8 cases are handled identically.
> Is there a typo/logic error here, or should this
> really just be
>
> case 0x5:
> /* SB, SH, SWL, SW, SDL, SDR, SWR, CACHE */
> is_write = 1;
>
> ?
>
> Is CACHE really a write insn ?
Indeed not. However, it's also illegal for user-mode, so we cannot arrive here
with SIGSEGV by executing it. So we could ignore that case and not decode this
field.
>> + case 0x7:
>> + switch((insn>>26) & 0x7) {
>> + case 0x0: /* SC */
>> + case 0x1: /* SWC1 */
>> + case 0x2: /* SWC2 */
>> + case 0x4: /* SCD */
>> + case 0x5: /* SDC1 */
>> + case 0x6: /* SDC2 */
>> + case 0x7: /* SD */
>> + is_write = 1;
Well, the unconditional check of SWC2/SDC2 is not quite right. MIPS64R6 removes
them and replaces them with some compact branches. That's easy enough to
include here, using
#if !defined(__mips_isa_rev) || __mips_isa_rev < 6
case 2: /* SWC2 */
case 6: /* SDC2 */
#endif
We should also add
#if defined(__mips16) || defined(__mips_micromips)
# error "Unsupported encoding"
#endif
I see no upstream compiler support for nanomips at all, so there's no point in
checking for that encoding. (Indeed, I wonder at the code in target/mips...
how could it be tested?)
r~
