[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: |
Thu, 24 Aug 2017 15:43:42 +0200 |
On Thu, 24 Aug 2017 11:18:52 +0200
Igor Mammedov <address@hidden> wrote:
> On Wed, 23 Aug 2017 14:58:39 -0300
> Eduardo Habkost <address@hidden> 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 <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.
> >
> > 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.
>
- [Qemu-devel] [PATCH for-2.11 13/27] microblaze: replace cpu_mb_init() with cpu_generic_init(), (continued)
- [Qemu-devel] [PATCH for-2.11 13/27] microblaze: replace cpu_mb_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
- [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Igor Mammedov, 2017/08/18
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Eduardo Habkost, 2017/08/18
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Eduardo Habkost, 2017/08/23
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Igor Mammedov, 2017/08/23
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Eduardo Habkost, 2017/08/23
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Igor Mammedov, 2017/08/23
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Eduardo Habkost, 2017/08/23
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Igor Mammedov, 2017/08/24
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser,
Igor Mammedov <=
- Re: [Qemu-devel] [PATCH for-2.11 06/27] x86: extract legacy cpu features format parser, Eduardo Habkost, 2017/08/24
[Qemu-devel] [PATCH for-2.11 14/27] nios2: replace cpu_nios2_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
[Qemu-devel] [PATCH for-2.11 15/27] tilegx: replace cpu_tilegx_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
[Qemu-devel] [PATCH for-2.11 18/27] sh4: replace cpu_sh4_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
[Qemu-devel] [PATCH for-2.11 20/27] cris: replace cpu_cris_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
[Qemu-devel] [PATCH for-2.11 22/27] lm32: replace cpu_lm32_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18
[Qemu-devel] [PATCH for-2.11 08/27] sparc: replace cpu_sparc_init() with cpu_generic_init(), Igor Mammedov, 2017/08/18