[RFC v1 06/12] cpus: pass CPUState to run_on_cpu helpers

[RFC v1 06/12] cpus: pass CPUState to run_on_cpu helpers

[Alex Bennée]

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>
---
 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 f7c7359..9177161 100644
--- a/cpus.c
+++ b/cpus.c
@@ -583,9 +583,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;
@@ -615,7 +614,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);
         }
     }
 
@@ -940,12 +939,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;
     }
 
@@ -970,12 +969,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;
     }
 
@@ -1014,7 +1013,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 3c7c8fa..d019724 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -123,10 +123,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 a0439a8..0ec3a96 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -487,7 +487,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;
 
@@ -738,15 +738,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 76bd78b..e2aeef3 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 e7be21e..ad1c261 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2106,10 +2106,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);
 }
@@ -2119,7 +2117,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 2dcb676..4b7fbb7 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -10,19 +10,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;
 }
@@ -31,7 +30,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
@@ -911,11 +909,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 ab08f1a..385d5bb 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;
@@ -627,7 +629,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:
@@ -637,7 +639,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 e7b66df..b31fbba 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -1755,10 +1755,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;
@@ -1768,34 +1766,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)
@@ -2137,7 +2131,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 5755839..1b50f59 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1117,11 +1117,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 87ab969..4ee678c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -145,10 +145,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);
 }
 
@@ -158,7 +156,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 4bfff34..7dc8e82 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -186,7 +186,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
 
@@ -236,7 +236,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 6d97c08..2112994 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -454,17 +454,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);
 }
 
-- 
2.7.4

Re: [RFC v1 06/12] cpus: pass CPUState to run_on_cpu helpers

[Eduardo Habkost]

On Fri, Apr 15, 2016 at 03:23:45PM +0100, Alex Bennée wrote:
> 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>
> ---
>  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 ++-----

What about target-s390x/kvm.c and target-s390x/misc_helper.c?

A few other minor comments/questions below. But the patch looks
good overall.

>  12 files changed, 39 insertions(+), 56 deletions(-)
> 
[...]
> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
> index 76bd78b..e2aeef3 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;

I would set env = &cpu->env to avoid confusion. Then the SpinKick
struct can be removed completely.

>      SpinInfo *curspin = kick->spin;
>      hwaddr map_size = 64 * 1024 * 1024;
[...]
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2dcb676..4b7fbb7 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -10,19 +10,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;
>  }
> @@ -31,7 +30,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
> @@ -911,11 +909,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);

This is not incorrect, but inconsistent: you replaced s->cpu
usage on the first line, but kept using s->cpu in the second
line.

Is there a specific reason you removed SPRSyncState.cs but kept
SetCompatState.cpu?


>  }
>  
[...]
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index 5755839..1b50f59 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1117,11 +1117,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;

I would eliminate MCEInjectionParams.cpu and set cenv = &cpu->env
above, to avoid confusion.

> -    CPUState *cpu = CPU(params->cpu);
>      uint64_t *banks = cenv->mce_banks + 4 * params->bank;
>  
>      cpu_synchronize_state(cpu);
[...]
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 6d97c08..2112994 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -454,17 +454,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);
>  }

The run_on_cpu callers at target-s390x/misc_helper.c still pass
an unnecessary non-NULL void* argument, making the code
confusing.

-- 
Eduardo

Re: [RFC v1 06/12] cpus: pass CPUState to run_on_cpu helpers

[Alex Bennée]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Fri, Apr 15, 2016 at 03:23:45PM +0100, Alex Bennée wrote:
>> 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>
>> ---
>>  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 ++-----
>
> What about target-s390x/kvm.c and target-s390x/misc_helper.c?
>
> A few other minor comments/questions below. But the patch looks
> good overall.

Good catch, I should have cross-built to ensure I got all the kvm based
helpers. I'll fix that up.

>
>>  12 files changed, 39 insertions(+), 56 deletions(-)
>>
> [...]
>> diff --git a/hw/ppc/ppce500_spin.c b/hw/ppc/ppce500_spin.c
>> index 76bd78b..e2aeef3 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;
>
> I would set env = &cpu->env to avoid confusion. Then the SpinKick
> struct can be removed completely.

ok

>
>>      SpinInfo *curspin = kick->spin;
>>      hwaddr map_size = 64 * 1024 * 1024;
> [...]
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 2dcb676..4b7fbb7 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -10,19 +10,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;
>>  }
>> @@ -31,7 +30,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
>> @@ -911,11 +909,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);
>
> This is not incorrect, but inconsistent: you replaced s->cpu
> usage on the first line, but kept using s->cpu in the second
> line.
>
> Is there a specific reason you removed SPRSyncState.cs but kept
> SetCompatState.cpu?

I was trying for minimal change but your right I can do better.

>
>
>>  }
>>
> [...]
>> diff --git a/target-i386/helper.c b/target-i386/helper.c
>> index 5755839..1b50f59 100644
>> --- a/target-i386/helper.c
>> +++ b/target-i386/helper.c
>> @@ -1117,11 +1117,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;
>
> I would eliminate MCEInjectionParams.cpu and set cenv = &cpu->env
> above, to avoid confusion.
>
>> -    CPUState *cpu = CPU(params->cpu);
>>      uint64_t *banks = cenv->mce_banks + 4 * params->bank;
>>
>>      cpu_synchronize_state(cpu);
> [...]
>> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
>> index 6d97c08..2112994 100644
>> --- a/target-s390x/cpu.h
>> +++ b/target-s390x/cpu.h
>> @@ -454,17 +454,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);
>>  }
>
> The run_on_cpu callers at target-s390x/misc_helper.c still pass
> an unnecessary non-NULL void* argument, making the code
> confusing.

Will fix.


--
Alex Bennée