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

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

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.
> 
> 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
> 
>