| Message ID | 1365172636-28628-7-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Reviewed-by: liguang <lig.fnst@cn.fujitsu.com> 在 2013-04-05五的 16:36 +0200，Igor Mammedov写道： > ... and call it if defined from CPUClass.realize() if CPU was hotplugged > > by default leave .resume() unset (i.e. NULL) and override it for softmmu > in qemu_init_vcpu() if it's still unset. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > cpus.c | 15 ++++++++++++--- > include/qom/cpu.h | 2 ++ > qom/cpu.c | 5 +++++ > 3 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/cpus.c b/cpus.c > index 9b9a32f..6b793c5 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -973,6 +973,13 @@ void pause_all_vcpus(void) > } > } > > +static void resume_vcpu(CPUState *cpu) > +{ > + cpu->stop = false; > + cpu->stopped = false; > + qemu_cpu_kick(cpu); > +} > + > void resume_all_vcpus(void) > { > CPUArchState *penv = first_cpu; > @@ -980,9 +987,7 @@ void resume_all_vcpus(void) > qemu_clock_enable(vm_clock, true); > while (penv) { > CPUState *pcpu = ENV_GET_CPU(penv); > - pcpu->stop = false; > - pcpu->stopped = false; > - qemu_cpu_kick(pcpu); > + resume_vcpu(pcpu); > penv = penv->next_cpu; > } > } > @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env) > { > CPUArchState *env = _env; > CPUState *cpu = ENV_GET_CPU(env); > + CPUClass *klass = CPU_GET_CLASS(cpu); > > + if (klass->resume == NULL) { > + klass->resume = resume_vcpu; > + } > cpu->nr_cores = smp_cores; > cpu->nr_threads = smp_threads; > cpu->stopped = true; > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 3664a1b..6d6eb7a 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; > * instantiatable CPU type. > * @reset: Callback to reset the #CPUState to its initial state. > * @do_interrupt: Callback for interrupt handling. > + * @resume: Callback for putting CPU in runable state > * @vmsd: State description for migration. > * > * Represents a CPU family or model. > @@ -58,6 +59,7 @@ typedef struct CPUClass { > > void (*reset)(CPUState *cpu); > void (*do_interrupt)(CPUState *cpu); > + void (*resume)(CPUState *cpu); > > const struct VMStateDescription *vmsd; > } CPUClass; > diff --git a/qom/cpu.c b/qom/cpu.c > index 0c76712..c062e00 100644 > --- a/qom/cpu.c > +++ b/qom/cpu.c > @@ -58,8 +58,13 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model) > > static void cpu_common_realizefn(DeviceState *dev, Error **errp) > { > + CPUClass *klass = CPU_GET_CLASS(dev); > + > if (dev->hotplugged) { > cpu_synchronize_post_init(CPU(dev)); > + if (klass->resume != NULL) { > + klass->resume(CPU(dev)); > + } > } > } >
On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote: > ... and call it if defined from CPUClass.realize() if CPU was hotplugged > > by default leave .resume() unset (i.e. NULL) and override it for softmmu > in qemu_init_vcpu() if it's still unset. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> [...] > diff --git a/cpus.c b/cpus.c > index 9b9a32f..6b793c5 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -973,6 +973,13 @@ void pause_all_vcpus(void) [...] > @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env) > { > CPUArchState *env = _env; > CPUState *cpu = ENV_GET_CPU(env); > + CPUClass *klass = CPU_GET_CLASS(cpu); > > + if (klass->resume == NULL) { > + klass->resume = resume_vcpu; > + } So you are initializing a field of CPUClass struct inside a CPU object initialization function. And that's a function that is not even converted to QOM yet, and buried inside a non-trivial function call tree (hence easy to be called at the wrong time if one day we reorder the initialization steps). Can't we do this on class_init(), where it belongs? If we need different implementations for softmmu/user, we can add a stub for *-user. I think even an explicit #ifdef inside resume_vcpu() would be preferable to this. > cpu->nr_cores = smp_cores; > cpu->nr_threads = smp_threads; > cpu->stopped = true; [...]
On Mon, 8 Apr 2013 17:13:11 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote: > > ... and call it if defined from CPUClass.realize() if CPU was hotplugged > > > > by default leave .resume() unset (i.e. NULL) and override it for softmmu > > in qemu_init_vcpu() if it's still unset. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > [...] > > diff --git a/cpus.c b/cpus.c > > index 9b9a32f..6b793c5 100644 > > --- a/cpus.c > > +++ b/cpus.c > > @@ -973,6 +973,13 @@ void pause_all_vcpus(void) > [...] > > @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env) > > { > > CPUArchState *env = _env; > > CPUState *cpu = ENV_GET_CPU(env); > > + CPUClass *klass = CPU_GET_CLASS(cpu); > > > > + if (klass->resume == NULL) { > > + klass->resume = resume_vcpu; > > + } > > So you are initializing a field of CPUClass struct inside a CPU object > initialization function. And that's a function that is not even > converted to QOM yet, and buried inside a non-trivial function call tree > (hence easy to be called at the wrong time if one day we reorder the > initialization steps). init step are not likely to change this drastically but main reason why it's here see below. > > Can't we do this on class_init(), where it belongs? If we need different > implementations for softmmu/user, we can add a stub for *-user. I think > even an explicit #ifdef inside resume_vcpu() would be preferable to > this. Generally I agree with you that class_init() would be more correct, but ifdefs in qom/cpu.c defeat purpose to build qom/cpu.c only once for all targets, that Andreas are working towards. It asks surely asks for resume_vcpu() stub in libqemustubs, and I'd do it there weren't objections to it. Paolo, it looks like a good candidate for libqemustubs, would it be ok to add stub there? > > > > cpu->nr_cores = smp_cores; > > cpu->nr_threads = smp_threads; > > cpu->stopped = true; > [...] >
Il 08/04/2013 22:13, Eduardo Habkost ha scritto: > So you are initializing a field of CPUClass struct inside a CPU object > initialization function. And that's a function that is not even > converted to QOM yet, and buried inside a non-trivial function call tree > (hence easy to be called at the wrong time if one day we reorder the > initialization steps). > > Can't we do this on class_init(), where it belongs? If we need different > implementations for softmmu/user, we can add a stub for *-user. Yes, please add a stub for the new function and override it in cpus.c. > I think even an explicit #ifdef inside resume_vcpu() would be > preferable to this. Using an #ifdef basically means putting it in exec.c. I'm not sure about that, it seems to fit more in cpus.c. Paolo
Am 09.04.2013 12:26, schrieb Igor Mammedov: > On Mon, 8 Apr 2013 17:13:11 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > >> On Fri, Apr 05, 2013 at 04:36:58PM +0200, Igor Mammedov wrote: >>> ... and call it if defined from CPUClass.realize() if CPU was hotplugged >>> >>> by default leave .resume() unset (i.e. NULL) and override it for softmmu >>> in qemu_init_vcpu() if it's still unset. >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> [...] >>> diff --git a/cpus.c b/cpus.c >>> index 9b9a32f..6b793c5 100644 >>> --- a/cpus.c >>> +++ b/cpus.c >>> @@ -973,6 +973,13 @@ void pause_all_vcpus(void) >> [...] >>> @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env) >>> { >>> CPUArchState *env = _env; >>> CPUState *cpu = ENV_GET_CPU(env); >>> + CPUClass *klass = CPU_GET_CLASS(cpu); >>> >>> + if (klass->resume == NULL) { >>> + klass->resume = resume_vcpu; >>> + } >> >> So you are initializing a field of CPUClass struct inside a CPU object >> initialization function. And that's a function that is not even >> converted to QOM yet, and buried inside a non-trivial function call tree >> (hence easy to be called at the wrong time if one day we reorder the >> initialization steps). > init step are not likely to change this drastically but main reason why > it's here see below. > >> >> Can't we do this on class_init(), where it belongs? If we need different >> implementations for softmmu/user, we can add a stub for *-user. I think >> even an explicit #ifdef inside resume_vcpu() would be preferable to >> this. > Generally I agree with you that class_init() would be more correct, but > ifdefs in qom/cpu.c defeat purpose to build qom/cpu.c only once for all > targets, that Andreas are working towards. Indeed. Anthony had suggested to introduce a cpu-softmmu.c or something like that it we ever need to distinguish. Not sure how they would interface though. ;) Andreas > > It asks surely asks for resume_vcpu() stub in libqemustubs, and I'd do it > there weren't objections to it. > > Paolo, > it looks like a good candidate for libqemustubs, would it be ok to add stub > there? > >> >> >>> cpu->nr_cores = smp_cores; >>> cpu->nr_threads = smp_threads; >>> cpu->stopped = true; >> [...] >> >
On Tue, 09 Apr 2013 13:20:27 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 08/04/2013 22:13, Eduardo Habkost ha scritto: > > So you are initializing a field of CPUClass struct inside a CPU object > > initialization function. And that's a function that is not even > > converted to QOM yet, and buried inside a non-trivial function call tree > > (hence easy to be called at the wrong time if one day we reorder the > > initialization steps). > > > > Can't we do this on class_init(), where it belongs? If we need different > > implementations for softmmu/user, we can add a stub for *-user. > > Yes, please add a stub for the new function and override it in cpus.c. Ok, I'll revert it to a way as it was in RFC http://lists.gnu.org/archive/html/qemu-devel/2013-03/msg03774.html minus crept in cpu_synchronize_post_init() line. There is no much use in introducing resume method when there are only one implementation and stub. > > > I think even an explicit #ifdef inside resume_vcpu() would be > > preferable to this. > > Using an #ifdef basically means putting it in exec.c. I'm not sure > about that, it seems to fit more in cpus.c. > > Paolo >
diff --git a/cpus.c b/cpus.c index 9b9a32f..6b793c5 100644 --- a/cpus.c +++ b/cpus.c @@ -973,6 +973,13 @@ void pause_all_vcpus(void) } } +static void resume_vcpu(CPUState *cpu) +{ + cpu->stop = false; + cpu->stopped = false; + qemu_cpu_kick(cpu); +} + void resume_all_vcpus(void) { CPUArchState *penv = first_cpu; @@ -980,9 +987,7 @@ void resume_all_vcpus(void) qemu_clock_enable(vm_clock, true); while (penv) { CPUState *pcpu = ENV_GET_CPU(penv); - pcpu->stop = false; - pcpu->stopped = false; - qemu_cpu_kick(pcpu); + resume_vcpu(pcpu); penv = penv->next_cpu; } } @@ -1042,7 +1047,11 @@ void qemu_init_vcpu(void *_env) { CPUArchState *env = _env; CPUState *cpu = ENV_GET_CPU(env); + CPUClass *klass = CPU_GET_CLASS(cpu); + if (klass->resume == NULL) { + klass->resume = resume_vcpu; + } cpu->nr_cores = smp_cores; cpu->nr_threads = smp_threads; cpu->stopped = true; diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 3664a1b..6d6eb7a 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -45,6 +45,7 @@ typedef struct CPUState CPUState; * instantiatable CPU type. * @reset: Callback to reset the #CPUState to its initial state. * @do_interrupt: Callback for interrupt handling. + * @resume: Callback for putting CPU in runable state * @vmsd: State description for migration. * * Represents a CPU family or model. @@ -58,6 +59,7 @@ typedef struct CPUClass { void (*reset)(CPUState *cpu); void (*do_interrupt)(CPUState *cpu); + void (*resume)(CPUState *cpu); const struct VMStateDescription *vmsd; } CPUClass; diff --git a/qom/cpu.c b/qom/cpu.c index 0c76712..c062e00 100644 --- a/qom/cpu.c +++ b/qom/cpu.c @@ -58,8 +58,13 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model) static void cpu_common_realizefn(DeviceState *dev, Error **errp) { + CPUClass *klass = CPU_GET_CLASS(dev); + if (dev->hotplugged) { cpu_synchronize_post_init(CPU(dev)); + if (klass->resume != NULL) { + klass->resume(CPU(dev)); + } } }
... and call it if defined from CPUClass.realize() if CPU was hotplugged by default leave .resume() unset (i.e. NULL) and override it for softmmu in qemu_init_vcpu() if it's still unset. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- cpus.c | 15 ++++++++++++--- include/qom/cpu.h | 2 ++ qom/cpu.c | 5 +++++ 3 files changed, 19 insertions(+), 3 deletions(-)