Re: [PATCH 1/1] accel/tcg/user-exec: support computing is_write for mips32

[Richard Henderson <richard.henderson@linaro.org>]

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~