On Thu, 24 Aug 2017 11:18:52 +0200 Igor Mammedov <imamm...@redhat.com> wrote:
> On Wed, 23 Aug 2017 14:58:39 -0300 > Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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 <ehabk...@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 <imamm...@redhat.com> > > > > > > > --- > > > > > > > CC: Richard Henderson <r...@twiddle.net> > > > > > > > CC: Eduardo Habkost <ehabk...@redhat.com> > > > > > > > CC: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk> > > > > > > > CC: Artyom Tarasenko <atar4q...@gmail.com> > > > > > > > CC: Philippe Mathieu-Daudé <f4...@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 <afaer...@suse.de> > > > > > > > + * Author: Andre Przywara <andre.przyw...@amd.com> > > > > > > > + * Author: Eduardo Habkost <ehabk...@redhat.com> > > > > > > > + * Author: Igor Mammedov <imamm...@redhat.com> > > > > > > > + * Author: Paolo Bonzini <pbonz...@redhat.com> > > > > > > > + * Author: Markus Armbruster <arm...@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. > > > > > > > > > > > > > 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. >
