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

[Philippe Mathieu-Daudé]

On 9/11/20 6:55 PM, Richard Henderson wrote:
> 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?)

I took the information from commit f7d257cb4a1
("qemu-doc: Add nanoMIPS ISA information") to add
the tests in  f375ad6a0d6 ("BootLinuxConsoleTest:
Test nanoMIPS kernels on the I7200 CPU"), but I
haven't tried to recompile these files myself.

Regards,

Phil.