[Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

Current CLI option -cpu cpux,features serves as template
for all created cpus of type: cpux. However QEMU parses
"features" every time it creates a cpu instance and applies
them to it while doing parsing.

That doesn't work well with -device/device_add infrastructure
as it has no idea about cpu specific hooks that's used for
parsing "features".
In order to make -device/device_add utilize "-cpu features"
template convert it into a set of global properties, so that
every new CPU created will have them applied automatically.

That also allows to parse features only once, instread of
doing it for every CPU instance created.


Igor Mammedov (8):
  target-i386: cpu: move features logic that requires CPUState to
    realize time
  target-i386: cpu: move xcc->kvm_required check to reaize time
  target-i386: cpu: use cpu_generic_init() in cpu_x86_init()
  target-i386: cpu: consolidate calls of object_property_parse() in
    x86_cpu_parse_featurestr
  target-sparc: cpu: use sparc_cpu_parse_features() directly
  cpu: use CPUClass->parse_features() as convertor to global properties
  arm: virt: parse cpu_model only once
  pc: parse cpu features only once

 hw/arm/virt.c      |  41 +++++++-------
 hw/i386/pc.c       |  37 +++++++++----
 include/qom/cpu.h  |   2 +-
 qom/cpu.c          |  29 +++++-----
 target-i386/cpu.c  | 155 +++++++++++++++++++----------------------------------
 target-i386/cpu.h  |   1 -
 target-sparc/cpu.c |   7 +--
 7 files changed, 125 insertions(+), 147 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

it will allow to make x86_cpu_parse_featurestr() a pure convertor
of legacy features string into global properties.
That way -cpu FOOCPU,feat1=x,feat2=y,... is parsed only once
and as result of x86_cpu_parse_featurestr() a corresponding set
of global properties for specified CPU type are registered.
So whenever a CPU device is created, generic device machinery
will apply that global properties for us.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3fbc6f3..6159a7f 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
     }
 }
 
+/* Features to be added */
+static FeatureWordArray plus_features = { 0 };
+/* Features to be removed */
+static FeatureWordArray minus_features = { 0 };
+
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
 static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
@@ -1939,13 +1944,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
 {
     X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
-    FeatureWord w;
-    /* Features to be added */
-    FeatureWordArray plus_features = { 0 };
-    /* Features to be removed */
-    FeatureWordArray minus_features = { 0 };
     uint32_t numvalue;
-    CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
 
     featurestr = features ? strtok(features, ",") : NULL;
@@ -2019,18 +2018,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         }
         featurestr = strtok(NULL, ",");
     }
-
-    if (cpu->host_features) {
-        for (w = 0; w < FEATURE_WORDS; w++) {
-            env->features[w] =
-                x86_cpu_get_supported_feature_word(w, cpu->migratable);
-        }
-    }
-
-    for (w = 0; w < FEATURE_WORDS; w++) {
-        env->features[w] |= plus_features[w];
-        env->features[w] &= ~minus_features[w];
-    }
 }
 
 /* Print all cpuid feature names in featureset
@@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     CPUX86State *env = &cpu->env;
     Error *local_err = NULL;
     static bool ht_warned;
+    FeatureWord w;
 
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
     }
 
+    if (cpu->host_features) {
+        for (w = 0; w < FEATURE_WORDS; w++) {
+            env->features[w] =
+                x86_cpu_get_supported_feature_word(w, cpu->migratable);
+        }
+    }
+
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        cpu->env.features[w] |= plus_features[w];
+        cpu->env.features[w] &= ~minus_features[w];
+    }
+
     if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
         env->cpuid_level = 7;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 06:37:23PM +0200, Igor Mammedov wrote:
> it will allow to make x86_cpu_parse_featurestr() a pure convertor
> of legacy features string into global properties.
> That way -cpu FOOCPU,feat1=x,feat2=y,... is parsed only once

I would write it as "will be parsed only once (this will be
implemented by the next patches)".

> and as result of x86_cpu_parse_featurestr() a corresponding set
> of global properties for specified CPU type are registered.

Global properties are not registered yet (after applying this
patch), so please explain that the static variables are a
temporary hack that will be replaced by global properties in
following patches.

> So whenever a CPU device is created, generic device machinery
> will apply that global properties for us.

I would write "apply those global properties" instead.

> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 3fbc6f3..6159a7f 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
>      }
>  }
>  
> +/* Features to be added */

Please add something like "Features to be added. Will be replaced
by global variables in the future".

> +static FeatureWordArray plus_features = { 0 };
> +/* Features to be removed */
> +static FeatureWordArray minus_features = { 0 };
> +

I see that this hack is replaced by the following patches, but is
there an easy way to remove the CPUState argument from
x86_cpu_parse_featurestr() before we introduce these static
variables? (No problem if there's no way to do that, as long as
the static variables are explicitly documented as a temporary
hack)

>  /* Parse "+feature,-feature,feature=foo" CPU feature string
>   */
>  static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> @@ -1939,13 +1944,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>  {
>      X86CPU *cpu = X86_CPU(cs);
>      char *featurestr; /* Single 'key=value" string being parsed */
> -    FeatureWord w;
> -    /* Features to be added */
> -    FeatureWordArray plus_features = { 0 };
> -    /* Features to be removed */
> -    FeatureWordArray minus_features = { 0 };
>      uint32_t numvalue;
> -    CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
> @@ -2019,18 +2018,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>          }
>          featurestr = strtok(NULL, ",");
>      }
> -
> -    if (cpu->host_features) {
> -        for (w = 0; w < FEATURE_WORDS; w++) {
> -            env->features[w] =
> -                x86_cpu_get_supported_feature_word(w, cpu->migratable);
> -        }
> -    }
> -
> -    for (w = 0; w < FEATURE_WORDS; w++) {
> -        env->features[w] |= plus_features[w];
> -        env->features[w] &= ~minus_features[w];
> -    }
>  }
>  
>  /* Print all cpuid feature names in featureset
> @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      CPUX86State *env = &cpu->env;
>      Error *local_err = NULL;
>      static bool ht_warned;
> +    FeatureWord w;
>  
>      if (cpu->apic_id < 0) {
>          error_setg(errp, "apic-id property was not initialized properly");
>          return;
>      }
>  
> +    if (cpu->host_features) {
> +        for (w = 0; w < FEATURE_WORDS; w++) {
> +            env->features[w] =
> +                x86_cpu_get_supported_feature_word(w, cpu->migratable);
> +        }
> +    }
> +
> +    for (w = 0; w < FEATURE_WORDS; w++) {
> +        cpu->env.features[w] |= plus_features[w];
> +        cpu->env.features[w] &= ~minus_features[w];
> +    }
> +
>      if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
>          env->cpuid_level = 7;
>      }
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Wed, 1 Jun 2016 14:43:09 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]

> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 3fbc6f3..6159a7f 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> >      }
> >  }
> >  
> > +/* Features to be added */
> 
> Please add something like "Features to be added. Will be replaced
> by global variables in the future".
> 
> > +static FeatureWordArray plus_features = { 0 };
> > +/* Features to be removed */
> > +static FeatureWordArray minus_features = { 0 };
> > +
> 
> I see that this hack is replaced by the following patches, but is
> there an easy way to remove the CPUState argument from
> x86_cpu_parse_featurestr() before we introduce these static
> variables? (No problem if there's no way to do that, as long as
> the static variables are explicitly documented as a temporary
> hack)
It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
local to x86 that probably would stay here forever.
I should add comment that explains why +- can't be replaced
with normal properties.

I don't plan to replace plus/minus_features with anything nor to
make this variables a global ones to spread +- x86/sparc legacy
format everywhere.
 
What I would do though before enabling -device/device_add for X86CPU is
to disable +- handling for new machine types so that CPUs would
follow generic property semantic of device used everywhere else.

> 
> >  /* Parse "+feature,-feature,feature=foo" CPU feature string
> >   */
> >  static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > @@ -1939,13 +1944,7 @@ static void
> > x86_cpu_parse_featurestr(CPUState *cs, char *features, {
> >      X86CPU *cpu = X86_CPU(cs);
> >      char *featurestr; /* Single 'key=value" string being parsed */
> > -    FeatureWord w;
> > -    /* Features to be added */
> > -    FeatureWordArray plus_features = { 0 };
> > -    /* Features to be removed */
> > -    FeatureWordArray minus_features = { 0 };
> >      uint32_t numvalue;
> > -    CPUX86State *env = &cpu->env;
> >      Error *local_err = NULL;
> >  
> >      featurestr = features ? strtok(features, ",") : NULL;
> > @@ -2019,18 +2018,6 @@ static void
> > x86_cpu_parse_featurestr(CPUState *cs, char *features, }
> >          featurestr = strtok(NULL, ",");
> >      }
> > -
> > -    if (cpu->host_features) {
> > -        for (w = 0; w < FEATURE_WORDS; w++) {
> > -            env->features[w] =
> > -                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > -        }
> > -    }
> > -
> > -    for (w = 0; w < FEATURE_WORDS; w++) {
> > -        env->features[w] |= plus_features[w];
> > -        env->features[w] &= ~minus_features[w];
> > -    }
> >  }
> >  
> >  /* Print all cpuid feature names in featureset
> > @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState
> > *dev, Error **errp) CPUX86State *env = &cpu->env;
> >      Error *local_err = NULL;
> >      static bool ht_warned;
> > +    FeatureWord w;
> >  
> >      if (cpu->apic_id < 0) {
> >          error_setg(errp, "apic-id property was not initialized
> > properly"); return;
> >      }
> >  
> > +    if (cpu->host_features) {
> > +        for (w = 0; w < FEATURE_WORDS; w++) {
> > +            env->features[w] =
> > +                x86_cpu_get_supported_feature_word(w,
> > cpu->migratable);
> > +        }
> > +    }
> > +
> > +    for (w = 0; w < FEATURE_WORDS; w++) {
> > +        cpu->env.features[w] |= plus_features[w];
> > +        cpu->env.features[w] &= ~minus_features[w];
> > +    }
> > +
> >      if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) {
> >          env->cpuid_level = 7;
> >      }
> > -- 
> > 1.8.3.1
> > 
> 

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:
> On Wed, 1 Jun 2016 14:43:09 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 3fbc6f3..6159a7f 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > >      }
> > >  }
> > >  
> > > +/* Features to be added */
> > 
> > Please add something like "Features to be added. Will be replaced
> > by global variables in the future".
> > 
> > > +static FeatureWordArray plus_features = { 0 };
> > > +/* Features to be removed */
> > > +static FeatureWordArray minus_features = { 0 };
> > > +
> > 
> > I see that this hack is replaced by the following patches, but is
> > there an easy way to remove the CPUState argument from
> > x86_cpu_parse_featurestr() before we introduce these static
> > variables? (No problem if there's no way to do that, as long as
> > the static variables are explicitly documented as a temporary
> > hack)
> It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> local to x86 that probably would stay here forever.
> I should add comment that explains why +- can't be replaced
> with normal properties.

Oh, I assumed it would be temporary. In that case, I would like
to avoid adding the static variables if possible.

> 
> I don't plan to replace plus/minus_features with anything nor to
> make this variables a global ones to spread +- x86/sparc legacy
> format everywhere.

Can't the +/- semantics be emulated by simply registering
plus_features/minus_features after the other global properties
are registered inside x86_cpu_parse_featurestr()?

>  
> What I would do though before enabling -device/device_add for X86CPU is
> to disable +- handling for new machine types so that CPUs would
> follow generic property semantic of device used everywhere else.

We can't do that unless we give libvirt (and users that have
their own scripts) time to adapt to the new syntax (and we warn
users that newer QEMU versions will require newer libvirt).

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Thu, 2 Jun 2016 11:38:26 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:
> > On Wed, 1 Jun 2016 14:43:09 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > [...]
> >   
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index 3fbc6f3..6159a7f 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > >      }
> > > >  }
> > > >  
> > > > +/* Features to be added */  
> > > 
> > > Please add something like "Features to be added. Will be replaced
> > > by global variables in the future".
> > >   
> > > > +static FeatureWordArray plus_features = { 0 };
> > > > +/* Features to be removed */
> > > > +static FeatureWordArray minus_features = { 0 };
> > > > +  
> > > 
> > > I see that this hack is replaced by the following patches, but is
> > > there an easy way to remove the CPUState argument from
> > > x86_cpu_parse_featurestr() before we introduce these static
> > > variables? (No problem if there's no way to do that, as long as
> > > the static variables are explicitly documented as a temporary
> > > hack)  
> > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> > local to x86 that probably would stay here forever.
> > I should add comment that explains why +- can't be replaced
> > with normal properties.  
> 
> Oh, I assumed it would be temporary. In that case, I would like
> to avoid adding the static variables if possible.
> 
> > 
> > I don't plan to replace plus/minus_features with anything nor to
> > make this variables a global ones to spread +- x86/sparc legacy
> > format everywhere.  
> 
> Can't the +/- semantics be emulated by simply registering
> plus_features/minus_features after the other global properties
> are registered inside x86_cpu_parse_featurestr()?
it could be done, at the first glance it will take 2 extra parsing passes

1: copy featurestr, parse feat=x,feat
2: copy featurestr, parse +feat
3: copy featurestr, parse -feat

but that probably will complicate way to disable +-feat handling in future,
with current static vars it's just a matter of specifying compat-prop
for X86CPU driver in appropriate machine type.
So I'd leave it as is unless you insist on doing it like you suggested above.
 
> >  
> > What I would do though before enabling -device/device_add for X86CPU is
> > to disable +- handling for new machine types so that CPUs would
> > follow generic property semantic of device used everywhere else.  
> 
> We can't do that unless we give libvirt (and users that have
> their own scripts) time to adapt to the new syntax
leaving it enabled will lead to mixed semantics in combination
with device_add that will be even more confusing to users
if users will use both:
 for example: -cpu cpu,-featx and -device cpu,featx=on
That's why I'm suggesting to make a clean break in new machine
type with error saying to replace legacy +-feat with canonical one.
For old machine types nothing would break as it would still use
legacy syntax and legacy cpu-add, with device_add disabled.

We probably can fix libvirt in sync with this QEMU release
if it still uses +- syntax.

> (and we warn users that newer QEMU versions will require newer libvirt).
yep we should do it in release notes.

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 11:38:26 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:
> > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > [...]
> > >   
> > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > index 3fbc6f3..6159a7f 100644
> > > > > --- a/target-i386/cpu.c
> > > > > +++ b/target-i386/cpu.c
> > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +/* Features to be added */  
> > > > 
> > > > Please add something like "Features to be added. Will be replaced
> > > > by global variables in the future".
> > > >   
> > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > +/* Features to be removed */
> > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > +  
> > > > 
> > > > I see that this hack is replaced by the following patches, but is
> > > > there an easy way to remove the CPUState argument from
> > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > variables? (No problem if there's no way to do that, as long as
> > > > the static variables are explicitly documented as a temporary
> > > > hack)  
> > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> > > local to x86 that probably would stay here forever.
> > > I should add comment that explains why +- can't be replaced
> > > with normal properties.  
> > 
> > Oh, I assumed it would be temporary. In that case, I would like
> > to avoid adding the static variables if possible.
> > 
> > > 
> > > I don't plan to replace plus/minus_features with anything nor to
> > > make this variables a global ones to spread +- x86/sparc legacy
> > > format everywhere.  
> > 
> > Can't the +/- semantics be emulated by simply registering
> > plus_features/minus_features after the other global properties
> > are registered inside x86_cpu_parse_featurestr()?
> it could be done, at the first glance it will take 2 extra parsing passes
> 
> 1: copy featurestr, parse feat=x,feat
> 2: copy featurestr, parse +feat
> 3: copy featurestr, parse -feat

Why? Can't we just replace plus_features and minus_features with
two string lists (or a QDict), and make the corresponding
object_property_parse()/qdev_prop_register_global() calls after
the main parsing loop?

(Didn't you do that in your old "target-i386: set [+-]feature
using static properties" patch?)

> 
> but that probably will complicate way to disable +-feat handling in future,
> with current static vars it's just a matter of specifying compat-prop
> for X86CPU driver in appropriate machine type.

I see. But I don't see why we need the extra machine-type cruft.

To me, the whole point of removing the old syntax is to make the
code simpler. If we have to add even more code/complexity just to
add a machine-type restriction (but keeping the old code there),
I don't see the point.

I believe we should either: 1) remove it completely after people
have time to update their scripts/libvirt; or 2) just keep it
working on all machine-types, but print a warning every time
people use the old syntax.

(I am not sure if we should do (1) without giving users a long
time to adapt, so I suggest we do (2) by now)


> So I'd leave it as is unless you insist on doing it like you suggested above.
>  
> > >  
> > > What I would do though before enabling -device/device_add for X86CPU is
> > > to disable +- handling for new machine types so that CPUs would
> > > follow generic property semantic of device used everywhere else.  
> > 
> > We can't do that unless we give libvirt (and users that have
> > their own scripts) time to adapt to the new syntax
> leaving it enabled will lead to mixed semantics in combination
> with device_add that will be even more confusing to users
> if users will use both:
>  for example: -cpu cpu,-featx and -device cpu,featx=on
> That's why I'm suggesting to make a clean break in new machine
> type with error saying to replace legacy +-feat with canonical one.
> For old machine types nothing would break as it would still use
> legacy syntax and legacy cpu-add, with device_add disabled.

Just warn users very clearly, if they use the old syntax. If they
insist in using it, they shouldn't blame us if they get confused.

> 
> We probably can fix libvirt in sync with this QEMU release
> if it still uses +- syntax.

I would wait for at least 1 or 2 libvirt releases before removing
support.

But as I said above: if we are not deleting any code (and are
adding extra code instead), I don't see the point of forcibly
disabling it. We can just leave it there and print a warning.

> 
> > (and we warn users that newer QEMU versions will require newer libvirt).
> yep we should do it in release notes.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Thu, 2 Jun 2016 14:34:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 11:38:26 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:
> > > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > [...]
> > > >   
> > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > index 3fbc6f3..6159a7f 100644
> > > > > > --- a/target-i386/cpu.c
> > > > > > +++ b/target-i386/cpu.c
> > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > +/* Features to be added */  
> > > > > 
> > > > > Please add something like "Features to be added. Will be
> > > > > replaced by global variables in the future".
> > > > >   
> > > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > > +/* Features to be removed */
> > > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > > +  
> > > > > 
> > > > > I see that this hack is replaced by the following patches,
> > > > > but is there an easy way to remove the CPUState argument from
> > > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > > variables? (No problem if there's no way to do that, as long
> > > > > as the static variables are explicitly documented as a
> > > > > temporary hack)  
> > > > It's hack to keep legacy +- semantic (i.e. it overrides
> > > > feat1=x,feat2) local to x86 that probably would stay here
> > > > forever. I should add comment that explains why +- can't be
> > > > replaced with normal properties.  
> > > 
> > > Oh, I assumed it would be temporary. In that case, I would like
> > > to avoid adding the static variables if possible.
> > > 
> > > > 
> > > > I don't plan to replace plus/minus_features with anything nor to
> > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > format everywhere.  
> > > 
> > > Can't the +/- semantics be emulated by simply registering
> > > plus_features/minus_features after the other global properties
> > > are registered inside x86_cpu_parse_featurestr()?
> > it could be done, at the first glance it will take 2 extra parsing
> > passes
> > 
> > 1: copy featurestr, parse feat=x,feat
> > 2: copy featurestr, parse +feat
> > 3: copy featurestr, parse -feat
> 
> Why? Can't we just replace plus_features and minus_features with
> two string lists (or a QDict), and make the corresponding
> object_property_parse()/qdev_prop_register_global() calls after
> the main parsing loop?
> 
> (Didn't you do that in your old "target-i386: set [+-]feature
> using static properties" patch?)
ok, I'll do that.

> 
> > 
> > but that probably will complicate way to disable +-feat handling in
> > future, with current static vars it's just a matter of specifying
> > compat-prop for X86CPU driver in appropriate machine type.
> 
> I see. But I don't see why we need the extra machine-type cruft.
> 
> To me, the whole point of removing the old syntax is to make the
> code simpler. If we have to add even more code/complexity just to
> add a machine-type restriction (but keeping the old code there),
> I don't see the point.
> 
> I believe we should either: 1) remove it completely after people
> have time to update their scripts/libvirt; or 2) just keep it
> working on all machine-types, but print a warning every time
> people use the old syntax.
> 
> (I am not sure if we should do (1) without giving users a long
> time to adapt, so I suggest we do (2) by now)
> 
> 
> > So I'd leave it as is unless you insist on doing it like you
> > suggested above. 
> > > >  
> > > > What I would do though before enabling -device/device_add for
> > > > X86CPU is to disable +- handling for new machine types so that
> > > > CPUs would follow generic property semantic of device used
> > > > everywhere else.  
> > > 
> > > We can't do that unless we give libvirt (and users that have
> > > their own scripts) time to adapt to the new syntax
> > leaving it enabled will lead to mixed semantics in combination
> > with device_add that will be even more confusing to users
> > if users will use both:
> >  for example: -cpu cpu,-featx and -device cpu,featx=on
> > That's why I'm suggesting to make a clean break in new machine
> > type with error saying to replace legacy +-feat with canonical one.
> > For old machine types nothing would break as it would still use
> > legacy syntax and legacy cpu-add, with device_add disabled.
> 
> Just warn users very clearly, if they use the old syntax. If they
> insist in using it, they shouldn't blame us if they get confused.
> 
> > 
> > We probably can fix libvirt in sync with this QEMU release
> > if it still uses +- syntax.
> 
> I would wait for at least 1 or 2 libvirt releases before removing
> support.
> 
> But as I said above: if we are not deleting any code (and are
> adding extra code instead), I don't see the point of forcibly
> disabling it. We can just leave it there and print a warning.
ok, lets go with warning, saying"

"[+-]feature syntax is obsoleted and will be removed in future,
it's recommended to use canonical property syntax 'feature=value'"

> 
> > 
> > > (and we warn users that newer QEMU versions will require newer
> > > libvirt).
> > yep we should do it in release notes.

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 08:23:37PM +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 14:34:27 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > > > 
> > > > > I don't plan to replace plus/minus_features with anything nor to
> > > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > > format everywhere.  
> > > > 
> > > > Can't the +/- semantics be emulated by simply registering
> > > > plus_features/minus_features after the other global properties
> > > > are registered inside x86_cpu_parse_featurestr()?
> > > it could be done, at the first glance it will take 2 extra parsing
> > > passes
> > > 
> > > 1: copy featurestr, parse feat=x,feat
> > > 2: copy featurestr, parse +feat
> > > 3: copy featurestr, parse -feat
> > 
> > Why? Can't we just replace plus_features and minus_features with
> > two string lists (or a QDict), and make the corresponding
> > object_property_parse()/qdev_prop_register_global() calls after
> > the main parsing loop?
> > 
> > (Didn't you do that in your old "target-i386: set [+-]feature
> > using static properties" patch?)
> ok, I'll do that.

Thanks!

[...]
> > But as I said above: if we are not deleting any code (and are
> > adding extra code instead), I don't see the point of forcibly
> > disabling it. We can just leave it there and print a warning.
> ok, lets go with warning, saying"
> 
> "[+-]feature syntax is obsoleted and will be removed in future,
> it's recommended to use canonical property syntax 'feature=value'"

We could print "+%s is obsolete [...] use '%s=on'" and
"-%s is obsolete [...] use '%s=off'", depending on the case. This
way, users can see more easily what's the proper syntax.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Thu, 2 Jun 2016 14:34:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 11:38:26 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:  
> > > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > [...]
> > > >     
> > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > index 3fbc6f3..6159a7f 100644
> > > > > > --- a/target-i386/cpu.c
> > > > > > +++ b/target-i386/cpu.c
> > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > > >      }
> > > > > >  }
> > > > > >  
> > > > > > +/* Features to be added */    
> > > > > 
> > > > > Please add something like "Features to be added. Will be replaced
> > > > > by global variables in the future".
> > > > >     
> > > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > > +/* Features to be removed */
> > > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > > +    
> > > > > 
> > > > > I see that this hack is replaced by the following patches, but is
> > > > > there an easy way to remove the CPUState argument from
> > > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > > variables? (No problem if there's no way to do that, as long as
> > > > > the static variables are explicitly documented as a temporary
> > > > > hack)    
> > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> > > > local to x86 that probably would stay here forever.
> > > > I should add comment that explains why +- can't be replaced
> > > > with normal properties.    
> > > 
> > > Oh, I assumed it would be temporary. In that case, I would like
> > > to avoid adding the static variables if possible.
> > >   
> > > > 
> > > > I don't plan to replace plus/minus_features with anything nor to
> > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > format everywhere.    
> > > 
> > > Can't the +/- semantics be emulated by simply registering
> > > plus_features/minus_features after the other global properties
> > > are registered inside x86_cpu_parse_featurestr()?  
> > it could be done, at the first glance it will take 2 extra parsing passes
> > 
> > 1: copy featurestr, parse feat=x,feat
> > 2: copy featurestr, parse +feat
> > 3: copy featurestr, parse -feat  
> 
> Why? Can't we just replace plus_features and minus_features with
> two string lists (or a QDict), and make the corresponding
> object_property_parse()/qdev_prop_register_global() calls after
> the main parsing loop?
> 
> (Didn't you do that in your old "target-i386: set [+-]feature
> using static properties" patch?)
It doesn't look like it will work due to broken 4d1b279b0 as
plus_features/minus_features are applied after:

    if (cpu->host_features) {                                                    
        for (w = 0; w < FEATURE_WORDS; w++) {                                    
            env->features[w] =                                                   
                x86_cpu_get_supported_feature_word(w, cpu->migratable);          
        }                                                                        
    }

and with above moving to realize(), +-feats would be overwritten by it.
Lets temporary use static variables as in this patch so not to delay
series on not related fixes. And deal with it when 4d1b279b0 is fixed.

1 way to deal with it is to wait several releases till users fix their
+-feats CLIs and then just drop it.

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Eduardo Habkost]

On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 14:34:27 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> > > On Thu, 2 Jun 2016 11:38:26 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >   
> > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:  
> > > > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > [...]
> > > > >     
> > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > index 3fbc6f3..6159a7f 100644
> > > > > > > --- a/target-i386/cpu.c
> > > > > > > +++ b/target-i386/cpu.c
> > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > > > >      }
> > > > > > >  }
> > > > > > >  
> > > > > > > +/* Features to be added */    
> > > > > > 
> > > > > > Please add something like "Features to be added. Will be replaced
> > > > > > by global variables in the future".
> > > > > >     
> > > > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > > > +/* Features to be removed */
> > > > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > > > +    
> > > > > > 
> > > > > > I see that this hack is replaced by the following patches, but is
> > > > > > there an easy way to remove the CPUState argument from
> > > > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > > > variables? (No problem if there's no way to do that, as long as
> > > > > > the static variables are explicitly documented as a temporary
> > > > > > hack)    
> > > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> > > > > local to x86 that probably would stay here forever.
> > > > > I should add comment that explains why +- can't be replaced
> > > > > with normal properties.    
> > > > 
> > > > Oh, I assumed it would be temporary. In that case, I would like
> > > > to avoid adding the static variables if possible.
> > > >   
> > > > > 
> > > > > I don't plan to replace plus/minus_features with anything nor to
> > > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > > format everywhere.    
> > > > 
> > > > Can't the +/- semantics be emulated by simply registering
> > > > plus_features/minus_features after the other global properties
> > > > are registered inside x86_cpu_parse_featurestr()?  
> > > it could be done, at the first glance it will take 2 extra parsing passes
> > > 
> > > 1: copy featurestr, parse feat=x,feat
> > > 2: copy featurestr, parse +feat
> > > 3: copy featurestr, parse -feat  
> > 
> > Why? Can't we just replace plus_features and minus_features with
> > two string lists (or a QDict), and make the corresponding
> > object_property_parse()/qdev_prop_register_global() calls after
> > the main parsing loop?
> > 
> > (Didn't you do that in your old "target-i386: set [+-]feature
> > using static properties" patch?)
> It doesn't look like it will work due to broken 4d1b279b0 as
> plus_features/minus_features are applied after:
> 
>     if (cpu->host_features) {                                                    
>         for (w = 0; w < FEATURE_WORDS; w++) {                                    
>             env->features[w] =                                                   
>                 x86_cpu_get_supported_feature_word(w, cpu->migratable);          
>         }                                                                        
>     }
> 
> and with above moving to realize(), +-feats would be overwritten by it.
> Lets temporary use static variables as in this patch so not to delay
> series on not related fixes. And deal with it when 4d1b279b0 is fixed.
> 
> 1 way to deal with it is to wait several releases till users fix their
> +-feats CLIs and then just drop it.

We can fix that after getting rid the host_features hack (and
also fix the "-cpu host,foo=off,foo=on" bug we already have).

In that case, I think we can live with the static variables
temporarily in the meantime. Can you add a comment above the
static variable declarations saying that they can't be replaced
by globals yet because of the host_features hack?

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov]

On Fri, 3 Jun 2016 16:26:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 14:34:27 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:  
> > > > On Thu, 2 Jun 2016 11:38:26 -0300
> > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >     
> > > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote:    
> > > > > > On Wed, 1 Jun 2016 14:43:09 -0300
> > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > > > [...]
> > > > > >       
> > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > > > > > index 3fbc6f3..6159a7f 100644
> > > > > > > > --- a/target-i386/cpu.c
> > > > > > > > +++ b/target-i386/cpu.c
> > > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s)
> > > > > > > >      }
> > > > > > > >  }
> > > > > > > >  
> > > > > > > > +/* Features to be added */      
> > > > > > > 
> > > > > > > Please add something like "Features to be added. Will be replaced
> > > > > > > by global variables in the future".
> > > > > > >       
> > > > > > > > +static FeatureWordArray plus_features = { 0 };
> > > > > > > > +/* Features to be removed */
> > > > > > > > +static FeatureWordArray minus_features = { 0 };
> > > > > > > > +      
> > > > > > > 
> > > > > > > I see that this hack is replaced by the following patches, but is
> > > > > > > there an easy way to remove the CPUState argument from
> > > > > > > x86_cpu_parse_featurestr() before we introduce these static
> > > > > > > variables? (No problem if there's no way to do that, as long as
> > > > > > > the static variables are explicitly documented as a temporary
> > > > > > > hack)      
> > > > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2)
> > > > > > local to x86 that probably would stay here forever.
> > > > > > I should add comment that explains why +- can't be replaced
> > > > > > with normal properties.      
> > > > > 
> > > > > Oh, I assumed it would be temporary. In that case, I would like
> > > > > to avoid adding the static variables if possible.
> > > > >     
> > > > > > 
> > > > > > I don't plan to replace plus/minus_features with anything nor to
> > > > > > make this variables a global ones to spread +- x86/sparc legacy
> > > > > > format everywhere.      
> > > > > 
> > > > > Can't the +/- semantics be emulated by simply registering
> > > > > plus_features/minus_features after the other global properties
> > > > > are registered inside x86_cpu_parse_featurestr()?    
> > > > it could be done, at the first glance it will take 2 extra parsing passes
> > > > 
> > > > 1: copy featurestr, parse feat=x,feat
> > > > 2: copy featurestr, parse +feat
> > > > 3: copy featurestr, parse -feat    
> > > 
> > > Why? Can't we just replace plus_features and minus_features with
> > > two string lists (or a QDict), and make the corresponding
> > > object_property_parse()/qdev_prop_register_global() calls after
> > > the main parsing loop?
> > > 
> > > (Didn't you do that in your old "target-i386: set [+-]feature
> > > using static properties" patch?)  
> > It doesn't look like it will work due to broken 4d1b279b0 as
> > plus_features/minus_features are applied after:
> > 
> >     if (cpu->host_features) {                                                    
> >         for (w = 0; w < FEATURE_WORDS; w++) {                                    
> >             env->features[w] =                                                   
> >                 x86_cpu_get_supported_feature_word(w, cpu->migratable);          
> >         }                                                                        
> >     }
> > 
> > and with above moving to realize(), +-feats would be overwritten by it.
> > Lets temporary use static variables as in this patch so not to delay
> > series on not related fixes. And deal with it when 4d1b279b0 is fixed.
> > 
> > 1 way to deal with it is to wait several releases till users fix their
> > +-feats CLIs and then just drop it.  
> 
> We can fix that after getting rid the host_features hack (and
> also fix the "-cpu host,foo=off,foo=on" bug we already have).
> 
> In that case, I think we can live with the static variables
> temporarily in the meantime. Can you add a comment above the
> static variable declarations saying that they can't be replaced
> by globals yet because of the host_features hack?
Sure, thanks!

[Qemu-devel] [PATCH RFC 2/8] target-i386: cpu: move xcc->kvm_required check to reaize time

[Igor Mammedov]

it will allow to drop custom cpu_x86_init() and use
cpu_generic_init() instead reducing cpu_x86_create()
to a simple 3-liner.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6159a7f..c31afc7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2200,7 +2200,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
-    X86CPUClass *xcc;
     ObjectClass *oc;
     gchar **model_pieces;
     char *name, *features;
@@ -2219,12 +2218,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
         error_setg(&error, "Unable to find CPU definition: %s", name);
         goto out;
     }
-    xcc = X86_CPU_CLASS(oc);
-
-    if (xcc->kvm_required && !kvm_enabled()) {
-        error_setg(&error, "CPU model '%s' requires KVM", name);
-        goto out;
-    }
 
     cpu = X86_CPU(object_new(object_class_get_name(oc)));
 
@@ -2901,6 +2894,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
     static bool ht_warned;
     FeatureWord w;
 
+    if (xcc->kvm_required && !kvm_enabled()) {
+        char *name = g_strdup(object_get_typename(OBJECT(dev)));
+        *strstr(name, X86_CPU_TYPE_SUFFIX) = 0;
+        error_setg(&local_err, "CPU model '%s' requires KVM", name);
+        goto out;
+    }
+
     if (cpu->apic_id < 0) {
         error_setg(errp, "apic-id property was not initialized properly");
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 2/8] target-i386: cpu: move xcc->kvm_required check to reaize time

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 06:37:24PM +0200, Igor Mammedov wrote:
> it will allow to drop custom cpu_x86_init() and use
> cpu_generic_init() instead reducing cpu_x86_create()
> to a simple 3-liner.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Nice, it also gets us closer to finally making X86CPU
qmp_device_list_properties()-safe. Only one request:

> ---
>  target-i386/cpu.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 6159a7f..c31afc7 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2200,7 +2200,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
>  X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>  {
>      X86CPU *cpu = NULL;
> -    X86CPUClass *xcc;
>      ObjectClass *oc;
>      gchar **model_pieces;
>      char *name, *features;
> @@ -2219,12 +2218,6 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
>          error_setg(&error, "Unable to find CPU definition: %s", name);
>          goto out;
>      }
> -    xcc = X86_CPU_CLASS(oc);
> -
> -    if (xcc->kvm_required && !kvm_enabled()) {
> -        error_setg(&error, "CPU model '%s' requires KVM", name);
> -        goto out;
> -    }
>  
>      cpu = X86_CPU(object_new(object_class_get_name(oc)));
>  
> @@ -2901,6 +2894,13 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
>      static bool ht_warned;
>      FeatureWord w;
>  
> +    if (xcc->kvm_required && !kvm_enabled()) {
> +        char *name = g_strdup(object_get_typename(OBJECT(dev)));
> +        *strstr(name, X86_CPU_TYPE_SUFFIX) = 0;

I am not sure if I actually like this trick, but in either case
please use x86_cpu_class_get_model_name().

> +        error_setg(&local_err, "CPU model '%s' requires KVM", name);
> +        goto out;
> +    }
> +
>      if (cpu->apic_id < 0) {
>          error_setg(errp, "apic-id property was not initialized properly");
>          return;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 2/8] target-i386: cpu: move xcc->kvm_required check to reaize time

[Igor Mammedov]

On Wed, 1 Jun 2016 14:46:51 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 01, 2016 at 06:37:24PM +0200, Igor Mammedov wrote:
> > it will allow to drop custom cpu_x86_init() and use
> > cpu_generic_init() instead reducing cpu_x86_create()
> > to a simple 3-liner.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Nice, it also gets us closer to finally making X86CPU
> qmp_device_list_properties()-safe. Only one request:
> 
> > ---
> >  target-i386/cpu.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 6159a7f..c31afc7 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -2200,7 +2200,6 @@ static void x86_cpu_load_def(X86CPU *cpu,
> > X86CPUDefinition *def, Error **errp) X86CPU *cpu_x86_create(const
> > char *cpu_model, Error **errp) {
> >      X86CPU *cpu = NULL;
> > -    X86CPUClass *xcc;
> >      ObjectClass *oc;
> >      gchar **model_pieces;
> >      char *name, *features;
> > @@ -2219,12 +2218,6 @@ X86CPU *cpu_x86_create(const char
> > *cpu_model, Error **errp) error_setg(&error, "Unable to find CPU
> > definition: %s", name); goto out;
> >      }
> > -    xcc = X86_CPU_CLASS(oc);
> > -
> > -    if (xcc->kvm_required && !kvm_enabled()) {
> > -        error_setg(&error, "CPU model '%s' requires KVM", name);
> > -        goto out;
> > -    }
> >  
> >      cpu = X86_CPU(object_new(object_class_get_name(oc)));
> >  
> > @@ -2901,6 +2894,13 @@ static void x86_cpu_realizefn(DeviceState
> > *dev, Error **errp) static bool ht_warned;
> >      FeatureWord w;
> >  
> > +    if (xcc->kvm_required && !kvm_enabled()) {
> > +        char *name = g_strdup(object_get_typename(OBJECT(dev)));
> > +        *strstr(name, X86_CPU_TYPE_SUFFIX) = 0;
> 
> I am not sure if I actually like this trick, but in either case
> please use x86_cpu_class_get_model_name().
thanks for pointing out that x86_cpu_class_get_model_name() exists.

> 
> > +        error_setg(&local_err, "CPU model '%s' requires KVM",
> > name);
> > +        goto out;
> > +    }
> > +
> >      if (cpu->apic_id < 0) {
> >          error_setg(errp, "apic-id property was not initialized
> > properly"); return;
> > -- 
> > 1.8.3.1
> > 
> 

[Qemu-devel] [PATCH RFC 3/8] target-i386: cpu: use cpu_generic_init() in cpu_x86_init()

[Igor Mammedov]

now cpu_x86_init() does nothing more or less
than duplicating cpu_generic_init() logic.
So simplify it by using cpu_generic_init().

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c31afc7..238f69d 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2240,25 +2240,7 @@ out:
 
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
-    Error *error = NULL;
-    X86CPU *cpu;
-
-    cpu = cpu_x86_create(cpu_model, &error);
-    if (error) {
-        goto out;
-    }
-
-    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
-
-out:
-    if (error) {
-        error_report_err(error);
-        if (cpu != NULL) {
-            object_unref(OBJECT(cpu));
-            cpu = NULL;
-        }
-    }
-    return cpu;
+    return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
 }
 
 static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 3/8] target-i386: cpu: use cpu_generic_init() in cpu_x86_init()

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 06:37:25PM +0200, Igor Mammedov wrote:
> now cpu_x86_init() does nothing more or less
> than duplicating cpu_generic_init() logic.
> So simplify it by using cpu_generic_init().
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Awesome.

It bothers me that we're replacing a function with consistent
error reporting with one where error reporting is inconsistent.
cpu_generic_init() sometimes prints error messages, and sometimes
simply returns NULL without printing anything.

But fixing that would require reviewing all cpu_init()
implementations. In the meantime, we are being as inconsistent as
all the other architectures that already use cpu_generic_init(),
so:

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

> ---
>  target-i386/cpu.c | 20 +-------------------
>  1 file changed, 1 insertion(+), 19 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c31afc7..238f69d 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -2240,25 +2240,7 @@ out:
>  
>  X86CPU *cpu_x86_init(const char *cpu_model)
>  {
> -    Error *error = NULL;
> -    X86CPU *cpu;
> -
> -    cpu = cpu_x86_create(cpu_model, &error);
> -    if (error) {
> -        goto out;
> -    }
> -
> -    object_property_set_bool(OBJECT(cpu), true, "realized", &error);
> -
> -out:
> -    if (error) {
> -        error_report_err(error);
> -        if (cpu != NULL) {
> -            object_unref(OBJECT(cpu));
> -            cpu = NULL;
> -        }
> -    }
> -    return cpu;
> +    return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
>  }
>  
>  static void x86_cpu_cpudef_class_init(ObjectClass *oc, void *data)
> -- 
> 1.8.3.1
> 

-- 
Eduardo

[Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 238f69d..618aef9 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
     uint32_t numvalue;
+    char num[32];
+    const char *name;
+    char *err;
     Error *local_err = NULL;
 
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
-        char *val;
+        char *val = NULL;
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
         } else if (featurestr[0] == '-') {
@@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             feat2prop(featurestr);
+            name = featurestr;
             if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                char num[32];
-
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
                     error_setg(errp, "bad numerical value %s", val);
@@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                     numvalue += 0x80000000;
                 }
                 snprintf(num, sizeof(num), "%" PRIu32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
+                val = num;
             } else if (!strcmp(featurestr, "tsc-freq")) {
                 int64_t tsc_freq;
-                char *err;
-                char num[32];
 
                 tsc_freq = qemu_strtosz_suffix_unit(val, &err,
                                                QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
@@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                     return;
                 }
                 snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
-                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
-                                      &local_err);
+                val = num;
+                name = "tsc-frequency";
             } else if (!strcmp(featurestr, "hv-spinlocks")) {
-                char *err;
                 const int min = 0xFFF;
-                char num[32];
+
                 numvalue = strtoul(val, &err, 0);
                 if (!*val || *err) {
                     error_setg(errp, "bad numerical value %s", val);
@@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                     numvalue = min;
                 }
                 snprintf(num, sizeof(num), "%" PRId32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
-            } else {
-                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
+                val = num;
             }
         } else {
             feat2prop(featurestr);
-            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
+            name = featurestr;
+            val = (char *)"on";
         }
+
+        if (val) {
+            object_property_parse(OBJECT(cpu), val, name, &local_err);
+        }
+
         if (local_err) {
             error_propagate(errp, local_err);
             return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 238f69d..618aef9 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>      X86CPU *cpu = X86_CPU(cs);
>      char *featurestr; /* Single 'key=value" string being parsed */
>      uint32_t numvalue;
> +    char num[32];
> +    const char *name;
> +    char *err;
>      Error *local_err = NULL;
>  
>      featurestr = features ? strtok(features, ",") : NULL;
>  
>      while (featurestr) {
> -        char *val;
> +        char *val = NULL;
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
>          } else if (featurestr[0] == '-') {
> @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>          } else if ((val = strchr(featurestr, '='))) {
>              *val = 0; val++;
>              feat2prop(featurestr);
> +            name = featurestr;
>              if (!strcmp(featurestr, "xlevel")) {
> -                char *err;
> -                char num[32];
> -
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
>                      error_setg(errp, "bad numerical value %s", val);
> @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                      numvalue += 0x80000000;
>                  }
>                  snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> +                val = num;
>              } else if (!strcmp(featurestr, "tsc-freq")) {
>                  int64_t tsc_freq;
> -                char *err;
> -                char num[32];
>  
>                  tsc_freq = qemu_strtosz_suffix_unit(val, &err,
>                                                 QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                      return;
>                  }
>                  snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> -                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> -                                      &local_err);
> +                val = num;
> +                name = "tsc-frequency";
>              } else if (!strcmp(featurestr, "hv-spinlocks")) {
> -                char *err;
>                  const int min = 0xFFF;
> -                char num[32];
> +
>                  numvalue = strtoul(val, &err, 0);
>                  if (!*val || *err) {
>                      error_setg(errp, "bad numerical value %s", val);
> @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                      numvalue = min;
>                  }
>                  snprintf(num, sizeof(num), "%" PRId32, numvalue);
> -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> -            } else {
> -                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
> +                val = num;
>              }
>          } else {
>              feat2prop(featurestr);
> -            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
> +            name = featurestr;
> +            val = (char *)"on";

You don't need this hack if you do something like this:


     while (featurestr) {
-        char *val = NULL;
+        const char *val = NULL;
+        char *eq = NULL;
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
         } else if (featurestr[0] == '-') {
             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
+        } else if ((eq = strchr(featurestr, '='))) {
+            *eq++ = 0;
+            val = eq;
             feat2prop(featurestr);
             name = featurestr;
             if (!strcmp(featurestr, "xlevel")) {


>          }
> +
> +        if (val) {
> +            object_property_parse(OBJECT(cpu), val, name, &local_err);
> +        }

I would find it easier to read if this part was inside the same
block as the code that sets name/val.

I would find it even better if object_property_parse() was in the
main loop body, and the +feature/-feature code placed inside an
if/continue branch (to make it clear that it is an exception, not
the rule). Like this:

/* untested */
static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                                     Error **errp)
{
    X86CPU *cpu = X86_CPU(cs);
    char *featurestr; /* Single 'key=value" string being parsed */
    char *err;
    Error *local_err = NULL;

    if (!features) {
        return;
    }

    for (featurestr = strtok(features, ",");
         featurestr;
         featurestr = strtok(NULL, ",")) {
        const char *name;
        const char *val = NULL;
        char *eq = NULL;
        uint32_t numvalue;
        char num[32];

        /* Compatibility syntax: */
        if (featurestr[0] == '+') {
            add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
            continue;
        } else if (featurestr[0] == '-') {
            add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
            continue;
        }

        eq = strchr(featurestr, '=');
        if (eq) {
            *eq++ = 0;
            val = eq;
        } else {
            val = "on";
        }

        feat2prop(featurestr);
        name = featurestr;

        /* Special cases: */
        if (!strcmp(name, "xlevel")) {
            numvalue = strtoul(val, &err, 0);
            if (!*val || *err) {
                error_setg(errp, "bad numerical value %s", val);
                return;
            }
            if (numvalue < 0x80000000) {
                error_report("xlevel value shall always be >= 0x80000000"
                             ", fixup will be removed in future versions");
                numvalue += 0x80000000;
            }
            snprintf(num, sizeof(num), "%" PRIu32, numvalue);
            val = num;
        } else if (!strcmp(name, "tsc-freq")) {
            int64_t tsc_freq;

            tsc_freq = qemu_strtosz_suffix_unit(val, &err,
                                           QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
            if (tsc_freq < 0 || *err) {
                error_setg(errp, "bad numerical value %s", val);
                return;
            }
            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
            val = num;
            name = "tsc-frequency";
        } else if (!strcmp(name, "hv-spinlocks")) {
            const int min = 0xFFF;

            numvalue = strtoul(val, &err, 0);
            if (!*val || *err) {
                error_setg(errp, "bad numerical value %s", val);
                return;
            }
            if (numvalue < min) {
                error_report("hv-spinlocks value shall always be >= 0x%x"
                             ", fixup will be removed in future versions",
                             min);
                numvalue = min;
            }
            snprintf(num, sizeof(num), "%" PRId32, numvalue);
            val = num;
        }

        object_property_parse(OBJECT(cpu), val, name, &local_err);

        if (local_err) {
            error_propagate(errp, local_err);
            return;
        }
    }
}


> +
>          if (local_err) {
>              error_propagate(errp, local_err);
>              return;
> -- 
> 1.8.3.1
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

On Wed, 1 Jun 2016 15:46:20 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 32 +++++++++++++++++---------------
> >  1 file changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 238f69d..618aef9 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >      X86CPU *cpu = X86_CPU(cs);
> >      char *featurestr; /* Single 'key=value" string being parsed */
> >      uint32_t numvalue;
> > +    char num[32];
> > +    const char *name;
> > +    char *err;
> >      Error *local_err = NULL;
> >  
> >      featurestr = features ? strtok(features, ",") : NULL;
> >  
> >      while (featurestr) {
> > -        char *val;
> > +        char *val = NULL;
> >          if (featurestr[0] == '+') {
> >              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> >          } else if (featurestr[0] == '-') {
> > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >          } else if ((val = strchr(featurestr, '='))) {
> >              *val = 0; val++;
> >              feat2prop(featurestr);
> > +            name = featurestr;
> >              if (!strcmp(featurestr, "xlevel")) {
> > -                char *err;
> > -                char num[32];
> > -
> >                  numvalue = strtoul(val, &err, 0);
> >                  if (!*val || *err) {
> >                      error_setg(errp, "bad numerical value %s", val);
> > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                      numvalue += 0x80000000;
> >                  }
> >                  snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> > +                val = num;
> >              } else if (!strcmp(featurestr, "tsc-freq")) {
> >                  int64_t tsc_freq;
> > -                char *err;
> > -                char num[32];
> >  
> >                  tsc_freq = qemu_strtosz_suffix_unit(val, &err,
> >                                                 QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                      return;
> >                  }
> >                  snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > -                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> > -                                      &local_err);
> > +                val = num;
> > +                name = "tsc-frequency";
> >              } else if (!strcmp(featurestr, "hv-spinlocks")) {
> > -                char *err;
> >                  const int min = 0xFFF;
> > -                char num[32];
> > +
> >                  numvalue = strtoul(val, &err, 0);
> >                  if (!*val || *err) {
> >                      error_setg(errp, "bad numerical value %s", val);
> > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                      numvalue = min;
> >                  }
> >                  snprintf(num, sizeof(num), "%" PRId32, numvalue);
> > -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> > -            } else {
> > -                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
> > +                val = num;
> >              }
> >          } else {
> >              feat2prop(featurestr);
> > -            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
> > +            name = featurestr;
> > +            val = (char *)"on";  
> 
> You don't need this hack if you do something like this:
> 
> 
>      while (featurestr) {
> -        char *val = NULL;
> +        const char *val = NULL;
> +        char *eq = NULL;
>          if (featurestr[0] == '+') {
>              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
>          } else if (featurestr[0] == '-') {
>              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> -        } else if ((val = strchr(featurestr, '='))) {
> -            *val = 0; val++;
> +        } else if ((eq = strchr(featurestr, '='))) {
> +            *eq++ = 0;
> +            val = eq;
>              feat2prop(featurestr);
>              name = featurestr;
>              if (!strcmp(featurestr, "xlevel")) {
> 
> 
> >          }
> > +
> > +        if (val) {
> > +            object_property_parse(OBJECT(cpu), val, name, &local_err);
> > +        }  
> 
> I would find it easier to read if this part was inside the same
> block as the code that sets name/val.
> 
> I would find it even better if object_property_parse() was in the
> main loop body, and the +feature/-feature code placed inside an
> if/continue branch (to make it clear that it is an exception, not
> the rule). Like this:

Thanks,
I'll take your variant with your permission.

> /* untested */
> static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
>                                      Error **errp)
> {
>     X86CPU *cpu = X86_CPU(cs);
>     char *featurestr; /* Single 'key=value" string being parsed */
>     char *err;
>     Error *local_err = NULL;
> 
>     if (!features) {
>         return;
>     }
> 
>     for (featurestr = strtok(features, ",");
>          featurestr;
>          featurestr = strtok(NULL, ",")) {
>         const char *name;
>         const char *val = NULL;
>         char *eq = NULL;
>         uint32_t numvalue;
>         char num[32];
> 
>         /* Compatibility syntax: */
>         if (featurestr[0] == '+') {
>             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
>             continue;
>         } else if (featurestr[0] == '-') {
>             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
>             continue;
>         }
> 
>         eq = strchr(featurestr, '=');
>         if (eq) {
>             *eq++ = 0;
>             val = eq;
>         } else {
>             val = "on";
>         }
> 
>         feat2prop(featurestr);
>         name = featurestr;
> 
>         /* Special cases: */
>         if (!strcmp(name, "xlevel")) {
>             numvalue = strtoul(val, &err, 0);
>             if (!*val || *err) {
>                 error_setg(errp, "bad numerical value %s", val);
>                 return;
>             }
>             if (numvalue < 0x80000000) {
>                 error_report("xlevel value shall always be >= 0x80000000"
>                              ", fixup will be removed in future versions");
>                 numvalue += 0x80000000;
>             }
>             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
>             val = num;
>         } else if (!strcmp(name, "tsc-freq")) {
>             int64_t tsc_freq;
> 
>             tsc_freq = qemu_strtosz_suffix_unit(val, &err,
>                                            QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
>             if (tsc_freq < 0 || *err) {
>                 error_setg(errp, "bad numerical value %s", val);
>                 return;
>             }
>             snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
>             val = num;
>             name = "tsc-frequency";
>         } else if (!strcmp(name, "hv-spinlocks")) {
>             const int min = 0xFFF;
> 
>             numvalue = strtoul(val, &err, 0);
>             if (!*val || *err) {
>                 error_setg(errp, "bad numerical value %s", val);
>                 return;
>             }
>             if (numvalue < min) {
>                 error_report("hv-spinlocks value shall always be >= 0x%x"
>                              ", fixup will be removed in future versions",
>                              min);
>                 numvalue = min;
>             }
>             snprintf(num, sizeof(num), "%" PRId32, numvalue);
>             val = num;
>         }
> 
>         object_property_parse(OBJECT(cpu), val, name, &local_err);
> 
>         if (local_err) {
>             error_propagate(errp, local_err);
>             return;
>         }
>     }
> }
> 
> 
> > +
> >          if (local_err) {
> >              error_propagate(errp, local_err);
> >              return;
> > -- 
> > 1.8.3.1
> >   
> 

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> On Wed, 1 Jun 2016 15:46:20 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Wed, Jun 01, 2016 at 06:37:26PM +0200, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  target-i386/cpu.c | 32 +++++++++++++++++---------------
> > >  1 file changed, 17 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 238f69d..618aef9 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1945,12 +1945,15 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >      X86CPU *cpu = X86_CPU(cs);
> > >      char *featurestr; /* Single 'key=value" string being parsed */
> > >      uint32_t numvalue;
> > > +    char num[32];
> > > +    const char *name;
> > > +    char *err;
> > >      Error *local_err = NULL;
> > >  
> > >      featurestr = features ? strtok(features, ",") : NULL;
> > >  
> > >      while (featurestr) {
> > > -        char *val;
> > > +        char *val = NULL;
> > >          if (featurestr[0] == '+') {
> > >              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> > >          } else if (featurestr[0] == '-') {
> > > @@ -1958,10 +1961,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >          } else if ((val = strchr(featurestr, '='))) {
> > >              *val = 0; val++;
> > >              feat2prop(featurestr);
> > > +            name = featurestr;
> > >              if (!strcmp(featurestr, "xlevel")) {
> > > -                char *err;
> > > -                char num[32];
> > > -
> > >                  numvalue = strtoul(val, &err, 0);
> > >                  if (!*val || *err) {
> > >                      error_setg(errp, "bad numerical value %s", val);
> > > @@ -1973,11 +1974,9 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >                      numvalue += 0x80000000;
> > >                  }
> > >                  snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > > -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> > > +                val = num;
> > >              } else if (!strcmp(featurestr, "tsc-freq")) {
> > >                  int64_t tsc_freq;
> > > -                char *err;
> > > -                char num[32];
> > >  
> > >                  tsc_freq = qemu_strtosz_suffix_unit(val, &err,
> > >                                                 QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> > > @@ -1986,12 +1985,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >                      return;
> > >                  }
> > >                  snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > -                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
> > > -                                      &local_err);
> > > +                val = num;
> > > +                name = "tsc-frequency";
> > >              } else if (!strcmp(featurestr, "hv-spinlocks")) {
> > > -                char *err;
> > >                  const int min = 0xFFF;
> > > -                char num[32];
> > > +
> > >                  numvalue = strtoul(val, &err, 0);
> > >                  if (!*val || *err) {
> > >                      error_setg(errp, "bad numerical value %s", val);
> > > @@ -2004,14 +2002,18 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> > >                      numvalue = min;
> > >                  }
> > >                  snprintf(num, sizeof(num), "%" PRId32, numvalue);
> > > -                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
> > > -            } else {
> > > -                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
> > > +                val = num;
> > >              }
> > >          } else {
> > >              feat2prop(featurestr);
> > > -            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
> > > +            name = featurestr;
> > > +            val = (char *)"on";  
> > 
> > You don't need this hack if you do something like this:
> > 
> > 
> >      while (featurestr) {
> > -        char *val = NULL;
> > +        const char *val = NULL;
> > +        char *eq = NULL;
> >          if (featurestr[0] == '+') {
> >              add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> >          } else if (featurestr[0] == '-') {
> >              add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> > -        } else if ((val = strchr(featurestr, '='))) {
> > -            *val = 0; val++;
> > +        } else if ((eq = strchr(featurestr, '='))) {
> > +            *eq++ = 0;
> > +            val = eq;
> >              feat2prop(featurestr);
> >              name = featurestr;
> >              if (!strcmp(featurestr, "xlevel")) {
> > 
> > 
> > >          }
> > > +
> > > +        if (val) {
> > > +            object_property_parse(OBJECT(cpu), val, name, &local_err);
> > > +        }  
> > 
> > I would find it easier to read if this part was inside the same
> > block as the code that sets name/val.
> > 
> > I would find it even better if object_property_parse() was in the
> > main loop body, and the +feature/-feature code placed inside an
> > if/continue branch (to make it clear that it is an exception, not
> > the rule). Like this:
> 
> Thanks,
> I'll take your variant with your permission.

Sure:

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

> 
> > /* untested */
> > static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
> >                                      Error **errp)
> > {
> >     X86CPU *cpu = X86_CPU(cs);
> >     char *featurestr; /* Single 'key=value" string being parsed */
> >     char *err;
> >     Error *local_err = NULL;
> > 
> >     if (!features) {
> >         return;
> >     }
> > 
> >     for (featurestr = strtok(features, ",");
> >          featurestr;
> >          featurestr = strtok(NULL, ",")) {
> >         const char *name;
> >         const char *val = NULL;
> >         char *eq = NULL;
> >         uint32_t numvalue;
> >         char num[32];
> > 
> >         /* Compatibility syntax: */
> >         if (featurestr[0] == '+') {
> >             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
> >             continue;
> >         } else if (featurestr[0] == '-') {
> >             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
> >             continue;
> >         }
> > 
> >         eq = strchr(featurestr, '=');
> >         if (eq) {
> >             *eq++ = 0;
> >             val = eq;
> >         } else {
> >             val = "on";
> >         }
> > 
> >         feat2prop(featurestr);
> >         name = featurestr;
> > 
> >         /* Special cases: */
> >         if (!strcmp(name, "xlevel")) {
> >             numvalue = strtoul(val, &err, 0);
> >             if (!*val || *err) {
> >                 error_setg(errp, "bad numerical value %s", val);
> >                 return;
> >             }
> >             if (numvalue < 0x80000000) {
> >                 error_report("xlevel value shall always be >= 0x80000000"
> >                              ", fixup will be removed in future versions");
> >                 numvalue += 0x80000000;
> >             }
> >             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> >             val = num;
> >         } else if (!strcmp(name, "tsc-freq")) {
> >             int64_t tsc_freq;
> > 
> >             tsc_freq = qemu_strtosz_suffix_unit(val, &err,
> >                                            QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
> >             if (tsc_freq < 0 || *err) {
> >                 error_setg(errp, "bad numerical value %s", val);
> >                 return;
> >             }
> >             snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> >             val = num;
> >             name = "tsc-frequency";
> >         } else if (!strcmp(name, "hv-spinlocks")) {
> >             const int min = 0xFFF;
> > 
> >             numvalue = strtoul(val, &err, 0);
> >             if (!*val || *err) {
> >                 error_setg(errp, "bad numerical value %s", val);
> >                 return;
> >             }
> >             if (numvalue < min) {
> >                 error_report("hv-spinlocks value shall always be >= 0x%x"
> >                              ", fixup will be removed in future versions",
> >                              min);
> >                 numvalue = min;
> >             }
> >             snprintf(num, sizeof(num), "%" PRId32, numvalue);
> >             val = num;
> >         }
> > 
> >         object_property_parse(OBJECT(cpu), val, name, &local_err);
> > 
> >         if (local_err) {
> >             error_propagate(errp, local_err);
> >             return;
> >         }
> >     }
> > }
> > 
> > 
> > > +
> > >          if (local_err) {
> > >              error_propagate(errp, local_err);
> > >              return;
> > > -- 
> > > 1.8.3.1
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

(CCing libvirt folks)

BTW:

On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
[...]
> >         /* Special cases: */
> >         if (!strcmp(name, "xlevel")) {
> >             numvalue = strtoul(val, &err, 0);
> >             if (!*val || *err) {
> >                 error_setg(errp, "bad numerical value %s", val);
> >                 return;
> >             }
> >             if (numvalue < 0x80000000) {
> >                 error_report("xlevel value shall always be >= 0x80000000"
> >                              ", fixup will be removed in future versions");
> >                 numvalue += 0x80000000;
> >             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> >             val = num;
[...]
> >         } else if (!strcmp(name, "hv-spinlocks")) {
> >             const int min = 0xFFF;
> > 
> >             numvalue = strtoul(val, &err, 0);
> >             if (!*val || *err) {
> >                 error_setg(errp, "bad numerical value %s", val);
> >                 return;
> >             }
> >             if (numvalue < min) {
> >                 error_report("hv-spinlocks value shall always be >= 0x%x"
> >                              ", fixup will be removed in future versions",
> >                              min);
> >                 numvalue = min;
> >             }

Those "fixup will be removed in future versions" warnings are
present since QEMU 1.7. Assuming that libvirt never allowed those
invalid values to be used in the configuration (did it?), I
believe we can safely remove the hv-spinlocks and xlevel fixups
in QEMU 2.7.

The hv-spinlocks setter already rejects invalid values. We just
need to make x86_cpu_realizefn() reject invalid xlevel values.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Peter Krempa]

On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:
> (CCing libvirt folks)
> 
> BTW:
> 
> On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> [...]
> > >         /* Special cases: */
> > >         if (!strcmp(name, "xlevel")) {
> > >             numvalue = strtoul(val, &err, 0);
> > >             if (!*val || *err) {
> > >                 error_setg(errp, "bad numerical value %s", val);
> > >                 return;
> > >             }
> > >             if (numvalue < 0x80000000) {
> > >                 error_report("xlevel value shall always be >= 0x80000000"
> > >                              ", fixup will be removed in future versions");
> > >                 numvalue += 0x80000000;
> > >             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > >             val = num;
> [...]
> > >         } else if (!strcmp(name, "hv-spinlocks")) {
> > >             const int min = 0xFFF;
> > > 
> > >             numvalue = strtoul(val, &err, 0);
> > >             if (!*val || *err) {
> > >                 error_setg(errp, "bad numerical value %s", val);
> > >                 return;
> > >             }
> > >             if (numvalue < min) {
> > >                 error_report("hv-spinlocks value shall always be >= 0x%x"
> > >                              ", fixup will be removed in future versions",
> > >                              min);
> > >                 numvalue = min;
> > >             }
> 
> Those "fixup will be removed in future versions" warnings are
> present since QEMU 1.7. Assuming that libvirt never allowed those
> invalid values to be used in the configuration (did it?), I
> believe we can safely remove the hv-spinlocks and xlevel fixups
> in QEMU 2.7.

I couldn't find anything regarding xlevel (so we might actually not
support it at all), but we indeed do limit the hv_spinlock count:


                if (def->hyperv_spinlocks < 0xFFF) {
                    virReportError(VIR_ERR_XML_ERROR, "%s",
                                   _("HyperV spinlock retry count must be "
                                     "at least 4095"));
                    goto error;
                }

Peter

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

On Thu, 2 Jun 2016 17:05:06 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:
> > (CCing libvirt folks)
> > 
> > BTW:
> > 
> > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> > [...]  
> > > >         /* Special cases: */
> > > >         if (!strcmp(name, "xlevel")) {
> > > >             numvalue = strtoul(val, &err, 0);
> > > >             if (!*val || *err) {
> > > >                 error_setg(errp, "bad numerical value %s", val);
> > > >                 return;
> > > >             }
> > > >             if (numvalue < 0x80000000) {
> > > >                 error_report("xlevel value shall always be >= 0x80000000"
> > > >                              ", fixup will be removed in future versions");
> > > >                 numvalue += 0x80000000;
> > > >             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > > >             val = num;  
> > [...]  
> > > >         } else if (!strcmp(name, "hv-spinlocks")) {
> > > >             const int min = 0xFFF;
> > > > 
> > > >             numvalue = strtoul(val, &err, 0);
> > > >             if (!*val || *err) {
> > > >                 error_setg(errp, "bad numerical value %s", val);
> > > >                 return;
> > > >             }
> > > >             if (numvalue < min) {
> > > >                 error_report("hv-spinlocks value shall always be >= 0x%x"
> > > >                              ", fixup will be removed in future versions",
> > > >                              min);
> > > >                 numvalue = min;
> > > >             }  
> > 
> > Those "fixup will be removed in future versions" warnings are
> > present since QEMU 1.7. Assuming that libvirt never allowed those
> > invalid values to be used in the configuration (did it?), I
> > believe we can safely remove the hv-spinlocks and xlevel fixups
> > in QEMU 2.7.  
> 
> I couldn't find anything regarding xlevel (so we might actually not
> support it at all), but we indeed do limit the hv_spinlock count:
> 
> 
>                 if (def->hyperv_spinlocks < 0xFFF) {
>                     virReportError(VIR_ERR_XML_ERROR, "%s",
>                                    _("HyperV spinlock retry count must be "
>                                      "at least 4095"));
>                     goto error;
>                 }
> 
> Peter
Peter,
Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax
or canonical property syntax there feat1=on,feat2=off

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Peter Krempa]

On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 17:05:06 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:
> > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:

[...]

> > I couldn't find anything regarding xlevel (so we might actually not
> > support it at all), but we indeed do limit the hv_spinlock count:
> > 
> > 
> >                 if (def->hyperv_spinlocks < 0xFFF) {
> >                     virReportError(VIR_ERR_XML_ERROR, "%s",
> >                                    _("HyperV spinlock retry count must be "
> >                                      "at least 4095"));
> >                     goto error;
> >                 }
> > 
> > Peter
> Peter,
> Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax
> or canonical property syntax there feat1=on,feat2=off

We use the legacy one:

-cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ...

and

-cpu 'qemu32,hv_relaxed,hv_vapic, ... 

for the hyperv features.

We probably can switch to the new one if there's a reasonable way how to
detect that qemu is supporting the new one.

Peter

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

On Fri, 3 Jun 2016 09:30:09 +0200
Peter Krempa <pkrempa@redhat.com> wrote:

> On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote:
> > On Thu, 2 Jun 2016 17:05:06 +0200
> > Peter Krempa <pkrempa@redhat.com> wrote:  
> > > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:  
> > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:  
> 
> [...]
> 
> > > I couldn't find anything regarding xlevel (so we might actually not
> > > support it at all), but we indeed do limit the hv_spinlock count:
> > > 
> > > 
> > >                 if (def->hyperv_spinlocks < 0xFFF) {
> > >                     virReportError(VIR_ERR_XML_ERROR, "%s",
> > >                                    _("HyperV spinlock retry count must be "
> > >                                      "at least 4095"));
> > >                     goto error;
> > >                 }
> > > 
> > > Peter  
> > Peter,
> > Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax
> > or canonical property syntax there feat1=on,feat2=off  
> 
> We use the legacy one:
> 
> -cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ...
> 
> and
> 
> -cpu 'qemu32,hv_relaxed,hv_vapic, ... 
> 
> for the hyperv features.
> 
> We probably can switch to the new one if there's a reasonable way how to
> detect that qemu is supporting the new one.
for x86 features became properties since 2.4 release (commit 38e5c119),
that's the one way to know it.
But it's still only +-features for sparc (that's the last remaining
target that has legacy parsing).

Another way to detect it is to probe via QOM if CPU has a property corresponding
to a feature.

Maybe Eduardo knows about other ways to do it.

> 
> Peter
> 

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Eduardo Habkost]

On Fri, Jun 03, 2016 at 11:37:49AM +0200, Igor Mammedov wrote:
> On Fri, 3 Jun 2016 09:30:09 +0200
> Peter Krempa <pkrempa@redhat.com> wrote:
> 
> > On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote:
> > > On Thu, 2 Jun 2016 17:05:06 +0200
> > > Peter Krempa <pkrempa@redhat.com> wrote:  
> > > > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:  
> > > > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:  
> > 
> > [...]
> > 
> > > > I couldn't find anything regarding xlevel (so we might actually not
> > > > support it at all), but we indeed do limit the hv_spinlock count:
> > > > 
> > > > 
> > > >                 if (def->hyperv_spinlocks < 0xFFF) {
> > > >                     virReportError(VIR_ERR_XML_ERROR, "%s",
> > > >                                    _("HyperV spinlock retry count must be "
> > > >                                      "at least 4095"));
> > > >                     goto error;
> > > >                 }
> > > > 
> > > > Peter  
> > > Peter,
> > > Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax
> > > or canonical property syntax there feat1=on,feat2=off  
> > 
> > We use the legacy one:
> > 
> > -cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ...
> > 
> > and
> > 
> > -cpu 'qemu32,hv_relaxed,hv_vapic, ... 
> > 
> > for the hyperv features.
> > 
> > We probably can switch to the new one if there's a reasonable way how to
> > detect that qemu is supporting the new one.
> for x86 features became properties since 2.4 release (commit 38e5c119),
> that's the one way to know it.
> But it's still only +-features for sparc (that's the last remaining
> target that has legacy parsing).
> 
> Another way to detect it is to probe via QOM if CPU has a property corresponding
> to a feature.
> 
> Maybe Eduardo knows about other ways to do it.

The properties could be detected by running
device-list-properties when libvirt is probing for QEMU binary
capabilities. But we still don't return any useful information
for abstract classes or X86CPU subclasses.

You could work around it by running qom-list on the first VCPU
object, but when libvirt probes QEMU binary capabilities, it uses
-machine none (which doesn't create any VCPU).

I will look for other mechanisms that can be used in QEMU
2.{4,5,6} already, but I am not sure we will find one.

If all fails, we can consider making query-command-line-options
return useful data for -cpu on QEMU 2.7.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Igor Mammedov]

On Thu, 2 Jun 2016 11:53:22 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> (CCing libvirt folks)
> 
> BTW:
> 
> On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> [...]
> > >         /* Special cases: */
> > >         if (!strcmp(name, "xlevel")) {
> > >             numvalue = strtoul(val, &err, 0);
> > >             if (!*val || *err) {
> > >                 error_setg(errp, "bad numerical value %s", val);
> > >                 return;
> > >             }
> > >             if (numvalue < 0x80000000) {
> > >                 error_report("xlevel value shall always be >= 0x80000000"
> > >                              ", fixup will be removed in future versions");
> > >                 numvalue += 0x80000000;
> > >             snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > >             val = num;  
> [...]
> > >         } else if (!strcmp(name, "hv-spinlocks")) {
> > >             const int min = 0xFFF;
> > > 
> > >             numvalue = strtoul(val, &err, 0);
> > >             if (!*val || *err) {
> > >                 error_setg(errp, "bad numerical value %s", val);
> > >                 return;
> > >             }
> > >             if (numvalue < min) {
> > >                 error_report("hv-spinlocks value shall always be >= 0x%x"
> > >                              ", fixup will be removed in future versions",
> > >                              min);
> > >                 numvalue = min;
> > >             }  
> 
> Those "fixup will be removed in future versions" warnings are
> present since QEMU 1.7. Assuming that libvirt never allowed those
> invalid values to be used in the configuration (did it?), I
> believe we can safely remove the hv-spinlocks and xlevel fixups
> in QEMU 2.7.
> 
> The hv-spinlocks setter already rejects invalid values. We just
> need to make x86_cpu_realizefn() reject invalid xlevel values.

I'll leave axing them to you.

[Qemu-devel] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups

[Eduardo Habkost]

The "fixup will be removed in future versions" warnings are
present since QEMU 1.7.0, at least, so users should have fixed
their scripts and configurations, already.

In the case of libvirt users, libvirt doesn't use the "xlevel"
option, and already rejects HyperV spinlock retry count < 0xFFF.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Igor, feel free to include this in the beginning of your series.
I believe it will help making the patches simpler.
---
 target-i386/cpu.c | 35 +----------------------------------
 1 file changed, 1 insertion(+), 34 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index f3f95cd..940aa22 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1967,23 +1967,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         } else if ((val = strchr(featurestr, '='))) {
             *val = 0; val++;
             feat2prop(featurestr);
-            if (!strcmp(featurestr, "xlevel")) {
-                char *err;
-                char num[32];
-
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                if (numvalue < 0x80000000) {
-                    error_report("xlevel value shall always be >= 0x80000000"
-                                 ", fixup will be removed in future versions");
-                    numvalue += 0x80000000;
-                }
-                snprintf(num, sizeof(num), "%" PRIu32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
-            } else if (!strcmp(featurestr, "tsc-freq")) {
+            if (!strcmp(featurestr, "tsc-freq")) {
                 int64_t tsc_freq;
                 char *err;
                 char num[32];
@@ -1997,23 +1981,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
                 snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
                 object_property_parse(OBJECT(cpu), num, "tsc-frequency",
                                       &local_err);
-            } else if (!strcmp(featurestr, "hv-spinlocks")) {
-                char *err;
-                const int min = 0xFFF;
-                char num[32];
-                numvalue = strtoul(val, &err, 0);
-                if (!*val || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                if (numvalue < min) {
-                    error_report("hv-spinlocks value shall always be >= 0x%x"
-                                 ", fixup will be removed in future versions",
-                                 min);
-                    numvalue = min;
-                }
-                snprintf(num, sizeof(num), "%" PRId32, numvalue);
-                object_property_parse(OBJECT(cpu), num, featurestr, &local_err);
             } else {
                 object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
             }
-- 
2.5.5

Re: [Qemu-devel] [libvirt] [PATCH] target-i386: Remove xlevel & hv-spinlocks option fixups

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 834 bytes --]

On 06/02/2016 10:55 AM, Eduardo Habkost wrote:
> The "fixup will be removed in future versions" warnings are
> present since QEMU 1.7.0, at least, so users should have fixed
> their scripts and configurations, already.
> 
> In the case of libvirt users, libvirt doesn't use the "xlevel"
> option, and already rejects HyperV spinlock retry count < 0xFFF.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Igor, feel free to include this in the beginning of your series.
> I believe it will help making the patches simpler.
> ---
>  target-i386/cpu.c | 35 +----------------------------------
>  1 file changed, 1 insertion(+), 34 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH RFC 5/8] target-sparc: cpu: use sparc_cpu_parse_features() directly

[Igor Mammedov]

make SPARC target use sparc_cpu_parse_features() directly
so it won't get in the way of switching other propertified
targets to handling features as global properties.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
SPARC target could be switched to features properties
later but that would need quite a bit of refactoring
in generating necessary CPU types and adding appropriate
properties.
---
 target-sparc/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-sparc/cpu.c b/target-sparc/cpu.c
index 5b74cfc..e4089f2 100644
--- a/target-sparc/cpu.c
+++ b/target-sparc/cpu.c
@@ -101,9 +101,11 @@ static void cpu_sparc_disas_set_info(CPUState *cpu, disassemble_info *info)
 #endif
 }
 
+static void sparc_cpu_parse_features(CPUState *cs, char *features,
+                                     Error **errp);
+
 static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
 {
-    CPUClass *cc = CPU_GET_CLASS(cpu);
     CPUSPARCState *env = &cpu->env;
     char *s = g_strdup(cpu_model);
     char *featurestr, *name = strtok(s, ",");
@@ -119,7 +121,7 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char *cpu_model)
     memcpy(env->def, def, sizeof(*def));
 
     featurestr = strtok(NULL, ",");
-    cc->parse_features(CPU(cpu), featurestr, &err);
+    sparc_cpu_parse_features(CPU(cpu), featurestr, &err);
     g_free(s);
     if (err) {
         error_report_err(err);
@@ -840,7 +842,6 @@ static void sparc_cpu_class_init(ObjectClass *oc, void *data)
     scc->parent_reset = cc->reset;
     cc->reset = sparc_cpu_reset;
 
-    cc->parse_features = sparc_cpu_parse_features;
     cc->has_work = sparc_cpu_has_work;
     cc->do_interrupt = sparc_cpu_do_interrupt;
     cc->cpu_exec_interrupt = sparc_cpu_exec_interrupt;
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 6/8] cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

Currently CPUClass->parse_features() is used to parse
-cpu features string and set properties on created CPU
instances.

But considering that features specified -cpu apply to
every created CPU instance, it doesn't make sence to
parse the same features string for every CPU created.
It also makes every target that cares about parsing
features string explicitly call CPUClass->parse_features()
parser, which gets in a way if we consider using
generic device_add for CPU hotplug as device_add
has not a clue about CPU specific hooks.

Turns out we can use global properties mechanism to set
properties on every created Device instance for a given
type. That way it's possible to convert CPU features
into a set of global properties for specified by
 -cpu cpu_model and common Device.device_post_init()
will apply them to every cpu model model automaticaaly
regardless whether it's manually created CPU or CPU
created with help of device_add.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
This patch only make CPUClass->parse_features()
a global properties convertor and follow up patches
will do cleanups that new behaviour allows.
---
 hw/arm/virt.c     |  7 ++++---
 include/qom/cpu.h |  2 +-
 qom/cpu.c         | 29 +++++++++++++++++------------
 target-i386/cpu.c | 25 +++++++++++++++++++------
 4 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index e77ed88..473e439 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1193,6 +1193,7 @@ static void machvirt_init(MachineState *machine)
 
     for (n = 0; n < smp_cpus; n++) {
         ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
+        const char *typename = object_class_get_name(oc);
         CPUClass *cc = CPU_CLASS(oc);
         Object *cpuobj;
         Error *err = NULL;
@@ -1202,10 +1203,10 @@ static void machvirt_init(MachineState *machine)
             error_report("Unable to find CPU definition");
             exit(1);
         }
-        cpuobj = object_new(object_class_get_name(oc));
+        /* convert -smp CPU options specified by the user into global props */
+        cc->parse_features(typename, cpuopts, &err);
+        cpuobj = object_new(typename);
 
-        /* Handle any CPU options specified by the user */
-        cc->parse_features(CPU(cpuobj), cpuopts, &err);
         g_free(cpuopts);
         if (err) {
             error_report_err(err);
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 32f3af3..cacb100 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -134,7 +134,7 @@ typedef struct CPUClass {
     /*< public >*/
 
     ObjectClass *(*class_by_name)(const char *cpu_model);
-    void (*parse_features)(CPUState *cpu, char *str, Error **errp);
+    void (*parse_features)(const char *typename, char *str, Error **errp);
 
     void (*reset)(CPUState *cpu);
     int reset_dump_flags;
diff --git a/qom/cpu.c b/qom/cpu.c
index 751e992..f3e3c02 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -28,6 +28,7 @@
 #include "exec/log.h"
 #include "qemu/error-report.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -46,7 +47,7 @@ bool cpu_exists(int64_t id)
 CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
 {
     char *str, *name, *featurestr;
-    CPUState *cpu;
+    CPUState *cpu = NULL;
     ObjectClass *oc;
     CPUClass *cc;
     Error *err = NULL;
@@ -60,16 +61,15 @@ CPUState *cpu_generic_init(const char *typename, const char *cpu_model)
         return NULL;
     }
 
-    cpu = CPU(object_new(object_class_get_name(oc)));
-    cc = CPU_GET_CLASS(cpu);
-
+    cc = CPU_CLASS(oc);
     featurestr = strtok(NULL, ",");
-    cc->parse_features(cpu, featurestr, &err);
+    cc->parse_features(object_class_get_name(oc), featurestr, &err);
     g_free(str);
     if (err != NULL) {
         goto out;
     }
 
+    cpu = CPU(object_new(object_class_get_name(oc)));
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
 out:
@@ -282,25 +282,29 @@ static ObjectClass *cpu_common_class_by_name(const char *cpu_model)
     return NULL;
 }
 
-static void cpu_common_parse_features(CPUState *cpu, char *features,
+static void cpu_common_parse_features(const char *typename, char *features,
                                       Error **errp)
 {
     char *featurestr; /* Single "key=value" string being parsed */
     char *val;
-    Error *err = NULL;
+    static bool cpu_globals_initialized;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
 
     featurestr = features ? strtok(features, ",") : NULL;
 
     while (featurestr) {
         val = strchr(featurestr, '=');
         if (val) {
+            GlobalProperty *prop = g_new0(typeof(*prop), 1);
             *val = 0;
             val++;
-            object_property_parse(OBJECT(cpu), val, featurestr, &err);
-            if (err) {
-                error_propagate(errp, err);
-                return;
-            }
+            prop->driver = typename;
+            prop->property = g_strdup(featurestr);
+            prop->value = g_strdup(val);
+            qdev_prop_register_global(prop);
         } else {
             error_setg(errp, "Expected key=value format, found %s.",
                        featurestr);
@@ -308,6 +312,7 @@ static void cpu_common_parse_features(CPUState *cpu, char *features,
         }
         featurestr = strtok(NULL, ",");
     }
+    cpu_globals_initialized = true;
 }
 
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 618aef9..871175a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1939,16 +1939,20 @@ static FeatureWordArray minus_features = { 0 };
 
 /* Parse "+feature,-feature,feature=foo" CPU feature string
  */
-static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
+static void x86_cpu_parse_featurestr(const char *typename, char *features,
                                      Error **errp)
 {
-    X86CPU *cpu = X86_CPU(cs);
     char *featurestr; /* Single 'key=value" string being parsed */
     uint32_t numvalue;
     char num[32];
     const char *name;
     char *err;
     Error *local_err = NULL;
+    static bool cpu_globals_initialized;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
 
     featurestr = features ? strtok(features, ",") : NULL;
 
@@ -2011,7 +2015,11 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         }
 
         if (val) {
-            object_property_parse(OBJECT(cpu), val, name, &local_err);
+            GlobalProperty *prop = g_new0(typeof(*prop), 1);
+            prop->driver = typename;
+            prop->property = g_strdup(name);
+            prop->value = g_strdup(val);
+            qdev_prop_register_global(prop);
         }
 
         if (local_err) {
@@ -2020,6 +2028,8 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
         }
         featurestr = strtok(NULL, ",");
     }
+
+    cpu_globals_initialized = true;
 }
 
 /* Print all cpuid feature names in featureset
@@ -2203,9 +2213,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
 {
     X86CPU *cpu = NULL;
     ObjectClass *oc;
+    CPUClass *cc;
     gchar **model_pieces;
     char *name, *features;
     Error *error = NULL;
+    const char *typename;
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
@@ -2220,10 +2232,11 @@ X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
         error_setg(&error, "Unable to find CPU definition: %s", name);
         goto out;
     }
+    cc = CPU_CLASS(oc);
+    typename = object_class_get_name(oc);
 
-    cpu = X86_CPU(object_new(object_class_get_name(oc)));
-
-    x86_cpu_parse_featurestr(CPU(cpu), features, &error);
+    cc->parse_features(typename, features, &error);
+    cpu = X86_CPU(object_new(typename));
     if (error) {
         goto out;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 6/8] cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 06:37:28PM +0200, Igor Mammedov wrote:
> Currently CPUClass->parse_features() is used to parse
> -cpu features string and set properties on created CPU
> instances.
> 
> But considering that features specified -cpu apply to
> every created CPU instance, it doesn't make sence to
> parse the same features string for every CPU created.
> It also makes every target that cares about parsing
> features string explicitly call CPUClass->parse_features()
> parser, which gets in a way if we consider using
> generic device_add for CPU hotplug as device_add
> has not a clue about CPU specific hooks.
> 
> Turns out we can use global properties mechanism to set
> properties on every created Device instance for a given
> type. That way it's possible to convert CPU features
> into a set of global properties for specified by
>  -cpu cpu_model and common Device.device_post_init()
> will apply them to every cpu model model automaticaaly
> regardless whether it's manually created CPU or CPU
> created with help of device_add.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
[...]
> -static void cpu_common_parse_features(CPUState *cpu, char *features,
> +static void cpu_common_parse_features(const char *typename, char *features,
>                                        Error **errp)
>  {
>      char *featurestr; /* Single "key=value" string being parsed */
>      char *val;
> -    Error *err = NULL;
> +    static bool cpu_globals_initialized;
> +
> +    if (cpu_globals_initialized) {
> +        return;
> +    }

Should we replace this with assert(!cpu_globals_initialized)
after applying patch 8/8?

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 6/8] cpu: use CPUClass->parse_features() as convertor to global properties

[Igor Mammedov]

On Wed, 1 Jun 2016 15:54:50 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 01, 2016 at 06:37:28PM +0200, Igor Mammedov wrote:
> > Currently CPUClass->parse_features() is used to parse
> > -cpu features string and set properties on created CPU
> > instances.
> > 
> > But considering that features specified -cpu apply to
> > every created CPU instance, it doesn't make sence to
> > parse the same features string for every CPU created.
> > It also makes every target that cares about parsing
> > features string explicitly call CPUClass->parse_features()
> > parser, which gets in a way if we consider using
> > generic device_add for CPU hotplug as device_add
> > has not a clue about CPU specific hooks.
> > 
> > Turns out we can use global properties mechanism to set
> > properties on every created Device instance for a given
> > type. That way it's possible to convert CPU features
> > into a set of global properties for specified by
> >  -cpu cpu_model and common Device.device_post_init()
> > will apply them to every cpu model model automaticaaly
> > regardless whether it's manually created CPU or CPU
> > created with help of device_add.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> [...]
> > -static void cpu_common_parse_features(CPUState *cpu, char
> > *features, +static void cpu_common_parse_features(const char
> > *typename, char *features, Error **errp)
> >  {
> >      char *featurestr; /* Single "key=value" string being parsed */
> >      char *val;
> > -    Error *err = NULL;
> > +    static bool cpu_globals_initialized;
> > +
> > +    if (cpu_globals_initialized) {
> > +        return;
> > +    }
> 
> Should we replace this with assert(!cpu_globals_initialized)
> after applying patch 8/8?
assert might potentially break cpu_init() users, so I went safest route
here.
 

Re: [Qemu-devel] [PATCH RFC 6/8] cpu: use CPUClass->parse_features() as convertor to global properties

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 12:06:59PM +0200, Igor Mammedov wrote:
> On Wed, 1 Jun 2016 15:54:50 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > -static void cpu_common_parse_features(CPUState *cpu, char
> > > *features, +static void cpu_common_parse_features(const char
> > > *typename, char *features, Error **errp)
> > >  {
> > >      char *featurestr; /* Single "key=value" string being parsed */
> > >      char *val;
> > > -    Error *err = NULL;
> > > +    static bool cpu_globals_initialized;
> > > +
> > > +    if (cpu_globals_initialized) {
> > > +        return;
> > > +    }
> > 
> > Should we replace this with assert(!cpu_globals_initialized)
> > after applying patch 8/8?
> assert might potentially break cpu_init() users, so I went safest route
> here.

No problem: we can add an assert() after fixing the cpu_init()
users, if necessary (we have only two of them).

-- 
Eduardo

[Qemu-devel] [PATCH RFC 7/8] arm: virt: parse cpu_model only once

[Igor Mammedov]

considering that features are converted to
global properties and global properties are
automatically applied to every new instance
of created CPU (at object_new() time), there
is no point in parsing cpu_model string every
time a CPU created.
So move parsing outside CPU creation loop and
do it only once.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/arm/virt.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 473e439..0dbee47 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1112,6 +1112,10 @@ static void machvirt_init(MachineState *machine)
     VirtGuestInfoState *guest_info_state = g_malloc0(sizeof *guest_info_state);
     VirtGuestInfo *guest_info = &guest_info_state->info;
     char **cpustr;
+    ObjectClass *oc;
+    const char *typename;
+    CPUClass *cc;
+    Error *err = NULL;
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
 
     if (!cpu_model) {
@@ -1191,27 +1195,24 @@ static void machvirt_init(MachineState *machine)
 
     create_fdt(vbi);
 
-    for (n = 0; n < smp_cpus; n++) {
-        ObjectClass *oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
-        const char *typename = object_class_get_name(oc);
-        CPUClass *cc = CPU_CLASS(oc);
-        Object *cpuobj;
-        Error *err = NULL;
-        char *cpuopts = g_strdup(cpustr[1]);
-
-        if (!oc) {
-            error_report("Unable to find CPU definition");
-            exit(1);
-        }
-        /* convert -smp CPU options specified by the user into global props */
-        cc->parse_features(typename, cpuopts, &err);
-        cpuobj = object_new(typename);
+    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
+    if (!oc) {
+        error_report("Unable to find CPU definition");
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
 
-        g_free(cpuopts);
-        if (err) {
-            error_report_err(err);
-            exit(1);
-        }
+    /* convert -smp CPU options specified by the user into global props */
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, cpustr[1], &err);
+    g_strfreev(cpustr);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
+
+    for (n = 0; n < smp_cpus; n++) {
+        Object *cpuobj = object_new(typename);
 
         if (!vms->secure) {
             object_property_set_bool(cpuobj, false, "has_el3", NULL);
@@ -1242,7 +1243,6 @@ static void machvirt_init(MachineState *machine)
 
         object_property_set_bool(cpuobj, true, "realized", NULL);
     }
-    g_strfreev(cpustr);
     fdt_add_timer_nodes(vbi, gic_version);
     fdt_add_cpu_nodes(vbi);
     fdt_add_psci_node(vbi);
-- 
1.8.3.1

[Qemu-devel] [PATCH RFC 8/8] pc: parse cpu features only once

[Igor Mammedov]

considering that features are converted to
global properties and global properties are
automatically applied to every new instance
of created CPU (at object_new() time), there
is no point in parsing cpu_model string every
time a CPU created.
So move parsing outside CPU creation loop and
do it only once.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/pc.c      | 37 ++++++++++++++++++++++++++++---------
 target-i386/cpu.c | 44 --------------------------------------------
 target-i386/cpu.h |  1 -
 3 files changed, 28 insertions(+), 54 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c48322b..0331e6d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1041,21 +1041,17 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int level)
     }
 }
 
-static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
+static X86CPU *pc_new_cpu(const char *typename, int64_t apic_id,
                           Error **errp)
 {
     X86CPU *cpu = NULL;
     Error *local_err = NULL;
 
-    cpu = cpu_x86_create(cpu_model, &local_err);
-    if (local_err != NULL) {
-        goto out;
-    }
+    cpu = X86_CPU(object_new(typename));
 
     object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
 
-out:
     if (local_err) {
         error_propagate(errp, local_err);
         object_unref(OBJECT(cpu));
@@ -1067,7 +1063,8 @@ out:
 void pc_hot_add_cpu(const int64_t id, Error **errp)
 {
     X86CPU *cpu;
-    MachineState *machine = MACHINE(qdev_get_machine());
+    ObjectClass *oc;
+    PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
     int64_t apic_id = x86_cpu_apic_id_from_index(id);
     Error *local_err = NULL;
 
@@ -1095,7 +1092,9 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
         return;
     }
 
-    cpu = pc_new_cpu(machine->cpu_model, apic_id, &local_err);
+    assert(pcms->possible_cpus->cpus[0].cpu); /* BSP is always present */
+    oc = OBJECT_CLASS(CPU_GET_CLASS(pcms->possible_cpus->cpus[0].cpu));
+    cpu = pc_new_cpu(object_class_get_name(oc), apic_id, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -1106,6 +1105,10 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 void pc_cpus_init(PCMachineState *pcms)
 {
     int i, j;
+    CPUClass *cc;
+    ObjectClass *oc;
+    const char *typename;
+    gchar **model_pieces;
     X86CPU *cpu = NULL;
     MachineState *machine = MACHINE(pcms);
 
@@ -1118,6 +1121,22 @@ void pc_cpus_init(PCMachineState *pcms)
 #endif
     }
 
+    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+
     /* Calculates the limit to CPU APIC ID values
      *
      * Limit for the APIC ID value, so that all
@@ -1148,7 +1167,7 @@ void pc_cpus_init(PCMachineState *pcms)
         }
 
         if (i < smp_cpus) {
-            cpu = pc_new_cpu(machine->cpu_model, x86_cpu_apic_id_from_index(i),
+            cpu = pc_new_cpu(typename, x86_cpu_apic_id_from_index(i),
                              &error_fatal);
             pcms->possible_cpus->cpus[i].cpu = CPU(cpu);
             object_unref(OBJECT(cpu));
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 871175a..c4f2e2c 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -2209,50 +2209,6 @@ static void x86_cpu_load_def(X86CPU *cpu, X86CPUDefinition *def, Error **errp)
 
 }
 
-X86CPU *cpu_x86_create(const char *cpu_model, Error **errp)
-{
-    X86CPU *cpu = NULL;
-    ObjectClass *oc;
-    CPUClass *cc;
-    gchar **model_pieces;
-    char *name, *features;
-    Error *error = NULL;
-    const char *typename;
-
-    model_pieces = g_strsplit(cpu_model, ",", 2);
-    if (!model_pieces[0]) {
-        error_setg(&error, "Invalid/empty CPU model name");
-        goto out;
-    }
-    name = model_pieces[0];
-    features = model_pieces[1];
-
-    oc = x86_cpu_class_by_name(name);
-    if (oc == NULL) {
-        error_setg(&error, "Unable to find CPU definition: %s", name);
-        goto out;
-    }
-    cc = CPU_CLASS(oc);
-    typename = object_class_get_name(oc);
-
-    cc->parse_features(typename, features, &error);
-    cpu = X86_CPU(object_new(typename));
-    if (error) {
-        goto out;
-    }
-
-out:
-    if (error != NULL) {
-        error_propagate(errp, error);
-        if (cpu) {
-            object_unref(OBJECT(cpu));
-            cpu = NULL;
-        }
-    }
-    g_strfreev(model_pieces);
-    return cpu;
-}
-
 X86CPU *cpu_x86_init(const char *cpu_model)
 {
     return X86_CPU(cpu_generic_init(TYPE_X86_CPU, cpu_model));
diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index a257c53..a9a3b87 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -1227,7 +1227,6 @@ void x86_cpu_exec_enter(CPUState *cpu);
 void x86_cpu_exec_exit(CPUState *cpu);
 
 X86CPU *cpu_x86_init(const char *cpu_model);
-X86CPU *cpu_x86_create(const char *cpu_model, Error **errp);
 int cpu_x86_exec(CPUState *cpu);
 void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf);
 int cpu_x86_support_mca_broadcast(CPUX86State *env);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Peter Maydell]

On 1 June 2016 at 17:37, Igor Mammedov <imammedo@redhat.com> wrote:
> Current CLI option -cpu cpux,features serves as template
> for all created cpus of type: cpux. However QEMU parses
> "features" every time it creates a cpu instance and applies
> them to it while doing parsing.
>
> That doesn't work well with -device/device_add infrastructure
> as it has no idea about cpu specific hooks that's used for
> parsing "features".
> In order to make -device/device_add utilize "-cpu features"
> template convert it into a set of global properties, so that
> every new CPU created will have them applied automatically.

Is there a plan for supporting heterogenous CPU setups?
(We don't really do those very well at the moment, but
we might in future. The closest we come today is the
zynqmp SoC which has Cortex-R5s and A53s in it.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Wed, Jun 01, 2016 at 07:21:00PM +0100, Peter Maydell wrote:
> On 1 June 2016 at 17:37, Igor Mammedov <imammedo@redhat.com> wrote:
> > Current CLI option -cpu cpux,features serves as template
> > for all created cpus of type: cpux. However QEMU parses
> > "features" every time it creates a cpu instance and applies
> > them to it while doing parsing.
> >
> > That doesn't work well with -device/device_add infrastructure
> > as it has no idea about cpu specific hooks that's used for
> > parsing "features".
> > In order to make -device/device_add utilize "-cpu features"
> > template convert it into a set of global properties, so that
> > every new CPU created will have them applied automatically.
> 
> Is there a plan for supporting heterogenous CPU setups?
> (We don't really do those very well at the moment, but
> we might in future. The closest we come today is the
> zynqmp SoC which has Cortex-R5s and A53s in it.)

I am not aware of any specific plan, but this series gets us
closer to eventually replacing -cpu (which doesn't allow
heterogenous CPU setups) with simple -device/-global combinations
(which would be more flexible).

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> Current CLI option -cpu cpux,features serves as template
> for all created cpus of type: cpux. However QEMU parses
> "features" every time it creates a cpu instance and applies
> them to it while doing parsing.
> 
> That doesn't work well with -device/device_add infrastructure
> as it has no idea about cpu specific hooks that's used for
> parsing "features".
> In order to make -device/device_add utilize "-cpu features"
> template convert it into a set of global properties, so that
> every new CPU created will have them applied automatically.
> 
> That also allows to parse features only once, instread of
> doing it for every CPU instance created.

While I think this makes sense for most cases, we (s390x) are
currently working on a mechanism to compare and baseline cpu models via
a qmp interface, to not have to replicate CPU models in libvirt
every time we do some changes.

To do that, we are creating temporary CPUs to handle the model
parsing. So, with our current prototype, we rely on the mechanism
to parse properties multiple time, as we are really creating
different CPUs.

While we could somehow change our mechanism I don't think this is
the right thing to do.

We will have to support heterogeneous cpu models (I think arm was one of
the guys requesting this if I'm not mistaking) and it somehow
contradicts to the general mechanism of device_add fully specifying
parameters. These would now be implicit parameters.

Would it be possible to do this for x86 only? Or find another way
to handle the "create just another ordinary CPU we already have"?

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Jiri Denemark]

On Thu, Jun 02, 2016 at 22:44:49 +0200, David Hildenbrand wrote:
> > Current CLI option -cpu cpux,features serves as template
> > for all created cpus of type: cpux. However QEMU parses
> > "features" every time it creates a cpu instance and applies
> > them to it while doing parsing.
> > 
> > That doesn't work well with -device/device_add infrastructure
> > as it has no idea about cpu specific hooks that's used for
> > parsing "features".
> > In order to make -device/device_add utilize "-cpu features"
> > template convert it into a set of global properties, so that
> > every new CPU created will have them applied automatically.
> > 
> > That also allows to parse features only once, instread of
> > doing it for every CPU instance created.
> 
> While I think this makes sense for most cases, we (s390x) are
> currently working on a mechanism to compare and baseline cpu models via
> a qmp interface, to not have to replicate CPU models in libvirt
> every time we do some changes.

BTW, we don't replicate any change to QEMU CPU models in libvirt. We
merely add new CPU models that match the current QEMU definition.
Existing CPU models are never changed. And while the CPU models are
currently used to check compatibility with host CPU, we will stop doing
that in the near future and our CPU models will mostly be used for
detecting what the host CPU is.

Jirka

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> On Thu, Jun 02, 2016 at 22:44:49 +0200, David Hildenbrand wrote:
> > > Current CLI option -cpu cpux,features serves as template
> > > for all created cpus of type: cpux. However QEMU parses
> > > "features" every time it creates a cpu instance and applies
> > > them to it while doing parsing.
> > > 
> > > That doesn't work well with -device/device_add infrastructure
> > > as it has no idea about cpu specific hooks that's used for
> > > parsing "features".
> > > In order to make -device/device_add utilize "-cpu features"
> > > template convert it into a set of global properties, so that
> > > every new CPU created will have them applied automatically.
> > > 
> > > That also allows to parse features only once, instread of
> > > doing it for every CPU instance created.  
> > 
> > While I think this makes sense for most cases, we (s390x) are
> > currently working on a mechanism to compare and baseline cpu models via
> > a qmp interface, to not have to replicate CPU models in libvirt
> > every time we do some changes.  
> 
> BTW, we don't replicate any change to QEMU CPU models in libvirt. We
> merely add new CPU models that match the current QEMU definition.
> Existing CPU models are never changed. And while the CPU models are
> currently used to check compatibility with host CPU, we will stop doing
> that in the near future and our CPU models will mostly be used for
> detecting what the host CPU is.
> 
> Jirka
> 


Hi Jirka,

thanks for that info!

unfortunately we will have to perform changes to the cpu models.
On z we have various cpu features that are confidential and only
added over time, although they are around for quite some time.
And we have features that are not implemented yet by KVM and will not
be implemented in the near future.
Therefore we will be adding features to our "flexible models"
while keeping migration safe variants with base features stable.

Some day in the future, we will also have all features around :)

Another problematic thing for us is, that we really have to rely
on a host cpu model detection using KVM. The features that
are visible in user space (similar to cpuid()) don't give any
guarantees to what we are able to virtualize.

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:
> > Current CLI option -cpu cpux,features serves as template
> > for all created cpus of type: cpux. However QEMU parses
> > "features" every time it creates a cpu instance and applies
> > them to it while doing parsing.
> > 
> > That doesn't work well with -device/device_add infrastructure
> > as it has no idea about cpu specific hooks that's used for
> > parsing "features".
> > In order to make -device/device_add utilize "-cpu features"
> > template convert it into a set of global properties, so that
> > every new CPU created will have them applied automatically.
> > 
> > That also allows to parse features only once, instread of
> > doing it for every CPU instance created.
> 
> While I think this makes sense for most cases, we (s390x) are
> currently working on a mechanism to compare and baseline cpu models via
> a qmp interface, to not have to replicate CPU models in libvirt
> every time we do some changes.
> 
> To do that, we are creating temporary CPUs to handle the model
> parsing. So, with our current prototype, we rely on the mechanism
> to parse properties multiple time, as we are really creating
> different CPUs.

This series only changes the code that exists for parsing the
-cpu option, and nothing else. Is this (the code that parses
-cpu) really what you need to reuse?

If all you need is to parse properties, why can't you reuse the
existing QOM/Device mechanisms to handle properties (the one used
by -device and device_add), instead of the -cpu code?

We need to use less of the infrastructure that exists for the
legacy -cpu option (and use more of the generic QOM/Device
mechanisms), not more of it.


> 
> While we could somehow change our mechanism I don't think this is
> the right thing to do.
> 

If reusing the existing parsing code is something you absolutely
need, we could split the process in two parts: 1) converting the
feature string to a list of property=value pairs; 2) registering
the property=value pairs as global properties. Then you coulde
reuse (1) only. But do you really need to reuse the parser for
the legacy -cpu option in your mechanism?

> We will have to support heterogeneous cpu models (I think arm was one of
> the guys requesting this if I'm not mistaking) and it somehow
> contradicts to the general mechanism of device_add fully specifying
> parameters. These would now be implicit parameters.

The -cpu interface really does contradict the general mechanism
of device_add. This whole series is about translating the
handling of -cpu to a more generic mechanism (-global), to allow
us to deprecate -cpu in the future. Why is that a bad thing?

> 
> Would it be possible to do this for x86 only? Or find another way
> to handle the "create just another ordinary CPU we already have"?

Can you clarify what "create just another ordinary CPU we already
have" mean? If you are talking about creating another CPU using
what's on -cpu, nothing changes. If you are talking about
creating another CPU with different options, I suggest not
reusing the code that was written for -cpu.

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:
> > > Current CLI option -cpu cpux,features serves as template
> > > for all created cpus of type: cpux. However QEMU parses
> > > "features" every time it creates a cpu instance and applies
> > > them to it while doing parsing.
> > > 
> > > That doesn't work well with -device/device_add infrastructure
> > > as it has no idea about cpu specific hooks that's used for
> > > parsing "features".
> > > In order to make -device/device_add utilize "-cpu features"
> > > template convert it into a set of global properties, so that
> > > every new CPU created will have them applied automatically.
> > > 
> > > That also allows to parse features only once, instread of
> > > doing it for every CPU instance created.  
> > 
> > While I think this makes sense for most cases, we (s390x) are
> > currently working on a mechanism to compare and baseline cpu models via
> > a qmp interface, to not have to replicate CPU models in libvirt
> > every time we do some changes.
> > 
> > To do that, we are creating temporary CPUs to handle the model
> > parsing. So, with our current prototype, we rely on the mechanism
> > to parse properties multiple time, as we are really creating
> > different CPUs.  
> 
> This series only changes the code that exists for parsing the
> -cpu option, and nothing else. Is this (the code that parses
> -cpu) really what you need to reuse?

I was reading "every new CPU created will have them applied automatically".
If I was having a basic understanding problem here, very good :)

The problematic part is when the properties are applied where the
"changed" data is stored (class. vs. instance).

e.g. in terms of s390x: z13 includes both vx and tx
-cpu z13,vx=off,tx=off

Now, what would happen on
a) device_add z13-s390-cpu // I assume vx=off, tx=off ?

b) device_add z13-s390-cpu,vx=on // vx=on suddenly for all vcpus or one
instance? I assume just this instance

c) device_add zBC12-s390-cpu // will I suddenly get a z13?
Or a zBC12 without tx and vx? I assume the latter.

d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?

d) has to work for us. Otherwise we will have to fallback to manual
property parsing.

> 
> If all you need is to parse properties, why can't you reuse the
> existing QOM/Device mechanisms to handle properties (the one used
> by -device and device_add), instead of the -cpu code?

We can, if my given example works. And the global properties
don't interfere with cpus.

> 
> We need to use less of the infrastructure that exists for the
> legacy -cpu option (and use more of the generic QOM/Device
> mechanisms), not more of it.

It is better to have one way of creating cpus that two.

> 
> 
> > 
> > While we could somehow change our mechanism I don't think this is
> > the right thing to do.
> >   
> 
> If reusing the existing parsing code is something you absolutely
> need, we could split the process in two parts: 1) converting the
> feature string to a list of property=value pairs; 2) registering
> the property=value pairs as global properties. Then you coulde
> reuse (1) only. But do you really need to reuse the parser for
> the legacy -cpu option in your mechanism?

It's really not about the parser, more about the global properties.

> 
> > We will have to support heterogeneous cpu models (I think arm was one of
> > the guys requesting this if I'm not mistaking) and it somehow
> > contradicts to the general mechanism of device_add fully specifying
> > parameters. These would now be implicit parameters.  
> 
> The -cpu interface really does contradict the general mechanism
> of device_add. This whole series is about translating the
> handling of -cpu to a more generic mechanism (-global), to allow
> us to deprecate -cpu in the future. Why is that a bad thing?

It is a bad thing as soon as they affect other devices.
If I did a -cpu z13,tx=off, I don't expect

a) a hot-plugged z13 to suddenly have tx=off
b) a hot-plugged zBC12 to suddenly have tx off

Won't libvirt have to specify the cpu name either way in device-add?
And your plan seems to be that the properties are suddenly implicit.
I don't see a problem with libvirt having to specify the properties
manually on device add.

I agree, cleaning up the parsing function indeed makes sense.

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Igor Mammedov]

On Fri, 3 Jun 2016 08:36:21 +0200
David Hildenbrand <dahi@linux.vnet.ibm.com> wrote:

> > On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:  
> > > > Current CLI option -cpu cpux,features serves as template
> > > > for all created cpus of type: cpux. However QEMU parses
> > > > "features" every time it creates a cpu instance and applies
> > > > them to it while doing parsing.
> > > > 
> > > > That doesn't work well with -device/device_add infrastructure
> > > > as it has no idea about cpu specific hooks that's used for
> > > > parsing "features".
> > > > In order to make -device/device_add utilize "-cpu features"
> > > > template convert it into a set of global properties, so that
> > > > every new CPU created will have them applied automatically.
> > > > 
> > > > That also allows to parse features only once, instread of
> > > > doing it for every CPU instance created.    
> > > 
> > > While I think this makes sense for most cases, we (s390x) are
> > > currently working on a mechanism to compare and baseline cpu models via
> > > a qmp interface, to not have to replicate CPU models in libvirt
> > > every time we do some changes.
> > > 
> > > To do that, we are creating temporary CPUs to handle the model
> > > parsing. So, with our current prototype, we rely on the mechanism
> > > to parse properties multiple time, as we are really creating
> > > different CPUs. 
> > This series only changes the code that exists for parsing the
> > -cpu option, and nothing else. Is this (the code that parses
> > -cpu) really what you need to reuse?  
> 
> I was reading "every new CPU created will have them applied automatically".
> If I was having a basic understanding problem here, very good :)
I should rephrase it to "every new CPU of given cpu type"

> The problematic part is when the properties are applied where the
> "changed" data is stored (class. vs. instance).
> 
> e.g. in terms of s390x: z13 includes both vx and tx
> -cpu z13,vx=off,tx=off
Above -cpu template will be translated into a corresponding
global properties template:

-global z13-s390-cpu.vx=off -global z13-s390-cpu.tx=off


> Now, what would happen on
> a) device_add z13-s390-cpu // I assume vx=off, tx=off ?
as with current -cpu + manual cpu_model parsing per instance,
you'll get above globals applied by the time object_new("z13-s390-cpu"))
is complete.
 
> b) device_add z13-s390-cpu,vx=on // vx=on suddenly for all vcpus or one
> instance? I assume just this instance
yes, You'll get vx=on for just this instance as it's not a global property
and it's applied after object_new() by -device/device_add
 
> c) device_add zBC12-s390-cpu // will I suddenly get a z13?
> Or a zBC12 without tx and vx? I assume the latter.
that depends on class hierarchy,
if zBC12 is inherited from z13 you'll get global properties of z13 applied.
if z13 is not parent of zBC12 then z13 global properties
are not applied to zBC12.
 
Look at qdev_prop_set_globals() and how it's used.

> d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?
nope, you'll z13 with tx and vx off

> d) has to work for us. Otherwise we will have to fallback to manual
> property parsing.
could you elaborate more why do you need (d) work
in combination with -cpu z13,vx=off,tx=off ?
Perhaps there is another way to do what you need.

Current code uses -cpu for creating homogeneous cpus of given type
(i.e.) as template and that's exactly what -global cpufoo,feat=x ... does.

> > If all you need is to parse properties, why can't you reuse the
> > existing QOM/Device mechanisms to handle properties (the one used
> > by -device and device_add), instead of the -cpu code?  
> 
> We can, if my given example works. And the global properties
> don't interfere with cpus.
if you need pure -device/device_add handling then don't use
-cpu CPUFOO,feat=x,... template as its current semantic
is to create all CPUs of type CPUFOO with feat=x,... applied

or more exactly do not use feat... part of it if you don't need
global properties.

> 
> > 
> > We need to use less of the infrastructure that exists for the
> > legacy -cpu option (and use more of the generic QOM/Device
> > mechanisms), not more of it.  
> 
> It is better to have one way of creating cpus that two.
yep, and that's where we heading to, first by making CPU a Device
and then switching to generic -device/device_add model, so use one or
another, -cpu is kept there for compat reasons and some day
we probably could kill it.

> 
> > 
> >   
> > > 
> > > While we could somehow change our mechanism I don't think this is
> > > the right thing to do.
> > >     
> > 
> > If reusing the existing parsing code is something you absolutely
> > need, we could split the process in two parts: 1) converting the
> > feature string to a list of property=value pairs; 2) registering
> > the property=value pairs as global properties. Then you coulde
> > reuse (1) only. But do you really need to reuse the parser for
> > the legacy -cpu option in your mechanism?  
> 
> It's really not about the parser, more about the global properties.
> 
> >   
> > > We will have to support heterogeneous cpu models (I think arm was one of
> > > the guys requesting this if I'm not mistaking) and it somehow
> > > contradicts to the general mechanism of device_add fully specifying
> > > parameters. These would now be implicit parameters.    
> > 
> > The -cpu interface really does contradict the general mechanism
> > of device_add. This whole series is about translating the
> > handling of -cpu to a more generic mechanism (-global), to allow
> > us to deprecate -cpu in the future. Why is that a bad thing?  
> 
> It is a bad thing as soon as they affect other devices.
> If I did a -cpu z13,tx=off, I don't expect
> 
> a) a hot-plugged z13 to suddenly have tx=off
you should expect it though, as -cpu z13,tx=off is template
for all z13 CPUs, it's not per instance thingy

> b) a hot-plugged zBC12 to suddenly have tx off
that will not have tx off unless zBC12 is inherited from z13

> Won't libvirt have to specify the cpu name either way in device-add?
> And your plan seems to be that the properties are suddenly implicit.
> I don't see a problem with libvirt having to specify the properties
> manually on device add.
libvirt could do verbose
#1  -device CPUFOO,feat1=x -device CPUFOO,feat1=x ...
or
#2  -cpu CPUFOO,feat1=x -device CPUFOO -device CPUFOO ...
  which is equivalent of generic device syntax and not use -cpu at all:
#3  -global CPUFOO.feat1=x -device CPUFOO -device CPUFOO ...

#3 is where we are heading to by making -device/device_add available for
CPUs. If you wish to have CPUs of the same type but with different
features, don't use global properties for that features.
Just like with any other device.

> I agree, cleaning up the parsing function indeed makes sense.
> 
> David
> 

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> > e.g. in terms of s390x: z13 includes both vx and tx
> > -cpu z13,vx=off,tx=off  
> Above -cpu template will be translated into a corresponding
> global properties template:
> 
> -global z13-s390-cpu.vx=off -global z13-s390-cpu.tx=off

This description makes it much clearer how you expect -cpu to behave. Thanks.
The important part for me was that -global applies to this cpu type only.

And our internal hierarchy should make sure that this is guaranteed. 
And good to known that it is a common schema we are following here.

> > d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?  
> nope, you'll z13 with tx and vx off
> 
> > d) has to work for us. Otherwise we will have to fallback to manual
> > property parsing.  
> could you elaborate more why do you need (d) work
> in combination with -cpu z13,vx=off,tx=off ?
> Perhaps there is another way to do what you need.

Say we started QEMU with -z13,vx=off,tx=off.
When I do a QMP query to e.g. compare two cpu models, say a temporary cpu
zBC12,tx=on and zBC13,tx=on is created. And the additional
parameters only affect these instances as I learned.

When I now compare them, the result will depend on the
way QEMU has been started. But most likely this is okay, as
we expect this interface to be used without any cpu model at all.

This then just has to be documented accordingly.

> 
> Current code uses -cpu for creating homogeneous cpus of given type
> (i.e.) as template and that's exactly what -global cpufoo,feat=x ... does.
> 
> > > If all you need is to parse properties, why can't you reuse the
> > > existing QOM/Device mechanisms to handle properties (the one used
> > > by -device and device_add), instead of the -cpu code?    
> > 
> > We can, if my given example works. And the global properties
> > don't interfere with cpus.  
> if you need pure -device/device_add handling then don't use
> -cpu CPUFOO,feat=x,... template as its current semantic
> is to create all CPUs of type CPUFOO with feat=x,... applied
> > It is a bad thing as soon as they affect other devices.
> > If I did a -cpu z13,tx=off, I don't expect
> > 
> > a) a hot-plugged z13 to suddenly have tx=off  
> you should expect it though, as -cpu z13,tx=off is template
> for all z13 CPUs, it's not per instance thingy
> 
> > b) a hot-plugged zBC12 to suddenly have tx off  
> that will not have tx off unless zBC12 is inherited from z13

They don't inherit, so this should be fine.

> 
> > Won't libvirt have to specify the cpu name either way in device-add?
> > And your plan seems to be that the properties are suddenly implicit.
> > I don't see a problem with libvirt having to specify the properties
> > manually on device add.  
> libvirt could do verbose
> #1  -device CPUFOO,feat1=x -device CPUFOO,feat1=x ...
> or
> #2  -cpu CPUFOO,feat1=x -device CPUFOO -device CPUFOO ...
>   which is equivalent of generic device syntax and not use -cpu at all:
> #3  -global CPUFOO.feat1=x -device CPUFOO -device CPUFOO ...
> 
> #3 is where we are heading to by making -device/device_add available for
> CPUs. If you wish to have CPUs of the same type but with different
> features, don't use global properties for that features.
> Just like with any other device.

So this should work for us, we just have to document that -global for cpus
or -cpu should not be used when trying to work with the new QMP 
queries.

Thanks for the explanation!

David

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[Eduardo Habkost]

On Fri, Jun 03, 2016 at 08:36:21AM +0200, David Hildenbrand wrote:
> > On Thu, Jun 02, 2016 at 10:44:49PM +0200, David Hildenbrand wrote:
> > > > Current CLI option -cpu cpux,features serves as template
> > > > for all created cpus of type: cpux. However QEMU parses
> > > > "features" every time it creates a cpu instance and applies
> > > > them to it while doing parsing.
> > > > 
> > > > That doesn't work well with -device/device_add infrastructure
> > > > as it has no idea about cpu specific hooks that's used for
> > > > parsing "features".
> > > > In order to make -device/device_add utilize "-cpu features"
> > > > template convert it into a set of global properties, so that
> > > > every new CPU created will have them applied automatically.
> > > > 
> > > > That also allows to parse features only once, instread of
> > > > doing it for every CPU instance created.  
> > > 
> > > While I think this makes sense for most cases, we (s390x) are
> > > currently working on a mechanism to compare and baseline cpu models via
> > > a qmp interface, to not have to replicate CPU models in libvirt
> > > every time we do some changes.
> > > 
> > > To do that, we are creating temporary CPUs to handle the model
> > > parsing. So, with our current prototype, we rely on the mechanism
> > > to parse properties multiple time, as we are really creating
> > > different CPUs.  
> > 
> > This series only changes the code that exists for parsing the
> > -cpu option, and nothing else. Is this (the code that parses
> > -cpu) really what you need to reuse?
> 
> I was reading "every new CPU created will have them applied automatically".
> If I was having a basic understanding problem here, very good :)

Sorry, I misunderstood you:

So, you won't reuse the code for -cpu, but with global
properties, you are going to be affected even if you do a simple
object_new()/device_add. I see the problem, now.

I assume Igor explained it, already, and his suggestion sounds OK
to you. But I will answer your questions to confirm that this is
really the case:

> 
> The problematic part is when the properties are applied where the
> "changed" data is stored (class. vs. instance).
> 
> e.g. in terms of s390x: z13 includes both vx and tx
> -cpu z13,vx=off,tx=off

This will be translated to:
-global z13-s390-cpu.vx=off -global z13-s390-cpu.tx=off

> 
> Now, what would happen on
> a) device_add z13-s390-cpu // I assume vx=off, tx=off ?

Yes.

> 
> b) device_add z13-s390-cpu,vx=on // vx=on suddenly for all vcpus or one
> instance? I assume just this instance

It would affect all z13-s390-cpu instances.

> 
> c) device_add zBC12-s390-cpu // will I suddenly get a z13?
> Or a zBC12 without tx and vx? I assume the latter.

A zBC12 with the default values (not affected by -cpu).

> 
> d) object_new("z13-s390-cpu")); // will I get a clean z13 with tx and vx on?

The same as device_add z13-s390-cpu (a z13 without
tx and vx).

> 
> d) has to work for us. Otherwise we will have to fallback to manual
> property parsing.

It will be affected by the globals, but I assume management code
is not going to use add extra -cpu arguments when probing for CPU
model information, right?

Users will need to be aware that -cpu is equivalent to -global,
and will affect CPU information returned by query-cpu-definitions
(or similar commands).

> 
> > 
> > If all you need is to parse properties, why can't you reuse the
> > existing QOM/Device mechanisms to handle properties (the one used
> > by -device and device_add), instead of the -cpu code?
> 
> We can, if my given example works. And the global properties
> don't interfere with cpus.

They do, but only the model specified in -cpu.

> 
> > 
> > We need to use less of the infrastructure that exists for the
> > legacy -cpu option (and use more of the generic QOM/Device
> > mechanisms), not more of it.
> 
> It is better to have one way of creating cpus that two.

Unfortunately we already have two ways of creating CPUs: -cpu and
device_add. We are trying to translate -cpu to something
equivalent to generic mechanisms (-device and -global), so we
have only one underlying mechanism.

> 
> > 
> > 
> > > 
> > > While we could somehow change our mechanism I don't think this is
> > > the right thing to do.
> > >   
> > 
> > If reusing the existing parsing code is something you absolutely
> > need, we could split the process in two parts: 1) converting the
> > feature string to a list of property=value pairs; 2) registering
> > the property=value pairs as global properties. Then you coulde
> > reuse (1) only. But do you really need to reuse the parser for
> > the legacy -cpu option in your mechanism?
> 
> It's really not about the parser, more about the global properties.

Understood.

> 
> > 
> > > We will have to support heterogeneous cpu models (I think arm was one of
> > > the guys requesting this if I'm not mistaking) and it somehow
> > > contradicts to the general mechanism of device_add fully specifying
> > > parameters. These would now be implicit parameters.  
> > 
> > The -cpu interface really does contradict the general mechanism
> > of device_add. This whole series is about translating the
> > handling of -cpu to a more generic mechanism (-global), to allow
> > us to deprecate -cpu in the future. Why is that a bad thing?
> 
> It is a bad thing as soon as they affect other devices.
> If I did a -cpu z13,tx=off, I don't expect
> 
> a) a hot-plugged z13 to suddenly have tx=off

It will.

(Igor, can you confirm?)


> b) a hot-plugged zBC12 to suddenly have tx off

It won't.


I understand that this may be confusing, but that's because -smp
and -cpu don't fit the QEMU device/object models. We are moving
towards allowing CPU topologies to be created using only -device.

To do that, we are gradually translating -cpu to the generic
mechanisms used by -device/-global, so command-line options could
be easily converted to use the new mechanisms in the future.

Does that make sense?

> 
> Won't libvirt have to specify the cpu name either way in device-add?
> And your plan seems to be that the properties are suddenly implicit.
> I don't see a problem with libvirt having to specify the properties
> manually on device add.
> 
> I agree, cleaning up the parsing function indeed makes sense.
> 
> David
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH RFC 0/8] cpus: make "-cpu cpux, features" global properties

[David Hildenbrand]

> I assume Igor explained it, already, and his suggestion sounds OK
> to you. But I will answer your questions to confirm that this is
> really the case:

Yes, this all sounds good to me, thanks for the additional explanation!
[...]

> 
> > 
> > d) has to work for us. Otherwise we will have to fallback to manual
> > property parsing.  
> 
> It will be affected by the globals, but I assume management code
> is not going to use add extra -cpu arguments when probing for CPU
> model information, right?

Yes, that's also what I figured out, this should not be a problem.

> 
> Users will need to be aware that -cpu is equivalent to -global,
> and will affect CPU information returned by query-cpu-definitions
> (or similar commands).
> 
> >   
> > > 
> > > If all you need is to parse properties, why can't you reuse the
> > > existing QOM/Device mechanisms to handle properties (the one used
> > > by -device and device_add), instead of the -cpu code?  
> > 
> > We can, if my given example works. And the global properties
> > don't interfere with cpus.  
> 
> They do, but only the model specified in -cpu.
> 
> >   
> > > 
> > > We need to use less of the infrastructure that exists for the
> > > legacy -cpu option (and use more of the generic QOM/Device
> > > mechanisms), not more of it.  
> > 
> > It is better to have one way of creating cpus that two.  
> 
> Unfortunately we already have two ways of creating CPUs: -cpu and
> device_add. We are trying to translate -cpu to something
> equivalent to generic mechanisms (-device and -global), so we
> have only one underlying mechanism.

Yes, understood. Sounds sane to me!
[...]

> I understand that this may be confusing, but that's because -smp
> and -cpu don't fit the QEMU device/object models. We are moving
> towards allowing CPU topologies to be created using only -device.
> 
> To do that, we are gradually translating -cpu to the generic
> mechanisms used by -device/-global, so command-line options could
> be easily converted to use the new mechanisms in the future.
> 
> Does that make sense?
> 

Absolutely!

David