Re: [RFC v5 4/4] cpus: extract out accel-specific code to each accel

[Claudio Fontana]

Hi Alex,

thanks for looking at this,

On 6/16/20 4:16 PM, Alex Bennée wrote:
> 
> Claudio Fontana <cfontana@suse.de> writes:
> 
>> each accelerator registers a new "CpusAccel" interface
>> implementation on initialization, providing functions for
>> starting a vcpu, kicking a vcpu, and sychronizing state.
>>
>> This way the code in cpus.c is now all general softmmu code,
>> nothing accelerator-specific anymore.
>>
>> There is still some ifdeffery for WIN32 though.
>>
>> Signed-off-by: Claudio Fontana <cfontana@suse.de>
>> ---
>>  MAINTAINERS                   |   1 +
>>  accel/Makefile.objs           |   2 +-
>>  accel/kvm/Makefile.objs       |   2 +
>>  accel/kvm/kvm-all.c           |  15 +-
>>  accel/kvm/kvm-cpus.c          |  94 +++++
>>  accel/kvm/kvm-cpus.h          |  17 +
>>  accel/qtest/Makefile.objs     |   2 +
>>  accel/qtest/qtest-cpus.c      | 105 +++++
>>  accel/qtest/qtest-cpus.h      |  17 +
>>  accel/{ => qtest}/qtest.c     |   7 +
>>  accel/stubs/kvm-stub.c        |   3 +-
>>  accel/tcg/Makefile.objs       |   1 +
>>  accel/tcg/tcg-all.c           |  12 +-
>>  accel/tcg/tcg-cpus.c          | 523 ++++++++++++++++++++++++
>>  accel/tcg/tcg-cpus.h          |  17 +
>>  hw/core/cpu.c                 |   1 +
>>  include/sysemu/cpus.h         |  32 ++
>>  include/sysemu/hw_accel.h     |  57 +--
>>  include/sysemu/kvm.h          |   2 +-
>>  softmmu/cpus.c                | 911 
>> ++++--------------------------------------
>>  stubs/Makefile.objs           |   1 +
>>  stubs/cpu-synchronize-state.c |  15 +
>>  target/i386/Makefile.objs     |   7 +-
>>  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/hvf/Makefile.objs |   2 +-
>>  target/i386/hvf/hvf-cpus.c    | 141 +++++++
>>  target/i386/hvf/hvf-cpus.h    |  17 +
>>  target/i386/hvf/hvf.c         |   3 +
>>  target/i386/whpx-all.c        |   3 +
>>  target/i386/whpx-cpus.c       |  96 +++++
>>  target/i386/whpx-cpus.h       |  17 +
>>  36 files changed, 1362 insertions(+), 903 deletions(-)
>>  create mode 100644 accel/kvm/kvm-cpus.c
>>  create mode 100644 accel/kvm/kvm-cpus.h
>>  create mode 100644 accel/qtest/Makefile.objs
>>  create mode 100644 accel/qtest/qtest-cpus.c
>>  create mode 100644 accel/qtest/qtest-cpus.h
>>  rename accel/{ => qtest}/qtest.c (86%)
>>  create mode 100644 accel/tcg/tcg-cpus.c
>>  create mode 100644 accel/tcg/tcg-cpus.h
>>  create mode 100644 stubs/cpu-synchronize-state.c
>>  create mode 100644 target/i386/hax-cpus.c
>>  create mode 100644 target/i386/hax-cpus.h
>>  create mode 100644 target/i386/hvf/hvf-cpus.c
>>  create mode 100644 target/i386/hvf/hvf-cpus.h
>>  create mode 100644 target/i386/whpx-cpus.c
>>  create mode 100644 target/i386/whpx-cpus.h
> 
> Predictably for such a spider patch I got a bunch of conflicts
> attempting to merge on my testing branch so only a few comments.
> 
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index f308537d42..ef8cbb2680 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -427,6 +427,7 @@ WHPX CPUs
>>  M: Sunil Muthuswamy <sunilmut@microsoft.com>
>>  S: Supported
>>  F: target/i386/whpx-all.c
>> +F: target/i386/whpx-cpus.c
>>  F: target/i386/whp-dispatch.h
>>  F: accel/stubs/whpx-stub.c
>>  F: include/sysemu/whpx.h
>> diff --git a/accel/Makefile.objs b/accel/Makefile.objs
>> index ff72f0d030..c5e58eb53d 100644
>> --- a/accel/Makefile.objs
>> +++ b/accel/Makefile.objs
>> @@ -1,5 +1,5 @@
>>  common-obj-$(CONFIG_SOFTMMU) += accel.o
>> -obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest.o
>> +obj-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_POSIX)) += qtest/
> 
> This does raise the question if qtest is "just another" accelerator then
> should we not be creating a CONFIG_QTEST symbol for explicitness?
> 
>>  obj-$(CONFIG_KVM) += kvm/
>>  obj-$(CONFIG_TCG) += tcg/
>>  obj-$(CONFIG_XEN) += xen/
> <snip>
>> +static void *qtest_cpu_thread_fn(void *arg)
>> +{
>> +#ifdef _WIN32
>> +    error_report("qtest is not supported under Windows");
>> +    exit(1);
>> +#else
> 
> This is literally impossible to build isn't it?
>>  
>>  static int qtest_init_accel(MachineState *ms)
>>  {
>> +    cpus_register_accel(&qtest_cpus);
>>      return 0;
>>  }
> 
> I wonder if these register functions could be moved to initfns like we
> use for our hardware models?

The context is the configure_accelerator() in vl.c , where we loop over 
possible candidate accelerators
and try to initialize them.

In this RFC the cpus_register_accel is triggered at accel_init_machine() time,
in the accelerator class init_machine() method, where we are trying to use a 
specific accelerator.

This is the case for qtest like for the other AccelClass types (tcg and the 
hardware accelerators).

If not in init_machine(), where would the registration best happen?

> 
> <snip>
>>  
>> +/*
>> + * every accelerator is supposed to register this.
>> + * Could be in the AccelClass instead, but ends up being too complicated
>> + * to access in practice, and inefficient for each call of each method.
>> + */
>> +static CpusAccel cpus_accel;
>> +
> 
> wait what? Does an indirection cause that much trouble? I'm surprised
> given how often we use it elsewhere in the code. I guess others might

CpusAccel is not used elsewhere currently in the codebase, it's new, or what do 
you mean?

> argue for a full QOM-ification of the accelerator but I think we can at
> least have an indirection rather than a copy of the structure.
> 
> 

As mentioned in v3 and v2, this is what we end up if we put CpusAccel inside 
the AccelClass,
every time we need a vcpu kick, sync state, etc:

1) current_accel() function call
2) pointer dereference (->accelerator)
3) object_class_dynamic_cast_assert function call (ACCEL_GET_CLASS -> 
OBJECT_CLASS_CHECK)
4) pointer dereference (-> AccelCpusInterface)
5) pointer dereference (-> method)
6) function call ( ->synchronize_state(cpu))

So the code then would look like this (more or less, probably I would put also 
an assert for non-NULL in there):

VERSION A)

void cpu_synchronize_state(CPUState *cpu)
{
    ACCEL_GET_CLASS(current_accel())->cpus_int->synchronize_state(cpu);
}

Instead with the current RFC code, this is what we end up with every time we 
need a vcpu kick, sync state, etc:

VERSION B)

void cpu_synchronize_state(CPUState *cpu)
{
    cpus_accel.synchronize_state(cpu);
}


Are you arguing in favor of VERSION A) here?

I would like to have an ACK from the owners of the hardware accels especially 
that the additional overhead in this code path
is of negligible importance..


Thank you for your comments,

Ciao,

Claudio