Re: [RFC v3 8/8] cpus: extract out hvf-specific code to target/i386/hvf/

[Claudio Fontana]

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
> 
> 
>