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

[Aleksandar Markovic]

> ... in the patch series I posted 25 October "[PATCH 00/11]
> target/mips: Amend R5900 support". I will post updated patches shortly!

Fridrik,

It is now code freeze before 3.1, the code base is being stabilized, and only 
important fixes are allowed to be integrated - so, in that light, a separate 
patch, or a small series, that addresses only concerns from the original mail 
of this thread is needed. Such series should not contain any additional 
features (like your v2 of the series "Amend..." does), and its patch titles 
should look like "Fix decoding mechanism of ..." or such.

Could you please provide those appropriate changes in that format?

Thanks,
Aleksandar

________________________________________
From: Fredrik Noring <address@hidden>
Sent: Thursday, November 1, 2018 6:23:53 PM
To: Philippe Mathieu-Daudé; Emilio G. Cota; Aleksandar Markovic
Cc: Stefan Markovic; Petar Jovanovic; Aleksandar Rikalo; Maciej W. Rozycki; 
Jürgen Urban; address@hidden
Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding

[ 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