From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:43414)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <cota@braap.org>) id 1gIE6v-000589-T6
	for qemu-devel@nongnu.org; Thu, 01 Nov 2018 10:38:17 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <cota@braap.org>) id 1gIE3z-0005Lm-8W
	for qemu-devel@nongnu.org; Thu, 01 Nov 2018 10:35:14 -0400
Received: from out3-smtp.messagingengine.com ([66.111.4.27]:49105)
	by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32)
	(Exim 4.71) (envelope-from <cota@braap.org>) id 1gIE3z-0005LU-23
	for qemu-devel@nongnu.org; Thu, 01 Nov 2018 10:35:11 -0400
Date: Thu, 1 Nov 2018 10:35:09 -0400
From: "Emilio G. Cota" <cota@braap.org>
Message-ID: <20181101143509.GA23431@flamenco>
References: <BN6PR2201MB125116B5479010DB326DCCFCC6CE0@BN6PR2201MB1251.namprd22.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BN6PR2201MB125116B5479010DB326DCCFCC6CE0@BN6PR2201MB1251.namprd22.prod.outlook.com>
Subject: Re: [Qemu-devel] Correction needed for R5900 instruction decoding
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel/>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Aleksandar Markovic <amarkovic@wavecomp.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>, Stefan Markovic <smarkovic@wavecomp.com>, Petar Jovanovic <pjovanovic@wavecomp.com>, Aleksandar Rikalo <arikalo@wavecomp.com>, Fredrik Noring <noring@nocrew.org>

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