[RFC,1/8] cpus: pass CPUState to run_on_cpu helpers

From: Alex Bennée <alex.bennee@linaro.org>

CPUState is a fairly common pointer to pass to these helpers. This means
if you need other arguments for the async_run_on_cpu case you end up
having to do a g_malloc to stuff additional data into the routine. For
the current users this isn't a massive deal but for MTTCG this gets
cumbersome when the only other parameter is often an address.

This adds the typedef run_on_cpu_func for helper functions which has an
explicit CPUState * passed as the first parameter. All the users of
run_on_cpu and async_run_on_cpu have had their helpers updated to use
CPUState where available.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
---
 cpus.c                | 15 +++++++--------
 hw/i386/kvm/apic.c    |  3 +--
 hw/i386/kvmvapic.c    |  8 ++++----
 hw/ppc/ppce500_spin.c |  3 +--
 hw/ppc/spapr.c        |  6 ++----
 hw/ppc/spapr_hcall.c  | 12 +++++-------
 include/qom/cpu.h     |  8 +++++---
 kvm-all.c             | 20 +++++++-------------
 target-i386/helper.c  |  3 +--
 target-i386/kvm.c     |  6 ++----
 target-s390x/cpu.c    |  4 ++--
 target-s390x/cpu.h    |  7 ++-----
 12 files changed, 39 insertions(+), 56 deletions(-)

On Mon, Jun 20, 2016 at 01:28:26AM +0300, Sergey Fedorov wrote:
> From: Alex Bennée <alex.bennee@linaro.org>
> 
> CPUState is a fairly common pointer to pass to these helpers. This means
> if you need other arguments for the async_run_on_cpu case you end up
> having to do a g_malloc to stuff additional data into the routine. For
> the current users this isn't a massive deal but for MTTCG this gets
> cumbersome when the only other parameter is often an address.
> 
> This adds the typedef run_on_cpu_func for helper functions which has an
> explicit CPUState * passed as the first parameter. All the users of
> run_on_cpu and async_run_on_cpu have had their helpers updated to use
> CPUState where available.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

[snip]

> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa255a946..a427492c0310 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2159,10 +2159,8 @@ static void spapr_machine_finalizefn(Object *obj)
>      g_free(spapr->kvm_type);
>  }
>  
> -static void ppc_cpu_do_nmi_on_cpu(void *arg)
> +static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
>  {
> -    CPUState *cs = arg;
> -
>      cpu_synchronize_state(cs);
>      ppc_cpu_do_system_reset(cs);
>  }
> @@ -2172,7 +2170,7 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>      CPUState *cs;
>  
>      CPU_FOREACH(cs) {
> -        async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, cs);
> +        async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, NULL);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2ba5cbdb194a..22d57469b8a5 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -13,19 +13,18 @@
>  #include "kvm_ppc.h"
>  
>  struct SPRSyncState {
> -    CPUState *cs;
>      int spr;
>      target_ulong value;
>      target_ulong mask;
>  };
>  
> -static void do_spr_sync(void *arg)
> +static void do_spr_sync(CPUState *cs, void *arg)
>  {
>      struct SPRSyncState *s = arg;
> -    PowerPCCPU *cpu = POWERPC_CPU(s->cs);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
>  
> -    cpu_synchronize_state(s->cs);
> +    cpu_synchronize_state(cs);
>      env->spr[s->spr] &= ~s->mask;
>      env->spr[s->spr] |= s->value;
>  }
> @@ -34,7 +33,6 @@ static void set_spr(CPUState *cs, int spr, target_ulong value,
>                      target_ulong mask)
>  {
>      struct SPRSyncState s = {
> -        .cs = cs,
>          .spr = spr,
>          .value = value,
>          .mask = mask
> @@ -908,11 +906,11 @@ typedef struct {
>      Error *err;
>  } SetCompatState;

You've missed the opportunity here to remove the cpu field from
SetCompatState, as you did with SPRSyncState.

>  
> -static void do_set_compat(void *arg)
> +static void do_set_compat(CPUState *cs, void *arg)
>  {
>      SetCompatState *s = arg;
>  
> -    cpu_synchronize_state(CPU(s->cpu));
> +    cpu_synchronize_state(cs);
>      ppc_set_compat(s->cpu, s->cpu_version, &s->err);
>  }

Sergey Fedorov <sergey.fedorov@linaro.org> writes:

> From: Alex Bennée <alex.bennee@linaro.org>
>
> CPUState is a fairly common pointer to pass to these helpers. This means
> if you need other arguments for the async_run_on_cpu case you end up
> having to do a g_malloc to stuff additional data into the routine. For
> the current users this isn't a massive deal but for MTTCG this gets
> cumbersome when the only other parameter is often an address.
>
> This adds the typedef run_on_cpu_func for helper functions which has an
> explicit CPUState * passed as the first parameter. All the users of
> run_on_cpu and async_run_on_cpu have had their helpers updated to use
> CPUState where available.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>

Is this as is or did you apply the fixes from the review comments?

> ---
>  cpus.c                | 15 +++++++--------
>  hw/i386/kvm/apic.c    |  3 +--
>  hw/i386/kvmvapic.c    |  8 ++++----
>  hw/ppc/ppce500_spin.c |  3 +--
>  hw/ppc/spapr.c        |  6 ++----
>  hw/ppc/spapr_hcall.c  | 12 +++++-------
>  include/qom/cpu.h     |  8 +++++---
>  kvm-all.c             | 20 +++++++-------------
>  target-i386/helper.c  |  3 +--
>  target-i386/kvm.c     |  6 ++----
>  target-s390x/cpu.c    |  4 ++--
>  target-s390x/cpu.h    |  7 ++-----
>  12 files changed, 39 insertions(+), 56 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 84c3520d446f..049c2d04e150 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -551,9 +551,8 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>
> -static void cpu_throttle_thread(void *opaque)
> +static void cpu_throttle_thread(CPUState *cpu, void *opaque)
>  {
> -    CPUState *cpu = opaque;
>      double pct;
>      double throttle_ratio;
>      long sleeptime_ns;
> @@ -583,7 +582,7 @@ static void cpu_throttle_timer_tick(void *opaque)
>      }
>      CPU_FOREACH(cpu) {
>          if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
> -            async_run_on_cpu(cpu, cpu_throttle_thread, cpu);
> +            async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>          }
>      }
>
> @@ -911,12 +910,12 @@ void qemu_init_cpu_loop(void)
>      qemu_thread_get_self(&io_thread);
>  }
>
> -void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
> +void run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data)
>  {
>      struct qemu_work_item wi;
>
>      if (qemu_cpu_is_self(cpu)) {
> -        func(data);
> +        func(cpu, data);
>          return;
>      }
>
> @@ -944,12 +943,12 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
>      }
>  }
>
> -void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
> +void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data)
>  {
>      struct qemu_work_item *wi;
>
>      if (qemu_cpu_is_self(cpu)) {
> -        func(data);
> +        func(cpu, data);
>          return;
>      }
>
> @@ -1000,7 +999,7 @@ static void flush_queued_work(CPUState *cpu)
>              cpu->queued_work_last = NULL;
>          }
>          qemu_mutex_unlock(&cpu->work_mutex);
> -        wi->func(wi->data);
> +        wi->func(cpu, wi->data);
>          qemu_mutex_lock(&cpu->work_mutex);
>          if (wi->free) {
>              g_free(wi);
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index c5983c79be47..9b66e741d4b4 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -125,10 +125,9 @@ static void kvm_apic_vapic_base_update(APICCommonState *s)
>      }
>  }
>
> -static void do_inject_external_nmi(void *data)
> +static void do_inject_external_nmi(CPUState *cpu, void *data)
>  {
>      APICCommonState *s = data;
> -    CPUState *cpu = CPU(s->cpu);
>      uint32_t lvt;
>      int ret;
>
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 3bf1ddd97612..8063d695312e 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -483,7 +483,7 @@ typedef struct VAPICEnableTPRReporting {
>      bool enable;
>  } VAPICEnableTPRReporting;
>
> -static void vapic_do_enable_tpr_reporting(void *data)
> +static void vapic_do_enable_tpr_reporting(CPUState *cpu, void *data)
>  {
>      VAPICEnableTPRReporting *info = data;
>
> @@ -734,15 +734,15 @@ static void vapic_realize(DeviceState *dev, Error **errp)
>      nb_option_roms++;
>  }
>
> -static void do_vapic_enable(void *data)
> +static void do_vapic_enable(CPUState *cpu, void *data)
>  {
>      VAPICROMState *s = data;
> -    X86CPU *cpu = X86_CPU(first_cpu);
> +    X86CPU *x86_cpu = X86_CPU(cpu);
>
>      static const uint8_t enabled = 1;
>      cpu_physical_memory_write(s->vapic_paddr + offsetof(VAPICState, enabled),
>                                &enabled, sizeof(enabled));
> -    apic_enable_vapic(cpu->apic_state, s->vapic_paddr);
> +    apic_enable_vapic(x86_cpu->apic_state, s->vapic_paddr);
>      s->state = VAPIC_ACTIVE;
>  }
>
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index 76bd78bfd7a6..e2aeef3701ec 100644
> --- a/hw/ppc/ppce500_spin.c
> +++ b/hw/ppc/ppce500_spin.c
> @@ -94,10 +94,9 @@ static void mmubooke_create_initial_mapping(CPUPPCState *env,
>      env->tlb_dirty = true;
>  }
>
> -static void spin_kick(void *data)
> +static void spin_kick(CPUState *cpu, void *data)
>  {
>      SpinKick *kick = data;
> -    CPUState *cpu = CPU(kick->cpu);
>      CPUPPCState *env = &kick->cpu->env;
>      SpinInfo *curspin = kick->spin;
>      hwaddr map_size = 64 * 1024 * 1024;
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 778fa255a946..a427492c0310 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2159,10 +2159,8 @@ static void spapr_machine_finalizefn(Object *obj)
>      g_free(spapr->kvm_type);
>  }
>
> -static void ppc_cpu_do_nmi_on_cpu(void *arg)
> +static void ppc_cpu_do_nmi_on_cpu(CPUState *cs, void *arg)
>  {
> -    CPUState *cs = arg;
> -
>      cpu_synchronize_state(cs);
>      ppc_cpu_do_system_reset(cs);
>  }
> @@ -2172,7 +2170,7 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>      CPUState *cs;
>
>      CPU_FOREACH(cs) {
> -        async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, cs);
> +        async_run_on_cpu(cs, ppc_cpu_do_nmi_on_cpu, NULL);
>      }
>  }
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2ba5cbdb194a..22d57469b8a5 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -13,19 +13,18 @@
>  #include "kvm_ppc.h"
>
>  struct SPRSyncState {
> -    CPUState *cs;
>      int spr;
>      target_ulong value;
>      target_ulong mask;
>  };
>
> -static void do_spr_sync(void *arg)
> +static void do_spr_sync(CPUState *cs, void *arg)
>  {
>      struct SPRSyncState *s = arg;
> -    PowerPCCPU *cpu = POWERPC_CPU(s->cs);
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
>      CPUPPCState *env = &cpu->env;
>
> -    cpu_synchronize_state(s->cs);
> +    cpu_synchronize_state(cs);
>      env->spr[s->spr] &= ~s->mask;
>      env->spr[s->spr] |= s->value;
>  }
> @@ -34,7 +33,6 @@ static void set_spr(CPUState *cs, int spr, target_ulong value,
>                      target_ulong mask)
>  {
>      struct SPRSyncState s = {
> -        .cs = cs,
>          .spr = spr,
>          .value = value,
>          .mask = mask
> @@ -908,11 +906,11 @@ typedef struct {
>      Error *err;
>  } SetCompatState;
>
> -static void do_set_compat(void *arg)
> +static void do_set_compat(CPUState *cs, void *arg)
>  {
>      SetCompatState *s = arg;
>
> -    cpu_synchronize_state(CPU(s->cpu));
> +    cpu_synchronize_state(cs);
>      ppc_set_compat(s->cpu, s->cpu_version, &s->err);
>  }
>
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3e1c63..4e688f645b4a 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -223,9 +223,11 @@ struct kvm_run;
>  #define TB_JMP_CACHE_SIZE (1 << TB_JMP_CACHE_BITS)
>
>  /* work queue */
> +typedef void (*run_on_cpu_func)(CPUState *cpu, void *data);
> +
>  struct qemu_work_item {
>      struct qemu_work_item *next;
> -    void (*func)(void *data);
> +    run_on_cpu_func func;
>      void *data;
>      int done;
>      bool free;
> @@ -610,7 +612,7 @@ bool cpu_is_stopped(CPUState *cpu);
>   *
>   * Schedules the function @func for execution on the vCPU @cpu.
>   */
> -void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
> +void run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data);
>
>  /**
>   * async_run_on_cpu:
> @@ -620,7 +622,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
>   *
>   * Schedules the function @func for execution on the vCPU @cpu asynchronously.
>   */
> -void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
> +void async_run_on_cpu(CPUState *cpu, run_on_cpu_func func, void *data);
>
>  /**
>   * qemu_get_cpu:
> diff --git a/kvm-all.c b/kvm-all.c
> index a88f917fda69..a2ee20cfb7f3 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -1828,10 +1828,8 @@ void kvm_flush_coalesced_mmio_buffer(void)
>      s->coalesced_flush_in_progress = false;
>  }
>
> -static void do_kvm_cpu_synchronize_state(void *arg)
> +static void do_kvm_cpu_synchronize_state(CPUState *cpu, void *arg)
>  {
> -    CPUState *cpu = arg;
> -
>      if (!cpu->kvm_vcpu_dirty) {
>          kvm_arch_get_registers(cpu);
>          cpu->kvm_vcpu_dirty = true;
> @@ -1841,34 +1839,30 @@ static void do_kvm_cpu_synchronize_state(void *arg)
>  void kvm_cpu_synchronize_state(CPUState *cpu)
>  {
>      if (!cpu->kvm_vcpu_dirty) {
> -        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, cpu);
> +        run_on_cpu(cpu, do_kvm_cpu_synchronize_state, NULL);
>      }
>  }
>
> -static void do_kvm_cpu_synchronize_post_reset(void *arg)
> +static void do_kvm_cpu_synchronize_post_reset(CPUState *cpu, void *arg)
>  {
> -    CPUState *cpu = arg;
> -
>      kvm_arch_put_registers(cpu, KVM_PUT_RESET_STATE);
>      cpu->kvm_vcpu_dirty = false;
>  }
>
>  void kvm_cpu_synchronize_post_reset(CPUState *cpu)
>  {
> -    run_on_cpu(cpu, do_kvm_cpu_synchronize_post_reset, cpu);
> +    run_on_cpu(cpu, do_kvm_cpu_synchronize_post_reset, NULL);
>  }
>
> -static void do_kvm_cpu_synchronize_post_init(void *arg)
> +static void do_kvm_cpu_synchronize_post_init(CPUState *cpu, void *arg)
>  {
> -    CPUState *cpu = arg;
> -
>      kvm_arch_put_registers(cpu, KVM_PUT_FULL_STATE);
>      cpu->kvm_vcpu_dirty = false;
>  }
>
>  void kvm_cpu_synchronize_post_init(CPUState *cpu)
>  {
> -    run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, cpu);
> +    run_on_cpu(cpu, do_kvm_cpu_synchronize_post_init, NULL);
>  }
>
>  int kvm_cpu_exec(CPUState *cpu)
> @@ -2210,7 +2204,7 @@ struct kvm_set_guest_debug_data {
>      int err;
>  };
>
> -static void kvm_invoke_set_guest_debug(void *data)
> +static void kvm_invoke_set_guest_debug(CPUState *unused_cpu, void *data)
>  {
>      struct kvm_set_guest_debug_data *dbg_data = data;
>
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 1c250b82452d..2e438fc1dd8f 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1122,11 +1122,10 @@ typedef struct MCEInjectionParams {
>      int flags;
>  } MCEInjectionParams;
>
> -static void do_inject_x86_mce(void *data)
> +static void do_inject_x86_mce(CPUState *cpu, void *data)
>  {
>      MCEInjectionParams *params = data;
>      CPUX86State *cenv = &params->cpu->env;
> -    CPUState *cpu = CPU(params->cpu);
>      uint64_t *banks = cenv->mce_banks + 4 * params->bank;
>
>      cpu_synchronize_state(cpu);
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index ff92b1d118e1..6a239cd16c0f 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -151,10 +151,8 @@ static int kvm_get_tsc(CPUState *cs)
>      return 0;
>  }
>
> -static inline void do_kvm_synchronize_tsc(void *arg)
> +static inline void do_kvm_synchronize_tsc(CPUState *cpu, void *arg)
>  {
> -    CPUState *cpu = arg;
> -
>      kvm_get_tsc(cpu);
>  }
>
> @@ -164,7 +162,7 @@ void kvm_synchronize_all_tsc(void)
>
>      if (kvm_enabled()) {
>          CPU_FOREACH(cpu) {
> -            run_on_cpu(cpu, do_kvm_synchronize_tsc, cpu);
> +            run_on_cpu(cpu, do_kvm_synchronize_tsc, NULL);
>          }
>      }
>  }
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index e43e2d61550b..4f09c647b7a8 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -188,7 +188,7 @@ static void s390_cpu_machine_reset_cb(void *opaque)
>  {
>      S390CPU *cpu = opaque;
>
> -    run_on_cpu(CPU(cpu), s390_do_cpu_full_reset, CPU(cpu));
> +    run_on_cpu(CPU(cpu), s390_do_cpu_full_reset, NULL);
>  }
>  #endif
>
> @@ -238,7 +238,7 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
>      s390_cpu_gdb_init(cs);
>      qemu_init_vcpu(cs);
>  #if !defined(CONFIG_USER_ONLY)
> -    run_on_cpu(cs, s390_do_cpu_full_reset, cs);
> +    run_on_cpu(cs, s390_do_cpu_full_reset, NULL);
>  #else
>      cpu_reset(cs);
>  #endif
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index bd6b2e57ef6c..3ae5d2d7de3f 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -499,17 +499,14 @@ static inline hwaddr decode_basedisp_s(CPUS390XState *env, uint32_t ipb,
>  #define decode_basedisp_rs decode_basedisp_s
>
>  /* helper functions for run_on_cpu() */
> -static inline void s390_do_cpu_reset(void *arg)
> +static inline void s390_do_cpu_reset(CPUState *cs, void *arg)
>  {
> -    CPUState *cs = arg;
>      S390CPUClass *scc = S390_CPU_GET_CLASS(cs);
>
>      scc->cpu_reset(cs);
>  }
> -static inline void s390_do_cpu_full_reset(void *arg)
> +static inline void s390_do_cpu_full_reset(CPUState *cs, void *arg)
>  {
> -    CPUState *cs = arg;
> -
>      cpu_reset(cs);
>  }


--
Alex Bennée

On 20/06/16 16:02, Alex Bennée wrote:
> Sergey Fedorov <sergey.fedorov@linaro.org> writes:
>
>> > From: Alex Bennée <alex.bennee@linaro.org>
>> >
>> > CPUState is a fairly common pointer to pass to these helpers. This means
>> > if you need other arguments for the async_run_on_cpu case you end up
>> > having to do a g_malloc to stuff additional data into the routine. For
>> > the current users this isn't a massive deal but for MTTCG this gets
>> > cumbersome when the only other parameter is often an address.
>> >
>> > This adds the typedef run_on_cpu_func for helper functions which has an
>> > explicit CPUState * passed as the first parameter. All the users of
>> > run_on_cpu and async_run_on_cpu have had their helpers updated to use
>> > CPUState where available.
>> >
>> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> > Signed-off-by: Sergey Fedorov <sergey.fedorov@linaro.org>
> Is this as is or did you apply the fixes from the review comments?
>
It's "as is" so far. I could take charge of this patch in some later
re-spin.

Kind regards,
Sergey