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 <cfont...@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.bolsha...@yadro.com>
> Tested-by: Roman Bolshakov <r.bolsha...@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
