From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42670) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ViP5m-0006wf-Ur for qemu-devel@nongnu.org; Mon, 18 Nov 2013 08:42:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ViP5g-0005uP-U9 for qemu-devel@nongnu.org; Mon, 18 Nov 2013 08:42:18 -0500 Received: from mail-ee0-f50.google.com ([74.125.83.50]:49624) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ViP5g-0005uD-Kq for qemu-devel@nongnu.org; Mon, 18 Nov 2013 08:42:12 -0500 Received: by mail-ee0-f50.google.com with SMTP id e53so671777eek.37 for ; Mon, 18 Nov 2013 05:42:11 -0800 (PST) Message-ID: <528A196E.9000107@linaro.org> Date: Mon, 18 Nov 2013 14:43:10 +0100 From: Claudio Fontana MIME-Version: 1.0 References: <1380242934-20953-1-git-send-email-agraf@suse.de> <1380242934-20953-15-git-send-email-agraf@suse.de> <5245CDB5.2000403@twiddle.net> <5289E8AD.2070902@linaro.org> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 14/60] AArch64: Add orr instruction emulation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Matz Cc: Peter Maydell , qemu-devel@nongnu.org, Alexander Graf , Dirk Mueller , Laurent Desnogues , Christoffer Dall , Richard Henderson Btw, in the first patch: On 11/18/2013 02:12 PM, Michael Matz wrote: > > From df54486da31d6329696effa61096eda5ab85395a Mon Sep 17 00:00:00 2001 > From: Michael Matz > Date: Sun, 24 Mar 2013 02:52:42 +0100 > Subject: [PATCH] Fix 32bit rotates. > > The 32bit shifts generally weren't careful with the upper parts, > either bits could leak in (for right shift) or leak or (for left shift). > And rotate was completely off, rotating around bit 63, not 31. > This fixes the CAST5 hash algorithm. > --- > target-arm/translate-a64.c | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index 96dc281..e3941a1 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -596,25 +596,49 @@ static TCGv_i64 get_shift(int reg, int shift_type, TCGv_i64 tcg_shift, > r = tcg_temp_new_i64(); > > /* XXX carry_out */ > + /* Careful with the width. We work on 64bit, but must make sure > + that we zero-extend the result on out, and ignore any upper bits, > + that might still be set in that register. */ > switch (shift_type) { > case 0: /* LSL */ > + /* left shift is easy, simply zero-extend on out */ > tcg_gen_shl_i64(r, cpu_reg(reg), tcg_shift); > + if (is_32bit) > + tcg_gen_ext32u_i64 (r, r); > break; > case 1: /* LSR */ > - tcg_gen_shr_i64(r, cpu_reg(reg), tcg_shift); > + /* For logical right shift we zero extend first, to zero > + the upper bits. We don't need to extend on out. */ > + if (is_32bit) { > + tcg_gen_ext32u_i64 (r, cpu_reg(reg)); > + tcg_gen_shr_i64 (r, r, tcg_shift); > + } else > + tcg_gen_shr_i64(r, cpu_reg(reg), tcg_shift); > break; > case 2: /* ASR */ > + /* For arithmetic right shift we sign extend first, then shift, > + and then need to clear the upper bits again. */ > if (is_32bit) { > TCGv_i64 tcg_tmp = tcg_temp_new_i64(); > tcg_gen_ext32s_i64(tcg_tmp, cpu_reg(reg)); > tcg_gen_sar_i64(r, tcg_tmp, tcg_shift); > + tcg_gen_ext32u_i64 (r, r); > tcg_temp_free_i64(tcg_tmp); > } else { > tcg_gen_sar_i64(r, cpu_reg(reg), tcg_shift); > } > break; > - case 3: > - tcg_gen_rotr_i64(r, cpu_reg(reg), tcg_shift); > + case 3: /* ROR */ > + /* For rotation extending doesn't help, we really have to use > + a 32bit rotate. */ > + if (is_32bit) { > + TCGv_i32 tmp = tcg_temp_new_i32(); > + tcg_gen_trunc_i64_i32(tmp, cpu_reg(reg)); > + tcg_gen_rotr_i32(tmp, tmp, tcg_shift); Isn't this problematic? We are using gen_rotr_i32, but passing tcg_shift, which is a TCGv_i64. I remember I had compilation failures in the past when I tried something similar, so my understanding is that this can work with a certain compiler under certain compiler options, but is not guaranteed to work in all cases. I think we need to either explicitly convert the tcg_shift to a TCGv_i32, or we need to use an open coded version of the rotr_i64 that inserts at (32 - n) instead of (64 - n) What do you think? C. > + tcg_gen_extu_i32_i64(r, tmp); > + tcg_temp_free_i32(tmp); > + } else > + tcg_gen_rotr_i64(r, cpu_reg(reg), tcg_shift); > break; > } > > -- 1.8.1.4 >