Re: [Qemu-devel] [RFC 7/9] target-i386: CPU subclass for -cpu "host"

[Eduardo Habkost]

On Wed, Jan 02, 2013 at 08:00:26PM +0100, Igor Mammedov wrote:
> On Fri, 28 Dec 2012 18:34:04 -0200
> Eduardo Habkost <address@hidden> 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 <address@hidden>
> > ---
> >  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