[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

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

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

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

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

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    | 23 ++++++++++++++++++-----
 target-i386/cpu.h    |  2 +-
 target-i386/helper.c | 26 ++++++++++++++------------
 3 files changed, 33 insertions(+), 18 deletions(-)

-- 
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 044e2d9..3276653 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1491,7 +1491,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;
@@ -1508,16 +1508,20 @@ 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;
     }
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
+        error_setg(errp, "Error parsing feature string: %s",
+                   features ? features : "(none)");
         goto error;
     }
     if (def->vendor1) {
@@ -1574,7 +1578,7 @@ X86CPU *cpu_x86_create(const char *cpu_model)
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &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