[Qemu-devel] [PATCH 0/2] target-i386: move CPU object creation to cpu.c (v2)

[Qemu-devel] [PATCH 0/2] target-i386: move CPU object creation to cpu.c (v2)

[Eduardo Habkost]

This small series moves the X86CPU object creation to the cpu.c code, as the
object creation will depend on the CPU class lookup, that will be done inside
cpu.c.

I expect this to make the x86 CPU subclass work simpler. Maybe it will help
simplify some of the CPU properties work, too.

This version is simply a rebase of v1 on top of Igor's series:
  Subject: [Qemu-devel] [PATCH 0/6] x86 CPU cleanup (wave 2)

Git branch for testing:
  git://github.com/ehabkost/qemu-hacks.git cpu-x86-create.v2
  https://github.com/ehabkost/qemu-hacks/tree/cpu-x86-create.v2

As Igor have suggested instead moving cpu_x86_init() to cpu.c, I will try to
explain the rationale, here:

The CPU creation consists of multiple steps:

1) Splitting cpu_model string into CPU model name and feature string;
2) Choosing the CPU model class;
3) Creating CPU object;
4) Parsing feature string;
5) cpu_x86_realize()

And:
 * cpu_x86_register() already does steps 1 and 4.
 * cpu_x86_init() does steps 2, 3, and 5
 * We need to separate step 5 from the rest, as step 5 will eventually
   become the realize() method of the CPU object.

To accomplish that, the first patch in this series simply moves 3 lines of code
(steps 2 and 3) from cpu_x86_init() to cpu_x86_register(), and renames
cpu_x86_register() to cpu_x86_create(), so now:

 * cpu_x86_create() does steps 1 to 4
 * cpu_x86_init() simply become a trivial wrapper to call cpu_x86_create()
   (steps 1-4) and cpu_x86_realize() (step 5)


Eduardo Habkost (2):
  target-i386: move CPU object creation to cpu.c
  target-i386: make cpu_x86_create() get Error argument

 target-i386/cpu.c    | 22 ++++++++++++++++++----
 target-i386/cpu.h    |  2 +-
 target-i386/helper.c | 26 ++++++++++++++------------
 3 files changed, 33 insertions(+), 17 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

As we will need to create the CPU object after splitting the CPU model
string (because we're going to use different subclasses for each CPU
model), move the CPU object creation to cpu_x86_register(), and at the
same time rename cpu_x86_register() to cpu_x86_create().

This will also simplify the CPU creation code to a trivial
cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
code that have to set additional properties before cpu_x86_realize() is
called (e.g. the PC CPU initialization code, that needs to set APIC IDs
depending on the CPU cores/threads topology).

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..b242bf1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1539,13 +1539,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+/* Create and initialize a X86CPU object, based on the full CPU model string
+ * (that may include "+feature,-feature,feature=xxx" feature strings)
+ */
+X86CPU *cpu_x86_create(const char *cpu_model)
 {
+    X86CPU *cpu;
+    CPUX86State *env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
     char *name, *features;
     gchar **model_pieces;
 
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
+
     memset(def, 0, sizeof(*def));
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
@@ -1578,10 +1587,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
 
     g_strfreev(model_pieces);
-    return 0;
+    return cpu;
 error:
+    object_delete(OBJECT(cpu));
     g_strfreev(model_pieces);
-    return -1;
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..3ebaae9 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..23af4a8 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
-    CPUX86State *env;
     Error *error = NULL;
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
+    cpu = cpu_x86_create(cpu_model);
+    if (!cpu) {
         return NULL;
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH 2/2] target-i386: make cpu_x86_create() get Error argument

[Eduardo Habkost]

Instead of forcing the caller to guess what went wrong while creating
the CPU object, return error information in a Error argument.

Also, as cpu_x86_create() won't print error messages itself anymore,
change cpu_x86_init() to print any error returned by cpu_x86_create()
or cpu_x86_realize().

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index b242bf1..fba872d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1542,7 +1542,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
 /* Create and initialize a X86CPU object, based on the full CPU model string
  * (that may include "+feature,-feature,feature=xxx" feature strings)
  */
-X86CPU *cpu_x86_create(const char *cpu_model)
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu;
     CPUX86State *env;
@@ -1559,12 +1559,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
+        error_setg(errp, "invalid CPU model string: %s", cpu_model);
         goto error;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
+        error_setg(errp, "CPU model not found: %s", name);
         goto error;
     }
 
@@ -1575,13 +1577,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
+        error_setg(errp, "Error parsing feature string: %s",
+                   features ? features : "(none)");
         goto error;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
     if (error) {
-        fprintf(stderr, "%s\n", error_get_pretty(error));
+        error_propagate(errp, error);
         error_free(error);
         goto error;
     }
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 3ebaae9..8ce9b9f 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
-X86CPU *cpu_x86_create(const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index 23af4a8..dafa666 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -23,6 +23,7 @@
 #include "sysemu.h"
 #include "monitor.h"
 #endif
+#include "qemu-error.h"
 
 //#define DEBUG_MMU
 
@@ -1242,21 +1243,27 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-    X86CPU *cpu;
+    X86CPU *cpu = NULL;
     Error *error = NULL;
 
-    cpu = cpu_x86_create(cpu_model);
-    if (!cpu) {
-        return NULL;
+    cpu = cpu_x86_create(cpu_model, &error);
+    if (error) {
+        goto error;
     }
 
     x86_cpu_realize(OBJECT(cpu), &error);
     if (error) {
-        error_free(error);
-        object_delete(OBJECT(cpu));
-        return NULL;
+        goto error;
     }
     return cpu;
+
+error:
+    if (cpu) {
+        object_delete(OBJECT(cpu));
+    }
+    error_report("%s", error_get_pretty(error));
+    error_free(error);
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/2] target-i386: make cpu_x86_create() get Error argument

[Igor Mammedov]

On Wed, 12 Dec 2012 16:16:23 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of forcing the caller to guess what went wrong while creating
> the CPU object, return error information in a Error argument.
> 
> Also, as cpu_x86_create() won't print error messages itself anymore,
> change cpu_x86_init() to print any error returned by cpu_x86_create()
> or cpu_x86_realize().
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c    |  8 ++++++--
>  target-i386/cpu.h    |  2 +-
>  target-i386/helper.c | 21 ++++++++++++++-------
>  3 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index b242bf1..fba872d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1542,7 +1542,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  /* Create and initialize a X86CPU object, based on the full CPU model string
>   * (that may include "+feature,-feature,feature=xxx" feature strings)
>   */
> -X86CPU *cpu_x86_create(const char *cpu_model)
> +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu;
>      CPUX86State *env;
> @@ -1559,12 +1559,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
>      if (!model_pieces[0]) {
> +        error_setg(errp, "invalid CPU model string: %s", cpu_model);
>          goto error;
>      }
>      name = model_pieces[0];
>      features = model_pieces[1];
>  
>      if (cpu_x86_find_by_name(def, name) < 0) {
> +        error_setg(errp, "CPU model not found: %s", name);
>          goto error;
>      }
>  
> @@ -1575,13 +1577,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
>                              &def->svm_features, &def->cpuid_7_0_ebx_features);
>  
>      if (cpu_x86_parse_featurestr(def, features) < 0) {
> +        error_setg(errp, "Error parsing feature string: %s",
> +                   features ? features : "(none)");
It could be simplified, it shouldn't get here if features == NULL

>          goto error;
>      }
>  
>      cpudef_2_x86_cpu(cpu, def, &error);
>  
>      if (error) {
> -        fprintf(stderr, "%s\n", error_get_pretty(error));
> +        error_propagate(errp, error);
Why do it here but not above?

>          error_free(error);
>          goto error;
>      }
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
[...]
>  
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
> -    X86CPU *cpu;
> +    X86CPU *cpu = NULL;
>      Error *error = NULL;
>  
> -    cpu = cpu_x86_create(cpu_model);
> -    if (!cpu) {
> -        return NULL;
> +    cpu = cpu_x86_create(cpu_model, &error);
> +    if (error) {
> +        goto error;
>      }
>  
>      x86_cpu_realize(OBJECT(cpu), &error);
if x86_cpu_realize() behave as visit* functions, i.e. return early if
error has been already set, error check & goto could be removed here
and above and consolidated at function exit.

>      if (error) {
> -        error_free(error);
> -        object_delete(OBJECT(cpu));
> -        return NULL;
> +        goto error;
>      }
>      return cpu;
> +
> +error:
> +    if (cpu) {
> +        object_delete(OBJECT(cpu));
> +    }
> +    error_report("%s", error_get_pretty(error));
> +    error_free(error);
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.11.7
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Igor Mammedov]

On Wed, 12 Dec 2012 16:16:22 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> As we will need to create the CPU object after splitting the CPU model
> string (because we're going to use different subclasses for each CPU
> model), move the CPU object creation to cpu_x86_register(), and at the
> same time rename cpu_x86_register() to cpu_x86_create().
> 
> This will also simplify the CPU creation code to a trivial
> cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
> code that have to set additional properties before cpu_x86_realize() is
> called (e.g. the PC CPU initialization code, that needs to set APIC IDs
> depending on the CPU cores/threads topology).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c    | 16 +++++++++++++---
>  target-i386/cpu.h    |  2 +-
>  target-i386/helper.c |  9 ++-------
>  3 files changed, 16 insertions(+), 11 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3b9bbfe..b242bf1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1539,13 +1539,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>  
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> +/* Create and initialize a X86CPU object, based on the full CPU model string
> + * (that may include "+feature,-feature,feature=xxx" feature strings)
feature format of cpu_model string misses just 'feature'

> + */
> +X86CPU *cpu_x86_create(const char *cpu_model)
>  {
> +    X86CPU *cpu;
> +    CPUX86State *env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
>  
> +    cpu = X86_CPU(object_new(TYPE_X86_CPU));
Could we put this after cpu_x86_parse_featurestr(), it's really not needed
before it now and eventually we would like to move it there anyway.

> +    env = &cpu->env;
> +    env->cpu_model_str = cpu_model;
> +
>      memset(def, 0, sizeof(*def));
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
> @@ -1578,10 +1587,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>  
>      g_strfreev(model_pieces);
> -    return 0;
> +    return cpu;
>  error:
> +    object_delete(OBJECT(cpu));
>      g_strfreev(model_pieces);
> -    return -1;
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 386c4f6..3ebaae9 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                     uint32_t *eax, uint32_t *ebx,
>                     uint32_t *ecx, uint32_t *edx);
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
> +X86CPU *cpu_x86_create(const char *cpu_model);
>  void cpu_clear_apic_feature(CPUX86State *env);
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..23af4a8 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
> -    CPUX86State *env;
>      Error *error = NULL;
>  
> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -    env = &cpu->env;
> -    env->cpu_model_str = cpu_model;
> -
> -    if (cpu_x86_register(cpu, cpu_model) < 0) {
> -        object_delete(OBJECT(cpu));
> +    cpu = cpu_x86_create(cpu_model);
> +    if (!cpu) {
>          return NULL;
>      }
>  
> -- 
> 1.7.11.7
> 


-- 
Regards,
  Igor

Re: [Qemu-devel] [PATCH 2/2] target-i386: make cpu_x86_create() get Error argument

[Eduardo Habkost]

On Fri, Dec 14, 2012 at 11:18:18AM +0100, Igor Mammedov wrote:
> On Wed, 12 Dec 2012 16:16:23 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Instead of forcing the caller to guess what went wrong while creating
> > the CPU object, return error information in a Error argument.
> > 
> > Also, as cpu_x86_create() won't print error messages itself anymore,
> > change cpu_x86_init() to print any error returned by cpu_x86_create()
> > or cpu_x86_realize().
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c    |  8 ++++++--
> >  target-i386/cpu.h    |  2 +-
> >  target-i386/helper.c | 21 ++++++++++++++-------
> >  3 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index b242bf1..fba872d 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1542,7 +1542,7 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  /* Create and initialize a X86CPU object, based on the full CPU model string
> >   * (that may include "+feature,-feature,feature=xxx" feature strings)
> >   */
> > -X86CPU *cpu_x86_create(const char *cpu_model)
> > +X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
> >  {
> >      X86CPU *cpu;
> >      CPUX86State *env;
> > @@ -1559,12 +1559,14 @@ X86CPU *cpu_x86_create(const char *cpu_model)
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> >      if (!model_pieces[0]) {
> > +        error_setg(errp, "invalid CPU model string: %s", cpu_model);
> >          goto error;
> >      }
> >      name = model_pieces[0];
> >      features = model_pieces[1];
> >  
> >      if (cpu_x86_find_by_name(def, name) < 0) {
> > +        error_setg(errp, "CPU model not found: %s", name);
> >          goto error;
> >      }
> >  
> > @@ -1575,13 +1577,15 @@ X86CPU *cpu_x86_create(const char *cpu_model)
> >                              &def->svm_features, &def->cpuid_7_0_ebx_features);
> >  
> >      if (cpu_x86_parse_featurestr(def, features) < 0) {
> > +        error_setg(errp, "Error parsing feature string: %s",
> > +                   features ? features : "(none)");
> It could be simplified, it shouldn't get here if features == NULL

It could be something like:

  if (features && cpu_x86_parse_featurestr(def, features) < 0) {
      ...
  }

Both options are reasonable to me.


> 
> >          goto error;
> >      }
> >  
> >      cpudef_2_x86_cpu(cpu, def, &error);
> >  
> >      if (error) {
> > -        fprintf(stderr, "%s\n", error_get_pretty(error));
> > +        error_propagate(errp, error);
> Why do it here but not above?

Because the other functions called above don't get an Error object
(yet).  :-)

> 
> >          error_free(error);
> >          goto error;
> >      }
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> [...]
> >  
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> > -    X86CPU *cpu;
> > +    X86CPU *cpu = NULL;
> >      Error *error = NULL;
> >  
> > -    cpu = cpu_x86_create(cpu_model);
> > -    if (!cpu) {
> > -        return NULL;
> > +    cpu = cpu_x86_create(cpu_model, &error);
> > +    if (error) {
> > +        goto error;
> >      }
> >  
> >      x86_cpu_realize(OBJECT(cpu), &error);
> if x86_cpu_realize() behave as visit* functions, i.e. return early if
> error has been already set, error check & goto could be removed here
> and above and consolidated at function exit.

I'm not sure I want to use that coding style. Expecting every function
to abort in the beginning if Error is set sounds fragile, to me. I would
even expect the maintainers to complain if I wrote the code that way (as
I never saw that style being used in any code except the visitors).

The visitors seem to be different because they are called from
automatically-generated QAPI code, that can't know if errors in a give
"visit" should abort the rest of the process, or not.


> 
> >      if (error) {
> > -        error_free(error);
> > -        object_delete(OBJECT(cpu));
> > -        return NULL;
> > +        goto error;
> >      }
> >      return cpu;
> > +
> > +error:
> > +    if (cpu) {
> > +        object_delete(OBJECT(cpu));
> > +    }
> > +    error_report("%s", error_get_pretty(error));
> > +    error_free(error);
> > +    return NULL;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -- 
> > 1.7.11.7
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

On Fri, Dec 14, 2012 at 11:26:55AM +0100, Igor Mammedov wrote:
> On Wed, 12 Dec 2012 16:16:22 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > As we will need to create the CPU object after splitting the CPU model
> > string (because we're going to use different subclasses for each CPU
> > model), move the CPU object creation to cpu_x86_register(), and at the
> > same time rename cpu_x86_register() to cpu_x86_create().
> > 
> > This will also simplify the CPU creation code to a trivial
> > cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
> > code that have to set additional properties before cpu_x86_realize() is
> > called (e.g. the PC CPU initialization code, that needs to set APIC IDs
> > depending on the CPU cores/threads topology).
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c    | 16 +++++++++++++---
> >  target-i386/cpu.h    |  2 +-
> >  target-i386/helper.c |  9 ++-------
> >  3 files changed, 16 insertions(+), 11 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3b9bbfe..b242bf1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1539,13 +1539,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  }
> >  #endif
> >  
> > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> > +/* Create and initialize a X86CPU object, based on the full CPU model string
> > + * (that may include "+feature,-feature,feature=xxx" feature strings)
> feature format of cpu_model string misses just 'feature'

Thanks. I'll change it.

> 
> > + */
> > +X86CPU *cpu_x86_create(const char *cpu_model)
> >  {
> > +    X86CPU *cpu;
> > +    CPUX86State *env;
> >      x86_def_t def1, *def = &def1;
> >      Error *error = NULL;
> >      char *name, *features;
> >      gchar **model_pieces;
> >  
> > +    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> Could we put this after cpu_x86_parse_featurestr(), it's really not needed
> before it now and eventually we would like to move it there anyway.

I believe we want to call cpu_x86_parse_featurestr() _after_ the CPU
object is created, don't we? Because one day the feature string is going
be used to set properties in the CPU object.

But we can move object_new() after g_strsplit() (because evantually the
CPU object creation is going to use the first part of cpu_model to
lookup the class name).

> 
> > +    env = &cpu->env;
> > +    env->cpu_model_str = cpu_model;
> > +
> >      memset(def, 0, sizeof(*def));
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> > @@ -1578,10 +1587,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >      }
> >  
> >      g_strfreev(model_pieces);
> > -    return 0;
> > +    return cpu;
> >  error:
> > +    object_delete(OBJECT(cpu));
> >      g_strfreev(model_pieces);
> > -    return -1;
> > +    return NULL;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 386c4f6..3ebaae9 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
> >  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >                     uint32_t *eax, uint32_t *ebx,
> >                     uint32_t *ecx, uint32_t *edx);
> > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
> > +X86CPU *cpu_x86_create(const char *cpu_model);
> >  void cpu_clear_apic_feature(CPUX86State *env);
> >  void host_cpuid(uint32_t function, uint32_t count,
> >                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index bf206cf..23af4a8 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
> >  X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> >      X86CPU *cpu;
> > -    CPUX86State *env;
> >      Error *error = NULL;
> >  
> > -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > -    env = &cpu->env;
> > -    env->cpu_model_str = cpu_model;
> > -
> > -    if (cpu_x86_register(cpu, cpu_model) < 0) {
> > -        object_delete(OBJECT(cpu));
> > +    cpu = cpu_x86_create(cpu_model);
> > +    if (!cpu) {
> >          return NULL;
> >      }
> >  
> > -- 
> > 1.7.11.7
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

On Tue, Dec 11, 2012 at 02:33:24PM +0100, Igor Mammedov wrote:
> On Mon, 10 Dec 2012 22:30:42 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > As we will need to create the CPU object after splitting the CPU model
> > string (because we're going to use different subclasses for each CPU
> > model), move the CPU object creation to cpu_x86_register(), and at the
> > same time rename cpu_x86_register() to cpu_x86_create().
> perhaps it would be better to move cpu_x86_init() inside cpu.c and embed
> cpu_x86_register() in it, as you have done in one of yours series, and avoid
> creating intermediate cpu_x86_create() when the end result of cpu subclasses
> & properties should become a simple cpu_x86_init():
>  1. lookup class name, 2. get cpu instance , 3. set properties, 4. realize() 

I don't think cpu_x86_create() is an intermediate code, it is becoming
what will be the final CPU creation code.

cpu_x86_create() is now doing all steps you suggest above above, except
for step 4 (realize). But leaving the realize() call outside of cpu.c is
actually a feature, because some code will need to do additional
initialization steps between steps 3 and 4 (so that code will become a
simple create()+[additional steps]+realize() function).

I believe that moving the object creation to cpu.c only (instead of the
whole cpu_x86_init() function) ended up being much simpler than the
original approach I used (that was moving the entire cpu_x86_init()
function to cpu.c).

cpu_x86_init() is now just a create()+realize() helper, and nothing
else, and I believe we won't need to touch it anymore except at some
point in the future when (hopefully) we will kill it and replace with a
generic target-independent function.


> 
> > 
> > This will also simplify the CPU creation code to a trivial
> > cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
> > code that have to set additional properties before cpu_x86_realize() is
> > called (e.g. the PC CPU initialization code, that needs to set APIC IDs
> > depending on the CPU cores/threads topology).
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c    | 17 +++++++++++++----
> >  target-i386/cpu.h    |  2 +-
> >  target-i386/helper.c |  9 ++-------
> >  3 files changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 7be3ad8..044e2d9 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
> >  }
> >  #endif
> >  
> > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> > +/* Create and initialize a X86CPU object, based on the full CPU model
> > string
> > + * (that may include "+feature,-feature,feature=xxx" feature strings)
> > + */
> > +X86CPU *cpu_x86_create(const char *cpu_model)
> >  {
> > -    CPUX86State *env = &cpu->env;
> > +    X86CPU *cpu;
> > +    CPUX86State *env;
> >      x86_def_t def1, *def = &def1;
> >      Error *error = NULL;
> >      char *name, *features;
> >      gchar **model_pieces;
> >  
> > +    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > +    env = &cpu->env;
> > +    env->cpu_model_str = cpu_model;
> > +
> >      memset(def, 0, sizeof(*def));
> >  
> >      model_pieces = g_strsplit(cpu_model, ",", 2);
> > @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) }
> >  
> >      g_strfreev(model_pieces);
> > -    return 0;
> > +    return cpu;
> >  error:
> > +    object_delete(OBJECT(cpu));
> >      g_strfreev(model_pieces);
> > -    return -1;
> > +    return NULL;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> > index 386c4f6..3ebaae9 100644
> > --- a/target-i386/cpu.h
> > +++ b/target-i386/cpu.h
> > @@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
> >  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >                     uint32_t *eax, uint32_t *ebx,
> >                     uint32_t *ecx, uint32_t *edx);
> > -int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
> > +X86CPU *cpu_x86_create(const char *cpu_model);
> >  void cpu_clear_apic_feature(CPUX86State *env);
> >  void host_cpuid(uint32_t function, uint32_t count,
> >                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t
> > *edx); diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index bf206cf..23af4a8 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> > unsigned int selector, X86CPU *cpu_x86_init(const char *cpu_model)
> >  {
> >      X86CPU *cpu;
> > -    CPUX86State *env;
> >      Error *error = NULL;
> >  
> > -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > -    env = &cpu->env;
> > -    env->cpu_model_str = cpu_model;
> > -
> > -    if (cpu_x86_register(cpu, cpu_model) < 0) {
> > -        object_delete(OBJECT(cpu));
> > +    cpu = cpu_x86_create(cpu_model);
> > +    if (!cpu) {
> >          return NULL;
> >      }
> >  
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Igor Mammedov]

On Mon, 10 Dec 2012 22:30:42 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> As we will need to create the CPU object after splitting the CPU model
> string (because we're going to use different subclasses for each CPU
> model), move the CPU object creation to cpu_x86_register(), and at the
> same time rename cpu_x86_register() to cpu_x86_create().
perhaps it would be better to move cpu_x86_init() inside cpu.c and embed
cpu_x86_register() in it, as you have done in one of yours series, and avoid
creating intermediate cpu_x86_create() when the end result of cpu subclasses
& properties should become a simple cpu_x86_init():
 1. lookup class name, 2. get cpu instance , 3. set properties, 4. realize() 

> 
> This will also simplify the CPU creation code to a trivial
> cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
> code that have to set additional properties before cpu_x86_realize() is
> called (e.g. the PC CPU initialization code, that needs to set APIC IDs
> depending on the CPU cores/threads topology).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c    | 17 +++++++++++++----
>  target-i386/cpu.h    |  2 +-
>  target-i386/helper.c |  9 ++-------
>  3 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7be3ad8..044e2d9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>  
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> +/* Create and initialize a X86CPU object, based on the full CPU model
> string
> + * (that may include "+feature,-feature,feature=xxx" feature strings)
> + */
> +X86CPU *cpu_x86_create(const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
> +    X86CPU *cpu;
> +    CPUX86State *env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
>  
> +    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> +    env = &cpu->env;
> +    env->cpu_model_str = cpu_model;
> +
>      memset(def, 0, sizeof(*def));
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
> @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char
> *cpu_model) }
>  
>      g_strfreev(model_pieces);
> -    return 0;
> +    return cpu;
>  error:
> +    object_delete(OBJECT(cpu));
>      g_strfreev(model_pieces);
> -    return -1;
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 386c4f6..3ebaae9 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                     uint32_t *eax, uint32_t *ebx,
>                     uint32_t *ecx, uint32_t *edx);
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
> +X86CPU *cpu_x86_create(const char *cpu_model);
>  void cpu_clear_apic_feature(CPUX86State *env);
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t
> *edx); diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..23af4a8 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env,
> unsigned int selector, X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
> -    CPUX86State *env;
>      Error *error = NULL;
>  
> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -    env = &cpu->env;
> -    env->cpu_model_str = cpu_model;
> -
> -    if (cpu_x86_register(cpu, cpu_model) < 0) {
> -        object_delete(OBJECT(cpu));
> +    cpu = cpu_x86_create(cpu_model);
> +    if (!cpu) {
>          return NULL;
>      }
>  

Re: [Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

On Mon, Dec 10, 2012 at 10:30:42PM -0200, Eduardo Habkost wrote:
> As we will need to create the CPU object after splitting the CPU model
> string (because we're going to use different subclasses for each CPU
> model), move the CPU object creation to cpu_x86_register(), and at the
> same time rename cpu_x86_register() to cpu_x86_create().
> 
> This will also simplify the CPU creation code to a trivial
> cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
> code that have to set additional properties before cpu_x86_realize() is
> called (e.g. the PC CPU initialization code, that needs to set APIC IDs
> depending on the CPU cores/threads topology).
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Igor just sent "[PATCH 0/6] x86 CPU cleanup (wave 2)", that conflicts
a little with this series.

As the series he sent is larger and some of the patches got some reviews
and comments previously, I will rebase this on top of his series and
resubmit.


> ---
>  target-i386/cpu.c    | 17 +++++++++++++----
>  target-i386/cpu.h    |  2 +-
>  target-i386/helper.c |  9 ++-------
>  3 files changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 7be3ad8..044e2d9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
>  }
>  #endif
>  
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> +/* Create and initialize a X86CPU object, based on the full CPU model string
> + * (that may include "+feature,-feature,feature=xxx" feature strings)
> + */
> +X86CPU *cpu_x86_create(const char *cpu_model)
>  {
> -    CPUX86State *env = &cpu->env;
> +    X86CPU *cpu;
> +    CPUX86State *env;
>      x86_def_t def1, *def = &def1;
>      Error *error = NULL;
>      char *name, *features;
>      gchar **model_pieces;
>  
> +    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> +    env = &cpu->env;
> +    env->cpu_model_str = cpu_model;
> +
>      memset(def, 0, sizeof(*def));
>  
>      model_pieces = g_strsplit(cpu_model, ",", 2);
> @@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>      }
>  
>      g_strfreev(model_pieces);
> -    return 0;
> +    return cpu;
>  error:
> +    object_delete(OBJECT(cpu));
>      g_strfreev(model_pieces);
> -    return -1;
> +    return NULL;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target-i386/cpu.h b/target-i386/cpu.h
> index 386c4f6..3ebaae9 100644
> --- a/target-i386/cpu.h
> +++ b/target-i386/cpu.h
> @@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
>  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>                     uint32_t *eax, uint32_t *ebx,
>                     uint32_t *ecx, uint32_t *edx);
> -int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
> +X86CPU *cpu_x86_create(const char *cpu_model);
>  void cpu_clear_apic_feature(CPUX86State *env);
>  void host_cpuid(uint32_t function, uint32_t count,
>                  uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
> diff --git a/target-i386/helper.c b/target-i386/helper.c
> index bf206cf..23af4a8 100644
> --- a/target-i386/helper.c
> +++ b/target-i386/helper.c
> @@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
>      X86CPU *cpu;
> -    CPUX86State *env;
>      Error *error = NULL;
>  
> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -    env = &cpu->env;
> -    env->cpu_model_str = cpu_model;
> -
> -    if (cpu_x86_register(cpu, cpu_model) < 0) {
> -        object_delete(OBJECT(cpu));
> +    cpu = cpu_x86_create(cpu_model);
> +    if (!cpu) {
>          return NULL;
>      }
>  
> -- 
> 1.7.11.7
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH 1/2] target-i386: move CPU object creation to cpu.c

[Eduardo Habkost]

As we will need to create the CPU object after splitting the CPU model
string (because we're going to use different subclasses for each CPU
model), move the CPU object creation to cpu_x86_register(), and at the
same time rename cpu_x86_register() to cpu_x86_create().

This will also simplify the CPU creation code to a trivial
cpu_x86_create()+cpu_x86_realize() sequence. This will be useful for
code that have to set additional properties before cpu_x86_realize() is
called (e.g. the PC CPU initialization code, that needs to set APIC IDs
depending on the CPU cores/threads topology).

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7be3ad8..044e2d9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1488,14 +1488,22 @@ static void filter_features_for_kvm(X86CPU *cpu)
 }
 #endif
 
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
+/* Create and initialize a X86CPU object, based on the full CPU model string
+ * (that may include "+feature,-feature,feature=xxx" feature strings)
+ */
+X86CPU *cpu_x86_create(const char *cpu_model)
 {
-    CPUX86State *env = &cpu->env;
+    X86CPU *cpu;
+    CPUX86State *env;
     x86_def_t def1, *def = &def1;
     Error *error = NULL;
     char *name, *features;
     gchar **model_pieces;
 
+    cpu = X86_CPU(object_new(TYPE_X86_CPU));
+    env = &cpu->env;
+    env->cpu_model_str = cpu_model;
+
     memset(def, 0, sizeof(*def));
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
@@ -1572,10 +1580,11 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
     }
 
     g_strfreev(model_pieces);
-    return 0;
+    return cpu;
 error:
+    object_delete(OBJECT(cpu));
     g_strfreev(model_pieces);
-    return -1;
+    return NULL;
 }
 
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index 386c4f6..3ebaae9 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -980,7 +980,7 @@ int cpu_x86_signal_handler(int host_signum, void *pinfo,
 void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
                    uint32_t *eax, uint32_t *ebx,
                    uint32_t *ecx, uint32_t *edx);
-int cpu_x86_register(X86CPU *cpu, const char *cpu_model);
+X86CPU *cpu_x86_create(const char *cpu_model);
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
diff --git a/target-i386/helper.c b/target-i386/helper.c
index bf206cf..23af4a8 100644
--- a/target-i386/helper.c
+++ b/target-i386/helper.c
@@ -1243,15 +1243,10 @@ int cpu_x86_get_descr_debug(CPUX86State *env, unsigned int selector,
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     X86CPU *cpu;
-    CPUX86State *env;
     Error *error = NULL;
 
-    cpu = X86_CPU(object_new(TYPE_X86_CPU));
-    env = &cpu->env;
-    env->cpu_model_str = cpu_model;
-
-    if (cpu_x86_register(cpu, cpu_model) < 0) {
-        object_delete(OBJECT(cpu));
+    cpu = cpu_x86_create(cpu_model);
+    if (!cpu) {
         return NULL;
     }
 
-- 
1.7.11.7