[ Philippe and Emilio -- thank you for cc-ing me. Good catch, since I'm not subscribed to the QEMU mailing list. Changes to the R5900 emulation are certainly of interest. ]
Hi Aleksandar, Philippe, On Thu, Nov 01, 2018 at 03:31:54PM +0100, Philippe Mathieu-Daudé wrote: > Cc'ing Fredrik. > > On 1/11/18 12:06, Aleksandar Markovic wrote: > > Hi, Fridrik, > > > > I did some closer code inspection of R5900 in last few days, and I > > noticed some sub-optimal implementation in the area where R5900-specific > > opcodes overlap with the rest-of-MIPS-CPUs opcodes. > > > > The right implementation should be based on the principle that all such > > cases are covered with if statements involving INSN_R5900 flag, like > > this: > > > > if (ctx->insn_flags & INSN_R5900) { > > <R5900-specific handling> > > } else { > > <rest-of-MIPS-handling> > > } > > > > You followed that principle for OPC_SPECIAL2 and OPC_SPECIAL3, but for > > some other opcodes not. For example, there are lines: > > > > if (reg == 0 && (opc == OPC_MFHI || opc == TX79_MMI_MFHI1 || > > opc == OPC_MFLO || opc == TX79_MMI_MFLO1)) { > > > > or > > > > switch (opc) { > > case OPC_MFHI: > > case TX79_MMI_MFHI1: > > > > Such implementation makes it difficult to discern R5900 and non-R5900 > > cases. Potentialy allows bugs to sneak in and affect non-R5900 support. MFLO1, MFHI1, MTLO1 and MTHI1 for the TX79 and the R5900 are already decoded in the ISA specific decode_tx79_mmi function, and thereby follow your first suggested pattern. They do however reuse the gen_HILO function, but it is a simpel matter to post a patch to make a new gen_tx79_HILO1 variant that is almost identical to the original gen_HILO. The only other case is gen_muldiv that is used for DIV1 and DIVU1. The same argument applies and a TX79 specific variant would be similar to the original, but I can certainly post a variant for that one too. > > The correction is not that difficult, I gather. Worse comme to worst, > > you can remove R5900 MFLO1 and MFHI1 altogether, they are not that > > essential at this moment, but do try correcting the decoding stuff as I > > described. Can you please make these changes in next few days or so > > (given that 3.1 release is getting closer and closer), and send them to > > the list? MFLO1 and MFHI1 are essential for MULT1, MULTU1, DIV1 and DIVU1 as well as MADD1 and MADDU1 in the patch series I posted 25 October "[PATCH 00/11] target/mips: Amend R5900 support". I will post updated patches shortly! > > It is my bad that I didn't spot this during review, but in any case, I > > think this should be fixed in 3.1 to make sure that non-R5900 > > functionalities are intact. It is a common pattern in target/mips/translate.c to cover several ISAs in the same gen_* and decode_* functions, especially when there are only minor differences between them. Fredrik