[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 14/60] AArch64: Add orr instruction emulation
From: |
Claudio Fontana |
Subject: |
Re: [Qemu-devel] [PATCH 14/60] AArch64: Add orr instruction emulation |
Date: |
Mon, 18 Nov 2013 14:43:10 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120909 Thunderbird/15.0.1 |
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 <address@hidden>
> 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
>