Re: [Qemu-devel] [PATCH for-1.2] cpus: Register reset callback centrally

[Igor Mammedov]

On 08/02/2012 12:44 AM, Andreas Färber wrote:
> Am 01.08.2012 23:13, schrieb Andreas Färber:
> > Despite repeated protest commit 65dee38052597b6285eb208125369f01b29ba6c1
> > (target-i386: move cpu_reset and reset callback to cpu.c) moved
> > registration of a reset callback from hw/pc.c to target-i386/cpu.c
> > while all other CPU reset handlers were still registered from machines.
> >
> > Instead, improve the situation by registering the callback in
> > qemu_init_vcpu(). Now new machines or CPUs no longer need to register
> > their own callbacks, and we can revert the code in target-i386/cpu.c.
> >
> > Signed-off-by: Andreas Färber <address@hidden>
> > Cc: Anthony Liguori <address@hidden>
> > Cc: Igor Mammedov <address@hidden>
> > ---
> >   cpus.c            |    8 ++++++++
> >   target-i386/cpu.c |   11 -----------
> >   2 files changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/cpus.c b/cpus.c
> > index 756e624..f717a61 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1020,6 +1020,13 @@ static void qemu_dummy_start_vcpu(CPUArchState *env)
> >       }
> >   }
> >
> > +static void cpu_machine_reset(void *opaque)
> > +{
> > +    CPUState *cpu = opaque;
> > +
> > +    cpu_reset(cpu);
> > +}
> > +
> >   void qemu_init_vcpu(void *_env)
> >   {
> >       CPUArchState *env = _env;
> > @@ -1027,6 +1034,7 @@ void qemu_init_vcpu(void *_env)
> >       env->nr_cores = smp_cores;
> >       env->nr_threads = smp_threads;
> >       env->stopped = 1;
> > +    qemu_register_reset(cpu_machine_reset, ENV_GET_CPU(env));
> >       if (kvm_enabled()) {
> >           qemu_kvm_start_vcpu(env);
> >       } else if (tcg_enabled()) {
> [snip]
>
> Note that this is safe as long as qemu_init_vcpu() happens inside
> cpu_foo_init(), the current convention. I believe Igor's APIC/BSP patch
about happens inside cpu_foo_init(): could you explain why it's safe 
only there?

for x86 qemu_kvm_start_vcpu(env) is inside of realize() function, and in 
case of using qdev_device_add() this call won't happen inside of 
cpu_foo_init().

And in future with completely qomified CPUs we could probably get rid of 
cpu_foo_init() and cpu_init() altogether.

> that just got committed replaced an earlier attempt to change that.
> Further reset handlers would then be registered after this one and IIUC
> executed in registration order so that worst case cpu_reset() would be
> executed multiple times if not dropped from machines' reset callbacks.
>
> What is missing here though is unregistration of the new reset callback
> for future hot unplug. But waiting for comments before I drill some
> qemu_finalize_vcpu() API callable from qom/cpu.c.
>
> Andreas