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

[Igor Mammedov <imammedo@redhat.com>]

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 <afaerber@suse.de>
>> Cc: Anthony Liguori <anthony@codemonkey.ws>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> ---
>>   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
>