[Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns

[Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns

[Peter Maydell]

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(-)

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.  */
-- 
2.19.2

[Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM

[Alex Bennée]

This is a port of my kvm-unit-tests barrier test. A couple of things
are done in a more user-space friendly way but the tests are the same.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/tcg/arm/Makefile.target |   6 +-
 tests/tcg/arm/barrier.c       | 472 ++++++++++++++++++++++++++++++++++
 2 files changed, 477 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/arm/barrier.c

diff --git a/tests/tcg/arm/Makefile.target b/tests/tcg/arm/Makefile.target
index aa4e4e3782..389616cb19 100644
--- a/tests/tcg/arm/Makefile.target
+++ b/tests/tcg/arm/Makefile.target
@@ -10,7 +10,7 @@ VPATH 		+= $(ARM_SRC)
 
 ARM_TESTS=hello-arm test-arm-iwmmxt
 
-TESTS += $(ARM_TESTS) fcvt
+TESTS += $(ARM_TESTS) fcvt barrier
 
 hello-arm: CFLAGS+=-marm -ffreestanding
 hello-arm: LDFLAGS+=-nostdlib
@@ -30,3 +30,7 @@ endif
 
 # On ARM Linux only supports 4k pages
 EXTRA_RUNS+=run-test-mmap-4096
+
+# Barrier tests need atomic definitions, steal QEMUs
+barrier: CFLAGS+=-I$(SRC_PATH)/include/qemu
+barrier: LDFLAGS+=-lpthread
diff --git a/tests/tcg/arm/barrier.c b/tests/tcg/arm/barrier.c
new file mode 100644
index 0000000000..ef47911e36
--- /dev/null
+++ b/tests/tcg/arm/barrier.c
@@ -0,0 +1,472 @@
+/*
+ * ARM Barrier Litmus Tests
+ *
+ * This test provides a framework for testing barrier conditions on
+ * the processor. It's simpler than the more involved barrier testing
+ * frameworks as we are looking for simple failures of QEMU's TCG not
+ * weird edge cases the silicon gets wrong.
+ */
+
+#include <string.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <pthread.h>
+#include <errno.h>
+
+/*
+ * Memory barriers from atomic.h
+ *
+ * While it would be nice to include atomic.h directly that
+ * complicates the build. However we can assume a modern compilers
+ * with the full set of __atomic C11 primitives.
+ */
+
+#define barrier()          ({ asm volatile("" ::: "memory"); (void)0; })
+#define smp_mb()           ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })
+#define smp_mb_release()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); })
+#define smp_mb_acquire()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); })
+
+#define smp_wmb()          smp_mb_release()
+#define smp_rmb()          smp_mb_acquire()
+
+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
+
+#define MAX_THREADS 2
+
+/* Array size and access controls */
+static int array_size = 100000;
+static int wait_if_ahead = 0;
+
+/*
+ * These test_array_* structures are a contiguous array modified by two or more
+ * competing CPUs. The padding is to ensure the variables do not share
+ * cache lines.
+ *
+ * All structures start zeroed.
+ */
+
+typedef struct test_array
+{
+    volatile unsigned int x;
+    uint8_t dummy[64];
+    volatile unsigned int y;
+    uint8_t dummy2[64];
+    volatile unsigned int r[MAX_THREADS];
+} test_array;
+
+/* Test definition structure
+ *
+ * The first function will always run on the primary CPU, it is
+ * usually the one that will detect any weirdness and trigger the
+ * failure of the test.
+ */
+
+typedef int (*test_fn)(void *arg);
+typedef void * (*thread_fn)(void *arg);
+
+typedef struct {
+    char *test_name;
+    bool  should_pass;
+    test_fn main_fn;
+    thread_fn secondary_fn;
+} test_descr_t;
+
+/*
+ * Synchronisation Helpers
+ */
+
+pthread_barrier_t sync_barrier;
+
+static void init_sync_point(void)
+{
+    pthread_barrier_init(&sync_barrier, NULL, 2);
+    smp_mb();
+}
+
+static inline void wait_for_main_thread()
+{
+    pthread_barrier_wait(&sync_barrier);
+}
+
+static inline void wake_up_secondary_thread()
+{
+    pthread_barrier_wait(&sync_barrier);
+}
+
+/*
+ * Litmus tests
+ */
+
+/* Simple Message Passing
+ *
+ * x is the message data
+ * y is the flag to indicate the data is ready
+ *
+ * Reading x == 0 when y == 1 is a failure.
+ */
+
+static void * message_passing_write(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wait_for_main_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        entry->x = 1;
+        entry->y = 1;
+    }
+
+    return NULL;
+}
+
+static int message_passing_read(void *arg)
+{
+    int i;
+    int errors = 0, ready = 0;
+    test_array *array = (test_array *) arg;
+
+    wake_up_secondary_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int x,y;
+        y = entry->y;
+        x = entry->x;
+
+        if (y && !x)
+            errors++;
+        ready += y;
+    }
+
+    return errors;
+}
+
+/* Simple Message Passing with barriers */
+static void * message_passing_write_barrier(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wait_for_main_thread();
+
+    for (i = 0; i< array_size; i++) {
+        volatile test_array *entry = &array[i];
+        entry->x = 1;
+        smp_wmb();
+        entry->y = 1;
+    }
+
+    return NULL;
+}
+
+static int message_passing_read_barrier(void *arg)
+{
+    int i;
+    int errors = 0, ready = 0, not_ready = 0;
+    test_array *array = (test_array *) arg;
+
+    wake_up_secondary_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int x, y;
+        y = entry->y;
+        smp_rmb();
+        x = entry->x;
+
+        if (y && !x)
+            errors++;
+
+        if (y) {
+            ready++;
+        } else {
+            not_ready++;
+
+            if (not_ready > 2) {
+                entry = &array[i+1];
+                do {
+                    not_ready = 0;
+                } while (wait_if_ahead && !entry->y);
+            }
+        }
+    }
+
+    return errors;
+}
+
+/* Simple Message Passing with Acquire/Release */
+static void * message_passing_write_release(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    for (i=0; i< array_size; i++) {
+        volatile test_array *entry = &array[i];
+        entry->x = 1;
+        __atomic_store_n(&entry->y, 1, __ATOMIC_RELEASE);
+    }
+
+    return NULL;
+}
+
+static int message_passing_read_acquire(void *arg)
+{
+    int i;
+    int errors = 0, ready = 0, not_ready = 0;
+    test_array *array = (test_array *) arg;
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int x, y;
+        __atomic_load(&entry->y, &y, __ATOMIC_ACQUIRE);
+        x = entry->x;
+
+        if (y && !x)
+            errors++;
+
+        if (y) {
+            ready++;
+        } else {
+            not_ready++;
+
+            if (not_ready > 2) {
+                entry = &array[i+1];
+                do {
+                    not_ready = 0;
+                } while (wait_if_ahead && !entry->y);
+            }
+        }
+    }
+
+    return errors;
+}
+
+/*
+ * Store after load
+ *
+ * T1: write 1 to x, load r from y
+ * T2: write 1 to y, load r from x
+ *
+ * Without memory fence r[0] && r[1] == 0
+ * With memory fence both == 0 should be impossible
+ */
+
+static int check_store_and_load_results(const char *name, int thread, test_array *array)
+{
+    int i;
+    int neither = 0;
+    int only_first = 0;
+    int only_second = 0;
+    int both = 0;
+
+    for (i=0; i< array_size; i++) {
+        volatile test_array *entry = &array[i];
+        if (entry->r[0] == 0 &&
+            entry->r[1] == 0) {
+            neither++;
+        } else if (entry->r[0] &&
+            entry->r[1]) {
+            both++;
+        } else if (entry->r[0]) {
+            only_first++;
+        } else {
+            only_second++;
+        }
+    }
+
+    printf("T%d: neither=%d only_t1=%d only_t2=%d both=%d\n", thread,
+           neither, only_first, only_second, both);
+
+    return neither;
+}
+
+/*
+ * This attempts to synchronise the start of both threads to roughly
+ * the same time. On real hardware there is a little latency as the
+ * secondary vCPUs are powered up however this effect it much more
+ * exaggerated on a TCG host.
+ *
+ * Busy waits until the we pass a future point in time, returns final
+ * start time.
+ */
+
+static int store_and_load_1(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wake_up_secondary_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int r;
+        entry->x = 1;
+        r = entry->y;
+        entry->r[0] = r;
+    }
+
+    return check_store_and_load_results("sal", 1, array);
+}
+
+static void * store_and_load_2(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wait_for_main_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int r;
+        entry->y = 1;
+        r = entry->x;
+        entry->r[1] = r;
+    }
+
+    check_store_and_load_results("sal", 2, array);
+
+    return NULL;
+}
+
+static int store_and_load_barrier_1(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wake_up_secondary_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int r;
+        entry->x = 1;
+        smp_mb();
+        r = entry->y;
+        entry->r[0] = r;
+    }
+
+    smp_mb();
+
+    return check_store_and_load_results("sal_barrier", 1, array);
+}
+
+static void * store_and_load_barrier_2(void *arg)
+{
+    int i;
+    test_array *array = (test_array *) arg;
+
+    wait_for_main_thread();
+
+    for (i = 0; i < array_size; i++) {
+        volatile test_array *entry = &array[i];
+        unsigned int r;
+        entry->y = 1;
+        smp_mb();
+        r = entry->x;
+        entry->r[1] = r;
+    }
+
+    check_store_and_load_results("sal_barrier", 2, array);
+
+    return NULL;
+}
+
+
+/* Test array */
+static test_descr_t tests[] = {
+
+    { "mp",         false,
+      message_passing_read,
+      message_passing_write
+    },
+
+    { "mp_barrier", true,
+      message_passing_read_barrier,
+      message_passing_write_barrier
+    },
+
+    { "mp_acqrel", true,
+      message_passing_read_acquire,
+      message_passing_write_release
+    },
+
+    { "sal",       false,
+      store_and_load_1,
+      store_and_load_2
+    },
+
+    { "sal_barrier", true,
+      store_and_load_barrier_1,
+      store_and_load_barrier_2
+    },
+};
+
+
+int setup_and_run_litmus(test_descr_t *test)
+{
+    pthread_t tid1;
+    int res;
+    test_array *array;
+
+    printf("Running test: %s\n", test->test_name);
+    array = calloc(array_size, sizeof(test_array));
+    printf("Allocated test array @ %p\n", array);
+
+    init_sync_point();
+
+    if (array) {
+        pthread_create(&tid1, NULL, test->secondary_fn, array);
+        res = test->main_fn(array);
+    } else {
+        printf("%s: failed to allocate memory", test->test_name);
+        res = 1;
+    }
+
+    /* ensure secondary thread has finished */
+    pthread_join(tid1, NULL);
+
+    free(array);
+    array = NULL;
+
+    return res;
+}
+
+int main(int argc, char **argv)
+{
+    int i;
+    int res = 0;
+
+    for (i = 0; i < argc; i++) {
+        char *arg = argv[i];
+        unsigned int j;
+
+        /* Test modifiers */
+        if (strstr(arg, "count=") != NULL) {
+            char *p = strstr(arg, "=");
+            array_size = atol(p+1);
+            continue;
+        } else if (strcmp (arg, "wait") == 0) {
+            wait_if_ahead = 1;
+            continue;
+        } else if (strcmp(arg, "help") == 0) {
+            printf("Tests: ");
+            for (j = 0; j < ARRAY_SIZE(tests); j++) {
+                printf("%s ", tests[j].test_name);
+            }
+            printf("\n");
+        }
+
+        for (j = 0; j < ARRAY_SIZE(tests); j++) {
+            if (strcmp(arg, tests[j].test_name) == 0) {
+                res += setup_and_run_litmus(&tests[j]);
+                continue;
+            }
+        }
+    }
+
+    return res;
+}
-- 
2.17.1

Re: [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns

[Alex Bennée]

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

Re: [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> This is a port of my kvm-unit-tests barrier test. A couple of things
> are done in a more user-space friendly way but the tests are the same.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
<snip>
> +
> +# Barrier tests need atomic definitions, steal QEMUs
> +barrier: CFLAGS+=-I$(SRC_PATH)/include/qemu

Hmm that should be:

modified   tests/tcg/arm/Makefile.target
@@ -10,7 +10,7 @@ VPATH                 += $(ARM_SRC)

 ARM_TESTS=hello-arm test-arm-iwmmxt

-TESTS += $(ARM_TESTS) fcvt
+TESTS += $(ARM_TESTS) fcvt barrier

 hello-arm: CFLAGS+=-marm -ffreestanding
 hello-arm: LDFLAGS+=-nostdlib
@@ -30,3 +30,7 @@ endif

 # On ARM Linux only supports 4k pages
 EXTRA_RUNS+=run-test-mmap-4096
+
+# Barrier tests need aqrel
+barrier: CFLAGS+=-march=armv8-a
+barrier: LDFLAGS+=-lpthread


--
Alex Bennée

Re: [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM

[no-reply]

Patchew URL: https://patchew.org/QEMU/20181219183902.27273-1-alex.bennee@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20181219183902.27273-1-alex.bennee@linaro.org
Type: series
Subject: [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
308bf65 tests/tcg: add barrier test for ARM

=== OUTPUT BEGIN ===
Checking PATCH 1/1: tests/tcg: add barrier test for ARM...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#37: 
new file mode 100644

ERROR: spaces required around that '::' (ctx:WxO)
#68: FILE: tests/tcg/arm/barrier.c:27:
+#define barrier()          ({ asm volatile("" ::: "memory"); (void)0; })
                                               ^

ERROR: spaces required around that ':' (ctx:OxW)
#68: FILE: tests/tcg/arm/barrier.c:27:
+#define barrier()          ({ asm volatile("" ::: "memory"); (void)0; })
                                                 ^

WARNING: line over 80 characters
#69: FILE: tests/tcg/arm/barrier.c:28:
+#define smp_mb()           ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })

ERROR: memory barrier without comment
#69: FILE: tests/tcg/arm/barrier.c:28:
+#define smp_mb()           ({ barrier(); __atomic_thread_fence(__ATOMIC_SEQ_CST); })

WARNING: line over 80 characters
#70: FILE: tests/tcg/arm/barrier.c:29:
+#define smp_mb_release()   ({ barrier(); __atomic_thread_fence(__ATOMIC_RELEASE); })

WARNING: line over 80 characters
#71: FILE: tests/tcg/arm/barrier.c:30:
+#define smp_mb_acquire()   ({ barrier(); __atomic_thread_fence(__ATOMIC_ACQUIRE); })

ERROR: memory barrier without comment
#73: FILE: tests/tcg/arm/barrier.c:32:
+#define smp_wmb()          smp_mb_release()

ERROR: memory barrier without comment
#74: FILE: tests/tcg/arm/barrier.c:33:
+#define smp_rmb()          smp_mb_acquire()

ERROR: do not initialise statics to 0 or NULL
#82: FILE: tests/tcg/arm/barrier.c:41:
+static int wait_if_ahead = 0;

ERROR: open brace '{' following struct go on the same line
#93: FILE: tests/tcg/arm/barrier.c:52:
+typedef struct test_array
+{

ERROR: Use of volatile is usually wrong, please add a comment
#94: FILE: tests/tcg/arm/barrier.c:53:
+    volatile unsigned int x;

ERROR: Use of volatile is usually wrong, please add a comment
#96: FILE: tests/tcg/arm/barrier.c:55:
+    volatile unsigned int y;

ERROR: Use of volatile is usually wrong, please add a comment
#98: FILE: tests/tcg/arm/barrier.c:57:
+    volatile unsigned int r[MAX_THREADS];

WARNING: Block comments use a leading /* on a separate line
#101: FILE: tests/tcg/arm/barrier.c:60:
+/* Test definition structure

ERROR: memory barrier without comment
#127: FILE: tests/tcg/arm/barrier.c:86:
+    smp_mb();

WARNING: Block comments use a leading /* on a separate line
#144: FILE: tests/tcg/arm/barrier.c:103:
+/* Simple Message Passing

ERROR: "foo * bar" should be "foo *bar"
#152: FILE: tests/tcg/arm/barrier.c:111:
+static void * message_passing_write(void *arg)

ERROR: Use of volatile is usually wrong, please add a comment
#160: FILE: tests/tcg/arm/barrier.c:119:
+        volatile test_array *entry = &array[i];

ERROR: Use of volatile is usually wrong, please add a comment
#177: FILE: tests/tcg/arm/barrier.c:136:
+        volatile test_array *entry = &array[i];

ERROR: space required after that ',' (ctx:VxV)
#178: FILE: tests/tcg/arm/barrier.c:137:
+        unsigned int x,y;
                       ^

ERROR: braces {} are necessary for all arms of this statement
#182: FILE: tests/tcg/arm/barrier.c:141:
+        if (y && !x)
[...]

ERROR: "foo * bar" should be "foo *bar"
#191: FILE: tests/tcg/arm/barrier.c:150:
+static void * message_passing_write_barrier(void *arg)

ERROR: spaces required around that '<' (ctx:VxW)
#198: FILE: tests/tcg/arm/barrier.c:157:
+    for (i = 0; i< array_size; i++) {
                  ^

ERROR: Use of volatile is usually wrong, please add a comment
#199: FILE: tests/tcg/arm/barrier.c:158:
+        volatile test_array *entry = &array[i];

ERROR: memory barrier without comment
#201: FILE: tests/tcg/arm/barrier.c:160:
+        smp_wmb();

ERROR: Use of volatile is usually wrong, please add a comment
#217: FILE: tests/tcg/arm/barrier.c:176:
+        volatile test_array *entry = &array[i];

ERROR: memory barrier without comment
#220: FILE: tests/tcg/arm/barrier.c:179:
+        smp_rmb();

ERROR: braces {} are necessary for all arms of this statement
#223: FILE: tests/tcg/arm/barrier.c:182:
+        if (y && !x)
[...]

ERROR: spaces required around that '+' (ctx:VxV)
#232: FILE: tests/tcg/arm/barrier.c:191:
+                entry = &array[i+1];
                                 ^

ERROR: "foo * bar" should be "foo *bar"
#244: FILE: tests/tcg/arm/barrier.c:203:
+static void * message_passing_write_release(void *arg)

ERROR: spaces required around that '=' (ctx:VxV)
#249: FILE: tests/tcg/arm/barrier.c:208:
+    for (i=0; i< array_size; i++) {
           ^

ERROR: spaces required around that '<' (ctx:VxW)
#249: FILE: tests/tcg/arm/barrier.c:208:
+    for (i=0; i< array_size; i++) {
                ^

ERROR: Use of volatile is usually wrong, please add a comment
#250: FILE: tests/tcg/arm/barrier.c:209:
+        volatile test_array *entry = &array[i];

ERROR: Use of volatile is usually wrong, please add a comment
#265: FILE: tests/tcg/arm/barrier.c:224:
+        volatile test_array *entry = &array[i];

ERROR: braces {} are necessary for all arms of this statement
#270: FILE: tests/tcg/arm/barrier.c:229:
+        if (y && !x)
[...]

ERROR: spaces required around that '+' (ctx:VxV)
#279: FILE: tests/tcg/arm/barrier.c:238:
+                entry = &array[i+1];
                                 ^

WARNING: line over 80 characters
#300: FILE: tests/tcg/arm/barrier.c:259:
+static int check_store_and_load_results(const char *name, int thread, test_array *array)

ERROR: spaces required around that '=' (ctx:VxV)
#308: FILE: tests/tcg/arm/barrier.c:267:
+    for (i=0; i< array_size; i++) {
           ^

ERROR: spaces required around that '<' (ctx:VxW)
#308: FILE: tests/tcg/arm/barrier.c:267:
+    for (i=0; i< array_size; i++) {
                ^

ERROR: Use of volatile is usually wrong, please add a comment
#309: FILE: tests/tcg/arm/barrier.c:268:
+        volatile test_array *entry = &array[i];

ERROR: Use of volatile is usually wrong, please add a comment
#347: FILE: tests/tcg/arm/barrier.c:306:
+        volatile test_array *entry = &array[i];

ERROR: "foo * bar" should be "foo *bar"
#357: FILE: tests/tcg/arm/barrier.c:316:
+static void * store_and_load_2(void *arg)

ERROR: Use of volatile is usually wrong, please add a comment
#365: FILE: tests/tcg/arm/barrier.c:324:
+        volatile test_array *entry = &array[i];

ERROR: Use of volatile is usually wrong, please add a comment
#385: FILE: tests/tcg/arm/barrier.c:344:
+        volatile test_array *entry = &array[i];

ERROR: memory barrier without comment
#388: FILE: tests/tcg/arm/barrier.c:347:
+        smp_mb();

ERROR: memory barrier without comment
#393: FILE: tests/tcg/arm/barrier.c:352:
+    smp_mb();

ERROR: "foo * bar" should be "foo *bar"
#398: FILE: tests/tcg/arm/barrier.c:357:
+static void * store_and_load_barrier_2(void *arg)

ERROR: Use of volatile is usually wrong, please add a comment
#406: FILE: tests/tcg/arm/barrier.c:365:
+        volatile test_array *entry = &array[i];

ERROR: memory barrier without comment
#409: FILE: tests/tcg/arm/barrier.c:368:
+        smp_mb();

ERROR: spaces required around that '+' (ctx:VxV)
#491: FILE: tests/tcg/arm/barrier.c:450:
+            array_size = atol(p+1);
                                ^

ERROR: space prohibited between function name and open parenthesis '('
#493: FILE: tests/tcg/arm/barrier.c:452:
+        } else if (strcmp (arg, "wait") == 0) {

total: 45 errors, 7 warnings, 487 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20181219183902.27273-1-alex.bennee@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com