From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= Subject: Re: [Qemu-devel] [uq/master PATCH 7/7 v8] target-i386: CPU model subclasses Date: Mon, 10 Feb 2014 01:23:37 +0100 Message-ID: <52F81C09.6070504@suse.de> References: <1391111339-6958-1-git-send-email-ehabkost@redhat.com> <1391111339-6958-8-git-send-email-ehabkost@redhat.com> <20140131181338.GB28427@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm@vger.kernel.org, libvir-list@redhat.com, Paolo Bonzini , Igor Mammedov , Jiri Denemark To: Eduardo Habkost , qemu-devel@nongnu.org Return-path: Received: from cantor2.suse.de ([195.135.220.15]:40217 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751732AbaBJAXn (ORCPT ); Sun, 9 Feb 2014 19:23:43 -0500 In-Reply-To: <20140131181338.GB28427@otherpad.lan.raisama.net> Sender: kvm-owner@vger.kernel.org List-ID: Am 31.01.2014 19:13, schrieb Eduardo Habkost: > Register separate QOM classes for each x86 CPU model. >=20 > This will allow management code to more easily probe what each CPU mo= del > provides, by simply creating objects using the appropriate class name= , > without having to restart QEMU. >=20 > This also allows us to eliminate the qdev_prop_set_globals_for_type() > hack to set CPU-model-specific global properties. >=20 > Instead of creating separate class_init functions for each class, I j= ust > used class_data to store a pointer to the X86CPUDefinition struct for > each CPU model. This should make the patch shorter and easier to revi= ew. > Later we can gradually convert each X86CPUDefinition field to lists o= f > per-class property defaults. >=20 > Written based on the ideas from the patch "[RFC v5] target-i386: Slim > conversion to X86CPU subclasses + KVM subclasses" written by Andreas > F=E4rber , Igor Mammedov . >=20 > The "host" CPU model is special, as the feature flags depend on KVM > being initialized. So it has its own class_init and instance_init > function, and feature flags are set on instance_init instead of > class_init. >=20 > Signed-off-by: Andreas F=E4rber > Signed-off-by: Igor Mammedov > Signed-off-by: Eduardo Habkost > --- > This patch is similar to the one sent by Andrea and then later > resubmitted by Igor as "[RFC v5] target-i386: Slim conversion to X86C= PU > subclasses + KVM subclasses", as it doesn't create one new class_init > function for each subclass. >=20 > Main differences v5 -> v6 are: > * Code was written from scratch (instead of using the previous patch= es > as base) > * I didn't mean to rewrite it entirely, but when doing additional > simplification of the CPU init logic on other patches, I ended u= p > rewriting it. > * I chose to keep the Signed-off-by lines because I built upon > Andreas's and Igor's ideas. Is that OK? Yes, your From and our Sobs in order is the expected way in this case. If Igor agrees I would propose to drop the textual repetition of this. I am ~1/3 through reviewing this and it looks pretty promising so far! Thanks a lot for your efforts. Meanwhile one cleanup idea inline... > * No KVM-specific subclasses, to keep things simpler. > * No embedding of X86CPUDefinition (x86_def_t) inside the class stru= ct, > instead keeping a pointer to the existing X86CPUDefinition struct. > * The "host" class is registered on cpu.c, but the CPUID data > is filled on instance_init instead of class_init (because KVM has = to > be initialized already). > * kvm_required field introduced to make sure the "host" class can'= t > be used without KVM. >=20 > Changes v6 -> v7: > * Rebase >=20 > Changes v7 -> v8: > * Removed CPU listing code (will be sent as a separate patch) > * Kept x86_cpudef_setup() (will be addressed in a separate patch) > --- > target-i386/cpu-qom.h | 13 ++++ > target-i386/cpu.c | 197 ++++++++++++++++++++++++++++++++--------= ---------- > 2 files changed, 138 insertions(+), 72 deletions(-) >=20 > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h > index 722f11a..60c5c32 100644 > --- a/target-i386/cpu-qom.h > +++ b/target-i386/cpu-qom.h > @@ -37,6 +37,9 @@ > #define X86_CPU_GET_CLASS(obj) \ > OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU) > =20 > + > +typedef struct X86CPUDefinition X86CPUDefinition; > + > /** > * X86CPUClass: > * @parent_realize: The parent class' realize handler. > @@ -49,6 +52,16 @@ typedef struct X86CPUClass { > CPUClass parent_class; > /*< public >*/ > =20 > + /* CPU model definition > + * Should be eventually replaced by subclass-specific property d= efaults > + */ > + X86CPUDefinition *cpu_def; > + /* CPU model requires KVM to be enabled */ > + bool kvm_required; > + /* Optional description of CPU model. > + * If unavailable, cpu_def->model_id is used */ > + const char *model_description; Here I wondered why you needed this? For PowerPCCPU subclasses we have reused DeviceClass::desc. Regards, Andreas > + > DeviceRealize parent_realize; > void (*parent_reset)(CPUState *cpu); > } X86CPUClass; [snip] --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45681) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCef7-0001i7-4Y for qemu-devel@nongnu.org; Sun, 09 Feb 2014 19:23:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WCef1-0001m3-58 for qemu-devel@nongnu.org; Sun, 09 Feb 2014 19:23:49 -0500 Received: from cantor2.suse.de ([195.135.220.15]:59296 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WCef0-0001lt-Ra for qemu-devel@nongnu.org; Sun, 09 Feb 2014 19:23:43 -0500 Message-ID: <52F81C09.6070504@suse.de> Date: Mon, 10 Feb 2014 01:23:37 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <1391111339-6958-1-git-send-email-ehabkost@redhat.com> <1391111339-6958-8-git-send-email-ehabkost@redhat.com> <20140131181338.GB28427@otherpad.lan.raisama.net> In-Reply-To: <20140131181338.GB28427@otherpad.lan.raisama.net> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [uq/master PATCH 7/7 v8] target-i386: CPU model subclasses List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost , qemu-devel@nongnu.org Cc: libvir-list@redhat.com, Paolo Bonzini , Jiri Denemark , kvm@vger.kernel.org, Igor Mammedov Am 31.01.2014 19:13, schrieb Eduardo Habkost: > Register separate QOM classes for each x86 CPU model. >=20 > This will allow management code to more easily probe what each CPU mode= l > provides, by simply creating objects using the appropriate class name, > without having to restart QEMU. >=20 > This also allows us to eliminate the qdev_prop_set_globals_for_type() > hack to set CPU-model-specific global properties. >=20 > Instead of creating separate class_init functions for each class, I jus= t > used class_data to store a pointer to the X86CPUDefinition struct for > each CPU model. This should make the patch shorter and easier to review= . > Later we can gradually convert each X86CPUDefinition field to lists of > per-class property defaults. >=20 > Written based on the ideas from the patch "[RFC v5] target-i386: Slim > conversion to X86CPU subclasses + KVM subclasses" written by Andreas > F=E4rber , Igor Mammedov . >=20 > The "host" CPU model is special, as the feature flags depend on KVM > being initialized. So it has its own class_init and instance_init > function, and feature flags are set on instance_init instead of > class_init. >=20 > Signed-off-by: Andreas F=E4rber > Signed-off-by: Igor Mammedov > Signed-off-by: Eduardo Habkost > --- > This patch is similar to the one sent by Andrea and then later > resubmitted by Igor as "[RFC v5] target-i386: Slim conversion to X86CPU > subclasses + KVM subclasses", as it doesn't create one new class_init > function for each subclass. >=20 > Main differences v5 -> v6 are: > * Code was written from scratch (instead of using the previous patches > as base) > * I didn't mean to rewrite it entirely, but when doing additional > simplification of the CPU init logic on other patches, I ended up > rewriting it. > * I chose to keep the Signed-off-by lines because I built upon > Andreas's and Igor's ideas. Is that OK? Yes, your From and our Sobs in order is the expected way in this case. If Igor agrees I would propose to drop the textual repetition of this. I am ~1/3 through reviewing this and it looks pretty promising so far! Thanks a lot for your efforts. Meanwhile one cleanup idea inline... > * No KVM-specific subclasses, to keep things simpler. > * No embedding of X86CPUDefinition (x86_def_t) inside the class struct= , > instead keeping a pointer to the existing X86CPUDefinition struct. > * The "host" class is registered on cpu.c, but the CPUID data > is filled on instance_init instead of class_init (because KVM has to > be initialized already). > * kvm_required field introduced to make sure the "host" class can't > be used without KVM. >=20 > Changes v6 -> v7: > * Rebase >=20 > Changes v7 -> v8: > * Removed CPU listing code (will be sent as a separate patch) > * Kept x86_cpudef_setup() (will be addressed in a separate patch) > --- > target-i386/cpu-qom.h | 13 ++++ > target-i386/cpu.c | 197 ++++++++++++++++++++++++++++++++----------= -------- > 2 files changed, 138 insertions(+), 72 deletions(-) >=20 > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h > index 722f11a..60c5c32 100644 > --- a/target-i386/cpu-qom.h > +++ b/target-i386/cpu-qom.h > @@ -37,6 +37,9 @@ > #define X86_CPU_GET_CLASS(obj) \ > OBJECT_GET_CLASS(X86CPUClass, (obj), TYPE_X86_CPU) > =20 > + > +typedef struct X86CPUDefinition X86CPUDefinition; > + > /** > * X86CPUClass: > * @parent_realize: The parent class' realize handler. > @@ -49,6 +52,16 @@ typedef struct X86CPUClass { > CPUClass parent_class; > /*< public >*/ > =20 > + /* CPU model definition > + * Should be eventually replaced by subclass-specific property def= aults > + */ > + X86CPUDefinition *cpu_def; > + /* CPU model requires KVM to be enabled */ > + bool kvm_required; > + /* Optional description of CPU model. > + * If unavailable, cpu_def->model_id is used */ > + const char *model_description; Here I wondered why you needed this? For PowerPCCPU subclasses we have reused DeviceClass::desc. Regards, Andreas > + > DeviceRealize parent_realize; > void (*parent_reset)(CPUState *cpu); > } X86CPUClass; [snip] --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg