Re: [PATCH v2 24/24] target/arm: Enforce alignment for sve unpredicated LDR/STR

[Peter Maydell]

On Tue, 8 Dec 2020 at 18:02, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate-sve.c | 58 +++++++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c
> index 6125e734af..b481e97428 100644
> --- a/target/arm/translate-sve.c
> +++ b/target/arm/translate-sve.c
> @@ -4263,7 +4263,8 @@ static bool trans_UCVTF_dd(DisasContext *s, arg_rpr_esz 
> *a)
>   * The load should begin at the address Rn + IMM.
>   */
>
> -static void do_ldr(DisasContext *s, uint32_t vofs, int len, int rn, int imm)
> +static void do_ldr(DisasContext *s, uint32_t vofs, int len,
> +                   MemOp align, int rn, int imm)
>  {
>      int len_align = QEMU_ALIGN_DOWN(len, 8);
>      int len_remain = len % 8;
> @@ -4276,6 +4277,10 @@ static void do_ldr(DisasContext *s, uint32_t vofs, int 
> len, int rn, int imm)
>      clean_addr = gen_mte_checkN(s, dirty_addr, false, rn != 31, len, MO_8);
>      tcg_temp_free_i64(dirty_addr);
>
> +    if (!s->align_mem) {
> +        align = 0;
> +    }
> +
>      /*
>       * Note that unpredicated load/store of vector/predicate registers
>       * are defined as a stream of bytes, which equates to little-endian
> @@ -4288,7 +4293,8 @@ static void do_ldr(DisasContext *s, uint32_t vofs, int 
> len, int rn, int imm)
>
>          t0 = tcg_temp_new_i64();
>          for (i = 0; i < len_align; i += 8) {
> -            tcg_gen_qemu_ld_i64(t0, clean_addr, midx, MO_LEQ);
> +            tcg_gen_qemu_ld_i64(t0, clean_addr, midx, MO_LEQ | align);
> +            align = 0;
>              tcg_gen_st_i64(t0, cpu_env, vofs + i);
>              tcg_gen_addi_i64(clean_addr, clean_addr, 8);
>          }
> @@ -4302,6 +4308,16 @@ static void do_ldr(DisasContext *s, uint32_t vofs, int 
> len, int rn, int imm)
>          clean_addr = new_tmp_a64_local(s);
>          tcg_gen_mov_i64(clean_addr, t0);
>
> +        if (align > MO_ALIGN_8) {
> +            t0 = tcg_temp_new_i64();
> +            tcg_gen_qemu_ld_i64(t0, clean_addr, midx, MO_LEQ | align);
> +            tcg_gen_addi_i64(clean_addr, clean_addr, 8);
> +            tcg_gen_addi_ptr(i, i, 8);
> +            tcg_gen_st_i64(t0, cpu_env, vofs);
> +            tcg_temp_free_i64(t0);
> +            align = 0;
> +        }
> +

Why do we need to do this (and the similar thing in do_str()) ?
Most of the rest of the patch is fairly clear in that it is just
passing the alignment requirement through to the load/store fns,
but this is a bit more opaque to me...

thanks
-- PMM