Re: [Qemu-devel] [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses

[Igor Mammedov]

On Thu, 7 Feb 2013 13:08:19 -0200
Eduardo Habkost <address@hidden> wrote:

> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> > From: Andreas Färber <address@hidden>
> > 
> > Move x86_def_t definition to header and embed into X86CPUClass.
> > Register types per built-in model definition.
> > 
> > Move version initialization from x86_cpudef_setup() to class_init.
> > 
> > Inline cpu_x86_register() into the X86CPU initfn.
> > Since instance_init cannot reports errors, drop error handling.
> > 
> > Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
> > Move handling of KVM host vendor override from cpu_x86_find_by_name()
> > to the kvm_arch_init() and class_init(). Use TYPE_X86_CPU class to
> > communicate kvm specific defaults to other sub-classes.
> > 
> > Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs
> > and only when KVM is enabled to avoid hacks in CPU code.
> > Make kvm_cpu_fill_host() into a host specific class_init and inline
> > cpu_x86_fill_model_id().
> > 
> > Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
> > comparison.
> > 
> > Signed-off-by: Andreas Färber <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v4:
> >   * set error if cpu model is not found and goto out;
> >   * copy vendor override from 'host' CPU class in sub-class'es
> >     class_init() if 'host' CPU class is available.
> >   * register type TYPE_HOST_X86_CPU in kvm_arch_init(), this type
> >     should be available only in KVM mode and we haven't printed it in
> >     -cpu ? output so far, so we can continue doing so. It's not
> >     really confusing to show 'host' cpu (even if we do it) when KVM
> >     is not enabled.
> >   * remove special case for 'host' CPU check in x86_cpu_class_by_name(),
> >     due to 'host' CPU will not find anything if not in KVM mode or
> >     return 'host' CPU class in KVM mode, i.e. treat it as regular CPUs.
> > ---
> >  target-i386/cpu-qom.h |   24 ++++
> >  target-i386/cpu.c     |  331 
> > ++++++++++++++++++-------------------------------
> >  target-i386/cpu.h     |    5 +-
> >  target-i386/kvm.c     |   72 +++++++++++
> >  4 files changed, 217 insertions(+), 215 deletions(-)
> > 
> > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> > index 48e6b54..80bf72d 100644
> > --- a/target-i386/cpu-qom.h
> > +++ b/target-i386/cpu-qom.h
> > @@ -30,6 +30,27 @@
> >  #define TYPE_X86_CPU "i386-cpu"
> >  #endif
> >  
> > +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
> 
> Can we introduce a X86_CPU_CLASS_NAME() macro to help us make the rules
> to generate CPU class names clearer?
> 
> e.g.:
> 
> #define X86_CPU_CLASS_NAME(s) s "-" TYPE_X86_CPU
> [...]
> #define TYPE_HOST_X86_CPU X86_CPU_CLASS_NAME("host")
> [...]
>   /* (at the class lookup code) */
>   typename = g_strdup_printf(X86_CPU_CLASS_NAME("%s"), name);
I kind of like Andreas' variant not hiding format string in macro, for
one doesn't have look-up what macro does to see how name will look.
Especially since it's called in only 2 places.

> 
> 
> 
> > +
> > +typedef struct x86_def_t {
> > +    const char *name;
> > +    uint32_t level;
> > +    /* vendor is zero-terminated, 12 character ASCII string */
> > +    char vendor[CPUID_VENDOR_SZ + 1];
> > +    int family;
> > +    int model;
> > +    int stepping;
> > +    uint32_t features, ext_features, ext2_features, ext3_features;
> > +    uint32_t kvm_features, svm_features;
> > +    uint32_t xlevel;
> > +    char model_id[48];
> > +    /* Store the results of Centaur's CPUID instructions */
> > +    uint32_t ext4_features;
> > +    uint32_t xlevel2;
> > +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> > +    uint32_t cpuid_7_0_ebx_features;
> > +} x86_def_t;
> > +
> >  #define X86_CPU_CLASS(klass) \
> >      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
> >  #define X86_CPU(obj) \
> > @@ -41,6 +62,7 @@
> >   * X86CPUClass:
> >   * @parent_realize: The parent class' realize handler.
> >   * @parent_reset: The parent class' reset handler.
> > + * @info: Model-specific data.
> >   *
> >   * An x86 CPU model or family.
> >   */
> > @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
> >  
> >      DeviceRealize parent_realize;
> >      void (*parent_reset)(CPUState *cpu);
> > +
> > +    x86_def_t info;
> 
> I thought you had suggesting making it a pointer. If you made it a
> pointer, you wouldn't need to move the x86_def_t declaration to
> cpu-qom.h.

x86_def_t is needed in kvm.c for host class. So there is no much point
in changing info into pointer, considering it's temporary solution.


> 
> >  } X86CPUClass;
> >  
> >  /**
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index 1aee097..62fdc84 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -47,8 +47,8 @@
[...]

> > @@ -2195,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
> >      CPUState *cs = CPU(obj);
> >      X86CPU *cpu = X86_CPU(obj);
> >      CPUX86State *env = &cpu->env;
> > +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> > +    const x86_def_t *def = &xcc->info;
> >      static int inited;
> >  
> >      cpu_exec_init(env);
> > @@ -2224,6 +2049,28 @@ static void x86_cpu_initfn(Object *obj)
> >                          x86_cpuid_get_tsc_freq,
> >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> >  
> > +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
> > +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
> > +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
> > +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
> > +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
> > +    env->cpuid_features = def->features;
> > +    env->cpuid_ext_features = def->ext_features;
> > +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
> > +    env->cpuid_ext2_features = def->ext2_features;
> > +    env->cpuid_ext3_features = def->ext3_features;
> > +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
> > +    env->cpuid_kvm_features = def->kvm_features;
> > +    if (kvm_enabled()) {
> > +        env->cpuid_kvm_features |= kvm_default_features;
> > +    }
> 
> "-cpu host,enforce" is supposed to never fail. What if the host doesn't
> support some of the features present in kvm_default_features? We need to
> use kvm_default_features only if the CPU model is not "host".
> 
> But this is an existing bug in the code, you are not introducing it with
> this patch.
> 
whould be moving it in x86_cpu_def_class_init() suitable solution?

> 
> > +    env->cpuid_svm_features = def->svm_features;
> > +    env->cpuid_ext4_features = def->ext4_features;
> > +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> > +    env->cpuid_xlevel2 = def->xlevel2;
> > +
> > +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
> > +
> >      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> >  
> >      /* init various static tables used in TCG mode */
> > @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> >      }
> >  }
> >  
> > +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> > +{
> > +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
> > +    X86CPUClass *hostcc;
> > +    x86_def_t *def = data;
> > +    int i;
> > +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" 
> > };
> > +
> > +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > +
> > +    /* host cpu class is available if KVM is enabled,
> > +     * get kvm overrides from it */
> > +    if (hoc) {
> > +        hostcc = X86_CPU_CLASS(hoc);
> > +        /* sysenter isn't supported in compatibility mode on AMD,
> > +         * syscall isn't supported in compatibility mode on Intel.
> > +         * Normally we advertise the actual CPU vendor, but you can
> > +         * override this using the 'vendor' property if you want to use
> > +         * KVM's sysenter/syscall emulation in compatibility mode and
> > +         * when doing cross vendor migration
> > +         */
> > +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> > +               sizeof(xcc->info.vendor));
> > +    }
> 
> Again, we have the same problem we had before, but now in the non-host
> classes. What if class_init is called before KVM is initialized? I
> believe we will be forced to move this hack to the instance init
> function.
I believe, the in the case where non-host CPU classes might be initialized
before KVM "-cpu ?" we do not care what their defaults are, since we only
would use class names there and then exit.

For case where classes could be inspected over QMP, OQM, KVM would be already
initialized if enabled and we would get proper initialization order without
hack.

Instead of adding hack, I'd rather enforce valid initialization order and
abort in initfn() if type was initialized without KVM present and KVM is
enabled at initfn() time. Something along the lines:

static x86_cpu_builtin_class_initialized_without_kvm;

x86_cpu_def_class_init() {
    ...
    if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kvm) {
        x86_cpu_builtin_class_initialized_without_kvm = true;
    }
    ...
}

initfn() {
    ...
    if (kvm_enabled() && x86_cpu_builtin_class_initialized_without_kvm) {
        abort();
    }
    ...
}

> If we still want the default vendor to be a static property in the
> class, we can do that if we set the default in a "tcg-vendor" property
> instead of a "vendor" property (that would be empty/unset by default),
> and x86_cpu_initfn() could do this:
> 
>     vendor = object_property_get_str(cpu, "vendor");
>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
>     if (vendor && vendor[0]) {
>       cpu->cpuid_vendor = vendor;
>     } else if (kvm_enabled()) {
>       cpu->cpuid_vendor = get_host_vendor();
>     } else {
>       cpu->cpuid_vendor = tcg_vendor;
>     }
> 
> > +
> > +    /* Look for specific models that have the QEMU version in .model_id */
> > +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
> > +        if (strcmp(versioned_models[i], def->name) == 0) {
> > +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
> > +                    "QEMU Virtual CPU version ");
> > +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
> > +                    qemu_get_version());
> > +            break;
> > +        }
> > +    }
> > +}
> > +
[...]

> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
> >      return ret;
> >  }
> >  
[...]

> >  int kvm_arch_init(KVMState *s)
> >  {
> >      QemuOptsList *list = qemu_find_opts("machine");
> > @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
> >      int ret;
> >      struct utsname utsname;
> >  
> > +    static const TypeInfo host_x86_cpu_type_info = {
> > +        .name = TYPE_HOST_X86_CPU,
> > +        .parent = TYPE_X86_CPU,
> > +        .class_init = kvm_host_cpu_class_init,
> > +    };
> > +
> >      ret = kvm_get_supported_msrs(s);
> >      if (ret < 0) {
> >          return ret;
> > @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
> >              }
> >          }
> >      }
> > +
> > +    type_register(&host_x86_cpu_type_info);
> 
> Are we really allowed to register QOM classes that late?
> 
> If QOM design allows us to register the class very late (I would like to
> confirm that), this approach sounds really clean and sane to me.
> Pre-KVM-init class introspection of the "host" class would be completely
> useless anyway (because all data in the "host" class depend on data
> available only post-KVM-init anyway).

Looking at type_register() impl. it seems safe to do so + relying on QBL for
type_table_add() safety. So it's really design question for QOM experts.

Antnony, Paolo, Andreas
 what do you think?

> 
> > +
> >      return 0;
> >  }
> >  
> > -- 
> > 1.7.1
> > 
> 
> -- 
> Eduardo


-- 
Regards,
  Igor