[Qemu-devel] [PATCH 0/6] target-i386: Remove side-effects from X86CPU::instance_init

[Qemu-devel] [PATCH 0/6] target-i386: Remove side-effects from X86CPU::instance_init

[Eduardo Habkost]

To allow new code to ask the CPU classes for CPU model information and
allow QOM properties to be queried by qmp_device_list_properties(), we
need to be able to safely instantiate a X86CPU object without any
side-effects.

This series moves some code from x86_cpu_initfn() to x86_cpu_realizefn(), so
that QEMU global state is affected only if the CPU object is realized.

To make the patch moving the cpu_exec_init() call easier to review, the series
also includes a few small changes to simplify cpu_exec_init().

This series is based on my x86 tree, located at:
  https://github.com/ehabkost/qemu.git x86

Eduardo Habkost (6):
  cpu: No need to zero-initialize numa_node
  cpu: Initialize breakpoint/watchpoint lists on cpu_common_initfn()
  cpu: Reorder cpu->as and cpu->thread_id initialization
  target-i386: Rename optimize_flags_init()
  target-i386: Move TCG initialization to realize time
  target-i386: Call cpu_exec_init() on realize

 exec.c                  | 12 +++++-------
 qom/cpu.c               |  2 ++
 target-i386/cpu.c       | 16 ++++++++--------
 target-i386/cpu.h       |  2 +-
 target-i386/translate.c |  2 +-
 5 files changed, 17 insertions(+), 17 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] cpu: No need to zero-initialize numa_node

[Eduardo Habkost]

QOM objects are already zero-filled when instantiated, there's no need
to explicitly set numa_node to 0.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/exec.c b/exec.c
index c85321a..3a61e51 100644
--- a/exec.c
+++ b/exec.c
@@ -542,7 +542,6 @@ void cpu_exec_init(CPUArchState *env)
         cpu_index++;
     }
     cpu->cpu_index = cpu_index;
-    cpu->numa_node = 0;
     QTAILQ_INIT(&cpu->breakpoints);
     QTAILQ_INIT(&cpu->watchpoints);
 #ifndef CONFIG_USER_ONLY
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/6] cpu: No need to zero-initialize numa_node

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:45 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> QOM objects are already zero-filled when instantiated, there's no need
> to explicitly set numa_node to 0.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index c85321a..3a61e51 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -542,7 +542,6 @@ void cpu_exec_init(CPUArchState *env)
>          cpu_index++;
>      }
>      cpu->cpu_index = cpu_index;
> -    cpu->numa_node = 0;
>      QTAILQ_INIT(&cpu->breakpoints);
>      QTAILQ_INIT(&cpu->watchpoints);
>  #ifndef CONFIG_USER_ONLY

[Qemu-devel] [PATCH 2/6] cpu: Initialize breakpoint/watchpoint lists on cpu_common_initfn()

[Eduardo Habkost]

One small step in the simplification of cpu_exec_init().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c    | 2 --
 qom/cpu.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index 3a61e51..8220535 100644
--- a/exec.c
+++ b/exec.c
@@ -542,8 +542,6 @@ void cpu_exec_init(CPUArchState *env)
         cpu_index++;
     }
     cpu->cpu_index = cpu_index;
-    QTAILQ_INIT(&cpu->breakpoints);
-    QTAILQ_INIT(&cpu->watchpoints);
 #ifndef CONFIG_USER_ONLY
     cpu->as = &address_space_memory;
     cpu->thread_id = qemu_get_thread_id();
diff --git a/qom/cpu.c b/qom/cpu.c
index 970377e..b69ac41 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -313,6 +313,8 @@ static void cpu_common_initfn(Object *obj)
     CPUClass *cc = CPU_GET_CLASS(obj);
 
     cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
+    QTAILQ_INIT(&cpu->breakpoints);
+    QTAILQ_INIT(&cpu->watchpoints);
 }
 
 static int64_t cpu_common_get_arch_id(CPUState *cpu)
-- 
2.1.0

Re: [Qemu-devel] [PATCH 2/6] cpu: Initialize breakpoint/watchpoint lists on cpu_common_initfn()

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> One small step in the simplification of cpu_exec_init().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c    | 2 --
>  qom/cpu.c | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 3a61e51..8220535 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -542,8 +542,6 @@ void cpu_exec_init(CPUArchState *env)
>          cpu_index++;
>      }
>      cpu->cpu_index = cpu_index;
> -    QTAILQ_INIT(&cpu->breakpoints);
> -    QTAILQ_INIT(&cpu->watchpoints);
>  #ifndef CONFIG_USER_ONLY
>      cpu->as = &address_space_memory;
>      cpu->thread_id = qemu_get_thread_id();
> diff --git a/qom/cpu.c b/qom/cpu.c
> index 970377e..b69ac41 100644
> --- a/qom/cpu.c
> +++ b/qom/cpu.c
> @@ -313,6 +313,8 @@ static void cpu_common_initfn(Object *obj)
>      CPUClass *cc = CPU_GET_CLASS(obj);
>  
>      cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
> +    QTAILQ_INIT(&cpu->breakpoints);
> +    QTAILQ_INIT(&cpu->watchpoints);
>  }
>  
>  static int64_t cpu_common_get_arch_id(CPUState *cpu)

[Qemu-devel] [PATCH 3/6] cpu: Reorder cpu->as and cpu->thread_id initialization

[Eduardo Habkost]

Instead of initializing cpu->as and cpu->thread_id while holding
cpu_list_lock(), initialize it earlier.

This allows the code handling cpu_index and global CPU list to be
isolated from the rest.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 exec.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/exec.c b/exec.c
index 8220535..2e370d0 100644
--- a/exec.c
+++ b/exec.c
@@ -534,6 +534,11 @@ void cpu_exec_init(CPUArchState *env)
     CPUState *some_cpu;
     int cpu_index;
 
+#ifndef CONFIG_USER_ONLY
+    cpu->as = &address_space_memory;
+    cpu->thread_id = qemu_get_thread_id();
+#endif
+
 #if defined(CONFIG_USER_ONLY)
     cpu_list_lock();
 #endif
@@ -542,10 +547,6 @@ void cpu_exec_init(CPUArchState *env)
         cpu_index++;
     }
     cpu->cpu_index = cpu_index;
-#ifndef CONFIG_USER_ONLY
-    cpu->as = &address_space_memory;
-    cpu->thread_id = qemu_get_thread_id();
-#endif
     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
 #if defined(CONFIG_USER_ONLY)
     cpu_list_unlock();
-- 
2.1.0

Re: [Qemu-devel] [PATCH 3/6] cpu: Reorder cpu->as and cpu->thread_id initialization

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:47 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of initializing cpu->as and cpu->thread_id while holding
> cpu_list_lock(), initialize it earlier.
> 
> This allows the code handling cpu_index and global CPU list to be
> isolated from the rest.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  exec.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 8220535..2e370d0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -534,6 +534,11 @@ void cpu_exec_init(CPUArchState *env)
>      CPUState *some_cpu;
>      int cpu_index;
>  
> +#ifndef CONFIG_USER_ONLY
> +    cpu->as = &address_space_memory;
> +    cpu->thread_id = qemu_get_thread_id();
> +#endif
> +
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_lock();
>  #endif
> @@ -542,10 +547,6 @@ void cpu_exec_init(CPUArchState *env)
>          cpu_index++;
>      }
>      cpu->cpu_index = cpu_index;
> -#ifndef CONFIG_USER_ONLY
> -    cpu->as = &address_space_memory;
> -    cpu->thread_id = qemu_get_thread_id();
> -#endif
>      QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>  #if defined(CONFIG_USER_ONLY)
>      cpu_list_unlock();

[Qemu-devel] [PATCH 4/6] target-i386: Rename optimize_flags_init()

[Eduardo Habkost]

Rename the function so that the reason for its existence is clearer: it
does x86-specific initialization of TCG structures.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c       | 2 +-
 target-i386/cpu.h       | 2 +-
 target-i386/translate.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 50907d0..b4e70d3 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2883,7 +2883,7 @@ static void x86_cpu_initfn(Object *obj)
     /* init various static tables used in TCG mode */
     if (tcg_enabled() && !inited) {
         inited = 1;
-        optimize_flags_init();
+        tcg_x86_init();
     }
 }
 
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 0638d24..52b460a 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1228,7 +1228,7 @@ static inline target_long lshift(target_long x, int n)
 #define ST1    ST(1)
 
 /* translate.c */
-void optimize_flags_init(void);
+void tcg_x86_init(void);
 
 #include "exec/cpu-all.h"
 #include "svm.h"
diff --git a/target-i386/translate.c b/target-i386/translate.c
index 094cec0..f19f20f 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7852,7 +7852,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
     return s->pc;
 }
 
-void optimize_flags_init(void)
+void tcg_x86_init(void)
 {
     static const char reg_names[CPU_NB_REGS][4] = {
 #ifdef TARGET_X86_64
-- 
2.1.0

Re: [Qemu-devel] [PATCH 4/6] target-i386: Rename optimize_flags_init()

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:48 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Rename the function so that the reason for its existence is clearer: it
> does x86-specific initialization of TCG structures.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c       | 2 +-
>  target-i386/cpu.h       | 2 +-
>  target-i386/translate.c | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 50907d0..b4e70d3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2883,7 +2883,7 @@ static void x86_cpu_initfn(Object *obj)
>      /* init various static tables used in TCG mode */
>      if (tcg_enabled() && !inited) {
>          inited = 1;
> -        optimize_flags_init();
> +        tcg_x86_init();
>      }
how about moving 'inited' handling inside of tcg_x86_init() along with renaming?

>  }
>  
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 0638d24..52b460a 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -1228,7 +1228,7 @@ static inline target_long lshift(target_long x, int n)
>  #define ST1    ST(1)
>  
>  /* translate.c */
> -void optimize_flags_init(void);
> +void tcg_x86_init(void);
>  
>  #include "exec/cpu-all.h"
>  #include "svm.h"
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 094cec0..f19f20f 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7852,7 +7852,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
>      return s->pc;
>  }
>  
> -void optimize_flags_init(void)
> +void tcg_x86_init(void)
>  {
>      static const char reg_names[CPU_NB_REGS][4] = {
>  #ifdef TARGET_X86_64

Re: [Qemu-devel] [PATCH 4/6] target-i386: Rename optimize_flags_init()

[Eduardo Habkost]

On Thu, Mar 05, 2015 at 05:31:39PM +0100, Igor Mammedov wrote:
> On Thu,  5 Mar 2015 12:38:48 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Rename the function so that the reason for its existence is clearer: it
> > does x86-specific initialization of TCG structures.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c       | 2 +-
> >  target-i386/cpu.h       | 2 +-
> >  target-i386/translate.c | 2 +-
> >  3 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 50907d0..b4e70d3 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2883,7 +2883,7 @@ static void x86_cpu_initfn(Object *obj)
> >      /* init various static tables used in TCG mode */
> >      if (tcg_enabled() && !inited) {
> >          inited = 1;
> > -        optimize_flags_init();
> > +        tcg_x86_init();
> >      }
> how about moving 'inited' handling inside of tcg_x86_init() along with renaming?

Makes sense, and it will help simplify patch 5/6. But I'll do that in a
separate patch.

> 
> >  }
> >  
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 0638d24..52b460a 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -1228,7 +1228,7 @@ static inline target_long lshift(target_long x, int n)
> >  #define ST1    ST(1)
> >  
> >  /* translate.c */
> > -void optimize_flags_init(void);
> > +void tcg_x86_init(void);
> >  
> >  #include "exec/cpu-all.h"
> >  #include "svm.h"
> > diff --git a/target-i386/translate.c b/target-i386/translate.c
> > index 094cec0..f19f20f 100644
> > --- a/target-i386/translate.c
> > +++ b/target-i386/translate.c
> > @@ -7852,7 +7852,7 @@ static target_ulong disas_insn(CPUX86State *env, DisasContext *s,
> >      return s->pc;
> >  }
> >  
> > -void optimize_flags_init(void)
> > +void tcg_x86_init(void)
> >  {
> >      static const char reg_names[CPU_NB_REGS][4] = {
> >  #ifdef TARGET_X86_64
> 

-- 
Eduardo

[Qemu-devel] [PATCH 5/6] target-i386: Move TCG initialization to realize time

[Eduardo Habkost]

To ask the CPU classes for CPU model information and allow QOM
properties to be queried by qmp_device_list_properties(), we need to be
able to safely instantiate a X86CPU object without any side-effects.

Move TCG initialization to realize time so it won't be called when just
doing object_new() on a X86CPU subclass.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b4e70d3..400b1e0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2756,6 +2756,12 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
     static bool ht_warned;
+    static bool tcg_initialized;
+
+    if (tcg_enabled() && !tcg_initialized) {
+        tcg_initialized = 1;
+        tcg_x86_init();
+    }
 
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
@@ -2832,7 +2838,6 @@ static void x86_cpu_initfn(Object *obj)
     X86CPU *cpu = X86_CPU(obj);
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
-    static int inited;
 
     cs->env_ptr = env;
     cpu_exec_init(env);
@@ -2879,12 +2884,6 @@ static void x86_cpu_initfn(Object *obj)
 #endif
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
-
-    /* init various static tables used in TCG mode */
-    if (tcg_enabled() && !inited) {
-        inited = 1;
-        tcg_x86_init();
-    }
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] target-i386: Call cpu_exec_init() on realize

[Eduardo Habkost]

To allow new code to ask the CPU classes for CPU model information and
allow QOM properties to be queried by qmp_device_list_properties(), we
need to be able to safely instantiate a X86CPU object without any
side-effects.

cpu_exec_init() has lots of side-effects on global QEMU state, move it
to realize so it will be called only if the X86CPU instance is realized.

For reference, this is the current cpu_exec_init() code:

> void cpu_exec_init(CPUArchState *env)
> {
>     CPUState *cpu = ENV_GET_CPU(env);
>     CPUClass *cc = CPU_GET_CLASS(cpu);
>     CPUState *some_cpu;
>     int cpu_index;
>
> #ifndef CONFIG_USER_ONLY
>     cpu->as = &address_space_memory;
>     cpu->thread_id = qemu_get_thread_id();
> #endif

Those fields should be used only after actually starting the VCPU and can be
initialized on realize.

>
> #if defined(CONFIG_USER_ONLY)
>     cpu_list_lock();
> #endif
>     cpu_index = 0;
>     CPU_FOREACH(some_cpu) {
>         cpu_index++;
>     }
>     cpu->cpu_index = cpu_index;
>     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
> #if defined(CONFIG_USER_ONLY)
>     cpu_list_unlock();
> #endif

The above initializes cpu_index and add the CPU to the global CPU list.
This affects QEMU global state and must be done only on realize.

>     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>     }
> #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>                     cpu_save, cpu_load, env);
>     assert(cc->vmsd == NULL);
>     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
> #endif
>     if (cc->vmsd != NULL) {
>         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>     }

vmstate and savevm registration also affects global QEMU state and should be
done only on realize.

> }

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 400b1e0..8b76604 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2758,6 +2758,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     static bool ht_warned;
     static bool tcg_initialized;
 
+    cpu_exec_init(env);
+
     if (tcg_enabled() && !tcg_initialized) {
         tcg_initialized = 1;
         tcg_x86_init();
@@ -2840,7 +2842,6 @@ static void x86_cpu_initfn(Object *obj)
     CPUX86State *env = &cpu->env;
 
     cs->env_ptr = env;
-    cpu_exec_init(env);
 
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
-- 
2.1.0

Re: [Qemu-devel] [PATCH 6/6] target-i386: Call cpu_exec_init() on realize

[Igor Mammedov]

On Thu,  5 Mar 2015 12:38:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> To allow new code to ask the CPU classes for CPU model information and
> allow QOM properties to be queried by qmp_device_list_properties(), we
> need to be able to safely instantiate a X86CPU object without any
> side-effects.
> 
> cpu_exec_init() has lots of side-effects on global QEMU state, move it
> to realize so it will be called only if the X86CPU instance is realized.
> 
> For reference, this is the current cpu_exec_init() code:
> 
> > void cpu_exec_init(CPUArchState *env)
> > {
> >     CPUState *cpu = ENV_GET_CPU(env);
> >     CPUClass *cc = CPU_GET_CLASS(cpu);
> >     CPUState *some_cpu;
> >     int cpu_index;
> >
> > #ifndef CONFIG_USER_ONLY
> >     cpu->as = &address_space_memory;
> >     cpu->thread_id = qemu_get_thread_id();
> > #endif
> 
> Those fields should be used only after actually starting the VCPU and can be
> initialized on realize.
> 
> >
> > #if defined(CONFIG_USER_ONLY)
> >     cpu_list_lock();
> > #endif
> >     cpu_index = 0;
> >     CPU_FOREACH(some_cpu) {
> >         cpu_index++;
> >     }
> >     cpu->cpu_index = cpu_index;
> >     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
> > #if defined(CONFIG_USER_ONLY)
> >     cpu_list_unlock();
> > #endif
> 
> The above initializes cpu_index and add the CPU to the global CPU list.
> This affects QEMU global state and must be done only on realize.
> 
> >     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
> >         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
> >     }
> > #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
> >     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
> >                     cpu_save, cpu_load, env);
> >     assert(cc->vmsd == NULL);
> >     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
> > #endif
> >     if (cc->vmsd != NULL) {
> >         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
> >     }
> 
> vmstate and savevm registration also affects global QEMU state and should be
> done only on realize.
> 
> > }
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 400b1e0..8b76604 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2758,6 +2758,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      static bool ht_warned;
>      static bool tcg_initialized;
>  
> +    cpu_exec_init(env);
> +
>      if (tcg_enabled() && !tcg_initialized) {
>          tcg_initialized = 1;
>          tcg_x86_init();
> @@ -2840,7 +2842,6 @@ static void x86_cpu_initfn(Object *obj)
>      CPUX86State *env = &cpu->env;
>  
>      cs->env_ptr = env;
> -    cpu_exec_init(env);
looks wrong, later in this function we do
 env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
and with this patch will always yield 0

>  
>      object_property_add(obj, "family", "int",
>                          x86_cpuid_version_get_family,

Re: [Qemu-devel] [PATCH 6/6] target-i386: Call cpu_exec_init() on realize

[Andreas Färber]

Am 05.03.2015 um 17:42 schrieb Igor Mammedov:
> On Thu,  5 Mar 2015 12:38:50 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> To allow new code to ask the CPU classes for CPU model information and
>> allow QOM properties to be queried by qmp_device_list_properties(), we
>> need to be able to safely instantiate a X86CPU object without any
>> side-effects.
>>
>> cpu_exec_init() has lots of side-effects on global QEMU state, move it
>> to realize so it will be called only if the X86CPU instance is realized.
>>
>> For reference, this is the current cpu_exec_init() code:
>>
>>> void cpu_exec_init(CPUArchState *env)
>>> {
>>>     CPUState *cpu = ENV_GET_CPU(env);
>>>     CPUClass *cc = CPU_GET_CLASS(cpu);
>>>     CPUState *some_cpu;
>>>     int cpu_index;
>>>
>>> #ifndef CONFIG_USER_ONLY
>>>     cpu->as = &address_space_memory;
>>>     cpu->thread_id = qemu_get_thread_id();
>>> #endif
>>
>> Those fields should be used only after actually starting the VCPU and can be
>> initialized on realize.
>>
>>>
>>> #if defined(CONFIG_USER_ONLY)
>>>     cpu_list_lock();
>>> #endif
>>>     cpu_index = 0;
>>>     CPU_FOREACH(some_cpu) {
>>>         cpu_index++;
>>>     }
>>>     cpu->cpu_index = cpu_index;
>>>     QTAILQ_INSERT_TAIL(&cpus, cpu, node);
>>> #if defined(CONFIG_USER_ONLY)
>>>     cpu_list_unlock();
>>> #endif
>>
>> The above initializes cpu_index and add the CPU to the global CPU list.
>> This affects QEMU global state and must be done only on realize.
>>
>>>     if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
>>>         vmstate_register(NULL, cpu_index, &vmstate_cpu_common, cpu);
>>>     }
>>> #if defined(CPU_SAVE_VERSION) && !defined(CONFIG_USER_ONLY)
>>>     register_savevm(NULL, "cpu", cpu_index, CPU_SAVE_VERSION,
>>>                     cpu_save, cpu_load, env);
>>>     assert(cc->vmsd == NULL);
>>>     assert(qdev_get_vmsd(DEVICE(cpu)) == NULL);
>>> #endif
>>>     if (cc->vmsd != NULL) {
>>>         vmstate_register(NULL, cpu_index, cc->vmsd, cpu);
>>>     }
>>
>> vmstate and savevm registration also affects global QEMU state and should be
>> done only on realize.
>>
>>> }
>>
>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>> ---
>>  target-i386/cpu.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> index 400b1e0..8b76604 100644
>> --- a/target-i386/cpu.c
>> +++ b/target-i386/cpu.c
>> @@ -2758,6 +2758,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>>      static bool ht_warned;
>>      static bool tcg_initialized;
>>  
>> +    cpu_exec_init(env);
>> +
>>      if (tcg_enabled() && !tcg_initialized) {
>>          tcg_initialized = 1;
>>          tcg_x86_init();
>> @@ -2840,7 +2842,6 @@ static void x86_cpu_initfn(Object *obj)
>>      CPUX86State *env = &cpu->env;
>>  
>>      cs->env_ptr = env;
>> -    cpu_exec_init(env);
> looks wrong, later in this function we do
>  env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> and with this patch will always yield 0

Being tackled in Eduardo's APIC series. ;)

Cheers,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)

Re: [Qemu-devel] [PATCH 6/6] target-i386: Call cpu_exec_init() on realize

[Eduardo Habkost]

On Thu, Mar 05, 2015 at 05:44:58PM +0100, Andreas Färber wrote:
> Am 05.03.2015 um 17:42 schrieb Igor Mammedov:
[...]
> >> @@ -2840,7 +2842,6 @@ static void x86_cpu_initfn(Object *obj)
> >>      CPUX86State *env = &cpu->env;
> >>  
> >>      cs->env_ptr = env;
> >> -    cpu_exec_init(env);
> > looks wrong, later in this function we do
> >  env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> > and with this patch will always yield 0
> 
> Being tackled in Eduardo's APIC series. ;)

Which is already queued at the x86 tree mentioned in the cover letter,
BTW:
  https://github.com/ehabkost/qemu.git x86

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/6] target-i386: Remove side-effects from X86CPU::instance_init

[Eduardo Habkost]

On Thu, Mar 05, 2015 at 12:38:44PM -0300, Eduardo Habkost wrote:
> Eduardo Habkost (6):
>   cpu: No need to zero-initialize numa_node
>   cpu: Initialize breakpoint/watchpoint lists on cpu_common_initfn()
>   cpu: Reorder cpu->as and cpu->thread_id initialization

Andreas, do you want to queue the patches above through your qom-cpu
tree? They are not a hard requirement for the patches below.

(The only difference is that the commit message in patch 6/6 refer to
the modified version of cpu_exec_init())

>   target-i386: Rename optimize_flags_init()
>   target-i386: Move TCG initialization to realize time
>   target-i386: Call cpu_exec_init() on realize
> 
>  exec.c                  | 12 +++++-------
>  qom/cpu.c               |  2 ++
>  target-i386/cpu.c       | 16 ++++++++--------
>  target-i386/cpu.h       |  2 +-
>  target-i386/translate.c |  2 +-
>  5 files changed, 17 insertions(+), 17 deletions(-)
> 
> -- 
> 2.1.0
> 
> 

-- 
Eduardo