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

[Eduardo Habkost]

On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> On Fri, 08 Feb 2013 12:16:17 +0100
> Andreas Färber <address@hidden> wrote:
> 
> > Am 08.02.2013 10:03, schrieb 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.

"2 places" is already too much duplication to my taste. :-)

But I am not completely against the current version.


> > > 
> > >>
> > >>
> > >>
> > >>> +
> > >>> +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.
> > 
> > The main reason I did it this way was to avoid a g_malloc0() in a
> > non-failing class_init. Also it is closer to having class fields sit
> > directly in X86CPUClass.

No problem to me. I think I even prefer embedding instead of using a
pointer.


> > 
> > >>>  } 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?

Probably, yes.

(This would be an additional argument for embedding the struct instead
of using a pointer, as now we will augment the x86_def_t data inside
class_init).



> > > 
> > >>
> > >>> +    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.

Who guarantees that KVM will be already initialized when we get a QMP
monitor? We can't do that today because of limitations in the QEMU main
code, but I believe we want to get rid of this limitation eventually,
instead of making it harder to get rid of.

If we could initialize KVM before QMP is initialized, we could simply
initialize KVM before class_init is called, instead. It would be easier
to reason about, and it would make the limitations of our code very
clear to anybody reading the code in main().

> > 
> > I think you're missing Eduardo's and my point:
> > 
> > diff --git a/vl.c b/vl.c
> > index a8dc73d..6b9378e 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> >      }
> > 
> >      module_call_init(MODULE_INIT_QOM);
> > +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> > 
> >      qemu_add_opts(&qemu_drive_opts);
> >      qemu_add_opts(&qemu_chardev_opts);
> > 
> > Anyone may iterate over QOM classes at any time after their type
> > registration, which is before the first round of option parsing.
> > Sometime later, after option parsing, there's the -cpu ? handling in
> > vl.c:3854, then vl.c:4018:configure_accelerator().
> > 
> > Like I said, mostly a theoretical issue today.
> Question is if we should drop this theoretical issue for 1.5?

I wouldn't call it just theoretical. It is something that will surely
hit us back. The people working on QMP or on the main() code 6 months
from now will no idea that our class_init code is broken and will
explode if class_init is called too early.


> 
> > 
> > Originally I had considered making kvm_init() re-entrant and calling it
> > from the offending class_init. But we must support the distro case of
> > compiling with CONFIG_KVM but the user's hardware not supporting KVM or
> > kvm module not being loaded or the user having insufficient priviledges
> > to access /dev/kvm.
> working without KVM is not issue, it just works with normal defaults. Applying
> KVM specific defaults to already initialized classes is.

My big question is: why exactly we want to initialize this stuff inside
class_init? Can't we (please!) put the KVM-specific logic inside
instance_init?

If "default vendor set in in built-in CPU model table" (TCG-only) has a
different meaning from "vendor set by command-line/global-property"
(affects TCG and KVM), it means we have two different knobs with two
diferent semantics. Hence my suggestion of adding two properties:
"tcg-vendor" and "vendor".


>  
> > 
> > > 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();
> > >     }
> > >     ...
> > > }
> > 
> Continuing on theoretical issue:
> > We could add an inited field to X86CPUClass that gets checked at initfn
> > time (only ever getting set to true by the pre-defined models). Then it
> > would be per model. And if we add a prototype for the ..._class_init we
> > could even call it late as proposed for -cpu host if we take some
>              ^^^^^^^^^^^^ is a tricky part, for global properties to work it
> would require, calling this hook after kvm_init() is succeeds and before
> any initfn() is called in general or as minimum before Device.initfn(). And it
> anyway will not make all CPU classes to have correct defaults in KVM mode,
> since only CPU class of created CPU instance will be fixed up.
> 
> 1. One way to make sure that built-in CPU classes have fixed up defaults is to
> iterate over them in kvm_arch_init() and possibly calling their class_init()
> again to reinitialize. It's still hack (due fixing something up), but it would
> give at least correct KVM mode defaults, regardless of the order classes are
> initialized.

Can't we do that more easily with the tcg-vendor/vendor properties?

It looks we are burning too much brain cycles trying to force a model
that's really unintuitive to the outside, where the default-value of a
class property depends on the options given to the QEMU command-line. We
don't have to do that.

The point of initializing stuff in class_init is to make introspection
easy. If we make the classes change how they look like depending on the
command-line configuration, the classes and the class introspection
system get less useful.

> 
> 2. But more correct way from POV of OOP would be one without any fixups, i.e.
> create extra KVM-builtin-CPU-classes that are derived from host class.
> and in object_class_by_name() lookup for them if kvm is enabled. But we could
> do this as follow-up to #1.
>  
> > precautions and add explanatory comments.
> > 
> > >> 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?
> > 
> > I already answered Eduardo on IRC that in general I see no restriction
> > not to register a type late.
> > 
> > The issue is that in this case we cannot rely on accessing the class
> > from another class_init that is registered before it, which you were
> > doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
> > route).
> If we are accessing host class somewhere, then we would like to access its
> initialized state, not a dummy state which gives us nothing.

Absolutely.

(Just like the built-in classes, that should be always properly
initialized by class_init.  ;-)


> 
> sure, I'll rename hoc => host_oc.
> 
> > 
> > Regards,
> > Andreas
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> > 
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo