| Message ID | 1467693772-7391-2-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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? > #else > > static int cpu_get_free_index(Error **errp) > @@ -638,8 +661,6 @@ static void cpu_release_index(CPUState *cpu) > > void cpu_exec_exit(CPUState *cpu) > { > - CPUClass *cc = CPU_GET_CLASS(cpu); > - > #if defined(CONFIG_USER_ONLY) > cpu_list_lock(); > #endif > @@ -656,19 +677,13 @@ void cpu_exec_exit(CPUState *cpu) > cpu->cpu_index = -1; > #if defined(CONFIG_USER_ONLY) > cpu_list_unlock(); > +#else > + cpu_vmstate_unregister(cpu); > #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 +720,9 @@ 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); > - } > + cpu_vmstate_register(cpu); > #endif > } >
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(), but ended up in some unrelated issue and hence didn't include that patch yet. 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 Regards, Bharata.
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. > > 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. >
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); + } +} + #else static int cpu_get_free_index(Error **errp) @@ -638,8 +661,6 @@ static void cpu_release_index(CPUState *cpu) void cpu_exec_exit(CPUState *cpu) { - CPUClass *cc = CPU_GET_CLASS(cpu); - #if defined(CONFIG_USER_ONLY) cpu_list_lock(); #endif @@ -656,19 +677,13 @@ void cpu_exec_exit(CPUState *cpu) cpu->cpu_index = -1; #if defined(CONFIG_USER_ONLY) cpu_list_unlock(); +#else + cpu_vmstate_unregister(cpu); #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 +720,9 @@ 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); - } + cpu_vmstate_register(cpu); #endif }
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> --- exec.c | 47 ++++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-)