[Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Don Slutz <Don@CloudSwitch.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
v2:
    - style change, add braces (reqested by Blue Swirl)
    - removed unused error_is_set(errp) in properties set loop
---
 target-i386/cpu.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c4e6163..e1e2346 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1264,6 +1266,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1355,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1408,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

Looks like mail server suffered failure. I'll try to repost later.

---


   ----- The following addresses had permanent fatal errors -----
<qemu-devel@nongnu.org>

   ----- Transcript of session follows -----
550 5.1.2 <qemu-devel@nongnu.org>... Host unknown (Name server: eggs.gnu.org.:
   host not found)

[Qemu-devel] [PATCH 00/22 v3] target-i386: convert CPU features into properties

[Igor Mammedov]

git tree for testing:
  https://github.com/imammedo/qemu/tree/x86-cpu-properties.v3

Depends on:
  [Qemu-devel] [PATCH 0/5] i386: cpu: remove duplicate feature names
    http://www.mail-archive.com/qemu-devel@nongnu.org/msg129458.html

Rebased on top of todays qemu tree.

Igor Mammedov (22):
  target-i386: return Error from cpu_x86_find_by_name()
  target-i386: cpu_x86_register(): report error from property setter
  target-i386: if x86_cpu_realize() failed report error and do cleanup
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: move out CPU features initialization in separate func
  target-i386: xlevel should be more than 0x80000000, move fixup into
    setter
  target-i386: convert cpuid features into properties
  target-i386: add stubs for
    hyperv_(vapic_recommended|relaxed_timing_enabled|get_spinlock_retries)()
  target-i386: convert 'hv_spinlocks' feature into property
  target-i386: convert 'hv_relaxed' feature into property
  target-i386: convert 'hv_vapic' feature into property
  target-i386: convert 'check' and 'enforce' features into properties
  add visitor for parsing hz[KMG] input string
  target-i386: use visit_type_hz to parse tsc_freq property value
  target-i386: introduce vendor-override property
  target-i386: use define for cpuid vendor string size
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: parse cpu_model string into set of stringified
    properties
  target-i386: use properties to set/unset user specified features on
    CPU
  target-i386: move init of "hypervisor" feature into CPU initializer
    from cpudef
  target-i386: move default init of cpuid_kvm_features bitmap into CPU
    initializer from cpudef
  target-i386: cleanup cpu_x86_find_by_name(), only fill x86_def_t in
    it

 qapi/qapi-visit-core.c      |   11 +
 qapi/qapi-visit-core.h      |    2 +
 qapi/string-input-visitor.c |   22 ++
 target-i386/cpu.c           |  733 ++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |   10 +-
 target-i386/helper.c        |    9 +-
 target-i386/hyperv.h        |    9 +-
 7 files changed, 507 insertions(+), 289 deletions(-)

[Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Don Slutz <Don@CloudSwitch.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
--
v2:
    - style change, add braces (reqested by Blue Swirl)
    - removed unused error_is_set(errp) in properties set loop
---
 target-i386/cpu.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c4e6163..e1e2346 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1264,6 +1266,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1355,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1408,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.1

[Qemu-devel] [PATCH 00/22 v2] target-i386: convert CPU features into properties

[Igor Mammedov]

Build and run tested in FC17 host with x86_64-linux-user, x86_64-softmmu
targets

git tree for testing:
  https://github.com/imammedo/qemu/tree/x86-cpu-properties.v2

Igor Mammedov (22):
  target-i386: return Error from cpu_x86_find_by_name()
  target-i386: cpu_x86_register(): report error from property setter
  target-i386: if x86_cpu_realize() failed report error and do cleanup
  target-i386: filter out not TCG features if running without kvm at
    realize time
  target-i386: move out CPU features initialization in separate func
  target-i386: xlevel should be more than 0x80000000, move fixup into
    setter
  target-i386: convert cpuid features into properties
  target-i386: add stubs for
    hyperv_(vapic_recommended|relaxed_timing_enabled|get_spinlock_retries)()
  target-i386: convert 'hv_spinlocks' feature into property
  target-i386: convert 'hv_relaxed' feature into property
  target-i386: convert 'hv_vapic' feature into property
  target-i386: convert 'check' and 'enforce' features into properties
  add visitor for parsing hz[KMG] input string
  target-i386: use visit_type_hz to parse tsc_freq property value
  target-i386: introduce vendor-override property
  target-i386: use define for cpuid vendor string size
  target-i386: replace uint32_t vendor fields by vendor string in
    x86_def_t
  target-i386: parse cpu_model string into set of stringified
    properties
  target-i386: use properties to set/unset user specified features on
    CPU
  target-i386: move init of "hypervisor" feature into CPU initializer
    from cpudef
  target-i386: move default init of cpuid_kvm_features bitmap into CPU
    initializer from cpudef
  target-i386: cleanup cpu_x86_find_by_name(), only fill x86_def_t in
    it

 qapi/qapi-visit-core.c      |  11 +
 qapi/qapi-visit-core.h      |   2 +
 qapi/string-input-visitor.c |  22 ++
 target-i386/cpu.c           | 723 +++++++++++++++++++++++++++-----------------
 target-i386/cpu.h           |  10 +-
 target-i386/helper.c        |   9 +-
 target-i386/hyperv.h        |   9 +-
 7 files changed, 498 insertions(+), 288 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

it will allow to use property setters there later.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
--
v2:
    style change, add braces (reqested by Blue Swirl)
---
 target-i386/cpu.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index ac12139..a89bdc4 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     cpu->env.tsc_khz = value / 1000;
 }
 
-static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
+static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
+                                const char *cpu_model, Error **errp)
 {
     unsigned int i;
     x86_def_t *def;
@@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
             fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
             goto error;
         }
+
+        if (error_is_set(errp)) {
+            goto error;
+        }
+
         featurestr = strtok(NULL, ",");
     }
     x86_cpu_def->features |= plus_features;
@@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
 
 error:
     g_free(s);
+    if (!error_is_set(errp)) {
+        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
+    }
     return -1;
 }
 
@@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     memset(def, 0, sizeof(*def));
 
-    if (cpu_x86_find_by_name(def, cpu_model) < 0)
-        return -1;
+    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
+        goto out;
+    }
+
     if (def->vendor1) {
         env->cpuid_vendor1 = def->vendor1;
         env->cpuid_vendor2 = def->vendor2;
@@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
+
+out:
     if (error_is_set(&error)) {
         error_free(error);
         return -1;
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Don Slutz]

On 09/07/12 16:54, Igor Mammedov wrote:
> it will allow to use property setters there later.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>      style change, add braces (reqested by Blue Swirl)
> ---
>   target-i386/cpu.c | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>       cpu->env.tsc_khz = value / 1000;
>   }
>   
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>   {
>       unsigned int i;
>       x86_def_t *def;
> @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>               fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>               goto error;
>           }
> +
> +        if (error_is_set(errp)) {
> +            goto error;
> +        }
> +
>           featurestr = strtok(NULL, ",");
>       }
>       x86_cpu_def->features |= plus_features;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>   
>   error:
>       g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>       return -1;
>   }
>   
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>   
>       memset(def, 0, sizeof(*def));
>   
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>       if (def->vendor1) {
>           env->cpuid_vendor1 = def->vendor1;
>           env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>           env->cpuid_svm_features &= TCG_SVM_FEATURES;
>       }
>       object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +
> +out:
>       if (error_is_set(&error)) {
>           error_free(error);
>           return -1;
Reviewed-by: Don Slutz <Don@CloudSwitch.com>

Re: [Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Eduardo Habkost]

On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> it will allow to use property setters there later.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> --
> v2:
>     style change, add braces (reqested by Blue Swirl)
> ---
>  target-i386/cpu.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ac12139..a89bdc4 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      cpu->env.tsc_khz = value / 1000;
>  }
>  
> -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> +                                const char *cpu_model, Error **errp)
>  {
>      unsigned int i;
>      x86_def_t *def;
> @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
>              goto error;
>          }
> +
> +        if (error_is_set(errp)) {
> +            goto error;
> +        }
> +

Why add this check, if you never use errp on any code above that line
(and eventually all the code above gets removed by patch 19)?

But is harmless (as error is never set anyway), and this is code that
simply gets removed later in this series. So:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


>          featurestr = strtok(NULL, ",");
>      }
>      x86_cpu_def->features |= plus_features;
> @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
>  
>  error:
>      g_free(s);
> +    if (!error_is_set(errp)) {
> +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> +    }
>      return -1;
>  }
>  
> @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>      memset(def, 0, sizeof(*def));
>  
> -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> -        return -1;
> +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> +        goto out;
> +    }
> +
>      if (def->vendor1) {
>          env->cpuid_vendor1 = def->vendor1;
>          env->cpuid_vendor2 = def->vendor2;
> @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>          env->cpuid_svm_features &= TCG_SVM_FEATURES;
>      }
>      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> +
> +out:
>      if (error_is_set(&error)) {
>          error_free(error);
>          return -1;
> -- 
> 1.7.11.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 01/22] target-i386: return Error from cpu_x86_find_by_name()

[Igor Mammedov]

On Thu, 13 Sep 2012 11:38:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Sep 07, 2012 at 10:54:50PM +0200, Igor Mammedov wrote:
> > it will allow to use property setters there later.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > --
> > v2:
> >     style change, add braces (reqested by Blue Swirl)
> > ---
> >  target-i386/cpu.c | 19 ++++++++++++++++---
> >  1 file changed, 16 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ac12139..a89bdc4 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1086,7 +1086,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      cpu->env.tsc_khz = value / 1000;
> >  }
> >  
> > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> > +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def,
> > +                                const char *cpu_model, Error **errp)
> >  {
> >      unsigned int i;
> >      x86_def_t *def;
> > @@ -1241,6 +1242,11 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> >              fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr);
> >              goto error;
> >          }
> > +
> > +        if (error_is_set(errp)) {
> > +            goto error;
> > +        }
> > +
> 
> Why add this check, if you never use errp on any code above that line
> (and eventually all the code above gets removed by patch 19)?
It looks like remnants from initial version where each converted feature had
setter in this loop. 
I'll remove it for next respin.

> 
> But is harmless (as error is never set anyway), and this is code that
> simply gets removed later in this series. So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> 
> >          featurestr = strtok(NULL, ",");
> >      }
> >      x86_cpu_def->features |= plus_features;
> > @@ -1264,6 +1270,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model)
> >  
> >  error:
> >      g_free(s);
> > +    if (!error_is_set(errp)) {
> > +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> > +    }
> >      return -1;
> >  }
> >  
> > @@ -1350,8 +1359,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >  
> >      memset(def, 0, sizeof(*def));
> >  
> > -    if (cpu_x86_find_by_name(def, cpu_model) < 0)
> > -        return -1;
> > +    if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) {
> > +        goto out;
> > +    }
> > +
> >      if (def->vendor1) {
> >          env->cpuid_vendor1 = def->vendor1;
> >          env->cpuid_vendor2 = def->vendor2;
> > @@ -1401,6 +1412,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
> >          env->cpuid_svm_features &= TCG_SVM_FEATURES;
> >      }
> >      object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
> > +
> > +out:
> >      if (error_is_set(&error)) {
> >          error_free(error);
> >          return -1;
> > -- 
> > 1.7.11.4
> > 
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor