From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41787) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b8Vvf-0005hr-UW for qemu-devel@nongnu.org; Thu, 02 Jun 2016 12:57:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b8Vvd-0006Vh-Nl for qemu-devel@nongnu.org; Thu, 02 Jun 2016 12:57:06 -0400 Date: Thu, 2 Jun 2016 18:56:55 +0200 From: Igor Mammedov Message-ID: <20160602185655.20d7c7e0@nial.brq.redhat.com> In-Reply-To: <20160602143826.GD19055@thinpad.lan.raisama.net> References: <1464799050-11002-1-git-send-email-imammedo@redhat.com> <1464799050-11002-2-git-send-email-imammedo@redhat.com> <20160601174309.GA13503@thinpad.lan.raisama.net> <20160602115930.6945e5d6@igors-macbook-pro.local> <20160602143826.GD19055@thinpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 1/8] target-i386: cpu: move features logic that requires CPUState to realize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: qemu-devel@nongnu.org, peter.maydell@linaro.org, pbonzini@redhat.com, rth@twiddle.net, blauwirbel@gmail.com, mark.cave-ayland@ilande.co.uk, qemu-arm@nongnu.org On Thu, 2 Jun 2016 11:38:26 -0300 Eduardo Habkost 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 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.