From patchwork Wed May 8 06:50:09 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aurelien Jarno X-Patchwork-Id: 242507 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 879332C010E for ; Wed, 8 May 2013 16:50:34 +1000 (EST) Received: from localhost ([::1]:48467 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZyCu-0002tb-KD for incoming@patchwork.ozlabs.org; Wed, 08 May 2013 02:50:32 -0400 Received: from eggs.gnu.org ([208.118.235.92]:42359) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZyCe-0002tV-7P for qemu-devel@nongnu.org; Wed, 08 May 2013 02:50:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UZyCc-0008Di-Ik for qemu-devel@nongnu.org; Wed, 08 May 2013 02:50:16 -0400 Received: from hall.aurel32.net ([2001:470:1f15:c4f::1]:55355) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UZyCc-0008DU-82; Wed, 08 May 2013 02:50:14 -0400 Received: from [2001:470:d4ed:0:ea11:32ff:fea1:831a] (helo=ohm.aurel32.net) by hall.aurel32.net with esmtpsa (TLS1.0:DHE_RSA_AES_128_CBC_SHA1:16) (Exim 4.72) (envelope-from ) id 1UZyCY-0003eP-VR; Wed, 08 May 2013 08:50:11 +0200 Received: from aurel32 by ohm.aurel32.net with local (Exim 4.80) (envelope-from ) id 1UZyCX-0005Sh-LG; Wed, 08 May 2013 08:50:09 +0200 Date: Wed, 8 May 2013 08:50:09 +0200 From: Aurelien Jarno To: Torbjorn Granlund Message-ID: <20130508065009.GP5000@ohm.aurel32.net> References: <86sj20rql4.fsf@shell.gmplib.org> <5187ECAD.4050901@suse.de> <86obcorn76.fsf@shell.gmplib.org> <15FCEEAE-FE2D-44B9-9DC3-5419B29D5B16@suse.de> <86a9o7qe3u.fsf_-_@shell.gmplib.org> <86fvxypyru.fsf_-_@shell.gmplib.org> <518935E4.70908@suse.de> <8638typsnp.fsf@shell.gmplib.org> <86ppx2oaen.fsf@shell.gmplib.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <86ppx2oaen.fsf@shell.gmplib.org> X-Mailer: Mutt 1.5.21 (2010-09-15) User-Agent: Mutt/1.5.21 (2010-09-15) X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2001:470:1f15:c4f::1 Cc: Richard Henderson , qemu-ppc@nongnu.org, Alexander Graf , qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [Qemu-ppc] Incorrect handling of more PPC64 insns (PATCH) X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Tue, May 07, 2013 at 09:30:24PM +0200, Torbjorn Granlund wrote: > I realised a possible problem with my suggested patch. > > What about a 32-bit processor? Then NARROW_MODE macro is identical 0. > > The pre-patch behaviour was then to ignore the L bit and decode both > 32-bit and 64-bit instruction in the same way. > > Apparently that is correct behaviour. (The manual is slightly vague, > but I let hardware decide.) > > With my patch, the bit is not ignored, and invalid code will be > generated for 32-bit targets, if they'd set the L bit. > > Here is an uglier but hopefully completely correct patch. > > diff --git a/target-ppc/translate.c b/target-ppc/translate.c > index 1a84653..69d684c 100644 > --- a/target-ppc/translate.c > +++ b/target-ppc/translate.c > @@ -675,49 +675,65 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg) > /* cmp */ > static void gen_cmp(DisasContext *ctx) > { > - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { > +#if defined(TARGET_PPC64) > + if (!(ctx->opcode & 0x00200000)) { > +#endif > gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], > 1, crfD(ctx->opcode)); > +#if defined(TARGET_PPC64) > } else { > gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], > 1, crfD(ctx->opcode)); > } > +#endif > } I agree that there is a bug there, and that cmp32 should be used with when L=0. That said your code is not going to generate and invalid code on a 32-bit CPU with L=1, but instead just skip the instruction. Moreover as Alexander pointed, TARGET_PPC64 doesn't mean it's a 64-bit CPU, but that the resulting qemu binaries support 64-bit CPU. What about the following patch (only lightly tested). From: Aurelien Jarno target-ppc: fix cmp instructions on 64-bit CPUs 64-bit CPUs check for the L bit of comparison instruction to determine if the instruction is 32-bit wide, and not to the MSR SF bit. L=1 on a 32-bit CPU should generate an invalid instruction exception. Reported-by: Torbjorn Granlund Signed-off-by: Aurelien Jarno --- target-ppc/translate.c | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/target-ppc/translate.c b/target-ppc/translate.c index 0886f4d..ab41dc1 100644 --- a/target-ppc/translate.c +++ b/target-ppc/translate.c @@ -675,48 +675,64 @@ static inline void gen_set_Rc0(DisasContext *ctx, TCGv reg) /* cmp */ static void gen_cmp(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (ctx->opcode & 0x00200000) { + if (unlikely(!(ctx->insns_flags & PPC_64B))) { + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); + } else { + gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], + 1, crfD(ctx->opcode)); + } + } else { gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], 1, crfD(ctx->opcode)); - } else { - gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], - 1, crfD(ctx->opcode)); } } /* cmpi */ static void gen_cmpi(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (ctx->opcode & 0x00200000) { + if (unlikely(!(ctx->insns_flags & PPC_64B))) { + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); + } else { + gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode), + 1, crfD(ctx->opcode)); + } + } else { gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode), 1, crfD(ctx->opcode)); - } else { - gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], SIMM(ctx->opcode), - 1, crfD(ctx->opcode)); } } /* cmpl */ static void gen_cmpl(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (ctx->opcode & 0x00200000) { + if (unlikely(!(ctx->insns_flags & PPC_64B))) { + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); + } else { + gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], + 0, crfD(ctx->opcode)); + } + } else { gen_op_cmp32(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], 0, crfD(ctx->opcode)); - } else { - gen_op_cmp(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], - 0, crfD(ctx->opcode)); } } /* cmpli */ static void gen_cmpli(DisasContext *ctx) { - if (NARROW_MODE(ctx) || !(ctx->opcode & 0x00200000)) { + if (ctx->opcode & 0x00200000) { + if (unlikely(!(ctx->insns_flags & PPC_64B))) { + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL); + } else { + gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode), + 0, crfD(ctx->opcode)); + } + } else { gen_op_cmpi32(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode), 0, crfD(ctx->opcode)); - } else { - gen_op_cmpi(cpu_gpr[rA(ctx->opcode)], UIMM(ctx->opcode), - 0, crfD(ctx->opcode)); } }