Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 23 Aug 2017 11:34:14 -0300
Eduardo Habkost <ehabkost@redhat.com> 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 <imammedo@redhat.com>
> > ---
> > CC: Richard Henderson <rth@twiddle.net>
> > CC: Eduardo Habkost <ehabkost@redhat.com>
> > CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > CC: Artyom Tarasenko <atar4qemu@gmail.com>
> > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  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 <afaerber@suse.de>
> > + * Author: Andre Przywara <andre.przywara@amd.com>
> > + * Author: Eduardo Habkost <ehabkost@redhat.com>
> > + * Author: Igor Mammedov <imammedo@redhat.com>
> > + * Author: Paolo Bonzini <pbonzini@redhat.com>
> > + * Author: Markus Armbruster <armbru@redhat.com>  
> 
> 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
> > 
> >   
>