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

[Emilio G. Cota]

Cc'ing Fredrik, who I think was the intended recipient of the below.

                E.

On Thu, Nov 01, 2018 at 11:06:30 +0000, 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.
> 
> 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?
> 
> 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.
> 
> Thanks,
> Aleksandar
> 
>