Patchwork [08/16] cpu: add helper cpu_exists(), to check if CPU with specified id exists

login
register
mail settings
Submitter Michael S. Tsirkin
Date April 22, 2013, 10:28 a.m.
Message ID <20130422102812.GD3808@redhat.com>
Download mbox | patch
Permalink /patch/238424/
State New
Headers show

Comments

Michael S. Tsirkin - April 22, 2013, 10:28 a.m.
On Tue, Apr 16, 2013 at 12:12:48AM +0200, Igor Mammedov wrote:
> ... it should be used only on slow path since it does recursive search
>     on /machine QOM tree for objects of TYPE_CPU
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

I would prefer qemu_for_each_cpu, that is also useful for ACPI.  And do
we need to scan all QOM?  This will take a while to repeat if there are
many devices.
How about the below? You can add a wrapper that sets
a bit if CPU exists.


commit 4ff1332ff56069574f450bb44819156bd91bd105
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Sun Apr 21 15:44:47 2013 +0300

    exec: add qemu_for_each_cpu
    
    Will be used by ACPI table generation.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Igor Mammedov - April 22, 2013, 10:45 a.m.
On Mon, 22 Apr 2013 13:28:12 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Apr 16, 2013 at 12:12:48AM +0200, Igor Mammedov wrote:
> > ... it should be used only on slow path since it does recursive search
> >     on /machine QOM tree for objects of TYPE_CPU
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I would prefer qemu_for_each_cpu, that is also useful for ACPI.  And do
> we need to scan all QOM?  This will take a while to repeat if there are
> many devices.
> How about the below? You can add a wrapper that sets
> a bit if CPU exists.
qemu_for_each_cpu looks usefull, it could replace a lot of place that do
open-coded exactly this operation and hide open-codded access to next_cpu
pointer. 

I can easily change to using it, and take it this patch into this series.

> 
> 
> commit 4ff1332ff56069574f450bb44819156bd91bd105
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Sun Apr 21 15:44:47 2013 +0300
> 
>     exec: add qemu_for_each_cpu
>     
>     Will be used by ACPI table generation.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/exec.c b/exec.c
> index fa1e0c3..2b44cbe 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -265,6 +265,19 @@ CPUState *qemu_get_cpu(int index)
>      return env ? cpu : NULL;
>  }
>  
> +void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg)
> +{
> +    CPUArchState *env = first_cpu;
> +    CPUState *cpu;
> +
> +    while (env) {
> +        cpu = ENV_GET_CPU(env);
> +        func(cpu, arg);
> +        env = env->next_cpu;
> +    }
> +}
> +
> +
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 3664a1b..db857e3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -223,6 +223,8 @@ void run_on_cpu(CPUState *cpu, void (*func)(void
> *data), void *data); */
>  CPUState *qemu_get_cpu(int index);
>  
> +void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg);
> +
>  #ifndef CONFIG_USER_ONLY
>  
>  typedef void (*CPUInterruptHandler)(CPUState *, int);
>
Andreas Färber - April 22, 2013, 11:15 a.m.
Am 22.04.2013 12:28, schrieb Michael S. Tsirkin:
> On Tue, Apr 16, 2013 at 12:12:48AM +0200, Igor Mammedov wrote:
>> ... it should be used only on slow path since it does recursive search
>>     on /machine QOM tree for objects of TYPE_CPU
>>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> I would prefer qemu_for_each_cpu, that is also useful for ACPI.  And do
> we need to scan all QOM?  This will take a while to repeat if there are
> many devices.
> How about the below? You can add a wrapper that sets
> a bit if CPU exists.

I really like it! Didn't get around to writing such a helper myself yet
- this will allow us to drop quite some CPUArchState usages in the tree.
If you can submit this as a proper patch (git-send-email, with a gtk-doc
comment, is the spacing intentional?) along with a trivial loop ->
helper conversion as proof of concept I'll queue it immediately.

Toda,
Andreas

> commit 4ff1332ff56069574f450bb44819156bd91bd105
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Sun Apr 21 15:44:47 2013 +0300
> 
>     exec: add qemu_for_each_cpu
>     
>     Will be used by ACPI table generation.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> diff --git a/exec.c b/exec.c
> index fa1e0c3..2b44cbe 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -265,6 +265,19 @@ CPUState *qemu_get_cpu(int index)
>      return env ? cpu : NULL;
>  }
>  
> +void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg)
> +{
> +    CPUArchState *env = first_cpu;
> +    CPUState *cpu;
> +
> +    while (env) {
> +        cpu = ENV_GET_CPU(env);
> +        func(cpu, arg);
> +        env = env->next_cpu;
> +    }
> +}
> +
> +
>  void cpu_exec_init(CPUArchState *env)
>  {
>      CPUState *cpu = ENV_GET_CPU(env);
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 3664a1b..db857e3 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -223,6 +223,8 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
>   */
>  CPUState *qemu_get_cpu(int index);
>  
> +void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg);
> +
>  #ifndef CONFIG_USER_ONLY
>  
>  typedef void (*CPUInterruptHandler)(CPUState *, int);
>

Patch

diff --git a/exec.c b/exec.c
index fa1e0c3..2b44cbe 100644
--- a/exec.c
+++ b/exec.c
@@ -265,6 +265,19 @@  CPUState *qemu_get_cpu(int index)
     return env ? cpu : NULL;
 }
 
+void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg)
+{
+    CPUArchState *env = first_cpu;
+    CPUState *cpu;
+
+    while (env) {
+        cpu = ENV_GET_CPU(env);
+        func(cpu, arg);
+        env = env->next_cpu;
+    }
+}
+
+
 void cpu_exec_init(CPUArchState *env)
 {
     CPUState *cpu = ENV_GET_CPU(env);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3664a1b..db857e3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -223,6 +223,8 @@  void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data);
  */
 CPUState *qemu_get_cpu(int index);
 
+void qemu_for_each_cpu( void (*func)( CPUState *c, void *arg), void *arg);
+
 #ifndef CONFIG_USER_ONLY
 
 typedef void (*CPUInterruptHandler)(CPUState *, int);