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

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

[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()) {
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 857b94e..a511de9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1704,13 +1704,6 @@ bool cpu_is_bsp(X86CPU *cpu)
 {
     return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
 }
-
-/* TODO: remove me, when reset over QOM tree is implemented */
-static void x86_cpu_machine_reset_cb(void *opaque)
-{
-    X86CPU *cpu = opaque;
-    cpu_reset(CPU(cpu));
-}
 #endif
 
 static void mce_init(X86CPU *cpu)
@@ -1733,10 +1726,6 @@ void x86_cpu_realize(Object *obj, Error **errp)
 {
     X86CPU *cpu = X86_CPU(obj);
 
-#ifndef CONFIG_USER_ONLY
-    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
-#endif
-
     mce_init(cpu);
     qemu_init_vcpu(&cpu->env);
     cpu_reset(CPU(cpu));
-- 
1.7.7

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

[Andreas Färber]

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
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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Igor Mammedov]

I'm ok with this patch.
It's reduces ugly ifdefs in wonderful realize function. :)

Hence,

Reviewed-By: Igor Mammedov <imammedo@redhat.com>

On 08/01/2012 11:13 PM, Andreas Färber wrote:
> 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()) {
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 857b94e..a511de9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1704,13 +1704,6 @@ bool cpu_is_bsp(X86CPU *cpu)
>   {
>       return cpu_get_apic_base(cpu->env.apic_state) & MSR_IA32_APICBASE_BSP;
>   }
> -
> -/* TODO: remove me, when reset over QOM tree is implemented */
> -static void x86_cpu_machine_reset_cb(void *opaque)
> -{
> -    X86CPU *cpu = opaque;
> -    cpu_reset(CPU(cpu));
> -}
>   #endif
>
>   static void mce_init(X86CPU *cpu)
> @@ -1733,10 +1726,6 @@ void x86_cpu_realize(Object *obj, Error **errp)
>   {
>       X86CPU *cpu = X86_CPU(obj);
>
> -#ifndef CONFIG_USER_ONLY
> -    qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
> -#endif
> -
>       mce_init(cpu);
>       qemu_init_vcpu(&cpu->env);
>       cpu_reset(CPU(cpu));
>

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 <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
>