[for-2.11,06/27] x86: extract legacy cpu features format parser

Commit Message

Igor Mammedov Aug. 18, 2017, 10:08 a.m. UTC

Move cpu_model +-feat parsing into a separate file so that it
could be reused later for parsing similar format of sparc target

Signed-off-by: Igor Mammedov <[e-mail]>
---
CC: Richard Henderson <[e-mail]>
CC: Eduardo Habkost <[e-mail]>
CC: Mark Cave-Ayland <[e-mail]>
CC: Artyom Tarasenko <[e-mail]>
CC: Philippe Mathieu-Daudé <[e-mail]>
---
 include/qom/cpu.h                     |   2 +
 default-configs/i386-bsd-user.mak     |   1 +
 default-configs/i386-linux-user.mak   |   1 +
 default-configs/i386-softmmu.mak      |   1 +
 default-configs/x86_64-bsd-user.mak   |   1 +
 default-configs/x86_64-linux-user.mak |   1 +
 default-configs/x86_64-softmmu.mak    |   1 +
 target/i386/cpu.c                     | 125 +-------------------------
 util/Makefile.objs                    |   1 +
 util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
 10 files changed, 171 insertions(+), 124 deletions(-)
 create mode 100644 util/legacy_cpu_features_parser.c

Comments

Eduardo Habkost Aug. 18, 2017, 6:08 p.m. UTC | #1

On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> Move cpu_model +-feat parsing into a separate file so that it
> could be reused later for parsing similar format of sparc target
> 
> Signed-off-by: Igor Mammedov <[e-mail]>
[...]
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 25eefea..30247dc 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -1038,4 +1038,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
>  
>  #define UNASSIGNED_CPU_INDEX -1
>  
> +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> +                                 Error **errp);
>  #endif
> diff --git a/default-configs/i386-bsd-user.mak b/default-configs/i386-bsd-user.mak
> index af1b31a..b28a05f 100644
> --- a/default-configs/i386-bsd-user.mak
> +++ b/default-configs/i386-bsd-user.mak
> @@ -1 +1,2 @@
>  # Default configuration for i386-bsd-user
> +CONFIG_LEGACY_CPU_FEATURES=y
> diff --git a/default-configs/i386-linux-user.mak b/default-configs/i386-linux-user.mak
> index 8657e68..c136967 100644
> --- a/default-configs/i386-linux-user.mak
> +++ b/default-configs/i386-linux-user.mak
> @@ -1 +1,2 @@
>  # Default configuration for i386-linux-user
> +CONFIG_LEGACY_CPU_FEATURES=y
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index d2ab2f6..e3e7c0e 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
>  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>  CONFIG_PXB=y
>  CONFIG_ACPI_VMGENID=y
> +CONFIG_LEGACY_CPU_FEATURES=y
> diff --git a/default-configs/x86_64-bsd-user.mak b/default-configs/x86_64-bsd-user.mak
> index 73e5d34..952323c 100644
> --- a/default-configs/x86_64-bsd-user.mak
> +++ b/default-configs/x86_64-bsd-user.mak
> @@ -1 +1,2 @@
>  # Default configuration for x86_64-bsd-user
> +CONFIG_LEGACY_CPU_FEATURES=y
> diff --git a/default-configs/x86_64-linux-user.mak b/default-configs/x86_64-linux-user.mak
> index bec1d9e..b513ef2 100644
> --- a/default-configs/x86_64-linux-user.mak
> +++ b/default-configs/x86_64-linux-user.mak
> @@ -1 +1,2 @@
>  # Default configuration for x86_64-linux-user
> +CONFIG_LEGACY_CPU_FEATURES=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 9bde2f1..6594ddf 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
>  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>  CONFIG_PXB=y
>  CONFIG_ACPI_VMGENID=y
> +CONFIG_LEGACY_CPU_FEATURES=y
[...]
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 50a55ec..14e28f7 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -45,3 +45,4 @@ util-obj-y += qht.o
>  util-obj-y += range.o
>  util-obj-y += stats64.o
>  util-obj-y += systemd.o
> +util-obj-$(CONFIG_LEGACY_CPU_FEATURES) += legacy_cpu_features_parser.o

Do we really need this?  Isn't it easier to just tell the linker
to drop the object file if it's unused?

Igor Mammedov Aug. 21, 2017, 11:06 a.m. UTC | #2

On Fri, 18 Aug 2017 15:08:48 -0300
Eduardo Habkost <[e-mail]> wrote:

> On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> > Move cpu_model +-feat parsing into a separate file so that it
> > could be reused later for parsing similar format of sparc target
> > 
> > Signed-off-by: Igor Mammedov <[e-mail]>  
> [...]
> > diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> > index 25eefea..30247dc 100644
> > --- a/include/qom/cpu.h
> > +++ b/include/qom/cpu.h
> > @@ -1038,4 +1038,6 @@ extern const struct VMStateDescription vmstate_cpu_common;
> >  
> >  #define UNASSIGNED_CPU_INDEX -1
> >  
> > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > +                                 Error **errp);
> >  #endif
> > diff --git a/default-configs/i386-bsd-user.mak b/default-configs/i386-bsd-user.mak
> > index af1b31a..b28a05f 100644
> > --- a/default-configs/i386-bsd-user.mak
> > +++ b/default-configs/i386-bsd-user.mak
> > @@ -1 +1,2 @@
> >  # Default configuration for i386-bsd-user
> > +CONFIG_LEGACY_CPU_FEATURES=y
> > diff --git a/default-configs/i386-linux-user.mak b/default-configs/i386-linux-user.mak
> > index 8657e68..c136967 100644
> > --- a/default-configs/i386-linux-user.mak
> > +++ b/default-configs/i386-linux-user.mak
> > @@ -1 +1,2 @@
> >  # Default configuration for i386-linux-user
> > +CONFIG_LEGACY_CPU_FEATURES=y
> > diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> > index d2ab2f6..e3e7c0e 100644
> > --- a/default-configs/i386-softmmu.mak
> > +++ b/default-configs/i386-softmmu.mak
> > @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
> >  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> >  CONFIG_PXB=y
> >  CONFIG_ACPI_VMGENID=y
> > +CONFIG_LEGACY_CPU_FEATURES=y
> > diff --git a/default-configs/x86_64-bsd-user.mak b/default-configs/x86_64-bsd-user.mak
> > index 73e5d34..952323c 100644
> > --- a/default-configs/x86_64-bsd-user.mak
> > +++ b/default-configs/x86_64-bsd-user.mak
> > @@ -1 +1,2 @@
> >  # Default configuration for x86_64-bsd-user
> > +CONFIG_LEGACY_CPU_FEATURES=y
> > diff --git a/default-configs/x86_64-linux-user.mak b/default-configs/x86_64-linux-user.mak
> > index bec1d9e..b513ef2 100644
> > --- a/default-configs/x86_64-linux-user.mak
> > +++ b/default-configs/x86_64-linux-user.mak
> > @@ -1 +1,2 @@
> >  # Default configuration for x86_64-linux-user
> > +CONFIG_LEGACY_CPU_FEATURES=y
> > diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> > index 9bde2f1..6594ddf 100644
> > --- a/default-configs/x86_64-softmmu.mak
> > +++ b/default-configs/x86_64-softmmu.mak
> > @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
> >  CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> >  CONFIG_PXB=y
> >  CONFIG_ACPI_VMGENID=y
> > +CONFIG_LEGACY_CPU_FEATURES=y  
> [...]
> > diff --git a/util/Makefile.objs b/util/Makefile.objs
> > index 50a55ec..14e28f7 100644
> > --- a/util/Makefile.objs
> > +++ b/util/Makefile.objs
> > @@ -45,3 +45,4 @@ util-obj-y += qht.o
> >  util-obj-y += range.o
> >  util-obj-y += stats64.o
> >  util-obj-y += systemd.o
> > +util-obj-$(CONFIG_LEGACY_CPU_FEATURES) += legacy_cpu_features_parser.o  
> 
> Do we really need this?  Isn't it easier to just tell the linker
> to drop the object file if it's unused?
building unconditionally will make target limited builds to compile file uselessly,
I don't care much about it but opted for a less impact way on build time vs patch size.

Eduardo Habkost Aug. 23, 2017, 2:34 p.m. UTC | #3

On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> Move cpu_model +-feat parsing into a separate file so that it
> could be reused later for parsing similar format of sparc target
> 
> Signed-off-by: Igor Mammedov <[e-mail]>
> ---
> CC: Richard Henderson <[e-mail]>
> CC: Eduardo Habkost <[e-mail]>
> CC: Mark Cave-Ayland <[e-mail]>
> CC: Artyom Tarasenko <[e-mail]>
> CC: Philippe Mathieu-Daudé <[e-mail]>
> ---
>  include/qom/cpu.h                     |   2 +
>  default-configs/i386-bsd-user.mak     |   1 +
>  default-configs/i386-linux-user.mak   |   1 +
>  default-configs/i386-softmmu.mak      |   1 +
>  default-configs/x86_64-bsd-user.mak   |   1 +
>  default-configs/x86_64-linux-user.mak |   1 +
>  default-configs/x86_64-softmmu.mak    |   1 +
>  target/i386/cpu.c                     | 125 +-------------------------
>  util/Makefile.objs                    |   1 +
>  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
>  10 files changed, 171 insertions(+), 124 deletions(-)
>  create mode 100644 util/legacy_cpu_features_parser.c
> 
[...]
> diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> new file mode 100644
> index 0000000..6b352a3
> --- /dev/null
> +++ b/util/legacy_cpu_features_parser.c
> @@ -0,0 +1,161 @@
> +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> + * used by x86/sparc targets
> + *
> + * Author: Andreas Färber <[e-mail]>
> + * Author: Andre Przywara <[e-mail]>
> + * Author: Eduardo Habkost <[e-mail]>
> + * Author: Igor Mammedov <[e-mail]>
> + * Author: Paolo Bonzini <[e-mail]>
> + * Author: Markus Armbruster <[e-mail]>

IANAL, but I believe a
  Copyright (c) <YEAR> <COPYRIGHT HOLDER>
line is needed here.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qemu/cutils.h"
> +#include "qom/cpu.h"
> +#include "qemu/error-report.h"
> +#include "hw/qdev-properties.h"
> +
> +static inline void feat2prop(char *s)
> +{
> +    while ((s = strchr(s, '_'))) {
> +        *s = '-';
> +    }
> +}
> +
> +static gint compare_string(gconstpointer a, gconstpointer b)
> +{
> +    return g_strcmp0(a, b);
> +}
> +
> +static void
> +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> +{
> +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> +    prop->driver = typename;
> +    prop->property = g_strdup(name);
> +    prop->value = g_strdup(val);
> +    prop->errp = &error_fatal;
> +    qdev_prop_register_global(prop);
> +}
> +
> +/* DO NOT USE WITH NEW CODE
> + * Parse "+feature,-feature,feature=foo" CPU feature string
> + */
> +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> +                                 Error **errp)
> +{
> +    /* Compatibily hack to maintain legacy +-feat semantic,
> +     * where +-feat overwrites any feature set by
> +     * feat=on|feat even if the later is parsed after +-feat
> +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> +     */
> +    GList *l, *plus_features = NULL, *minus_features = NULL;
> +    char *featurestr; /* Single 'key=value" string being parsed */
> +    static bool cpu_globals_initialized;
> +    bool ambiguous = false;
> +
> +    if (cpu_globals_initialized) {
> +        return;
> +    }
> +    cpu_globals_initialized = true;
> +
> +    if (!features) {
> +        return;
> +    }
> +
> +    for (featurestr = strtok(features, ",");
> +         featurestr;
> +         featurestr = strtok(NULL, ",")) {
> +        const char *name;
> +        const char *val = NULL;
> +        char *eq = NULL;
> +        char num[32];
> +
> +        /* Compatibility syntax: */
> +        if (featurestr[0] == '+') {
> +            plus_features = g_list_append(plus_features,
> +                                          g_strdup(featurestr + 1));
> +            continue;
> +        } else if (featurestr[0] == '-') {
> +            minus_features = g_list_append(minus_features,
> +                                           g_strdup(featurestr + 1));
> +            continue;
> +        }

These 6 lines of code (or something equivalent to them) are
supposed to be the only difference to the generic parsing
function.  I would simply make this feature (support for
[+-]feature) enabled by a CPUClass::plus_minus_features flag
handled by cpu_common_parse_features().

(But this can be done as a follow-up patch.)

> +
> +        eq = strchr(featurestr, '=');
> +        if (eq) {
> +            *eq++ = 0;
> +            val = eq;
> +        } else {
> +            val = "on";
> +        }
> +
> +        feat2prop(featurestr);
> +        name = featurestr;
> +
> +        if (g_list_find_custom(plus_features, name, compare_string)) {
> +            warn_report("Ambiguous CPU model string. "
> +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> +                        name, name, val);
> +            ambiguous = true;
> +        }
> +        if (g_list_find_custom(minus_features, name, compare_string)) {
> +            warn_report("Ambiguous CPU model string. "
> +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> +                        name, name, val);
> +            ambiguous = true;
> +        }
> +
> +        /* Special case: */
> +        if (!strcmp(name, "tsc-freq")) {
> +            int ret;
> +            uint64_t tsc_freq;
> +
> +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> +            if (ret < 0 || tsc_freq > INT64_MAX) {
> +                error_setg(errp, "bad numerical value %s", val);
> +                return;
> +            }
> +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> +            val = num;
> +            name = "tsc-frequency";
> +        }

This is x86-specific and should stay in x86-specific code.  It
can probably be handled by the tsc-freq setter.

> +
> +        cpu_add_feat_as_prop(typename, name, val);
> +    }
> +
> +    if (ambiguous) {
> +        warn_report("Compatibility of ambiguous CPU model "
> +                    "strings won't be kept on future QEMU versions");
> +    }

As noted in the review of the x86 patch that removes the
plus_features/minus_features static variables, this obsolete (and
confusing) property ordering misfeature should be removed before
we make this code generic and reuse it on other architectures.

> +
> +    for (l = plus_features; l; l = l->next) {
> +        const char *name = l->data;
> +        cpu_add_feat_as_prop(typename, name, "on");
> +    }
> +    if (plus_features) {
> +        g_list_free_full(plus_features, g_free);
> +    }
> +
> +    for (l = minus_features; l; l = l->next) {
> +        const char *name = l->data;
> +        cpu_add_feat_as_prop(typename, name, "off");
> +    }
> +    if (minus_features) {
> +        g_list_free_full(minus_features, g_free);
> +    }
> +}
> -- 
> 2.7.4
> 
>

Igor Mammedov Aug. 23, 2017, 4:29 p.m. UTC | #4

On Wed, 23 Aug 2017 11:34:14 -0300
Eduardo Habkost <[e-mail]> wrote:

> On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> > Move cpu_model +-feat parsing into a separate file so that it
> > could be reused later for parsing similar format of sparc target
> > 
> > Signed-off-by: Igor Mammedov <[e-mail]>
> > ---
> > CC: Richard Henderson <[e-mail]>
> > CC: Eduardo Habkost <[e-mail]>
> > CC: Mark Cave-Ayland <[e-mail]>
> > CC: Artyom Tarasenko <[e-mail]>
> > CC: Philippe Mathieu-Daudé <[e-mail]>
> > ---
> >  include/qom/cpu.h                     |   2 +
> >  default-configs/i386-bsd-user.mak     |   1 +
> >  default-configs/i386-linux-user.mak   |   1 +
> >  default-configs/i386-softmmu.mak      |   1 +
> >  default-configs/x86_64-bsd-user.mak   |   1 +
> >  default-configs/x86_64-linux-user.mak |   1 +
> >  default-configs/x86_64-softmmu.mak    |   1 +
> >  target/i386/cpu.c                     | 125 +-------------------------
> >  util/Makefile.objs                    |   1 +
> >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> >  10 files changed, 171 insertions(+), 124 deletions(-)
> >  create mode 100644 util/legacy_cpu_features_parser.c
> >   
> [...]
> > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > new file mode 100644
> > index 0000000..6b352a3
> > --- /dev/null
> > +++ b/util/legacy_cpu_features_parser.c
> > @@ -0,0 +1,161 @@
> > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > + * used by x86/sparc targets
> > + *
> > + * Author: Andreas Färber <[e-mail]>
> > + * Author: Andre Przywara <[e-mail]>
> > + * Author: Eduardo Habkost <[e-mail]>
> > + * Author: Igor Mammedov <[e-mail]>
> > + * Author: Paolo Bonzini <[e-mail]>
> > + * Author: Markus Armbruster <[e-mail]>  
> 
> IANAL, but I believe a
>   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> line is needed here.
> 
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "qapi/error.h"
> > +#include "qemu/cutils.h"
> > +#include "qom/cpu.h"
> > +#include "qemu/error-report.h"
> > +#include "hw/qdev-properties.h"
> > +
> > +static inline void feat2prop(char *s)
> > +{
> > +    while ((s = strchr(s, '_'))) {
> > +        *s = '-';
> > +    }
> > +}
> > +
> > +static gint compare_string(gconstpointer a, gconstpointer b)
> > +{
> > +    return g_strcmp0(a, b);
> > +}
> > +
> > +static void
> > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > +{
> > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > +    prop->driver = typename;
> > +    prop->property = g_strdup(name);
> > +    prop->value = g_strdup(val);
> > +    prop->errp = &error_fatal;
> > +    qdev_prop_register_global(prop);
> > +}
> > +
> > +/* DO NOT USE WITH NEW CODE
> > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > + */
> > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > +                                 Error **errp)
> > +{
> > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > +     * where +-feat overwrites any feature set by
> > +     * feat=on|feat even if the later is parsed after +-feat
> > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > +     */
> > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > +    char *featurestr; /* Single 'key=value" string being parsed */
> > +    static bool cpu_globals_initialized;
> > +    bool ambiguous = false;
> > +
> > +    if (cpu_globals_initialized) {
> > +        return;
> > +    }
> > +    cpu_globals_initialized = true;
> > +
> > +    if (!features) {
> > +        return;
> > +    }
> > +
> > +    for (featurestr = strtok(features, ",");
> > +         featurestr;
> > +         featurestr = strtok(NULL, ",")) {
> > +        const char *name;
> > +        const char *val = NULL;
> > +        char *eq = NULL;
> > +        char num[32];
> > +
> > +        /* Compatibility syntax: */
> > +        if (featurestr[0] == '+') {
> > +            plus_features = g_list_append(plus_features,
> > +                                          g_strdup(featurestr + 1));
> > +            continue;
> > +        } else if (featurestr[0] == '-') {
> > +            minus_features = g_list_append(minus_features,
> > +                                           g_strdup(featurestr + 1));
> > +            continue;
> > +        }  
> 
> These 6 lines of code (or something equivalent to them) are
> supposed to be the only difference to the generic parsing
> function.  I would simply make this feature (support for
> [+-]feature) enabled by a CPUClass::plus_minus_features flag
> handled by cpu_common_parse_features().
I'd rather keep plus/minus nonsense  under the hood separate
legacy function so it get reused unintentionally and keep generic
parser clean.

I didn't have any intent of generalizing +-feat handling
but rather to remove code duplication between the only users
(x86/sparc) that happened to use syntax and share the same semantics.

As an alternative I can copy-past x86 variant into sparc
(modulo x86 harmless fixups), that will add some code duplication
I've tried to avoid with this patch, but it won't cause
misunderstanding about generalizing legacy hacks.

> (But this can be done as a follow-up patch.)
> 
> > +
> > +        eq = strchr(featurestr, '=');
> > +        if (eq) {
> > +            *eq++ = 0;
> > +            val = eq;
> > +        } else {
> > +            val = "on";
> > +        }
> > +
> > +        feat2prop(featurestr);
> > +        name = featurestr;
> > +
> > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > +            warn_report("Ambiguous CPU model string. "
> > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > +                        name, name, val);
> > +            ambiguous = true;
> > +        }
> > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > +            warn_report("Ambiguous CPU model string. "
> > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > +                        name, name, val);
> > +            ambiguous = true;
> > +        }
> > +
> > +        /* Special case: */
> > +        if (!strcmp(name, "tsc-freq")) {
> > +            int ret;
> > +            uint64_t tsc_freq;
> > +
> > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > +                error_setg(errp, "bad numerical value %s", val);
> > +                return;
> > +            }
> > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > +            val = num;
> > +            name = "tsc-frequency";
> > +        }  
> 
> This is x86-specific and should stay in x86-specific code.  It
> can probably be handled by the tsc-freq setter.
there was reason why it wasn't moved to tsc-frequency setter,
the former is pure integer type of property,
while here we can get suffixed string that scales by 1000.

Short of creating new visitor for KHz (I don't really looking forward to it),
it's simpler to leave fixup alone in legacy parser that's shared only between
x86/sparc as it doesn't conflict with sparc and won't break anything.


> > +
> > +        cpu_add_feat_as_prop(typename, name, val);
> > +    }
> > +
> > +    if (ambiguous) {
> > +        warn_report("Compatibility of ambiguous CPU model "
> > +                    "strings won't be kept on future QEMU versions");
> > +    }  
> 
> As noted in the review of the x86 patch that removes the
> plus_features/minus_features static variables, this obsolete (and
> confusing) property ordering misfeature should be removed before
> we make this code generic and reuse it on other architectures.
As it's been replied removing ordering is behavioral change for
both x86 and sparc, which is not related to series.
If you wish, I'll post a patch that will what you suggest
on top of series.


> > +
> > +    for (l = plus_features; l; l = l->next) {
> > +        const char *name = l->data;
> > +        cpu_add_feat_as_prop(typename, name, "on");
> > +    }
> > +    if (plus_features) {
> > +        g_list_free_full(plus_features, g_free);
> > +    }
> > +
> > +    for (l = minus_features; l; l = l->next) {
> > +        const char *name = l->data;
> > +        cpu_add_feat_as_prop(typename, name, "off");
> > +    }
> > +    if (minus_features) {
> > +        g_list_free_full(minus_features, g_free);
> > +    }
> > +}
> > -- 
> > 2.7.4
> > 
> >   
>

Eduardo Habkost Aug. 23, 2017, 4:46 p.m. UTC | #5

On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:
> On Wed, 23 Aug 2017 11:34:14 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> > > Move cpu_model +-feat parsing into a separate file so that it
> > > could be reused later for parsing similar format of sparc target
> > > 
> > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > ---
> > > CC: Richard Henderson <[e-mail]>
> > > CC: Eduardo Habkost <[e-mail]>
> > > CC: Mark Cave-Ayland <[e-mail]>
> > > CC: Artyom Tarasenko <[e-mail]>
> > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > ---
> > >  include/qom/cpu.h                     |   2 +
> > >  default-configs/i386-bsd-user.mak     |   1 +
> > >  default-configs/i386-linux-user.mak   |   1 +
> > >  default-configs/i386-softmmu.mak      |   1 +
> > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > >  default-configs/x86_64-linux-user.mak |   1 +
> > >  default-configs/x86_64-softmmu.mak    |   1 +
> > >  target/i386/cpu.c                     | 125 +-------------------------
> > >  util/Makefile.objs                    |   1 +
> > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > >  create mode 100644 util/legacy_cpu_features_parser.c
> > >   
> > [...]
> > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > new file mode 100644
> > > index 0000000..6b352a3
> > > --- /dev/null
> > > +++ b/util/legacy_cpu_features_parser.c
> > > @@ -0,0 +1,161 @@
> > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > + * used by x86/sparc targets
> > > + *
> > > + * Author: Andreas Färber <[e-mail]>
> > > + * Author: Andre Przywara <[e-mail]>
> > > + * Author: Eduardo Habkost <[e-mail]>
> > > + * Author: Igor Mammedov <[e-mail]>
> > > + * Author: Paolo Bonzini <[e-mail]>
> > > + * Author: Markus Armbruster <[e-mail]>  
> > 
> > IANAL, but I believe a
> >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > line is needed here.
> > 
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > +
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > +
> > > + * You should have received a copy of the GNU General Public License along
> > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "qapi/error.h"
> > > +#include "qemu/cutils.h"
> > > +#include "qom/cpu.h"
> > > +#include "qemu/error-report.h"
> > > +#include "hw/qdev-properties.h"
> > > +
> > > +static inline void feat2prop(char *s)
> > > +{
> > > +    while ((s = strchr(s, '_'))) {
> > > +        *s = '-';
> > > +    }
> > > +}
> > > +
> > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > +{
> > > +    return g_strcmp0(a, b);
> > > +}
> > > +
> > > +static void
> > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > +{
> > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > +    prop->driver = typename;
> > > +    prop->property = g_strdup(name);
> > > +    prop->value = g_strdup(val);
> > > +    prop->errp = &error_fatal;
> > > +    qdev_prop_register_global(prop);
> > > +}
> > > +
> > > +/* DO NOT USE WITH NEW CODE
> > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > + */
> > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > +                                 Error **errp)
> > > +{
> > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > +     * where +-feat overwrites any feature set by
> > > +     * feat=on|feat even if the later is parsed after +-feat
> > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > +     */
> > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > +    static bool cpu_globals_initialized;
> > > +    bool ambiguous = false;
> > > +
> > > +    if (cpu_globals_initialized) {
> > > +        return;
> > > +    }
> > > +    cpu_globals_initialized = true;
> > > +
> > > +    if (!features) {
> > > +        return;
> > > +    }
> > > +
> > > +    for (featurestr = strtok(features, ",");
> > > +         featurestr;
> > > +         featurestr = strtok(NULL, ",")) {
> > > +        const char *name;
> > > +        const char *val = NULL;
> > > +        char *eq = NULL;
> > > +        char num[32];
> > > +
> > > +        /* Compatibility syntax: */
> > > +        if (featurestr[0] == '+') {
> > > +            plus_features = g_list_append(plus_features,
> > > +                                          g_strdup(featurestr + 1));
> > > +            continue;
> > > +        } else if (featurestr[0] == '-') {
> > > +            minus_features = g_list_append(minus_features,
> > > +                                           g_strdup(featurestr + 1));
> > > +            continue;
> > > +        }  
> > 
> > These 6 lines of code (or something equivalent to them) are
> > supposed to be the only difference to the generic parsing
> > function.  I would simply make this feature (support for
> > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > handled by cpu_common_parse_features().
> I'd rather keep plus/minus nonsense  under the hood separate
> legacy function so it get reused unintentionally and keep generic
> parser clean.
> 
> I didn't have any intent of generalizing +-feat handling
> but rather to remove code duplication between the only users
> (x86/sparc) that happened to use syntax and share the same semantics.

Generalizing it to be controlled by a CPUClass flag will make it
easier to refactor the feature parsing later to use the QemuOpts
parser.  But I agree there's no need to do that on this series.

(We might even decide to make [+-]feat work on all other
architectures.  We already agreed recently that we won't
deprecate it in x86, we could as well enable the same syntax
uniformly across all architectures.)

> 
> As an alternative I can copy-past x86 variant into sparc
> (modulo x86 harmless fixups), that will add some code duplication
> I've tried to avoid with this patch, but it won't cause
> misunderstanding about generalizing legacy hacks.

Works for me.  We can then cleanup the x86 code and make it use
cpu_legacy_parse_featurestr() later.

> 
> > (But this can be done as a follow-up patch.)
> > 
> > > +
> > > +        eq = strchr(featurestr, '=');
> > > +        if (eq) {
> > > +            *eq++ = 0;
> > > +            val = eq;
> > > +        } else {
> > > +            val = "on";
> > > +        }
> > > +
> > > +        feat2prop(featurestr);
> > > +        name = featurestr;
> > > +
> > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > +            warn_report("Ambiguous CPU model string. "
> > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > +                        name, name, val);
> > > +            ambiguous = true;
> > > +        }
> > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > +            warn_report("Ambiguous CPU model string. "
> > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > +                        name, name, val);
> > > +            ambiguous = true;
> > > +        }
> > > +
> > > +        /* Special case: */
> > > +        if (!strcmp(name, "tsc-freq")) {
> > > +            int ret;
> > > +            uint64_t tsc_freq;
> > > +
> > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > +                error_setg(errp, "bad numerical value %s", val);
> > > +                return;
> > > +            }
> > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > +            val = num;
> > > +            name = "tsc-frequency";
> > > +        }  
> > 
> > This is x86-specific and should stay in x86-specific code.  It
> > can probably be handled by the tsc-freq setter.
> there was reason why it wasn't moved to tsc-frequency setter,
> the former is pure integer type of property,
> while here we can get suffixed string that scales by 1000.
> 
> Short of creating new visitor for KHz (I don't really looking forward to it),
> it's simpler to leave fixup alone in legacy parser that's shared only between
> x86/sparc as it doesn't conflict with sparc and won't break anything.

It doesn't break anything, but it will move x86-specific cruft to
code that is supposed to be generic.

Creating a write-only "tsc-freq" property that accepts a string
isn't hard to do.

If you are not willing to do it in this series, you can write a
generic parser now (without x86-specific cruft), use it only on
sparc, and later we can refactor x86 so it can also use the
generic one.

> 
> 
> > > +
> > > +        cpu_add_feat_as_prop(typename, name, val);
> > > +    }
> > > +
> > > +    if (ambiguous) {
> > > +        warn_report("Compatibility of ambiguous CPU model "
> > > +                    "strings won't be kept on future QEMU versions");
> > > +    }  
> > 
> > As noted in the review of the x86 patch that removes the
> > plus_features/minus_features static variables, this obsolete (and
> > confusing) property ordering misfeature should be removed before
> > we make this code generic and reuse it on other architectures.
> As it's been replied removing ordering is behavioral change for
> both x86 and sparc, which is not related to series.
> If you wish, I'll post a patch that will what you suggest
> on top of series.

I would agree if sparc also implemented the weird ordering.  But
sparc does not implement it (it doesn't support feat=(on|off)
yet).  We shouldn't introduce that misfeature in sparc if we're
already planning to remove it.

Igor Mammedov Aug. 23, 2017, 5:37 p.m. UTC | #6

On Wed, 23 Aug 2017 13:46:38 -0300
Eduardo Habkost <[e-mail]> wrote:

> On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:
> > On Wed, 23 Aug 2017 11:34:14 -0300
> > Eduardo Habkost <[e-mail]> wrote:
> > 
> > > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> > > > Move cpu_model +-feat parsing into a separate file so that it
> > > > could be reused later for parsing similar format of sparc target
> > > > 
> > > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > > ---
> > > > CC: Richard Henderson <[e-mail]>
> > > > CC: Eduardo Habkost <[e-mail]>
> > > > CC: Mark Cave-Ayland <[e-mail]>
> > > > CC: Artyom Tarasenko <[e-mail]>
> > > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > > ---
> > > >  include/qom/cpu.h                     |   2 +
> > > >  default-configs/i386-bsd-user.mak     |   1 +
> > > >  default-configs/i386-linux-user.mak   |   1 +
> > > >  default-configs/i386-softmmu.mak      |   1 +
> > > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > > >  default-configs/x86_64-linux-user.mak |   1 +
> > > >  default-configs/x86_64-softmmu.mak    |   1 +
> > > >  target/i386/cpu.c                     | 125 +-------------------------
> > > >  util/Makefile.objs                    |   1 +
> > > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > > >  create mode 100644 util/legacy_cpu_features_parser.c
> > > >   
> > > [...]
> > > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > > new file mode 100644
> > > > index 0000000..6b352a3
> > > > --- /dev/null
> > > > +++ b/util/legacy_cpu_features_parser.c
> > > > @@ -0,0 +1,161 @@
> > > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > > + * used by x86/sparc targets
> > > > + *
> > > > + * Author: Andreas Färber <[e-mail]>
> > > > + * Author: Andre Przywara <[e-mail]>
> > > > + * Author: Eduardo Habkost <[e-mail]>
> > > > + * Author: Igor Mammedov <[e-mail]>
> > > > + * Author: Paolo Bonzini <[e-mail]>
> > > > + * Author: Markus Armbruster <[e-mail]>  
> > > 
> > > IANAL, but I believe a
> > >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > > line is needed here.
> > > 
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or modify
> > > > + * it under the terms of the GNU General Public License as published by
> > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > + * (at your option) any later version.
> > > > +
> > > > + * This program is distributed in the hope that it will be useful,
> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > + * GNU General Public License for more details.
> > > > +
> > > > + * You should have received a copy of the GNU General Public License along
> > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > + */
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "qapi/error.h"
> > > > +#include "qemu/cutils.h"
> > > > +#include "qom/cpu.h"
> > > > +#include "qemu/error-report.h"
> > > > +#include "hw/qdev-properties.h"
> > > > +
> > > > +static inline void feat2prop(char *s)
> > > > +{
> > > > +    while ((s = strchr(s, '_'))) {
> > > > +        *s = '-';
> > > > +    }
> > > > +}
> > > > +
> > > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > > +{
> > > > +    return g_strcmp0(a, b);
> > > > +}
> > > > +
> > > > +static void
> > > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > > +{
> > > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > > +    prop->driver = typename;
> > > > +    prop->property = g_strdup(name);
> > > > +    prop->value = g_strdup(val);
> > > > +    prop->errp = &error_fatal;
> > > > +    qdev_prop_register_global(prop);
> > > > +}
> > > > +
> > > > +/* DO NOT USE WITH NEW CODE
> > > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > > + */
> > > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > > +                                 Error **errp)
> > > > +{
> > > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > > +     * where +-feat overwrites any feature set by
> > > > +     * feat=on|feat even if the later is parsed after +-feat
> > > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > > +     */
> > > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > > +    static bool cpu_globals_initialized;
> > > > +    bool ambiguous = false;
> > > > +
> > > > +    if (cpu_globals_initialized) {
> > > > +        return;
> > > > +    }
> > > > +    cpu_globals_initialized = true;
> > > > +
> > > > +    if (!features) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    for (featurestr = strtok(features, ",");
> > > > +         featurestr;
> > > > +         featurestr = strtok(NULL, ",")) {
> > > > +        const char *name;
> > > > +        const char *val = NULL;
> > > > +        char *eq = NULL;
> > > > +        char num[32];
> > > > +
> > > > +        /* Compatibility syntax: */
> > > > +        if (featurestr[0] == '+') {
> > > > +            plus_features = g_list_append(plus_features,
> > > > +                                          g_strdup(featurestr + 1));
> > > > +            continue;
> > > > +        } else if (featurestr[0] == '-') {
> > > > +            minus_features = g_list_append(minus_features,
> > > > +                                           g_strdup(featurestr + 1));
> > > > +            continue;
> > > > +        }  
> > > 
> > > These 6 lines of code (or something equivalent to them) are
> > > supposed to be the only difference to the generic parsing
> > > function.  I would simply make this feature (support for
> > > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > > handled by cpu_common_parse_features().
> > I'd rather keep plus/minus nonsense  under the hood separate
> > legacy function so it get reused unintentionally and keep generic
> > parser clean.
> > 
> > I didn't have any intent of generalizing +-feat handling
> > but rather to remove code duplication between the only users
> > (x86/sparc) that happened to use syntax and share the same semantics.
> 
> Generalizing it to be controlled by a CPUClass flag will make it
> easier to refactor the feature parsing later to use the QemuOpts
> parser.  But I agree there's no need to do that on this series.
> 
> (We might even decide to make [+-]feat work on all other
> architectures.  We already agreed recently that we won't
> deprecate it in x86, we could as well enable the same syntax
> uniformly across all architectures.)
I'd just stick to canonical feat=on|off and keep legacy to x86|sparc,
but it's this to discuss if future and relevant here.

> 
> > 
> > As an alternative I can copy-past x86 variant into sparc
> > (modulo x86 harmless fixups), that will add some code duplication
> > I've tried to avoid with this patch, but it won't cause
> > misunderstanding about generalizing legacy hacks.
> 
> Works for me.  We can then cleanup the x86 code and make it use
> cpu_legacy_parse_featurestr() later.
Then, I'll just copy and replace sparc variant with x86 impl.
(removing from the copy x86 only parts and making parser stricter
where possible) and drop 5-6 patches that touched x86 for the purpose
of sharing code.

and respin v3 tomorrow.

> 
> > 
> > > (But this can be done as a follow-up patch.)
> > > 
> > > > +
> > > > +        eq = strchr(featurestr, '=');
> > > > +        if (eq) {
> > > > +            *eq++ = 0;
> > > > +            val = eq;
> > > > +        } else {
> > > > +            val = "on";
> > > > +        }
> > > > +
> > > > +        feat2prop(featurestr);
> > > > +        name = featurestr;
> > > > +
> > > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > > +            warn_report("Ambiguous CPU model string. "
> > > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > > +                        name, name, val);
> > > > +            ambiguous = true;
> > > > +        }
> > > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > > +            warn_report("Ambiguous CPU model string. "
> > > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > > +                        name, name, val);
> > > > +            ambiguous = true;
> > > > +        }
> > > > +
> > > > +        /* Special case: */
> > > > +        if (!strcmp(name, "tsc-freq")) {
> > > > +            int ret;
> > > > +            uint64_t tsc_freq;
> > > > +
> > > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > > +                error_setg(errp, "bad numerical value %s", val);
> > > > +                return;
> > > > +            }
> > > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > > +            val = num;
> > > > +            name = "tsc-frequency";
> > > > +        }  
> > > 
> > > This is x86-specific and should stay in x86-specific code.  It
> > > can probably be handled by the tsc-freq setter.
> > there was reason why it wasn't moved to tsc-frequency setter,
> > the former is pure integer type of property,
> > while here we can get suffixed string that scales by 1000.
> > 
> > Short of creating new visitor for KHz (I don't really looking forward to it),
> > it's simpler to leave fixup alone in legacy parser that's shared only between
> > x86/sparc as it doesn't conflict with sparc and won't break anything.
> 
> It doesn't break anything, but it will move x86-specific cruft to
> code that is supposed to be generic.
> 
> Creating a write-only "tsc-freq" property that accepts a string
> isn't hard to do.
Nice idea, it might just work.

> 
> If you are not willing to do it in this series, you can write a
> generic parser now (without x86-specific cruft), use it only on
> sparc, and later we can refactor x86 so it can also use the
> generic one.
So far, I'm inclined towards opposing +-feat generalizing and keeping
canonical form anywhere except of x86/sparc were we have to tolerate it.


> > > > +
> > > > +        cpu_add_feat_as_prop(typename, name, val);
> > > > +    }
> > > > +
> > > > +    if (ambiguous) {
> > > > +        warn_report("Compatibility of ambiguous CPU model "
> > > > +                    "strings won't be kept on future QEMU versions");
> > > > +    }  
> > > 
> > > As noted in the review of the x86 patch that removes the
> > > plus_features/minus_features static variables, this obsolete (and
> > > confusing) property ordering misfeature should be removed before
> > > we make this code generic and reuse it on other architectures.
> > As it's been replied removing ordering is behavioral change for
> > both x86 and sparc, which is not related to series.
> > If you wish, I'll post a patch that will what you suggest
> > on top of series.
> 
> I would agree if sparc also implemented the weird ordering.  But
> sparc does not implement it (it doesn't support feat=(on|off)
> yet).  We shouldn't introduce that misfeature in sparc if we're
> already planning to remove it.
It didn't have canonic form but it gains this ability with this series.
So we could do better for it by forbidding mixed syntax from starters
(+1 for copy-past) but we have to keep minus overrides plus semantics,
so ambiguous check is still there but it would lead to hard error instead of
warning.

Eduardo Habkost Aug. 23, 2017, 5:58 p.m. UTC | #7

On Wed, Aug 23, 2017 at 07:37:39PM +0200, Igor Mammedov wrote:
> On Wed, 23 Aug 2017 13:46:38 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:
> > > On Wed, 23 Aug 2017 11:34:14 -0300
> > > Eduardo Habkost <[e-mail]> wrote:
> > > 
> > > > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:
> > > > > Move cpu_model +-feat parsing into a separate file so that it
> > > > > could be reused later for parsing similar format of sparc target
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > > > ---
> > > > > CC: Richard Henderson <[e-mail]>
> > > > > CC: Eduardo Habkost <[e-mail]>
> > > > > CC: Mark Cave-Ayland <[e-mail]>
> > > > > CC: Artyom Tarasenko <[e-mail]>
> > > > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > > > ---
> > > > >  include/qom/cpu.h                     |   2 +
> > > > >  default-configs/i386-bsd-user.mak     |   1 +
> > > > >  default-configs/i386-linux-user.mak   |   1 +
> > > > >  default-configs/i386-softmmu.mak      |   1 +
> > > > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > > > >  default-configs/x86_64-linux-user.mak |   1 +
> > > > >  default-configs/x86_64-softmmu.mak    |   1 +
> > > > >  target/i386/cpu.c                     | 125 +-------------------------
> > > > >  util/Makefile.objs                    |   1 +
> > > > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > > > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > > > >  create mode 100644 util/legacy_cpu_features_parser.c
> > > > >   
> > > > [...]
> > > > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > > > new file mode 100644
> > > > > index 0000000..6b352a3
> > > > > --- /dev/null
> > > > > +++ b/util/legacy_cpu_features_parser.c
> > > > > @@ -0,0 +1,161 @@
> > > > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > > > + * used by x86/sparc targets
> > > > > + *
> > > > > + * Author: Andreas Färber <[e-mail]>
> > > > > + * Author: Andre Przywara <[e-mail]>
> > > > > + * Author: Eduardo Habkost <[e-mail]>
> > > > > + * Author: Igor Mammedov <[e-mail]>
> > > > > + * Author: Paolo Bonzini <[e-mail]>
> > > > > + * Author: Markus Armbruster <[e-mail]>  
> > > > 
> > > > IANAL, but I believe a
> > > >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > > > line is needed here.
> > > > 
> > > > > + *
> > > > > + * This program is free software; you can redistribute it and/or modify
> > > > > + * it under the terms of the GNU General Public License as published by
> > > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > > + * (at your option) any later version.
> > > > > +
> > > > > + * This program is distributed in the hope that it will be useful,
> > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > + * GNU General Public License for more details.
> > > > > +
> > > > > + * You should have received a copy of the GNU General Public License along
> > > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > > + */
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "qapi/error.h"
> > > > > +#include "qemu/cutils.h"
> > > > > +#include "qom/cpu.h"
> > > > > +#include "qemu/error-report.h"
> > > > > +#include "hw/qdev-properties.h"
> > > > > +
> > > > > +static inline void feat2prop(char *s)
> > > > > +{
> > > > > +    while ((s = strchr(s, '_'))) {
> > > > > +        *s = '-';
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > > > +{
> > > > > +    return g_strcmp0(a, b);
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > > > +{
> > > > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > > > +    prop->driver = typename;
> > > > > +    prop->property = g_strdup(name);
> > > > > +    prop->value = g_strdup(val);
> > > > > +    prop->errp = &error_fatal;
> > > > > +    qdev_prop_register_global(prop);
> > > > > +}
> > > > > +
> > > > > +/* DO NOT USE WITH NEW CODE
> > > > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > > > + */
> > > > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > > > +                                 Error **errp)
> > > > > +{
> > > > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > > > +     * where +-feat overwrites any feature set by
> > > > > +     * feat=on|feat even if the later is parsed after +-feat
> > > > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > > > +     */
> > > > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > > > +    static bool cpu_globals_initialized;
> > > > > +    bool ambiguous = false;
> > > > > +
> > > > > +    if (cpu_globals_initialized) {
> > > > > +        return;
> > > > > +    }
> > > > > +    cpu_globals_initialized = true;
> > > > > +
> > > > > +    if (!features) {
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    for (featurestr = strtok(features, ",");
> > > > > +         featurestr;
> > > > > +         featurestr = strtok(NULL, ",")) {
> > > > > +        const char *name;
> > > > > +        const char *val = NULL;
> > > > > +        char *eq = NULL;
> > > > > +        char num[32];
> > > > > +
> > > > > +        /* Compatibility syntax: */
> > > > > +        if (featurestr[0] == '+') {
> > > > > +            plus_features = g_list_append(plus_features,
> > > > > +                                          g_strdup(featurestr + 1));
> > > > > +            continue;
> > > > > +        } else if (featurestr[0] == '-') {
> > > > > +            minus_features = g_list_append(minus_features,
> > > > > +                                           g_strdup(featurestr + 1));
> > > > > +            continue;
> > > > > +        }  
> > > > 
> > > > These 6 lines of code (or something equivalent to them) are
> > > > supposed to be the only difference to the generic parsing
> > > > function.  I would simply make this feature (support for
> > > > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > > > handled by cpu_common_parse_features().
> > > I'd rather keep plus/minus nonsense  under the hood separate
> > > legacy function so it get reused unintentionally and keep generic
> > > parser clean.
> > > 
> > > I didn't have any intent of generalizing +-feat handling
> > > but rather to remove code duplication between the only users
> > > (x86/sparc) that happened to use syntax and share the same semantics.
> > 
> > Generalizing it to be controlled by a CPUClass flag will make it
> > easier to refactor the feature parsing later to use the QemuOpts
> > parser.  But I agree there's no need to do that on this series.
> > 
> > (We might even decide to make [+-]feat work on all other
> > architectures.  We already agreed recently that we won't
> > deprecate it in x86, we could as well enable the same syntax
> > uniformly across all architectures.)
> I'd just stick to canonical feat=on|off and keep legacy to x86|sparc,
> but it's this to discuss if future and relevant here.
> 
> > 
> > > 
> > > As an alternative I can copy-past x86 variant into sparc
> > > (modulo x86 harmless fixups), that will add some code duplication
> > > I've tried to avoid with this patch, but it won't cause
> > > misunderstanding about generalizing legacy hacks.
> > 
> > Works for me.  We can then cleanup the x86 code and make it use
> > cpu_legacy_parse_featurestr() later.
> Then, I'll just copy and replace sparc variant with x86 impl.
> (removing from the copy x86 only parts and making parser stricter
> where possible) and drop 5-6 patches that touched x86 for the purpose
> of sharing code.
> 
> and respin v3 tomorrow.

I just have one worry about this plan, below:

> 
> > 
> > > 
> > > > (But this can be done as a follow-up patch.)
> > > > 
> > > > > +
> > > > > +        eq = strchr(featurestr, '=');
> > > > > +        if (eq) {
> > > > > +            *eq++ = 0;
> > > > > +            val = eq;
> > > > > +        } else {
> > > > > +            val = "on";
> > > > > +        }
> > > > > +
> > > > > +        feat2prop(featurestr);
> > > > > +        name = featurestr;
> > > > > +
> > > > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > > > +                        name, name, val);
> > > > > +            ambiguous = true;
> > > > > +        }
> > > > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > > > +                        name, name, val);
> > > > > +            ambiguous = true;
> > > > > +        }
> > > > > +
> > > > > +        /* Special case: */
> > > > > +        if (!strcmp(name, "tsc-freq")) {
> > > > > +            int ret;
> > > > > +            uint64_t tsc_freq;
> > > > > +
> > > > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > > > +                error_setg(errp, "bad numerical value %s", val);
> > > > > +                return;
> > > > > +            }
> > > > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > > > +            val = num;
> > > > > +            name = "tsc-frequency";
> > > > > +        }  
> > > > 
> > > > This is x86-specific and should stay in x86-specific code.  It
> > > > can probably be handled by the tsc-freq setter.
> > > there was reason why it wasn't moved to tsc-frequency setter,
> > > the former is pure integer type of property,
> > > while here we can get suffixed string that scales by 1000.
> > > 
> > > Short of creating new visitor for KHz (I don't really looking forward to it),
> > > it's simpler to leave fixup alone in legacy parser that's shared only between
> > > x86/sparc as it doesn't conflict with sparc and won't break anything.
> > 
> > It doesn't break anything, but it will move x86-specific cruft to
> > code that is supposed to be generic.
> > 
> > Creating a write-only "tsc-freq" property that accepts a string
> > isn't hard to do.
> Nice idea, it might just work.
> 
> > 
> > If you are not willing to do it in this series, you can write a
> > generic parser now (without x86-specific cruft), use it only on
> > sparc, and later we can refactor x86 so it can also use the
> > generic one.
> So far, I'm inclined towards opposing +-feat generalizing and keeping
> canonical form anywhere except of x86/sparc were we have to tolerate it.
> 
> 
> > > > > +
> > > > > +        cpu_add_feat_as_prop(typename, name, val);
> > > > > +    }
> > > > > +
> > > > > +    if (ambiguous) {
> > > > > +        warn_report("Compatibility of ambiguous CPU model "
> > > > > +                    "strings won't be kept on future QEMU versions");
> > > > > +    }  
> > > > 
> > > > As noted in the review of the x86 patch that removes the
> > > > plus_features/minus_features static variables, this obsolete (and
> > > > confusing) property ordering misfeature should be removed before
> > > > we make this code generic and reuse it on other architectures.
> > > As it's been replied removing ordering is behavioral change for
> > > both x86 and sparc, which is not related to series.
> > > If you wish, I'll post a patch that will what you suggest
> > > on top of series.
> > 
> > I would agree if sparc also implemented the weird ordering.  But
> > sparc does not implement it (it doesn't support feat=(on|off)
> > yet).  We shouldn't introduce that misfeature in sparc if we're
> > already planning to remove it.
> It didn't have canonic form but it gains this ability with this series.
> So we could do better for it by forbidding mixed syntax from starters

We don't need to forbid mixed syntax.  We can simply apply
[+-]feat and feat=on|off in the same order they appear in the
command-line.

But:

> (+1 for copy-past) but we have to keep minus overrides plus semantics,
> so ambiguous check is still there but it would lead to hard error instead of
> warning.

I forgot about this additional weird semantics
(minus-override-plus).  :(

I think we must deprecate the minus-override-plus semantics too
and move to command-line-order eventually (on both x86 and
sparc).  But to do that, we need to make the code print a warning
on sparc like we do on x86.

If we don't remove that weird semantics before making sparc
support feat=on|off, we will have to worry about the semantics of
"-feat,feat=on" on top of that.  I would simply wait for 2
releases and remove minus-override-plus, before implementing
feat=on|off on sparc, to avoid creating a new set of problems.
Is support for feat=on|off on sparc a must-have for QEMU 2.11?

Igor Mammedov Aug. 24, 2017, 9:18 a.m. UTC | #8

On Wed, 23 Aug 2017 14:58:39 -0300
Eduardo Habkost <[e-mail]> wrote:

> On Wed, Aug 23, 2017 at 07:37:39PM +0200, Igor Mammedov wrote:
> > On Wed, 23 Aug 2017 13:46:38 -0300
> > Eduardo Habkost <[e-mail]> wrote:
> >   
> > > On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:  
> > > > On Wed, 23 Aug 2017 11:34:14 -0300
> > > > Eduardo Habkost <[e-mail]> wrote:
> > > >   
> > > > > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:  
> > > > > > Move cpu_model +-feat parsing into a separate file so that it
> > > > > > could be reused later for parsing similar format of sparc target
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > > > > ---
> > > > > > CC: Richard Henderson <[e-mail]>
> > > > > > CC: Eduardo Habkost <[e-mail]>
> > > > > > CC: Mark Cave-Ayland <[e-mail]>
> > > > > > CC: Artyom Tarasenko <[e-mail]>
> > > > > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > > > > ---
> > > > > >  include/qom/cpu.h                     |   2 +
> > > > > >  default-configs/i386-bsd-user.mak     |   1 +
> > > > > >  default-configs/i386-linux-user.mak   |   1 +
> > > > > >  default-configs/i386-softmmu.mak      |   1 +
> > > > > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > > > > >  default-configs/x86_64-linux-user.mak |   1 +
> > > > > >  default-configs/x86_64-softmmu.mak    |   1 +
> > > > > >  target/i386/cpu.c                     | 125 +-------------------------
> > > > > >  util/Makefile.objs                    |   1 +
> > > > > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > > > > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > > > > >  create mode 100644 util/legacy_cpu_features_parser.c
> > > > > >     
> > > > > [...]  
> > > > > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > > > > new file mode 100644
> > > > > > index 0000000..6b352a3
> > > > > > --- /dev/null
> > > > > > +++ b/util/legacy_cpu_features_parser.c
> > > > > > @@ -0,0 +1,161 @@
> > > > > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > > > > + * used by x86/sparc targets
> > > > > > + *
> > > > > > + * Author: Andreas Färber <[e-mail]>
> > > > > > + * Author: Andre Przywara <[e-mail]>
> > > > > > + * Author: Eduardo Habkost <[e-mail]>
> > > > > > + * Author: Igor Mammedov <[e-mail]>
> > > > > > + * Author: Paolo Bonzini <[e-mail]>
> > > > > > + * Author: Markus Armbruster <[e-mail]>    
> > > > > 
> > > > > IANAL, but I believe a
> > > > >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > > > > line is needed here.
> > > > >   
> > > > > > + *
> > > > > > + * This program is free software; you can redistribute it and/or modify
> > > > > > + * it under the terms of the GNU General Public License as published by
> > > > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > > > + * (at your option) any later version.
> > > > > > +
> > > > > > + * This program is distributed in the hope that it will be useful,
> > > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > > + * GNU General Public License for more details.
> > > > > > +
> > > > > > + * You should have received a copy of the GNU General Public License along
> > > > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > > > + */
> > > > > > +
> > > > > > +#include "qemu/osdep.h"
> > > > > > +#include "qapi/error.h"
> > > > > > +#include "qemu/cutils.h"
> > > > > > +#include "qom/cpu.h"
> > > > > > +#include "qemu/error-report.h"
> > > > > > +#include "hw/qdev-properties.h"
> > > > > > +
> > > > > > +static inline void feat2prop(char *s)
> > > > > > +{
> > > > > > +    while ((s = strchr(s, '_'))) {
> > > > > > +        *s = '-';
> > > > > > +    }
> > > > > > +}
> > > > > > +
> > > > > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > > > > +{
> > > > > > +    return g_strcmp0(a, b);
> > > > > > +}
> > > > > > +
> > > > > > +static void
> > > > > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > > > > +{
> > > > > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > > > > +    prop->driver = typename;
> > > > > > +    prop->property = g_strdup(name);
> > > > > > +    prop->value = g_strdup(val);
> > > > > > +    prop->errp = &error_fatal;
> > > > > > +    qdev_prop_register_global(prop);
> > > > > > +}
> > > > > > +
> > > > > > +/* DO NOT USE WITH NEW CODE
> > > > > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > > > > + */
> > > > > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > > > > +                                 Error **errp)
> > > > > > +{
> > > > > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > > > > +     * where +-feat overwrites any feature set by
> > > > > > +     * feat=on|feat even if the later is parsed after +-feat
> > > > > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > > > > +     */
> > > > > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > > > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > > > > +    static bool cpu_globals_initialized;
> > > > > > +    bool ambiguous = false;
> > > > > > +
> > > > > > +    if (cpu_globals_initialized) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +    cpu_globals_initialized = true;
> > > > > > +
> > > > > > +    if (!features) {
> > > > > > +        return;
> > > > > > +    }
> > > > > > +
> > > > > > +    for (featurestr = strtok(features, ",");
> > > > > > +         featurestr;
> > > > > > +         featurestr = strtok(NULL, ",")) {
> > > > > > +        const char *name;
> > > > > > +        const char *val = NULL;
> > > > > > +        char *eq = NULL;
> > > > > > +        char num[32];
> > > > > > +
> > > > > > +        /* Compatibility syntax: */
> > > > > > +        if (featurestr[0] == '+') {
> > > > > > +            plus_features = g_list_append(plus_features,
> > > > > > +                                          g_strdup(featurestr + 1));
> > > > > > +            continue;
> > > > > > +        } else if (featurestr[0] == '-') {
> > > > > > +            minus_features = g_list_append(minus_features,
> > > > > > +                                           g_strdup(featurestr + 1));
> > > > > > +            continue;
> > > > > > +        }    
> > > > > 
> > > > > These 6 lines of code (or something equivalent to them) are
> > > > > supposed to be the only difference to the generic parsing
> > > > > function.  I would simply make this feature (support for
> > > > > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > > > > handled by cpu_common_parse_features().  
> > > > I'd rather keep plus/minus nonsense  under the hood separate
> > > > legacy function so it get reused unintentionally and keep generic
> > > > parser clean.
> > > > 
> > > > I didn't have any intent of generalizing +-feat handling
> > > > but rather to remove code duplication between the only users
> > > > (x86/sparc) that happened to use syntax and share the same semantics.  
> > > 
> > > Generalizing it to be controlled by a CPUClass flag will make it
> > > easier to refactor the feature parsing later to use the QemuOpts
> > > parser.  But I agree there's no need to do that on this series.
> > > 
> > > (We might even decide to make [+-]feat work on all other
> > > architectures.  We already agreed recently that we won't
> > > deprecate it in x86, we could as well enable the same syntax
> > > uniformly across all architectures.)  
> > I'd just stick to canonical feat=on|off and keep legacy to x86|sparc,
> > but it's this to discuss if future and relevant here.
> >   
> > >   
> > > > 
> > > > As an alternative I can copy-past x86 variant into sparc
> > > > (modulo x86 harmless fixups), that will add some code duplication
> > > > I've tried to avoid with this patch, but it won't cause
> > > > misunderstanding about generalizing legacy hacks.  
> > > 
> > > Works for me.  We can then cleanup the x86 code and make it use
> > > cpu_legacy_parse_featurestr() later.  
> > Then, I'll just copy and replace sparc variant with x86 impl.
> > (removing from the copy x86 only parts and making parser stricter
> > where possible) and drop 5-6 patches that touched x86 for the purpose
> > of sharing code.
> > 
> > and respin v3 tomorrow.  
> 
> I just have one worry about this plan, below:
> 
> >   
> > >   
> > > >   
> > > > > (But this can be done as a follow-up patch.)
> > > > >   
> > > > > > +
> > > > > > +        eq = strchr(featurestr, '=');
> > > > > > +        if (eq) {
> > > > > > +            *eq++ = 0;
> > > > > > +            val = eq;
> > > > > > +        } else {
> > > > > > +            val = "on";
> > > > > > +        }
> > > > > > +
> > > > > > +        feat2prop(featurestr);
> > > > > > +        name = featurestr;
> > > > > > +
> > > > > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > > > > +                        name, name, val);
> > > > > > +            ambiguous = true;
> > > > > > +        }
> > > > > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > > > > +                        name, name, val);
> > > > > > +            ambiguous = true;
> > > > > > +        }
> > > > > > +
> > > > > > +        /* Special case: */
> > > > > > +        if (!strcmp(name, "tsc-freq")) {
> > > > > > +            int ret;
> > > > > > +            uint64_t tsc_freq;
> > > > > > +
> > > > > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > > > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > > > > +                error_setg(errp, "bad numerical value %s", val);
> > > > > > +                return;
> > > > > > +            }
> > > > > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > > > > +            val = num;
> > > > > > +            name = "tsc-frequency";
> > > > > > +        }    
> > > > > 
> > > > > This is x86-specific and should stay in x86-specific code.  It
> > > > > can probably be handled by the tsc-freq setter.  
> > > > there was reason why it wasn't moved to tsc-frequency setter,
> > > > the former is pure integer type of property,
> > > > while here we can get suffixed string that scales by 1000.
> > > > 
> > > > Short of creating new visitor for KHz (I don't really looking forward to it),
> > > > it's simpler to leave fixup alone in legacy parser that's shared only between
> > > > x86/sparc as it doesn't conflict with sparc and won't break anything.  
> > > 
> > > It doesn't break anything, but it will move x86-specific cruft to
> > > code that is supposed to be generic.
> > > 
> > > Creating a write-only "tsc-freq" property that accepts a string
> > > isn't hard to do.  
> > Nice idea, it might just work.
> >   
> > > 
> > > If you are not willing to do it in this series, you can write a
> > > generic parser now (without x86-specific cruft), use it only on
> > > sparc, and later we can refactor x86 so it can also use the
> > > generic one.  
> > So far, I'm inclined towards opposing +-feat generalizing and keeping
> > canonical form anywhere except of x86/sparc were we have to tolerate it.
> > 
> >   
> > > > > > +
> > > > > > +        cpu_add_feat_as_prop(typename, name, val);
> > > > > > +    }
> > > > > > +
> > > > > > +    if (ambiguous) {
> > > > > > +        warn_report("Compatibility of ambiguous CPU model "
> > > > > > +                    "strings won't be kept on future QEMU versions");
> > > > > > +    }    
> > > > > 
> > > > > As noted in the review of the x86 patch that removes the
> > > > > plus_features/minus_features static variables, this obsolete (and
> > > > > confusing) property ordering misfeature should be removed before
> > > > > we make this code generic and reuse it on other architectures.  
> > > > As it's been replied removing ordering is behavioral change for
> > > > both x86 and sparc, which is not related to series.
> > > > If you wish, I'll post a patch that will what you suggest
> > > > on top of series.  
> > > 
> > > I would agree if sparc also implemented the weird ordering.  But
> > > sparc does not implement it (it doesn't support feat=(on|off)
> > > yet).  We shouldn't introduce that misfeature in sparc if we're
> > > already planning to remove it.  
> > It didn't have canonic form but it gains this ability with this series.
> > So we could do better for it by forbidding mixed syntax from starters  
> 
> We don't need to forbid mixed syntax.  We can simply apply
> [+-]feat and feat=on|off in the same order they appear in the
> command-line.
> 
> But:
> 
> > (+1 for copy-past) but we have to keep minus overrides plus semantics,
> > so ambiguous check is still there but it would lead to hard error instead of
> > warning.  
> 
> I forgot about this additional weird semantics
> (minus-override-plus).  :(
> 
> I think we must deprecate the minus-override-plus semantics too
> and move to command-line-order eventually (on both x86 and
> sparc).  But to do that, we need to make the code print a warning
> on sparc like we do on x86.
> 
> If we don't remove that weird semantics before making sparc
> support feat=on|off, we will have to worry about the semantics of
> "-feat,feat=on" on top of that.  I would simply wait for 2
> releases and remove minus-override-plus, before implementing
> feat=on|off on sparc, to avoid creating a new set of problems.
> Is support for feat=on|off on sparc a must-have for QEMU 2.11?
Probably it isn't must have (as far as I can tell now) but
I won't bet on it.

Supporting both ways as far as mix is forbidden is fine,
so I'd 'introduce' feat=on|off (feat=on|off behavior is
inherited from QOM Boolean property, so we would have
to cripple parser intentionally).

Later we can drop minus-override-plus or whatever else,
but outside of this series pls.

Igor Mammedov Aug. 24, 2017, 1:43 p.m. UTC | #9

On Thu, 24 Aug 2017 11:18:52 +0200
Igor Mammedov <[e-mail]> wrote:

> On Wed, 23 Aug 2017 14:58:39 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Wed, Aug 23, 2017 at 07:37:39PM +0200, Igor Mammedov wrote:  
> > > On Wed, 23 Aug 2017 13:46:38 -0300
> > > Eduardo Habkost <[e-mail]> wrote:
> > >     
> > > > On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:    
> > > > > On Wed, 23 Aug 2017 11:34:14 -0300
> > > > > Eduardo Habkost <[e-mail]> wrote:
> > > > >     
> > > > > > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:    
> > > > > > > Move cpu_model +-feat parsing into a separate file so that it
> > > > > > > could be reused later for parsing similar format of sparc target
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > > > > > ---
> > > > > > > CC: Richard Henderson <[e-mail]>
> > > > > > > CC: Eduardo Habkost <[e-mail]>
> > > > > > > CC: Mark Cave-Ayland <[e-mail]>
> > > > > > > CC: Artyom Tarasenko <[e-mail]>
> > > > > > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > > > > > ---
> > > > > > >  include/qom/cpu.h                     |   2 +
> > > > > > >  default-configs/i386-bsd-user.mak     |   1 +
> > > > > > >  default-configs/i386-linux-user.mak   |   1 +
> > > > > > >  default-configs/i386-softmmu.mak      |   1 +
> > > > > > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > > > > > >  default-configs/x86_64-linux-user.mak |   1 +
> > > > > > >  default-configs/x86_64-softmmu.mak    |   1 +
> > > > > > >  target/i386/cpu.c                     | 125 +-------------------------
> > > > > > >  util/Makefile.objs                    |   1 +
> > > > > > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > > > > > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > > > > > >  create mode 100644 util/legacy_cpu_features_parser.c
> > > > > > >       
> > > > > > [...]    
> > > > > > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..6b352a3
> > > > > > > --- /dev/null
> > > > > > > +++ b/util/legacy_cpu_features_parser.c
> > > > > > > @@ -0,0 +1,161 @@
> > > > > > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > > > > > + * used by x86/sparc targets
> > > > > > > + *
> > > > > > > + * Author: Andreas Färber <[e-mail]>
> > > > > > > + * Author: Andre Przywara <[e-mail]>
> > > > > > > + * Author: Eduardo Habkost <[e-mail]>
> > > > > > > + * Author: Igor Mammedov <[e-mail]>
> > > > > > > + * Author: Paolo Bonzini <[e-mail]>
> > > > > > > + * Author: Markus Armbruster <[e-mail]>      
> > > > > > 
> > > > > > IANAL, but I believe a
> > > > > >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > > > > > line is needed here.
> > > > > >     
> > > > > > > + *
> > > > > > > + * This program is free software; you can redistribute it and/or modify
> > > > > > > + * it under the terms of the GNU General Public License as published by
> > > > > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > > > > + * (at your option) any later version.
> > > > > > > +
> > > > > > > + * This program is distributed in the hope that it will be useful,
> > > > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > > > + * GNU General Public License for more details.
> > > > > > > +
> > > > > > > + * You should have received a copy of the GNU General Public License along
> > > > > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "qapi/error.h"
> > > > > > > +#include "qemu/cutils.h"
> > > > > > > +#include "qom/cpu.h"
> > > > > > > +#include "qemu/error-report.h"
> > > > > > > +#include "hw/qdev-properties.h"
> > > > > > > +
> > > > > > > +static inline void feat2prop(char *s)
> > > > > > > +{
> > > > > > > +    while ((s = strchr(s, '_'))) {
> > > > > > > +        *s = '-';
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > > > > > +{
> > > > > > > +    return g_strcmp0(a, b);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void
> > > > > > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > > > > > +{
> > > > > > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > > > > > +    prop->driver = typename;
> > > > > > > +    prop->property = g_strdup(name);
> > > > > > > +    prop->value = g_strdup(val);
> > > > > > > +    prop->errp = &error_fatal;
> > > > > > > +    qdev_prop_register_global(prop);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* DO NOT USE WITH NEW CODE
> > > > > > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > > > > > + */
> > > > > > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > > > > > +                                 Error **errp)
> > > > > > > +{
> > > > > > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > > > > > +     * where +-feat overwrites any feature set by
> > > > > > > +     * feat=on|feat even if the later is parsed after +-feat
> > > > > > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > > > > > +     */
> > > > > > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > > > > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > > > > > +    static bool cpu_globals_initialized;
> > > > > > > +    bool ambiguous = false;
> > > > > > > +
> > > > > > > +    if (cpu_globals_initialized) {
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +    cpu_globals_initialized = true;
> > > > > > > +
> > > > > > > +    if (!features) {
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    for (featurestr = strtok(features, ",");
> > > > > > > +         featurestr;
> > > > > > > +         featurestr = strtok(NULL, ",")) {
> > > > > > > +        const char *name;
> > > > > > > +        const char *val = NULL;
> > > > > > > +        char *eq = NULL;
> > > > > > > +        char num[32];
> > > > > > > +
> > > > > > > +        /* Compatibility syntax: */
> > > > > > > +        if (featurestr[0] == '+') {
> > > > > > > +            plus_features = g_list_append(plus_features,
> > > > > > > +                                          g_strdup(featurestr + 1));
> > > > > > > +            continue;
> > > > > > > +        } else if (featurestr[0] == '-') {
> > > > > > > +            minus_features = g_list_append(minus_features,
> > > > > > > +                                           g_strdup(featurestr + 1));
> > > > > > > +            continue;
> > > > > > > +        }      
> > > > > > 
> > > > > > These 6 lines of code (or something equivalent to them) are
> > > > > > supposed to be the only difference to the generic parsing
> > > > > > function.  I would simply make this feature (support for
> > > > > > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > > > > > handled by cpu_common_parse_features().    
> > > > > I'd rather keep plus/minus nonsense  under the hood separate
> > > > > legacy function so it get reused unintentionally and keep generic
> > > > > parser clean.
> > > > > 
> > > > > I didn't have any intent of generalizing +-feat handling
> > > > > but rather to remove code duplication between the only users
> > > > > (x86/sparc) that happened to use syntax and share the same semantics.    
> > > > 
> > > > Generalizing it to be controlled by a CPUClass flag will make it
> > > > easier to refactor the feature parsing later to use the QemuOpts
> > > > parser.  But I agree there's no need to do that on this series.
> > > > 
> > > > (We might even decide to make [+-]feat work on all other
> > > > architectures.  We already agreed recently that we won't
> > > > deprecate it in x86, we could as well enable the same syntax
> > > > uniformly across all architectures.)    
> > > I'd just stick to canonical feat=on|off and keep legacy to x86|sparc,
> > > but it's this to discuss if future and relevant here.
> > >     
> > > >     
> > > > > 
> > > > > As an alternative I can copy-past x86 variant into sparc
> > > > > (modulo x86 harmless fixups), that will add some code duplication
> > > > > I've tried to avoid with this patch, but it won't cause
> > > > > misunderstanding about generalizing legacy hacks.    
> > > > 
> > > > Works for me.  We can then cleanup the x86 code and make it use
> > > > cpu_legacy_parse_featurestr() later.    
> > > Then, I'll just copy and replace sparc variant with x86 impl.
> > > (removing from the copy x86 only parts and making parser stricter
> > > where possible) and drop 5-6 patches that touched x86 for the purpose
> > > of sharing code.
> > > 
> > > and respin v3 tomorrow.    
> > 
> > I just have one worry about this plan, below:
> >   
> > >     
> > > >     
> > > > >     
> > > > > > (But this can be done as a follow-up patch.)
> > > > > >     
> > > > > > > +
> > > > > > > +        eq = strchr(featurestr, '=');
> > > > > > > +        if (eq) {
> > > > > > > +            *eq++ = 0;
> > > > > > > +            val = eq;
> > > > > > > +        } else {
> > > > > > > +            val = "on";
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        feat2prop(featurestr);
> > > > > > > +        name = featurestr;
> > > > > > > +
> > > > > > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > > > > > +                        name, name, val);
> > > > > > > +            ambiguous = true;
> > > > > > > +        }
> > > > > > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > > > > > +                        name, name, val);
> > > > > > > +            ambiguous = true;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        /* Special case: */
> > > > > > > +        if (!strcmp(name, "tsc-freq")) {
> > > > > > > +            int ret;
> > > > > > > +            uint64_t tsc_freq;
> > > > > > > +
> > > > > > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > > > > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > > > > > +                error_setg(errp, "bad numerical value %s", val);
> > > > > > > +                return;
> > > > > > > +            }
> > > > > > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > > > > > +            val = num;
> > > > > > > +            name = "tsc-frequency";
> > > > > > > +        }      
> > > > > > 
> > > > > > This is x86-specific and should stay in x86-specific code.  It
> > > > > > can probably be handled by the tsc-freq setter.    
> > > > > there was reason why it wasn't moved to tsc-frequency setter,
> > > > > the former is pure integer type of property,
> > > > > while here we can get suffixed string that scales by 1000.
> > > > > 
> > > > > Short of creating new visitor for KHz (I don't really looking forward to it),
> > > > > it's simpler to leave fixup alone in legacy parser that's shared only between
> > > > > x86/sparc as it doesn't conflict with sparc and won't break anything.    
> > > > 
> > > > It doesn't break anything, but it will move x86-specific cruft to
> > > > code that is supposed to be generic.
> > > > 
> > > > Creating a write-only "tsc-freq" property that accepts a string
> > > > isn't hard to do.    
> > > Nice idea, it might just work.
> > >     
> > > > 
> > > > If you are not willing to do it in this series, you can write a
> > > > generic parser now (without x86-specific cruft), use it only on
> > > > sparc, and later we can refactor x86 so it can also use the
> > > > generic one.    
> > > So far, I'm inclined towards opposing +-feat generalizing and keeping
> > > canonical form anywhere except of x86/sparc were we have to tolerate it.
> > > 
> > >     
> > > > > > > +
> > > > > > > +        cpu_add_feat_as_prop(typename, name, val);
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if (ambiguous) {
> > > > > > > +        warn_report("Compatibility of ambiguous CPU model "
> > > > > > > +                    "strings won't be kept on future QEMU versions");
> > > > > > > +    }      
> > > > > > 
> > > > > > As noted in the review of the x86 patch that removes the
> > > > > > plus_features/minus_features static variables, this obsolete (and
> > > > > > confusing) property ordering misfeature should be removed before
> > > > > > we make this code generic and reuse it on other architectures.    
> > > > > As it's been replied removing ordering is behavioral change for
> > > > > both x86 and sparc, which is not related to series.
> > > > > If you wish, I'll post a patch that will what you suggest
> > > > > on top of series.    
> > > > 
> > > > I would agree if sparc also implemented the weird ordering.  But
> > > > sparc does not implement it (it doesn't support feat=(on|off)
> > > > yet).  We shouldn't introduce that misfeature in sparc if we're
> > > > already planning to remove it.    
> > > It didn't have canonic form but it gains this ability with this series.
> > > So we could do better for it by forbidding mixed syntax from starters    
> > 
> > We don't need to forbid mixed syntax.  We can simply apply
> > [+-]feat and feat=on|off in the same order they appear in the
> > command-line.
> > 
> > But:
> >   
> > > (+1 for copy-past) but we have to keep minus overrides plus semantics,
> > > so ambiguous check is still there but it would lead to hard error instead of
> > > warning.    
> > 
> > I forgot about this additional weird semantics
> > (minus-override-plus).  :(
> > 
> > I think we must deprecate the minus-override-plus semantics too
> > and move to command-line-order eventually (on both x86 and
> > sparc).  But to do that, we need to make the code print a warning
> > on sparc like we do on x86.
> > 
> > If we don't remove that weird semantics before making sparc
> > support feat=on|off, we will have to worry about the semantics of
> > "-feat,feat=on" on top of that.  I would simply wait for 2
> > releases and remove minus-override-plus, before implementing
> > feat=on|off on sparc, to avoid creating a new set of problems.
> > Is support for feat=on|off on sparc a must-have for QEMU 2.11?  
> Probably it isn't must have (as far as I can tell now) but
> I won't bet on it.
> 
> Supporting both ways as far as mix is forbidden is fine,
> so I'd 'introduce' feat=on|off (feat=on|off behavior is
> inherited from QOM Boolean property, so we would have
> to cripple parser intentionally).
It looks simpler to support only +-feat with feat=on|off disabled
explicitly, so I'll do it this way as you've suggested.

PS:
found 1 more bug in cpu_generic_init() while testing this,
so +1 patch to fix it

> Later we can drop minus-override-plus or whatever else,
> but outside of this series pls.
>

Eduardo Habkost Aug. 24, 2017, 1:45 p.m. UTC | #10

On Thu, Aug 24, 2017 at 11:18:52AM +0200, Igor Mammedov wrote:
> On Wed, 23 Aug 2017 14:58:39 -0300
> Eduardo Habkost <[e-mail]> wrote:
> 
> > On Wed, Aug 23, 2017 at 07:37:39PM +0200, Igor Mammedov wrote:
> > > On Wed, 23 Aug 2017 13:46:38 -0300
> > > Eduardo Habkost <[e-mail]> wrote:
> > >   
> > > > On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:  
> > > > > On Wed, 23 Aug 2017 11:34:14 -0300
> > > > > Eduardo Habkost <[e-mail]> wrote:
> > > > >   
> > > > > > On Fri, Aug 18, 2017 at 12:08:38PM +0200, Igor Mammedov wrote:  
> > > > > > > Move cpu_model +-feat parsing into a separate file so that it
> > > > > > > could be reused later for parsing similar format of sparc target
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <[e-mail]>
> > > > > > > ---
> > > > > > > CC: Richard Henderson <[e-mail]>
> > > > > > > CC: Eduardo Habkost <[e-mail]>
> > > > > > > CC: Mark Cave-Ayland <[e-mail]>
> > > > > > > CC: Artyom Tarasenko <[e-mail]>
> > > > > > > CC: Philippe Mathieu-Daudé <[e-mail]>
> > > > > > > ---
> > > > > > >  include/qom/cpu.h                     |   2 +
> > > > > > >  default-configs/i386-bsd-user.mak     |   1 +
> > > > > > >  default-configs/i386-linux-user.mak   |   1 +
> > > > > > >  default-configs/i386-softmmu.mak      |   1 +
> > > > > > >  default-configs/x86_64-bsd-user.mak   |   1 +
> > > > > > >  default-configs/x86_64-linux-user.mak |   1 +
> > > > > > >  default-configs/x86_64-softmmu.mak    |   1 +
> > > > > > >  target/i386/cpu.c                     | 125 +-------------------------
> > > > > > >  util/Makefile.objs                    |   1 +
> > > > > > >  util/legacy_cpu_features_parser.c     | 161 ++++++++++++++++++++++++++++++++++
> > > > > > >  10 files changed, 171 insertions(+), 124 deletions(-)
> > > > > > >  create mode 100644 util/legacy_cpu_features_parser.c
> > > > > > >     
> > > > > > [...]  
> > > > > > > diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..6b352a3
> > > > > > > --- /dev/null
> > > > > > > +++ b/util/legacy_cpu_features_parser.c
> > > > > > > @@ -0,0 +1,161 @@
> > > > > > > +/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
> > > > > > > + * used by x86/sparc targets
> > > > > > > + *
> > > > > > > + * Author: Andreas Färber <[e-mail]>
> > > > > > > + * Author: Andre Przywara <[e-mail]>
> > > > > > > + * Author: Eduardo Habkost <[e-mail]>
> > > > > > > + * Author: Igor Mammedov <[e-mail]>
> > > > > > > + * Author: Paolo Bonzini <[e-mail]>
> > > > > > > + * Author: Markus Armbruster <[e-mail]>    
> > > > > > 
> > > > > > IANAL, but I believe a
> > > > > >   Copyright (c) <YEAR> <COPYRIGHT HOLDER>
> > > > > > line is needed here.
> > > > > >   
> > > > > > > + *
> > > > > > > + * This program is free software; you can redistribute it and/or modify
> > > > > > > + * it under the terms of the GNU General Public License as published by
> > > > > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > > > > + * (at your option) any later version.
> > > > > > > +
> > > > > > > + * This program is distributed in the hope that it will be useful,
> > > > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > > > > > + * GNU General Public License for more details.
> > > > > > > +
> > > > > > > + * You should have received a copy of the GNU General Public License along
> > > > > > > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > > > > > > + */
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "qapi/error.h"
> > > > > > > +#include "qemu/cutils.h"
> > > > > > > +#include "qom/cpu.h"
> > > > > > > +#include "qemu/error-report.h"
> > > > > > > +#include "hw/qdev-properties.h"
> > > > > > > +
> > > > > > > +static inline void feat2prop(char *s)
> > > > > > > +{
> > > > > > > +    while ((s = strchr(s, '_'))) {
> > > > > > > +        *s = '-';
> > > > > > > +    }
> > > > > > > +}
> > > > > > > +
> > > > > > > +static gint compare_string(gconstpointer a, gconstpointer b)
> > > > > > > +{
> > > > > > > +    return g_strcmp0(a, b);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void
> > > > > > > +cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
> > > > > > > +{
> > > > > > > +    GlobalProperty *prop = g_new0(typeof(*prop), 1);
> > > > > > > +    prop->driver = typename;
> > > > > > > +    prop->property = g_strdup(name);
> > > > > > > +    prop->value = g_strdup(val);
> > > > > > > +    prop->errp = &error_fatal;
> > > > > > > +    qdev_prop_register_global(prop);
> > > > > > > +}
> > > > > > > +
> > > > > > > +/* DO NOT USE WITH NEW CODE
> > > > > > > + * Parse "+feature,-feature,feature=foo" CPU feature string
> > > > > > > + */
> > > > > > > +void cpu_legacy_parse_featurestr(const char *typename, char *features,
> > > > > > > +                                 Error **errp)
> > > > > > > +{
> > > > > > > +    /* Compatibily hack to maintain legacy +-feat semantic,
> > > > > > > +     * where +-feat overwrites any feature set by
> > > > > > > +     * feat=on|feat even if the later is parsed after +-feat
> > > > > > > +     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
> > > > > > > +     */
> > > > > > > +    GList *l, *plus_features = NULL, *minus_features = NULL;
> > > > > > > +    char *featurestr; /* Single 'key=value" string being parsed */
> > > > > > > +    static bool cpu_globals_initialized;
> > > > > > > +    bool ambiguous = false;
> > > > > > > +
> > > > > > > +    if (cpu_globals_initialized) {
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +    cpu_globals_initialized = true;
> > > > > > > +
> > > > > > > +    if (!features) {
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    for (featurestr = strtok(features, ",");
> > > > > > > +         featurestr;
> > > > > > > +         featurestr = strtok(NULL, ",")) {
> > > > > > > +        const char *name;
> > > > > > > +        const char *val = NULL;
> > > > > > > +        char *eq = NULL;
> > > > > > > +        char num[32];
> > > > > > > +
> > > > > > > +        /* Compatibility syntax: */
> > > > > > > +        if (featurestr[0] == '+') {
> > > > > > > +            plus_features = g_list_append(plus_features,
> > > > > > > +                                          g_strdup(featurestr + 1));
> > > > > > > +            continue;
> > > > > > > +        } else if (featurestr[0] == '-') {
> > > > > > > +            minus_features = g_list_append(minus_features,
> > > > > > > +                                           g_strdup(featurestr + 1));
> > > > > > > +            continue;
> > > > > > > +        }    
> > > > > > 
> > > > > > These 6 lines of code (or something equivalent to them) are
> > > > > > supposed to be the only difference to the generic parsing
> > > > > > function.  I would simply make this feature (support for
> > > > > > [+-]feature) enabled by a CPUClass::plus_minus_features flag
> > > > > > handled by cpu_common_parse_features().  
> > > > > I'd rather keep plus/minus nonsense  under the hood separate
> > > > > legacy function so it get reused unintentionally and keep generic
> > > > > parser clean.
> > > > > 
> > > > > I didn't have any intent of generalizing +-feat handling
> > > > > but rather to remove code duplication between the only users
> > > > > (x86/sparc) that happened to use syntax and share the same semantics.  
> > > > 
> > > > Generalizing it to be controlled by a CPUClass flag will make it
> > > > easier to refactor the feature parsing later to use the QemuOpts
> > > > parser.  But I agree there's no need to do that on this series.
> > > > 
> > > > (We might even decide to make [+-]feat work on all other
> > > > architectures.  We already agreed recently that we won't
> > > > deprecate it in x86, we could as well enable the same syntax
> > > > uniformly across all architectures.)  
> > > I'd just stick to canonical feat=on|off and keep legacy to x86|sparc,
> > > but it's this to discuss if future and relevant here.
> > >   
> > > >   
> > > > > 
> > > > > As an alternative I can copy-past x86 variant into sparc
> > > > > (modulo x86 harmless fixups), that will add some code duplication
> > > > > I've tried to avoid with this patch, but it won't cause
> > > > > misunderstanding about generalizing legacy hacks.  
> > > > 
> > > > Works for me.  We can then cleanup the x86 code and make it use
> > > > cpu_legacy_parse_featurestr() later.  
> > > Then, I'll just copy and replace sparc variant with x86 impl.
> > > (removing from the copy x86 only parts and making parser stricter
> > > where possible) and drop 5-6 patches that touched x86 for the purpose
> > > of sharing code.
> > > 
> > > and respin v3 tomorrow.  
> > 
> > I just have one worry about this plan, below:
> > 
> > >   
> > > >   
> > > > >   
> > > > > > (But this can be done as a follow-up patch.)
> > > > > >   
> > > > > > > +
> > > > > > > +        eq = strchr(featurestr, '=');
> > > > > > > +        if (eq) {
> > > > > > > +            *eq++ = 0;
> > > > > > > +            val = eq;
> > > > > > > +        } else {
> > > > > > > +            val = "on";
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        feat2prop(featurestr);
> > > > > > > +        name = featurestr;
> > > > > > > +
> > > > > > > +        if (g_list_find_custom(plus_features, name, compare_string)) {
> > > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > > +                        "Don't mix both \"+%s\" and \"%s=%s\"",
> > > > > > > +                        name, name, val);
> > > > > > > +            ambiguous = true;
> > > > > > > +        }
> > > > > > > +        if (g_list_find_custom(minus_features, name, compare_string)) {
> > > > > > > +            warn_report("Ambiguous CPU model string. "
> > > > > > > +                        "Don't mix both \"-%s\" and \"%s=%s\"",
> > > > > > > +                        name, name, val);
> > > > > > > +            ambiguous = true;
> > > > > > > +        }
> > > > > > > +
> > > > > > > +        /* Special case: */
> > > > > > > +        if (!strcmp(name, "tsc-freq")) {
> > > > > > > +            int ret;
> > > > > > > +            uint64_t tsc_freq;
> > > > > > > +
> > > > > > > +            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
> > > > > > > +            if (ret < 0 || tsc_freq > INT64_MAX) {
> > > > > > > +                error_setg(errp, "bad numerical value %s", val);
> > > > > > > +                return;
> > > > > > > +            }
> > > > > > > +            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
> > > > > > > +            val = num;
> > > > > > > +            name = "tsc-frequency";
> > > > > > > +        }    
> > > > > > 
> > > > > > This is x86-specific and should stay in x86-specific code.  It
> > > > > > can probably be handled by the tsc-freq setter.  
> > > > > there was reason why it wasn't moved to tsc-frequency setter,
> > > > > the former is pure integer type of property,
> > > > > while here we can get suffixed string that scales by 1000.
> > > > > 
> > > > > Short of creating new visitor for KHz (I don't really looking forward to it),
> > > > > it's simpler to leave fixup alone in legacy parser that's shared only between
> > > > > x86/sparc as it doesn't conflict with sparc and won't break anything.  
> > > > 
> > > > It doesn't break anything, but it will move x86-specific cruft to
> > > > code that is supposed to be generic.
> > > > 
> > > > Creating a write-only "tsc-freq" property that accepts a string
> > > > isn't hard to do.  
> > > Nice idea, it might just work.
> > >   
> > > > 
> > > > If you are not willing to do it in this series, you can write a
> > > > generic parser now (without x86-specific cruft), use it only on
> > > > sparc, and later we can refactor x86 so it can also use the
> > > > generic one.  
> > > So far, I'm inclined towards opposing +-feat generalizing and keeping
> > > canonical form anywhere except of x86/sparc were we have to tolerate it.
> > > 
> > >   
> > > > > > > +
> > > > > > > +        cpu_add_feat_as_prop(typename, name, val);
> > > > > > > +    }
> > > > > > > +
> > > > > > > +    if (ambiguous) {
> > > > > > > +        warn_report("Compatibility of ambiguous CPU model "
> > > > > > > +                    "strings won't be kept on future QEMU versions");
> > > > > > > +    }    
> > > > > > 
> > > > > > As noted in the review of the x86 patch that removes the
> > > > > > plus_features/minus_features static variables, this obsolete (and
> > > > > > confusing) property ordering misfeature should be removed before
> > > > > > we make this code generic and reuse it on other architectures.  
> > > > > As it's been replied removing ordering is behavioral change for
> > > > > both x86 and sparc, which is not related to series.
> > > > > If you wish, I'll post a patch that will what you suggest
> > > > > on top of series.  
> > > > 
> > > > I would agree if sparc also implemented the weird ordering.  But
> > > > sparc does not implement it (it doesn't support feat=(on|off)
> > > > yet).  We shouldn't introduce that misfeature in sparc if we're
> > > > already planning to remove it.  
> > > It didn't have canonic form but it gains this ability with this series.
> > > So we could do better for it by forbidding mixed syntax from starters  
> > 
> > We don't need to forbid mixed syntax.  We can simply apply
> > [+-]feat and feat=on|off in the same order they appear in the
> > command-line.
> > 
> > But:
> > 
> > > (+1 for copy-past) but we have to keep minus overrides plus semantics,
> > > so ambiguous check is still there but it would lead to hard error instead of
> > > warning.  
> > 
> > I forgot about this additional weird semantics
> > (minus-override-plus).  :(
> > 
> > I think we must deprecate the minus-override-plus semantics too
> > and move to command-line-order eventually (on both x86 and
> > sparc).  But to do that, we need to make the code print a warning
> > on sparc like we do on x86.
> > 
> > If we don't remove that weird semantics before making sparc
> > support feat=on|off, we will have to worry about the semantics of
> > "-feat,feat=on" on top of that.  I would simply wait for 2
> > releases and remove minus-override-plus, before implementing
> > feat=on|off on sparc, to avoid creating a new set of problems.
> > Is support for feat=on|off on sparc a must-have for QEMU 2.11?
> Probably it isn't must have (as far as I can tell now) but
> I won't bet on it.
> 
> Supporting both ways as far as mix is forbidden is fine,
> so I'd 'introduce' feat=on|off (feat=on|off behavior is
> inherited from QOM Boolean property, so we would have
> to cripple parser intentionally).

Yes, if you really want to rewrite the sparc parser now without
changing behavior, I think we shouldn't allow "-feat,feat=on".
But there's no need for the extra work: if we just wait until we
remove the minus-overrides-plus behavior everywhere before
replacing the sparc parser.

> 
> Later we can drop minus-override-plus or whatever else,
> but outside of this series pls.

No problem to me.  I just don't want this series to create an
additional set of problems to solve on sparc.

Patch

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 25eefea..30247dc 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -1038,4 +1038,6 @@  extern const struct VMStateDescription vmstate_cpu_common;
 
 #define UNASSIGNED_CPU_INDEX -1
 
+void cpu_legacy_parse_featurestr(const char *typename, char *features,
+                                 Error **errp);
 #endif
diff --git a/default-configs/i386-bsd-user.mak b/default-configs/i386-bsd-user.mak
index af1b31a..b28a05f 100644
--- a/default-configs/i386-bsd-user.mak
+++ b/default-configs/i386-bsd-user.mak
@@ -1 +1,2 @@ 
 # Default configuration for i386-bsd-user
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/default-configs/i386-linux-user.mak b/default-configs/i386-linux-user.mak
index 8657e68..c136967 100644
--- a/default-configs/i386-linux-user.mak
+++ b/default-configs/i386-linux-user.mak
@@ -1 +1,2 @@ 
 # Default configuration for i386-linux-user
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index d2ab2f6..e3e7c0e 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -59,3 +59,4 @@  CONFIG_SMBIOS=y
 CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
 CONFIG_PXB=y
 CONFIG_ACPI_VMGENID=y
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/default-configs/x86_64-bsd-user.mak b/default-configs/x86_64-bsd-user.mak
index 73e5d34..952323c 100644
--- a/default-configs/x86_64-bsd-user.mak
+++ b/default-configs/x86_64-bsd-user.mak
@@ -1 +1,2 @@ 
 # Default configuration for x86_64-bsd-user
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/default-configs/x86_64-linux-user.mak b/default-configs/x86_64-linux-user.mak
index bec1d9e..b513ef2 100644
--- a/default-configs/x86_64-linux-user.mak
+++ b/default-configs/x86_64-linux-user.mak
@@ -1 +1,2 @@ 
 # Default configuration for x86_64-linux-user
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 9bde2f1..6594ddf 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -59,3 +59,4 @@  CONFIG_SMBIOS=y
 CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
 CONFIG_PXB=y
 CONFIG_ACPI_VMGENID=y
+CONFIG_LEGACY_CPU_FEATURES=y
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 84f552d..ac60c1a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -17,7 +17,6 @@ 
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 #include "qemu/osdep.h"
-#include "qemu/cutils.h"
 
 #include "cpu.h"
 #include "exec/exec-all.h"
@@ -2030,13 +2029,6 @@  static const PropertyInfo qdev_prop_spinlocks = {
 /* Convert all '_' in a feature string option name to '-', to make feature
  * name conform to QOM property naming rule, which uses '-' instead of '_'.
  */
-static inline void feat2prop(char *s)
-{
-    while ((s = strchr(s, '_'))) {
-        *s = '-';
-    }
-}
-
 /* Return the feature property name for a feature flag bit */
 static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
 {
@@ -2058,126 +2050,11 @@  static const char *x86_cpu_feature_name(FeatureWord w, int bitnr)
     return feature_word_info[w].feat_names[bitnr];
 }
 
-static gint compare_string(gconstpointer a, gconstpointer b)
-{
-    return g_strcmp0(a, b);
-}
-
-static void
-cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
-{
-    GlobalProperty *prop = g_new0(typeof(*prop), 1);
-    prop->driver = typename;
-    prop->property = g_strdup(name);
-    prop->value = g_strdup(val);
-    prop->errp = &error_fatal;
-    qdev_prop_register_global(prop);
-}
-
 /* Parse "+feature,-feature,feature=foo" CPU feature string */
 static void x86_cpu_parse_featurestr(const char *typename, char *features,
                                      Error **errp)
 {
-    /* Compatibily hack to maintain legacy +-feat semantic,
-     * where +-feat overwrites any feature set by
-     * feat=on|feat even if the later is parsed after +-feat
-     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
-     */
-    GList *l, *plus_features = NULL, *minus_features = NULL;
-    char *featurestr; /* Single 'key=value" string being parsed */
-    static bool cpu_globals_initialized;
-    bool ambiguous = false;
-
-    if (cpu_globals_initialized) {
-        return;
-    }
-    cpu_globals_initialized = true;
-
-    if (!features) {
-        return;
-    }
-
-    for (featurestr = strtok(features, ",");
-         featurestr;
-         featurestr = strtok(NULL, ",")) {
-        const char *name;
-        const char *val = NULL;
-        char *eq = NULL;
-        char num[32];
-
-        /* Compatibility syntax: */
-        if (featurestr[0] == '+') {
-            plus_features = g_list_append(plus_features,
-                                          g_strdup(featurestr + 1));
-            continue;
-        } else if (featurestr[0] == '-') {
-            minus_features = g_list_append(minus_features,
-                                           g_strdup(featurestr + 1));
-            continue;
-        }
-
-        eq = strchr(featurestr, '=');
-        if (eq) {
-            *eq++ = 0;
-            val = eq;
-        } else {
-            val = "on";
-        }
-
-        feat2prop(featurestr);
-        name = featurestr;
-
-        if (g_list_find_custom(plus_features, name, compare_string)) {
-            warn_report("Ambiguous CPU model string. "
-                        "Don't mix both \"+%s\" and \"%s=%s\"",
-                        name, name, val);
-            ambiguous = true;
-        }
-        if (g_list_find_custom(minus_features, name, compare_string)) {
-            warn_report("Ambiguous CPU model string. "
-                        "Don't mix both \"-%s\" and \"%s=%s\"",
-                        name, name, val);
-            ambiguous = true;
-        }
-
-        /* Special case: */
-        if (!strcmp(name, "tsc-freq")) {
-            int ret;
-            uint64_t tsc_freq;
-
-            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
-            if (ret < 0 || tsc_freq > INT64_MAX) {
-                error_setg(errp, "bad numerical value %s", val);
-                return;
-            }
-            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
-            val = num;
-            name = "tsc-frequency";
-        }
-
-        cpu_add_feat_as_prop(typename, name, val);
-    }
-
-    if (ambiguous) {
-        warn_report("Compatibility of ambiguous CPU model "
-                    "strings won't be kept on future QEMU versions");
-    }
-
-    for (l = plus_features; l; l = l->next) {
-        const char *name = l->data;
-        cpu_add_feat_as_prop(typename, name, "on");
-    }
-    if (plus_features) {
-        g_list_free_full(plus_features, g_free);
-    }
-
-    for (l = minus_features; l; l = l->next) {
-        const char *name = l->data;
-        cpu_add_feat_as_prop(typename, name, "off");
-    }
-    if (minus_features) {
-        g_list_free_full(minus_features, g_free);
-    }
+    cpu_legacy_parse_featurestr(typename, features, errp);
 }
 
 static void x86_cpu_expand_features(X86CPU *cpu);
diff --git a/util/Makefile.objs b/util/Makefile.objs
index 50a55ec..14e28f7 100644
--- a/util/Makefile.objs
+++ b/util/Makefile.objs
@@ -45,3 +45,4 @@  util-obj-y += qht.o
 util-obj-y += range.o
 util-obj-y += stats64.o
 util-obj-y += systemd.o
+util-obj-$(CONFIG_LEGACY_CPU_FEATURES) += legacy_cpu_features_parser.o
diff --git a/util/legacy_cpu_features_parser.c b/util/legacy_cpu_features_parser.c
new file mode 100644
index 0000000..6b352a3
--- /dev/null
+++ b/util/legacy_cpu_features_parser.c
@@ -0,0 +1,161 @@ 
+/* Support for legacy -cpu cpu,features CLI option with +-feat syntax,
+ * used by x86/sparc targets
+ *
+ * Author: Andreas Färber <[e-mail]>
+ * Author: Andre Przywara <[e-mail]>
+ * Author: Eduardo Habkost <[e-mail]>
+ * Author: Igor Mammedov <[e-mail]>
+ * Author: Paolo Bonzini <[e-mail]>
+ * Author: Markus Armbruster <[e-mail]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+#include "qom/cpu.h"
+#include "qemu/error-report.h"
+#include "hw/qdev-properties.h"
+
+static inline void feat2prop(char *s)
+{
+    while ((s = strchr(s, '_'))) {
+        *s = '-';
+    }
+}
+
+static gint compare_string(gconstpointer a, gconstpointer b)
+{
+    return g_strcmp0(a, b);
+}
+
+static void
+cpu_add_feat_as_prop(const char *typename, const char *name, const char *val)
+{
+    GlobalProperty *prop = g_new0(typeof(*prop), 1);
+    prop->driver = typename;
+    prop->property = g_strdup(name);
+    prop->value = g_strdup(val);
+    prop->errp = &error_fatal;
+    qdev_prop_register_global(prop);
+}
+
+/* DO NOT USE WITH NEW CODE
+ * Parse "+feature,-feature,feature=foo" CPU feature string
+ */
+void cpu_legacy_parse_featurestr(const char *typename, char *features,
+                                 Error **errp)
+{
+    /* Compatibily hack to maintain legacy +-feat semantic,
+     * where +-feat overwrites any feature set by
+     * feat=on|feat even if the later is parsed after +-feat
+     * (i.e. "-x2apic,x2apic=on" will result in x2apic disabled)
+     */
+    GList *l, *plus_features = NULL, *minus_features = NULL;
+    char *featurestr; /* Single 'key=value" string being parsed */
+    static bool cpu_globals_initialized;
+    bool ambiguous = false;
+
+    if (cpu_globals_initialized) {
+        return;
+    }
+    cpu_globals_initialized = true;
+
+    if (!features) {
+        return;
+    }
+
+    for (featurestr = strtok(features, ",");
+         featurestr;
+         featurestr = strtok(NULL, ",")) {
+        const char *name;
+        const char *val = NULL;
+        char *eq = NULL;
+        char num[32];
+
+        /* Compatibility syntax: */
+        if (featurestr[0] == '+') {
+            plus_features = g_list_append(plus_features,
+                                          g_strdup(featurestr + 1));
+            continue;
+        } else if (featurestr[0] == '-') {
+            minus_features = g_list_append(minus_features,
+                                           g_strdup(featurestr + 1));
+            continue;
+        }
+
+        eq = strchr(featurestr, '=');
+        if (eq) {
+            *eq++ = 0;
+            val = eq;
+        } else {
+            val = "on";
+        }
+
+        feat2prop(featurestr);
+        name = featurestr;
+
+        if (g_list_find_custom(plus_features, name, compare_string)) {
+            warn_report("Ambiguous CPU model string. "
+                        "Don't mix both \"+%s\" and \"%s=%s\"",
+                        name, name, val);
+            ambiguous = true;
+        }
+        if (g_list_find_custom(minus_features, name, compare_string)) {
+            warn_report("Ambiguous CPU model string. "
+                        "Don't mix both \"-%s\" and \"%s=%s\"",
+                        name, name, val);
+            ambiguous = true;
+        }
+
+        /* Special case: */
+        if (!strcmp(name, "tsc-freq")) {
+            int ret;
+            uint64_t tsc_freq;
+
+            ret = qemu_strtosz_metric(val, NULL, &tsc_freq);
+            if (ret < 0 || tsc_freq > INT64_MAX) {
+                error_setg(errp, "bad numerical value %s", val);
+                return;
+            }
+            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
+            val = num;
+            name = "tsc-frequency";
+        }
+
+        cpu_add_feat_as_prop(typename, name, val);
+    }
+
+    if (ambiguous) {
+        warn_report("Compatibility of ambiguous CPU model "
+                    "strings won't be kept on future QEMU versions");
+    }
+
+    for (l = plus_features; l; l = l->next) {
+        const char *name = l->data;
+        cpu_add_feat_as_prop(typename, name, "on");
+    }
+    if (plus_features) {
+        g_list_free_full(plus_features, g_free);
+    }
+
+    for (l = minus_features; l; l = l->next) {
+        const char *name = l->data;
+        cpu_add_feat_as_prop(typename, name, "off");
+    }
+    if (minus_features) {
+        g_list_free_full(minus_features, g_free);
+    }
+}