Re: [Qemu-devel] [PATCH v8 00/38] target/mips: Limited support for the R5900

[Fredrik Noring <noring@nocrew.org>]

Hi Maciej,

> > Is the membership field intended to be used? The opcodes for CLZ and CLO
> > clash with the R5900 opcodes for MADD1 and MADDU1, resulting in incorrect
> > disassembly of MADD1 and MADDU1. For example:
> > 
> > 	0x70853020 madd1  a2,a0,a1  disassembles into  clz a2 or a1,a0
> > 	0x70853021 maddu1 a2,a0,a1  disassembles into  clo a2 or a1,a0
> > 
> > (CLZ and CLO are members of I32|N55, whereas MADD1 and MADDU1 are EE.)
> 
>  It looks like a disassembler bug somewhere then (maybe in your patched 
> version only), because the R5900 is not supposed to match I32 (because it 
> does not implement the MIPS32 ISA; it's only MIPS I aka I1 with additions 
> or MIPS IV aka I4 with exclusions, or anything between with both additions 
> and exclusions, with I believe MIPS III aka I3 being the closest match), 
> and it is not supposed to match N55 either (because it is obviously not a 
> Vr5500 processor).

I think the "bug" is that the membership field is defined but unused, so
opcode memberships are simply ignored. OPCODE_IS_MEMBER is defined to be
always true, for all opcodes and all ISAs.

>  Overall this source file is clearly a modified copy of an ancient version 
> of the opcode table included with the opcodes library from binutils and I 
> think it would benefit from a refresh.  In particular separating an ASE 
> field and adding an exclusions field, as it has been done with opcodes, 
> would make it much easier to maintain this table.  The table in opcodes is 
> already messy due to several exceptions to the alphabetical order (and it 
> could be improved a bit I believe), but I find its QEMU version even 
> messier.

Agreed! QEMU's scripts/checkpatch.pl warns and errors on 80 and 90 column
violations, so trying avoid check breakage leaves the table unaligned too.

Fredrik