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

[Eduardo Habkost]

On Thu, Jun 02, 2016 at 06:56:55PM +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 11:38:26 -0300
> Eduardo Habkost <address@hidden> 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 <address@hidden> 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.

-- 
Eduardo