Patchwork [RFC,7/9] target-i386: CPU subclass for -cpu "host"

login
register
mail settings
Submitter Eduardo Habkost
Date Dec. 28, 2012, 8:34 p.m.
Message ID <1356726846-10637-8-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/208573/
State New
Headers show

Comments

Eduardo Habkost - Dec. 28, 2012, 8:34 p.m.
Note that we are initializing the CPU features inside instance_init (and
not storing any CPU feature information inside the class struct) because
kvm_cpu_fill_host() needs KVM to be initialized, and we can't guarantee
that KVM will be initialized when class_init is called.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)
Igor Mammedov - Jan. 2, 2013, 7 p.m.
On Fri, 28 Dec 2012 18:34:04 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Note that we are initializing the CPU features inside instance_init (and
> not storing any CPU feature information inside the class struct) because
> kvm_cpu_fill_host() needs KVM to be initialized, and we can't guarantee
> that KVM will be initialized when class_init is called.
initializing defaults in initfn will be broken after we convert features into
static properties due to all initfn()s are called before static properties
defaults are set.

Is it possible to initialize kvm first before calling class_init().

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c824c08..2b6cc3b 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -330,6 +330,14 @@ typedef struct x86_def_t {
>  #define TCG_SVM_FEATURES 0
>  #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
>  
> +
> +/* CPU class name definitions: */
> +
> +#define CPU_CLASS_NAME(name) (name "-" TYPE_X86_CPU)
> +
> +#define TYPE_X86_HOST_CPU CPU_CLASS_NAME("host")
> +
> +
>  /* maintains list of cpu model definitions
>   */
>  static x86_def_t *x86_defs = {NULL};
> @@ -1221,9 +1229,7 @@ static X86CPU *x86_cpu_create_from_name(const char *name, Error **errp)
>  
>      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
>  #ifdef CONFIG_KVM
> -        cpu = X86_CPU(object_new(TYPE_X86_CPU));
> -        kvm_cpu_fill_host(x86_cpu_def);
> -        cpudef_2_x86_cpu(cpu, x86_cpu_def, &error);
> +        cpu = X86_CPU(object_new(TYPE_X86_HOST_CPU));
>  #endif
>      } else {
>          x86_def_t *def;
> @@ -2168,9 +2174,42 @@ static const TypeInfo x86_cpu_type_info = {
>      .class_init = x86_cpu_common_class_init,
>  };
>  
> +#ifdef CONFIG_KVM
> +
> +static void x86_host_cpu_initfn(Object *obj)
> +{
> +    X86CPU *cpu = X86_CPU(obj);
> +    Error *err = NULL;
> +    x86_def_t cpudef;
> +
> +    memset(&cpudef, 0, sizeof(cpudef));
> +    kvm_cpu_fill_host(&cpudef);
> +    cpudef_2_x86_cpu(cpu, &cpudef, &err);
> +
> +    if (err) {
> +        error_report("unexpected cpu init error: %s", error_get_pretty(err));
> +        exit(1);
> +    }
> +}
> +
> +static const TypeInfo x86_host_cpu_type_info = {
> +    .name = TYPE_X86_HOST_CPU,
> +    .parent = TYPE_X86_CPU,
> +    .instance_size = sizeof(X86CPU),
> +    .instance_init = x86_host_cpu_initfn,
> +    .abstract = false,
> +    .class_size = sizeof(X86CPUClass),
> +};
> +
> +#endif /* CONFIG_KVM */
> +
> +
>  static void x86_cpu_register_types(void)
>  {
>      type_register_static(&x86_cpu_type_info);
> +#ifdef CONFIG_KVM
> +    type_register_static(&x86_host_cpu_type_info);
> +#endif
>  }
>  
>  type_init(x86_cpu_register_types)
> -- 
> 1.7.11.7
>
Igor Mammedov - Jan. 2, 2013, 8:07 p.m.
On Wed, 2 Jan 2013 20:00:26 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Fri, 28 Dec 2012 18:34:04 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Note that we are initializing the CPU features inside instance_init (and
> > not storing any CPU feature information inside the class struct) because
> > kvm_cpu_fill_host() needs KVM to be initialized, and we can't guarantee
> > that KVM will be initialized when class_init is called.
> initializing defaults in initfn will be broken after we convert features into
> static properties due to all initfn()s are called before static properties
> defaults are set.
Never mind, It was my wish-full thinking;
currently device_initfn() sets static property defaults, then sets global
properties and only then children initfn()s are called.

> 
> Is it possible to initialize kvm first before calling class_init().
> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c824c08..2b6cc3b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -330,6 +330,14 @@ typedef struct x86_def_t {
> >  #define TCG_SVM_FEATURES 0
> >  #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
> >  
> > +
> > +/* CPU class name definitions: */
> > +
> > +#define CPU_CLASS_NAME(name) (name "-" TYPE_X86_CPU)
> > +
> > +#define TYPE_X86_HOST_CPU CPU_CLASS_NAME("host")
> > +
> > +
> >  /* maintains list of cpu model definitions
> >   */
> >  static x86_def_t *x86_defs = {NULL};
> > @@ -1221,9 +1229,7 @@ static X86CPU *x86_cpu_create_from_name(const char *name, Error **errp)
> >  
> >      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> >  #ifdef CONFIG_KVM
> > -        cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > -        kvm_cpu_fill_host(x86_cpu_def);
> > -        cpudef_2_x86_cpu(cpu, x86_cpu_def, &error);
> > +        cpu = X86_CPU(object_new(TYPE_X86_HOST_CPU));
> >  #endif
> >      } else {
> >          x86_def_t *def;
> > @@ -2168,9 +2174,42 @@ static const TypeInfo x86_cpu_type_info = {
> >      .class_init = x86_cpu_common_class_init,
> >  };
> >  
> > +#ifdef CONFIG_KVM
> > +
> > +static void x86_host_cpu_initfn(Object *obj)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    Error *err = NULL;
> > +    x86_def_t cpudef;
> > +
> > +    memset(&cpudef, 0, sizeof(cpudef));
> > +    kvm_cpu_fill_host(&cpudef);
> > +    cpudef_2_x86_cpu(cpu, &cpudef, &err);
> > +
> > +    if (err) {
> > +        error_report("unexpected cpu init error: %s", error_get_pretty(err));
> > +        exit(1);
> > +    }
> > +}
> > +
> > +static const TypeInfo x86_host_cpu_type_info = {
> > +    .name = TYPE_X86_HOST_CPU,
> > +    .parent = TYPE_X86_CPU,
> > +    .instance_size = sizeof(X86CPU),
> > +    .instance_init = x86_host_cpu_initfn,
> > +    .abstract = false,
> > +    .class_size = sizeof(X86CPUClass),
> > +};
> > +
> > +#endif /* CONFIG_KVM */
> > +
> > +
> >  static void x86_cpu_register_types(void)
> >  {
> >      type_register_static(&x86_cpu_type_info);
> > +#ifdef CONFIG_KVM
> > +    type_register_static(&x86_host_cpu_type_info);
> > +#endif
> >  }
> >  
> >  type_init(x86_cpu_register_types)
> > -- 
> > 1.7.11.7
> > 
> 
> 
> -- 
> Regards,
>   Igor
>
Eduardo Habkost - Jan. 2, 2013, 8:16 p.m.
On Wed, Jan 02, 2013 at 08:00:26PM +0100, Igor Mammedov wrote:
> On Fri, 28 Dec 2012 18:34:04 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Note that we are initializing the CPU features inside instance_init (and
> > not storing any CPU feature information inside the class struct) because
> > kvm_cpu_fill_host() needs KVM to be initialized, and we can't guarantee
> > that KVM will be initialized when class_init is called.
> initializing defaults in initfn will be broken after we convert features into
> static properties due to all initfn()s are called before static properties
> defaults are set.

You are right because of the plan to use global properties. I didn't
care about property defaults because the -cpu host class wouldn't have
introspectable defaults, but global properties need to work properly so
"-cpu host,+foobar,enforce" keep working.


> 
> Is it possible to initialize kvm first before calling class_init().

Summarizing what was discussed on IRC:

 - class_init is too early, because it may be called before we even know
   what's the accel configuration requested by the user (and I believe
   this is by design, as we want to allow a client to list all available
   classes and their available properties before configuring anything).
   So we can't call kvm_arch_get_supported_cpuid() on class_init.
 - CPU instance_init is too late because global properties are set
   inside device_initfn(), before the CPU class instance_init function
   is called.

So we need something to be called after kvm_init() is called, but before
the CPU objects are created. We already have that: kvm_init() itself.  :-)

So I will try to send a new RFC that has a function like:

  void kvm_finish_cpu_host_class_init(KVMState *s, X86CPUClass *cc);

and kvm_init() would then call:

  kvm_finish_cpu_host_class_init(kvm_state, X86_CPU_CLASS(object_class_by_name(CPU_CLASS_NAME("host"))));

We considered other more complex approaches (like making the CPU feature
properties tristate [on/off/host] instead of boolean), but that would be
simply a further improvement, maybe to be proposed as part of the x86
CPU properties work.

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target-i386/cpu.c | 45 ++++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c824c08..2b6cc3b 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -330,6 +330,14 @@ typedef struct x86_def_t {
> >  #define TCG_SVM_FEATURES 0
> >  #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
> >  
> > +
> > +/* CPU class name definitions: */
> > +
> > +#define CPU_CLASS_NAME(name) (name "-" TYPE_X86_CPU)
> > +
> > +#define TYPE_X86_HOST_CPU CPU_CLASS_NAME("host")
> > +
> > +
> >  /* maintains list of cpu model definitions
> >   */
> >  static x86_def_t *x86_defs = {NULL};
> > @@ -1221,9 +1229,7 @@ static X86CPU *x86_cpu_create_from_name(const char *name, Error **errp)
> >  
> >      if (kvm_enabled() && name && strcmp(name, "host") == 0) {
> >  #ifdef CONFIG_KVM
> > -        cpu = X86_CPU(object_new(TYPE_X86_CPU));
> > -        kvm_cpu_fill_host(x86_cpu_def);
> > -        cpudef_2_x86_cpu(cpu, x86_cpu_def, &error);
> > +        cpu = X86_CPU(object_new(TYPE_X86_HOST_CPU));
> >  #endif
> >      } else {
> >          x86_def_t *def;
> > @@ -2168,9 +2174,42 @@ static const TypeInfo x86_cpu_type_info = {
> >      .class_init = x86_cpu_common_class_init,
> >  };
> >  
> > +#ifdef CONFIG_KVM
> > +
> > +static void x86_host_cpu_initfn(Object *obj)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    Error *err = NULL;
> > +    x86_def_t cpudef;
> > +
> > +    memset(&cpudef, 0, sizeof(cpudef));
> > +    kvm_cpu_fill_host(&cpudef);
> > +    cpudef_2_x86_cpu(cpu, &cpudef, &err);
> > +
> > +    if (err) {
> > +        error_report("unexpected cpu init error: %s", error_get_pretty(err));
> > +        exit(1);
> > +    }
> > +}
> > +
> > +static const TypeInfo x86_host_cpu_type_info = {
> > +    .name = TYPE_X86_HOST_CPU,
> > +    .parent = TYPE_X86_CPU,
> > +    .instance_size = sizeof(X86CPU),
> > +    .instance_init = x86_host_cpu_initfn,
> > +    .abstract = false,
> > +    .class_size = sizeof(X86CPUClass),
> > +};
> > +
> > +#endif /* CONFIG_KVM */
> > +
> > +
> >  static void x86_cpu_register_types(void)
> >  {
> >      type_register_static(&x86_cpu_type_info);
> > +#ifdef CONFIG_KVM
> > +    type_register_static(&x86_host_cpu_type_info);
> > +#endif
> >  }
> >  
> >  type_init(x86_cpu_register_types)
> > -- 
> > 1.7.11.7
> > 
> 
> 
> -- 
> Regards,
>   Igor
Eduardo Habkost - Jan. 2, 2013, 8:20 p.m.
On Wed, Jan 02, 2013 at 09:07:26PM +0100, Igor Mammedov wrote:
> On Wed, 2 Jan 2013 20:00:26 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > On Fri, 28 Dec 2012 18:34:04 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > Note that we are initializing the CPU features inside instance_init (and
> > > not storing any CPU feature information inside the class struct) because
> > > kvm_cpu_fill_host() needs KVM to be initialized, and we can't guarantee
> > > that KVM will be initialized when class_init is called.
> > initializing defaults in initfn will be broken after we convert features into
> > static properties due to all initfn()s are called before static properties
> > defaults are set.
> Never mind, It was my wish-full thinking;
> currently device_initfn() sets static property defaults, then sets global
> properties and only then children initfn()s are called.

So it won't break because of property defaults, but instead it will
break because we need global properties to work properly. It's even
worse because I didn't care about property defaults for -cpu host, but
we care about keeping global properties working (so
"-cpu host,+foobar,enforce" works).

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c824c08..2b6cc3b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -330,6 +330,14 @@  typedef struct x86_def_t {
 #define TCG_SVM_FEATURES 0
 #define TCG_7_0_EBX_FEATURES (CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_SMAP)
 
+
+/* CPU class name definitions: */
+
+#define CPU_CLASS_NAME(name) (name "-" TYPE_X86_CPU)
+
+#define TYPE_X86_HOST_CPU CPU_CLASS_NAME("host")
+
+
 /* maintains list of cpu model definitions
  */
 static x86_def_t *x86_defs = {NULL};
@@ -1221,9 +1229,7 @@  static X86CPU *x86_cpu_create_from_name(const char *name, Error **errp)
 
     if (kvm_enabled() && name && strcmp(name, "host") == 0) {
 #ifdef CONFIG_KVM
-        cpu = X86_CPU(object_new(TYPE_X86_CPU));
-        kvm_cpu_fill_host(x86_cpu_def);
-        cpudef_2_x86_cpu(cpu, x86_cpu_def, &error);
+        cpu = X86_CPU(object_new(TYPE_X86_HOST_CPU));
 #endif
     } else {
         x86_def_t *def;
@@ -2168,9 +2174,42 @@  static const TypeInfo x86_cpu_type_info = {
     .class_init = x86_cpu_common_class_init,
 };
 
+#ifdef CONFIG_KVM
+
+static void x86_host_cpu_initfn(Object *obj)
+{
+    X86CPU *cpu = X86_CPU(obj);
+    Error *err = NULL;
+    x86_def_t cpudef;
+
+    memset(&cpudef, 0, sizeof(cpudef));
+    kvm_cpu_fill_host(&cpudef);
+    cpudef_2_x86_cpu(cpu, &cpudef, &err);
+
+    if (err) {
+        error_report("unexpected cpu init error: %s", error_get_pretty(err));
+        exit(1);
+    }
+}
+
+static const TypeInfo x86_host_cpu_type_info = {
+    .name = TYPE_X86_HOST_CPU,
+    .parent = TYPE_X86_CPU,
+    .instance_size = sizeof(X86CPU),
+    .instance_init = x86_host_cpu_initfn,
+    .abstract = false,
+    .class_size = sizeof(X86CPUClass),
+};
+
+#endif /* CONFIG_KVM */
+
+
 static void x86_cpu_register_types(void)
 {
     type_register_static(&x86_cpu_type_info);
+#ifdef CONFIG_KVM
+    type_register_static(&x86_host_cpu_type_info);
+#endif
 }
 
 type_init(x86_cpu_register_types)