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

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

Hi Aleksandar,

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.

Don't be too bad on yourself, we are human thus not perfect :) This is 
why having more that one (or not always the same) person reviewing is 
helpful.

You can share the blame with all the person subscribed to the list who 
did not look at the patch ;)

Regards,

Phil.

> 
> Thanks,
> Aleksandar
> 
>