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

["Alex Bennée" <alex.bennee@linaro.org>]

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