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@xxxxxxxxxx>
> ---
> 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 = ¶ms->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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
