Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time

[Igor Mammedov <imammedo@redhat.com>]

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.