| Message ID | 1540213077-15211-1-git-send-email-aleksandar.markovic@rt-rk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22 October 2018 at 13:57, Aleksandar Markovic <aleksandar.markovic@rt-rk.com> wrote: > From: Aleksandar Markovic <amarkovic@wavecomp.com> > > The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: > > Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) > > are available in the git repository at: > > https://github.com/AMarkovic/qemu tags/mips-queue-oct-2018-part-2 > > for you to fetch changes up to 2ec219776c633df9e43c5fa1557f70ee4f735f9d: > > target/mips: Fix decoding of ALIGN and DALIGN instructions (2018-10-22 14:41:47 +0200) > > ---------------------------------------------------------------- > MIPS queue for October 2018 - part 2 > > Limited support for R5900 ISA, MMI ASE, and two misc fixes. > > ---------------------------------------------------------------- Hi: I get compile errors on 32-bit hosts: /home/petmay01/qemu-for-merges/disas/mips.c:615:35: error: large integer implicitly truncated to unsigned type [-Werror=overflow] #define INSN_5900 0x100000000 ^ /home/petmay01/qemu-for-merges/disas/mips.c:1200:17: note: in expansion of macro 'INSN_5900' #define EE INSN_5900 /* Emotion Engine */ ^ /home/petmay01/qemu-for-merges/disas/mips.c:2326:73: note: in expansion of macro 'EE' {"div1", "z,s,t", 0x7000001a, 0xfc00ffff, RD_s | RD_t | WR_HILO, 0, EE }, ^ (and repeats on other similar uses). This is because this line is an initializer for "struct mips_opcode", and the final field is "unsigned long membership", which may be only 32 bits wide, but you're trying to put a number in that's too big for that. thanks -- PMM
Hi Peter, Aleksandar, > Hi: I get compile errors on 32-bit hosts: > > /home/petmay01/qemu-for-merges/disas/mips.c:615:35: error: large > integer implicitly truncated to unsigned type [-Werror=overflow] > #define INSN_5900 0x100000000 > ^ > /home/petmay01/qemu-for-merges/disas/mips.c:1200:17: note: in > expansion of macro 'INSN_5900' > #define EE INSN_5900 /* Emotion Engine */ > ^ > /home/petmay01/qemu-for-merges/disas/mips.c:2326:73: note: in > expansion of macro 'EE' > {"div1", "z,s,t", 0x7000001a, 0xfc00ffff, RD_s | RD_t | WR_HILO, 0, EE }, > ^ > > (and repeats on other similar uses). > > This is because this line is an initializer for "struct mips_opcode", > and the final field is "unsigned long membership", which may be only > 32 bits wide, but you're trying to put a number in that's too big for that. I am sorry about that. We are out of bits. Option 1: Discard all disassembly parts of the series. I would prefer this if possible -- they are not essential now in my opinion. Option 2: Drop all R5900 related changes for now. Option 3: Extend the mips_opcode::membership field. Fredrik
On 10/23/18 9:37 PM, Fredrik Noring wrote: > Hi Peter, Aleksandar, > >> Hi: I get compile errors on 32-bit hosts: >> >> /home/petmay01/qemu-for-merges/disas/mips.c:615:35: error: large >> integer implicitly truncated to unsigned type [-Werror=overflow] >> #define INSN_5900 0x100000000 >> ^ >> /home/petmay01/qemu-for-merges/disas/mips.c:1200:17: note: in >> expansion of macro 'INSN_5900' >> #define EE INSN_5900 /* Emotion Engine */ >> ^ >> /home/petmay01/qemu-for-merges/disas/mips.c:2326:73: note: in >> expansion of macro 'EE' >> {"div1", "z,s,t", 0x7000001a, 0xfc00ffff, RD_s | RD_t | WR_HILO, 0, EE }, >> ^ >> >> (and repeats on other similar uses). >> >> This is because this line is an initializer for "struct mips_opcode", >> and the final field is "unsigned long membership", which may be only >> 32 bits wide, but you're trying to put a number in that's too big for that. > > I am sorry about that. We are out of bits. > > Option 1: Discard all disassembly parts of the series. I would prefer this > if possible -- they are not essential now in my opinion. > > Option 2: Drop all R5900 related changes for now. > > Option 3: Extend the mips_opcode::membership field. It's trivial to extend the field to uint64_t. r~
Hi Richard, > > Option 3: Extend the mips_opcode::membership field. > > It's trivial to extend the field to uint64_t. 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.) Fredrik --- a/disas/mips.c +++ b/disas/mips.c @@ -2549,12 +2553,16 @@ const struct mips_opcode mips_builtin_opcodes[] = {"madd", "s,t", 0x70000000, 0xfc00ffff, RD_s|RD_t|WR_HILO|IS_M, 0, G1 }, {"madd", "7,s,t", 0x70000000, 0xfc00e7ff, MOD_a|RD_s|RD_t, 0, D33 }, {"madd", "d,s,t", 0x70000000, 0xfc0007ff, RD_s|RD_t|WR_HILO|WR_d|IS_M, 0, G1 }, +{"madd1", "s,t", 0x70000020, 0xfc00ffff, RD_s | RD_t | WR_HILO | IS_M, 0, EE }, +{"madd1", "d,s,t", 0x70000020, 0xfc0007ff, RD_s | RD_t | WR_HILO | WR_d | IS_M, 0, EE }, {"maddp", "s,t", 0x70000441, 0xfc00ffff, RD_s|RD_t|MOD_HILO, 0, SMT }, {"maddu", "s,t", 0x0000001d, 0xfc00ffff, RD_s|RD_t|WR_HILO, 0, L1 }, {"maddu", "s,t", 0x70000001, 0xfc00ffff, RD_s|RD_t|MOD_HILO, 0, I32|N55 }, {"maddu", "s,t", 0x70000001, 0xfc00ffff, RD_s|RD_t|WR_HILO|IS_M, 0, G1 }, {"maddu", "7,s,t", 0x70000001, 0xfc00e7ff, MOD_a|RD_s|RD_t, 0, D33 }, {"maddu", "d,s,t", 0x70000001, 0xfc0007ff, RD_s|RD_t|WR_HILO|WR_d|IS_M, 0, G1 }, +{"maddu1", "s,t", 0x70000021, 0xfc00ffff, RD_s | RD_t | WR_HILO | IS_M, 0, EE }, +{"maddu1", "d,s,t", 0x70000021, 0xfc0007ff, RD_s | RD_t | WR_HILO | WR_d | IS_M, 0, EE }, {"madd16", "s,t", 0x00000028, 0xfc00ffff, RD_s|RD_t|MOD_HILO, 0, N411 }, {"max.ob", "X,Y,Q", 0x78000007, 0xfc20003f, WR_D|RD_S|RD_T|FP_D, 0, MX|SB1 }, {"max.ob", "D,S,T", 0x4ac00007, 0xffe0003f, WR_D|RD_S|RD_T, 0, N54 },
Hi Fredrik, > > > Option 3: Extend the mips_opcode::membership field. > > > > It's trivial to extend the field to uint64_t. > > 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). 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. HTH, Maciej
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
On 10/25/18 7:03 PM, Maciej W. Rozycki wrote: > 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. You can't do that because of GPL v3, sadly. r~
On Fri, 26 Oct 2018, Richard Henderson wrote: > > 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. > > You can't do that because of GPL v3, sadly. I've been aware of that, however the changes I mentioned are pretty mechanical and can be easily made from scratch by someone them who hasn't looked at binutils, even based on the description I already made. You don't copyright an idea, only actual written code. Maciej
From: Aleksandar Markovic <amarkovic@wavecomp.com> The following changes since commit b312532fd03413d0e6ae6767ec793a3e30f487b8: Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging (2018-10-19 19:01:07 +0100) are available in the git repository at: https://github.com/AMarkovic/qemu tags/mips-queue-oct-2018-part-2 for you to fetch changes up to 2ec219776c633df9e43c5fa1557f70ee4f735f9d: target/mips: Fix decoding of ALIGN and DALIGN instructions (2018-10-22 14:41:47 +0200) ---------------------------------------------------------------- MIPS queue for October 2018 - part 2 Limited support for R5900 ISA, MMI ASE, and two misc fixes. ---------------------------------------------------------------- Aleksandar Markovic (2): target/mips: Fix the title of translate.c target/mips: Fix decoding of ALIGN and DALIGN instructions Fredrik Noring (32): target/mips: Define R5900 ISA, MMI ASE, and R5900 CPU preprocessor constants disas/mips: Define R5900 disassembly constants target/mips: R5900 Multimedia Instruction overview note target/mips: Define R5900 MMI class, and LQ and SQ opcode constants target/mips: Define R5900 MMI{0, 1, 2, 3} subclasses and MMI opcode constants target/mips: Define R5900 MMI0 opcode constants target/mips: Define R5900 MMI1 opcode constants target/mips: Define R5900 MMI2 opcode constants target/mips: Define R5900 MMI3 opcode constants target/mips: Placeholder for R5900 SQ, handle user mode RDHWR target/mips: Placeholder for R5900 LQ target/mips: Placeholder for R5900 MMI instruction class target/mips: Placeholder for R5900 MMI0 instruction subclass target/mips: Placeholder for R5900 MMI1 instruction subclass target/mips: Placeholder for R5900 MMI2 instruction subclass target/mips: Placeholder for R5900 MMI3 instruction subclass target/mips: Support R5900 three-operand MULT and MULTU instructions target/mips: Support R5900 three-operand MULT1 and MULTU1 instructions target/mips: Support R5900 MFLO1, MTLO1, MFHI1 and MTHI1 instructions target/mips: Support R5900 DIV1 and DIVU1 instructions target/mips: Support R5900 MOVN, MOVZ and PREF instructions from MIPS IV target/mips: R5900 DMULT[U], DDIV[U], LL[D] and SC[D] are user only tests/tcg/mips: Test R5900 three-operand MULT tests/tcg/mips: Test R5900 three-operand MULTU tests/tcg/mips: Test R5900 three-operand MULT1 tests/tcg/mips: Test R5900 three-operand MULTU1 tests/tcg/mips: Test R5900 MFLO1 and MFHI1 tests/tcg/mips: Test R5900 MTLO1 and MTHI1 tests/tcg/mips: Test R5900 DIV1 tests/tcg/mips: Test R5900 DIVU1 target/mips: Define the R5900 CPU linux-user/mips: Recognise the R5900 CPU model disas/mips.c | 16 + linux-user/mips/target_elf.h | 3 + target/mips/mips-defs.h | 3 + target/mips/translate.c | 865 ++++++++++++++++++++++++++++++++++++- target/mips/translate_init.inc.c | 59 +++ tests/tcg/mips/mipsr5900/Makefile | 30 ++ tests/tcg/mips/mipsr5900/div1.c | 73 ++++ tests/tcg/mips/mipsr5900/divu1.c | 48 ++ tests/tcg/mips/mipsr5900/mflohi1.c | 35 ++ tests/tcg/mips/mipsr5900/mtlohi1.c | 40 ++ tests/tcg/mips/mipsr5900/mult.c | 76 ++++ tests/tcg/mips/mipsr5900/multu.c | 68 +++ 12 files changed, 1297 insertions(+), 19 deletions(-) create mode 100644 tests/tcg/mips/mipsr5900/Makefile create mode 100644 tests/tcg/mips/mipsr5900/div1.c create mode 100644 tests/tcg/mips/mipsr5900/divu1.c create mode 100644 tests/tcg/mips/mipsr5900/mflohi1.c create mode 100644 tests/tcg/mips/mipsr5900/mtlohi1.c create mode 100644 tests/tcg/mips/mipsr5900/mult.c create mode 100644 tests/tcg/mips/mipsr5900/multu.c