[PATCH v1 00/21] accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path

[PATCH v1 00/21] accel/tcg: remove implied BQL from cpu_handle_interrupt/exception path

[Robert Foley]

The purpose of this change is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

The BQL is a bottleneck in scaling to more cores.
And this cpu_handle_interrupt/exception path is one of
the key BQL users as measured by the QEMU sync profiling (qsp).

We have chosen to break up the process of removing
BQL from this path into two pieces:

1) Changes to the core/common functions of cpu_handle_interrupt/exception
   to drop the holding of the BQL. The holding of the BQL is pushed down
   to the per-arch implementation code.
   This set of changes is handled in this patch.

   This approach of pushing the BQL down to the per arch functions was
   suggested by Paolo Bonzini.
   For reference, here are two key posts in the discussion, explaining
   the reasoning/benefits of this approach.
   https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
   https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

2) Removing the BQL from the per-arch functions.
   Since the arch now has the code that grabs the BQL, each arch can
   change its use of the BQL for interrupts independently.
   We leave it up to the arch to make the change at the time that makes sense.

It is worth mentioning that we are working on per-arch changes
in line with 2), and plan to submit these.
In other words, we plan to set the groundwork with this
patch series and then will take advantage of it in later series.

This patch series is based on the per-CPU locks patch:
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05314.html

Robert Foley (21):
  accel/tcg:  Change interrupt/exception handling to remove implied BQL
  target/alpha: add BQL to do_interrupt and cpu_exec_interrupt
  target/arm: add BQL to do_interrupt and cpu_exec_interrupt
  target/avr: add BQL to do_interrupt and cpu_exec_interrupt
  target/cris: add BQL to do_interrupt and cpu_exec_interrupt
  target/hppa: add BQL to do_interrupt and cpu_exec_interrupt
  target/i386: add BQL to do_interrupt and cpu_exec_interrupt
  target/lm32: add BQL to do_interrupt and cpu_exec_interrupt
  target/m68k: add BQL to do_interrupt and cpu_exec_interrupt
  target/microblaze: add BQL to do_interrupt and cpu_exec_interrupt
  target/mips: add BQL to do_interrupt and cpu_exec_interrupt
  target/nios2: add BQL to do_interrupt and cpu_exec_interrupt
  target/openrisc: add BQL to do_interrupt and cpu_exec_interrupt
  target/ppc: add BQL to do_interrupt and cpu_exec_interrupt
  target/riscv: add BQL to do_interrupt and cpu_exec_interrupt
  target/rx: add BQL to do_interrupt and cpu_exec_interrupt
  target/s390x: add BQL to do_interrupt and cpu_exec_interrupt
  target/sh4: add BQL to do_interrupt and cpu_exec_interrupt
  target/sparc: add BQL to do_interrupt and cpu_exec_interrupt
  target/unicore32: add BQL to do_interrupt and cpu_exec_interrupt
  target/xtensa: add BQL to do_interrupt and cpu_exec_interrupt

 accel/tcg/cpu-exec.c        | 19 +++++++++++--------
 target/alpha/helper.c       | 15 +++++++++++++--
 target/arm/cpu.c            | 13 ++++++++++---
 target/arm/helper.c         | 17 ++++++++++++++++-
 target/avr/helper.c         | 12 +++++++++++-
 target/cris/helper.c        | 18 ++++++++++++++++++
 target/hppa/int_helper.c    | 25 +++++++++++++++++++------
 target/i386/seg_helper.c    |  7 +++++--
 target/lm32/helper.c        | 10 ++++++++++
 target/m68k/op_helper.c     |  5 +++++
 target/microblaze/helper.c  | 20 ++++++++++++++++++++
 target/mips/helper.c        | 10 ++++++++++
 target/nios2/cpu.c          |  3 +++
 target/nios2/helper.c       |  8 +++++++-
 target/openrisc/interrupt.c | 10 ++++++++++
 target/ppc/excp_helper.c    |  5 +++++
 target/riscv/cpu_helper.c   | 10 ++++++++++
 target/rx/helper.c          | 10 ++++++++++
 target/s390x/excp_helper.c  | 12 +++++++++++-
 target/sh4/helper.c         | 13 +++++++++++--
 target/sparc/cpu.c          |  3 +++
 target/sparc/int32_helper.c | 13 ++++++++++++-
 target/unicore32/helper.c   |  3 +++
 target/unicore32/softmmu.c  |  7 +++++++
 target/xtensa/exc_helper.c  |  2 ++
 25 files changed, 242 insertions(+), 28 deletions(-)

-- 
2.17.1

[PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

This change removes the implied BQL from the cpu_handle_interrupt,
and cpu_handle_exception paths. This BQL acquire is being pushed
down into the per arch implementation.

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 accel/tcg/cpu-exec.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 80d0e649b2..8e2bfd97a1 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
 #else
         if (replay_exception()) {
             CPUClass *cc = CPU_GET_CLASS(cpu);
-            qemu_mutex_lock_iothread();
             cc->do_interrupt(cpu);
-            qemu_mutex_unlock_iothread();
             cpu->exception_index = -1;
 
             if (unlikely(cpu->singlestep_enabled)) {
@@ -558,7 +556,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
     if (unlikely(cpu_interrupt_request(cpu))) {
         int interrupt_request;
 
-        qemu_mutex_lock_iothread();
+        cpu_mutex_lock(cpu);
         interrupt_request = cpu_interrupt_request(cpu);
         if (unlikely(cpu->singlestep_enabled & SSTEP_NOIRQ)) {
             /* Mask out external interrupts for this step. */
@@ -567,7 +565,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
         if (interrupt_request & CPU_INTERRUPT_DEBUG) {
             cpu_reset_interrupt(cpu, CPU_INTERRUPT_DEBUG);
             cpu->exception_index = EXCP_DEBUG;
-            qemu_mutex_unlock_iothread();
+            cpu_mutex_unlock(cpu);
             return true;
         }
         if (replay_mode == REPLAY_MODE_PLAY && !replay_has_interrupt()) {
@@ -577,13 +575,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
             cpu_reset_interrupt(cpu, CPU_INTERRUPT_HALT);
             cpu_halted_set(cpu, 1);
             cpu->exception_index = EXCP_HLT;
-            qemu_mutex_unlock_iothread();
+            cpu_mutex_unlock(cpu);
             return true;
         }
 #if defined(TARGET_I386)
         else if (interrupt_request & CPU_INTERRUPT_INIT) {
             X86CPU *x86_cpu = X86_CPU(cpu);
             CPUArchState *env = &x86_cpu->env;
+            cpu_mutex_unlock(cpu);
+            qemu_mutex_lock_iothread();
             replay_interrupt();
             cpu_svm_check_intercept_param(env, SVM_EXIT_INIT, 0, 0);
             do_cpu_init(x86_cpu);
@@ -595,7 +595,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
         else if (interrupt_request & CPU_INTERRUPT_RESET) {
             replay_interrupt();
             cpu_reset(cpu);
-            qemu_mutex_unlock_iothread();
+            cpu_mutex_unlock(cpu);
             return true;
         }
 #endif
@@ -604,7 +604,9 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
            True when it is, and we should restart on a new TB,
            and via longjmp via cpu_loop_exit.  */
         else {
+            cpu_mutex_unlock(cpu);
             if (cc->cpu_exec_interrupt(cpu, interrupt_request)) {
+                cpu_mutex_lock(cpu);
                 replay_interrupt();
                 /*
                  * After processing the interrupt, ensure an EXCP_DEBUG is
@@ -614,6 +616,8 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
                 cpu->exception_index =
                     (cpu->singlestep_enabled ? EXCP_DEBUG : -1);
                 *last_tb = NULL;
+            } else {
+                cpu_mutex_lock(cpu);
             }
             /* The target hook may have updated the 'cpu->interrupt_request';
              * reload the 'interrupt_request' value */
@@ -627,7 +631,7 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
         }
 
         /* If we exit via cpu_loop_exit/longjmp it is reset in cpu_exec */
-        qemu_mutex_unlock_iothread();
+        cpu_mutex_unlock(cpu);
     }
 
     /* Finally, check if we need to exit to the main loop.  */
@@ -691,7 +695,6 @@ static inline void cpu_loop_exec_tb(CPUState *cpu, TranslationBlock *tb,
     }
 #endif
 }
-
 /* main execution loop */
 
 int cpu_exec(CPUState *cpu)
-- 
2.17.1

[PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/alpha/helper.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/target/alpha/helper.c b/target/alpha/helper.c
index 55d7274d94..18169ae1c5 100644
--- a/target/alpha/helper.c
+++ b/target/alpha/helper.c
@@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
 {
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
-    int i = cs->exception_index;
-
+    int i;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
+    i = cs->exception_index;
     if (qemu_loglevel_mask(CPU_LOG_INT)) {
         static int count;
         const char *name = "<unknown>";
@@ -405,6 +409,9 @@ void alpha_cpu_do_interrupt(CPUState *cs)
     /* Switch to PALmode.  */
     env->flags |= ENV_FLAG_PAL_MODE;
 #endif /* !USER_ONLY */
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool alpha_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -412,9 +419,11 @@ bool alpha_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     AlphaCPU *cpu = ALPHA_CPU(cs);
     CPUAlphaState *env = &cpu->env;
     int idx = -1;
+    qemu_mutex_lock_iothread();
 
     /* We never take interrupts while in PALmode.  */
     if (env->flags & ENV_FLAG_PAL_MODE) {
+        qemu_mutex_unlock_iothread();
         return false;
     }
 
@@ -446,8 +455,10 @@ bool alpha_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         cs->exception_index = idx;
         env->error_code = 0;
         alpha_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 03/21] target/arm: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/arm/cpu.c    | 13 ++++++++++---
 target/arm/helper.c | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 401832ea95..b8544f0f0a 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -528,12 +528,17 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     CPUClass *cc = CPU_GET_CLASS(cs);
     CPUARMState *env = cs->env_ptr;
-    uint32_t cur_el = arm_current_el(env);
-    bool secure = arm_is_secure(env);
-    uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+    uint32_t cur_el;
+    bool secure;
+    uint64_t hcr_el2;
     uint32_t target_el;
     uint32_t excp_idx;
 
+    qemu_mutex_lock_iothread();
+    cur_el = arm_current_el(env);
+    secure = arm_is_secure(env);
+    hcr_el2 = arm_hcr_el2_eff(env);
+
     /* The prioritization of interrupts is IMPLEMENTATION DEFINED. */
 
     if (interrupt_request & CPU_INTERRUPT_FIQ) {
@@ -568,12 +573,14 @@ bool arm_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             goto found;
         }
     }
+    qemu_mutex_unlock_iothread();
     return false;
 
  found:
     cs->exception_index = excp_idx;
     env->exception.target_el = target_el;
     cc->do_interrupt(cs);
+    qemu_mutex_unlock_iothread();
     return true;
 }
 
diff --git a/target/arm/helper.c b/target/arm/helper.c
index c5ea2c25ea..3a22d40598 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9759,7 +9759,13 @@ void arm_cpu_do_interrupt(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
     CPUARMState *env = &cpu->env;
-    unsigned int new_el = env->exception.target_el;
+    unsigned int new_el;
+
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
+    new_el = env->exception.target_el;
 
     assert(!arm_feature(env, ARM_FEATURE_M));
 
@@ -9776,6 +9782,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
     if (arm_is_psci_call(cpu, cs->exception_index)) {
         arm_handle_psci_call(cpu);
         qemu_log_mask(CPU_LOG_INT, "...handled as PSCI call\n");
+        if (bql) {
+            qemu_mutex_unlock_iothread();
+        }
         return;
     }
 
@@ -9787,6 +9796,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
 #ifdef CONFIG_TCG
     if (cs->exception_index == EXCP_SEMIHOST) {
         handle_semihosting(cs);
+        if (bql) {
+            qemu_mutex_unlock_iothread();
+        }
         return;
     }
 #endif
@@ -9808,6 +9820,9 @@ void arm_cpu_do_interrupt(CPUState *cs)
     if (!kvm_enabled()) {
         cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
     }
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 #endif /* !CONFIG_USER_ONLY */
 
-- 
2.17.1

[PATCH v1 04/21] target/avr: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/avr/helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/avr/helper.c b/target/avr/helper.c
index d96d14372b..f0d625c195 100644
--- a/target/avr/helper.c
+++ b/target/avr/helper.c
@@ -30,6 +30,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     CPUClass *cc = CPU_GET_CLASS(cs);
     AVRCPU *cpu = AVR_CPU(cs);
     CPUAVRState *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if (interrupt_request & CPU_INTERRUPT_RESET) {
         if (cpu_interrupts_enabled(env)) {
@@ -53,6 +54,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             ret = true;
         }
     }
+    qemu_mutex_unlock_iothread();
     return ret;
 }
 
@@ -61,10 +63,15 @@ void avr_cpu_do_interrupt(CPUState *cs)
     AVRCPU *cpu = AVR_CPU(cs);
     CPUAVRState *env = &cpu->env;
 
-    uint32_t ret = env->pc_w;
+    uint32_t ret;
     int vector = 0;
     int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
     int base = 0;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
+    ret = env->pc_w;
 
     if (cs->exception_index == EXCP_RESET) {
         vector = 0;
@@ -87,6 +94,9 @@ void avr_cpu_do_interrupt(CPUState *cs)
     env->sregI = 0; /* clear Global Interrupt Flag */
 
     cs->exception_index = -1;
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
-- 
2.17.1

[PATCH v1 05/21] target/cris: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/cris/helper.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/target/cris/helper.c b/target/cris/helper.c
index 67946d9246..22aecde0f5 100644
--- a/target/cris/helper.c
+++ b/target/cris/helper.c
@@ -45,8 +45,10 @@ void cris_cpu_do_interrupt(CPUState *cs)
     CRISCPU *cpu = CRIS_CPU(cs);
     CPUCRISState *env = &cpu->env;
 
+    qemu_mutex_lock_iothread();
     cs->exception_index = -1;
     env->pregs[PR_ERP] = env->pc;
+    qemu_mutex_unlock_iothread();
 }
 
 void crisv10_cpu_do_interrupt(CPUState *cs)
@@ -128,6 +130,10 @@ void crisv10_cpu_do_interrupt(CPUState *cs)
     CRISCPU *cpu = CRIS_CPU(cs);
     CPUCRISState *env = &cpu->env;
     int ex_vec = -1;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     D_LOG("exception index=%d interrupt_req=%d\n",
           cs->exception_index,
@@ -183,6 +189,9 @@ void crisv10_cpu_do_interrupt(CPUState *cs)
                   env->pregs[PR_CCS],
                   env->pregs[PR_PID],
                   env->pregs[PR_ERP]);
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 void cris_cpu_do_interrupt(CPUState *cs)
@@ -190,6 +199,10 @@ void cris_cpu_do_interrupt(CPUState *cs)
     CRISCPU *cpu = CRIS_CPU(cs);
     CPUCRISState *env = &cpu->env;
     int ex_vec = -1;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     D_LOG("exception index=%d interrupt_req=%d\n",
           cs->exception_index,
@@ -265,6 +278,9 @@ void cris_cpu_do_interrupt(CPUState *cs)
           env->pregs[PR_CCS],
           env->pregs[PR_PID],
           env->pregs[PR_ERP]);
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 hwaddr cris_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -294,6 +310,7 @@ bool cris_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     CRISCPU *cpu = CRIS_CPU(cs);
     CPUCRISState *env = &cpu->env;
     bool ret = false;
+    qemu_mutex_lock_iothread();
 
     if (interrupt_request & CPU_INTERRUPT_HARD
         && (env->pregs[PR_CCS] & I_FLAG)
@@ -315,6 +332,7 @@ bool cris_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             ret = true;
         }
     }
+    qemu_mutex_unlock_iothread();
 
     return ret;
 }
-- 
2.17.1

[PATCH v1 06/21] target/hppa: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/hppa/int_helper.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/target/hppa/int_helper.c b/target/hppa/int_helper.c
index 462747baf8..eda40bc5d9 100644
--- a/target/hppa/int_helper.c
+++ b/target/hppa/int_helper.c
@@ -94,12 +94,20 @@ void hppa_cpu_do_interrupt(CPUState *cs)
 {
     HPPACPU *cpu = HPPA_CPU(cs);
     CPUHPPAState *env = &cpu->env;
-    int i = cs->exception_index;
-    target_ureg iaoq_f = env->iaoq_f;
-    target_ureg iaoq_b = env->iaoq_b;
-    uint64_t iasq_f = env->iasq_f;
-    uint64_t iasq_b = env->iasq_b;
-
+    int i;
+    target_ureg iaoq_f;
+    target_ureg iaoq_b;
+    uint64_t iasq_f;
+    uint64_t iasq_b;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
+    i = cs->exception_index;
+    iaoq_f = env->iaoq_f;
+    iaoq_b = env->iaoq_b;
+    iasq_f = env->iasq_f;
+    iasq_b = env->iasq_b;
 #ifndef CONFIG_USER_ONLY
     target_ureg old_psw;
 
@@ -244,6 +252,9 @@ void hppa_cpu_do_interrupt(CPUState *cs)
                                env->cr[CR_IOR]));
     }
     cs->exception_index = -1;
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool hppa_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -251,6 +262,7 @@ bool hppa_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 #ifndef CONFIG_USER_ONLY
     HPPACPU *cpu = HPPA_CPU(cs);
     CPUHPPAState *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     /* If interrupts are requested and enabled, raise them.  */
     if ((env->psw & PSW_I) && (interrupt_request & CPU_INTERRUPT_HARD)) {
@@ -258,6 +270,7 @@ bool hppa_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         hppa_cpu_do_interrupt(cs);
         return true;
     }
+    qemu_mutex_unlock_iothread();
 #endif
     return false;
 }
-- 
2.17.1

[PATCH v1 07/21] target/i386: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/i386/seg_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/i386/seg_helper.c b/target/i386/seg_helper.c
index 818f65f35f..114d4a0d24 100644
--- a/target/i386/seg_helper.c
+++ b/target/i386/seg_helper.c
@@ -1284,7 +1284,7 @@ void x86_cpu_do_interrupt(CPUState *cs)
 {
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
-
+    qemu_mutex_lock_iothread();
 #if defined(CONFIG_USER_ONLY)
     /* if user mode only, we simulate a fake exception
        which will be handled outside the cpu execution
@@ -1308,6 +1308,7 @@ void x86_cpu_do_interrupt(CPUState *cs)
         env->old_exception = -1;
     }
 #endif
+    qemu_mutex_unlock_iothread();
 }
 
 void do_interrupt_x86_hardirq(CPUX86State *env, int intno, int is_hw)
@@ -1320,9 +1321,10 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     X86CPU *cpu = X86_CPU(cs);
     CPUX86State *env = &cpu->env;
     int intno;
-
+    qemu_mutex_lock_iothread();
     interrupt_request = x86_cpu_pending_interrupt(cs, interrupt_request);
     if (!interrupt_request) {
+        qemu_mutex_unlock_iothread();
         return false;
     }
 
@@ -1377,6 +1379,7 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     }
 
     /* Ensure that no TB jump will be modified as the program flow was changed.  */
+    qemu_mutex_unlock_iothread();
     return true;
 }
 
-- 
2.17.1

[PATCH v1 08/21] target/lm32: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/lm32/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/lm32/helper.c b/target/lm32/helper.c
index 1130fc8884..4439b06ecc 100644
--- a/target/lm32/helper.c
+++ b/target/lm32/helper.c
@@ -152,6 +152,10 @@ void lm32_cpu_do_interrupt(CPUState *cs)
 {
     LM32CPU *cpu = LM32_CPU(cs);
     CPULM32State *env = &cpu->env;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     qemu_log_mask(CPU_LOG_INT,
             "exception at pc=%x type=%x\n", env->pc, cs->exception_index);
@@ -196,18 +200,24 @@ void lm32_cpu_do_interrupt(CPUState *cs)
                   cs->exception_index);
         break;
     }
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool lm32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     LM32CPU *cpu = LM32_CPU(cs);
     CPULM32State *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if ((interrupt_request & CPU_INTERRUPT_HARD) && (env->ie & IE_IE)) {
         cs->exception_index = EXCP_IRQ;
         lm32_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 09/21] target/m68k: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/m68k/op_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 4a032a150e..0c3333476a 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -448,7 +448,9 @@ void m68k_cpu_do_interrupt(CPUState *cs)
     M68kCPU *cpu = M68K_CPU(cs);
     CPUM68KState *env = &cpu->env;
 
+    qemu_mutex_lock_iothread();
     do_interrupt_all(env, 0);
+    qemu_mutex_unlock_iothread();
 }
 
 static inline void do_interrupt_m68k_hardirq(CPUM68KState *env)
@@ -508,6 +510,7 @@ bool m68k_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     M68kCPU *cpu = M68K_CPU(cs);
     CPUM68KState *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if (interrupt_request & CPU_INTERRUPT_HARD
         && ((env->sr & SR_I) >> SR_I_SHIFT) < env->pending_level) {
@@ -519,8 +522,10 @@ bool m68k_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
          */
         cs->exception_index = env->pending_vector;
         do_interrupt_m68k_hardirq(env);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 10/21] target/microblaze: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/microblaze/helper.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
index ab2ceeb055..ae8ff2bea4 100644
--- a/target/microblaze/helper.c
+++ b/target/microblaze/helper.c
@@ -32,10 +32,17 @@ void mb_cpu_do_interrupt(CPUState *cs)
 {
     MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
     CPUMBState *env = &cpu->env;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     cs->exception_index = -1;
     env->res_addr = RES_ADDR_NONE;
     env->regs[14] = env->sregs[SR_PC];
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
@@ -113,6 +120,10 @@ void mb_cpu_do_interrupt(CPUState *cs)
     MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
     CPUMBState *env = &cpu->env;
     uint32_t t;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     /* IMM flag cannot propagate across a branch and into the dslot.  */
     assert(!((env->iflags & D_FLAG) && (env->iflags & IMM_FLAG)));
@@ -123,6 +134,9 @@ void mb_cpu_do_interrupt(CPUState *cs)
         case EXCP_HW_EXCP:
             if (!(env->pvr.regs[0] & PVR0_USE_EXC_MASK)) {
                 qemu_log_mask(LOG_GUEST_ERROR, "Exception raised on system without exceptions!\n");
+                if (bql) {
+                    qemu_mutex_unlock_iothread();
+                }
                 return;
             }
 
@@ -262,6 +276,9 @@ void mb_cpu_do_interrupt(CPUState *cs)
                       cs->exception_index);
             break;
     }
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 hwaddr mb_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
@@ -291,6 +308,7 @@ bool mb_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     MicroBlazeCPU *cpu = MICROBLAZE_CPU(cs);
     CPUMBState *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if ((interrupt_request & CPU_INTERRUPT_HARD)
         && (env->sregs[SR_MSR] & MSR_IE)
@@ -298,7 +316,9 @@ bool mb_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         && !(env->iflags & (D_FLAG | IMM_FLAG))) {
         cs->exception_index = EXCP_IRQ;
         mb_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
-- 
2.17.1

[PATCH v1 11/21] target/mips: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/mips/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/mips/helper.c b/target/mips/helper.c
index afd78b1990..6595d18702 100644
--- a/target/mips/helper.c
+++ b/target/mips/helper.c
@@ -1085,6 +1085,10 @@ static inline void set_badinstr_registers(CPUMIPSState *env)
 
 void mips_cpu_do_interrupt(CPUState *cs)
 {
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 #if !defined(CONFIG_USER_ONLY)
     MIPSCPU *cpu = MIPS_CPU(cs);
     CPUMIPSState *env = &cpu->env;
@@ -1396,10 +1400,14 @@ void mips_cpu_do_interrupt(CPUState *cs)
     }
 #endif
     cs->exception_index = EXCP_NONE;
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool mips_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+    qemu_mutex_lock_iothread();
     if (interrupt_request & CPU_INTERRUPT_HARD) {
         MIPSCPU *cpu = MIPS_CPU(cs);
         CPUMIPSState *env = &cpu->env;
@@ -1410,9 +1418,11 @@ bool mips_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             cs->exception_index = EXCP_EXT_INTERRUPT;
             env->error_code = 0;
             mips_cpu_do_interrupt(cs);
+            qemu_mutex_unlock_iothread();
             return true;
         }
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 12/21] target/nios2: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/nios2/cpu.c    | 3 +++
 target/nios2/helper.c | 8 +++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/nios2/cpu.c b/target/nios2/cpu.c
index fe5fd9adfd..fd05406eac 100644
--- a/target/nios2/cpu.c
+++ b/target/nios2/cpu.c
@@ -102,13 +102,16 @@ static bool nios2_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     Nios2CPU *cpu = NIOS2_CPU(cs);
     CPUNios2State *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if ((interrupt_request & CPU_INTERRUPT_HARD) &&
         (env->regs[CR_STATUS] & CR_STATUS_PIE)) {
         cs->exception_index = EXCP_IRQ;
         nios2_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
diff --git a/target/nios2/helper.c b/target/nios2/helper.c
index 57c97bde3c..46d53551d4 100644
--- a/target/nios2/helper.c
+++ b/target/nios2/helper.c
@@ -52,7 +52,10 @@ void nios2_cpu_do_interrupt(CPUState *cs)
 {
     Nios2CPU *cpu = NIOS2_CPU(cs);
     CPUNios2State *env = &cpu->env;
-
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
     switch (cs->exception_index) {
     case EXCP_IRQ:
         assert(env->regs[CR_STATUS] & CR_STATUS_PIE);
@@ -198,6 +201,9 @@ void nios2_cpu_do_interrupt(CPUState *cs)
                   cs->exception_index);
         break;
     }
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 hwaddr nios2_cpu_get_phys_page_debug(CPUState *cs, vaddr addr)
-- 
2.17.1

[PATCH v1 13/21] target/openrisc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/openrisc/interrupt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/openrisc/interrupt.c b/target/openrisc/interrupt.c
index 3eab771dcd..361f242954 100644
--- a/target/openrisc/interrupt.c
+++ b/target/openrisc/interrupt.c
@@ -28,6 +28,10 @@
 
 void openrisc_cpu_do_interrupt(CPUState *cs)
 {
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 #ifndef CONFIG_USER_ONLY
     OpenRISCCPU *cpu = OPENRISC_CPU(cs);
     CPUOpenRISCState *env = &cpu->env;
@@ -99,6 +103,9 @@ void openrisc_cpu_do_interrupt(CPUState *cs)
 #endif
 
     cs->exception_index = -1;
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool openrisc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -106,6 +113,7 @@ bool openrisc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     OpenRISCCPU *cpu = OPENRISC_CPU(cs);
     CPUOpenRISCState *env = &cpu->env;
     int idx = -1;
+    qemu_mutex_lock_iothread();
 
     if ((interrupt_request & CPU_INTERRUPT_HARD) && (env->sr & SR_IEE)) {
         idx = EXCP_INT;
@@ -116,7 +124,9 @@ bool openrisc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     if (idx >= 0) {
         cs->exception_index = idx;
         openrisc_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
-- 
2.17.1

[PATCH v1 14/21] target/ppc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/ppc/excp_helper.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index bf9e1e27e9..4530230d65 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -870,7 +870,9 @@ void ppc_cpu_do_interrupt(CPUState *cs)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
 
+    qemu_mutex_lock_iothread();
     powerpc_excp(cpu, env->excp_model, cs->exception_index);
+    qemu_mutex_unlock_iothread();
 }
 
 static void ppc_hw_interrupt(CPUPPCState *env)
@@ -1056,14 +1058,17 @@ bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
+    qemu_mutex_lock_iothread();
 
     if (interrupt_request & CPU_INTERRUPT_HARD) {
         ppc_hw_interrupt(env);
         if (env->pending_interrupts == 0) {
             cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
         }
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 15/21] target/riscv: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/riscv/cpu_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 75d2ae3434..5050802e95 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -80,14 +80,17 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
 #if !defined(CONFIG_USER_ONLY)
     if (interrupt_request & CPU_INTERRUPT_HARD) {
+        qemu_mutex_lock_iothread();
         RISCVCPU *cpu = RISCV_CPU(cs);
         CPURISCVState *env = &cpu->env;
         int interruptno = riscv_cpu_local_irq_pending(env);
         if (interruptno >= 0) {
             cs->exception_index = RISCV_EXCP_INT_FLAG | interruptno;
             riscv_cpu_do_interrupt(cs);
+            qemu_mutex_unlock_iothread();
             return true;
         }
+        qemu_mutex_unlock_iothread();
     }
 #endif
     return false;
@@ -822,6 +825,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
  */
 void riscv_cpu_do_interrupt(CPUState *cs)
 {
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 #if !defined(CONFIG_USER_ONLY)
 
     RISCVCPU *cpu = RISCV_CPU(cs);
@@ -982,4 +989,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
 
 #endif
     cs->exception_index = EXCP_NONE; /* mark handled to qemu */
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
-- 
2.17.1

[PATCH v1 15/21] target/riscv: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/riscv/cpu_helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 75d2ae3434..5050802e95 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -80,14 +80,17 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
 #if !defined(CONFIG_USER_ONLY)
     if (interrupt_request & CPU_INTERRUPT_HARD) {
+        qemu_mutex_lock_iothread();
         RISCVCPU *cpu = RISCV_CPU(cs);
         CPURISCVState *env = &cpu->env;
         int interruptno = riscv_cpu_local_irq_pending(env);
         if (interruptno >= 0) {
             cs->exception_index = RISCV_EXCP_INT_FLAG | interruptno;
             riscv_cpu_do_interrupt(cs);
+            qemu_mutex_unlock_iothread();
             return true;
         }
+        qemu_mutex_unlock_iothread();
     }
 #endif
     return false;
@@ -822,6 +825,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
  */
 void riscv_cpu_do_interrupt(CPUState *cs)
 {
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 #if !defined(CONFIG_USER_ONLY)
 
     RISCVCPU *cpu = RISCV_CPU(cs);
@@ -982,4 +989,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
 
 #endif
     cs->exception_index = EXCP_NONE; /* mark handled to qemu */
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
-- 
2.17.1

[PATCH v1 16/21] target/rx: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/rx/helper.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/target/rx/helper.c b/target/rx/helper.c
index a6a337a311..a456b727ed 100644
--- a/target/rx/helper.c
+++ b/target/rx/helper.c
@@ -48,6 +48,10 @@ void rx_cpu_do_interrupt(CPUState *cs)
     CPURXState *env = &cpu->env;
     int do_irq = cs->interrupt_request & INT_FLAGS;
     uint32_t save_psw;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     env->in_sleep = 0;
 
@@ -117,6 +121,9 @@ void rx_cpu_do_interrupt(CPUState *cs)
                       (vec & 0xff), expname);
     }
     env->regs[0] = env->isp;
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool rx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
@@ -124,6 +131,7 @@ bool rx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     RXCPU *cpu = RXCPU(cs);
     CPURXState *env = &cpu->env;
     int accept = 0;
+    qemu_mutex_lock_iothread();
     /* hardware interrupt (Normal) */
     if ((interrupt_request & CPU_INTERRUPT_HARD) &&
         env->psw_i && (env->psw_ipl < env->req_ipl)) {
@@ -138,8 +146,10 @@ bool rx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
     }
     if (accept) {
         rx_cpu_do_interrupt(cs);
+        qemu_mutex_unlock_iothread();
         return true;
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/s390x/excp_helper.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
index dde7afc2f0..b215b4a4a7 100644
--- a/target/s390x/excp_helper.c
+++ b/target/s390x/excp_helper.c
@@ -470,7 +470,10 @@ void s390_cpu_do_interrupt(CPUState *cs)
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
     bool stopped = false;
-
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
     qemu_log_mask(CPU_LOG_INT, "%s: %d at psw=%" PRIx64 ":%" PRIx64 "\n",
                   __func__, cs->exception_index, env->psw.mask, env->psw.addr);
 
@@ -541,10 +544,14 @@ try_deliver:
         /* unhalt if we had a WAIT PSW somehwere in our injection chain */
         s390_cpu_unhalt(cpu);
     }
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+    qemu_mutex_lock_iothread();
     if (interrupt_request & CPU_INTERRUPT_HARD) {
         S390CPU *cpu = S390_CPU(cs);
         CPUS390XState *env = &cpu->env;
@@ -552,10 +559,12 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         if (env->ex_value) {
             /* Execution of the target insn is indivisible from
                the parent EXECUTE insn.  */
+            qemu_mutex_unlock_iothread();
             return false;
         }
         if (s390_cpu_has_int(cpu)) {
             s390_cpu_do_interrupt(cs);
+            qemu_mutex_unlock_iothread();
             return true;
         }
         if (env->psw.mask & PSW_MASK_WAIT) {
@@ -564,6 +573,7 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT);
         }
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
-- 
2.17.1

[PATCH v1 18/21] target/sh4: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/sh4/helper.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/sh4/helper.c b/target/sh4/helper.c
index 1e32365c75..c4d5b9a374 100644
--- a/target/sh4/helper.c
+++ b/target/sh4/helper.c
@@ -62,8 +62,11 @@ void superh_cpu_do_interrupt(CPUState *cs)
 {
     SuperHCPU *cpu = SUPERH_CPU(cs);
     CPUSH4State *env = &cpu->env;
-    int do_irq = cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
-    int do_exp, irq_vector = cs->exception_index;
+    int do_irq;
+    int do_exp, irq_vector;
+    qemu_mutex_lock_iothread();
+    do_irq = cpu_interrupt_request(cs) & CPU_INTERRUPT_HARD;
+    irq_vector = cs->exception_index;
 
     /* prioritize exceptions over interrupts */
 
@@ -79,9 +82,11 @@ void superh_cpu_do_interrupt(CPUState *cs)
                should be loaded with the kernel entry point.
                qemu_system_reset_request takes care of that.  */
             qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+            qemu_mutex_unlock_iothread();
             return;
         }
         if (do_irq && !env->in_sleep) {
+            qemu_mutex_unlock_iothread();
             return; /* masked */
         }
     }
@@ -91,6 +96,7 @@ void superh_cpu_do_interrupt(CPUState *cs)
         irq_vector = sh_intc_get_pending_vector(env->intc_handle,
 						(env->sr >> 4) & 0xf);
         if (irq_vector == -1) {
+            qemu_mutex_unlock_iothread();
             return; /* masked */
 	}
     }
@@ -180,14 +186,17 @@ void superh_cpu_do_interrupt(CPUState *cs)
             env->pc = env->vbr + 0x100;
             break;
         }
+        qemu_mutex_unlock_iothread();
         return;
     }
 
     if (do_irq) {
         env->intevt = irq_vector;
         env->pc = env->vbr + 0x600;
+        qemu_mutex_unlock_iothread();
         return;
     }
+    qemu_mutex_unlock_iothread();
 }
 
 static void update_itlb_use(CPUSH4State * env, int itlbnb)
-- 
2.17.1

[PATCH v1 19/21] target/sparc: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/sparc/cpu.c          |  3 +++
 target/sparc/int32_helper.c | 13 ++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index 20c7c0c434..13b5a038e8 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -79,6 +79,7 @@ static void sparc_cpu_reset(DeviceState *dev)
 
 static bool sparc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+    qemu_mutex_lock_iothread();
     if (interrupt_request & CPU_INTERRUPT_HARD) {
         SPARCCPU *cpu = SPARC_CPU(cs);
         CPUSPARCState *env = &cpu->env;
@@ -90,10 +91,12 @@ static bool sparc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
             if (type != TT_EXTINT || cpu_pil_allowed(env, pil)) {
                 cs->exception_index = env->interrupt_index;
                 sparc_cpu_do_interrupt(cs);
+                qemu_mutex_unlock_iothread();
                 return true;
             }
         }
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
 
diff --git a/target/sparc/int32_helper.c b/target/sparc/int32_helper.c
index 9a71e1abd8..3940e945ed 100644
--- a/target/sparc/int32_helper.c
+++ b/target/sparc/int32_helper.c
@@ -69,7 +69,12 @@ void sparc_cpu_do_interrupt(CPUState *cs)
 {
     SPARCCPU *cpu = SPARC_CPU(cs);
     CPUSPARCState *env = &cpu->env;
-    int cwp, intno = cs->exception_index;
+    int cwp, intno;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
+    intno = cs->exception_index;
 
     /* Compute PSR before exposing state.  */
     if (env->cc_op != CC_OP_FLAGS) {
@@ -115,6 +120,9 @@ void sparc_cpu_do_interrupt(CPUState *cs)
                           "Error state",
                       cs->exception_index, excp_name_str(cs->exception_index));
         }
+        if (bql) {
+            qemu_mutex_unlock_iothread();
+        }
         return;
     }
 #endif
@@ -136,6 +144,9 @@ void sparc_cpu_do_interrupt(CPUState *cs)
         env->qemu_irq_ack(env, env->irq_manager, intno);
     }
 #endif
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
2.17.1

[PATCH v1 20/21] target/unicore32: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/unicore32/helper.c  | 3 +++
 target/unicore32/softmmu.c | 7 +++++++
 2 files changed, 10 insertions(+)

diff --git a/target/unicore32/helper.c b/target/unicore32/helper.c
index 54c26871fe..d79284d224 100644
--- a/target/unicore32/helper.c
+++ b/target/unicore32/helper.c
@@ -169,6 +169,7 @@ void helper_cp1_putc(target_ulong regval)
 
 bool uc32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
 {
+    qemu_mutex_lock_iothread();
     if (interrupt_request & CPU_INTERRUPT_HARD) {
         UniCore32CPU *cpu = UNICORE32_CPU(cs);
         CPUUniCore32State *env = &cpu->env;
@@ -176,8 +177,10 @@ bool uc32_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
         if (!(env->uncached_asr & ASR_I)) {
             cs->exception_index = UC32_EXCP_INTR;
             uc32_cpu_do_interrupt(cs);
+            qemu_mutex_unlock_iothread();
             return true;
         }
     }
+    qemu_mutex_unlock_iothread();
     return false;
 }
diff --git a/target/unicore32/softmmu.c b/target/unicore32/softmmu.c
index 9660bd2a27..ca9b92aad0 100644
--- a/target/unicore32/softmmu.c
+++ b/target/unicore32/softmmu.c
@@ -81,6 +81,10 @@ void uc32_cpu_do_interrupt(CPUState *cs)
     CPUUniCore32State *env = &cpu->env;
     uint32_t addr;
     int new_mode;
+    bool bql = !qemu_mutex_iothread_locked();
+    if (bql) {
+        qemu_mutex_lock_iothread();
+    }
 
     switch (cs->exception_index) {
     case UC32_EXCP_PRIV:
@@ -118,6 +122,9 @@ void uc32_cpu_do_interrupt(CPUState *cs)
     env->regs[30] = env->regs[31];
     env->regs[31] = addr;
     cpu_interrupt_request_or(cs, CPU_INTERRUPT_EXITTB);
+    if (bql) {
+        qemu_mutex_unlock_iothread();
+    }
 }
 
 static int get_phys_addr_ucv2(CPUUniCore32State *env, uint32_t address,
-- 
2.17.1

[PATCH v1 21/21] target/xtensa: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

This is part of a series of changes to remove the implied BQL
from the common code of cpu_handle_interrupt and
cpu_handle_exception.  As part of removing the implied BQL
from the common code, we are pushing the BQL holding
down into the per-arch implementation functions of
do_interrupt and cpu_exec_interrupt.

The purpose of this set of changes is to set the groundwork
so that an arch could move towards removing
the BQL from the cpu_handle_interrupt/exception paths.

This approach was suggested by Paolo Bonzini.
For reference, here are two key posts in the discussion, explaining
the reasoning/benefits of this approach.
https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html

Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
 target/xtensa/exc_helper.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/xtensa/exc_helper.c b/target/xtensa/exc_helper.c
index 01d1e56feb..fd33a56847 100644
--- a/target/xtensa/exc_helper.c
+++ b/target/xtensa/exc_helper.c
@@ -200,6 +200,7 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
     XtensaCPU *cpu = XTENSA_CPU(cs);
     CPUXtensaState *env = &cpu->env;
 
+    qemu_mutex_lock_iothread();
     if (cs->exception_index == EXC_IRQ) {
         qemu_log_mask(CPU_LOG_INT,
                       "%s(EXC_IRQ) level = %d, cintlevel = %d, "
@@ -252,6 +253,7 @@ void xtensa_cpu_do_interrupt(CPUState *cs)
         break;
     }
     check_interrupts(env);
+    qemu_mutex_unlock_iothread();
 }
 #else
 void xtensa_cpu_do_interrupt(CPUState *cs)
-- 
2.17.1

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Richard Henderson]

On 8/5/20 11:12 AM, Robert Foley wrote:
> This change removes the implied BQL from the cpu_handle_interrupt,
> and cpu_handle_exception paths. This BQL acquire is being pushed
> down into the per arch implementation.
> 
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
>  accel/tcg/cpu-exec.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 80d0e649b2..8e2bfd97a1 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>  #else
>          if (replay_exception()) {
>              CPUClass *cc = CPU_GET_CLASS(cpu);
> -            qemu_mutex_lock_iothread();
>              cc->do_interrupt(cpu);
> -            qemu_mutex_unlock_iothread();
>              cpu->exception_index = -1;
>  

This patch is not bisectable.  The removal of the lock here needs to happen at
the end, or something.


r~

Re: [PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Richard Henderson]

On 8/5/20 11:12 AM, Robert Foley wrote:
> @@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
>  {
>      AlphaCPU *cpu = ALPHA_CPU(cs);
>      CPUAlphaState *env = &cpu->env;
> -    int i = cs->exception_index;
> -
> +    int i;
> +    bool bql = !qemu_mutex_iothread_locked();
> +    if (bql) {
> +        qemu_mutex_lock_iothread();
> +    }

Why does this patch for alpha need to check qemu_mutex_iothread_locked and the
next patch for arm does not?


r~

Re: [PATCH v1 02/21] target/alpha: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

On Wed, 5 Aug 2020 at 15:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/5/20 11:12 AM, Robert Foley wrote:
> > @@ -299,8 +299,12 @@ void alpha_cpu_do_interrupt(CPUState *cs)
> >  {
> >      AlphaCPU *cpu = ALPHA_CPU(cs);
> >      CPUAlphaState *env = &cpu->env;
> > -    int i = cs->exception_index;
> > -
> > +    int i;
> > +    bool bql = !qemu_mutex_iothread_locked();
> > +    if (bql) {
> > +        qemu_mutex_lock_iothread();
> > +    }
>
> Why does this patch for alpha need to check qemu_mutex_iothread_locked and the
> next patch for arm does not?
>

In alpha (and arm) the do_interrupt function can be called separately or by
cpu_exec_interrupt.  In the case where do_interrupt gets called separately
it needs to take the BQL (bql == true).
In the case where cpu_exec_interrupt is holding the BQL, and calls do_interrupt,
do_interrupt needs to check qemu_mutex_iothread_locked, and in this case not get
the lock (bql == false).

The next patch for arm, checks qemu_mutex_iothread_locked in its do_interrupt
function, but not in its cpu_exec_interrupt function, the same pattern
as for alpha.

Thanks & Regards,
-Rob

>
> r~

Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Cornelia Huck]

On Wed,  5 Aug 2020 14:12:59 -0400
Robert Foley <robert.foley@linaro.org> wrote:

> This is part of a series of changes to remove the implied BQL
> from the common code of cpu_handle_interrupt and
> cpu_handle_exception.  As part of removing the implied BQL
> from the common code, we are pushing the BQL holding
> down into the per-arch implementation functions of
> do_interrupt and cpu_exec_interrupt.
> 
> The purpose of this set of changes is to set the groundwork
> so that an arch could move towards removing
> the BQL from the cpu_handle_interrupt/exception paths.
> 
> This approach was suggested by Paolo Bonzini.
> For reference, here are two key posts in the discussion, explaining
> the reasoning/benefits of this approach.
> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html
> 
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
>  target/s390x/excp_helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c
> index dde7afc2f0..b215b4a4a7 100644
> --- a/target/s390x/excp_helper.c
> +++ b/target/s390x/excp_helper.c
> @@ -470,7 +470,10 @@ void s390_cpu_do_interrupt(CPUState *cs)
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
>      bool stopped = false;
> -
> +    bool bql = !qemu_mutex_iothread_locked();
> +    if (bql) {
> +        qemu_mutex_lock_iothread();
> +    }

I'm not sure I like that conditional locking. Can we instead create
__s390_cpu_do_interrupt() or so, move the meat of this function there,
take the bql unconditionally here, and...

>      qemu_log_mask(CPU_LOG_INT, "%s: %d at psw=%" PRIx64 ":%" PRIx64 "\n",
>                    __func__, cs->exception_index, env->psw.mask, env->psw.addr);
>  
> @@ -541,10 +544,14 @@ try_deliver:
>          /* unhalt if we had a WAIT PSW somehwere in our injection chain */
>          s390_cpu_unhalt(cpu);
>      }
> +    if (bql) {
> +        qemu_mutex_unlock_iothread();
> +    }
>  }
>  
>  bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
> +    qemu_mutex_lock_iothread();
>      if (interrupt_request & CPU_INTERRUPT_HARD) {
>          S390CPU *cpu = S390_CPU(cs);
>          CPUS390XState *env = &cpu->env;
> @@ -552,10 +559,12 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>          if (env->ex_value) {
>              /* Execution of the target insn is indivisible from
>                 the parent EXECUTE insn.  */
> +            qemu_mutex_unlock_iothread();
>              return false;
>          }
>          if (s390_cpu_has_int(cpu)) {
>              s390_cpu_do_interrupt(cs);

...call __s390_cpu_do_interrupt() here?

> +            qemu_mutex_unlock_iothread();
>              return true;
>          }
>          if (env->psw.mask & PSW_MASK_WAIT) {
> @@ -564,6 +573,7 @@ bool s390_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>              cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HALT);
>          }
>      }
> +    qemu_mutex_unlock_iothread();
>      return false;
>  }
>  

Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Paolo Bonzini]

On 06/08/20 10:59, Cornelia Huck wrote:
>>      bool stopped = false;
>> -
>> +    bool bql = !qemu_mutex_iothread_locked();
>> +    if (bql) {
>> +        qemu_mutex_lock_iothread();
>> +    }
> I'm not sure I like that conditional locking. Can we instead create
> __s390_cpu_do_interrupt() or so, move the meat of this function there,
> take the bql unconditionally here, and...
> 

Agreed, except the usual convention would be s390_cpu_do_interrupt_locked.

Paolo

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Paolo Bonzini]

On 05/08/20 21:18, Richard Henderson wrote:
> On 8/5/20 11:12 AM, Robert Foley wrote:
>> This change removes the implied BQL from the cpu_handle_interrupt,
>> and cpu_handle_exception paths. This BQL acquire is being pushed
>> down into the per arch implementation.
>>
>> Signed-off-by: Robert Foley <robert.foley@linaro.org>
>> ---
>>  accel/tcg/cpu-exec.c | 19 +++++++++++--------
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
>> index 80d0e649b2..8e2bfd97a1 100644
>> --- a/accel/tcg/cpu-exec.c
>> +++ b/accel/tcg/cpu-exec.c
>> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
>>  #else
>>          if (replay_exception()) {
>>              CPUClass *cc = CPU_GET_CLASS(cpu);
>> -            qemu_mutex_lock_iothread();
>>              cc->do_interrupt(cpu);
>> -            qemu_mutex_unlock_iothread();
>>              cpu->exception_index = -1;
>>  
> 
> This patch is not bisectable.  The removal of the lock here needs to happen at
> the end, or something.

Indeed the series should be structured like this:

1) rename all *_do_interrupt functions to *_do_interrupt_locked

2) add back *_do_interrupt that takes the BQL and calls
*_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
cpu-exec.c

3) modify the cpu_mutex and BQL critical sections around
->cpu_exec_interrupt, so that the BQL critical section covers just the
call to ->cpu_exec_interrupt.  Document which fields are now covered by
cpu_mutex.

4/5) same as 1/2 for ->cpu_exec_interrupt

Patches 1/2 would be pretty large, but they're trivial to review just by
grepping for "->do_interrupt\s*=", and likewise for 4/5.

Thanks,

Paolo

Re: [PATCH v1 17/21] target/s390x: add BQL to do_interrupt and cpu_exec_interrupt

[Alex Bennée]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 06/08/20 10:59, Cornelia Huck wrote:
>>>      bool stopped = false;
>>> -
>>> +    bool bql = !qemu_mutex_iothread_locked();
>>> +    if (bql) {
>>> +        qemu_mutex_lock_iothread();
>>> +    }
>> I'm not sure I like that conditional locking. Can we instead create
>> __s390_cpu_do_interrupt() or so, move the meat of this function there,
>> take the bql unconditionally here, and...
>> 
>
> Agreed, except the usual convention would be
> s390_cpu_do_interrupt_locked.

We should probably document this convention in CODING_STYLE.rst/Naming

>
> Paolo


-- 
Alex Bennée

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 05/08/20 21:18, Richard Henderson wrote:
> > On 8/5/20 11:12 AM, Robert Foley wrote:
> >> This change removes the implied BQL from the cpu_handle_interrupt,
> >> and cpu_handle_exception paths. This BQL acquire is being pushed
> >> down into the per arch implementation.
> >>
> >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> >> ---
> >>  accel/tcg/cpu-exec.c | 19 +++++++++++--------
> >>  1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> >> index 80d0e649b2..8e2bfd97a1 100644
> >> --- a/accel/tcg/cpu-exec.c
> >> +++ b/accel/tcg/cpu-exec.c
> >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> >>  #else
> >>          if (replay_exception()) {
> >>              CPUClass *cc = CPU_GET_CLASS(cpu);
> >> -            qemu_mutex_lock_iothread();
> >>              cc->do_interrupt(cpu);
> >> -            qemu_mutex_unlock_iothread();
> >>              cpu->exception_index = -1;
> >>
> >
> > This patch is not bisectable.  The removal of the lock here needs to happen at
> > the end, or something.
>
> Indeed the series should be structured like this:
>
> 1) rename all *_do_interrupt functions to *_do_interrupt_locked
>
> 2) add back *_do_interrupt that takes the BQL and calls
> *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> cpu-exec.c
>
> 3) modify the cpu_mutex and BQL critical sections around
> ->cpu_exec_interrupt, so that the BQL critical section covers just the
> call to ->cpu_exec_interrupt.  Document which fields are now covered by
> cpu_mutex.
>
> 4/5) same as 1/2 for ->cpu_exec_interrupt
>
> Patches 1/2 would be pretty large, but they're trivial to review just by
> grepping for "->do_interrupt\s*=", and likewise for 4/5.
>

Thanks for the details !

It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.

We will go in this direction.

Thanks,
-Rob

> Thanks,
>
> Paolo
>

Re: [PATCH v1 04/21] target/avr: add BQL to do_interrupt and cpu_exec_interrupt

[Michael Rolnik]

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

Hi Robert.

I am sorry but how can I apply it? following this what I get

error: patch failed: accel/tcg/cpu-exec.c:558
error: accel/tcg/cpu-exec.c: patch does not apply
error: patch failed: target/arm/helper.c:9808
error: target/arm/helper.c: patch does not apply
error: patch failed: target/ppc/excp_helper.c:1056
error: target/ppc/excp_helper.c: patch does not apply
error: patch failed: target/sh4/helper.c:62
error: target/sh4/helper.c: patch does not apply
error: patch failed: target/unicore32/softmmu.c:118
error: target/unicore32/softmmu.c: patch does not apply



On Wed, Aug 5, 2020 at 9:17 PM Robert Foley <robert.foley@linaro.org> wrote:

> This is part of a series of changes to remove the implied BQL
> from the common code of cpu_handle_interrupt and
> cpu_handle_exception.  As part of removing the implied BQL
> from the common code, we are pushing the BQL holding
> down into the per-arch implementation functions of
> do_interrupt and cpu_exec_interrupt.
>
> The purpose of this set of changes is to set the groundwork
> so that an arch could move towards removing
> the BQL from the cpu_handle_interrupt/exception paths.
>
> This approach was suggested by Paolo Bonzini.
> For reference, here are two key posts in the discussion, explaining
> the reasoning/benefits of this approach.
> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
>  target/avr/helper.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/target/avr/helper.c b/target/avr/helper.c
> index d96d14372b..f0d625c195 100644
> --- a/target/avr/helper.c
> +++ b/target/avr/helper.c
> @@ -30,6 +30,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>      CPUClass *cc = CPU_GET_CLASS(cs);
>      AVRCPU *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
> +    qemu_mutex_lock_iothread();
>
>      if (interrupt_request & CPU_INTERRUPT_RESET) {
>          if (cpu_interrupts_enabled(env)) {
> @@ -53,6 +54,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int
> interrupt_request)
>              ret = true;
>          }
>      }
> +    qemu_mutex_unlock_iothread();
>      return ret;
>  }
>
> @@ -61,10 +63,15 @@ void avr_cpu_do_interrupt(CPUState *cs)
>      AVRCPU *cpu = AVR_CPU(cs);
>      CPUAVRState *env = &cpu->env;
>
> -    uint32_t ret = env->pc_w;
> +    uint32_t ret;
>      int vector = 0;
>      int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
>      int base = 0;
> +    bool bql = !qemu_mutex_iothread_locked();
> +    if (bql) {
> +        qemu_mutex_lock_iothread();
> +    }
> +    ret = env->pc_w;
>
>      if (cs->exception_index == EXCP_RESET) {
>          vector = 0;
> @@ -87,6 +94,9 @@ void avr_cpu_do_interrupt(CPUState *cs)
>      env->sregI = 0; /* clear Global Interrupt Flag */
>
>      cs->exception_index = -1;
> +    if (bql) {
> +        qemu_mutex_unlock_iothread();
> +    }
>  }
>
>  int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
> --
> 2.17.1
>
>

-- 
Best Regards,
Michael Rolnik

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

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Paolo Bonzini]

On 06/08/20 18:11, Robert Foley wrote:
>> Indeed the series should be structured like this:
>>
>> 1) rename all *_do_interrupt functions to *_do_interrupt_locked
>>
>> 2) add back *_do_interrupt that takes the BQL and calls
>> *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
>> cpu-exec.c
>>
>> 3) modify the cpu_mutex and BQL critical sections around
>> ->cpu_exec_interrupt, so that the BQL critical section covers just the
>> call to ->cpu_exec_interrupt.  Document which fields are now covered by
>> cpu_mutex.
>>
>> 4/5) same as 1/2 for ->cpu_exec_interrupt
>>
>> Patches 1/2 would be pretty large, but they're trivial to review just by
>> grepping for "->do_interrupt\s*=", and likewise for 4/5.
>>
> 
> Thanks for the details !
> 
> It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.

No, five patches. :)

Paolo

Re: [PATCH v1 04/21] target/avr: add BQL to do_interrupt and cpu_exec_interrupt

[Robert Foley]

On Thu, 6 Aug 2020 at 14:37, Michael Rolnik <mrolnik@gmail.com> wrote:
>
> Hi Robert.
>
> I am sorry but how can I apply it? following this what I get
>
> error: patch failed: accel/tcg/cpu-exec.c:558
> error: accel/tcg/cpu-exec.c: patch does not apply
> error: patch failed: target/arm/helper.c:9808
> error: target/arm/helper.c: patch does not apply
> error: patch failed: target/ppc/excp_helper.c:1056
> error: target/ppc/excp_helper.c: patch does not apply
> error: patch failed: target/sh4/helper.c:62
> error: target/sh4/helper.c: patch does not apply
> error: patch failed: target/unicore32/softmmu.c:118
> error: target/unicore32/softmmu.c: patch does not apply
>

Hi Michael,
This patch is based on the per-cpu locks patch series:
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg05314.html

Our current WIP tree for this interrupts patch is here:
https://github.com/rf972/qemu/commits/int_core_v1.4

Also, just so you know, based on the initial feedback we are going
to substantially change this series.

Another version will be sent out in a few days.

Thanks & Regards,
-Rob
>
>
> On Wed, Aug 5, 2020 at 9:17 PM Robert Foley <robert.foley@linaro.org> wrote:
>>
>> This is part of a series of changes to remove the implied BQL
>> from the common code of cpu_handle_interrupt and
>> cpu_handle_exception.  As part of removing the implied BQL
>> from the common code, we are pushing the BQL holding
>> down into the per-arch implementation functions of
>> do_interrupt and cpu_exec_interrupt.
>>
>> The purpose of this set of changes is to set the groundwork
>> so that an arch could move towards removing
>> the BQL from the cpu_handle_interrupt/exception paths.
>>
>> This approach was suggested by Paolo Bonzini.
>> For reference, here are two key posts in the discussion, explaining
>> the reasoning/benefits of this approach.
>> https://lists.gnu.org/archive/html/qemu-devel/2020-07/msg08731.html
>> https://lists.gnu.org/archive/html/qemu-devel/2020-08/msg00044.html
>>
>> Signed-off-by: Robert Foley <robert.foley@linaro.org>
>> ---
>>  target/avr/helper.c | 12 +++++++++++-
>>  1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/avr/helper.c b/target/avr/helper.c
>> index d96d14372b..f0d625c195 100644
>> --- a/target/avr/helper.c
>> +++ b/target/avr/helper.c
>> @@ -30,6 +30,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>      CPUClass *cc = CPU_GET_CLASS(cs);
>>      AVRCPU *cpu = AVR_CPU(cs);
>>      CPUAVRState *env = &cpu->env;
>> +    qemu_mutex_lock_iothread();
>>
>>      if (interrupt_request & CPU_INTERRUPT_RESET) {
>>          if (cpu_interrupts_enabled(env)) {
>> @@ -53,6 +54,7 @@ bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>>              ret = true;
>>          }
>>      }
>> +    qemu_mutex_unlock_iothread();
>>      return ret;
>>  }
>>
>> @@ -61,10 +63,15 @@ void avr_cpu_do_interrupt(CPUState *cs)
>>      AVRCPU *cpu = AVR_CPU(cs);
>>      CPUAVRState *env = &cpu->env;
>>
>> -    uint32_t ret = env->pc_w;
>> +    uint32_t ret;
>>      int vector = 0;
>>      int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
>>      int base = 0;
>> +    bool bql = !qemu_mutex_iothread_locked();
>> +    if (bql) {
>> +        qemu_mutex_lock_iothread();
>> +    }
>> +    ret = env->pc_w;
>>
>>      if (cs->exception_index == EXCP_RESET) {
>>          vector = 0;
>> @@ -87,6 +94,9 @@ void avr_cpu_do_interrupt(CPUState *cs)
>>      env->sregI = 0; /* clear Global Interrupt Flag */
>>
>>      cs->exception_index = -1;
>> +    if (bql) {
>> +        qemu_mutex_unlock_iothread();
>> +    }
>>  }
>>
>>  int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
>> --
>> 2.17.1
>>
>
>
> --
> Best Regards,
> Michael Rolnik

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

The comment around documenting the cpu_mutex fields and critical sections
got us thinking and revisiting our locking assumptions in cpu_handle_interrupt.

Initially we were thinking that removing the BQL from cpu_handle_interrupt
meant that we needed to replace it with the cpu mutex to protect the per cpu
data that is accessed like interrupt_request.  We are reconsidering this and
now thinking that the cpu mutex might not be needed here.

BQL is clearly needed to protect the critical section around the call to
->cpu_exec_interrupt.  What else is the BQL protecting in cpu_handle_interrupt
that we need to consider?  Are we missing anything here?

It's also worth mentioning that we did give this approach a try.
We tried out changes to cpu_handle_interrupt(),
dropping the BQL from all but around ->cpu_exec_interrupt, and not using the
cpu_mutex at all.  It seemed to be functional, passing all the tests that
we tried (so far). :)

Thanks,
-Rob

On Thu, 6 Aug 2020 at 12:11, Robert Foley <robert.foley@linaro.org> wrote:
>
> On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> > On 05/08/20 21:18, Richard Henderson wrote:
> > > On 8/5/20 11:12 AM, Robert Foley wrote:
> > >> This change removes the implied BQL from the cpu_handle_interrupt,
> > >> and cpu_handle_exception paths. This BQL acquire is being pushed
> > >> down into the per arch implementation.
> > >>
> > >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> > >> ---
> > >>  accel/tcg/cpu-exec.c | 19 +++++++++++--------
> > >>  1 file changed, 11 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > >> index 80d0e649b2..8e2bfd97a1 100644
> > >> --- a/accel/tcg/cpu-exec.c
> > >> +++ b/accel/tcg/cpu-exec.c
> > >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> > >>  #else
> > >>          if (replay_exception()) {
> > >>              CPUClass *cc = CPU_GET_CLASS(cpu);
> > >> -            qemu_mutex_lock_iothread();
> > >>              cc->do_interrupt(cpu);
> > >> -            qemu_mutex_unlock_iothread();
> > >>              cpu->exception_index = -1;
> > >>
> > >
> > > This patch is not bisectable.  The removal of the lock here needs to happen at
> > > the end, or something.
> >
> > Indeed the series should be structured like this:
> >
> > 1) rename all *_do_interrupt functions to *_do_interrupt_locked
> >
> > 2) add back *_do_interrupt that takes the BQL and calls
> > *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> > cpu-exec.c
> >
> > 3) modify the cpu_mutex and BQL critical sections around
> > ->cpu_exec_interrupt, so that the BQL critical section covers just the
> > call to ->cpu_exec_interrupt.  Document which fields are now covered by
> > cpu_mutex.
> >
> > 4/5) same as 1/2 for ->cpu_exec_interrupt
> >
> > Patches 1/2 would be pretty large, but they're trivial to review just by
> > grepping for "->do_interrupt\s*=", and likewise for 4/5.
> >
>
> Thanks for the details !
>
> It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.
>
> We will go in this direction.
>
> Thanks,
> -Rob
>
> > Thanks,
> >
> > Paolo
> >

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

We found cases where a few of the *_cpu_exec_interrupt per arch functions
call CPU class's cc->do_interrupt function pointer (for example
arm_cpu_exec_interrupt does this).

This is an issue because *_cpu_exec_interrupt will hold the BQL across the
call to cc->do_interrupt, and *_do_interrupt will also hold the BQL.

Most of the arches do not have this issue because they call the *_do_interrupt
function for that arch directly, and in those cases we will change to call
the function *_do_interrupt_locked.

We see a few possible solutions to this:

1) We could go back to the option of conditionalizing the BQL inside
   these few *_do_interrupt functions, only acquiring the BQL if it is not
   already held.  I counted 3 different arches that directly use the
->do_interrupt
   function from their *_cpu_exec_interrupt.

2) Another perhaps cleaner option is to add a new cpu class function
->do_interrupt_locked.
   This lets callers like *_cpu_exec_interrupt call to ->do_interrupt_locked
   with lock held and solves the issue without resorting to conditional locking.

   Another benefit we could gain from this approach is to simplify our solution
   overall by adding a common do_interrupt function.

   void cpu_common_do_interrupt(CPUState *cs)
   {
        CPUClass *cc = CPU_GET_CLASS(cpu);
        qemu_mutex_lock_iothread();
        cc->do_interrupt_locked(cpu);
        qemu_mutex_unlock_iothread();
    }
   cc->do_interrupt would be set to cpu_common_do_interrupt by default
in cpu_class_init.
   In other words, the base cpu class would handle holding the BQL for us,
   and we would not need to implement a new *_do_interrupt function
for each arch.

We are thinking that 2) would be a good option.

What are the opinions on these possible solutions?  Or are there other
solutions that we should consider here?

Thanks & Regards,
-Rob


On Thu, 6 Aug 2020 at 16:04, Robert Foley <robert.foley@linaro.org> wrote:
>
> The comment around documenting the cpu_mutex fields and critical sections
> got us thinking and revisiting our locking assumptions in cpu_handle_interrupt.
>
> Initially we were thinking that removing the BQL from cpu_handle_interrupt
> meant that we needed to replace it with the cpu mutex to protect the per cpu
> data that is accessed like interrupt_request.  We are reconsidering this and
> now thinking that the cpu mutex might not be needed here.
>
> BQL is clearly needed to protect the critical section around the call to
> ->cpu_exec_interrupt.  What else is the BQL protecting in cpu_handle_interrupt
> that we need to consider?  Are we missing anything here?
>
> It's also worth mentioning that we did give this approach a try.
> We tried out changes to cpu_handle_interrupt(),
> dropping the BQL from all but around ->cpu_exec_interrupt, and not using the
> cpu_mutex at all.  It seemed to be functional, passing all the tests that
> we tried (so far). :)
>
> Thanks,
> -Rob
>
> On Thu, 6 Aug 2020 at 12:11, Robert Foley <robert.foley@linaro.org> wrote:
> >
> > On Thu, 6 Aug 2020 at 05:22, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > >
> > > On 05/08/20 21:18, Richard Henderson wrote:
> > > > On 8/5/20 11:12 AM, Robert Foley wrote:
> > > >> This change removes the implied BQL from the cpu_handle_interrupt,
> > > >> and cpu_handle_exception paths. This BQL acquire is being pushed
> > > >> down into the per arch implementation.
> > > >>
> > > >> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> > > >> ---
> > > >>  accel/tcg/cpu-exec.c | 19 +++++++++++--------
> > > >>  1 file changed, 11 insertions(+), 8 deletions(-)
> > > >>
> > > >> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> > > >> index 80d0e649b2..8e2bfd97a1 100644
> > > >> --- a/accel/tcg/cpu-exec.c
> > > >> +++ b/accel/tcg/cpu-exec.c
> > > >> @@ -517,9 +517,7 @@ static inline bool cpu_handle_exception(CPUState *cpu, int *ret)
> > > >>  #else
> > > >>          if (replay_exception()) {
> > > >>              CPUClass *cc = CPU_GET_CLASS(cpu);
> > > >> -            qemu_mutex_lock_iothread();
> > > >>              cc->do_interrupt(cpu);
> > > >> -            qemu_mutex_unlock_iothread();
> > > >>              cpu->exception_index = -1;
> > > >>
> > > >
> > > > This patch is not bisectable.  The removal of the lock here needs to happen at
> > > > the end, or something.
> > >
> > > Indeed the series should be structured like this:
> > >
> > > 1) rename all *_do_interrupt functions to *_do_interrupt_locked
> > >
> > > 2) add back *_do_interrupt that takes the BQL and calls
> > > *_do_interrupt_locked, point ->do_interrupt to it, remove the BQL from
> > > cpu-exec.c
> > >
> > > 3) modify the cpu_mutex and BQL critical sections around
> > > ->cpu_exec_interrupt, so that the BQL critical section covers just the
> > > call to ->cpu_exec_interrupt.  Document which fields are now covered by
> > > cpu_mutex.
> > >
> > > 4/5) same as 1/2 for ->cpu_exec_interrupt
> > >
> > > Patches 1/2 would be pretty large, but they're trivial to review just by
> > > grepping for "->do_interrupt\s*=", and likewise for 4/5.
> > >
> >
> > Thanks for the details !
> >
> > It seems like we will have 3 separate patches for this series, 1/2, 3, and 4/5.
> >
> > We will go in this direction.
> >
> > Thanks,
> > -Rob
> >
> > > Thanks,
> > >
> > > Paolo
> > >

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Paolo Bonzini]

On 08/08/20 00:18, Robert Foley wrote:
> 2) Another perhaps cleaner option is to add a new cpu class function
> ->do_interrupt_locked.
>    This lets callers like *_cpu_exec_interrupt call to ->do_interrupt_locked
>    with lock held and solves the issue without resorting to conditional locking.
> 
>    Another benefit we could gain from this approach is to simplify our solution
>    overall by adding a common do_interrupt function.
> 
>    void cpu_common_do_interrupt(CPUState *cs)
>    {
>         CPUClass *cc = CPU_GET_CLASS(cpu);
>         qemu_mutex_lock_iothread();
>         cc->do_interrupt_locked(cpu);
>         qemu_mutex_unlock_iothread();
>     }
>    cc->do_interrupt would be set to cpu_common_do_interrupt by default
> in cpu_class_init.
>    In other words, the base cpu class would handle holding the BQL for us,
>    and we would not need to implement a new *_do_interrupt function
> for each arch.
> 
> We are thinking that 2) would be a good option.

Yes, it is.  The only slight complication is that you'd have both
->do_interrupt and ->do_interrupt_locked so you probably should add some
consistency check, for example

    /*
     * cc->do_interrupt_locked should only be needed if
     * the class uses cpu_common_do_interrupt.
     */
    assert(cc->do_interrupt == cpu_common_do_interrupt ||
           !cc->do_interrupt_locked);

Therefore, a variant is to add ->do_interrupt_locked to ARMCPUClass and
CRISCPUClass (target/avr/helper.c can just call
avr_cpu_do_interrupt_locked, because that's the only value that
cc->do_interrupt can have).  Then ARM and CRIS can have a do_interrupt
like you wrote above:

void arm_do_interrupt(CPUState *cs)
{
    ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs);
    qemu_mutex_lock_iothread();
    acc->do_interrupt_locked(cpu);
    qemu_mutex_unlock_iothread();
}

with a small duplication between ARM and CRIS but on the other hand a
simpler definition of the common CPUClass.

Paolo

Re: [PATCH v1 01/21] accel/tcg: Change interrupt/exception handling to remove implied BQL

[Robert Foley]

On Sat, 8 Aug 2020 at 08:00, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > We are thinking that 2) would be a good option.
>
> Yes, it is.  The only slight complication is that you'd have both
> ->do_interrupt and ->do_interrupt_locked so you probably should add some
> consistency check, for example
>
>     /*
>      * cc->do_interrupt_locked should only be needed if
>      * the class uses cpu_common_do_interrupt.
>      */
>     assert(cc->do_interrupt == cpu_common_do_interrupt ||
>            !cc->do_interrupt_locked);
>
> Therefore, a variant is to add ->do_interrupt_locked to ARMCPUClass and
> CRISCPUClass (target/avr/helper.c can just call
> avr_cpu_do_interrupt_locked, because that's the only value that
> cc->do_interrupt can have).  Then ARM and CRIS can have a do_interrupt
> like you wrote above:
>
> void arm_do_interrupt(CPUState *cs)
> {
>     ARMCPUClass *acc = ARM_CPU_GET_CLASS(cs);
>     qemu_mutex_lock_iothread();
>     acc->do_interrupt_locked(cpu);
>     qemu_mutex_unlock_iothread();
> }
>
> with a small duplication between ARM and CRIS but on the other hand a
> simpler definition of the common CPUClass.


Thanks for all the details! It sounds like a good approach and we will
move forward with it.

Thanks & Regards,
-Rob

>
> Paolo
>