register a "CpusAccel" interface for HVF as well.

Signed-off-by: Claudio Fontana <cfontana@suse.de>
---
 softmmu/cpus.c                |  63 --------------------
 target/i386/hvf/Makefile.objs |   2 +-
 target/i386/hvf/hvf-cpus.c    | 131 ++++++++++++++++++++++++++++++++++++++++++
 target/i386/hvf/hvf-cpus.h    |  17 ++++++
 target/i386/hvf/hvf.c         |   3 +
 5 files changed, 152 insertions(+), 64 deletions(-)
 create mode 100644 target/i386/hvf/hvf-cpus.c
 create mode 100644 target/i386/hvf/hvf-cpus.h

diff --git a/softmmu/cpus.c b/softmmu/cpus.c
index 586b4acaab..d327b2685c 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/hvf.h"
 #include "exec/exec-all.h"
 #include "qemu/thread.h"
 #include "qemu/plugin.h"
@@ -391,48 +390,6 @@ void qemu_wait_io_event(CPUState *cpu)
     qemu_wait_io_event_common(cpu);
 }
 
-/* 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)
-{
-    CPUState *cpu = arg;
-
-    int r;
-
-    assert(hvf_enabled());
-
-    rcu_register_thread();
-
-    qemu_mutex_lock_iothread();
-    qemu_thread_get_self(cpu->thread);
-
-    cpu->thread_id = qemu_get_thread_id();
-    cpu->can_do_io = 1;
-    current_cpu = cpu;
-
-    hvf_init_vcpu(cpu);
-
-    /* signal CPU creation */
-    cpu_thread_signal_created(cpu);
-    qemu_guest_random_seed_thread_part2(cpu->random_seed);
-
-    do {
-        if (cpu_can_run(cpu)) {
-            r = hvf_vcpu_exec(cpu);
-            if (r == EXCP_DEBUG) {
-                cpu_handle_guest_debug(cpu);
-            }
-        }
-        qemu_wait_io_event(cpu);
-    } while (!cpu->unplug || cpu_can_run(cpu));
-
-    hvf_vcpu_destroy(cpu);
-    cpu_thread_signal_destroyed(cpu);
-    qemu_mutex_unlock_iothread();
-    rcu_unregister_thread();
-    return NULL;
-}
-
 void cpus_kick_thread(CPUState *cpu)
 {
 #ifndef _WIN32
@@ -603,24 +560,6 @@ void cpu_remove_sync(CPUState *cpu)
     qemu_mutex_lock_iothread();
 }
 
-static void qemu_hvf_start_vcpu(CPUState *cpu)
-{
-    char thread_name[VCPU_THREAD_NAME_SIZE];
-
-    /* HVF currently does not support TCG, and only runs in
-     * unrestricted-guest mode. */
-    assert(hvf_enabled());
-
-    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/HVF",
-             cpu->cpu_index);
-    qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
-                       cpu, QEMU_THREAD_JOINABLE);
-}
-
 void cpus_register_accel(CpusAccel *ca)
 {
     assert(ca != NULL);
@@ -648,8 +587,6 @@ void qemu_init_vcpu(CPUState *cpu)
     if (cpus_accel) {
         /* accelerator already implements the CpusAccel interface */
         cpus_accel->create_vcpu_thread(cpu);
-    } else if (hvf_enabled()) {
-        qemu_hvf_start_vcpu(cpu);
     } else {
         assert(0);
     }
diff --git a/target/i386/hvf/Makefile.objs b/target/i386/hvf/Makefile.objs
index 927b86bc67..af9f7dcfc1 100644
--- a/target/i386/hvf/Makefile.objs
+++ b/target/i386/hvf/Makefile.objs
@@ -1,2 +1,2 @@
-obj-y += hvf.o
+obj-y += hvf.o hvf-cpus.o
 obj-y += x86.o x86_cpuid.o x86_decode.o x86_descr.o x86_emu.o x86_flags.o x86_mmu.o x86hvf.o x86_task.o
diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-cpus.c
new file mode 100644
index 0000000000..9540157f1e
--- /dev/null
+++ b/target/i386/hvf/hvf-cpus.c
@@ -0,0 +1,131 @@
+/*
+ * Copyright 2008 IBM Corporation
+ *           2008 Red Hat, Inc.
+ * Copyright 2011 Intel Corporation
+ * Copyright 2016 Veertu, Inc.
+ * Copyright 2017 The Android Open Source Project
+ *
+ * QEMU Hypervisor.framework support
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ *
+ * This file contain code under public domain from the hvdos project:
+ * https://github.com/mist64/hvdos
+ *
+ * Parts Copyright (c) 2011 NetApp, Inc.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY NETAPP, INC ``AS IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED.  IN NO EVENT SHALL NETAPP, INC OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
+ * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY
+ * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/error-report.h"
+#include "qemu/main-loop.h"
+#include "sysemu/hvf.h"
+#include "sysemu/runstate.h"
+#include "target/i386/cpu.h"
+#include "qemu/guest-random.h"
+
+#include "hvf-cpus.h"
+
+/*
+ * The HVF-specific vCPU thread function. This one should only run when the host
+ * CPU supports the VMX "unrestricted guest" feature.
+ */
+static void *hvf_cpu_thread_fn(void *arg)
+{
+    CPUState *cpu = arg;
+
+    int r;
+
+    assert(hvf_enabled());
+
+    rcu_register_thread();
+
+    qemu_mutex_lock_iothread();
+    qemu_thread_get_self(cpu->thread);
+
+    cpu->thread_id = qemu_get_thread_id();
+    cpu->can_do_io = 1;
+    current_cpu = cpu;
+
+    hvf_init_vcpu(cpu);
+
+    /* signal CPU creation */
+    cpu_thread_signal_created(cpu);
+    qemu_guest_random_seed_thread_part2(cpu->random_seed);
+
+    do {
+        if (cpu_can_run(cpu)) {
+            r = hvf_vcpu_exec(cpu);
+            if (r == EXCP_DEBUG) {
+                cpu_handle_guest_debug(cpu);
+            }
+        }
+        qemu_wait_io_event(cpu);
+    } while (!cpu->unplug || cpu_can_run(cpu));
+
+    hvf_vcpu_destroy(cpu);
+    cpu_thread_signal_destroyed(cpu);
+    qemu_mutex_unlock_iothread();
+    rcu_unregister_thread();
+    return NULL;
+}
+
+static void hvf_start_vcpu_thread(CPUState *cpu)
+{
+    char thread_name[VCPU_THREAD_NAME_SIZE];
+
+    /*
+     * HVF currently does not support TCG, and only runs in
+     * unrestricted-guest mode.
+     */
+    assert(hvf_enabled());
+
+    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/HVF",
+             cpu->cpu_index);
+    qemu_thread_create(cpu->thread, thread_name, hvf_cpu_thread_fn,
+                       cpu, QEMU_THREAD_JOINABLE);
+}
+
+CpusAccel hvf_cpus = {
+    .create_vcpu_thread = hvf_start_vcpu_thread,
+
+    .synchronize_post_reset = hvf_cpu_synchronize_post_reset,
+    .synchronize_post_init = hvf_cpu_synchronize_post_init,
+    .synchronize_state = hvf_cpu_synchronize_state,
+    .synchronize_pre_loadvm = hvf_cpu_synchronize_pre_loadvm,
+};
diff --git a/target/i386/hvf/hvf-cpus.h b/target/i386/hvf/hvf-cpus.h
new file mode 100644
index 0000000000..b66f4889b0
--- /dev/null
+++ b/target/i386/hvf/hvf-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 HVF_CPUS_H
+#define HVF_CPUS_H
+
+#include "sysemu/cpus.h"
+
+extern CpusAccel hvf_cpus;
+
+#endif /* HVF_CPUS_H */
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index d81f569aed..7ac6987c1b 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -72,6 +72,8 @@
 #include "sysemu/accel.h"
 #include "target/i386/cpu.h"
 
+#include "hvf-cpus.h"
+
 HVFState *hvf_state;
 
 static void assert_hvf_ok(hv_return_t ret)
@@ -894,6 +896,7 @@ 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;
 }
 
-- 
2.16.4

On Mon, Aug 03, 2020 at 11:05:33AM +0200, Claudio Fontana wrote:
> register a "CpusAccel" interface for HVF as well.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  softmmu/cpus.c                |  63 --------------------
>  target/i386/hvf/Makefile.objs |   2 +-
>  target/i386/hvf/hvf-cpus.c    | 131 ++++++++++++++++++++++++++++++++++++++++++
>  target/i386/hvf/hvf-cpus.h    |  17 ++++++
>  target/i386/hvf/hvf.c         |   3 +
>  5 files changed, 152 insertions(+), 64 deletions(-)
>  create mode 100644 target/i386/hvf/hvf-cpus.c
>  create mode 100644 target/i386/hvf/hvf-cpus.h
> 
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 586b4acaab..d327b2685c 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/hvf.h"

I wonder if the declarations should be moved from sysemu/hvf.h to
someplace inside target/i386/hvf/:

int hvf_init_vcpu(CPUState *);
int hvf_vcpu_exec(CPUState *);
void hvf_cpu_synchronize_state(CPUState *);
void hvf_cpu_synchronize_post_reset(CPUState *);
void hvf_cpu_synchronize_post_init(CPUState *);
void hvf_cpu_synchronize_pre_loadvm(CPUState *);
void hvf_vcpu_destroy(CPUState *);

They're not used outside of target/i386/hvf/

I also wonder if we need stubs at all?

>  #include "exec/exec-all.h"
>  #include "qemu/thread.h"
>  #include "qemu/plugin.h"
> @@ -391,48 +390,6 @@ void qemu_wait_io_event(CPUState *cpu)
>      qemu_wait_io_event_common(cpu);
>  }
>  
> -/* 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)
> -{
> -    CPUState *cpu = arg;
> -
> -    int r;
> -
> -    assert(hvf_enabled());
> -
> -    rcu_register_thread();
> -
> -    qemu_mutex_lock_iothread();
> -    qemu_thread_get_self(cpu->thread);
> -
> -    cpu->thread_id = qemu_get_thread_id();
> -    cpu->can_do_io = 1;
> -    current_cpu = cpu;
> -
> -    hvf_init_vcpu(cpu);
> -
> -    /* signal CPU creation */
> -    cpu_thread_signal_created(cpu);
> -    qemu_guest_random_seed_thread_part2(cpu->random_seed);
> -
> -    do {
> -        if (cpu_can_run(cpu)) {
> -            r = hvf_vcpu_exec(cpu);
> -            if (r == EXCP_DEBUG) {
> -                cpu_handle_guest_debug(cpu);
> -            }
> -        }
> -        qemu_wait_io_event(cpu);
> -    } while (!cpu->unplug || cpu_can_run(cpu));
> -
> -    hvf_vcpu_destroy(cpu);
> -    cpu_thread_signal_destroyed(cpu);
> -    qemu_mutex_unlock_iothread();
> -    rcu_unregister_thread();
> -    return NULL;
> -}
> -
>  void cpus_kick_thread(CPUState *cpu)
>  {
>  #ifndef _WIN32
> @@ -603,24 +560,6 @@ void cpu_remove_sync(CPUState *cpu)
>      qemu_mutex_lock_iothread();
>  }
>  
> -static void qemu_hvf_start_vcpu(CPUState *cpu)
> -{
> -    char thread_name[VCPU_THREAD_NAME_SIZE];
> -
> -    /* HVF currently does not support TCG, and only runs in
> -     * unrestricted-guest mode. */
> -    assert(hvf_enabled());
> -
> -    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/HVF",
> -             cpu->cpu_index);
> -    qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
> -                       cpu, QEMU_THREAD_JOINABLE);
> -}
> -
>  void cpus_register_accel(CpusAccel *ca)
>  {
>      assert(ca != NULL);
> @@ -648,8 +587,6 @@ void qemu_init_vcpu(CPUState *cpu)
>      if (cpus_accel) {
>          /* accelerator already implements the CpusAccel interface */
>          cpus_accel->create_vcpu_thread(cpu);
> -    } else if (hvf_enabled()) {
> -        qemu_hvf_start_vcpu(cpu);
>      } else {
>          assert(0);
>      }
> diff --git a/target/i386/hvf/Makefile.objs b/target/i386/hvf/Makefile.objs
> index 927b86bc67..af9f7dcfc1 100644
> --- a/target/i386/hvf/Makefile.objs
> +++ b/target/i386/hvf/Makefile.objs
> @@ -1,2 +1,2 @@
> -obj-y += hvf.o
> +obj-y += hvf.o hvf-cpus.o
>  obj-y += x86.o x86_cpuid.o x86_decode.o x86_descr.o x86_emu.o x86_flags.o x86_mmu.o x86hvf.o x86_task.o
> diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-cpus.c
> new file mode 100644
> index 0000000000..9540157f1e
> --- /dev/null
> +++ b/target/i386/hvf/hvf-cpus.c

I'd prefer singular form in variables and file names. More on that in
the comment to patch 2.

Besides that it works fine,

Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>

Regards,
Roman

On 8/11/20 11:00 AM, Roman Bolshakov wrote:
> On Mon, Aug 03, 2020 at 11:05:33AM +0200, Claudio Fontana wrote:
>> register a "CpusAccel" interface for HVF as well.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  softmmu/cpus.c                |  63 --------------------
>>  target/i386/hvf/Makefile.objs |   2 +-
>>  target/i386/hvf/hvf-cpus.c    | 131 ++++++++++++++++++++++++++++++++++++++++++
>>  target/i386/hvf/hvf-cpus.h    |  17 ++++++
>>  target/i386/hvf/hvf.c         |   3 +
>>  5 files changed, 152 insertions(+), 64 deletions(-)
>>  create mode 100644 target/i386/hvf/hvf-cpus.c
>>  create mode 100644 target/i386/hvf/hvf-cpus.h
>>
>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>> index 586b4acaab..d327b2685c 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/hvf.h"
> 
> I wonder if the declarations should be moved from sysemu/hvf.h to
> someplace inside target/i386/hvf/:
> 
> int hvf_init_vcpu(CPUState *);
> int hvf_vcpu_exec(CPUState *);
> void hvf_cpu_synchronize_state(CPUState *);
> void hvf_cpu_synchronize_post_reset(CPUState *);
> void hvf_cpu_synchronize_post_init(CPUState *);
> void hvf_cpu_synchronize_pre_loadvm(CPUState *);
> void hvf_vcpu_destroy(CPUState *);
> 
> They're not used outside of target/i386/hvf/
> 
> I also wonder if we need stubs at all?
> 
>>  #include "exec/exec-all.h"
>>  #include "qemu/thread.h"
>>  #include "qemu/plugin.h"
>> @@ -391,48 +390,6 @@ void qemu_wait_io_event(CPUState *cpu)
>>      qemu_wait_io_event_common(cpu);
>>  }
>>  
>> -/* 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)
>> -{
>> -    CPUState *cpu = arg;
>> -
>> -    int r;
>> -
>> -    assert(hvf_enabled());
>> -
>> -    rcu_register_thread();
>> -
>> -    qemu_mutex_lock_iothread();
>> -    qemu_thread_get_self(cpu->thread);
>> -
>> -    cpu->thread_id = qemu_get_thread_id();
>> -    cpu->can_do_io = 1;
>> -    current_cpu = cpu;
>> -
>> -    hvf_init_vcpu(cpu);
>> -
>> -    /* signal CPU creation */
>> -    cpu_thread_signal_created(cpu);
>> -    qemu_guest_random_seed_thread_part2(cpu->random_seed);
>> -
>> -    do {
>> -        if (cpu_can_run(cpu)) {
>> -            r = hvf_vcpu_exec(cpu);
>> -            if (r == EXCP_DEBUG) {
>> -                cpu_handle_guest_debug(cpu);
>> -            }
>> -        }
>> -        qemu_wait_io_event(cpu);
>> -    } while (!cpu->unplug || cpu_can_run(cpu));
>> -
>> -    hvf_vcpu_destroy(cpu);
>> -    cpu_thread_signal_destroyed(cpu);
>> -    qemu_mutex_unlock_iothread();
>> -    rcu_unregister_thread();
>> -    return NULL;
>> -}
>> -
>>  void cpus_kick_thread(CPUState *cpu)
>>  {
>>  #ifndef _WIN32
>> @@ -603,24 +560,6 @@ void cpu_remove_sync(CPUState *cpu)
>>      qemu_mutex_lock_iothread();
>>  }
>>  
>> -static void qemu_hvf_start_vcpu(CPUState *cpu)
>> -{
>> -    char thread_name[VCPU_THREAD_NAME_SIZE];
>> -
>> -    /* HVF currently does not support TCG, and only runs in
>> -     * unrestricted-guest mode. */
>> -    assert(hvf_enabled());
>> -
>> -    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/HVF",
>> -             cpu->cpu_index);
>> -    qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
>> -                       cpu, QEMU_THREAD_JOINABLE);
>> -}
>> -
>>  void cpus_register_accel(CpusAccel *ca)
>>  {
>>      assert(ca != NULL);
>> @@ -648,8 +587,6 @@ void qemu_init_vcpu(CPUState *cpu)
>>      if (cpus_accel) {
>>          /* accelerator already implements the CpusAccel interface */
>>          cpus_accel->create_vcpu_thread(cpu);
>> -    } else if (hvf_enabled()) {
>> -        qemu_hvf_start_vcpu(cpu);
>>      } else {
>>          assert(0);
>>      }
>> diff --git a/target/i386/hvf/Makefile.objs b/target/i386/hvf/Makefile.objs
>> index 927b86bc67..af9f7dcfc1 100644
>> --- a/target/i386/hvf/Makefile.objs
>> +++ b/target/i386/hvf/Makefile.objs
>> @@ -1,2 +1,2 @@
>> -obj-y += hvf.o
>> +obj-y += hvf.o hvf-cpus.o
>>  obj-y += x86.o x86_cpuid.o x86_decode.o x86_descr.o x86_emu.o x86_flags.o x86_mmu.o x86hvf.o x86_task.o
>> diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-cpus.c
>> new file mode 100644
>> index 0000000000..9540157f1e
>> --- /dev/null
>> +++ b/target/i386/hvf/hvf-cpus.c
> 
> I'd prefer singular form in variables and file names. More on that in
> the comment to patch 2.
> 
> Besides that it works fine,
> 
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Regards,
> Roman
> 

Hi Roman,

thanks, sure lets discuss more the naming stuff on patch 2.

I noticed a missing chunk in this patch, ie, it leaves a lingering

} else if (hvf_enabled()) {

in cpu_synchronize_pre_loadvm().

that needs to be elided, should not change the behavior, but who knows. I will respin this one in the next version.

Thank you!

Claudio

On 8/11/20 3:42 PM, Claudio Fontana wrote:
> On 8/11/20 11:00 AM, Roman Bolshakov wrote:
>> On Mon, Aug 03, 2020 at 11:05:33AM +0200, Claudio Fontana wrote:
>>> register a "CpusAccel" interface for HVF as well.
>>>
>>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>>> ---
>>>  softmmu/cpus.c                |  63 --------------------
>>>  target/i386/hvf/Makefile.objs |   2 +-
>>>  target/i386/hvf/hvf-cpus.c    | 131 ++++++++++++++++++++++++++++++++++++++++++
>>>  target/i386/hvf/hvf-cpus.h    |  17 ++++++
>>>  target/i386/hvf/hvf.c         |   3 +
>>>  5 files changed, 152 insertions(+), 64 deletions(-)
>>>  create mode 100644 target/i386/hvf/hvf-cpus.c
>>>  create mode 100644 target/i386/hvf/hvf-cpus.h
>>>
>>> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
>>> index 586b4acaab..d327b2685c 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/hvf.h"
>>
>> I wonder if the declarations should be moved from sysemu/hvf.h to
>> someplace inside target/i386/hvf/:
>>
>> int hvf_init_vcpu(CPUState *);
>> int hvf_vcpu_exec(CPUState *);
>> void hvf_cpu_synchronize_state(CPUState *);
>> void hvf_cpu_synchronize_post_reset(CPUState *);
>> void hvf_cpu_synchronize_post_init(CPUState *);
>> void hvf_cpu_synchronize_pre_loadvm(CPUState *);
>> void hvf_vcpu_destroy(CPUState *);
>>
>> They're not used outside of target/i386/hvf/
>>
>> I also wonder if we need stubs at all?

Ah, missed this,

yes good catch! I think we can remove quite a few stubs and not only for HVF!

Thanks a lot,

Claudio


>>
>>>  #include "exec/exec-all.h"
>>>  #include "qemu/thread.h"
>>>  #include "qemu/plugin.h"
>>> @@ -391,48 +390,6 @@ void qemu_wait_io_event(CPUState *cpu)
>>>      qemu_wait_io_event_common(cpu);
>>>  }
>>>  
>>> -/* 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)
>>> -{
>>> -    CPUState *cpu = arg;
>>> -
>>> -    int r;
>>> -
>>> -    assert(hvf_enabled());
>>> -
>>> -    rcu_register_thread();
>>> -
>>> -    qemu_mutex_lock_iothread();
>>> -    qemu_thread_get_self(cpu->thread);
>>> -
>>> -    cpu->thread_id = qemu_get_thread_id();
>>> -    cpu->can_do_io = 1;
>>> -    current_cpu = cpu;
>>> -
>>> -    hvf_init_vcpu(cpu);
>>> -
>>> -    /* signal CPU creation */
>>> -    cpu_thread_signal_created(cpu);
>>> -    qemu_guest_random_seed_thread_part2(cpu->random_seed);
>>> -
>>> -    do {
>>> -        if (cpu_can_run(cpu)) {
>>> -            r = hvf_vcpu_exec(cpu);
>>> -            if (r == EXCP_DEBUG) {
>>> -                cpu_handle_guest_debug(cpu);
>>> -            }
>>> -        }
>>> -        qemu_wait_io_event(cpu);
>>> -    } while (!cpu->unplug || cpu_can_run(cpu));
>>> -
>>> -    hvf_vcpu_destroy(cpu);
>>> -    cpu_thread_signal_destroyed(cpu);
>>> -    qemu_mutex_unlock_iothread();
>>> -    rcu_unregister_thread();
>>> -    return NULL;
>>> -}
>>> -
>>>  void cpus_kick_thread(CPUState *cpu)
>>>  {
>>>  #ifndef _WIN32
>>> @@ -603,24 +560,6 @@ void cpu_remove_sync(CPUState *cpu)
>>>      qemu_mutex_lock_iothread();
>>>  }
>>>  
>>> -static void qemu_hvf_start_vcpu(CPUState *cpu)
>>> -{
>>> -    char thread_name[VCPU_THREAD_NAME_SIZE];
>>> -
>>> -    /* HVF currently does not support TCG, and only runs in
>>> -     * unrestricted-guest mode. */
>>> -    assert(hvf_enabled());
>>> -
>>> -    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/HVF",
>>> -             cpu->cpu_index);
>>> -    qemu_thread_create(cpu->thread, thread_name, qemu_hvf_cpu_thread_fn,
>>> -                       cpu, QEMU_THREAD_JOINABLE);
>>> -}
>>> -
>>>  void cpus_register_accel(CpusAccel *ca)
>>>  {
>>>      assert(ca != NULL);
>>> @@ -648,8 +587,6 @@ void qemu_init_vcpu(CPUState *cpu)
>>>      if (cpus_accel) {
>>>          /* accelerator already implements the CpusAccel interface */
>>>          cpus_accel->create_vcpu_thread(cpu);
>>> -    } else if (hvf_enabled()) {
>>> -        qemu_hvf_start_vcpu(cpu);
>>>      } else {
>>>          assert(0);
>>>      }
>>> diff --git a/target/i386/hvf/Makefile.objs b/target/i386/hvf/Makefile.objs
>>> index 927b86bc67..af9f7dcfc1 100644
>>> --- a/target/i386/hvf/Makefile.objs
>>> +++ b/target/i386/hvf/Makefile.objs
>>> @@ -1,2 +1,2 @@
>>> -obj-y += hvf.o
>>> +obj-y += hvf.o hvf-cpus.o
>>>  obj-y += x86.o x86_cpuid.o x86_decode.o x86_descr.o x86_emu.o x86_flags.o x86_mmu.o x86hvf.o x86_task.o
>>> diff --git a/target/i386/hvf/hvf-cpus.c b/target/i386/hvf/hvf-cpus.c
>>> new file mode 100644
>>> index 0000000000..9540157f1e
>>> --- /dev/null
>>> +++ b/target/i386/hvf/hvf-cpus.c
>>
>> I'd prefer singular form in variables and file names. More on that in
>> the comment to patch 2.
>>
>> Besides that it works fine,
>>
>> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
>> Tested-by: Roman Bolshakov <r.bolshakov@yadro.com>
>>
>> Regards,
>> Roman
>>
> 
> Hi Roman,
> 
> thanks, sure lets discuss more the naming stuff on patch 2.
> 
> I noticed a missing chunk in this patch, ie, it leaves a lingering
> 
> } else if (hvf_enabled()) {
> 
> in cpu_synchronize_pre_loadvm().
> 
> that needs to be elided, should not change the behavior, but who knows. I will respin this one in the next version.
> 
> Thank you!
> 
> Claudio
> 
> 
>