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

[Igor Mammedov]

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.

> 
> But as you note, we could do that with the existing code itself.
> Commit 741da0d3 limited the vmstate_register calls
> to !CONFIG_USER_ONLY. I am still not sure why cpu vmstate
> registration isn't needed for CONFIG_USER_ONLY
there isn't migration for *-user targets but exec.c is common, hence
ifdefs
 
> 
> Regards,
> Bharata.
>