On Tue, 5 Jul 2016 13:08:28 +0530
Bharata B Rao <bhar...@linux.vnet.ibm.com> wrote:
> On Tue, Jul 05, 2016 at 09:22:30AM +0200, Igor Mammedov wrote:
> > On Tue, 5 Jul 2016 12:05:35 +0530
> > Bharata B Rao <bhar...@linux.vnet.ibm.com> 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 <bhar...@linux.vnet.ibm.com> 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 <bhar...@linux.vnet.ibm.com>
> > > > > >
> > > > > > Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> > > > > >
> > > > > > > ---
> > > > > > > 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.
>
> You are right, I didn't realize that most archs don't define their own
> cpu_xxx_unrealize call. So this should be a simple change. Will
> include a patch to move vmstate_[un]register() calls to
> cpu_common_[un]realize() in the next version.
just replace this patch with cpu_common_[un]realize() variant
>
> Regards,
> Bharata.
>
