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

[Alex Bennée]

Claudio Fontana <cfontana@suse.de> writes:

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

Ahh I see - this is once the decision about which accelerator has been
made. I was thinking along the lines of the init functions driven by:

  #define type_init(function) module_init(function, MODULE_INIT_QOM)

which would then populate the list of available accelerators in a more
QOM like manner. I assume having a completely configurable set of
accelerators is the eventual aim of this?

>
>> 
>> <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);
> }

I don't think it has to be quite so extreme. I was just arguing for
something along the lines of:

static CpuAccel *accel;

and

void cpu_synchronize_state(CPUState *cpu)
{
   if (accel && accel->synchronize_state) {
      accel->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:

I don't think a pointer de-reference alone is super critical for
something that happens on the outside of the main run loop. It might be
a different argument if this was somewhere in the hot path.

> Are you arguing in favor of VERSION A) here?

Version C ;-)

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


-- 
Alex Bennée