Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-30 Thread Igor Mammedov 
On Fri, 27 May 2016 17:32:34 -0300
Eduardo Habkost  wrote:

> On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
> > On Tue, 24 May 2016 09:34:05 -0300
> > Eduardo Habkost  wrote:
> >   
> > > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:  
> > > > On Fri,  6 May 2016 15:11:31 -0300
> > > > Eduardo Habkost  wrote:  
> [...]
> > > > > -/* Convert all '_' in a feature string option name to '-', to make 
> > > > > feature
> > > > > - * name conform to QOM property naming rule, which uses '-' instead 
> > > > > of '_'.
> > > > > +/* Convert all '_' in a feature string option name to '-', to keep 
> > > > > compatibility
> > > > > + * with old feature names that used "_" instead of "-".
> > > > >   */
> > > > >  static inline void feat2prop(char *s)
> > > > >  {
> > > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState 
> > > > > *cs, char *features,
> > > > >  while (featurestr) {
> > > > >  char *val;
> > > > I'd place a single feat2prop() here
> > > > and delete it from other call sites in this function.
> > > 
> > > A previous version of this patch had it. But it would change the
> > > property value too, not just the property name (breaking stuff
> > > like "model-id=some_string").
> > >  
> > it's bug in feat2prop(), which probably should be fixed there,
> > so it would do what comment above it says. Or as alternative:  
> 
> The comment above it doesn't say anything about stopping at a '='
> delimiter. I always expected it to just replace "_" with "-" in a
> null-terminated string.
> 
> (I am not completely against making it stop at '=', but I believe
> your suggestion below sounds better).
> 
> > 
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index ca2a893..e46e4c3 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> > char *features
> >  featurestr = features ? strtok(features, ",") : NULL;
> >  
> >  while (featurestr) {
> > -char *val;
> > +char *val = strchr(featurestr, '=');
> > +if (val) {
> > +*val = 0; val++;
> > +}
> > +feat2prop(featurestr);  
> 
> This would make "+feature=FOO" treated as a valid option, and it
> isn't. It would keep the existing behavior if we did this:
> 
> -  if (featurestr[0] == '+') {
> +  if (featurestr[0] == '+' && !val) {
>add_flagname_to_bitmaps(featurestr + 1, plus_features, 
> _err);
> -  } else if (featurestr[0] == '-') {
> +  if (featurestr[0] == '+' && !val) {
>add_flagname_to_bitmaps(featurestr + 1, minus_features, 
> _err);
> 
> In either case, I prefer to get this optimization reviewed as a
> separate patch. Can you send it as a follow-up?
sure

> 
> > -} else if ((val = strchr(featurestr, '='))) {
> > -*val = 0; val++;
> > -feat2prop(featurestr);
> > +} else if (val) {
> >  if (!strcmp(featurestr, "xlevel")) {
> >  char *err;
> >  char num[32];
> > @@ -2000,7 +2002,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> > char *features,
> >  object_property_parse(OBJECT(cpu), val, featurestr, 
> > _err);
> >  }
> >  } else {
> > -feat2prop(featurestr);
> >  object_property_parse(OBJECT(cpu), "on", featurestr, 
> > _err);
> >  }
> >  if (local_err) {
> > 
> >   
>

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-27 Thread Eduardo Habkost 
On Tue, May 24, 2016 at 03:22:27PM +0200, Igor Mammedov wrote:
> On Tue, 24 May 2016 09:34:05 -0300
> Eduardo Habkost  wrote:
> 
> > On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> > > On Fri,  6 May 2016 15:11:31 -0300
> > > Eduardo Habkost  wrote:
[...]
> > > > -/* Convert all '_' in a feature string option name to '-', to make 
> > > > feature
> > > > - * name conform to QOM property naming rule, which uses '-' instead of 
> > > > '_'.
> > > > +/* Convert all '_' in a feature string option name to '-', to keep 
> > > > compatibility
> > > > + * with old feature names that used "_" instead of "-".
> > > >   */
> > > >  static inline void feat2prop(char *s)
> > > >  {
> > > > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState 
> > > > *cs, char *features,
> > > >  while (featurestr) {
> > > >  char *val;  
> > > I'd place a single feat2prop() here
> > > and delete it from other call sites in this function.  
> > 
> > A previous version of this patch had it. But it would change the
> > property value too, not just the property name (breaking stuff
> > like "model-id=some_string").
> >
> it's bug in feat2prop(), which probably should be fixed there,
> so it would do what comment above it says. Or as alternative:

The comment above it doesn't say anything about stopping at a '='
delimiter. I always expected it to just replace "_" with "-" in a
null-terminated string.

(I am not completely against making it stop at '=', but I believe
your suggestion below sounds better).

> 
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index ca2a893..e46e4c3 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1941,14 +1941,16 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> char *features
>  featurestr = features ? strtok(features, ",") : NULL;
>  
>  while (featurestr) {
> -char *val;
> +char *val = strchr(featurestr, '=');
> +if (val) {
> +*val = 0; val++;
> +}
> +feat2prop(featurestr);

This would make "+feature=FOO" treated as a valid option, and it
isn't. It would keep the existing behavior if we did this:

-  if (featurestr[0] == '+') {
+  if (featurestr[0] == '+' && !val) {
   add_flagname_to_bitmaps(featurestr + 1, plus_features, 
_err);
-  } else if (featurestr[0] == '-') {
+  if (featurestr[0] == '+' && !val) {
   add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);

In either case, I prefer to get this optimization reviewed as a
separate patch. Can you send it as a follow-up?

> -} else if ((val = strchr(featurestr, '='))) {
> -*val = 0; val++;
> -feat2prop(featurestr);
> +} else if (val) {
>  if (!strcmp(featurestr, "xlevel")) {
>  char *err;
>  char num[32];
> @@ -2000,7 +2002,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char 
> *features,
>  object_property_parse(OBJECT(cpu), val, featurestr, 
> _err);
>  }
>  } else {
> -feat2prop(featurestr);
>  object_property_parse(OBJECT(cpu), "on", featurestr, _err);
>  }
>  if (local_err) {
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-24 Thread Igor Mammedov 
On Tue, 24 May 2016 09:34:05 -0300
Eduardo Habkost  wrote:

> On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> > On Fri,  6 May 2016 15:11:31 -0300
> > Eduardo Habkost  wrote:
> >   
> > > This makes the feature name tables in feature_word_info all match
> > > the actual QOM property names we use.
> > > 
> > > This will make the command-line interface more consistent,
> > > allowing the QOM property names to be used as "-cpu" arguments
> > > directly.
> > > 
> > > Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> > > compatibility with the old that had underscores.
> > > 
> > > Cc: Jiri Denemark 
> > > Cc: libvir-l...@redhat.com
> > > Signed-off-by: Eduardo Habkost 
> > > ---
> > >  target-i386/cpu.c | 32 
> > >  1 file changed, 16 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index 309ef55..e7365d1 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -187,7 +187,7 @@ static const char *feature_name[] = {
> > >  };
> > >  static const char *ext_feature_name[] = {
> > >  "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
> > > "monitor",
> > > -"ds_cpl", "vmx", "smx", "est",
> > > +"ds-cpl", "vmx", "smx", "est",
> > >  "tm2", "ssse3", "cid", NULL,
> > >  "fma", "cx16", "xtpr", "pdcm",
> > >  NULL, "pcid", "dca", "sse4.1|sse4_1",
> > > @@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
> > >  NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
> > >  NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
> > >  "nx|xd", NULL, "mmxext", NULL /* mmx */,
> > > -NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, 
> > > "rdtscp",
> > > +NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, 
> > > "rdtscp",
> > >  NULL, "lm|i64", "3dnowext", "3dnow",
> > >  };
> > >  static const char *ext3_feature_name[] = {
> > > -"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
> > > ExtApicSpace */,
> > > +"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
> > > ExtApicSpace */,
> > >  "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
> > >  "3dnowprefetch", "osvw", "ibs", "xop",
> > >  "skinit", "wdt", NULL, "lwp",
> > > -"fma4", "tce", NULL, "nodeid_msr",
> > > -NULL, "tbm", "topoext", "perfctr_core",
> > > -"perfctr_nb", NULL, NULL, NULL,
> > > +"fma4", "tce", NULL, "nodeid-msr",
> > > +NULL, "tbm", "topoext", "perfctr-core",
> > > +"perfctr-nb", NULL, NULL, NULL,
> > >  NULL, NULL, NULL, NULL,
> > >  };
> > >  
> > > @@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
> > >  };
> > >  
> > >  static const char *kvm_feature_name[] = {
> > > -"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
> > > -"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
> > > +"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
> > > +"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
> > >  NULL, NULL, NULL, NULL,
> > >  NULL, NULL, NULL, NULL,
> > >  NULL, NULL, NULL, NULL,
> > > @@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
> > >  };
> > >  
> > >  static const char *svm_feature_name[] = {
> > > -"npt", "lbrv", "svm_lock", "nrip_save",
> > > -"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> > > -NULL, NULL, "pause_filter", NULL,
> > > +"npt", "lbrv", "svm-lock", "nrip-save",
> > > +"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
> > > +NULL, NULL, "pause-filter", NULL,
> > >  "pfthreshold", NULL, NULL, NULL,
> > >  NULL, NULL, NULL, NULL,
> > >  NULL, NULL, NULL, NULL,
> > > @@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
> > >  };
> > >  
> > >  static const char *cpuid_7_0_ebx_feature_name[] = {
> > > -"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> > > +"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> > >  "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
> > >  "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", 
> > > "clflushopt",
> > >  "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> > > @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
> > >  .set   = x86_set_hv_spinlocks,
> > >  };
> > >  
> > > -/* Convert all '_' in a feature string option name to '-', to make 
> > > feature
> > > - * name conform to QOM property naming rule, which uses '-' instead of 
> > > '_'.
> > > +/* Convert all '_' in a feature string option name to '-', to keep 
> > > compatibility
> > > + * with old feature names that used "_" instead of "-".
> > >   */
> > >  static inline void feat2prop(char *s)
> > >  {
> > > @@ -1925,8 +1925,10 @@

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-24 Thread Eduardo Habkost 
On Tue, May 24, 2016 at 02:17:03PM +0200, Igor Mammedov wrote:
> On Fri,  6 May 2016 15:11:31 -0300
> Eduardo Habkost  wrote:
> 
> > This makes the feature name tables in feature_word_info all match
> > the actual QOM property names we use.
> > 
> > This will make the command-line interface more consistent,
> > allowing the QOM property names to be used as "-cpu" arguments
> > directly.
> > 
> > Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> > compatibility with the old that had underscores.
> > 
> > Cc: Jiri Denemark 
> > Cc: libvir-l...@redhat.com
> > Signed-off-by: Eduardo Habkost 
> > ---
> >  target-i386/cpu.c | 32 
> >  1 file changed, 16 insertions(+), 16 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 309ef55..e7365d1 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -187,7 +187,7 @@ static const char *feature_name[] = {
> >  };
> >  static const char *ext_feature_name[] = {
> >  "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
> > "monitor",
> > -"ds_cpl", "vmx", "smx", "est",
> > +"ds-cpl", "vmx", "smx", "est",
> >  "tm2", "ssse3", "cid", NULL,
> >  "fma", "cx16", "xtpr", "pdcm",
> >  NULL, "pcid", "dca", "sse4.1|sse4_1",
> > @@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
> >  NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
> >  NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
> >  "nx|xd", NULL, "mmxext", NULL /* mmx */,
> > -NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, 
> > "rdtscp",
> > +NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, 
> > "rdtscp",
> >  NULL, "lm|i64", "3dnowext", "3dnow",
> >  };
> >  static const char *ext3_feature_name[] = {
> > -"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
> > ExtApicSpace */,
> > +"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
> > ExtApicSpace */,
> >  "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
> >  "3dnowprefetch", "osvw", "ibs", "xop",
> >  "skinit", "wdt", NULL, "lwp",
> > -"fma4", "tce", NULL, "nodeid_msr",
> > -NULL, "tbm", "topoext", "perfctr_core",
> > -"perfctr_nb", NULL, NULL, NULL,
> > +"fma4", "tce", NULL, "nodeid-msr",
> > +NULL, "tbm", "topoext", "perfctr-core",
> > +"perfctr-nb", NULL, NULL, NULL,
> >  NULL, NULL, NULL, NULL,
> >  };
> >  
> > @@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
> >  };
> >  
> >  static const char *kvm_feature_name[] = {
> > -"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
> > -"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
> > +"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
> > +"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
> >  NULL, NULL, NULL, NULL,
> >  NULL, NULL, NULL, NULL,
> >  NULL, NULL, NULL, NULL,
> > @@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
> >  };
> >  
> >  static const char *svm_feature_name[] = {
> > -"npt", "lbrv", "svm_lock", "nrip_save",
> > -"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> > -NULL, NULL, "pause_filter", NULL,
> > +"npt", "lbrv", "svm-lock", "nrip-save",
> > +"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
> > +NULL, NULL, "pause-filter", NULL,
> >  "pfthreshold", NULL, NULL, NULL,
> >  NULL, NULL, NULL, NULL,
> >  NULL, NULL, NULL, NULL,
> > @@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
> >  };
> >  
> >  static const char *cpuid_7_0_ebx_feature_name[] = {
> > -"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> > +"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> >  "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
> >  "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", 
> > "clflushopt",
> >  "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> > @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
> >  .set   = x86_set_hv_spinlocks,
> >  };
> >  
> > -/* Convert all '_' in a feature string option name to '-', to make feature
> > - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> > +/* Convert all '_' in a feature string option name to '-', to keep 
> > compatibility
> > + * with old feature names that used "_" instead of "-".
> >   */
> >  static inline void feat2prop(char *s)
> >  {
> > @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> > char *features,
> >  while (featurestr) {
> >  char *val;
> I'd place a single feat2prop() here
> and delete it from other call sites in this function.

A previous version of this patch had it. But it would change the
property value too, not just the

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-24 Thread Igor Mammedov 
On Fri,  6 May 2016 15:11:31 -0300
Eduardo Habkost  wrote:

> This makes the feature name tables in feature_word_info all match
> the actual QOM property names we use.
> 
> This will make the command-line interface more consistent,
> allowing the QOM property names to be used as "-cpu" arguments
> directly.
> 
> Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> compatibility with the old that had underscores.
> 
> Cc: Jiri Denemark 
> Cc: libvir-l...@redhat.com
> Signed-off-by: Eduardo Habkost 
> ---
>  target-i386/cpu.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 309ef55..e7365d1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -187,7 +187,7 @@ static const char *feature_name[] = {
>  };
>  static const char *ext_feature_name[] = {
>  "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
> "monitor",
> -"ds_cpl", "vmx", "smx", "est",
> +"ds-cpl", "vmx", "smx", "est",
>  "tm2", "ssse3", "cid", NULL,
>  "fma", "cx16", "xtpr", "pdcm",
>  NULL, "pcid", "dca", "sse4.1|sse4_1",
> @@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
>  NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
>  NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
>  "nx|xd", NULL, "mmxext", NULL /* mmx */,
> -NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
> +NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
>  NULL, "lm|i64", "3dnowext", "3dnow",
>  };
>  static const char *ext3_feature_name[] = {
> -"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
> ExtApicSpace */,
> +"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
> ExtApicSpace */,
>  "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
>  "3dnowprefetch", "osvw", "ibs", "xop",
>  "skinit", "wdt", NULL, "lwp",
> -"fma4", "tce", NULL, "nodeid_msr",
> -NULL, "tbm", "topoext", "perfctr_core",
> -"perfctr_nb", NULL, NULL, NULL,
> +"fma4", "tce", NULL, "nodeid-msr",
> +NULL, "tbm", "topoext", "perfctr-core",
> +"perfctr-nb", NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  };
>  
> @@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
>  };
>  
>  static const char *kvm_feature_name[] = {
> -"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
> -"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
> +"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
> +"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
> @@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
>  };
>  
>  static const char *svm_feature_name[] = {
> -"npt", "lbrv", "svm_lock", "nrip_save",
> -"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> -NULL, NULL, "pause_filter", NULL,
> +"npt", "lbrv", "svm-lock", "nrip-save",
> +"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
> +NULL, NULL, "pause-filter", NULL,
>  "pfthreshold", NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
> @@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
>  };
>  
>  static const char *cpuid_7_0_ebx_feature_name[] = {
> -"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> +"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
>  "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
>  "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
>  "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
>  .set   = x86_set_hv_spinlocks,
>  };
>  
> -/* Convert all '_' in a feature string option name to '-', to make feature
> - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> +/* Convert all '_' in a feature string option name to '-', to keep 
> compatibility
> + * with old feature names that used "_" instead of "-".
>   */
>  static inline void feat2prop(char *s)
>  {
> @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> char *features,
>  while (featurestr) {
>  char *val;
I'd place a single feat2prop() here
and delete it from other call sites in this function.

>  if (featurestr[0] == '+') {
> +feat2prop(featurestr);
>  add_flagname_to_bitmaps(featurestr + 1, plus_features, 
> _err);
>  } else if (featurestr[0] == '-') {
> +feat2prop(featurestr);
>  add_flagname_to_bitmaps(featurestr + 1, minus_features, 
> _err);
>  } else if ((val = strchr(featurestr, '='))) {
>

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-10 Thread Jiri Denemark 
On Tue, May 10, 2016 at 17:19:51 +0200, Igor Mammedov wrote:
> On Fri,  6 May 2016 15:11:31 -0300
> Eduardo Habkost  wrote:
> 
> > This makes the feature name tables in feature_word_info all match
> > the actual QOM property names we use.
> > 
> > This will make the command-line interface more consistent,
> > allowing the QOM property names to be used as "-cpu" arguments
> > directly.
> 
> wouldn't that change output of '-cpu help',
> can it break libvirt?

Don't worry, for any QEMU >= 1.2.0 libvirt uses only QMP commands when
probing capabilities. And currently we don't even parse feature names
anywhere, we only use feature names when constructing -cpu command line
and that parts seems to be covered by this patch.

Jirka

Re: [Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-10 Thread Igor Mammedov 
On Fri,  6 May 2016 15:11:31 -0300
Eduardo Habkost  wrote:

> This makes the feature name tables in feature_word_info all match
> the actual QOM property names we use.
> 
> This will make the command-line interface more consistent,
> allowing the QOM property names to be used as "-cpu" arguments
> directly.

wouldn't that change output of '-cpu help',
can it break libvirt?


> Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
> compatibility with the old that had underscores.
> 
> Cc: Jiri Denemark 
> Cc: libvir-l...@redhat.com
> Signed-off-by: Eduardo Habkost 
> ---
>  target-i386/cpu.c | 32 
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 309ef55..e7365d1 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -187,7 +187,7 @@ static const char *feature_name[] = {
>  };
>  static const char *ext_feature_name[] = {
>  "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
> "monitor",
> -"ds_cpl", "vmx", "smx", "est",
> +"ds-cpl", "vmx", "smx", "est",
>  "tm2", "ssse3", "cid", NULL,
>  "fma", "cx16", "xtpr", "pdcm",
>  NULL, "pcid", "dca", "sse4.1|sse4_1",
> @@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
>  NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
>  NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
>  "nx|xd", NULL, "mmxext", NULL /* mmx */,
> -NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
> +NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
>  NULL, "lm|i64", "3dnowext", "3dnow",
>  };
>  static const char *ext3_feature_name[] = {
> -"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
> ExtApicSpace */,
> +"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
> ExtApicSpace */,
>  "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
>  "3dnowprefetch", "osvw", "ibs", "xop",
>  "skinit", "wdt", NULL, "lwp",
> -"fma4", "tce", NULL, "nodeid_msr",
> -NULL, "tbm", "topoext", "perfctr_core",
> -"perfctr_nb", NULL, NULL, NULL,
> +"fma4", "tce", NULL, "nodeid-msr",
> +NULL, "tbm", "topoext", "perfctr-core",
> +"perfctr-nb", NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  };
>  
> @@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
>  };
>  
>  static const char *kvm_feature_name[] = {
> -"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
> -"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
> +"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
> +"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
> @@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
>  };
>  
>  static const char *svm_feature_name[] = {
> -"npt", "lbrv", "svm_lock", "nrip_save",
> -"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
> -NULL, NULL, "pause_filter", NULL,
> +"npt", "lbrv", "svm-lock", "nrip-save",
> +"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
> +NULL, NULL, "pause-filter", NULL,
>  "pfthreshold", NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
>  NULL, NULL, NULL, NULL,
> @@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
>  };
>  
>  static const char *cpuid_7_0_ebx_feature_name[] = {
> -"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
> +"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
>  "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
>  "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
>  "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
> @@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
>  .set   = x86_set_hv_spinlocks,
>  };
>  
> -/* Convert all '_' in a feature string option name to '-', to make feature
> - * name conform to QOM property naming rule, which uses '-' instead of '_'.
> +/* Convert all '_' in a feature string option name to '-', to keep 
> compatibility
> + * with old feature names that used "_" instead of "-".
>   */
>  static inline void feat2prop(char *s)
>  {
> @@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, 
> char *features,
>  while (featurestr) {
>  char *val;
>  if (featurestr[0] == '+') {
> +feat2prop(featurestr);
>  add_flagname_to_bitmaps(featurestr + 1, plus_features, 
> _err);
>  } else if (featurestr[0] == '-') {
> +feat2prop(featurestr);
>  add_flagname_to_bitmaps(featurestr + 1, minus_features, 
> _err);
>  } else if ((val = strchr(featurestr, '='))) {
>  *val = 0; val++;
> @@

[Qemu-devel] [PATCH 8/9] target-i386: Use "-" instead of "_" on all feature names

2016-05-06 Thread Eduardo Habkost 
This makes the feature name tables in feature_word_info all match
the actual QOM property names we use.

This will make the command-line interface more consistent,
allowing the QOM property names to be used as "-cpu" arguments
directly.

Add extra feat2prop() calls to x86_cpu_parse_featurestr() to keep
compatibility with the old that had underscores.

Cc: Jiri Denemark 
Cc: libvir-l...@redhat.com
Signed-off-by: Eduardo Habkost 
---
 target-i386/cpu.c | 32 
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 309ef55..e7365d1 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -187,7 +187,7 @@ static const char *feature_name[] = {
 };
 static const char *ext_feature_name[] = {
 "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", "monitor",
-"ds_cpl", "vmx", "smx", "est",
+"ds-cpl", "vmx", "smx", "est",
 "tm2", "ssse3", "cid", NULL,
 "fma", "cx16", "xtpr", "pdcm",
 NULL, "pcid", "dca", "sse4.1|sse4_1",
@@ -207,17 +207,17 @@ static const char *ext2_feature_name[] = {
 NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
 NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
 "nx|xd", NULL, "mmxext", NULL /* mmx */,
-NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
+NULL /* fxsr */, "fxsr-opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
 NULL, "lm|i64", "3dnowext", "3dnow",
 };
 static const char *ext3_feature_name[] = {
-"lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
ExtApicSpace */,
+"lahf-lm" /* AMD LahfSahf */, "cmp-legacy", "svm", "extapic" /* AMD 
ExtApicSpace */,
 "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
 "3dnowprefetch", "osvw", "ibs", "xop",
 "skinit", "wdt", NULL, "lwp",
-"fma4", "tce", NULL, "nodeid_msr",
-NULL, "tbm", "topoext", "perfctr_core",
-"perfctr_nb", NULL, NULL, NULL,
+"fma4", "tce", NULL, "nodeid-msr",
+NULL, "tbm", "topoext", "perfctr-core",
+"perfctr-nb", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 };
 
@@ -233,8 +233,8 @@ static const char *ext4_feature_name[] = {
 };
 
 static const char *kvm_feature_name[] = {
-"kvmclock", "kvm_nopiodelay", "kvm_mmu", "kvmclock",
-"kvm_asyncpf", "kvm_steal_time", "kvm_pv_eoi", "kvm_pv_unhalt",
+"kvmclock", "kvm-nopiodelay", "kvm-mmu", "kvmclock",
+"kvm-asyncpf", "kvm-steal-time", "kvm-pv-eoi", "kvm-pv-unhalt",
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -244,9 +244,9 @@ static const char *kvm_feature_name[] = {
 };
 
 static const char *svm_feature_name[] = {
-"npt", "lbrv", "svm_lock", "nrip_save",
-"tsc_scale", "vmcb_clean",  "flushbyasid", "decodeassists",
-NULL, NULL, "pause_filter", NULL,
+"npt", "lbrv", "svm-lock", "nrip-save",
+"tsc-scale", "vmcb-clean",  "flushbyasid", "decodeassists",
+NULL, NULL, "pause-filter", NULL,
 "pfthreshold", NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
 NULL, NULL, NULL, NULL,
@@ -255,7 +255,7 @@ static const char *svm_feature_name[] = {
 };
 
 static const char *cpuid_7_0_ebx_feature_name[] = {
-"fsgsbase", "tsc_adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
+"fsgsbase", "tsc-adjust", NULL, "bmi1", "hle", "avx2", NULL, "smep",
 "bmi2", "erms", "invpcid", "rtm", NULL, NULL, "mpx", NULL,
 "avx512f", NULL, "rdseed", "adx", "smap", NULL, "pcommit", "clflushopt",
 "clwb", NULL, "avx512pf", "avx512er", "avx512cd", NULL, NULL, NULL,
@@ -1894,8 +1894,8 @@ static PropertyInfo qdev_prop_spinlocks = {
 .set   = x86_set_hv_spinlocks,
 };
 
-/* Convert all '_' in a feature string option name to '-', to make feature
- * name conform to QOM property naming rule, which uses '-' instead of '_'.
+/* Convert all '_' in a feature string option name to '-', to keep 
compatibility
+ * with old feature names that used "_" instead of "-".
  */
 static inline void feat2prop(char *s)
 {
@@ -1925,8 +1925,10 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char 
*features,
 while (featurestr) {
 char *val;
 if (featurestr[0] == '+') {
+feat2prop(featurestr);
 add_flagname_to_bitmaps(featurestr + 1, plus_features, _err);
 } else if (featurestr[0] == '-') {
+feat2prop(featurestr);
 add_flagname_to_bitmaps(featurestr + 1, minus_features, 
_err);
 } else if ((val = strchr(featurestr, '='))) {
 *val = 0; val++;
@@ -3137,11 +3139,9 @@ static void x86_cpu_register_feature_bit_props(X86CPU 
*cpu,
 
 names = g_strsplit(fi->feat_names[bitnr], "|", 0);
 
-feat2prop(names[0]);
 x86_cpu_register_bit_prop(cpu, names[0], >env.features[w], bitnr);
 
 for (i = 1; names[i]; i++) {
-feat2prop(names[i]);
 object_property_add_alias(obj, names[i], obj, names[0],