Re: [RFC v3 2/8] cpus: prepare new CpusAccel cpu accelerator interface

[Roman Bolshakov]

On Mon, Aug 03, 2020 at 11:05:27AM +0200, Claudio Fontana wrote:
> The new interface starts unused, will start being used by the
> next patches.
> 
> It provides methods for each accelerator to start a vcpu, kick a vcpu,
> synchronize state, get cpu virtual clock and elapsed ticks.
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  hw/core/cpu.c                  |   1 +
>  hw/i386/x86.c                  |   2 +-
>  include/sysemu/cpu-timers.h    |   9 +-
>  include/sysemu/cpus.h          |  36 ++++++++
>  include/sysemu/hw_accel.h      |  69 ++-------------
>  softmmu/cpu-timers.c           |   9 +-
>  softmmu/cpus.c                 | 194 
> ++++++++++++++++++++++++++++++++---------
>  stubs/Makefile.objs            |   2 +
>  stubs/cpu-synchronize-state.c  |  15 ++++
>  stubs/cpus-get-virtual-clock.c |   8 ++
>  util/qemu-timer.c              |   8 +-
>  11 files changed, 231 insertions(+), 122 deletions(-)
>  create mode 100644 stubs/cpu-synchronize-state.c
>  create mode 100644 stubs/cpus-get-virtual-clock.c
> 
> diff --git a/hw/core/cpu.c b/hw/core/cpu.c
> index 594441a150..b389a312df 100644
> --- a/hw/core/cpu.c
> +++ b/hw/core/cpu.c
> @@ -33,6 +33,7 @@
>  #include "hw/qdev-properties.h"
>  #include "trace-root.h"
>  #include "qemu/plugin.h"
> +#include "sysemu/hw_accel.h"
>  
>  CPUInterruptHandler cpu_interrupt_handler;
>  
> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
> index 58cf2229d5..00c35bad7e 100644
> --- a/hw/i386/x86.c
> +++ b/hw/i386/x86.c
> @@ -264,7 +264,7 @@ static long get_file_size(FILE *f)
>  /* TSC handling */
>  uint64_t cpu_get_tsc(CPUX86State *env)
>  {
> -    return cpu_get_ticks();
> +    return cpus_get_elapsed_ticks();

Hi Claudio,

I still don't understand why plural form of "cpus" is used in files,
CpusAccel interface name and cpus_ prefix of the functions/variables.

Original cpus.c had functions to create CPU threads for multiple
accelerators, that justified naming of cpus.c. It had TCG, KVM and other
kinds of vCPUs. After you factor cpus.c into separate implementations of
CPU interface it should get singular form.

I’m not a native English speaker but the naming looks confusing to me.

>  }
>  
>  /* IRQ handling */
> diff --git a/softmmu/cpus.c b/softmmu/cpus.c
> index 54fdb2761c..bad6302ca3 100644
> --- a/softmmu/cpus.c
> +++ b/softmmu/cpus.c
> @@ -87,7 +87,7 @@ bool cpu_is_stopped(CPUState *cpu)
>      return cpu->stopped || !runstate_is_running();
>  }
>  
> -static inline bool cpu_work_list_empty(CPUState *cpu)
> +bool cpu_work_list_empty(CPUState *cpu)
>  {
>      bool ret;
>  
> @@ -97,7 +97,7 @@ static inline bool cpu_work_list_empty(CPUState *cpu)
>      return ret;
>  }
>  
> -static bool cpu_thread_is_idle(CPUState *cpu)
> +bool cpu_thread_is_idle(CPUState *cpu)
>  {
>      if (cpu->stop || !cpu_work_list_empty(cpu)) {
>          return false;
> @@ -215,6 +215,11 @@ void hw_error(const char *fmt, ...)
>      abort();
>  }
>  
> +/*
> + * The chosen accelerator is supposed to register this.
> + */
> +static CpusAccel *cpus_accel;
> +
>  void cpu_synchronize_all_states(void)
>  {
>      CPUState *cpu;
> @@ -251,6 +256,102 @@ void cpu_synchronize_all_pre_loadvm(void)
>      }
>  }
>  
> +void cpu_synchronize_state(CPUState *cpu)
> +{
> +    if (cpus_accel && cpus_accel->synchronize_state) {
> +        cpus_accel->synchronize_state(cpu);

I think the condition can be removed altogether if you move it to the
bootom inside else body. cpu_interrupt_handler and cpu_interrupt() in
hw/core/cpu.c is an example of that. Likely cpu_interrupt_handler should
be part of the accel interface. You might also avoid indirected function
call by using standalone fuction pointer. Like that:


void cpu_synchronize_state(CPUState *cpu)
{
    if (cpus_accel && cpus_accel->synchronize_state) {
        cpus_accel->synchronize_state(cpu);
    }
    if (kvm_enabled()) {
        kvm_cpu_synchronize_state(cpu);
    }
    else if (hax_enabled()) {
        hax_cpu_synchronize_state(cpu);
    }
    else if (whpx_enabled()) {
        whpx_cpu_synchronize_state(cpu);
    } else {
        cpu_synchronize_state_handler(cpu);
    }
}

After you finish factoring, it becomes:


void cpu_synchronize_state(CPUState *cpu)
{
    cpu_synchronize_state_handler(cpu);
}

cpu_register_accel would just assign non-NULL function pointer
from a CPUAccel field over generic_cpu_synchronize_state_handler.

Regards,
Roman

> +    }
> +    if (kvm_enabled()) {
> +        kvm_cpu_synchronize_state(cpu);
> +    }
> +    if (hax_enabled()) {
> +        hax_cpu_synchronize_state(cpu);
> +    }
> +    if (whpx_enabled()) {
> +        whpx_cpu_synchronize_state(cpu);
> +    }
> +}
> +