diff mbox series

target/arm: Emit barriers for A32/T32 load-acquire/store-release insns

Message ID 20181210152850.435-1-peter.maydell@linaro.org
State Superseded
Headers show
Series target/arm: Emit barriers for A32/T32 load-acquire/store-release insns | expand

Commit Message

Peter Maydell Dec. 10, 2018, 3:28 p.m. UTC 
Now that MTTCG is here, the comment in the 32-bit Arm decoder that
"Since the emulation does not have barriers, the acquire/release
semantics need no special handling" is no longer true. Emit the
correct barriers for the load-acquire/store-release insns, as
we already do in the A64 decoder.

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

---
I've not run into this in practice, and there's very little
aarch32 code out there that is built to use the new-in-v8
instructions as far as I'm aware, but since it would be very
painful to track down this bug if we ran into it in the
wild it seems worth fixing...
---
 target/arm/translate.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

-- 
2.19.2

Comments

Alex Bennée Dec. 19, 2018, 6:45 p.m. UTC | #1 
Peter Maydell <peter.maydell@linaro.org> writes:

> Now that MTTCG is here, the comment in the 32-bit Arm decoder that

> "Since the emulation does not have barriers, the acquire/release

> semantics need no special handling" is no longer true. Emit the

> correct barriers for the load-acquire/store-release insns, as

> we already do in the A64 decoder.

>

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

> ---

> I've not run into this in practice, and there's very little

> aarch32 code out there that is built to use the new-in-v8

> instructions as far as I'm aware, but since it would be very

> painful to track down this bug if we ran into it in the

> wild it seems worth fixing...


There should be a patch in reply to this that ports my barrier tests to
a linux-user tcg test case. However I've been unable to get the "mp"
test to fail under translation, even on aarch64 hosts
(ThunderX/qemu-test) which are nominally out of order. x86 hosts are
pretty forgiving anyway. The SynQuacer being A53 based is stubbornly
in-order so won't fail anyway. If you have access to any fancy OoO
AArch32 hardware to confirm the test is good please let me know.

Anyway I can at least confirm that when running the mp_acqrel test case
that the correct barrier ops are emitted. All the barrier tests pass
so:

Tested-by: Alex Bennée <alex.bennee@linaro.org>

[by inspection]
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>



> ---

>  target/arm/translate.c | 33 ++++++++++++++++++++++++++-------

>  1 file changed, 26 insertions(+), 7 deletions(-)

>

> diff --git a/target/arm/translate.c b/target/arm/translate.c

> index 7c4675ffd8a..e8fd824f3f5 100644

> --- a/target/arm/translate.c

> +++ b/target/arm/translate.c

> @@ -9733,6 +9733,8 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>                      rd = (insn >> 12) & 0xf;

>                      if (insn & (1 << 23)) {

>                          /* load/store exclusive */

> +                        bool is_ld = extract32(insn, 20, 1);

> +                        bool is_lasr = !extract32(insn, 8, 1);

>                          int op2 = (insn >> 8) & 3;

>                          op1 = (insn >> 21) & 0x3;

>

> @@ -9760,11 +9762,12 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>                          addr = tcg_temp_local_new_i32();

>                          load_reg_var(s, addr, rn);

>

> -                        /* Since the emulation does not have barriers,

> -                           the acquire/release semantics need no special

> -                           handling */

> +                        if (is_lasr && !is_ld) {

> +                            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);

> +                        }

> +

>                          if (op2 == 0) {

> -                            if (insn & (1 << 20)) {

> +                            if (is_ld) {

>                                  tmp = tcg_temp_new_i32();

>                                  switch (op1) {

>                                  case 0: /* lda */

> @@ -9810,7 +9813,7 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>                                  }

>                                  tcg_temp_free_i32(tmp);

>                              }

> -                        } else if (insn & (1 << 20)) {

> +                        } else if (is_ld) {

>                              switch (op1) {

>                              case 0: /* ldrex */

>                                  gen_load_exclusive(s, rd, 15, addr, 2);

> @@ -9847,6 +9850,10 @@ static void disas_arm_insn(DisasContext *s, unsigned int insn)

>                              }

>                          }

>                          tcg_temp_free_i32(addr);

> +

> +                        if (is_lasr && is_ld) {

> +                            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);

> +                        }

>                      } else if ((insn & 0x00300f00) == 0) {

>                          /* 0bcccc_0001_0x00_xxxx_xxxx_0000_1001_xxxx

>                          *  - SWP, SWPB

> @@ -10862,6 +10869,8 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>                  tcg_gen_addi_i32(tmp, tmp, s->pc);

>                  store_reg(s, 15, tmp);

>              } else {

> +                bool is_lasr = false;

> +                bool is_ld = extract32(insn, 20, 1);

>                  int op2 = (insn >> 6) & 0x3;

>                  op = (insn >> 4) & 0x3;

>                  switch (op2) {

> @@ -10883,12 +10892,18 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>                  case 3:

>                      /* Load-acquire/store-release exclusive */

>                      ARCH(8);

> +                    is_lasr = true;

>                      break;

>                  }

> +

> +                if (is_lasr && !is_ld) {

> +                    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);

> +                }

> +

>                  addr = tcg_temp_local_new_i32();

>                  load_reg_var(s, addr, rn);

>                  if (!(op2 & 1)) {

> -                    if (insn & (1 << 20)) {

> +                    if (is_ld) {

>                          tmp = tcg_temp_new_i32();

>                          switch (op) {

>                          case 0: /* ldab */

> @@ -10927,12 +10942,16 @@ static void disas_thumb2_insn(DisasContext *s, uint32_t insn)

>                          }

>                          tcg_temp_free_i32(tmp);

>                      }

> -                } else if (insn & (1 << 20)) {

> +                } else if (is_ld) {

>                      gen_load_exclusive(s, rs, rd, addr, op);

>                  } else {

>                      gen_store_exclusive(s, rm, rs, rd, addr, op);

>                  }

>                  tcg_temp_free_i32(addr);

> +

> +                if (is_lasr && is_ld) {

> +                    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);

> +                }

>              }

>          } else {

>              /* Load/store multiple, RFE, SRS.  */



--
Alex Bennée
diff mbox series

Patch

diff --git a/target/arm/translate.c b/target/arm/translate.c
index 7c4675ffd8a..e8fd824f3f5 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -9733,6 +9733,8 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                     rd = (insn >> 12) & 0xf;
                     if (insn & (1 << 23)) {
                         /* load/store exclusive */
+                        bool is_ld = extract32(insn, 20, 1);
+                        bool is_lasr = !extract32(insn, 8, 1);
                         int op2 = (insn >> 8) & 3;
                         op1 = (insn >> 21) & 0x3;
 
@@ -9760,11 +9762,12 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                         addr = tcg_temp_local_new_i32();
                         load_reg_var(s, addr, rn);
 
-                        /* Since the emulation does not have barriers,
-                           the acquire/release semantics need no special
-                           handling */
+                        if (is_lasr && !is_ld) {
+                            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
+                        }
+
                         if (op2 == 0) {
-                            if (insn & (1 << 20)) {
+                            if (is_ld) {
                                 tmp = tcg_temp_new_i32();
                                 switch (op1) {
                                 case 0: /* lda */
@@ -9810,7 +9813,7 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                                 }
                                 tcg_temp_free_i32(tmp);
                             }
-                        } else if (insn & (1 << 20)) {
+                        } else if (is_ld) {
                             switch (op1) {
                             case 0: /* ldrex */
                                 gen_load_exclusive(s, rd, 15, addr, 2);
@@ -9847,6 +9850,10 @@  static void disas_arm_insn(DisasContext *s, unsigned int insn)
                             }
                         }
                         tcg_temp_free_i32(addr);
+
+                        if (is_lasr && is_ld) {
+                            tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
+                        }
                     } else if ((insn & 0x00300f00) == 0) {
                         /* 0bcccc_0001_0x00_xxxx_xxxx_0000_1001_xxxx
                         *  - SWP, SWPB
@@ -10862,6 +10869,8 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 tcg_gen_addi_i32(tmp, tmp, s->pc);
                 store_reg(s, 15, tmp);
             } else {
+                bool is_lasr = false;
+                bool is_ld = extract32(insn, 20, 1);
                 int op2 = (insn >> 6) & 0x3;
                 op = (insn >> 4) & 0x3;
                 switch (op2) {
@@ -10883,12 +10892,18 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                 case 3:
                     /* Load-acquire/store-release exclusive */
                     ARCH(8);
+                    is_lasr = true;
                     break;
                 }
+
+                if (is_lasr && !is_ld) {
+                    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_STRL);
+                }
+
                 addr = tcg_temp_local_new_i32();
                 load_reg_var(s, addr, rn);
                 if (!(op2 & 1)) {
-                    if (insn & (1 << 20)) {
+                    if (is_ld) {
                         tmp = tcg_temp_new_i32();
                         switch (op) {
                         case 0: /* ldab */
@@ -10927,12 +10942,16 @@  static void disas_thumb2_insn(DisasContext *s, uint32_t insn)
                         }
                         tcg_temp_free_i32(tmp);
                     }
-                } else if (insn & (1 << 20)) {
+                } else if (is_ld) {
                     gen_load_exclusive(s, rs, rd, addr, op);
                 } else {
                     gen_store_exclusive(s, rm, rs, rd, addr, op);
                 }
                 tcg_temp_free_i32(addr);
+
+                if (is_lasr && is_ld) {
+                    tcg_gen_mb(TCG_MO_ALL | TCG_BAR_LDAQ);
+                }
             }
         } else {
             /* Load/store multiple, RFE, SRS.  */