| Message ID | 20160705063535.GA25522@in.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 5 Jul 2016 12:05:35 +0530 Bharata B Rao <bharata@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 <bharata@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 <bharata@linux.vnet.ibm.com> > > > > > > > > Reviewed-by: David Gibson <david@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. >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; >
On Tue, 5 Jul 2016 12:05:35 +0530 Bharata B Rao <bharata@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 <bharata@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 <bharata@linux.vnet.ibm.com> > > > > > > > > Reviewed-by: David Gibson <david@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. 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 ? > Even if affects all targets, the change is quite mechanical and should not be a problem I guess. > 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; >
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 <bharata@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 <bharata@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 <bharata@linux.vnet.ibm.com> > > > > > > > > > > Reviewed-by: David Gibson <david@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. Regards, Bharata.
On Tue, 5 Jul 2016 13:08:28 +0530 Bharata B Rao <bharata@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 <bharata@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 <bharata@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 <bharata@linux.vnet.ibm.com> > > > > > > > > > > > > Reviewed-by: David Gibson <david@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. >
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;