| Message ID | 1464799050-11002-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 01, 2016 at 06:37:23PM +0200, Igor Mammedov wrote: > it will allow to make x86_cpu_parse_featurestr() a pure convertor > of legacy features string into global properties. > That way -cpu FOOCPU,feat1=x,feat2=y,... is parsed only once I would write it as "will be parsed only once (this will be implemented by the next patches)". > and as result of x86_cpu_parse_featurestr() a corresponding set > of global properties for specified CPU type are registered. Global properties are not registered yet (after applying this patch), so please explain that the static variables are a temporary hack that will be replaced by global properties in following patches. > So whenever a CPU device is created, generic device machinery > will apply that global properties for us. I would write "apply those global properties" instead. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 36 ++++++++++++++++++------------------ > 1 file changed, 18 insertions(+), 18 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 3fbc6f3..6159a7f 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > } > } > > +/* Features to be added */ Please add something like "Features to be added. Will be replaced by global variables in the future". > +static FeatureWordArray plus_features = { 0 }; > +/* Features to be removed */ > +static FeatureWordArray minus_features = { 0 }; > + I see that this hack is replaced by the following patches, but is there an easy way to remove the CPUState argument from x86_cpu_parse_featurestr() before we introduce these static variables? (No problem if there's no way to do that, as long as the static variables are explicitly documented as a temporary hack) > /* Parse "+feature,-feature,feature=foo" CPU feature string > */ > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > @@ -1939,13 +1944,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > { > X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > - FeatureWord w; > - /* Features to be added */ > - FeatureWordArray plus_features = { 0 }; > - /* Features to be removed */ > - FeatureWordArray minus_features = { 0 }; > uint32_t numvalue; > - CPUX86State *env = &cpu->env; > Error *local_err = NULL; > > featurestr = features ? strtok(features, ",") : NULL; > @@ -2019,18 +2018,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > } > featurestr = strtok(NULL, ","); > } > - > - if (cpu->host_features) { > - for (w = 0; w < FEATURE_WORDS; w++) { > - env->features[w] = > - x86_cpu_get_supported_feature_word(w, cpu->migratable); > - } > - } > - > - for (w = 0; w < FEATURE_WORDS; w++) { > - env->features[w] |= plus_features[w]; > - env->features[w] &= ~minus_features[w]; > - } > } > > /* Print all cpuid feature names in featureset > @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) > CPUX86State *env = &cpu->env; > Error *local_err = NULL; > static bool ht_warned; > + FeatureWord w; > > if (cpu->apic_id < 0) { > error_setg(errp, "apic-id property was not initialized properly"); > return; > } > > + if (cpu->host_features) { > + for (w = 0; w < FEATURE_WORDS; w++) { > + env->features[w] = > + x86_cpu_get_supported_feature_word(w, cpu->migratable); > + } > + } > + > + for (w = 0; w < FEATURE_WORDS; w++) { > + cpu->env.features[w] |= plus_features[w]; > + cpu->env.features[w] &= ~minus_features[w]; > + } > + > if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) { > env->cpuid_level = 7; > } > -- > 1.8.3.1 >
On Wed, 1 Jun 2016 14:43:09 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 3fbc6f3..6159a7f 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > } > > } > > > > +/* Features to be added */ > > Please add something like "Features to be added. Will be replaced > by global variables in the future". > > > +static FeatureWordArray plus_features = { 0 }; > > +/* Features to be removed */ > > +static FeatureWordArray minus_features = { 0 }; > > + > > I see that this hack is replaced by the following patches, but is > there an easy way to remove the CPUState argument from > x86_cpu_parse_featurestr() before we introduce these static > variables? (No problem if there's no way to do that, as long as > the static variables are explicitly documented as a temporary > hack) It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) local to x86 that probably would stay here forever. I should add comment that explains why +- can't be replaced with normal properties. I don't plan to replace plus/minus_features with anything nor to make this variables a global ones to spread +- x86/sparc legacy format everywhere. What I would do though before enabling -device/device_add for X86CPU is to disable +- handling for new machine types so that CPUs would follow generic property semantic of device used everywhere else. > > > /* Parse "+feature,-feature,feature=foo" CPU feature string > > */ > > static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > > @@ -1939,13 +1944,7 @@ static void > > x86_cpu_parse_featurestr(CPUState *cs, char *features, { > > X86CPU *cpu = X86_CPU(cs); > > char *featurestr; /* Single 'key=value" string being parsed */ > > - FeatureWord w; > > - /* Features to be added */ > > - FeatureWordArray plus_features = { 0 }; > > - /* Features to be removed */ > > - FeatureWordArray minus_features = { 0 }; > > uint32_t numvalue; > > - CPUX86State *env = &cpu->env; > > Error *local_err = NULL; > > > > featurestr = features ? strtok(features, ",") : NULL; > > @@ -2019,18 +2018,6 @@ static void > > x86_cpu_parse_featurestr(CPUState *cs, char *features, } > > featurestr = strtok(NULL, ","); > > } > > - > > - if (cpu->host_features) { > > - for (w = 0; w < FEATURE_WORDS; w++) { > > - env->features[w] = > > - x86_cpu_get_supported_feature_word(w, > > cpu->migratable); > > - } > > - } > > - > > - for (w = 0; w < FEATURE_WORDS; w++) { > > - env->features[w] |= plus_features[w]; > > - env->features[w] &= ~minus_features[w]; > > - } > > } > > > > /* Print all cpuid feature names in featureset > > @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState > > *dev, Error **errp) CPUX86State *env = &cpu->env; > > Error *local_err = NULL; > > static bool ht_warned; > > + FeatureWord w; > > > > if (cpu->apic_id < 0) { > > error_setg(errp, "apic-id property was not initialized > > properly"); return; > > } > > > > + if (cpu->host_features) { > > + for (w = 0; w < FEATURE_WORDS; w++) { > > + env->features[w] = > > + x86_cpu_get_supported_feature_word(w, > > cpu->migratable); > > + } > > + } > > + > > + for (w = 0; w < FEATURE_WORDS; w++) { > > + cpu->env.features[w] |= plus_features[w]; > > + cpu->env.features[w] &= ~minus_features[w]; > > + } > > + > > if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) { > > env->cpuid_level = 7; > > } > > -- > > 1.8.3.1 > > >
On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > On Wed, 1 Jun 2016 14:43:09 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > [...] > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 3fbc6f3..6159a7f 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > } > > > } > > > > > > +/* Features to be added */ > > > > Please add something like "Features to be added. Will be replaced > > by global variables in the future". > > > > > +static FeatureWordArray plus_features = { 0 }; > > > +/* Features to be removed */ > > > +static FeatureWordArray minus_features = { 0 }; > > > + > > > > I see that this hack is replaced by the following patches, but is > > there an easy way to remove the CPUState argument from > > x86_cpu_parse_featurestr() before we introduce these static > > variables? (No problem if there's no way to do that, as long as > > the static variables are explicitly documented as a temporary > > hack) > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > local to x86 that probably would stay here forever. > I should add comment that explains why +- can't be replaced > with normal properties. Oh, I assumed it would be temporary. In that case, I would like to avoid adding the static variables if possible. > > I don't plan to replace plus/minus_features with anything nor to > make this variables a global ones to spread +- x86/sparc legacy > format everywhere. Can't the +/- semantics be emulated by simply registering plus_features/minus_features after the other global properties are registered inside x86_cpu_parse_featurestr()? > > What I would do though before enabling -device/device_add for X86CPU is > to disable +- handling for new machine types so that CPUs would > follow generic property semantic of device used everywhere else. We can't do that unless we give libvirt (and users that have their own scripts) time to adapt to the new syntax (and we warn users that newer QEMU versions will require newer libvirt).
On Thu, 2 Jun 2016 11:38:26 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > On Wed, 1 Jun 2016 14:43:09 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > [...] > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > index 3fbc6f3..6159a7f 100644 > > > > --- a/target-i386/cpu.c > > > > +++ b/target-i386/cpu.c > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > } > > > > } > > > > > > > > +/* Features to be added */ > > > > > > Please add something like "Features to be added. Will be replaced > > > by global variables in the future". > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > +/* Features to be removed */ > > > > +static FeatureWordArray minus_features = { 0 }; > > > > + > > > > > > I see that this hack is replaced by the following patches, but is > > > there an easy way to remove the CPUState argument from > > > x86_cpu_parse_featurestr() before we introduce these static > > > variables? (No problem if there's no way to do that, as long as > > > the static variables are explicitly documented as a temporary > > > hack) > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > > local to x86 that probably would stay here forever. > > I should add comment that explains why +- can't be replaced > > with normal properties. > > Oh, I assumed it would be temporary. In that case, I would like > to avoid adding the static variables if possible. > > > > > I don't plan to replace plus/minus_features with anything nor to > > make this variables a global ones to spread +- x86/sparc legacy > > format everywhere. > > Can't the +/- semantics be emulated by simply registering > plus_features/minus_features after the other global properties > are registered inside x86_cpu_parse_featurestr()? it could be done, at the first glance it will take 2 extra parsing passes 1: copy featurestr, parse feat=x,feat 2: copy featurestr, parse +feat 3: copy featurestr, parse -feat but that probably will complicate way to disable +-feat handling in future, with current static vars it's just a matter of specifying compat-prop for X86CPU driver in appropriate machine type. So I'd leave it as is unless you insist on doing it like you suggested above. > > > > What I would do though before enabling -device/device_add for X86CPU is > > to disable +- handling for new machine types so that CPUs would > > follow generic property semantic of device used everywhere else. > > We can't do that unless we give libvirt (and users that have > their own scripts) time to adapt to the new syntax leaving it enabled will lead to mixed semantics in combination with device_add that will be even more confusing to users if users will use both: for example: -cpu cpu,-featx and -device cpu,featx=on That's why I'm suggesting to make a clean break in new machine type with error saying to replace legacy +-feat with canonical one. For old machine types nothing would break as it would still use legacy syntax and legacy cpu-add, with device_add disabled. We probably can fix libvirt in sync with this QEMU release if it still uses +- syntax. > (and we warn users that newer QEMU versions will require newer libvirt). yep we should do it in release notes.
On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote: > On Thu, 2 Jun 2016 11:38:26 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > > On Wed, 1 Jun 2016 14:43:09 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > [...] > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > index 3fbc6f3..6159a7f 100644 > > > > > --- a/target-i386/cpu.c > > > > > +++ b/target-i386/cpu.c > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > > } > > > > > } > > > > > > > > > > +/* Features to be added */ > > > > > > > > Please add something like "Features to be added. Will be replaced > > > > by global variables in the future". > > > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > > +/* Features to be removed */ > > > > > +static FeatureWordArray minus_features = { 0 }; > > > > > + > > > > > > > > I see that this hack is replaced by the following patches, but is > > > > there an easy way to remove the CPUState argument from > > > > x86_cpu_parse_featurestr() before we introduce these static > > > > variables? (No problem if there's no way to do that, as long as > > > > the static variables are explicitly documented as a temporary > > > > hack) > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > > > local to x86 that probably would stay here forever. > > > I should add comment that explains why +- can't be replaced > > > with normal properties. > > > > Oh, I assumed it would be temporary. In that case, I would like > > to avoid adding the static variables if possible. > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > make this variables a global ones to spread +- x86/sparc legacy > > > format everywhere. > > > > Can't the +/- semantics be emulated by simply registering > > plus_features/minus_features after the other global properties > > are registered inside x86_cpu_parse_featurestr()? > it could be done, at the first glance it will take 2 extra parsing passes > > 1: copy featurestr, parse feat=x,feat > 2: copy featurestr, parse +feat > 3: copy featurestr, parse -feat Why? Can't we just replace plus_features and minus_features with two string lists (or a QDict), and make the corresponding object_property_parse()/qdev_prop_register_global() calls after the main parsing loop? (Didn't you do that in your old "target-i386: set [+-]feature using static properties" patch?) > > but that probably will complicate way to disable +-feat handling in future, > with current static vars it's just a matter of specifying compat-prop > for X86CPU driver in appropriate machine type. I see. But I don't see why we need the extra machine-type cruft. To me, the whole point of removing the old syntax is to make the code simpler. If we have to add even more code/complexity just to add a machine-type restriction (but keeping the old code there), I don't see the point. I believe we should either: 1) remove it completely after people have time to update their scripts/libvirt; or 2) just keep it working on all machine-types, but print a warning every time people use the old syntax. (I am not sure if we should do (1) without giving users a long time to adapt, so I suggest we do (2) by now) > So I'd leave it as is unless you insist on doing it like you suggested above. > > > > > > > What I would do though before enabling -device/device_add for X86CPU is > > > to disable +- handling for new machine types so that CPUs would > > > follow generic property semantic of device used everywhere else. > > > > We can't do that unless we give libvirt (and users that have > > their own scripts) time to adapt to the new syntax > leaving it enabled will lead to mixed semantics in combination > with device_add that will be even more confusing to users > if users will use both: > for example: -cpu cpu,-featx and -device cpu,featx=on > That's why I'm suggesting to make a clean break in new machine > type with error saying to replace legacy +-feat with canonical one. > For old machine types nothing would break as it would still use > legacy syntax and legacy cpu-add, with device_add disabled. Just warn users very clearly, if they use the old syntax. If they insist in using it, they shouldn't blame us if they get confused. > > We probably can fix libvirt in sync with this QEMU release > if it still uses +- syntax. I would wait for at least 1 or 2 libvirt releases before removing support. But as I said above: if we are not deleting any code (and are adding extra code instead), I don't see the point of forcibly disabling it. We can just leave it there and print a warning. > > > (and we warn users that newer QEMU versions will require newer libvirt). > yep we should do it in release notes.
On Thu, 2 Jun 2016 14:34:27 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote: > > On Thu, 2 Jun 2016 11:38:26 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > > > On Wed, 1 Jun 2016 14:43:09 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > [...] > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > > index 3fbc6f3..6159a7f 100644 > > > > > > --- a/target-i386/cpu.c > > > > > > +++ b/target-i386/cpu.c > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > > > } > > > > > > } > > > > > > > > > > > > +/* Features to be added */ > > > > > > > > > > Please add something like "Features to be added. Will be > > > > > replaced by global variables in the future". > > > > > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > > > +/* Features to be removed */ > > > > > > +static FeatureWordArray minus_features = { 0 }; > > > > > > + > > > > > > > > > > I see that this hack is replaced by the following patches, > > > > > but is there an easy way to remove the CPUState argument from > > > > > x86_cpu_parse_featurestr() before we introduce these static > > > > > variables? (No problem if there's no way to do that, as long > > > > > as the static variables are explicitly documented as a > > > > > temporary hack) > > > > It's hack to keep legacy +- semantic (i.e. it overrides > > > > feat1=x,feat2) local to x86 that probably would stay here > > > > forever. I should add comment that explains why +- can't be > > > > replaced with normal properties. > > > > > > Oh, I assumed it would be temporary. In that case, I would like > > > to avoid adding the static variables if possible. > > > > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > > make this variables a global ones to spread +- x86/sparc legacy > > > > format everywhere. > > > > > > Can't the +/- semantics be emulated by simply registering > > > plus_features/minus_features after the other global properties > > > are registered inside x86_cpu_parse_featurestr()? > > it could be done, at the first glance it will take 2 extra parsing > > passes > > > > 1: copy featurestr, parse feat=x,feat > > 2: copy featurestr, parse +feat > > 3: copy featurestr, parse -feat > > Why? Can't we just replace plus_features and minus_features with > two string lists (or a QDict), and make the corresponding > object_property_parse()/qdev_prop_register_global() calls after > the main parsing loop? > > (Didn't you do that in your old "target-i386: set [+-]feature > using static properties" patch?) ok, I'll do that. > > > > > but that probably will complicate way to disable +-feat handling in > > future, with current static vars it's just a matter of specifying > > compat-prop for X86CPU driver in appropriate machine type. > > I see. But I don't see why we need the extra machine-type cruft. > > To me, the whole point of removing the old syntax is to make the > code simpler. If we have to add even more code/complexity just to > add a machine-type restriction (but keeping the old code there), > I don't see the point. > > I believe we should either: 1) remove it completely after people > have time to update their scripts/libvirt; or 2) just keep it > working on all machine-types, but print a warning every time > people use the old syntax. > > (I am not sure if we should do (1) without giving users a long > time to adapt, so I suggest we do (2) by now) > > > > So I'd leave it as is unless you insist on doing it like you > > suggested above. > > > > > > > > What I would do though before enabling -device/device_add for > > > > X86CPU is to disable +- handling for new machine types so that > > > > CPUs would follow generic property semantic of device used > > > > everywhere else. > > > > > > We can't do that unless we give libvirt (and users that have > > > their own scripts) time to adapt to the new syntax > > leaving it enabled will lead to mixed semantics in combination > > with device_add that will be even more confusing to users > > if users will use both: > > for example: -cpu cpu,-featx and -device cpu,featx=on > > That's why I'm suggesting to make a clean break in new machine > > type with error saying to replace legacy +-feat with canonical one. > > For old machine types nothing would break as it would still use > > legacy syntax and legacy cpu-add, with device_add disabled. > > Just warn users very clearly, if they use the old syntax. If they > insist in using it, they shouldn't blame us if they get confused. > > > > > We probably can fix libvirt in sync with this QEMU release > > if it still uses +- syntax. > > I would wait for at least 1 or 2 libvirt releases before removing > support. > > But as I said above: if we are not deleting any code (and are > adding extra code instead), I don't see the point of forcibly > disabling it. We can just leave it there and print a warning. ok, lets go with warning, saying" "[+-]feature syntax is obsoleted and will be removed in future, it's recommended to use canonical property syntax 'feature=value'" > > > > > > (and we warn users that newer QEMU versions will require newer > > > libvirt). > > yep we should do it in release notes.
On Thu, Jun 02, 2016 at 08:23:37PM +0200, Igor Mammedov wrote: > On Thu, 2 Jun 2016 14:34:27 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: [...] > > > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > > > make this variables a global ones to spread +- x86/sparc legacy > > > > > format everywhere. > > > > > > > > Can't the +/- semantics be emulated by simply registering > > > > plus_features/minus_features after the other global properties > > > > are registered inside x86_cpu_parse_featurestr()? > > > it could be done, at the first glance it will take 2 extra parsing > > > passes > > > > > > 1: copy featurestr, parse feat=x,feat > > > 2: copy featurestr, parse +feat > > > 3: copy featurestr, parse -feat > > > > Why? Can't we just replace plus_features and minus_features with > > two string lists (or a QDict), and make the corresponding > > object_property_parse()/qdev_prop_register_global() calls after > > the main parsing loop? > > > > (Didn't you do that in your old "target-i386: set [+-]feature > > using static properties" patch?) > ok, I'll do that. Thanks! [...] > > But as I said above: if we are not deleting any code (and are > > adding extra code instead), I don't see the point of forcibly > > disabling it. We can just leave it there and print a warning. > ok, lets go with warning, saying" > > "[+-]feature syntax is obsoleted and will be removed in future, > it's recommended to use canonical property syntax 'feature=value'" We could print "+%s is obsolete [...] use '%s=on'" and "-%s is obsolete [...] use '%s=off'", depending on the case. This way, users can see more easily what's the proper syntax.
On Thu, 2 Jun 2016 14:34:27 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote: > > On Thu, 2 Jun 2016 11:38:26 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > > > On Wed, 1 Jun 2016 14:43:09 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > [...] > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > > index 3fbc6f3..6159a7f 100644 > > > > > > --- a/target-i386/cpu.c > > > > > > +++ b/target-i386/cpu.c > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > > > } > > > > > > } > > > > > > > > > > > > +/* Features to be added */ > > > > > > > > > > Please add something like "Features to be added. Will be replaced > > > > > by global variables in the future". > > > > > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > > > +/* Features to be removed */ > > > > > > +static FeatureWordArray minus_features = { 0 }; > > > > > > + > > > > > > > > > > I see that this hack is replaced by the following patches, but is > > > > > there an easy way to remove the CPUState argument from > > > > > x86_cpu_parse_featurestr() before we introduce these static > > > > > variables? (No problem if there's no way to do that, as long as > > > > > the static variables are explicitly documented as a temporary > > > > > hack) > > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > > > > local to x86 that probably would stay here forever. > > > > I should add comment that explains why +- can't be replaced > > > > with normal properties. > > > > > > Oh, I assumed it would be temporary. In that case, I would like > > > to avoid adding the static variables if possible. > > > > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > > make this variables a global ones to spread +- x86/sparc legacy > > > > format everywhere. > > > > > > Can't the +/- semantics be emulated by simply registering > > > plus_features/minus_features after the other global properties > > > are registered inside x86_cpu_parse_featurestr()? > > it could be done, at the first glance it will take 2 extra parsing passes > > > > 1: copy featurestr, parse feat=x,feat > > 2: copy featurestr, parse +feat > > 3: copy featurestr, parse -feat > > Why? Can't we just replace plus_features and minus_features with > two string lists (or a QDict), and make the corresponding > object_property_parse()/qdev_prop_register_global() calls after > the main parsing loop? > > (Didn't you do that in your old "target-i386: set [+-]feature > using static properties" patch?) It doesn't look like it will work due to broken 4d1b279b0 as plus_features/minus_features are applied after: if (cpu->host_features) { for (w = 0; w < FEATURE_WORDS; w++) { env->features[w] = x86_cpu_get_supported_feature_word(w, cpu->migratable); } } and with above moving to realize(), +-feats would be overwritten by it. Lets temporary use static variables as in this patch so not to delay series on not related fixes. And deal with it when 4d1b279b0 is fixed. 1 way to deal with it is to wait several releases till users fix their +-feats CLIs and then just drop it.
On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote: > On Thu, 2 Jun 2016 14:34:27 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote: > > > On Thu, 2 Jun 2016 11:38:26 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > > > > On Wed, 1 Jun 2016 14:43:09 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > [...] > > > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > > > index 3fbc6f3..6159a7f 100644 > > > > > > > --- a/target-i386/cpu.c > > > > > > > +++ b/target-i386/cpu.c > > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +/* Features to be added */ > > > > > > > > > > > > Please add something like "Features to be added. Will be replaced > > > > > > by global variables in the future". > > > > > > > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > > > > +/* Features to be removed */ > > > > > > > +static FeatureWordArray minus_features = { 0 }; > > > > > > > + > > > > > > > > > > > > I see that this hack is replaced by the following patches, but is > > > > > > there an easy way to remove the CPUState argument from > > > > > > x86_cpu_parse_featurestr() before we introduce these static > > > > > > variables? (No problem if there's no way to do that, as long as > > > > > > the static variables are explicitly documented as a temporary > > > > > > hack) > > > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > > > > > local to x86 that probably would stay here forever. > > > > > I should add comment that explains why +- can't be replaced > > > > > with normal properties. > > > > > > > > Oh, I assumed it would be temporary. In that case, I would like > > > > to avoid adding the static variables if possible. > > > > > > > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > > > make this variables a global ones to spread +- x86/sparc legacy > > > > > format everywhere. > > > > > > > > Can't the +/- semantics be emulated by simply registering > > > > plus_features/minus_features after the other global properties > > > > are registered inside x86_cpu_parse_featurestr()? > > > it could be done, at the first glance it will take 2 extra parsing passes > > > > > > 1: copy featurestr, parse feat=x,feat > > > 2: copy featurestr, parse +feat > > > 3: copy featurestr, parse -feat > > > > Why? Can't we just replace plus_features and minus_features with > > two string lists (or a QDict), and make the corresponding > > object_property_parse()/qdev_prop_register_global() calls after > > the main parsing loop? > > > > (Didn't you do that in your old "target-i386: set [+-]feature > > using static properties" patch?) > It doesn't look like it will work due to broken 4d1b279b0 as > plus_features/minus_features are applied after: > > if (cpu->host_features) { > for (w = 0; w < FEATURE_WORDS; w++) { > env->features[w] = > x86_cpu_get_supported_feature_word(w, cpu->migratable); > } > } > > and with above moving to realize(), +-feats would be overwritten by it. > Lets temporary use static variables as in this patch so not to delay > series on not related fixes. And deal with it when 4d1b279b0 is fixed. > > 1 way to deal with it is to wait several releases till users fix their > +-feats CLIs and then just drop it. We can fix that after getting rid the host_features hack (and also fix the "-cpu host,foo=off,foo=on" bug we already have). In that case, I think we can live with the static variables temporarily in the meantime. Can you add a comment above the static variable declarations saying that they can't be replaced by globals yet because of the host_features hack?
On Fri, 3 Jun 2016 16:26:22 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Jun 03, 2016 at 12:13:18PM +0200, Igor Mammedov wrote: > > On Thu, 2 Jun 2016 14:34:27 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote: > > > > On Thu, 2 Jun 2016 11:38:26 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Thu, Jun 02, 2016 at 11:59:30AM +0200, Igor Mammedov wrote: > > > > > > On Wed, 1 Jun 2016 14:43:09 -0300 > > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > [...] > > > > > > > > > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > > > > > > index 3fbc6f3..6159a7f 100644 > > > > > > > > --- a/target-i386/cpu.c > > > > > > > > +++ b/target-i386/cpu.c > > > > > > > > @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) > > > > > > > > } > > > > > > > > } > > > > > > > > > > > > > > > > +/* Features to be added */ > > > > > > > > > > > > > > Please add something like "Features to be added. Will be replaced > > > > > > > by global variables in the future". > > > > > > > > > > > > > > > +static FeatureWordArray plus_features = { 0 }; > > > > > > > > +/* Features to be removed */ > > > > > > > > +static FeatureWordArray minus_features = { 0 }; > > > > > > > > + > > > > > > > > > > > > > > I see that this hack is replaced by the following patches, but is > > > > > > > there an easy way to remove the CPUState argument from > > > > > > > x86_cpu_parse_featurestr() before we introduce these static > > > > > > > variables? (No problem if there's no way to do that, as long as > > > > > > > the static variables are explicitly documented as a temporary > > > > > > > hack) > > > > > > It's hack to keep legacy +- semantic (i.e. it overrides feat1=x,feat2) > > > > > > local to x86 that probably would stay here forever. > > > > > > I should add comment that explains why +- can't be replaced > > > > > > with normal properties. > > > > > > > > > > Oh, I assumed it would be temporary. In that case, I would like > > > > > to avoid adding the static variables if possible. > > > > > > > > > > > > > > > > > I don't plan to replace plus/minus_features with anything nor to > > > > > > make this variables a global ones to spread +- x86/sparc legacy > > > > > > format everywhere. > > > > > > > > > > Can't the +/- semantics be emulated by simply registering > > > > > plus_features/minus_features after the other global properties > > > > > are registered inside x86_cpu_parse_featurestr()? > > > > it could be done, at the first glance it will take 2 extra parsing passes > > > > > > > > 1: copy featurestr, parse feat=x,feat > > > > 2: copy featurestr, parse +feat > > > > 3: copy featurestr, parse -feat > > > > > > Why? Can't we just replace plus_features and minus_features with > > > two string lists (or a QDict), and make the corresponding > > > object_property_parse()/qdev_prop_register_global() calls after > > > the main parsing loop? > > > > > > (Didn't you do that in your old "target-i386: set [+-]feature > > > using static properties" patch?) > > It doesn't look like it will work due to broken 4d1b279b0 as > > plus_features/minus_features are applied after: > > > > if (cpu->host_features) { > > for (w = 0; w < FEATURE_WORDS; w++) { > > env->features[w] = > > x86_cpu_get_supported_feature_word(w, cpu->migratable); > > } > > } > > > > and with above moving to realize(), +-feats would be overwritten by it. > > Lets temporary use static variables as in this patch so not to delay > > series on not related fixes. And deal with it when 4d1b279b0 is fixed. > > > > 1 way to deal with it is to wait several releases till users fix their > > +-feats CLIs and then just drop it. > > We can fix that after getting rid the host_features hack (and > also fix the "-cpu host,foo=off,foo=on" bug we already have). > > In that case, I think we can live with the static variables > temporarily in the meantime. Can you add a comment above the > static variable declarations saying that they can't be replaced > by globals yet because of the host_features hack? Sure, thanks!
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 3fbc6f3..6159a7f 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1932,6 +1932,11 @@ static inline void feat2prop(char *s) } } +/* Features to be added */ +static FeatureWordArray plus_features = { 0 }; +/* Features to be removed */ +static FeatureWordArray minus_features = { 0 }; + /* Parse "+feature,-feature,feature=foo" CPU feature string */ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, @@ -1939,13 +1944,7 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, { X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ - FeatureWord w; - /* Features to be added */ - FeatureWordArray plus_features = { 0 }; - /* Features to be removed */ - FeatureWordArray minus_features = { 0 }; uint32_t numvalue; - CPUX86State *env = &cpu->env; Error *local_err = NULL; featurestr = features ? strtok(features, ",") : NULL; @@ -2019,18 +2018,6 @@ static void x86_cpu_parse_featurestr(CPUState *cs, char *features, } featurestr = strtok(NULL, ","); } - - if (cpu->host_features) { - for (w = 0; w < FEATURE_WORDS; w++) { - env->features[w] = - x86_cpu_get_supported_feature_word(w, cpu->migratable); - } - } - - for (w = 0; w < FEATURE_WORDS; w++) { - env->features[w] |= plus_features[w]; - env->features[w] &= ~minus_features[w]; - } } /* Print all cpuid feature names in featureset @@ -2912,12 +2899,25 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp) CPUX86State *env = &cpu->env; Error *local_err = NULL; static bool ht_warned; + FeatureWord w; if (cpu->apic_id < 0) { error_setg(errp, "apic-id property was not initialized properly"); return; } + if (cpu->host_features) { + for (w = 0; w < FEATURE_WORDS; w++) { + env->features[w] = + x86_cpu_get_supported_feature_word(w, cpu->migratable); + } + } + + for (w = 0; w < FEATURE_WORDS; w++) { + cpu->env.features[w] |= plus_features[w]; + cpu->env.features[w] &= ~minus_features[w]; + } + if (env->features[FEAT_7_0_EBX] && env->cpuid_level < 7) { env->cpuid_level = 7; }
it will allow to make x86_cpu_parse_featurestr() a pure convertor of legacy features string into global properties. That way -cpu FOOCPU,feat1=x,feat2=y,... is parsed only once and as result of x86_cpu_parse_featurestr() a corresponding set of global properties for specified CPU type are registered. So whenever a CPU device is created, generic device machinery will apply that global properties for us. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)