| Message ID | 1356726846-10637-8-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
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).
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)
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(-)