[Qemu-devel] [PATCH v2 0/6] target-mips: support MTTCG feature

[Qemu-devel] [PATCH v2 0/6] target-mips: support MTTCG feature

[Aleksandar Markovic]

From: Aleksandar Markovic <aleksandar.markovic@mips.com>

v1->v2:

  - patches #3 and #4 are squashed into one to avoid bisect breaking
  - improved locking features in patch #5 (formerly #6)
  - commit messages reviewed and improved
  - rebased to the latest code


This series introduces MTTCG feature for MIPS targets by adding all
missing bits and pieces, and formally enabling corresponding QEMU
builds to support such configurations.

PATCH ORGANIZATION
==================

The organization of patches is as follows:

  - patches 1 and 2 deal with MIPS' LL/SC instruction emulation
    improvements related to MTTCG. They are based on a previously
    sent patch series by Leon Alrae (this is the last version, v3):
    http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg06870.html

  - patches 3, 4, 5, and 6 deal with locking/synchronization issues
    that surfaced while introducing MTTCG for MIPS. Similar sets of
    patches have been already integrated for some other platforms
    (arm, intel, ppc, sparc).

  - patch 7 just enables QEMU build system to support MTTCG feature
    for MIPS targets.

PERFORMANCE TESTING
===================

Performance testing was performed using atomic_add-bench test program
that tests LL/SC-related functionality in multithread environment. The
observed performance gain was significant.

For the sake of comparison, test case organization mimics the one from
a previously sent patch set:

target-arm: emulate aarch64's LL/SC using cmpxchg helpers
https://lists.gnu.org/archive/html/qemu-devel/2016-10/msg06653.html

-----------------------------------------------------------------------

          atomic_add-bench: 1000000 ops/thread, [0,1] range
                                               
throughput                                  M - MTTCG     N - no MTTCG

 50 +---------+---------+---------+---------+---------+---------+----+
    |                                                                |
    |M                                                               |
 40 +.                                                               +
    |.                                                               |
    |.                                                               |
 30 +.                                                               +
    |.                                                               |
    |.                                                               |
 20 +.                                                               +
    | M                                                              |
    | .                                                              |
 10 +  .M...M.......M.......M.......M.......M.......M.......M.......M+
    |N                                                               |
    | N.N...N.......N.......N.......N.......N.......N.......N.......N|
  0 +---------+---------+---------+---------+---------+---------+----+
    0         10        20        30        40        50        60

                            number of threads

-----------------------------------------------------------------------

          atomic_add-bench: 1000000 ops/thread, [0,2] range
                                               
throughput                                  M - MTTCG     N - no MTTCG

 50 +---------+---------+---------+---------+---------+---------+----+
    |                                                                |
    |M                                                               |
 40 +.                                                               +
    |.                                                               |
    |.                                                               |
 30 + .                                                              +
    | M                                                              |
    | .                                                              |
 20 +  .M...M.......M.......M.......M.......M.......M.......M.......M+
    |                                                                |
    |                                                                |
 10 +                                                                +
    |N                                                               |
    | N.N...N.......N.......N.......N.......N.......N.......N.......N|
  0 +---------+---------+---------+---------+---------+---------+----+
    0         10        20        30        40        50        60

                            number of threads

-----------------------------------------------------------------------

          atomic_add-bench: 1000000 ops/thread, [0,1] range
                                               
throughput                                  M - MTTCG     N - no MTTCG

150 +---------+---------+---------+---------+---------+---------+----+
    |                                                                |
    |                                            ...M...        ....M|
120 +                   ....M.......M........M...       ....M...     +
    |           ....M...                                             |
    |     ..M...                                                     |
 90 +    .                                                           +
    |  .M                                                            |
    | .                                                              |
 60 + M                                                              +
    |.                                                               |
    |M                                                               |
 30 +                                                                +
    |                                                                |
    |NN.N...N.......N.......N.......N.......N.......N.......N.......N|
  0 +---------+---------+---------+---------+---------+---------+----+
    0         10        20        30        40        50        60

                            number of threads

-----------------------------------------------------------------------

          atomic_add-bench: 1000000 ops/thread, [0,2] range
                                               
throughput                                  M - MTTCG     N - no MTTCG

150 +---------+---------+---------+---------+---------+---------+----+
    |                                            ...M.......M.......M|
    |                           ....M...       ..                    |
120 +           ....M.......M...        ....M..                      +
    |     ..M...                                                     |
    |   M.                                                           |
 90 +  .                                                             +
    | .                                                              |
    | .                                                              |
 60 + M                                                              +
    |.                                                               |
    |M                                                               |
 30 +                                                                +
    |                                                                |
    |NN.N...N.......N.......N.......N.......N.......N.......N.......N|
  0 +---------+---------+---------+---------+---------+---------+----+
    0         10        20        30        40        50        60

                            number of threads

-----------------------------------------------------------------------

Numerical data:

Ops
Range-->      1               2              128            1024

# of     no              no              no              no
 thr.    MTTCG  MTTCG    MTTCG  MTTCG    MTTCG  MTTCG    MTTCG  MTTCG

  1      4.95   42.61    4.94   42.27    4.89   42.24    4.85   41.81
  2      1.23   18.41    1.29   25.71    1.33   57.41    1.36   60.34
  4      0.46   11.99    0.48   19.69    0.53   78.98    0.50   95.39
  8      0.18    9.59    0.18   19.11    0.19  104.66    0.20  112.66
 16      0.11   11.19    0.12   19.12    0.12  108.29    0.13  121.90
 24      0.10   10.18    0.09   19.14    0.11  115.53    0.10  127.40
 32      0.11   11.15    0.12   19.36    0.09  120.60    0.10  131.60
 40      0.08   10.47    0.11   20.88    0.12  124.59    0.10  124.74
 48      0.12   11.78    0.13   20.09    0.11  129.24    0.11  137.19
 56      0.14   12.40    0.13   22.13    0.15  124.16    0.15  138.52
 64      0.14   11.08    0.20   21.08    0.18  131.28    0.19  144.84

-----------------------------------------------------------------------

Graphical representation:

 https://i.imgur.com/OtNLpVX.png

-----------------------------------------------------------------------

REGRESSION TESTING
==================

Regression testing was also performed. The main test bed for regression
testing was LTP test suite executed on QEMU-emulated Debian mips64
system.

Some LTP tests (getrusage04, copy_file_range01) that used to fail for
non-MTTCG systems, pass for MTTCG-enabled systems. Also, some LTP tests
(nanosleep01, poll02, pselect01) intermittently fail on both non-MTTCG
and MTTCG configurations, and therefore do not represent valid
regressions.

Emulation by itself did not appear to have any problems while executing
LTP test suite.

QEMU user mode MTTCG-enabled emulation was also tested to some extent.

Aleksandar Markovic (2):
  hw/mips_int: hold BQL for all interrupt requests
  target/mips: introduce MTTCG-enabled builds

Goran Ferenc (1):
  target/mips: hold BQL in mips_vpe_wake()

Leon Alrae (2):
  target/mips: compare virtual addresses in LL/SC sequence
  target/mips: reimplement SC instruction emulation and use cmpxchg

Miodrag Dinic (1):
  hw/mips_cpc: kick a VP when putting it into Run state

 configure               |   3 ++
 hw/mips/mips_int.c      |  12 +++++
 hw/misc/mips_cpc.c      |  17 ++++++-
 linux-user/main.c       |  58 ------------------------
 target/mips/cpu.h       |   9 ++--
 target/mips/helper.c    |   6 +--
 target/mips/helper.h    |   2 -
 target/mips/machine.c   |   7 +--
 target/mips/op_helper.c |  74 +++++++++---------------------
 target/mips/translate.c | 118 ++++++++++++++++--------------------------------
 10 files changed, 100 insertions(+), 206 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/6] target/mips: compare virtual addresses in LL/SC sequence

[Aleksandar Markovic]

From: Leon Alrae <leon.alrae@imgtec.com>

Do only virtual addresses comaprisons in LL/SC sequence emulations.

Until this patch, physical addresses had been compared in SC part of
LL/SC sequence, even though such comparisons could be avoided. Getting
rid of them allows throwing away SC helpers and having common SC
implementations in user and system mode, avoiding the need for two
separate implementations selected by #ifdef CONFIG_USER_ONLY.

Correct guest software should not rely on LL/SC if they accesses the
same physical address via different virtual addresses or if page
mapping gets changed between LL/SC due to manipulating TLB entries.
MIPS Instruction Set Manual clearly says that an RMW sequence must
use the same address in the LL and SC (virtual address, physical
address, cacheability and coherency attributes must be identical).
Otherwise, the result of the SC is not predictable. This patch takes
advantage of this fact and removes the virtual->physical address
translation from SC helper.

lladdr served as Coprocessor 0 LLAddr register which captures physical
address of the most recent LL instruction, and also lladdr was used
for comparison with following SC physical address. This patch changes
the meaning of lladdr - now it will only keep the virtual address of
the most recent LL. Additionally, CP0_LLAddr field is introduced which
is the actual Coperocessor 0 LLAddr register that guest can access.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 target/mips/cpu.h       |  3 ++-
 target/mips/machine.c   |  7 ++++---
 target/mips/op_helper.c | 29 +++++++++++++++++------------
 target/mips/translate.c |  4 ++--
 4 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 7f8ba5f..57ca861 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -480,10 +480,11 @@ struct CPUMIPSState {
 #define CP0C5_NFExists   0
     int32_t CP0_Config6;
     int32_t CP0_Config7;
+    uint64_t CP0_LLAddr;
     uint64_t CP0_MAAR[MIPS_MAAR_MAX];
     int32_t CP0_MAARI;
     /* XXX: Maybe make LLAddr per-TC? */
-    uint64_t lladdr;
+    target_ulong lladdr; /* LL virtual address compared against SC */
     target_ulong llval;
     target_ulong llnewval;
     target_ulong llreg;
diff --git a/target/mips/machine.c b/target/mips/machine.c
index 20100d5..155170c 100644
--- a/target/mips/machine.c
+++ b/target/mips/machine.c
@@ -212,8 +212,8 @@ const VMStateDescription vmstate_tlb = {
 
 const VMStateDescription vmstate_mips_cpu = {
     .name = "cpu",
-    .version_id = 10,
-    .minimum_version_id = 10,
+    .version_id = 11,
+    .minimum_version_id = 11,
     .post_load = cpu_post_load,
     .fields = (VMStateField[]) {
         /* Active TC */
@@ -283,9 +283,10 @@ const VMStateDescription vmstate_mips_cpu = {
         VMSTATE_INT32(env.CP0_Config3, MIPSCPU),
         VMSTATE_INT32(env.CP0_Config6, MIPSCPU),
         VMSTATE_INT32(env.CP0_Config7, MIPSCPU),
+        VMSTATE_UINT64(env.CP0_LLAddr, MIPSCPU),
         VMSTATE_UINT64_ARRAY(env.CP0_MAAR, MIPSCPU, MIPS_MAAR_MAX),
         VMSTATE_INT32(env.CP0_MAARI, MIPSCPU),
-        VMSTATE_UINT64(env.lladdr, MIPSCPU),
+        VMSTATE_UINTTL(env.lladdr, MIPSCPU),
         VMSTATE_UINTTL_ARRAY(env.CP0_WatchLo, MIPSCPU, 8),
         VMSTATE_INT32_ARRAY(env.CP0_WatchHi, MIPSCPU, 8),
         VMSTATE_UINTTL(env.CP0_XContext, MIPSCPU),
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 798cdad..cc5cd4e 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -255,15 +255,15 @@ static inline hwaddr do_translate_address(CPUMIPSState *env,
                                                       target_ulong address,
                                                       int rw, uintptr_t retaddr)
 {
-    hwaddr lladdr;
+    hwaddr paddr;
     CPUState *cs = CPU(mips_env_get_cpu(env));
 
-    lladdr = cpu_mips_translate_address(env, address, rw);
+    paddr = cpu_mips_translate_address(env, address, rw);
 
-    if (lladdr == -1LL) {
+    if (paddr == -1LL) {
         cpu_loop_exit_restore(cs, retaddr);
     } else {
-        return lladdr;
+        return paddr;
     }
 }
 
@@ -274,7 +274,8 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx)  \
         env->CP0_BadVAddr = arg;                                              \
         do_raise_exception(env, EXCP_AdEL, GETPC());                          \
     }                                                                         \
-    env->lladdr = do_translate_address(env, arg, 0, GETPC());                 \
+    env->CP0_LLAddr = do_translate_address(env, arg, 0, GETPC());             \
+    env->lladdr = arg;                                                        \
     env->llval = do_##insn(env, arg, mem_idx, GETPC());                       \
     return env->llval;                                                        \
 }
@@ -294,7 +295,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg1,              \
         env->CP0_BadVAddr = arg2;                                             \
         do_raise_exception(env, EXCP_AdES, GETPC());                          \
     }                                                                         \
-    if (do_translate_address(env, arg2, 1, GETPC()) == env->lladdr) {         \
+    if (arg2 == env->lladdr) {                                                \
         tmp = do_##ld_insn(env, arg2, mem_idx, GETPC());                      \
         if (tmp == env->llval) {                                              \
             do_##st_insn(env, arg2, arg1, mem_idx, GETPC());                  \
@@ -873,7 +874,7 @@ target_ulong helper_mftc0_status(CPUMIPSState *env)
 
 target_ulong helper_mfc0_lladdr(CPUMIPSState *env)
 {
-    return (int32_t)(env->lladdr >> env->CP0_LLAddr_shift);
+    return (int32_t)(env->CP0_LLAddr >> env->CP0_LLAddr_shift);
 }
 
 target_ulong helper_mfc0_maar(CPUMIPSState *env)
@@ -949,7 +950,7 @@ target_ulong helper_dmfc0_tcschefback(CPUMIPSState *env)
 
 target_ulong helper_dmfc0_lladdr(CPUMIPSState *env)
 {
-    return env->lladdr >> env->CP0_LLAddr_shift;
+    return env->CP0_LLAddr >> env->CP0_LLAddr_shift;
 }
 
 target_ulong helper_dmfc0_maar(CPUMIPSState *env)
@@ -1177,7 +1178,8 @@ void helper_mtc0_tcrestart(CPUMIPSState *env, target_ulong arg1)
 {
     env->active_tc.PC = arg1;
     env->active_tc.CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
-    env->lladdr = 0ULL;
+    env->CP0_LLAddr = 0;
+    env->lladdr = 0;
     /* MIPS16 not implemented. */
 }
 
@@ -1189,12 +1191,14 @@ void helper_mttc0_tcrestart(CPUMIPSState *env, target_ulong arg1)
     if (other_tc == other->current_tc) {
         other->active_tc.PC = arg1;
         other->active_tc.CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
-        other->lladdr = 0ULL;
+        other->CP0_LLAddr = 0;
+        other->lladdr = 0;
         /* MIPS16 not implemented. */
     } else {
         other->tcs[other_tc].PC = arg1;
         other->tcs[other_tc].CP0_TCStatus &= ~(1 << CP0TCSt_TDS);
-        other->lladdr = 0ULL;
+        other->CP0_LLAddr = 0;
+        other->lladdr = 0;
         /* MIPS16 not implemented. */
     }
 }
@@ -1620,7 +1624,7 @@ void helper_mtc0_lladdr(CPUMIPSState *env, target_ulong arg1)
 {
     target_long mask = env->CP0_LLAddr_rw_bitmask;
     arg1 = arg1 << env->CP0_LLAddr_shift;
-    env->lladdr = (env->lladdr & ~mask) | (arg1 & mask);
+    env->CP0_LLAddr = (env->CP0_LLAddr & ~mask) | (arg1 & mask);
 }
 
 #define MTC0_MAAR_MASK(env) \
@@ -2318,6 +2322,7 @@ static inline void exception_return(CPUMIPSState *env)
 void helper_eret(CPUMIPSState *env)
 {
     exception_return(env);
+    env->CP0_LLAddr = 1;
     env->lladdr = 1;
 }
 
diff --git a/target/mips/translate.c b/target/mips/translate.c
index d05ee67..c9104a7 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -4913,7 +4913,7 @@ static void gen_mfhc0(DisasContext *ctx, TCGv arg, int reg, int sel)
     case 17:
         switch (sel) {
         case 0:
-            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, lladdr),
+            gen_mfhc0_load64(arg, offsetof(CPUMIPSState, CP0_LLAddr),
                              ctx->CP0_LLAddr_shift);
             rn = "LLAddr";
             break;
@@ -20440,7 +20440,7 @@ void mips_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
                 env->CP0_Status, env->CP0_Cause, env->CP0_EPC);
     cpu_fprintf(f, "    Config0 0x%08x Config1 0x%08x LLAddr 0x%016"
                 PRIx64 "\n",
-                env->CP0_Config0, env->CP0_Config1, env->lladdr);
+                env->CP0_Config0, env->CP0_Config1, env->CP0_LLAddr);
     cpu_fprintf(f, "    Config2 0x%08x Config3 0x%08x\n",
                 env->CP0_Config2, env->CP0_Config3);
     cpu_fprintf(f, "    Config4 0x%08x Config5 0x%08x\n",
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/6] target/mips: reimplement SC instruction emulation and use cmpxchg

[Aleksandar Markovic]

From: Leon Alrae <lean.alrae@imgtec.com>

Completely rewrite conditional stores handling. Use cmpxchg.

This eliminates need for separate implementations of SC instruction
emulation for user and system emulation.

Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
---
 linux-user/main.c       |  58 ------------------------
 target/mips/cpu.h       |   4 --
 target/mips/helper.c    |   6 +--
 target/mips/helper.h    |   2 -
 target/mips/op_helper.c |  25 -----------
 target/mips/translate.c | 114 ++++++++++++++++--------------------------------
 6 files changed, 39 insertions(+), 170 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index fd79006..cef076f 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -2160,55 +2160,6 @@ static const uint8_t mips_syscall_args[] = {
 #  undef MIPS_SYS
 # endif /* O32 */
 
-static int do_store_exclusive(CPUMIPSState *env)
-{
-    target_ulong addr;
-    target_ulong page_addr;
-    target_ulong val;
-    int flags;
-    int segv = 0;
-    int reg;
-    int d;
-
-    addr = env->lladdr;
-    page_addr = addr & TARGET_PAGE_MASK;
-    start_exclusive();
-    mmap_lock();
-    flags = page_get_flags(page_addr);
-    if ((flags & PAGE_READ) == 0) {
-        segv = 1;
-    } else {
-        reg = env->llreg & 0x1f;
-        d = (env->llreg & 0x20) != 0;
-        if (d) {
-            segv = get_user_s64(val, addr);
-        } else {
-            segv = get_user_s32(val, addr);
-        }
-        if (!segv) {
-            if (val != env->llval) {
-                env->active_tc.gpr[reg] = 0;
-            } else {
-                if (d) {
-                    segv = put_user_u64(env->llnewval, addr);
-                } else {
-                    segv = put_user_u32(env->llnewval, addr);
-                }
-                if (!segv) {
-                    env->active_tc.gpr[reg] = 1;
-                }
-            }
-        }
-    }
-    env->lladdr = -1;
-    if (!segv) {
-        env->active_tc.PC += 4;
-    }
-    mmap_unlock();
-    end_exclusive();
-    return segv;
-}
-
 /* Break codes */
 enum {
     BRK_OVERFLOW = 6,
@@ -2358,15 +2309,6 @@ done_syscall:
                   }
             }
             break;
-        case EXCP_SC:
-            if (do_store_exclusive(env)) {
-                info.si_signo = TARGET_SIGSEGV;
-                info.si_errno = 0;
-                info.si_code = TARGET_SEGV_MAPERR;
-                info._sifields._sigfault._addr = env->active_tc.PC;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
-            }
-            break;
         case EXCP_DSPDIS:
             info.si_signo = TARGET_SIGILL;
             info.si_errno = 0;
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 57ca861..3fa85b0 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -486,8 +486,6 @@ struct CPUMIPSState {
     /* XXX: Maybe make LLAddr per-TC? */
     target_ulong lladdr; /* LL virtual address compared against SC */
     target_ulong llval;
-    target_ulong llnewval;
-    target_ulong llreg;
     uint64_t CP0_LLAddr_rw_bitmask;
     int CP0_LLAddr_shift;
     target_ulong CP0_WatchLo[8];
@@ -727,8 +725,6 @@ enum {
 
     EXCP_LAST = EXCP_TLBRI,
 };
-/* Dummy exception for conditional stores.  */
-#define EXCP_SC 0x100
 
 /*
  * This is an internally generated WAKE request line.
diff --git a/target/mips/helper.c b/target/mips/helper.c
index 8cf91ce..5002eb2 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -1084,10 +1084,8 @@ void QEMU_NORETURN do_raise_exception_err(CPUMIPSState *env,
 {
     CPUState *cs = CPU(mips_env_get_cpu(env));
 
-    if (exception < EXCP_SC) {
-        qemu_log_mask(CPU_LOG_INT, "%s: %d %d\n",
-                      __func__, exception, error_code);
-    }
+    qemu_log_mask(CPU_LOG_INT, "%s: %d %d\n",
+                  __func__, exception, error_code);
     cs->exception_index = exception;
     env->error_code = error_code;
 
diff --git a/target/mips/helper.h b/target/mips/helper.h
index 5f49234..04c36e9 100644
--- a/target/mips/helper.h
+++ b/target/mips/helper.h
@@ -13,10 +13,8 @@ DEF_HELPER_4(swr, void, env, tl, tl, int)
 
 #ifndef CONFIG_USER_ONLY
 DEF_HELPER_3(ll, tl, env, tl, int)
-DEF_HELPER_4(sc, tl, env, tl, tl, int)
 #ifdef TARGET_MIPS64
 DEF_HELPER_3(lld, tl, env, tl, int)
-DEF_HELPER_4(scd, tl, env, tl, tl, int)
 #endif
 #endif
 
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index cc5cd4e..44a9b06 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -284,31 +284,6 @@ HELPER_LD_ATOMIC(ll, lw, 0x3)
 HELPER_LD_ATOMIC(lld, ld, 0x7)
 #endif
 #undef HELPER_LD_ATOMIC
-
-#define HELPER_ST_ATOMIC(name, ld_insn, st_insn, almask)                      \
-target_ulong helper_##name(CPUMIPSState *env, target_ulong arg1,              \
-                           target_ulong arg2, int mem_idx)                    \
-{                                                                             \
-    target_long tmp;                                                          \
-                                                                              \
-    if (arg2 & almask) {                                                      \
-        env->CP0_BadVAddr = arg2;                                             \
-        do_raise_exception(env, EXCP_AdES, GETPC());                          \
-    }                                                                         \
-    if (arg2 == env->lladdr) {                                                \
-        tmp = do_##ld_insn(env, arg2, mem_idx, GETPC());                      \
-        if (tmp == env->llval) {                                              \
-            do_##st_insn(env, arg2, arg1, mem_idx, GETPC());                  \
-            return 1;                                                         \
-        }                                                                     \
-    }                                                                         \
-    return 0;                                                                 \
-}
-HELPER_ST_ATOMIC(sc, lw, sw, 0x3)
-#ifdef TARGET_MIPS64
-HELPER_ST_ATOMIC(scd, ld, sd, 0x7)
-#endif
-#undef HELPER_ST_ATOMIC
 #endif
 
 #ifdef TARGET_WORDS_BIGENDIAN
diff --git a/target/mips/translate.c b/target/mips/translate.c
index c9104a7..f851d41 100644
--- a/target/mips/translate.c
+++ b/target/mips/translate.c
@@ -1383,6 +1383,7 @@ static TCGv_i32 hflags;
 static TCGv_i32 fpu_fcr0, fpu_fcr31;
 static TCGv_i64 fpu_f64[32];
 static TCGv_i64 msa_wr_d[64];
+static TCGv cpu_lladdr, cpu_llval;
 
 #include "exec/gen-icount.h"
 
@@ -2073,48 +2074,6 @@ OP_LD_ATOMIC(lld,ld64);
 #endif
 #undef OP_LD_ATOMIC
 
-#ifdef CONFIG_USER_ONLY
-#define OP_ST_ATOMIC(insn,fname,ldname,almask)                               \
-static inline void op_st_##insn(TCGv arg1, TCGv arg2, int rt, int mem_idx,   \
-                                DisasContext *ctx)                           \
-{                                                                            \
-    TCGv t0 = tcg_temp_new();                                                \
-    TCGLabel *l1 = gen_new_label();                                          \
-    TCGLabel *l2 = gen_new_label();                                          \
-                                                                             \
-    tcg_gen_andi_tl(t0, arg2, almask);                                       \
-    tcg_gen_brcondi_tl(TCG_COND_EQ, t0, 0, l1);                              \
-    tcg_gen_st_tl(arg2, cpu_env, offsetof(CPUMIPSState, CP0_BadVAddr));          \
-    generate_exception(ctx, EXCP_AdES);                                      \
-    gen_set_label(l1);                                                       \
-    tcg_gen_ld_tl(t0, cpu_env, offsetof(CPUMIPSState, lladdr));                  \
-    tcg_gen_brcond_tl(TCG_COND_NE, arg2, t0, l2);                            \
-    tcg_gen_movi_tl(t0, rt | ((almask << 3) & 0x20));                        \
-    tcg_gen_st_tl(t0, cpu_env, offsetof(CPUMIPSState, llreg));                   \
-    tcg_gen_st_tl(arg1, cpu_env, offsetof(CPUMIPSState, llnewval));              \
-    generate_exception_end(ctx, EXCP_SC);                                    \
-    gen_set_label(l2);                                                       \
-    tcg_gen_movi_tl(t0, 0);                                                  \
-    gen_store_gpr(t0, rt);                                                   \
-    tcg_temp_free(t0);                                                       \
-}
-#else
-#define OP_ST_ATOMIC(insn,fname,ldname,almask)                               \
-static inline void op_st_##insn(TCGv arg1, TCGv arg2, int rt, int mem_idx,   \
-                                DisasContext *ctx)                           \
-{                                                                            \
-    TCGv t0 = tcg_temp_new();                                                \
-    gen_helper_1e2i(insn, t0, arg1, arg2, mem_idx);                          \
-    gen_store_gpr(t0, rt);                                                   \
-    tcg_temp_free(t0);                                                       \
-}
-#endif
-OP_ST_ATOMIC(sc,st32,ld32s,0x3);
-#if defined(TARGET_MIPS64)
-OP_ST_ATOMIC(scd,st64,ld64,0x7);
-#endif
-#undef OP_ST_ATOMIC
-
 static void gen_base_offset_addr (DisasContext *ctx, TCGv addr,
                                   int base, int16_t offset)
 {
@@ -2401,37 +2360,34 @@ static void gen_st (DisasContext *ctx, uint32_t opc, int rt,
 
 
 /* Store conditional */
-static void gen_st_cond (DisasContext *ctx, uint32_t opc, int rt,
-                         int base, int16_t offset)
+static void gen_st_cond(DisasContext *ctx, int rt, int base, int offset,
+                        TCGMemOp tcg_mo)
 {
-    TCGv t0, t1;
-    int mem_idx = ctx->mem_idx;
+    TCGv addr, t0, val;
+    TCGLabel *l1 = gen_new_label();
+    TCGLabel *done = gen_new_label();
+
+     t0 = tcg_temp_new();
+    addr = tcg_temp_new();
+    /* compare the address against that of the preceeding LL */
+    gen_base_offset_addr(ctx, addr, base, offset);
+    tcg_gen_brcond_tl(TCG_COND_EQ, addr, cpu_lladdr, l1);
+    tcg_temp_free(addr);
+    tcg_gen_movi_tl(t0, 0);
+    gen_store_gpr(t0, rt);
+    tcg_gen_br(done);
 
-#ifdef CONFIG_USER_ONLY
-    t0 = tcg_temp_local_new();
-    t1 = tcg_temp_local_new();
-#else
-    t0 = tcg_temp_new();
-    t1 = tcg_temp_new();
-#endif
-    gen_base_offset_addr(ctx, t0, base, offset);
-    gen_load_gpr(t1, rt);
-    switch (opc) {
-#if defined(TARGET_MIPS64)
-    case OPC_SCD:
-    case R6_OPC_SCD:
-        op_st_scd(t1, t0, rt, mem_idx, ctx);
-        break;
-#endif
-    case OPC_SCE:
-        mem_idx = MIPS_HFLAG_UM;
-        /* fall through */
-    case OPC_SC:
-    case R6_OPC_SC:
-        op_st_sc(t1, t0, rt, mem_idx, ctx);
-        break;
-    }
-    tcg_temp_free(t1);
+    gen_set_label(l1);
+    /* generate cmpxchg */
+    val = tcg_temp_new();
+    gen_load_gpr(val, rt);
+    tcg_gen_atomic_cmpxchg_tl(t0, cpu_lladdr, cpu_llval, val,
+                              ctx->mem_idx, tcg_mo);
+    tcg_gen_setcond_tl(TCG_COND_EQ, t0, t0, cpu_llval);
+    gen_store_gpr(t0, rt);
+    tcg_temp_free(val);
+
+    gen_set_label(done);
     tcg_temp_free(t0);
 }
 
@@ -14889,13 +14845,13 @@ static void decode_micromips32_opc(CPUMIPSState *env, DisasContext *ctx)
             gen_st(ctx, mips32_op, rt, rs, offset);
             break;
         case SC:
-            gen_st_cond(ctx, OPC_SC, rt, rs, offset);
+            gen_st_cond(ctx, rt, rs, offset, MO_TESL);
             break;
 #if defined(TARGET_MIPS64)
         case SCD:
             check_insn(ctx, ISA_MIPS3);
             check_mips_64(ctx);
-            gen_st_cond(ctx, OPC_SCD, rt, rs, offset);
+            gen_st_cond(ctx, rt, rs, offset, MO_TEQ);
             break;
 #endif
         case LD_EVA:
@@ -17695,7 +17651,7 @@ static void decode_opc_special3_r6(CPUMIPSState *env, DisasContext *ctx)
         }
         break;
     case R6_OPC_SC:
-        gen_st_cond(ctx, op1, rt, rs, imm);
+        gen_st_cond(ctx, rt, rs, imm, MO_TESL);
         break;
     case R6_OPC_LL:
         gen_ld(ctx, op1, rt, rs, imm);
@@ -17719,7 +17675,7 @@ static void decode_opc_special3_r6(CPUMIPSState *env, DisasContext *ctx)
         break;
 #if defined(TARGET_MIPS64)
     case R6_OPC_SCD:
-        gen_st_cond(ctx, op1, rt, rs, imm);
+        gen_st_cond(ctx, rt, rs, imm, MO_TEQ);
         break;
     case R6_OPC_LLD:
         gen_ld(ctx, op1, rt, rs, imm);
@@ -19844,7 +19800,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
     case OPC_SC:
         check_insn(ctx, ISA_MIPS2);
          check_insn_opc_removed(ctx, ISA_MIPS32R6);
-         gen_st_cond(ctx, op, rt, rs, imm);
+         gen_st_cond(ctx, rt, rs, imm, MO_TESL);
          break;
     case OPC_CACHE:
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
@@ -20130,7 +20086,7 @@ static void decode_opc(CPUMIPSState *env, DisasContext *ctx)
         check_insn_opc_removed(ctx, ISA_MIPS32R6);
         check_insn(ctx, ISA_MIPS3);
         check_mips_64(ctx);
-        gen_st_cond(ctx, op, rt, rs, imm);
+        gen_st_cond(ctx, rt, rs, imm, MO_TEQ);
         break;
     case OPC_BNVC: /* OPC_BNEZALC, OPC_BNEC, OPC_DADDI */
         if (ctx->insn_flags & ISA_MIPS32R6) {
@@ -20497,6 +20453,10 @@ void mips_tcg_init(void)
     fpu_fcr31 = tcg_global_mem_new_i32(cpu_env,
                                        offsetof(CPUMIPSState, active_fpu.fcr31),
                                        "fcr31");
+    cpu_lladdr = tcg_global_mem_new(cpu_env, offsetof(CPUMIPSState, lladdr),
+                                    "lladdr");
+    cpu_llval = tcg_global_mem_new(cpu_env, offsetof(CPUMIPSState, llval),
+                                   "llval");
 }
 
 #include "translate_init.c"
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/6] target/mips: reimplement SC instruction emulation and use cmpxchg

[Emilio G. Cota]

On Thu, Feb 22, 2018 at 19:31:39 +0100, Aleksandar Markovic wrote:
> From: Leon Alrae <lean.alrae@imgtec.com>
> 
> Completely rewrite conditional stores handling. Use cmpxchg.
> 
> This eliminates need for separate implementations of SC instruction
> emulation for user and system emulation.
> 
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Tested-by: Emilio G. Cota <cota@braap.org>

Got some stats with qht-bench on mipsel-user -- note that I think
you'd get more meaningful numbers with qht-bench than with
atomic_add-bench, since the latter isn't representative of a useful
workload--rather it's a pathological case unless that doesn't
scale at all unless you increase the range quite a bit.

Also, you might want to add perf numbers to the appropriate commit
log, otherwise they'll be absent from the commit history.

Here are some numbers I just gathered for qht-bench, feel
free to include them in this commit's log:

  mipsel-linux-user tests/qht-bench -d 5 -n 1 -u 20 -g 10000
             Host: AMD Opteron(tm) Processor 6376           
                                                            
  120 +-+-+---------+----------+---------+---------#D-+-+   
      |   +         +          +         +       ## +   |   
      | master ***B***                         ##       |   
  100 +MTTCGv2 ###D###                      ###       +-+   
      |                                   ##            |   
      |                                 ##              |   
   80 +-+                            ###              +-+   
      |                            ##                   |   
      |                        # ##                     |   
   60 +-+                     #D#                     +-+   
      |                    ###                          |   
      |                  ##                             |   
   40 +-+             ###                             +-+   
      |            #D#                                  |   
      |          ##                                     |   
   20 +-+      D#                                     +-+   
      |      ##                                         |   
      |   +##       +          +         +          +   |   
    0 +-+-B****B****B**********B********************B-+-+   
          1         16      Threads      48         64      

  png: https://imgur.com/hTwhw70

Thanks for doing this work!

		Emilio

Re: [Qemu-devel] [PATCH v2 2/6] target/mips: reimplement SC instruction emulation and use cmpxchg

[Emilio G. Cota]

On Thu, Feb 22, 2018 at 17:33:27 -0500, Emilio G. Cota wrote:
> Here are some numbers I just gathered for qht-bench, feel
> free to include them in this commit's log:
> 
>   mipsel-linux-user tests/qht-bench -d 5 -n 1 -u 20 -g 10000
>              Host: AMD Opteron(tm) Processor 6376           

Obviously "-n $THREADS", not "-n 1".
Updated png: https://imgur.com/aI9Hvb6

		E.

[Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

[Aleksandar Markovic]

From: Aleksandar Markovic <aleksandar.markovic@mips.com>

Make sure BQL is held for all interrupt requests.

For MTTCG-enabled configurations, handling soft and hard interrupts
between vCPUs must be properly locked. By acquiring BQL, make sure
all paths triggering an IRQ are synchronized.

Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 hw/mips/mips_int.c      | 12 ++++++++++++
 target/mips/op_helper.c | 21 +++------------------
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 48192d2..5ddeb15 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -21,6 +21,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "hw/hw.h"
 #include "hw/mips/cpudevs.h"
 #include "cpu.h"
@@ -32,10 +33,17 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
     MIPSCPU *cpu = opaque;
     CPUMIPSState *env = &cpu->env;
     CPUState *cs = CPU(cpu);
+    bool locked = false;
 
     if (irq < 0 || irq > 7)
         return;
 
+    /* Make sure locking works even if BQL is already held by the caller */
+    if (!qemu_mutex_iothread_locked()) {
+        locked = true;
+        qemu_mutex_lock_iothread();
+    }
+
     if (level) {
         env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
 
@@ -56,6 +64,10 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
     } else {
         cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
     }
+
+    if (locked) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 44a9b06..6bd8e59 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -17,7 +17,6 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
-#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "internal.h"
 #include "qemu/host-utils.h"
@@ -809,11 +808,7 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env)
 
 target_ulong helper_mfc0_count(CPUMIPSState *env)
 {
-    int32_t count;
-    qemu_mutex_lock_iothread();
-    count = (int32_t) cpu_mips_get_count(env);
-    qemu_mutex_unlock_iothread();
-    return count;
+    return (int32_t)cpu_mips_get_count(env);
 }
 
 target_ulong helper_mftc0_entryhi(CPUMIPSState *env)
@@ -1388,9 +1383,7 @@ void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1)
 {
-    qemu_mutex_lock_iothread();
     cpu_mips_store_count(env, arg1);
-    qemu_mutex_unlock_iothread();
 }
 
 void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
@@ -1439,9 +1432,7 @@ void helper_mttc0_entryhi(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_compare(CPUMIPSState *env, target_ulong arg1)
 {
-    qemu_mutex_lock_iothread();
     cpu_mips_store_compare(env, arg1);
-    qemu_mutex_unlock_iothread();
 }
 
 void helper_mtc0_status(CPUMIPSState *env, target_ulong arg1)
@@ -1495,9 +1486,7 @@ void helper_mtc0_srsctl(CPUMIPSState *env, target_ulong arg1)
 
 void helper_mtc0_cause(CPUMIPSState *env, target_ulong arg1)
 {
-    qemu_mutex_lock_iothread();
     cpu_mips_store_cause(env, arg1);
-    qemu_mutex_unlock_iothread();
 }
 
 void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1)
@@ -2339,16 +2328,12 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
 
 target_ulong helper_rdhwr_cc(CPUMIPSState *env)
 {
-    int32_t count;
     check_hwrena(env, 2, GETPC());
 #ifdef CONFIG_USER_ONLY
-    count = env->CP0_Count;
+    return env->CP0_Count;
 #else
-    qemu_mutex_lock_iothread();
-    count = (int32_t)cpu_mips_get_count(env);
-    qemu_mutex_unlock_iothread();
+    return (int32_t)cpu_mips_get_count(env);
 #endif
-    return count;
 }
 
 target_ulong helper_rdhwr_ccres(CPUMIPSState *env)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

[Alex Bennée]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Aleksandar Markovic <aleksandar.markovic@mips.com>
>
> Make sure BQL is held for all interrupt requests.
>
> For MTTCG-enabled configurations, handling soft and hard interrupts
> between vCPUs must be properly locked. By acquiring BQL, make sure
> all paths triggering an IRQ are synchronized.
>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>

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

> ---
>  hw/mips/mips_int.c      | 12 ++++++++++++
>  target/mips/op_helper.c | 21 +++------------------
>  2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 48192d2..5ddeb15 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -21,6 +21,7 @@
>   */
>
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "hw/hw.h"
>  #include "hw/mips/cpudevs.h"
>  #include "cpu.h"
> @@ -32,10 +33,17 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>      MIPSCPU *cpu = opaque;
>      CPUMIPSState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
> +    bool locked = false;
>
>      if (irq < 0 || irq > 7)
>          return;
>
> +    /* Make sure locking works even if BQL is already held by the caller */
> +    if (!qemu_mutex_iothread_locked()) {
> +        locked = true;
> +        qemu_mutex_lock_iothread();
> +    }
> +
>      if (level) {
>          env->CP0_Cause |= 1 << (irq + CP0Ca_IP);
>
> @@ -56,6 +64,10 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>      } else {
>          cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
>      }
> +
> +    if (locked) {
> +        qemu_mutex_unlock_iothread();
> +    }
>  }
>
>  void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 44a9b06..6bd8e59 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -17,7 +17,6 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> -#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "internal.h"
>  #include "qemu/host-utils.h"
> @@ -809,11 +808,7 @@ target_ulong helper_mftc0_tcschefback(CPUMIPSState *env)
>
>  target_ulong helper_mfc0_count(CPUMIPSState *env)
>  {
> -    int32_t count;
> -    qemu_mutex_lock_iothread();
> -    count = (int32_t) cpu_mips_get_count(env);
> -    qemu_mutex_unlock_iothread();
> -    return count;
> +    return (int32_t)cpu_mips_get_count(env);
>  }
>
>  target_ulong helper_mftc0_entryhi(CPUMIPSState *env)
> @@ -1388,9 +1383,7 @@ void helper_mtc0_hwrena(CPUMIPSState *env, target_ulong arg1)
>
>  void helper_mtc0_count(CPUMIPSState *env, target_ulong arg1)
>  {
> -    qemu_mutex_lock_iothread();
>      cpu_mips_store_count(env, arg1);
> -    qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mtc0_entryhi(CPUMIPSState *env, target_ulong arg1)
> @@ -1439,9 +1432,7 @@ void helper_mttc0_entryhi(CPUMIPSState *env, target_ulong arg1)
>
>  void helper_mtc0_compare(CPUMIPSState *env, target_ulong arg1)
>  {
> -    qemu_mutex_lock_iothread();
>      cpu_mips_store_compare(env, arg1);
> -    qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mtc0_status(CPUMIPSState *env, target_ulong arg1)
> @@ -1495,9 +1486,7 @@ void helper_mtc0_srsctl(CPUMIPSState *env, target_ulong arg1)
>
>  void helper_mtc0_cause(CPUMIPSState *env, target_ulong arg1)
>  {
> -    qemu_mutex_lock_iothread();
>      cpu_mips_store_cause(env, arg1);
> -    qemu_mutex_unlock_iothread();
>  }
>
>  void helper_mttc0_cause(CPUMIPSState *env, target_ulong arg1)
> @@ -2339,16 +2328,12 @@ target_ulong helper_rdhwr_synci_step(CPUMIPSState *env)
>
>  target_ulong helper_rdhwr_cc(CPUMIPSState *env)
>  {
> -    int32_t count;
>      check_hwrena(env, 2, GETPC());
>  #ifdef CONFIG_USER_ONLY
> -    count = env->CP0_Count;
> +    return env->CP0_Count;
>  #else
> -    qemu_mutex_lock_iothread();
> -    count = (int32_t)cpu_mips_get_count(env);
> -    qemu_mutex_unlock_iothread();
> +    return (int32_t)cpu_mips_get_count(env);
>  #endif
> -    return count;
>  }
>
>  target_ulong helper_rdhwr_ccres(CPUMIPSState *env)


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

[Paolo Bonzini]

On 04/04/2018 12:23, Alex Bennée wrote:
> 
>> From: Aleksandar Markovic <aleksandar.markovic@mips.com>
>>
>> Make sure BQL is held for all interrupt requests.
>>
>> For MTTCG-enabled configurations, handling soft and hard interrupts
>> between vCPUs must be properly locked. By acquiring BQL, make sure
>> all paths triggering an IRQ are synchronized.
>>
>> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
>> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>

Is this actually necessary?  What paths are not taking the lock?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 04/04/2018 12:23, Alex Bennée wrote:
>>
>>> From: Aleksandar Markovic <aleksandar.markovic@mips.com>
>>>
>>> Make sure BQL is held for all interrupt requests.
>>>
>>> For MTTCG-enabled configurations, handling soft and hard interrupts
>>> between vCPUs must be properly locked. By acquiring BQL, make sure
>>> all paths triggering an IRQ are synchronized.
>>>
>>> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
>>> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
>
> Is this actually necessary?  What paths are not taking the lock?

Helpers functions have to manually take the lock. AIUI from this patch
the if (locked) dance allows a single function to be used which may
trigger an IRQ from both helpers (no automatic locking) and hw emulation
(locked by default).

>
> Thanks,
>
> Paolo


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 3/6] hw/mips_int: hold BQL for all interrupt requests

[Paolo Bonzini]

On 04/04/2018 15:44, Alex Bennée wrote:
>>>> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
>>>> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
>> Is this actually necessary?  What paths are not taking the lock?
> Helpers functions have to manually take the lock. AIUI from this patch
> the if (locked) dance allows a single function to be used which may
> trigger an IRQ from both helpers (no automatic locking) and hw emulation
> (locked by default).

This makes it harder to understand which paths actually need to take the
lock, and to split the lock in the future.  We do it in some cases, but
in general defining "*_locked" or "*_unlocked" functions is easier on
the brain.

Thanks,

Paolo

[Qemu-devel] [PATCH v2 4/6] target/mips: hold BQL in mips_vpe_wake()

[Aleksandar Markovic]

From: Goran Ferenc <goran.ferenc@mips.com>

Hold BQL whenever mips_vpe_wake() is invoked.

Without this patch, MIPS MT with MTTCG enabled triggers an abort in
tcg_handle_interrupt() due to an unlocked access to cpu_interrupt().
This patch makes sure that the BQL is held in this case.

Signed-off-by: Goran Ferenc <goran.ferenc@mips.com>
Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 target/mips/op_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
index 6bd8e59..d71d916 100644
--- a/target/mips/op_helper.c
+++ b/target/mips/op_helper.c
@@ -17,6 +17,7 @@
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
+#include "qemu/main-loop.h"
 #include "cpu.h"
 #include "internal.h"
 #include "qemu/host-utils.h"
@@ -542,7 +543,9 @@ static inline void mips_vpe_wake(MIPSCPU *c)
     /* Don't set ->halted = 0 directly, let it be done via cpu_has_work
        because there might be other conditions that state that c should
        be sleeping.  */
+    qemu_mutex_lock_iothread();
     cpu_interrupt(CPU(c), CPU_INTERRUPT_WAKE);
+    qemu_mutex_unlock_iothread();
 }
 
 static inline void mips_vpe_sleep(MIPSCPU *cpu)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/6] target/mips: hold BQL in mips_vpe_wake()

[Alex Bennée]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Goran Ferenc <goran.ferenc@mips.com>
>
> Hold BQL whenever mips_vpe_wake() is invoked.
>
> Without this patch, MIPS MT with MTTCG enabled triggers an abort in
> tcg_handle_interrupt() due to an unlocked access to cpu_interrupt().
> This patch makes sure that the BQL is held in this case.
>
> Signed-off-by: Goran Ferenc <goran.ferenc@mips.com>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>

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

> ---
>  target/mips/op_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/target/mips/op_helper.c b/target/mips/op_helper.c
> index 6bd8e59..d71d916 100644
> --- a/target/mips/op_helper.c
> +++ b/target/mips/op_helper.c
> @@ -17,6 +17,7 @@
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
>  #include "cpu.h"
>  #include "internal.h"
>  #include "qemu/host-utils.h"
> @@ -542,7 +543,9 @@ static inline void mips_vpe_wake(MIPSCPU *c)
>      /* Don't set ->halted = 0 directly, let it be done via cpu_has_work
>         because there might be other conditions that state that c should
>         be sleeping.  */
> +    qemu_mutex_lock_iothread();
>      cpu_interrupt(CPU(c), CPU_INTERRUPT_WAKE);
> +    qemu_mutex_unlock_iothread();
>  }
>
>  static inline void mips_vpe_sleep(MIPSCPU *cpu)


--
Alex Bennée

[Qemu-devel] [PATCH v2 5/6] hw/mips_cpc: kick a VP when putting it into Run state

[Aleksandar Markovic]

From: Miodrag Dinic <miodrag.dinic@mips.com>

While testing mttcg VP0 could get stuck in a loop waiting for other
VPs to come up (which never actually happens). To fix this, kick VPs
while they are being powered up by Cluster Power Controller in an
async task which is triggered once the host thread is being spawned.

Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 hw/misc/mips_cpc.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/hw/misc/mips_cpc.c b/hw/misc/mips_cpc.c
index 6d34574..712d842 100644
--- a/hw/misc/mips_cpc.c
+++ b/hw/misc/mips_cpc.c
@@ -30,6 +30,14 @@ static inline uint64_t cpc_vp_run_mask(MIPSCPCState *cpc)
     return (1ULL << cpc->num_vp) - 1;
 }
 
+static void mips_cpu_reset_async_work(CPUState *cs, run_on_cpu_data data)
+{
+    MIPSCPCState *cpc = (MIPSCPCState *) data.host_ptr;
+
+    cpu_reset(cs);
+    cpc->vp_running |= 1ULL << cs->cpu_index;
+}
+
 static void cpc_run_vp(MIPSCPCState *cpc, uint64_t vp_run)
 {
     CPUState *cs = first_cpu;
@@ -37,8 +45,13 @@ static void cpc_run_vp(MIPSCPCState *cpc, uint64_t vp_run)
     CPU_FOREACH(cs) {
         uint64_t i = 1ULL << cs->cpu_index;
         if (i & vp_run & ~cpc->vp_running) {
-            cpu_reset(cs);
-            cpc->vp_running |= i;
+            /*
+             * To avoid racing with a CPU we are just kicking off.
+             * We do the final bit of preparation for the work in
+             * the target CPUs context.
+             */
+            async_safe_run_on_cpu(cs, mips_cpu_reset_async_work,
+                                  RUN_ON_CPU_HOST_PTR(cpc));
         }
     }
 }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 5/6] hw/mips_cpc: kick a VP when putting it into Run state

[Alex Bennée]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Miodrag Dinic <miodrag.dinic@mips.com>
>
> While testing mttcg VP0 could get stuck in a loop waiting for other
> VPs to come up (which never actually happens). To fix this, kick VPs
> while they are being powered up by Cluster Power Controller in an
> async task which is triggered once the host thread is being spawned.
>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
> Signed-off-by: Leon Alrae <leon.alrae@imgtec.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>

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

> ---
>  hw/misc/mips_cpc.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/mips_cpc.c b/hw/misc/mips_cpc.c
> index 6d34574..712d842 100644
> --- a/hw/misc/mips_cpc.c
> +++ b/hw/misc/mips_cpc.c
> @@ -30,6 +30,14 @@ static inline uint64_t cpc_vp_run_mask(MIPSCPCState *cpc)
>      return (1ULL << cpc->num_vp) - 1;
>  }
>
> +static void mips_cpu_reset_async_work(CPUState *cs, run_on_cpu_data data)
> +{
> +    MIPSCPCState *cpc = (MIPSCPCState *) data.host_ptr;
> +
> +    cpu_reset(cs);
> +    cpc->vp_running |= 1ULL << cs->cpu_index;
> +}
> +
>  static void cpc_run_vp(MIPSCPCState *cpc, uint64_t vp_run)
>  {
>      CPUState *cs = first_cpu;
> @@ -37,8 +45,13 @@ static void cpc_run_vp(MIPSCPCState *cpc, uint64_t vp_run)
>      CPU_FOREACH(cs) {
>          uint64_t i = 1ULL << cs->cpu_index;
>          if (i & vp_run & ~cpc->vp_running) {
> -            cpu_reset(cs);
> -            cpc->vp_running |= i;
> +            /*
> +             * To avoid racing with a CPU we are just kicking off.
> +             * We do the final bit of preparation for the work in
> +             * the target CPUs context.
> +             */
> +            async_safe_run_on_cpu(cs, mips_cpu_reset_async_work,
> +                                  RUN_ON_CPU_HOST_PTR(cpc));
>          }
>      }
>  }


--
Alex Bennée

[Qemu-devel] [PATCH v2 6/6] target/mips: introduce MTTCG-enabled builds

[Aleksandar Markovic]

From: Aleksandar Markovic <aleksandar.markovic@mips.com>

Introduce MTTCG-enabled QEMU builds for mips32, mipsn32, and mips64.

Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>
---
 configure         | 3 +++
 target/mips/cpu.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/configure b/configure
index ed45a3c..9b338dd 100755
--- a/configure
+++ b/configure
@@ -6735,16 +6735,19 @@ case "$target_name" in
     bflt="yes"
   ;;
   mips|mipsel)
+    mttcg="yes"
     TARGET_ARCH=mips
     echo "TARGET_ABI_MIPSO32=y" >> $config_target_mak
   ;;
   mipsn32|mipsn32el)
+    mttcg="yes"
     TARGET_ARCH=mips64
     TARGET_BASE_ARCH=mips
     echo "TARGET_ABI_MIPSN32=y" >> $config_target_mak
     echo "TARGET_ABI32=y" >> $config_target_mak
   ;;
   mips64|mips64el)
+    mttcg="yes"
     TARGET_ARCH=mips64
     TARGET_BASE_ARCH=mips
     echo "TARGET_ABI_MIPSN64=y" >> $config_target_mak
diff --git a/target/mips/cpu.h b/target/mips/cpu.h
index 3fa85b0..8f41952 100644
--- a/target/mips/cpu.h
+++ b/target/mips/cpu.h
@@ -11,6 +11,8 @@
 #include "exec/cpu-defs.h"
 #include "fpu/softfloat.h"
 
+#define TCG_GUEST_DEFAULT_MO (0)
+
 struct CPUMIPSState;
 
 typedef struct CPUMIPSTLBContext CPUMIPSTLBContext;
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 6/6] target/mips: introduce MTTCG-enabled builds

[Alex Bennée]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Aleksandar Markovic <aleksandar.markovic@mips.com>
>
> Introduce MTTCG-enabled QEMU builds for mips32, mipsn32, and mips64.
>
> Signed-off-by: Miodrag Dinic <miodrag.dinic@mips.com>
> Signed-off-by: Aleksandar Markovic <aleksandar.markovic@mips.com>

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

> ---
>  configure         | 3 +++
>  target/mips/cpu.h | 2 ++
>  2 files changed, 5 insertions(+)
>
> diff --git a/configure b/configure
> index ed45a3c..9b338dd 100755
> --- a/configure
> +++ b/configure
> @@ -6735,16 +6735,19 @@ case "$target_name" in
>      bflt="yes"
>    ;;
>    mips|mipsel)
> +    mttcg="yes"
>      TARGET_ARCH=mips
>      echo "TARGET_ABI_MIPSO32=y" >> $config_target_mak
>    ;;
>    mipsn32|mipsn32el)
> +    mttcg="yes"
>      TARGET_ARCH=mips64
>      TARGET_BASE_ARCH=mips
>      echo "TARGET_ABI_MIPSN32=y" >> $config_target_mak
>      echo "TARGET_ABI32=y" >> $config_target_mak
>    ;;
>    mips64|mips64el)
> +    mttcg="yes"
>      TARGET_ARCH=mips64
>      TARGET_BASE_ARCH=mips
>      echo "TARGET_ABI_MIPSN64=y" >> $config_target_mak
> diff --git a/target/mips/cpu.h b/target/mips/cpu.h
> index 3fa85b0..8f41952 100644
> --- a/target/mips/cpu.h
> +++ b/target/mips/cpu.h
> @@ -11,6 +11,8 @@
>  #include "exec/cpu-defs.h"
>  #include "fpu/softfloat.h"
>
> +#define TCG_GUEST_DEFAULT_MO (0)
> +
>  struct CPUMIPSState;
>
>  typedef struct CPUMIPSTLBContext CPUMIPSTLBContext;


--
Alex Bennée

Re: [Qemu-devel] [PATCH v2 0/6] target-mips: support MTTCG feature

[Alex Bennée]

Aleksandar Markovic <aleksandar.markovic@rt-rk.com> writes:

> From: Aleksandar Markovic <aleksandar.markovic@mips.com>
>
> v1->v2:
>
>   - patches #3 and #4 are squashed into one to avoid bisect breaking
>   - improved locking features in patch #5 (formerly #6)
>   - commit messages reviewed and improved
>   - rebased to the latest code
>
>
> This series introduces MTTCG feature for MIPS targets by adding all
> missing bits and pieces, and formally enabling corresponding QEMU
> builds to support such configurations.

You are better able to test the changes than me but I've built the
patches and eyeballed the BQL/async patches and they look good to me.

Sorry it took so long to get around to looking at them.

whole series:

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

--
Alex Bennée