* [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns
@ 2018-12-10 15:28 Peter Maydell
2018-12-19 18:39 ` [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM Alex Bennée
2018-12-19 18:45 ` [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Alex Bennée
0 siblings, 2 replies; 5+ messages in thread
From: Peter Maydell @ 2018-12-10 15:28 UTC (permalink / raw)
To: qemu-arm, qemu-devel; +Cc: patches, Alex Bennée, Richard Henderson
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM
2018-12-10 15:28 [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Peter Maydell
@ 2018-12-19 18:39 ` Alex Bennée
2018-12-20 11:17 ` Alex Bennée
2018-12-25 13:49 ` no-reply
2018-12-19 18:45 ` [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Alex Bennée
1 sibling, 2 replies; 5+ messages in thread
From: Alex Bennée @ 2018-12-19 18:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, peter.maydell, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns
2018-12-10 15:28 [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Peter Maydell
2018-12-19 18:39 ` [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM Alex Bennée
@ 2018-12-19 18:45 ` Alex Bennée
1 sibling, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2018-12-19 18:45 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-arm, qemu-devel, patches, Richard Henderson
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM
2018-12-19 18:39 ` [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM Alex Bennée
@ 2018-12-20 11:17 ` Alex Bennée
2018-12-25 13:49 ` no-reply
1 sibling, 0 replies; 5+ messages in thread
From: Alex Bennée @ 2018-12-20 11:17 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, peter.maydell
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM
2018-12-19 18:39 ` [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM Alex Bennée
2018-12-20 11:17 ` Alex Bennée
@ 2018-12-25 13:49 ` no-reply
1 sibling, 0 replies; 5+ messages in thread
From: no-reply @ 2018-12-25 13:49 UTC (permalink / raw)
To: alex.bennee; +Cc: fam, qemu-devel, peter.maydell, qemu-arm
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-12-25 14:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-10 15:28 [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Peter Maydell
2018-12-19 18:39 ` [Qemu-devel] [PATCH] tests/tcg: add barrier test for ARM Alex Bennée
2018-12-20 11:17 ` Alex Bennée
2018-12-25 13:49 ` no-reply
2018-12-19 18:45 ` [Qemu-devel] [PATCH] target/arm: Emit barriers for A32/T32 load-acquire/store-release insns Alex Bennée
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).