qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Igor Mammedov
Subject: Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser
Date: Wed, 23 Aug 2017 19:37:39 +0200

On Wed, 23 Aug 2017 13:46:38 -0300
Eduardo Habkost <address@hidden> 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 <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.)
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.






reply via email to

[Prev in Thread] Current Thread [Next in Thread]