[PATCH for-6.0 00/11] target/arm: enforce alignment

[PATCH for-6.0 00/11] target/arm: enforce alignment

[Richard Henderson]

As reported in https://bugs.launchpad.net/bugs/1905356

Not implementing SCTLR.A, but all of the other required
alignment for SCTLR.A=0 in Table A3-1.


r~

Richard Henderson (11):
  target/arm: Enforce word alignment for LDRD/STRD
  target/arm: Enforce alignment for LDA/LDAH/STL/STLH
  target/arm: Enforce alignment for LDM/STM
  target/arm: Enforce alignment for RFE
  target/arm: Enforce alignment for SRS
  target/arm: Enforce alignment for VLDM/VSTM
  target/arm: Enforce alignment for VLDR/VSTR
  target/arm: Enforce alignment for VLD1 (all lanes)
  target/arm: Enforce alignment for VLDn/VSTn (multiple)
  target/arm: Fix decode of align in VLDST_single
  target/arm: Enforce alignment for VLDn/VSTn (single)

 target/arm/neon-ls.decode       |  4 +--
 target/arm/translate.c          | 32 +++++++++--------
 target/arm/translate-neon.c.inc | 63 +++++++++++++++++++++++++--------
 target/arm/translate-vfp.c.inc  | 30 ++++++++++------
 4 files changed, 88 insertions(+), 41 deletions(-)

-- 
2.25.1

[PATCH 01/11] target/arm: Enforce word alignment for LDRD/STRD

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 6d04ca3a8a..17883d00f4 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -6458,7 +6458,7 @@ static bool trans_LDRD_rr(DisasContext *s, arg_ldst_rr *a)
     addr = op_addr_rr_pre(s, a);
 
     tmp = tcg_temp_new_i32();
-    gen_aa32_ld_i32(s, tmp, addr, mem_idx, MO_UL | s->be_data);
+    gen_aa32_ld_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN_4 | s->be_data);
     store_reg(s, a->rt, tmp);
 
     tcg_gen_addi_i32(addr, addr, 4);
@@ -6487,7 +6487,7 @@ static bool trans_STRD_rr(DisasContext *s, arg_ldst_rr *a)
     addr = op_addr_rr_pre(s, a);
 
     tmp = load_reg(s, a->rt);
-    gen_aa32_st_i32(s, tmp, addr, mem_idx, MO_UL | s->be_data);
+    gen_aa32_st_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN_4 | s->be_data);
     tcg_temp_free_i32(tmp);
 
     tcg_gen_addi_i32(addr, addr, 4);
@@ -6595,7 +6595,7 @@ static bool op_ldrd_ri(DisasContext *s, arg_ldst_ri *a, int rt2)
     addr = op_addr_ri_pre(s, a);
 
     tmp = tcg_temp_new_i32();
-    gen_aa32_ld_i32(s, tmp, addr, mem_idx, MO_UL | s->be_data);
+    gen_aa32_ld_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN_4 | s->be_data);
     store_reg(s, a->rt, tmp);
 
     tcg_gen_addi_i32(addr, addr, 4);
@@ -6634,7 +6634,7 @@ static bool op_strd_ri(DisasContext *s, arg_ldst_ri *a, int rt2)
     addr = op_addr_ri_pre(s, a);
 
     tmp = load_reg(s, a->rt);
-    gen_aa32_st_i32(s, tmp, addr, mem_idx, MO_UL | s->be_data);
+    gen_aa32_st_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN_4 | s->be_data);
     tcg_temp_free_i32(tmp);
 
     tcg_gen_addi_i32(addr, addr, 4);
-- 
2.25.1

Re: [PATCH 01/11] target/arm: Enforce word alignment for LDRD/STRD

[Philippe Mathieu-Daudé]

On 11/25/20 5:06 AM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 02/11] target/arm: Enforce alignment for LDA/LDAH/STL/STLH

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 17883d00f4..73b3d8cbbf 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -6876,7 +6876,7 @@ static bool op_stl(DisasContext *s, arg_STL *a, MemOp mop)
 
 static bool trans_STL(DisasContext *s, arg_STL *a)
 {
-    return op_stl(s, a, MO_UL);
+    return op_stl(s, a, MO_UL | MO_ALIGN);
 }
 
 static bool trans_STLB(DisasContext *s, arg_STL *a)
@@ -6886,7 +6886,7 @@ static bool trans_STLB(DisasContext *s, arg_STL *a)
 
 static bool trans_STLH(DisasContext *s, arg_STL *a)
 {
-    return op_stl(s, a, MO_UW);
+    return op_stl(s, a, MO_UW | MO_ALIGN);
 }
 
 static bool op_ldrex(DisasContext *s, arg_LDREX *a, MemOp mop, bool acq)
@@ -7033,7 +7033,7 @@ static bool op_lda(DisasContext *s, arg_LDA *a, MemOp mop)
 
 static bool trans_LDA(DisasContext *s, arg_LDA *a)
 {
-    return op_lda(s, a, MO_UL);
+    return op_lda(s, a, MO_UL | MO_ALIGN);
 }
 
 static bool trans_LDAB(DisasContext *s, arg_LDA *a)
@@ -7043,7 +7043,7 @@ static bool trans_LDAB(DisasContext *s, arg_LDA *a)
 
 static bool trans_LDAH(DisasContext *s, arg_LDA *a)
 {
-    return op_lda(s, a, MO_UW);
+    return op_lda(s, a, MO_UW | MO_ALIGN);
 }
 
 /*
-- 
2.25.1

Re: [PATCH 02/11] target/arm: Enforce alignment for LDA/LDAH/STL/STLH

[Philippe Mathieu-Daudé]

On 11/25/20 5:06 AM, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 03/11] target/arm: Enforce alignment for LDM/STM

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 73b3d8cbbf..fe4400fa6c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -7814,7 +7814,7 @@ static bool op_stm(DisasContext *s, arg_ldst_block *a, int min_n)
         } else {
             tmp = load_reg(s, i);
         }
-        gen_aa32_st32(s, tmp, addr, mem_idx);
+        gen_aa32_st_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN | s->be_data);
         tcg_temp_free_i32(tmp);
 
         /* No need to add after the last transfer.  */
@@ -7889,7 +7889,7 @@ static bool do_ldm(DisasContext *s, arg_ldst_block *a, int min_n)
         }
 
         tmp = tcg_temp_new_i32();
-        gen_aa32_ld32u(s, tmp, addr, mem_idx);
+        gen_aa32_ld_i32(s, tmp, addr, mem_idx, MO_UL | MO_ALIGN | s->be_data);
         if (user) {
             tmp2 = tcg_const_i32(i);
             gen_helper_set_user_reg(cpu_env, tmp2, tmp);
-- 
2.25.1

[PATCH 04/11] target/arm: Enforce alignment for RFE

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index fe4400fa6c..4406f6a67c 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8250,10 +8250,12 @@ static bool trans_RFE(DisasContext *s, arg_RFE *a)
 
     /* Load PC into tmp and CPSR into tmp2.  */
     t1 = tcg_temp_new_i32();
-    gen_aa32_ld32u(s, t1, addr, get_mem_index(s));
+    gen_aa32_ld_i32(s, t1, addr, get_mem_index(s),
+                    MO_UL | MO_ALIGN | s->be_data);
     tcg_gen_addi_i32(addr, addr, 4);
     t2 = tcg_temp_new_i32();
-    gen_aa32_ld32u(s, t2, addr, get_mem_index(s));
+    gen_aa32_ld_i32(s, t2, addr, get_mem_index(s),
+                    MO_UL | MO_ALIGN | s->be_data);
 
     if (a->w) {
         /* Base writeback.  */
-- 
2.25.1

[PATCH 05/11] target/arm: Enforce alignment for SRS

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 4406f6a67c..b1f43bfb8f 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -5124,11 +5124,13 @@ static void gen_srs(DisasContext *s,
     }
     tcg_gen_addi_i32(addr, addr, offset);
     tmp = load_reg(s, 14);
-    gen_aa32_st32(s, tmp, addr, get_mem_index(s));
+    gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
+                    MO_UL | MO_ALIGN | s->be_data);
     tcg_temp_free_i32(tmp);
     tmp = load_cpu_field(spsr);
     tcg_gen_addi_i32(addr, addr, 4);
-    gen_aa32_st32(s, tmp, addr, get_mem_index(s));
+    gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
+                    MO_UL | MO_ALIGN | s->be_data);
     tcg_temp_free_i32(tmp);
     if (writeback) {
         switch (amode) {
-- 
2.25.1

Re: [PATCH 05/11] target/arm: Enforce alignment for SRS

[Peter Maydell]

On Wed, 25 Nov 2020 at 04:09, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 4406f6a67c..b1f43bfb8f 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -5124,11 +5124,13 @@ static void gen_srs(DisasContext *s,
>      }
>      tcg_gen_addi_i32(addr, addr, offset);
>      tmp = load_reg(s, 14);
> -    gen_aa32_st32(s, tmp, addr, get_mem_index(s));
> +    gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
> +                    MO_UL | MO_ALIGN | s->be_data);
>      tcg_temp_free_i32(tmp);
>      tmp = load_cpu_field(spsr);
>      tcg_gen_addi_i32(addr, addr, 4);
> -    gen_aa32_st32(s, tmp, addr, get_mem_index(s));
> +    gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
> +                    MO_UL | MO_ALIGN | s->be_data);

Having just come back to look at this as a result of reading
a review comment from you on the v8.1M series, it's a bit
unfortunate that we now have to remember to factor in s->be_data
in every memory access. Previously gen_aa32_st32() got this
right for us automatically, as well as being able to provide
the right sized MO_UL or whatever part... Can we make the
new API a bit less awkward ? (I suspect we're eventually
going to want to be able to pass an enum for "always OK
unaligned", "never OK unaligned", or "OK unaligned only
if SCTLR.A is 0", for that matter.)

thanks
-- PMM

[PATCH 06/11] target/arm: Enforce alignment for VLDM/VSTM

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-vfp.c.inc | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/arm/translate-vfp.c.inc b/target/arm/translate-vfp.c.inc
index 96948f5a2d..58b31ecc3f 100644
--- a/target/arm/translate-vfp.c.inc
+++ b/target/arm/translate-vfp.c.inc
@@ -1065,12 +1065,14 @@ static bool trans_VLDM_VSTM_sp(DisasContext *s, arg_VLDM_VSTM_sp *a)
     for (i = 0; i < n; i++) {
         if (a->l) {
             /* load */
-            gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
+            gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s),
+                            MO_UL | MO_ALIGN | s->be_data);
             vfp_store_reg32(tmp, a->vd + i);
         } else {
             /* store */
             vfp_load_reg32(tmp, a->vd + i);
-            gen_aa32_st32(s, tmp, addr, get_mem_index(s));
+            gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
+                            MO_UL | MO_ALIGN | s->be_data);
         }
         tcg_gen_addi_i32(addr, addr, offset);
     }
@@ -1148,12 +1150,14 @@ static bool trans_VLDM_VSTM_dp(DisasContext *s, arg_VLDM_VSTM_dp *a)
     for (i = 0; i < n; i++) {
         if (a->l) {
             /* load */
-            gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
+            gen_aa32_ld_i64(s, tmp, addr, get_mem_index(s),
+                            MO_Q | MO_ALIGN_4 | s->be_data);
             vfp_store_reg64(tmp, a->vd + i);
         } else {
             /* store */
             vfp_load_reg64(tmp, a->vd + i);
-            gen_aa32_st64(s, tmp, addr, get_mem_index(s));
+            gen_aa32_st_i64(s, tmp, addr, get_mem_index(s),
+                            MO_Q | MO_ALIGN_4 | s->be_data);
         }
         tcg_gen_addi_i32(addr, addr, offset);
     }
-- 
2.25.1

[PATCH 07/11] target/arm: Enforce alignment for VLDR/VSTR

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-vfp.c.inc | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/target/arm/translate-vfp.c.inc b/target/arm/translate-vfp.c.inc
index 58b31ecc3f..51e85c2767 100644
--- a/target/arm/translate-vfp.c.inc
+++ b/target/arm/translate-vfp.c.inc
@@ -926,11 +926,13 @@ static bool trans_VLDR_VSTR_hp(DisasContext *s, arg_VLDR_VSTR_sp *a)
     addr = add_reg_for_lit(s, a->rn, offset);
     tmp = tcg_temp_new_i32();
     if (a->l) {
-        gen_aa32_ld16u(s, tmp, addr, get_mem_index(s));
+        gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s),
+                        MO_UW | MO_ALIGN | s->be_data);
         vfp_store_reg32(tmp, a->vd);
     } else {
         vfp_load_reg32(tmp, a->vd);
-        gen_aa32_st16(s, tmp, addr, get_mem_index(s));
+        gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
+                        MO_UW | MO_ALIGN | s->be_data);
     }
     tcg_temp_free_i32(tmp);
     tcg_temp_free_i32(addr);
@@ -960,11 +962,13 @@ static bool trans_VLDR_VSTR_sp(DisasContext *s, arg_VLDR_VSTR_sp *a)
     addr = add_reg_for_lit(s, a->rn, offset);
     tmp = tcg_temp_new_i32();
     if (a->l) {
-        gen_aa32_ld32u(s, tmp, addr, get_mem_index(s));
+        gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s),
+                        MO_UL | MO_ALIGN | s->be_data);
         vfp_store_reg32(tmp, a->vd);
     } else {
         vfp_load_reg32(tmp, a->vd);
-        gen_aa32_st32(s, tmp, addr, get_mem_index(s));
+        gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
+                        MO_UL | MO_ALIGN | s->be_data);
     }
     tcg_temp_free_i32(tmp);
     tcg_temp_free_i32(addr);
@@ -1001,11 +1005,13 @@ static bool trans_VLDR_VSTR_dp(DisasContext *s, arg_VLDR_VSTR_dp *a)
     addr = add_reg_for_lit(s, a->rn, offset);
     tmp = tcg_temp_new_i64();
     if (a->l) {
-        gen_aa32_ld64(s, tmp, addr, get_mem_index(s));
+        gen_aa32_ld_i64(s, tmp, addr, get_mem_index(s),
+                        MO_Q | MO_ALIGN_4 | s->be_data);
         vfp_store_reg64(tmp, a->vd);
     } else {
         vfp_load_reg64(tmp, a->vd);
-        gen_aa32_st64(s, tmp, addr, get_mem_index(s));
+        gen_aa32_st_i64(s, tmp, addr, get_mem_index(s),
+                        MO_Q | MO_ALIGN_4 | s->be_data);
     }
     tcg_temp_free_i64(tmp);
     tcg_temp_free_i32(addr);
-- 
2.25.1

[PATCH 08/11] target/arm: Enforce alignment for VLD1 (all lanes)

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-neon.c.inc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate-neon.c.inc b/target/arm/translate-neon.c.inc
index f6c68e30ab..32e47331a5 100644
--- a/target/arm/translate-neon.c.inc
+++ b/target/arm/translate-neon.c.inc
@@ -518,6 +518,7 @@ static bool trans_VLD_all_lanes(DisasContext *s, arg_VLD_all_lanes *a)
     int reg, stride, vec_size;
     int vd = a->vd;
     int size = a->size;
+    MemOp mop = size | s->be_data | (a->a ? MO_ALIGN : 0);
     int nregs = a->n + 1;
     TCGv_i32 addr, tmp;
 
@@ -559,8 +560,7 @@ static bool trans_VLD_all_lanes(DisasContext *s, arg_VLD_all_lanes *a)
     addr = tcg_temp_new_i32();
     load_reg_var(s, addr, a->rn);
     for (reg = 0; reg < nregs; reg++) {
-        gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s),
-                        s->be_data | size);
+        gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s), mop);
         if ((vd & 1) && vec_size == 16) {
             /*
              * We cannot write 16 bytes at once because the
-- 
2.25.1

[PATCH 09/11] target/arm: Enforce alignment for VLDn/VSTn (multiple)

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-neon.c.inc | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/target/arm/translate-neon.c.inc b/target/arm/translate-neon.c.inc
index 32e47331a5..c4be019d9c 100644
--- a/target/arm/translate-neon.c.inc
+++ b/target/arm/translate-neon.c.inc
@@ -427,9 +427,12 @@ static void gen_neon_ldst_base_update(DisasContext *s, int rm, int rn,
 
 static bool trans_VLDST_multiple(DisasContext *s, arg_VLDST_multiple *a)
 {
+    static const MemOp mop_align[4] = {
+        0, MO_ALIGN_8, MO_ALIGN_16, MO_ALIGN_32
+    };
     /* Neon load/store multiple structures */
     int nregs, interleave, spacing, reg, n;
-    MemOp endian = s->be_data;
+    MemOp mop, endian = s->be_data;
     int mmu_idx = get_mem_index(s);
     int size = a->size;
     TCGv_i64 tmp64;
@@ -487,6 +490,10 @@ static bool trans_VLDST_multiple(DisasContext *s, arg_VLDST_multiple *a)
     addr = tcg_temp_new_i32();
     tmp = tcg_const_i32(1 << size);
     load_reg_var(s, addr, a->rn);
+
+    /* Enforce the requested alignment for the first memory operation. */
+    mop = endian | size | mop_align[a->align];
+
     for (reg = 0; reg < nregs; reg++) {
         for (n = 0; n < 8 >> size; n++) {
             int xs;
@@ -494,13 +501,16 @@ static bool trans_VLDST_multiple(DisasContext *s, arg_VLDST_multiple *a)
                 int tt = a->vd + reg + spacing * xs;
 
                 if (a->l) {
-                    gen_aa32_ld_i64(s, tmp64, addr, mmu_idx, endian | size);
+                    gen_aa32_ld_i64(s, tmp64, addr, mmu_idx, mop);
                     neon_store_element64(tt, n, size, tmp64);
                 } else {
                     neon_load_element64(tmp64, tt, n, size);
-                    gen_aa32_st_i64(s, tmp64, addr, mmu_idx, endian | size);
+                    gen_aa32_st_i64(s, tmp64, addr, mmu_idx, mop);
                 }
                 tcg_gen_add_i32(addr, addr, tmp);
+
+                /* Subsequent memory operations inherit alignment */
+                mop &= ~MO_AMASK;
             }
         }
     }
-- 
2.25.1

[PATCH 10/11] target/arm: Fix decode of align in VLDST_single

[Richard Henderson]

The encoding of size = 2 and size = 3 had the incorrect decode
for align, overlapping the stride field.  This error was hidden
by what should have been unnecessary masking in translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/neon-ls.decode       | 4 ++--
 target/arm/translate-neon.c.inc | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/arm/neon-ls.decode b/target/arm/neon-ls.decode
index c17f5019e3..0a2a0e15db 100644
--- a/target/arm/neon-ls.decode
+++ b/target/arm/neon-ls.decode
@@ -46,7 +46,7 @@ VLD_all_lanes  1111 0100 1 . 1 0 rn:4 .... 11 n:2 size:2 t:1 a:1 rm:4 \
 
 VLDST_single   1111 0100 1 . l:1 0 rn:4 .... 00 n:2 reg_idx:3 align:1 rm:4 \
                vd=%vd_dp size=0 stride=1
-VLDST_single   1111 0100 1 . l:1 0 rn:4 .... 01 n:2 reg_idx:2 align:2 rm:4 \
+VLDST_single   1111 0100 1 . l:1 0 rn:4 .... 01 n:2 reg_idx:2 . align:1 rm:4 \
                vd=%vd_dp size=1 stride=%imm1_5_p1
-VLDST_single   1111 0100 1 . l:1 0 rn:4 .... 10 n:2 reg_idx:1 align:3 rm:4 \
+VLDST_single   1111 0100 1 . l:1 0 rn:4 .... 10 n:2 reg_idx:1 . align:2 rm:4 \
                vd=%vd_dp size=2 stride=%imm1_6_p1
diff --git a/target/arm/translate-neon.c.inc b/target/arm/translate-neon.c.inc
index c4be019d9c..330b5fc7b0 100644
--- a/target/arm/translate-neon.c.inc
+++ b/target/arm/translate-neon.c.inc
@@ -616,7 +616,7 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
     switch (nregs) {
     case 1:
         if (((a->align & (1 << a->size)) != 0) ||
-            (a->size == 2 && ((a->align & 3) == 1 || (a->align & 3) == 2))) {
+            (a->size == 2 && (a->align == 1 || a->align == 2))) {
             return false;
         }
         break;
@@ -631,7 +631,7 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
         }
         break;
     case 4:
-        if ((a->size == 2) && ((a->align & 3) == 3)) {
+        if (a->size == 2 && a->align == 3) {
             return false;
         }
         break;
-- 
2.25.1

Re: [PATCH 10/11] target/arm: Fix decode of align in VLDST_single

[Peter Maydell]

On Wed, 25 Nov 2020 at 04:12, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The encoding of size = 2 and size = 3 had the incorrect decode
> for align, overlapping the stride field.  This error was hidden
> by what should have been unnecessary masking in translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/neon-ls.decode       | 4 ++--
>  target/arm/translate-neon.c.inc | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH 11/11] target/arm: Enforce alignment for VLDn/VSTn (single)

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-neon.c.inc | 39 ++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/target/arm/translate-neon.c.inc b/target/arm/translate-neon.c.inc
index 330b5fc7b0..160dc3d755 100644
--- a/target/arm/translate-neon.c.inc
+++ b/target/arm/translate-neon.c.inc
@@ -602,6 +602,7 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
     int nregs = a->n + 1;
     int vd = a->vd;
     TCGv_i32 addr, tmp;
+    MemOp mop;
 
     if (!arm_dc_feature(s, ARM_FEATURE_NEON)) {
         return false;
@@ -651,25 +652,47 @@ static bool trans_VLDST_single(DisasContext *s, arg_VLDST_single *a)
         return true;
     }
 
+    mop = s->be_data | a->size;
+    if (a->align) {
+        static const MemOp mop_align[] = {
+            MO_ALIGN_2, MO_ALIGN_4, MO_ALIGN_8, MO_ALIGN_16
+        };
+
+        switch (nregs) {
+        case 1:
+            mop |= MO_ALIGN;
+            break;
+        case 2:
+            mop |= mop_align[a->size];
+            break;
+        case 3:
+            /* the align field is repurposed for VLD3 */
+            break;
+        case 4:
+            mop |= mop_align[a->size + a->align];
+            break;
+        default:
+            g_assert_not_reached();
+        }
+    }
+
     tmp = tcg_temp_new_i32();
     addr = tcg_temp_new_i32();
     load_reg_var(s, addr, a->rn);
-    /*
-     * TODO: if we implemented alignment exceptions, we should check
-     * addr against the alignment encoded in a->align here.
-     */
+
     for (reg = 0; reg < nregs; reg++) {
         if (a->l) {
-            gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s),
-                            s->be_data | a->size);
+            gen_aa32_ld_i32(s, tmp, addr, get_mem_index(s), mop);
             neon_store_element(vd, a->reg_idx, a->size, tmp);
         } else { /* Store */
             neon_load_element(tmp, vd, a->reg_idx, a->size);
-            gen_aa32_st_i32(s, tmp, addr, get_mem_index(s),
-                            s->be_data | a->size);
+            gen_aa32_st_i32(s, tmp, addr, get_mem_index(s), mop);
         }
         vd += a->stride;
         tcg_gen_addi_i32(addr, addr, 1 << a->size);
+
+        /* Subsequent memory operations inherit alignment */
+        mop &= ~MO_AMASK;
     }
     tcg_temp_free_i32(addr);
     tcg_temp_free_i32(tmp);
-- 
2.25.1

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Peter Maydell]

On Wed, 25 Nov 2020 at 04:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
> As reported in https://bugs.launchpad.net/bugs/1905356
>
> Not implementing SCTLR.A, but all of the other required
> alignment for SCTLR.A=0 in Table A3-1.

Any particular reason not to also support SCTLR.A ?

Also not implementing the old v5 unaligned access behaviour,
but we never have done that...

Whole series:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Richard Henderson]

On 12/1/20 8:27 AM, Peter Maydell wrote:
> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> As reported in https://bugs.launchpad.net/bugs/1905356
>>
>> Not implementing SCTLR.A, but all of the other required
>> alignment for SCTLR.A=0 in Table A3-1.
> 
> Any particular reason not to also support SCTLR.A ?

No, it's just a bigger job.  I'll put it on the to-do list.


r~

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Peter Maydell]

On Wed, 25 Nov 2020 at 04:06, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> As reported in https://bugs.launchpad.net/bugs/1905356
>
> Not implementing SCTLR.A, but all of the other required
> alignment for SCTLR.A=0 in Table A3-1.

Something in this series breaks the 'make check-acceptance'
record-and-replay test:

 (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
PASS (9.14 s)
 (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
Timeout reached\nOriginal status: ERROR\n{'name':
'31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
(90.19 s)

The log shows the "recording execution" apparently hanging,
with the last output from the guest
[    3.183662] Registering SWP/SWPB emulation handler

thanks
-- PMM

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Philippe Mathieu-Daudé]

Cc'ing Pavel

On 12/1/20 4:55 PM, Peter Maydell wrote:
> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> As reported in https://bugs.launchpad.net/bugs/1905356
>>
>> Not implementing SCTLR.A, but all of the other required
>> alignment for SCTLR.A=0 in Table A3-1.
> 
> Something in this series breaks the 'make check-acceptance'
> record-and-replay test:
> 
>  (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
> PASS (9.14 s)
>  (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
> Timeout reached\nOriginal status: ERROR\n{'name':
> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
> 'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
> (90.19 s)
> 
> The log shows the "recording execution" apparently hanging,
> with the last output from the guest
> [    3.183662] Registering SWP/SWPB emulation handler
> 
> thanks
> -- PMM
> 

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Pavel Dovgalyuk]

On 03.12.2020 15:30, Philippe Mathieu-Daudé wrote:
> Cc'ing Pavel
> 
> On 12/1/20 4:55 PM, Peter Maydell wrote:
>> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
>> <richard.henderson@linaro.org> wrote:
>>>
>>> As reported in https://bugs.launchpad.net/bugs/1905356
>>>
>>> Not implementing SCTLR.A, but all of the other required
>>> alignment for SCTLR.A=0 in Table A3-1.
>>
>> Something in this series breaks the 'make check-acceptance'
>> record-and-replay test:
>>
>>   (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
>> PASS (9.14 s)
>>   (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
>> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
>> Timeout reached\nOriginal status: ERROR\n{'name':
>> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
>> 'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
>> (90.19 s)
>>
>> The log shows the "recording execution" apparently hanging,
>> with the last output from the guest
>> [    3.183662] Registering SWP/SWPB emulation handler

I looked through the patches and it does not seem that they can break 
anything.
Could it be the same avocado/chardev socket glitch as in some previous 
failures?
What happens when re-running this test?

Pavel Dovgalyuk

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Peter Maydell]

On Thu, 3 Dec 2020 at 16:10, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>
> On 03.12.2020 15:30, Philippe Mathieu-Daudé wrote:
> > Cc'ing Pavel
> >
> > On 12/1/20 4:55 PM, Peter Maydell wrote:
> >> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
> >> <richard.henderson@linaro.org> wrote:
> >>>
> >>> As reported in https://bugs.launchpad.net/bugs/1905356
> >>>
> >>> Not implementing SCTLR.A, but all of the other required
> >>> alignment for SCTLR.A=0 in Table A3-1.
> >>
> >> Something in this series breaks the 'make check-acceptance'
> >> record-and-replay test:
> >>
> >>   (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
> >> PASS (9.14 s)
> >>   (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
> >> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
> >> Timeout reached\nOriginal status: ERROR\n{'name':
> >> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
> >> 'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
> >> (90.19 s)
> >>
> >> The log shows the "recording execution" apparently hanging,
> >> with the last output from the guest
> >> [    3.183662] Registering SWP/SWPB emulation handler
>
> I looked through the patches and it does not seem that they can break
> anything.
> Could it be the same avocado/chardev socket glitch as in some previous
> failures?
> What happens when re-running this test?

I ran it a couple of times with the patchset and it failed the same
way each time. Without is fine.

thanks
-- PMM

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Pavel Dovgalyuk]

On 03.12.2020 19:14, Peter Maydell wrote:
> On Thu, 3 Dec 2020 at 16:10, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>>
>> On 03.12.2020 15:30, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Pavel
>>>
>>> On 12/1/20 4:55 PM, Peter Maydell wrote:
>>>> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
>>>> <richard.henderson@linaro.org> wrote:
>>>>>
>>>>> As reported in https://bugs.launchpad.net/bugs/1905356
>>>>>
>>>>> Not implementing SCTLR.A, but all of the other required
>>>>> alignment for SCTLR.A=0 in Table A3-1.
>>>>
>>>> Something in this series breaks the 'make check-acceptance'
>>>> record-and-replay test:
>>>>
>>>>    (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
>>>> PASS (9.14 s)
>>>>    (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
>>>> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
>>>> Timeout reached\nOriginal status: ERROR\n{'name':
>>>> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
>>>> 'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
>>>> (90.19 s)
>>>>
>>>> The log shows the "recording execution" apparently hanging,
>>>> with the last output from the guest
>>>> [    3.183662] Registering SWP/SWPB emulation handler
>>
>> I looked through the patches and it does not seem that they can break
>> anything.
>> Could it be the same avocado/chardev socket glitch as in some previous
>> failures?
>> What happens when re-running this test?
> 
> I ran it a couple of times with the patchset and it failed the same
> way each time. Without is fine.

I applied the patches and got no failures on my local machine.

Do you have any ideas on debugging this bug?
What does "arm-clang" means? Is the host compiler is clang?

Pavel Dovgalyuk

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Peter Maydell]

On Fri, 4 Dec 2020 at 06:17, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>
> On 03.12.2020 19:14, Peter Maydell wrote:
> > On Thu, 3 Dec 2020 at 16:10, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
> >>
> >> On 03.12.2020 15:30, Philippe Mathieu-Daudé wrote:
> >>> Cc'ing Pavel
> >>>
> >>> On 12/1/20 4:55 PM, Peter Maydell wrote:
> >>>> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
> >>>> <richard.henderson@linaro.org> wrote:
> >>>>>
> >>>>> As reported in https://bugs.launchpad.net/bugs/1905356
> >>>>>
> >>>>> Not implementing SCTLR.A, but all of the other required
> >>>>> alignment for SCTLR.A=0 in Table A3-1.
> >>>>
> >>>> Something in this series breaks the 'make check-acceptance'
> >>>> record-and-replay test:
> >>>>
> >>>>    (30/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
> >>>> PASS (9.14 s)
> >>>>    (31/40) tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
> >>>> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
> >>>> Timeout reached\nOriginal status: ERROR\n{'name':
> >>>> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
> >>>> 'logdir': '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
> >>>> (90.19 s)
> >>>>
> >>>> The log shows the "recording execution" apparently hanging,
> >>>> with the last output from the guest
> >>>> [    3.183662] Registering SWP/SWPB emulation handler
> >>
> >> I looked through the patches and it does not seem that they can break
> >> anything.
> >> Could it be the same avocado/chardev socket glitch as in some previous
> >> failures?
> >> What happens when re-running this test?
> >
> > I ran it a couple of times with the patchset and it failed the same
> > way each time. Without is fine.
>
> I applied the patches and got no failures on my local machine.
>
> Do you have any ideas on debugging this bug?
> What does "arm-clang" means? Is the host compiler is clang?

Yes, it's a clang build (with the sanitizers enabled, though I didn't
see any output from the sanitizers in the logfile).

thanks
-- PMM

Re: [PATCH for-6.0 00/11] target/arm: enforce alignment

[Richard Henderson]

On 12/4/20 12:17 AM, Pavel Dovgalyuk wrote:
> On 03.12.2020 19:14, Peter Maydell wrote:
>> On Thu, 3 Dec 2020 at 16:10, Pavel Dovgalyuk <pavel.dovgalyuk@ispras.ru> wrote:
>>>
>>> On 03.12.2020 15:30, Philippe Mathieu-Daudé wrote:
>>>> Cc'ing Pavel
>>>>
>>>> On 12/1/20 4:55 PM, Peter Maydell wrote:
>>>>> On Wed, 25 Nov 2020 at 04:06, Richard Henderson
>>>>> <richard.henderson@linaro.org> wrote:
>>>>>>
>>>>>> As reported in https://bugs.launchpad.net/bugs/1905356
>>>>>>
>>>>>> Not implementing SCTLR.A, but all of the other required
>>>>>> alignment for SCTLR.A=0 in Table A3-1.
>>>>>
>>>>> Something in this series breaks the 'make check-acceptance'
>>>>> record-and-replay test:
>>>>>
>>>>>    (30/40)
>>>>> tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_aarch64_virt:
>>>>> PASS (9.14 s)
>>>>>    (31/40)
>>>>> tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt:
>>>>> INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred:
>>>>> Timeout reached\nOriginal status: ERROR\n{'name':
>>>>> '31-tests/acceptance/replay_kernel.py:ReplayKernelNormal.test_arm_virt',
>>>>> 'logdir':
>>>>> '/home/petmay01/linaro/qemu-from-laptop/qemu/build/arm-clang/tests/result...
>>>>> (90.19 s)
>>>>>
>>>>> The log shows the "recording execution" apparently hanging,
>>>>> with the last output from the guest
>>>>> [    3.183662] Registering SWP/SWPB emulation handler
>>>
>>> I looked through the patches and it does not seem that they can break
>>> anything.
>>> Could it be the same avocado/chardev socket glitch as in some previous
>>> failures?
>>> What happens when re-running this test?
>>
>> I ran it a couple of times with the patchset and it failed the same
>> way each time. Without is fine.
> 
> I applied the patches and got no failures on my local machine.
> 
> Do you have any ideas on debugging this bug?
> What does "arm-clang" means? Is the host compiler is clang?

I have reproduced it:

qemu-system-arm: /home/rth/qemu/qemu/include/tcg/tcg.h:339: get_alignment_bits:
Assertion `(((1 << (10 - 1)) | (1 << (10 - 2)) | (1 << (10 - 3)) | (1 << (10 -
4)) | (1 << (10 - 5)) | (1 << (10 - 6))) & ((1 << a) - 1)) == 0' failed.
Aborted (core dumped)

You need --enable-debug-tcg for this assert.

It's incredibly stupid of avocado to report SIGABRT as a timeout.


r~