Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[Fredrik Noring]

[ 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