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