Re: [Qemu-ppc] [RFC PATCH v1 1/5] cpu, target-ppc: Move cpu_vmstate_[un]register calls to cpu_common_[un]realize

[Igor Mammedov]

On Wed,  6 Jul 2016 14:29:17 +0530
Bharata B Rao <address@hidden> wrote:

> Move vmstate_register() call to cpu_common_realize().
> Introduce cpu_common_unrealize() and move vmstate_unregister() to it.
> 
> Change those archs that implement their own CPU unrealize routine to
> mandatorily call CPUClass::unrealize().
> 
> Signed-off-by: Bharata B Rao <address@hidden>
> ---
>  exec.c                      | 53
> ++++++++++++++++++++++++++++-----------------
> include/qom/cpu.h           |  2 ++ qom/cpu.c                   |  7
> ++++++ target-ppc/cpu-qom.h        |  2 ++
>  target-ppc/translate_init.c |  3 +++
>  5 files changed, 47 insertions(+), 20 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 0122ef7..fb73910 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -594,9 +594,7 @@ AddressSpace *cpu_get_address_space(CPUState
> *cpu, int asidx) /* Return the AddressSpace corresponding to the
> specified index */ return cpu->cpu_ases[asidx].as;
>  }
> -#endif
>  
> -#ifndef CONFIG_USER_ONLY
>  static DECLARE_BITMAP(cpu_index_map, MAX_CPUMASK_BITS);
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -617,6 +615,31 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> +    }
> +    if (cc->vmsd != NULL) {
> +        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> +    }
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +
> +    if (cc->vmsd != NULL) {
> +        vmstate_unregister(NULL, cc->vmsd, cpu);
> +    }
> +    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> +        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> +    }
> +}

Is there any reason to keep these in exec.c,
I'd put them in qom/cpu.c


>  #else
>  
>  static int cpu_get_free_index(Error **errp)
> @@ -634,12 +657,18 @@ static void cpu_release_index(CPUState *cpu)
>  {
>      return;
>  }
> +
> +void cpu_vmstate_register(CPUState *cpu)
> +{
> +}
> +
> +void cpu_vmstate_unregister(CPUState *cpu)
> +{
> +}
>  #endif
>  
>  void cpu_exec_exit(CPUState *cpu)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
> -
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -657,18 +686,10 @@ void cpu_exec_exit(CPUState *cpu)
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
>  #endif
> -
> -    if (cc->vmsd != NULL) {
> -        vmstate_unregister(NULL, cc->vmsd, cpu);
> -    }
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
> -    }
>  }
>  
>  void cpu_exec_init(CPUState *cpu, Error **errp)
>  {
> -    CPUClass *cc = CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  
>      cpu->as = NULL;
> @@ -705,15 +726,7 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      }
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
> -    (void) cc;
>      cpu_list_unlock();
> -#else
> -    if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_common,
> cpu);
> -    }
> -    if (cc->vmsd != NULL) {
> -        vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu);
> -    }
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 32f3af3..29ccf5c 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -870,4 +870,6 @@ extern const struct VMStateDescription
> vmstate_cpu_common; .offset =
> 0,                                                            \ }
>  
> +void cpu_vmstate_register(CPUState *cpu);
> +void cpu_vmstate_unregister(CPUState *cpu);
>  #endif
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 751e992..488ecc6 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -310,10 +310,16 @@ static void cpu_common_parse_features(CPUState
> *cpu, char *features, }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    cpu_vmstate_unregister(CPU(dev));
> +}
> +
>  static void cpu_common_realizefn(DeviceState *dev, Error **errp)
>  {
>      CPUState *cpu = CPU(dev);
>  
> +    cpu_vmstate_register(cpu);
>      if (dev->hotplugged) {
>          cpu_synchronize_post_init(cpu);
>          cpu_resume(cpu);
> @@ -367,6 +373,7 @@ static void cpu_class_init(ObjectClass *klass,
> void *data) k->cpu_exec_exit = cpu_common_noop;
>      k->cpu_exec_interrupt = cpu_common_exec_interrupt;
>      dc->realize = cpu_common_realizefn;
> +    dc->unrealize = cpu_common_unrealizefn;
>      /*
>       * Reason: CPUs still need special care by board code: wiring up
>       * IRQs, adding reset handlers, halting non-first CPUs, ...
> diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
> index 2864105..6ec2fca 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -163,6 +163,7 @@ struct ppc_segment_page_sizes;
>  /**
>   * PowerPCCPUClass:
>   * @parent_realize: The parent class' realize handler.
> + * @parent_unrealize: The parent class' unrealize handler.
>   * @parent_reset: The parent class' reset handler.
>   *
>   * A PowerPC CPU model.
> @@ -173,6 +174,7 @@ typedef struct PowerPCCPUClass {
>      /*< public >*/
>  
>      DeviceRealize parent_realize;
> +    DeviceUnrealize parent_unrealize;
>      void (*parent_reset)(CPUState *cpu);
>  
>      uint32_t pvr;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 8f257fb..efd6b88 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9773,10 +9773,12 @@ static void ppc_cpu_realizefn(DeviceState
> *dev, Error **errp) static void ppc_cpu_unrealizefn(DeviceState *dev,
> Error **errp) {
>      PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUPPCState *env = &cpu->env;
>      opc_handler_t **table;
>      int i, j;
>  
> +    pcc->parent_unrealize(dev, errp);
>      cpu_exec_exit(CPU(dev));
>  
>      for (i = 0; i < PPC_CPU_OPCODES_LEN; i++) {
> @@ -10364,6 +10366,7 @@ static void ppc_cpu_class_init(ObjectClass
> *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      pcc->parent_realize = dc->realize;
> +    pcc->parent_unrealize = dc->unrealize;
>      pcc->pvr_match = ppc_pvr_match_default;
>      pcc->interrupts_big_endian =
> ppc_cpu_interrupts_big_endian_always; dc->realize = ppc_cpu_realizefn;