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

[Qemu-devel] [PATCH v2 0/4] 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.

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

Changes v1 -> v2:
* New patch: Move TCG initialization check to tcg_x86_init()
* Call cpu_exec_init() after basic parameter validation on realizefn
* Move tcg_x86_init() call after basic parameter validation inside
  realizefn

Eduardo Habkost (4):
  target-i386: Rename optimize_flags_init()
  target-i386: Move TCG initialization check to tcg_x86_init()
  target-i386: Move TCG initialization to realize time
  target-i386: Call cpu_exec_init() on realize

 target-i386/cpu.c       | 14 ++++++--------
 target-i386/cpu.h       |  2 +-
 target-i386/translate.c |  8 +++++++-
 3 files changed, 14 insertions(+), 10 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH v2 1/4] 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 21f7b99..22c0f5b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3074,7 +3074,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 737a68e..4e48cf2 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1236,7 +1236,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 8b35de1..e6e60e5 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7789,7 +7789,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

[Qemu-devel] [PATCH v2 2/4] target-i386: Move TCG initialization check to tcg_x86_init()

[Eduardo Habkost]

Instead of requiring cpu.c to check if TCG was already initialized,
simply let the function be called multiple times.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c       | 4 +---
 target-i386/translate.c | 6 ++++++
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 22c0f5b..f054a69 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -3021,7 +3021,6 @@ static void x86_cpu_initfn(Object *obj)
     X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
     CPUX86State *env = &cpu->env;
     FeatureWord w;
-    static int inited;
 
     cs->env_ptr = env;
     cpu_exec_init(cs, &error_abort);
@@ -3072,8 +3071,7 @@ static void x86_cpu_initfn(Object *obj)
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
 
     /* init various static tables used in TCG mode */
-    if (tcg_enabled() && !inited) {
-        inited = 1;
+    if (tcg_enabled()) {
         tcg_x86_init();
     }
 }
diff --git a/target-i386/translate.c b/target-i386/translate.c
index e6e60e5..8ed3902 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7821,6 +7821,12 @@ void tcg_x86_init(void)
 #endif
     };
     int i;
+    static bool initialized = false;
+
+    if (initialized) {
+        return;
+    }
+    initialized = true;
 
     cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
     cpu_cc_op = tcg_global_mem_new_i32(TCG_AREG0,
-- 
2.1.0

[Qemu-devel] [PATCH v2 3/4] target-i386: Move TCG initialization to realize time

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

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>
---
Changes v1 -> v2:
 * Now the inited/tcg_initialized variable doesn't exist anymore
 * Move tcg_x86_init() call after basic parameter validation inside
   realizefn
---
 target-i386/cpu.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f054a69..2e5a303 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2832,6 +2832,10 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
+    if (tcg_enabled()) {
+        tcg_x86_init();
+    }
+
 #ifndef CONFIG_USER_ONLY
     qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
 
@@ -3069,11 +3073,6 @@ static void x86_cpu_initfn(Object *obj)
     }
 
     x86_cpu_load_def(cpu, xcc->cpu_def, &error_abort);
-
-    /* init various static tables used in TCG mode */
-    if (tcg_enabled()) {
-        tcg_x86_init();
-    }
 }
 
 static int64_t x86_cpu_get_arch_id(CPUState *cs)
-- 
2.1.0

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

[Eduardo Habkost]

QOM instance_init functions are not supposed to have any side-effects,
as new objects may be created at any moment for querying property
information (see qmp_device_list_properties()).

Calling cpu_exec_init() also affects QEMU's ability to handle errors
during CPU creation, as some actions done by cpu_exec_init() can't be
reverted.

Move cpu_exec_init() call to realize so a simple object_new() won't
trigger it, and so that it is called after some basic validation of CPU
parameters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
 * Call cpu_exec_init() after basic parameter validation
---
 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 2e5a303..19d1525 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2832,6 +2832,8 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         goto out;
     }
 
+    cpu_exec_init(cs, &error_abort);
+
     if (tcg_enabled()) {
         tcg_x86_init();
     }
@@ -3027,7 +3029,6 @@ static void x86_cpu_initfn(Object *obj)
     FeatureWord w;
 
     cs->env_ptr = env;
-    cpu_exec_init(cs, &error_abort);
 
     object_property_add(obj, "family", "int",
                         x86_cpuid_version_get_family,
-- 
2.1.0

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

[Bharata B Rao]

On Sat, Sep 19, 2015 at 1:08 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> QOM instance_init functions are not supposed to have any side-effects,
> as new objects may be created at any moment for querying property
> information (see qmp_device_list_properties()).
>
> Calling cpu_exec_init() also affects QEMU's ability to handle errors
> during CPU creation, as some actions done by cpu_exec_init() can't be
> reverted.
>
> Move cpu_exec_init() call to realize so a simple object_new() won't
> trigger it, and so that it is called after some basic validation of CPU
> parameters.

Since you are moving cpu_exec_init() to realize, does it make sense to
define unrealize and call cpu_exec_exit() from it ?

Regards,
Bharata.

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

[Eduardo Habkost]

On Mon, Sep 21, 2015 at 11:11:47AM +0530, Bharata B Rao wrote:
> On Sat, Sep 19, 2015 at 1:08 AM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > QOM instance_init functions are not supposed to have any side-effects,
> > as new objects may be created at any moment for querying property
> > information (see qmp_device_list_properties()).
> >
> > Calling cpu_exec_init() also affects QEMU's ability to handle errors
> > during CPU creation, as some actions done by cpu_exec_init() can't be
> > reverted.
> >
> > Move cpu_exec_init() call to realize so a simple object_new() won't
> > trigger it, and so that it is called after some basic validation of CPU
> > parameters.
> 
> Since you are moving cpu_exec_init() to realize, does it make sense to
> define unrealize and call cpu_exec_exit() from it ?

It does make sense. But it needs to be done more carefully because
currently cpu_exec_exit() is likely to make QEMU crash, and calling it
from unrealize would make the crash triggerable using a QMP qom-set
command.

-- 
Eduardo