diff mbox series

[v5,10/14] cpus: add handle_interrupt to the CpusAccel interface

Message ID 20200812183250.9221-11-cfontana@suse.de (mailing list archive)
State New, archived
Headers show
Series QEMU cpus.c refactoring part2 | expand

Commit Message

Claudio Fontana Aug. 12, 2020, 6:32 p.m. UTC 
kvm: uses the generic handler
qtest: uses the generic handler
whpx: changed to use the generic handler (identical implementation)
hax: changed to use the generic handler (identical implementation)
hvf: changed to use the generic handler (identical implementation)
tcg: adapt tcg-cpus to point to the tcg-specific handler

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 accel/tcg/tcg-all.c    | 26 --------------------------
 accel/tcg/tcg-cpus.c   | 28 ++++++++++++++++++++++++++++
 hw/core/cpu.c          | 13 -------------
 include/hw/core/cpu.h  | 14 --------------
 include/sysemu/cpus.h  |  2 ++
 softmmu/cpus.c         | 18 ++++++++++++++++++
 target/i386/hax-all.c  | 10 ----------
 target/i386/hvf/hvf.c  |  9 ---------
 target/i386/whpx-all.c | 10 ----------
 9 files changed, 48 insertions(+), 82 deletions(-)

Comments

Richard Henderson Aug. 14, 2020, 9:01 p.m. UTC | #1 
On 8/12/20 11:32 AM, Claudio Fontana wrote:
> +static void generic_handle_interrupt(CPUState *cpu, int mask)
> +{
> +    cpu->interrupt_request |= mask;
> +
> +    if (!qemu_cpu_is_self(cpu)) {
> +        qemu_cpu_kick(cpu);
> +    }
> +}
> +
> +void cpu_interrupt(CPUState *cpu, int mask)
> +{
> +    if (cpus_accel && cpus_accel->handle_interrupt) {
> +        cpus_accel->handle_interrupt(cpu, mask);
> +    } else {
> +        generic_handle_interrupt(cpu, mask);
> +    }
> +}

First, by this point you have converted all of the accelerators, so I would
expect cpus_accel to always be non-null.  I would expect a patch immediately
preceding this one to place an assert to that effect somewhere in the startup
code, and to remove all of the checks.

Second, I would prefer that all methods be non-null, so that you don't need to
check that either.  This patch would add generic_handle_interrupt (perhaps
named cpus_accel_default_handle_interrupt declared in sysemu/cpus.h?) to the
CpusAccel structure of all except TCG.

Similarly for all other methods that are checking non-null-ness of the method
pointer.  Perhaps assert non-null for each method in cpus_register_accel().


r~
Roman Bolshakov Aug. 17, 2020, 12:31 p.m. UTC | #2 
On Fri, Aug 14, 2020 at 02:01:01PM -0700, Richard Henderson wrote:
> On 8/12/20 11:32 AM, Claudio Fontana wrote:
> > +static void generic_handle_interrupt(CPUState *cpu, int mask)
> > +{
> > +    cpu->interrupt_request |= mask;
> > +
> > +    if (!qemu_cpu_is_self(cpu)) {
> > +        qemu_cpu_kick(cpu);
> > +    }
> > +}
> > +
> > +void cpu_interrupt(CPUState *cpu, int mask)
> > +{
> > +    if (cpus_accel && cpus_accel->handle_interrupt) {
> > +        cpus_accel->handle_interrupt(cpu, mask);
> > +    } else {
> > +        generic_handle_interrupt(cpu, mask);
> > +    }
> > +}
> 
> First, by this point you have converted all of the accelerators, so I would
> expect cpus_accel to always be non-null.  I would expect a patch immediately
> preceding this one to place an assert to that effect somewhere in the startup
> code, and to remove all of the checks.
> 
> Second, I would prefer that all methods be non-null, so that you don't need to
> check that either.  This patch would add generic_handle_interrupt (perhaps
> named cpus_accel_default_handle_interrupt declared in sysemu/cpus.h?) to the
> CpusAccel structure of all except TCG.
> 
> Similarly for all other methods that are checking non-null-ness of the method
> pointer.  Perhaps assert non-null for each method in cpus_register_accel().
> 
> 

I concur with that. It's similar to my comment to the previous revision
of the series.

Regards,
Roman
Claudio Fontana Aug. 19, 2020, 1:50 p.m. UTC | #3 
On 8/14/20 11:01 PM, Richard Henderson wrote:
> On 8/12/20 11:32 AM, Claudio Fontana wrote:
>> +static void generic_handle_interrupt(CPUState *cpu, int mask)
>> +{
>> +    cpu->interrupt_request |= mask;
>> +
>> +    if (!qemu_cpu_is_self(cpu)) {
>> +        qemu_cpu_kick(cpu);
>> +    }
>> +}
>> +
>> +void cpu_interrupt(CPUState *cpu, int mask)
>> +{
>> +    if (cpus_accel && cpus_accel->handle_interrupt) {
>> +        cpus_accel->handle_interrupt(cpu, mask);
>> +    } else {
>> +        generic_handle_interrupt(cpu, mask);
>> +    }
>> +}
> 
> First, by this point you have converted all of the accelerators, so I would
> expect cpus_accel to always be non-null.  I would expect a patch immediately
> preceding this one to place an assert to that effect somewhere in the startup
> code, and to remove all of the checks.

Ok.

> 
> Second, I would prefer that all methods be non-null, so that you don't need to
> check that either.  

This contrasts a bit with the previous comments by Paolo to try to express only the "interesting" bits in the CpusAccel struct in each module.


> This patch would add generic_handle_interrupt (perhaps
> named cpus_accel_default_handle_interrupt declared in sysemu/cpus.h?) to the
> CpusAccel structure of all except TCG.
> 
> Similarly for all other methods that are checking non-null-ness of the method
> pointer.  Perhaps assert non-null for each method in cpus_register_accel().
> 
> 
> r~
> 

That was my initial approach in very old versions of the series (all fields as explicit and mandatory),
I changed direction there due to Paolo's comments. For me both solutions are fine, each has its merits and downsides.

Thanks,

Claudio
diff mbox series

Patch

diff --git a/accel/tcg/tcg-all.c b/accel/tcg/tcg-all.c
index 01957b130d..af9bf5c5bb 100644
--- a/accel/tcg/tcg-all.c
+++ b/accel/tcg/tcg-all.c
@@ -47,31 +47,6 @@  typedef struct TCGState {
 #define TCG_STATE(obj) \
         OBJECT_CHECK(TCGState, (obj), TYPE_TCG_ACCEL)
 
-/* mask must never be zero, except for A20 change call */
-static void tcg_handle_interrupt(CPUState *cpu, int mask)
-{
-    int old_mask;
-    g_assert(qemu_mutex_iothread_locked());
-
-    old_mask = cpu->interrupt_request;
-    cpu->interrupt_request |= mask;
-
-    /*
-     * If called from iothread context, wake the target cpu in
-     * case its halted.
-     */
-    if (!qemu_cpu_is_self(cpu)) {
-        qemu_cpu_kick(cpu);
-    } else {
-        atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
-        if (icount_enabled() &&
-            !cpu->can_do_io
-            && (mask & ~old_mask) != 0) {
-            cpu_abort(cpu, "Raised interrupt while not in I/O function");
-        }
-    }
-}
-
 /*
  * We default to false if we know other options have been enabled
  * which are currently incompatible with MTTCG. Otherwise when each
@@ -128,7 +103,6 @@  static int tcg_init(MachineState *ms)
     TCGState *s = TCG_STATE(current_accel());
 
     tcg_exec_init(s->tb_size * 1024 * 1024);
-    cpu_interrupt_handler = tcg_handle_interrupt;
     mttcg_enabled = s->mttcg_enabled;
     cpus_register_accel(&tcg_cpus);
 
diff --git a/accel/tcg/tcg-cpus.c b/accel/tcg/tcg-cpus.c
index c82d142523..465d9955de 100644
--- a/accel/tcg/tcg-cpus.c
+++ b/accel/tcg/tcg-cpus.c
@@ -533,9 +533,37 @@  static int64_t tcg_get_elapsed_ticks(void)
     return cpu_get_ticks();
 }
 
+/* mask must never be zero, except for A20 change call */
+static void tcg_handle_interrupt(CPUState *cpu, int mask)
+{
+    int old_mask;
+    g_assert(qemu_mutex_iothread_locked());
+
+    old_mask = cpu->interrupt_request;
+    cpu->interrupt_request |= mask;
+
+    /*
+     * If called from iothread context, wake the target cpu in
+     * case its halted.
+     */
+    if (!qemu_cpu_is_self(cpu)) {
+        qemu_cpu_kick(cpu);
+    } else {
+        atomic_set(&cpu_neg(cpu)->icount_decr.u16.high, -1);
+        if (icount_enabled() &&
+            !cpu->can_do_io
+            && (mask & ~old_mask) != 0) {
+            cpu_abort(cpu, "Raised interrupt while not in I/O function");
+        }
+    }
+}
+
 CpusAccel tcg_cpus = {
     .create_vcpu_thread = tcg_start_vcpu_thread,
     .kick_vcpu_thread = tcg_kick_vcpu_thread,
+
+    .handle_interrupt = tcg_handle_interrupt,
+
     .get_virtual_clock = tcg_get_virtual_clock,
     .get_elapsed_ticks = tcg_get_elapsed_ticks,
 };
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index b389a312df..06bec40042 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -35,8 +35,6 @@ 
 #include "qemu/plugin.h"
 #include "sysemu/hw_accel.h"
 
-CPUInterruptHandler cpu_interrupt_handler;
-
 CPUState *cpu_by_arch_id(int64_t id)
 {
     CPUState *cpu;
@@ -394,17 +392,6 @@  static vaddr cpu_adjust_watchpoint_address(CPUState *cpu, vaddr addr, int len)
     return addr;
 }
 
-static void generic_handle_interrupt(CPUState *cpu, int mask)
-{
-    cpu->interrupt_request |= mask;
-
-    if (!qemu_cpu_is_self(cpu)) {
-        qemu_cpu_kick(cpu);
-    }
-}
-
-CPUInterruptHandler cpu_interrupt_handler = generic_handle_interrupt;
-
 static void cpu_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 8f145733ce..efd33d87fd 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -838,12 +838,6 @@  bool cpu_exists(int64_t id);
  */
 CPUState *cpu_by_arch_id(int64_t id);
 
-#ifndef CONFIG_USER_ONLY
-
-typedef void (*CPUInterruptHandler)(CPUState *, int);
-
-extern CPUInterruptHandler cpu_interrupt_handler;
-
 /**
  * cpu_interrupt:
  * @cpu: The CPU to set an interrupt on.
@@ -851,17 +845,9 @@  extern CPUInterruptHandler cpu_interrupt_handler;
  *
  * Invokes the interrupt handler.
  */
-static inline void cpu_interrupt(CPUState *cpu, int mask)
-{
-    cpu_interrupt_handler(cpu, mask);
-}
-
-#else /* USER_ONLY */
 
 void cpu_interrupt(CPUState *cpu, int mask);
 
-#endif /* USER_ONLY */
-
 #ifdef NEED_CPU_H
 
 #ifdef CONFIG_SOFTMMU
diff --git a/include/sysemu/cpus.h b/include/sysemu/cpus.h
index db196dd96f..11ac0387e6 100644
--- a/include/sysemu/cpus.h
+++ b/include/sysemu/cpus.h
@@ -16,6 +16,8 @@  typedef struct CpusAccel {
     void (*synchronize_state)(CPUState *cpu);
     void (*synchronize_pre_loadvm)(CPUState *cpu);
 
+    void (*handle_interrupt)(CPUState *cpu, int mask);
+
     int64_t (*get_virtual_clock)(void);
     int64_t (*get_elapsed_ticks)(void);
 } CpusAccel;
diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 1125fb5fc9..17531161fc 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -213,6 +213,24 @@  int64_t cpus_get_elapsed_ticks(void)
     return cpu_get_ticks();
 }
 
+static void generic_handle_interrupt(CPUState *cpu, int mask)
+{
+    cpu->interrupt_request |= mask;
+
+    if (!qemu_cpu_is_self(cpu)) {
+        qemu_cpu_kick(cpu);
+    }
+}
+
+void cpu_interrupt(CPUState *cpu, int mask)
+{
+    if (cpus_accel && cpus_accel->handle_interrupt) {
+        cpus_accel->handle_interrupt(cpu, mask);
+    } else {
+        generic_handle_interrupt(cpu, mask);
+    }
+}
+
 static int do_vm_stop(RunState state, bool send_stop)
 {
     int ret = 0;
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index b66ddeb8bf..fd1ab673d7 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -297,15 +297,6 @@  int hax_vm_destroy(struct hax_vm *vm)
     return 0;
 }
 
-static void hax_handle_interrupt(CPUState *cpu, int mask)
-{
-    cpu->interrupt_request |= mask;
-
-    if (!qemu_cpu_is_self(cpu)) {
-        qemu_cpu_kick(cpu);
-    }
-}
-
 static int hax_init(ram_addr_t ram_size, int max_cpus)
 {
     struct hax_state *hax = NULL;
@@ -350,7 +341,6 @@  static int hax_init(ram_addr_t ram_size, int max_cpus)
     qversion.cur_version = hax_cur_version;
     qversion.min_version = hax_min_version;
     hax_notify_qemu_version(hax->vm->fd, &qversion);
-    cpu_interrupt_handler = hax_handle_interrupt;
 
     return ret;
   error:
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 7ac6987c1b..ed9356565c 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -262,14 +262,6 @@  static void update_apic_tpr(CPUState *cpu)
 
 #define VECTORING_INFO_VECTOR_MASK     0xff
 
-static void hvf_handle_interrupt(CPUState * cpu, int mask)
-{
-    cpu->interrupt_request |= mask;
-    if (!qemu_cpu_is_self(cpu)) {
-        qemu_cpu_kick(cpu);
-    }
-}
-
 void hvf_handle_io(CPUArchState *env, uint16_t port, void *buffer,
                   int direction, int size, int count)
 {
@@ -894,7 +886,6 @@  static int hvf_accel_init(MachineState *ms)
     }
   
     hvf_state = s;
-    cpu_interrupt_handler = hvf_handle_interrupt;
     memory_listener_register(&hvf_memory_listener, &address_space_memory);
     cpus_register_accel(&hvf_cpus);
     return 0;
diff --git a/target/i386/whpx-all.c b/target/i386/whpx-all.c
index 8b6986c864..b3d17fbe04 100644
--- a/target/i386/whpx-all.c
+++ b/target/i386/whpx-all.c
@@ -1413,15 +1413,6 @@  static void whpx_memory_init(void)
     memory_listener_register(&whpx_memory_listener, &address_space_memory);
 }
 
-static void whpx_handle_interrupt(CPUState *cpu, int mask)
-{
-    cpu->interrupt_request |= mask;
-
-    if (!qemu_cpu_is_self(cpu)) {
-        qemu_cpu_kick(cpu);
-    }
-}
-
 /*
  * Load the functions from the given library, using the given handle. If a
  * handle is provided, it is used, otherwise the library is opened. The
@@ -1576,7 +1567,6 @@  static int whpx_accel_init(MachineState *ms)
 
     whpx_memory_init();
 
-    cpu_interrupt_handler = whpx_handle_interrupt;
     cpus_register_accel(&whpx_cpus);
 
     printf("Windows Hypervisor Platform accelerator is operational\n");