diff mbox series

[PULL,07/37] cpus: extract out hax-specific code to target/i386/

Message ID 20201006072947.487729-8-pbonzini@redhat.com
State New
Headers show
Series Build system + accel + record/replay patches for 2020-10-06 | expand

Commit Message

Paolo Bonzini Oct. 6, 2020, 7:29 a.m. UTC
From: Claudio Fontana <cfontana@suse.de>

register a "CpusAccel" interface for HAX as well.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 softmmu/cpus.c            | 80 +-----------------------------------
 target/i386/hax-all.c     |  6 ++-
 target/i386/hax-cpus.c    | 85 +++++++++++++++++++++++++++++++++++++++
 target/i386/hax-cpus.h    | 17 ++++++++
 target/i386/hax-i386.h    |  2 +
 target/i386/hax-posix.c   | 12 ++++++
 target/i386/hax-windows.c | 20 +++++++++
 target/i386/meson.build   |  9 ++++-
 8 files changed, 149 insertions(+), 82 deletions(-)
 create mode 100644 target/i386/hax-cpus.c
 create mode 100644 target/i386/hax-cpus.h

Comments

Volker Rümelin Oct. 16, 2020, 6:48 a.m. UTC | #1
> From: Claudio Fontana <cfontana@suse.de>

>

> register a "CpusAccel" interface for HAX as well.

>


> diff --git a/softmmu/cpus.c b/softmmu/cpus.c

> index 9fa73735a2..900fff827a 100644

> --- a/softmmu/cpus.c

> +++ b/softmmu/cpus.c

> @@ -416,35 +403,6 @@ void qemu_wait_io_event(CPUState *cpu)

>      qemu_wait_io_event_common(cpu);

>  }

>  

> -static void *qemu_hax_cpu_thread_fn(void *arg)

> -{

> -    CPUState *cpu = arg;

> -    int r;

> -

> -    rcu_register_thread();

> -    qemu_mutex_lock_iothread();

> -    qemu_thread_get_self(cpu->thread);

> -

> -    cpu->thread_id = qemu_get_thread_id();

> -    current_cpu = cpu;


Hi Claudio,

is there a reason why you removed current_cpu = cpu; from hax_cpu_thread_fn() when you moved that function to target/i386/hax-cpus.c? This change broke HAX on Windows. Adding back that line makes it work again.

The simplest reproducer is:
$ ./qemu-system-x86_64.exe -machine pc,accel=hax -smp 2 -display gtk
HAX is working and emulator runs in fast virt mode.

Then the QEMU window opens and shows 'Guest has not initialized the display (yet).' forever.

A look at the Windows Task Manager suggests one thread is busy looping.

With best regards,
Volker

> -    hax_init_vcpu(cpu);

> -    cpu_thread_signal_created(cpu);

> -    qemu_guest_random_seed_thread_part2(cpu->random_seed);

> -

> -    do {

> -        if (cpu_can_run(cpu)) {

> -            r = hax_smp_cpu_exec(cpu);

> -            if (r == EXCP_DEBUG) {

> -                cpu_handle_guest_debug(cpu);

> -            }

> -        }

> -

> -        qemu_wait_io_event(cpu);

> -    } while (!cpu->unplug || cpu_can_run(cpu));

> -    rcu_unregister_thread();

> -    return NULL;

> -}

> -

>  /* The HVF-specific vCPU thread function. This one should only run when the host

>   * CPU supports the VMX "unrestricted guest" feature. */

>  static void *qemu_hvf_cpu_thread_fn(void *arg)

>
Claudio Fontana Oct. 16, 2020, 8 a.m. UTC | #2
On 10/16/20 8:48 AM, Volker Rümelin wrote:
>> From: Claudio Fontana <cfontana@suse.de>

>>

>> register a "CpusAccel" interface for HAX as well.

>>

> 

>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c

>> index 9fa73735a2..900fff827a 100644

>> --- a/softmmu/cpus.c

>> +++ b/softmmu/cpus.c

>> @@ -416,35 +403,6 @@ void qemu_wait_io_event(CPUState *cpu)

>>      qemu_wait_io_event_common(cpu);

>>  }

>>  

>> -static void *qemu_hax_cpu_thread_fn(void *arg)

>> -{

>> -    CPUState *cpu = arg;

>> -    int r;

>> -

>> -    rcu_register_thread();

>> -    qemu_mutex_lock_iothread();

>> -    qemu_thread_get_self(cpu->thread);

>> -

>> -    cpu->thread_id = qemu_get_thread_id();

>> -    current_cpu = cpu;

> 

> Hi Claudio,

> 

> is there a reason why you removed current_cpu = cpu; from hax_cpu_thread_fn() when you moved that function to target/i386/hax-cpus.c? This change broke HAX on Windows. Adding back that line makes it work again.



Hello Volker, I see the change in the history and it was clearly an ugly mistake on my part.
There was no reason or intention to remove the current_cpu = cpu assignment

The fix seems indeed to just + current_cpu = cpu;
and I will send a patch momentarily that does just that,

but I don't know of any CI coverage for Windows + hax currently,
so it would be good if you could spin the change to verify that it fixes the problem.

Ciao,

Claudio

> 

> The simplest reproducer is:

> $ ./qemu-system-x86_64.exe -machine pc,accel=hax -smp 2 -display gtk

> HAX is working and emulator runs in fast virt mode.

> 

> Then the QEMU window opens and shows 'Guest has not initialized the display (yet).' forever.

> 

> A look at the Windows Task Manager suggests one thread is busy looping.

> 

> With best regards,

> Volker

> 

>> -    hax_init_vcpu(cpu);

>> -    cpu_thread_signal_created(cpu);

>> -    qemu_guest_random_seed_thread_part2(cpu->random_seed);

>> -

>> -    do {

>> -        if (cpu_can_run(cpu)) {

>> -            r = hax_smp_cpu_exec(cpu);

>> -            if (r == EXCP_DEBUG) {

>> -                cpu_handle_guest_debug(cpu);

>> -            }

>> -        }

>> -

>> -        qemu_wait_io_event(cpu);

>> -    } while (!cpu->unplug || cpu_can_run(cpu));

>> -    rcu_unregister_thread();

>> -    return NULL;

>> -}

>> -

>>  /* The HVF-specific vCPU thread function. This one should only run when the host

>>   * CPU supports the VMX "unrestricted guest" feature. */

>>  static void *qemu_hvf_cpu_thread_fn(void *arg)

>>
Volker Rümelin Oct. 17, 2020, 7:17 a.m. UTC | #3
>> Hi Claudio,

>>

>> is there a reason why you removed current_cpu = cpu; from hax_cpu_thread_fn() when you moved that function to target/i386/hax-cpus.c? This change broke HAX on Windows. Adding back that line makes it work again.

>

> Hello Volker, I see the change in the history and it was clearly an ugly mistake on my part.

> There was no reason or intention to remove the current_cpu = cpu assignment

>

> The fix seems indeed to just + current_cpu = cpu;

> and I will send a patch momentarily that does just that,

>

> but I don't know of any CI coverage for Windows + hax currently,

> so it would be good if you could spin the change to verify that it fixes the problem.

>


That was very quick. Only 5 hours from bug report to pull request. I've tested current master together with the patch from Paolo's PR. HAX on Windows works.

With best regards,
Volker
diff mbox series

Patch

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 9fa73735a2..900fff827a 100644
--- a/softmmu/cpus.c
+++ b/softmmu/cpus.c
@@ -33,7 +33,6 @@ 
 #include "exec/gdbstub.h"
 #include "sysemu/hw_accel.h"
 #include "sysemu/kvm.h"
-#include "sysemu/hax.h"
 #include "sysemu/hvf.h"
 #include "sysemu/whpx.h"
 #include "exec/exec-all.h"
@@ -179,9 +178,6 @@  void cpu_synchronize_state(CPUState *cpu)
     if (cpus_accel && cpus_accel->synchronize_state) {
         cpus_accel->synchronize_state(cpu);
     }
-    if (hax_enabled()) {
-        hax_cpu_synchronize_state(cpu);
-    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_state(cpu);
     }
@@ -192,9 +188,6 @@  void cpu_synchronize_post_reset(CPUState *cpu)
     if (cpus_accel && cpus_accel->synchronize_post_reset) {
         cpus_accel->synchronize_post_reset(cpu);
     }
-    if (hax_enabled()) {
-        hax_cpu_synchronize_post_reset(cpu);
-    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_post_reset(cpu);
     }
@@ -205,9 +198,6 @@  void cpu_synchronize_post_init(CPUState *cpu)
     if (cpus_accel && cpus_accel->synchronize_post_init) {
         cpus_accel->synchronize_post_init(cpu);
     }
-    if (hax_enabled()) {
-        hax_cpu_synchronize_post_init(cpu);
-    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_post_init(cpu);
     }
@@ -218,9 +208,6 @@  void cpu_synchronize_pre_loadvm(CPUState *cpu)
     if (cpus_accel && cpus_accel->synchronize_pre_loadvm) {
         cpus_accel->synchronize_pre_loadvm(cpu);
     }
-    if (hax_enabled()) {
-        hax_cpu_synchronize_pre_loadvm(cpu);
-    }
     if (hvf_enabled()) {
         hvf_cpu_synchronize_pre_loadvm(cpu);
     }
@@ -416,35 +403,6 @@  void qemu_wait_io_event(CPUState *cpu)
     qemu_wait_io_event_common(cpu);
 }
 
-static void *qemu_hax_cpu_thread_fn(void *arg)
-{
-    CPUState *cpu = arg;
-    int r;
-
-    rcu_register_thread();
-    qemu_mutex_lock_iothread();
-    qemu_thread_get_self(cpu->thread);
-
-    cpu->thread_id = qemu_get_thread_id();
-    current_cpu = cpu;
-    hax_init_vcpu(cpu);
-    cpu_thread_signal_created(cpu);
-    qemu_guest_random_seed_thread_part2(cpu->random_seed);
-
-    do {
-        if (cpu_can_run(cpu)) {
-            r = hax_smp_cpu_exec(cpu);
-            if (r == EXCP_DEBUG) {
-                cpu_handle_guest_debug(cpu);
-            }
-        }
-
-        qemu_wait_io_event(cpu);
-    } while (!cpu->unplug || cpu_can_run(cpu));
-    rcu_unregister_thread();
-    return NULL;
-}
-
 /* The HVF-specific vCPU thread function. This one should only run when the host
  * CPU supports the VMX "unrestricted guest" feature. */
 static void *qemu_hvf_cpu_thread_fn(void *arg)
@@ -529,12 +487,6 @@  static void *qemu_whpx_cpu_thread_fn(void *arg)
     return NULL;
 }
 
-#ifdef _WIN32
-static void CALLBACK dummy_apc_func(ULONG_PTR unused)
-{
-}
-#endif
-
 void cpus_kick_thread(CPUState *cpu)
 {
 #ifndef _WIN32
@@ -553,10 +505,6 @@  void cpus_kick_thread(CPUState *cpu)
     if (!qemu_cpu_is_self(cpu)) {
         if (whpx_enabled()) {
             whpx_vcpu_kick(cpu);
-        } else if (!QueueUserAPC(dummy_apc_func, cpu->hThread, 0)) {
-            fprintf(stderr, "%s: QueueUserAPC failed with error %lu\n",
-                    __func__, GetLastError());
-            exit(1);
         }
     }
 #endif
@@ -567,14 +515,7 @@  void qemu_cpu_kick(CPUState *cpu)
     qemu_cond_broadcast(cpu->halt_cond);
     if (cpus_accel && cpus_accel->kick_vcpu_thread) {
         cpus_accel->kick_vcpu_thread(cpu);
-    } else {
-        if (hax_enabled()) {
-            /*
-             * FIXME: race condition with the exit_request check in
-             * hax_vcpu_hax_exec
-             */
-            cpu->exit_request = 1;
-        }
+    } else { /* default */
         cpus_kick_thread(cpu);
     }
 }
@@ -722,23 +663,6 @@  void cpu_remove_sync(CPUState *cpu)
     qemu_mutex_lock_iothread();
 }
 
-static void qemu_hax_start_vcpu(CPUState *cpu)
-{
-    char thread_name[VCPU_THREAD_NAME_SIZE];
-
-    cpu->thread = g_malloc0(sizeof(QemuThread));
-    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
-    qemu_cond_init(cpu->halt_cond);
-
-    snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
-             cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_hax_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
-#ifdef _WIN32
-    cpu->hThread = qemu_thread_get_handle(cpu->thread);
-#endif
-}
-
 static void qemu_hvf_start_vcpu(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
@@ -800,8 +724,6 @@  void qemu_init_vcpu(CPUState *cpu)
     if (cpus_accel) {
         /* accelerator already implements the CpusAccel interface */
         cpus_accel->create_vcpu_thread(cpu);
-    } else if (hax_enabled()) {
-        qemu_hax_start_vcpu(cpu);
     } else if (hvf_enabled()) {
         qemu_hvf_start_vcpu(cpu);
     } else if (whpx_enabled()) {
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index c93bb23a44..b66ddeb8bf 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -32,9 +32,10 @@ 
 #include "sysemu/accel.h"
 #include "sysemu/reset.h"
 #include "sysemu/runstate.h"
-#include "qemu/main-loop.h"
 #include "hw/boards.h"
 
+#include "hax-cpus.h"
+
 #define DEBUG_HAX 0
 
 #define DPRINTF(fmt, ...) \
@@ -374,6 +375,9 @@  static int hax_accel_init(MachineState *ms)
                 !ret ? "working" : "not working",
                 !ret ? "fast virt" : "emulation");
     }
+    if (ret == 0) {
+        cpus_register_accel(&hax_cpus);
+    }
     return ret;
 }
 
diff --git a/target/i386/hax-cpus.c b/target/i386/hax-cpus.c
new file mode 100644
index 0000000000..9aad98bc7a
--- /dev/null
+++ b/target/i386/hax-cpus.c
@@ -0,0 +1,85 @@ 
+/*
+ * QEMU HAX support
+ *
+ * Copyright IBM, Corp. 2008
+ *           Red Hat, Inc. 2008
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Glauber Costa     <gcosta@redhat.com>
+ *
+ * Copyright (c) 2011 Intel Corporation
+ *  Written by:
+ *  Jiang Yunhong<yunhong.jiang@intel.com>
+ *  Xin Xiaohui<xiaohui.xin@intel.com>
+ *  Zhang Xiantao<xiantao.zhang@intel.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+#include "hax-i386.h"
+#include "sysemu/runstate.h"
+#include "sysemu/cpus.h"
+#include "qemu/guest-random.h"
+
+#include "hax-cpus.h"
+
+static void *hax_cpu_thread_fn(void *arg)
+{
+    CPUState *cpu = arg;
+    int r;
+
+    rcu_register_thread();
+    qemu_mutex_lock_iothread();
+    qemu_thread_get_self(cpu->thread);
+
+    cpu->thread_id = qemu_get_thread_id();
+    hax_init_vcpu(cpu);
+    cpu_thread_signal_created(cpu);
+    qemu_guest_random_seed_thread_part2(cpu->random_seed);
+
+    do {
+        if (cpu_can_run(cpu)) {
+            r = hax_smp_cpu_exec(cpu);
+            if (r == EXCP_DEBUG) {
+                cpu_handle_guest_debug(cpu);
+            }
+        }
+
+        qemu_wait_io_event(cpu);
+    } while (!cpu->unplug || cpu_can_run(cpu));
+    rcu_unregister_thread();
+    return NULL;
+}
+
+static void hax_start_vcpu_thread(CPUState *cpu)
+{
+    char thread_name[VCPU_THREAD_NAME_SIZE];
+
+    cpu->thread = g_malloc0(sizeof(QemuThread));
+    cpu->halt_cond = g_malloc0(sizeof(QemuCond));
+    qemu_cond_init(cpu->halt_cond);
+
+    snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/HAX",
+             cpu->cpu_index);
+    qemu_thread_create(cpu->thread, thread_name, hax_cpu_thread_fn,
+                       cpu, QEMU_THREAD_JOINABLE);
+#ifdef _WIN32
+    cpu->hThread = qemu_thread_get_handle(cpu->thread);
+#endif
+}
+
+const CpusAccel hax_cpus = {
+    .create_vcpu_thread = hax_start_vcpu_thread,
+    .kick_vcpu_thread = hax_kick_vcpu_thread,
+
+    .synchronize_post_reset = hax_cpu_synchronize_post_reset,
+    .synchronize_post_init = hax_cpu_synchronize_post_init,
+    .synchronize_state = hax_cpu_synchronize_state,
+    .synchronize_pre_loadvm = hax_cpu_synchronize_pre_loadvm,
+};
diff --git a/target/i386/hax-cpus.h b/target/i386/hax-cpus.h
new file mode 100644
index 0000000000..a64417fe2d
--- /dev/null
+++ b/target/i386/hax-cpus.h
@@ -0,0 +1,17 @@ 
+/*
+ * Accelerator CPUS Interface
+ *
+ * Copyright 2020 SUSE LLC
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HAX_CPUS_H
+#define HAX_CPUS_H
+
+#include "sysemu/cpus.h"
+
+extern const CpusAccel hax_cpus;
+
+#endif /* HAX_CPUS_H */
diff --git a/target/i386/hax-i386.h b/target/i386/hax-i386.h
index ec28708185..48c4abe14e 100644
--- a/target/i386/hax-i386.h
+++ b/target/i386/hax-i386.h
@@ -60,6 +60,8 @@  int hax_inject_interrupt(CPUArchState *env, int vector);
 struct hax_vm *hax_vm_create(struct hax_state *hax, int max_cpus);
 int hax_vcpu_run(struct hax_vcpu_state *vcpu);
 int hax_vcpu_create(int id);
+void hax_kick_vcpu_thread(CPUState *cpu);
+
 int hax_sync_vcpu_state(CPUArchState *env, struct vcpu_state_t *state,
                         int set);
 int hax_sync_msr(CPUArchState *env, struct hax_msr_data *msrs, int set);
diff --git a/target/i386/hax-posix.c b/target/i386/hax-posix.c
index 5f9d1b803d..6fb7867d11 100644
--- a/target/i386/hax-posix.c
+++ b/target/i386/hax-posix.c
@@ -16,6 +16,8 @@ 
 
 #include "target/i386/hax-i386.h"
 
+#include "sysemu/cpus.h"
+
 hax_fd hax_mod_open(void)
 {
     int fd = open("/dev/HAX", O_RDWR);
@@ -292,3 +294,13 @@  int hax_inject_interrupt(CPUArchState *env, int vector)
 
     return ioctl(fd, HAX_VCPU_IOCTL_INTERRUPT, &vector);
 }
+
+void hax_kick_vcpu_thread(CPUState *cpu)
+{
+    /*
+     * FIXME: race condition with the exit_request check in
+     * hax_vcpu_hax_exec
+     */
+    cpu->exit_request = 1;
+    cpus_kick_thread(cpu);
+}
diff --git a/target/i386/hax-windows.c b/target/i386/hax-windows.c
index 863c2bcc19..469b48e608 100644
--- a/target/i386/hax-windows.c
+++ b/target/i386/hax-windows.c
@@ -463,3 +463,23 @@  int hax_inject_interrupt(CPUArchState *env, int vector)
         return 0;
     }
 }
+
+static void CALLBACK dummy_apc_func(ULONG_PTR unused)
+{
+}
+
+void hax_kick_vcpu_thread(CPUState *cpu)
+{
+    /*
+     * FIXME: race condition with the exit_request check in
+     * hax_vcpu_hax_exec
+     */
+    cpu->exit_request = 1;
+    if (!qemu_cpu_is_self(cpu)) {
+        if (!QueueUserAPC(dummy_apc_func, cpu->hThread, 0)) {
+            fprintf(stderr, "%s: QueueUserAPC failed with error %lu\n",
+                    __func__, GetLastError());
+            exit(1);
+        }
+    }
+}
diff --git a/target/i386/meson.build b/target/i386/meson.build
index e0b71ade56..1db619841c 100644
--- a/target/i386/meson.build
+++ b/target/i386/meson.build
@@ -31,8 +31,13 @@  i386_softmmu_ss.add(files(
 i386_softmmu_ss.add(when: 'CONFIG_HYPERV', if_true: files('hyperv.c'), if_false: files('hyperv-stub.c'))
 i386_softmmu_ss.add(when: 'CONFIG_KVM', if_true: files('kvm.c'))
 i386_softmmu_ss.add(when: 'CONFIG_WHPX', if_true: files('whpx-all.c'))
-i386_softmmu_ss.add(when: ['CONFIG_POSIX', 'CONFIG_HAX'], if_true: files('hax-all.c', 'hax-mem.c', 'hax-posix.c'))
-i386_softmmu_ss.add(when: ['CONFIG_WIN32', 'CONFIG_HAX'], if_true: files('hax-all.c', 'hax-mem.c', 'hax-windows.c'))
+i386_softmmu_ss.add(when: 'CONFIG_HAX', if_true: files(
+  'hax-all.c',
+  'hax-mem.c',
+  'hax-cpus.c',
+))
+i386_softmmu_ss.add(when: ['CONFIG_HAX', 'CONFIG_POSIX'], if_true: files('hax-posix.c'))
+i386_softmmu_ss.add(when: ['CONFIG_HAX', 'CONFIG_WIN32'], if_true: files('hax-windows.c'))
 
 subdir('hvf')