[PATCH v3 0/5] tcg: Issue memory barriers for guest memory model

[PATCH v3 0/5] tcg: Issue memory barriers for guest memory model

[Richard Henderson]

v1: https://lore.kernel.org/qemu-devel/20210316220735.2048137-1-richard.henderson@linaro.org/
v2: https://lore.kernel.org/qemu-devel/20230306015710.1868853-1-richard.henderson@linaro.org/

Changes for v3:
  * Update for tcg-built-once.
  * Require TCG_GUEST_DEFAULT_MO if TARGET_SUPPORTS_MTTCG.


r~


Richard Henderson (5):
  target/microblaze: Define TCG_GUEST_DEFAULT_MO
  tcg: Do not elide memory barriers for !CF_PARALLEL in system mode
  tcg: Elide memory barriers implied by the host memory model
  tcg: Add host memory barriers to cpu_ldst.h interfaces
  accel/tcg: Remove check_tcg_memory_orders_compatible

 accel/tcg/internal.h    | 34 ++++++++++++++++++++++++++++++++++
 target/microblaze/cpu.h |  3 +++
 accel/tcg/cputlb.c      | 10 ++++++++++
 accel/tcg/tcg-all.c     | 39 ++++++++++-----------------------------
 accel/tcg/user-exec.c   | 10 ++++++++++
 tcg/tcg-op.c            | 20 ++++++++++++++++++--
 6 files changed, 85 insertions(+), 31 deletions(-)

-- 
2.34.1

[PATCH v3 1/5] target/microblaze: Define TCG_GUEST_DEFAULT_MO

[Richard Henderson]

The microblaze architecture does not reorder instructions.
While there is an MBAR wait-for-data-access instruction,
this concerns synchronizing with DMA.

This should have been defined when enabling MTTCG.

Cc: Alistair Francis <alistair.francis@wdc.com>
Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Fixes: d449561b130 ("configure: microblaze: Enable mttcg")
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/microblaze/cpu.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
index 88324d0bc1..b474abcc2a 100644
--- a/target/microblaze/cpu.h
+++ b/target/microblaze/cpu.h
@@ -24,6 +24,9 @@
 #include "exec/cpu-defs.h"
 #include "qemu/cpu-float.h"
 
+/* MicroBlaze is always in-order. */
+#define TCG_GUEST_DEFAULT_MO  TCG_MO_ALL
+
 typedef struct CPUArchState CPUMBState;
 #if !defined(CONFIG_USER_ONLY)
 #include "mmu.h"
-- 
2.34.1

[PATCH v3 2/5] tcg: Do not elide memory barriers for !CF_PARALLEL in system mode

[Richard Henderson]

The virtio devices require proper memory ordering between
the vcpus and the iothreads.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/tcg-op.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index c07de5d9f8..7aadb37756 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -102,7 +102,19 @@ void tcg_gen_br(TCGLabel *l)
 
 void tcg_gen_mb(TCGBar mb_type)
 {
-    if (tcg_ctx->gen_tb->cflags & CF_PARALLEL) {
+#ifdef CONFIG_USER_ONLY
+    bool parallel = tcg_ctx->gen_tb->cflags & CF_PARALLEL;
+#else
+    /*
+     * It is tempting to elide the barrier in a uniprocessor context.
+     * However, even with a single cpu we have i/o threads running in
+     * parallel, and lack of memory order can result in e.g. virtio
+     * queue entries being read incorrectly.
+     */
+    bool parallel = true;
+#endif
+
+    if (parallel) {
         tcg_gen_op1(INDEX_op_mb, mb_type);
     }
 }
-- 
2.34.1

[PATCH v3 3/5] tcg: Elide memory barriers implied by the host memory model

[Richard Henderson]

Reduce the set of required barriers to those needed by
the host right from the beginning.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/tcg-op.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 7aadb37756..574001c221 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -115,7 +115,11 @@ void tcg_gen_mb(TCGBar mb_type)
 #endif
 
     if (parallel) {
-        tcg_gen_op1(INDEX_op_mb, mb_type);
+        /* We can elide anything which the host provides for free. */
+        mb_type &= ~TCG_TARGET_DEFAULT_MO;
+        if (mb_type & TCG_MO_ALL) {
+            tcg_gen_op1(INDEX_op_mb, mb_type);
+        }
     }
 }
 
-- 
2.34.1

[PATCH v3 4/5] tcg: Add host memory barriers to cpu_ldst.h interfaces

[Richard Henderson]

Bring the helpers into line with the rest of tcg in respecting
guest memory ordering.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/internal.h  | 34 ++++++++++++++++++++++++++++++++++
 accel/tcg/cputlb.c    | 10 ++++++++++
 accel/tcg/user-exec.c | 10 ++++++++++
 3 files changed, 54 insertions(+)

diff --git a/accel/tcg/internal.h b/accel/tcg/internal.h
index 24f225cac7..be0c7753fb 100644
--- a/accel/tcg/internal.h
+++ b/accel/tcg/internal.h
@@ -78,4 +78,38 @@ extern int64_t max_advance;
 
 extern bool one_insn_per_tb;
 
+/**
+ * tcg_req_mo:
+ * @type: TCGBar
+ *
+ * Filter @type to the barrier that is required for the guest
+ * memory ordering vs the host memory ordering.  A non-zero
+ * result indicates that some barrier is required.
+ *
+ * If TCG_GUEST_DEFAULT_MO is not defined, assume that the
+ * guest requires strict ordering.
+ *
+ * This is a macro so that it's constant even without optimization.
+ */
+#ifdef TCG_GUEST_DEFAULT_MO
+# define tcg_req_mo(type) \
+    ((type) & TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO)
+#else
+# define tcg_req_mo(type) ((type) & ~TCG_TARGET_DEFAULT_MO)
+#endif
+
+/**
+ * cpu_req_mo:
+ * @type: TCGBar
+ *
+ * If tcg_req_mo indicates a barrier for @type is required
+ * for the guest memory model, issue a host memory barrier.
+ */
+#define cpu_req_mo(type)          \
+    do {                          \
+        if (tcg_req_mo(type)) {   \
+            smp_mb();             \
+        }                         \
+    } while (0)
+
 #endif /* ACCEL_TCG_INTERNAL_H */
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index 5e2ca47243..a48e1c9693 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -2342,6 +2342,7 @@ static uint8_t do_ld1_mmu(CPUArchState *env, target_ulong addr, MemOpIdx oi,
     MMULookupLocals l;
     bool crosspage;
 
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     crosspage = mmu_lookup(env, addr, oi, ra, access_type, &l);
     tcg_debug_assert(!crosspage);
 
@@ -2363,6 +2364,7 @@ static uint16_t do_ld2_mmu(CPUArchState *env, target_ulong addr, MemOpIdx oi,
     uint16_t ret;
     uint8_t a, b;
 
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     crosspage = mmu_lookup(env, addr, oi, ra, access_type, &l);
     if (likely(!crosspage)) {
         return do_ld_2(env, &l.page[0], l.mmu_idx, access_type, l.memop, ra);
@@ -2393,6 +2395,7 @@ static uint32_t do_ld4_mmu(CPUArchState *env, target_ulong addr, MemOpIdx oi,
     bool crosspage;
     uint32_t ret;
 
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     crosspage = mmu_lookup(env, addr, oi, ra, access_type, &l);
     if (likely(!crosspage)) {
         return do_ld_4(env, &l.page[0], l.mmu_idx, access_type, l.memop, ra);
@@ -2420,6 +2423,7 @@ static uint64_t do_ld8_mmu(CPUArchState *env, target_ulong addr, MemOpIdx oi,
     bool crosspage;
     uint64_t ret;
 
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     crosspage = mmu_lookup(env, addr, oi, ra, access_type, &l);
     if (likely(!crosspage)) {
         return do_ld_8(env, &l.page[0], l.mmu_idx, access_type, l.memop, ra);
@@ -2472,6 +2476,7 @@ static Int128 do_ld16_mmu(CPUArchState *env, target_ulong addr,
     Int128 ret;
     int first;
 
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_LOAD, &l);
     if (likely(!crosspage)) {
         /* Perform the load host endian. */
@@ -2804,6 +2809,7 @@ void helper_stb_mmu(CPUArchState *env, uint64_t addr, uint32_t val,
     bool crosspage;
 
     tcg_debug_assert((get_memop(oi) & MO_SIZE) == MO_8);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_STORE, &l);
     tcg_debug_assert(!crosspage);
 
@@ -2817,6 +2823,7 @@ static void do_st2_mmu(CPUArchState *env, target_ulong addr, uint16_t val,
     bool crosspage;
     uint8_t a, b;
 
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_STORE, &l);
     if (likely(!crosspage)) {
         do_st_2(env, &l.page[0], val, l.mmu_idx, l.memop, ra);
@@ -2845,6 +2852,7 @@ static void do_st4_mmu(CPUArchState *env, target_ulong addr, uint32_t val,
     MMULookupLocals l;
     bool crosspage;
 
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_STORE, &l);
     if (likely(!crosspage)) {
         do_st_4(env, &l.page[0], val, l.mmu_idx, l.memop, ra);
@@ -2872,6 +2880,7 @@ static void do_st8_mmu(CPUArchState *env, target_ulong addr, uint64_t val,
     MMULookupLocals l;
     bool crosspage;
 
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_STORE, &l);
     if (likely(!crosspage)) {
         do_st_8(env, &l.page[0], val, l.mmu_idx, l.memop, ra);
@@ -2901,6 +2910,7 @@ static void do_st16_mmu(CPUArchState *env, target_ulong addr, Int128 val,
     uint64_t a, b;
     int first;
 
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     crosspage = mmu_lookup(env, addr, oi, ra, MMU_DATA_STORE, &l);
     if (likely(!crosspage)) {
         /* Swap to host endian if necessary, then store. */
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index dc8d6b5d40..ce65021cd4 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -914,6 +914,7 @@ static uint8_t do_ld1_mmu(CPUArchState *env, abi_ptr addr,
     uint8_t ret;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_8);
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_LOAD);
     ret = ldub_p(haddr);
     clear_helper_retaddr();
@@ -947,6 +948,7 @@ static uint16_t do_ld2_mmu(CPUArchState *env, abi_ptr addr,
     uint16_t ret;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_16);
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_LOAD);
     ret = load_atom_2(env, ra, haddr, mop);
     clear_helper_retaddr();
@@ -984,6 +986,7 @@ static uint32_t do_ld4_mmu(CPUArchState *env, abi_ptr addr,
     uint32_t ret;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_32);
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_LOAD);
     ret = load_atom_4(env, ra, haddr, mop);
     clear_helper_retaddr();
@@ -1021,6 +1024,7 @@ static uint64_t do_ld8_mmu(CPUArchState *env, abi_ptr addr,
     uint64_t ret;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_64);
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_LOAD);
     ret = load_atom_8(env, ra, haddr, mop);
     clear_helper_retaddr();
@@ -1052,6 +1056,7 @@ static Int128 do_ld16_mmu(CPUArchState *env, abi_ptr addr,
     Int128 ret;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_128);
+    cpu_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_LOAD);
     ret = load_atom_16(env, ra, haddr, mop);
     clear_helper_retaddr();
@@ -1087,6 +1092,7 @@ static void do_st1_mmu(CPUArchState *env, abi_ptr addr, uint8_t val,
     void *haddr;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_8);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_STORE);
     stb_p(haddr, val);
     clear_helper_retaddr();
@@ -1111,6 +1117,7 @@ static void do_st2_mmu(CPUArchState *env, abi_ptr addr, uint16_t val,
     void *haddr;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_16);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_STORE);
 
     if (mop & MO_BSWAP) {
@@ -1139,6 +1146,7 @@ static void do_st4_mmu(CPUArchState *env, abi_ptr addr, uint32_t val,
     void *haddr;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_32);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_STORE);
 
     if (mop & MO_BSWAP) {
@@ -1167,6 +1175,7 @@ static void do_st8_mmu(CPUArchState *env, abi_ptr addr, uint64_t val,
     void *haddr;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_64);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_STORE);
 
     if (mop & MO_BSWAP) {
@@ -1195,6 +1204,7 @@ static void do_st16_mmu(CPUArchState *env, abi_ptr addr, Int128 val,
     void *haddr;
 
     tcg_debug_assert((mop & MO_SIZE) == MO_128);
+    cpu_req_mo(TCG_MO_LD_ST | TCG_MO_ST_ST);
     haddr = cpu_mmu_lookup(env, addr, mop, ra, MMU_DATA_STORE);
 
     if (mop & MO_BSWAP) {
-- 
2.34.1

[PATCH v3 5/5] accel/tcg: Remove check_tcg_memory_orders_compatible

[Richard Henderson]

We now issue host memory barriers to match the guest memory order.
Continue to disable MTTCG only if the guest has not been ported.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/tcg-all.c | 39 ++++++++++-----------------------------
 1 file changed, 10 insertions(+), 29 deletions(-)

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 02af6a2891..03dfd67e9e 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -64,37 +64,23 @@ DECLARE_INSTANCE_CHECKER(TCGState, TCG_STATE,
  * they can set the appropriate CONFIG flags in ${target}-softmmu.mak
  *
  * Once a guest architecture has been converted to the new primitives
- * there are two remaining limitations to check.
- *
- * - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
- * - The host must have a stronger memory order than the guest
- *
- * It may be possible in future to support strong guests on weak hosts
- * but that will require tagging all load/stores in a guest with their
- * implicit memory order requirements which would likely slow things
- * down a lot.
+ * there is one remaining limitation to check:
+ *   - The guest can't be oversized (e.g. 64 bit guest on 32 bit host)
  */
 
-static bool check_tcg_memory_orders_compatible(void)
-{
-#if defined(TCG_GUEST_DEFAULT_MO) && defined(TCG_TARGET_DEFAULT_MO)
-    return (TCG_GUEST_DEFAULT_MO & ~TCG_TARGET_DEFAULT_MO) == 0;
-#else
-    return false;
-#endif
-}
-
 static bool default_mttcg_enabled(void)
 {
     if (icount_enabled() || TCG_OVERSIZED_GUEST) {
         return false;
-    } else {
-#ifdef TARGET_SUPPORTS_MTTCG
-        return check_tcg_memory_orders_compatible();
-#else
-        return false;
-#endif
     }
+#ifdef TARGET_SUPPORTS_MTTCG
+# ifndef TCG_GUEST_DEFAULT_MO
+#  error "TARGET_SUPPORTS_MTTCG without TCG_GUEST_DEFAULT_MO"
+# endif
+    return true;
+#else
+    return false;
+#endif
 }
 
 static void tcg_accel_instance_init(Object *obj)
@@ -162,11 +148,6 @@ static void tcg_set_thread(Object *obj, const char *value, Error **errp)
             warn_report("Guest not yet converted to MTTCG - "
                         "you may get unexpected results");
 #endif
-            if (!check_tcg_memory_orders_compatible()) {
-                warn_report("Guest expects a stronger memory ordering "
-                            "than the host provides");
-                error_printf("This may cause strange/hard to debug errors\n");
-            }
             s->mttcg_enabled = true;
         }
     } else if (strcmp(value, "single") == 0) {
-- 
2.34.1

Re: [PATCH v3 0/5] tcg: Issue memory barriers for guest memory model

[Richard Henderson]

On 6/19/23 16:23, Richard Henderson wrote:
> v1: https://lore.kernel.org/qemu-devel/20210316220735.2048137-1-richard.henderson@linaro.org/
> v2: https://lore.kernel.org/qemu-devel/20230306015710.1868853-1-richard.henderson@linaro.org/
> 
> Changes for v3:
>    * Update for tcg-built-once.
>    * Require TCG_GUEST_DEFAULT_MO if TARGET_SUPPORTS_MTTCG.

I just noticed that patches 2,3,5 were reviewed (thanks phil)
and I failed to copy the r-b.  I have now done so.


r~

> Richard Henderson (5):
>    target/microblaze: Define TCG_GUEST_DEFAULT_MO
>    tcg: Do not elide memory barriers for !CF_PARALLEL in system mode
>    tcg: Elide memory barriers implied by the host memory model
>    tcg: Add host memory barriers to cpu_ldst.h interfaces
>    accel/tcg: Remove check_tcg_memory_orders_compatible
> 
>   accel/tcg/internal.h    | 34 ++++++++++++++++++++++++++++++++++
>   target/microblaze/cpu.h |  3 +++
>   accel/tcg/cputlb.c      | 10 ++++++++++
>   accel/tcg/tcg-all.c     | 39 ++++++++++-----------------------------
>   accel/tcg/user-exec.c   | 10 ++++++++++
>   tcg/tcg-op.c            | 20 ++++++++++++++++++--
>   6 files changed, 85 insertions(+), 31 deletions(-)

Re: [PATCH v3 4/5] tcg: Add host memory barriers to cpu_ldst.h interfaces

[Philippe Mathieu-Daudé]

On 19/6/23 16:23, Richard Henderson wrote:
> Bring the helpers into line with the rest of tcg in respecting
> guest memory ordering.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/internal.h  | 34 ++++++++++++++++++++++++++++++++++
>   accel/tcg/cputlb.c    | 10 ++++++++++
>   accel/tcg/user-exec.c | 10 ++++++++++
>   3 files changed, 54 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/5] target/microblaze: Define TCG_GUEST_DEFAULT_MO

[Philippe Mathieu-Daudé]

On 19/6/23 16:23, Richard Henderson wrote:
> The microblaze architecture does not reorder instructions.
> While there is an MBAR wait-for-data-access instruction,
> this concerns synchronizing with DMA.
> 
> This should have been defined when enabling MTTCG.
> 
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Fixes: d449561b130 ("configure: microblaze: Enable mttcg")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/microblaze/cpu.h | 3 +++
>   1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/5] target/microblaze: Define TCG_GUEST_DEFAULT_MO

[Philippe Mathieu-Daudé]

On 19/6/23 16:23, Richard Henderson wrote:
> The microblaze architecture does not reorder instructions.
> While there is an MBAR wait-for-data-access instruction,
> this concerns synchronizing with DMA.
> 
> This should have been defined when enabling MTTCG.
> 
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Fixes: d449561b130 ("configure: microblaze: Enable mttcg")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/microblaze/cpu.h | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> index 88324d0bc1..b474abcc2a 100644
> --- a/target/microblaze/cpu.h
> +++ b/target/microblaze/cpu.h
> @@ -24,6 +24,9 @@
>   #include "exec/cpu-defs.h"
>   #include "qemu/cpu-float.h"
>   
> +/* MicroBlaze is always in-order. */
> +#define TCG_GUEST_DEFAULT_MO  TCG_MO_ALL

Targets missing such definition:

- cris
- m68k
- nios2
- rx
- sh4
- sparc/64 (!)
- tricore

I expect targets designed for embedded systems
to be in-order for power efficiency.

What about having each target being explicit about that,
having a build failure if TCG_GUEST_DEFAULT_MO is not defined,
instead of the '#ifdef TCG_GUEST_DEFAULT_MO' in accel/tcg/?

Re: [PATCH v3 1/5] target/microblaze: Define TCG_GUEST_DEFAULT_MO

[Richard Henderson]

On 6/20/23 17:41, Philippe Mathieu-Daudé wrote:
> On 19/6/23 16:23, Richard Henderson wrote:
>> The microblaze architecture does not reorder instructions.
>> While there is an MBAR wait-for-data-access instruction,
>> this concerns synchronizing with DMA.
>>
>> This should have been defined when enabling MTTCG.
>>
>> Cc: Alistair Francis <alistair.francis@wdc.com>
>> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> Fixes: d449561b130 ("configure: microblaze: Enable mttcg")
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   target/microblaze/cpu.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
>> index 88324d0bc1..b474abcc2a 100644
>> --- a/target/microblaze/cpu.h
>> +++ b/target/microblaze/cpu.h
>> @@ -24,6 +24,9 @@
>>   #include "exec/cpu-defs.h"
>>   #include "qemu/cpu-float.h"
>> +/* MicroBlaze is always in-order. */
>> +#define TCG_GUEST_DEFAULT_MO  TCG_MO_ALL
> 
> Targets missing such definition:
> 
> - cris
> - m68k
> - nios2
> - rx
> - sh4
> - sparc/64 (!)
> - tricore
> 
> I expect targets designed for embedded systems
> to be in-order for power efficiency.
> 
> What about having each target being explicit about that,
> having a build failure if TCG_GUEST_DEFAULT_MO is not defined,
> instead of the '#ifdef TCG_GUEST_DEFAULT_MO' in accel/tcg/?

I'd be ok with that.


r~

Re: [PATCH v3 1/5] target/microblaze: Define TCG_GUEST_DEFAULT_MO

[Edgar E. Iglesias]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Mon, Jun 19, 2023 at 4:23 PM Richard Henderson <
richard.henderson@linaro.org> wrote:

> The microblaze architecture does not reorder instructions.
> While there is an MBAR wait-for-data-access instruction,
> this concerns synchronizing with DMA.
>
> This should have been defined when enabling MTTCG.


Reviewed-by: Edgar E. Iglesias <edgar@zeroasic.com>

There might be MicroBlaze systems that allow reordering of load vs store
streams but it doesn't seem to be documented and I'm not 100% certain so
this change LGTM!

Thanks,
Edgar



>
> Cc: Alistair Francis <alistair.francis@wdc.com>
> Cc: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Fixes: d449561b130 ("configure: microblaze: Enable mttcg")
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/microblaze/cpu.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/microblaze/cpu.h b/target/microblaze/cpu.h
> index 88324d0bc1..b474abcc2a 100644
> --- a/target/microblaze/cpu.h
> +++ b/target/microblaze/cpu.h
> @@ -24,6 +24,9 @@
>  #include "exec/cpu-defs.h"
>  #include "qemu/cpu-float.h"
>
> +/* MicroBlaze is always in-order. */
> +#define TCG_GUEST_DEFAULT_MO  TCG_MO_ALL
> +
>  typedef struct CPUArchState CPUMBState;
>  #if !defined(CONFIG_USER_ONLY)
>  #include "mmu.h"
> --
> 2.34.1
>
>

[-- Attachment #2: Type: text/html, Size: 2263 bytes --]

Re: [PATCH v3 0/5] tcg: Issue memory barriers for guest memory model

[Richard Henderson]

On 6/19/23 16:23, Richard Henderson wrote:
> v1: https://lore.kernel.org/qemu-devel/20210316220735.2048137-1-richard.henderson@linaro.org/
> v2: https://lore.kernel.org/qemu-devel/20230306015710.1868853-1-richard.henderson@linaro.org/
> 
> Changes for v3:
>    * Update for tcg-built-once.
>    * Require TCG_GUEST_DEFAULT_MO if TARGET_SUPPORTS_MTTCG.
> 
> 
> r~
> 
> 
> Richard Henderson (5):
>    target/microblaze: Define TCG_GUEST_DEFAULT_MO
>    tcg: Do not elide memory barriers for !CF_PARALLEL in system mode
>    tcg: Elide memory barriers implied by the host memory model
>    tcg: Add host memory barriers to cpu_ldst.h interfaces
>    accel/tcg: Remove check_tcg_memory_orders_compatible
> 
>   accel/tcg/internal.h    | 34 ++++++++++++++++++++++++++++++++++
>   target/microblaze/cpu.h |  3 +++
>   accel/tcg/cputlb.c      | 10 ++++++++++
>   accel/tcg/tcg-all.c     | 39 ++++++++++-----------------------------
>   accel/tcg/user-exec.c   | 10 ++++++++++
>   tcg/tcg-op.c            | 20 ++++++++++++++++++--
>   6 files changed, 85 insertions(+), 31 deletions(-)
> 

Applied to tcg-next.


r~