On Fri, 18 Aug 2017 14:40:29 -0300
Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Thu, Aug 17, 2017 at 04:07:56PM +0200, Igor Mammedov wrote:
> > Since
> > (commit d4a606b3 i386: Don't override -cpu options on -cpu host/max)
> > it became possible to delete hack where it was necessary to
> > postpone applying plus/minus features to realize time
> > after max_features were applied to keep legacy +-feat
> > override semantics.
> >
> > With above commit it's possible to convert +-feat to a set
> > of GlobalProperty items and keep +-feat override semantics,
> > these properties should be added to global list at the end
> > to override properties that were set with feat=on|off syntax.
> >
> > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> [...]
> > +/* Parse "+feature,-feature,feature=foo" CPU feature string */
> > static void x86_cpu_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;
>
> The warning about ambiguous CPU options exists since 2.8, I think
> this is a good opportunity to get rid of the "[+-]feat overrides
> feat=on|off" rule and simplify the parsing code. Do you want to
> do this in the same patch?
I'd prefer not to do it in this patch/series, as it's not related.
WE can do cleanups later on top.
>
>
> > char *featurestr; /* Single 'key=value" string being parsed */
> > static bool cpu_globals_initialized;
> > bool ambiguous = false;
> > @@ -2095,7 +2104,6 @@ static void x86_cpu_parse_featurestr(const char
> > *typename, char *features,
> > const char *val = NULL;
> > char *eq = NULL;
> > char num[32];
> > - GlobalProperty *prop;
> >
> > /* Compatibility syntax: */
> > if (featurestr[0] == '+') {
> > @@ -2147,21 +2155,32 @@ static void x86_cpu_parse_featurestr(const char
> > *typename, char *features,
> > name = "tsc-frequency";
> > }
> >
> > - 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);
> > + 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");
> > }
> > +
> > + 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);
> > + }
> > }
> >
> > -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp);
> > +static void x86_cpu_expand_features(X86CPU *cpu);
> > static int x86_cpu_filter_features(X86CPU *cpu);
> >
> > /* Check for missing features that may prevent the CPU class from
> > @@ -2172,7 +2191,6 @@ static void
> > x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> > {
> > X86CPU *xc;
> > FeatureWord w;
> > - Error *err = NULL;
> > strList **next = missing_feats;
> >
> > if (xcc->kvm_required && !kvm_enabled()) {
> > @@ -2184,18 +2202,7 @@ static void
> > x86_cpu_class_check_missing_features(X86CPUClass *xcc,
> >
> > xc = X86_CPU(object_new(object_class_get_name(OBJECT_CLASS(xcc))));
> >
> > - x86_cpu_expand_features(xc, &err);
> > - if (err) {
> > - /* Errors at x86_cpu_expand_features should never happen,
> > - * but in case it does, just report the model as not
> > - * runnable at all using the "type" property.
> > - */
> > - strList *new = g_new0(strList, 1);
> > - new->value = g_strdup("type");
> > - *next = new;
> > - next = &new->next;
> > - }
> > -
> > + x86_cpu_expand_features(xc);
> > x86_cpu_filter_features(xc);
> >
> > for (w = 0; w < FEATURE_WORDS; w++) {
> > @@ -2559,11 +2566,7 @@ static X86CPU *x86_cpu_from_model(const char *model,
> > QDict *props, Error **errp)
> > }
> > }
> >
> > - x86_cpu_expand_features(xc, &err);
> > - if (err) {
> > - goto out;
> > - }
> > -
> > + x86_cpu_expand_features(xc);
> > out:
> > if (err) {
> > error_propagate(errp, err);
> > @@ -3453,18 +3456,11 @@ static void x86_cpu_enable_xsave_components(X86CPU
> > *cpu)
> > /* Expand CPU configuration data, based on configured features
> > * and host/accelerator capabilities when appropriate.
> > */
> > -static void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
> > +static void x86_cpu_expand_features(X86CPU *cpu)
> > {
> > CPUX86State *env = &cpu->env;
> > FeatureWord w;
> > - GList *l;
> > - Error *local_err = NULL;
> >
> > - /*TODO: Now cpu->max_features doesn't overwrite features
> > - * set using QOM properties, and we can convert
> > - * plus_features & minus_features to global properties
> > - * inside x86_cpu_parse_featurestr() too.
> > - */
> > if (cpu->max_features) {
> > for (w = 0; w < FEATURE_WORDS; w++) {
> > /* Override only features that weren't set explicitly
> > @@ -3476,22 +3472,6 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> > Error **errp)
> > }
> > }
> >
> > - for (l = plus_features; l; l = l->next) {
> > - const char *prop = l->data;
> > - object_property_set_bool(OBJECT(cpu), true, prop, &local_err);
> > - if (local_err) {
> > - goto out;
> > - }
> > - }
> > -
> > - for (l = minus_features; l; l = l->next) {
> > - const char *prop = l->data;
> > - object_property_set_bool(OBJECT(cpu), false, prop, &local_err);
> > - if (local_err) {
> > - goto out;
> > - }
> > - }
> > -
> > if (!kvm_enabled() || !cpu->expose_kvm) {
> > env->features[FEAT_KVM] = 0;
> > }
> > @@ -3527,11 +3507,6 @@ static void x86_cpu_expand_features(X86CPU *cpu,
> > Error **errp)
> > if (env->cpuid_xlevel2 == UINT32_MAX) {
> > env->cpuid_xlevel2 = env->cpuid_min_xlevel2;
> > }
> > -
> > -out:
> > - if (local_err != NULL) {
> > - error_propagate(errp, local_err);
> > - }
> > }
> >
> > /*
> > @@ -3587,10 +3562,7 @@ static void x86_cpu_realizefn(DeviceState *dev,
> > Error **errp)
> > return;
> > }
> >
> > - x86_cpu_expand_features(cpu, &local_err);
> > - if (local_err) {
> > - goto out;
> > - }
> > + x86_cpu_expand_features(cpu);
> >
> > if (x86_cpu_filter_features(cpu) &&
> > (cpu->check_cpuid || cpu->enforce_cpuid)) {
> > --
> > 2.7.4
> >
> >
>
