Re: [Qemu-arm] [PATCH] target-arm/translate-a64.c: Correct unallocated checks for ldst_excl

[Peter Maydell]

Ping? I forgot to mark this for-2.5, and given how long the bug's
been hanging around there's not much urgency to fixing it, but
we might as well put the fix into 2.5 if it gets reviewed.

thanks
-- PMM


On 16 November 2015 at 18:28, Peter Maydell <address@hidden> wrote:
> The checks for the unallocated encodings in the ldst_excl group
> (exclusives and load-acquire/store-release) were not correct. This
> error meant that in turn we ended up with code attempting to handle
> the non-existent case of "non-exclusive load-acquire/store-release
> pair". Delete that broken and now unreachable code.
>
> Reported-by: Laurent Desnogues <address@hidden>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> The easiest way to validate that we have the unallocated
> conditions correct now is to look at C4.4.6 "load/store exclusive"
> in the v8 ARM ARM rev A.h: our three conditions correspond
> to the three "unallocated" rows in the decode table.
>
> PS: Laurent originally reported this way back in 2014:
> http://lists.gnu.org/archive/html/qemu-devel/2014-10/msg01255.html
>
>  target-arm/translate-a64.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index fe485a4..890ace4 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1833,7 +1833,8 @@ static void disas_ldst_excl(DisasContext *s, uint32_t 
> insn)
>      int size = extract32(insn, 30, 2);
>      TCGv_i64 tcg_addr;
>
> -    if ((!is_excl && !is_lasr) ||
> +    if ((!is_excl && !is_pair && !is_lasr) ||
> +        (!is_excl && is_pair) ||
>          (is_pair && size < 2)) {
>          unallocated_encoding(s);
>          return;
> @@ -1862,15 +1863,6 @@ static void disas_ldst_excl(DisasContext *s, uint32_t 
> insn)
>          } else {
>              do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
>          }
> -        if (is_pair) {
> -            TCGv_i64 tcg_rt2 = cpu_reg(s, rt);
> -            tcg_gen_addi_i64(tcg_addr, tcg_addr, 1 << size);
> -            if (is_store) {
> -                do_gpr_st(s, tcg_rt2, tcg_addr, size);
> -            } else {
> -                do_gpr_ld(s, tcg_rt2, tcg_addr, size, false, false);
> -            }
> -        }
>      }
>  }
>
> --
> 1.9.1
>