Re: [Qemu-devel] [RFC PATCH v0 1/5] cpu: Factor out cpu vmstate_[un]register into separate routines

[Igor Mammedov]

On Tue, 5 Jul 2016 12:05:35 +0530
Bharata B Rao <address@hidden> wrote:

> On Tue, Jul 05, 2016 at 07:49:38AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 10:46:07 +0530
> > Bharata B Rao <address@hidden> wrote:
> > 
> > > On Tue, Jul 05, 2016 at 02:56:13PM +1000, David Gibson wrote:
> > > > On Tue, Jul 05, 2016 at 10:12:48AM +0530, Bharata B Rao wrote:
> > > > > Consolidates cpu vmstate_[un]register calls into separate
> > > > > routines. No functionality change except that
> > > > > vmstate_unregister calls are now done under !CONFIG_USER_ONLY
> > > > > to match with vmstate_register calls.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <address@hidden>
> > > > 
> > > > Reviewed-by: David Gibson <address@hidden>
> > > > 
> > > > > ---
> > > > >  exec.c | 47 ++++++++++++++++++++++++++++-------------------
> > > > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/exec.c b/exec.c
> > > > > index 0122ef7..8ce8e90 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);
> > > > >  }
> > > > > +
> > > > > +static 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);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static 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);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > 
> > > > Given you're factoring this out, would it make sense to defined
> > > > no-op versions for CONFIG_USER_ONLY, to reduce the amount of
> > > > ifdefs at the call site?
> > > 
> > > I did that in a subsequent patch that moved the calls to these
> > > routines into cpu_common_[un]realize()cpu_common_[un]realize(),
> > > but ended up in some unrelated issue and hence didn't include
> > > that patch yet.
> > I'd prefer to see it moved to cpu_common_[un]realize() directly
> > without tis intermediate transition as compat logic could be
> > implemented much cleaner if it's there.
> 
> If I implement cpu_common_unrealize() and the associated logic similar
> to the existing cpu_common_realize(), it would involve changes to all
> archs.
maybe I'm missing something but why all arch will be involved?
The only ones that could be affected are the ones that have their own
cpu_xxx_unrealize() implemented without calling CPUClass::unrealize.

 
>I have done the change only to ppc in the below experimental
> patch. Is this kind of change preferred or is it possible to do this
> in non-invasive way ?
> 
> diff --git a/exec.c b/exec.c
> index 2e8ad14..5274cc8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -617,7 +617,7 @@ static void cpu_release_index(CPUState *cpu)
>  }
>  
>  /* TODO: cpu_index is int while .get_arch_id()is int64_t */
> -static void cpu_vmstate_register(CPUState *cpu)
> +void cpu_vmstate_register(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>      int instance_id = cpu->prefer_arch_id_over_cpu_index ?
> @@ -631,7 +631,7 @@ static void cpu_vmstate_register(CPUState *cpu)
>      }
>  }
>  
> -static void cpu_vmstate_unregister(CPUState *cpu)
> +void cpu_vmstate_unregister(CPUState *cpu)
>  {
>      CPUClass *cc = CPU_GET_CLASS(cpu);
>  
> @@ -660,6 +660,14 @@ 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)
> @@ -680,8 +688,6 @@ void cpu_exec_exit(CPUState *cpu)
>      cpu->cpu_index = -1;
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_unregister(cpu);
>  #endif
>  }
>  
> @@ -724,8 +730,6 @@ void cpu_exec_init(CPUState *cpu, Error **errp)
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();
> -#else
> -    cpu_vmstate_register(cpu);
>  #endif
>  }
>  
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 1f1706e..08eab39 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -874,4 +874,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..65623e8 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -314,12 +314,20 @@ 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);
>      }
>  }
>  
> +static void cpu_common_unrealizefn(DeviceState *dev, Error **errp)
> +{
> +    CPUState *cpu = CPU(dev);
> +
> +    cpu_vmstate_unregister(cpu);
> +}
> +
>  static void cpu_common_initfn(Object *obj)
>  {
>      CPUState *cpu = CPU(obj);
> @@ -367,6 +375,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..9b8fe36 100644
> --- a/target-ppc/cpu-qom.h
> +++ b/target-ppc/cpu-qom.h
> @@ -173,6 +173,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 09602be..f4986e4 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9743,10 +9743,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++) {
> @@ -10336,6 +10338,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;
>