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

[Eduardo Habkost]

On Wed, Aug 23, 2017 at 06:29:02PM +0200, Igor Mammedov wrote:
> On Wed, 23 Aug 2017 11:34:14 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > > ---
> > > CC: Richard Henderson <address@hidden>
> > > CC: Eduardo Habkost <address@hidden>
> > > CC: Mark Cave-Ayland <address@hidden>
> > > CC: Artyom Tarasenko <address@hidden>
> > > CC: Philippe Mathieu-Daudé <address@hidden>
> > > ---
> > >  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 <address@hidden>
> > > + * Author: Andre Przywara <address@hidden>
> > > + * Author: Eduardo Habkost <address@hidden>
> > > + * Author: Igor Mammedov <address@hidden>
> > > + * Author: Paolo Bonzini <address@hidden>
> > > + * Author: Markus Armbruster <address@hidden>  
> > 
> > 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.

-- 
Eduardo