Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses

[Igor Mammedov]

On Thu, 14 Feb 2013 09:44:59 -0200
Eduardo Habkost <address@hidden> wrote:

> On Wed, Feb 13, 2013 at 04:33:33PM +0100, Igor Mammedov wrote:
> > From: Andreas Färber <address@hidden>
> > 
> > Depends on
> > http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg00677.html
> > 
> > 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().
> > 
> > Move default setting of CPUID_EXT_HYPERVISOR to class_init().
> > 
> > Move KVM specific built-in CPU defaults overrides in a kvm specific
> > x86_cpu_kvm_def_class_init(). And select TCG vs KVM class of CPU
> > to create at runtime in x86_cpu_class_by_name() when kvm_enable()
> > is available.
> > 
> > Inline cpu_x86_register() into the X86CPU initfn.
> > Since instance_init cannot reports errors, die there if some
> > of default values are incorrect, instead of ignoring errors.
> > 
> > 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-kvm-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs
> > and only when KVM is enabled to avoid workarounds in name to class
> > lookup code in x86_cpu_class_by_name().
> > 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-kvm-{i386,86_64}-cpu
> > for comparison.
> > 
> > Signed-off-by: Andreas Färber <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v5:
> >   * 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.
> >   * register KVM specific subclasses for built-in CPU models.
> >   * abort() in instance_init() if property setter fails to set default
> >     value.
> > 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.
> > ---
> >  target-i386/cpu-qom.h |   24 ++++
> >  target-i386/cpu.c     |  348
> > +++++++++++++++++++------------------------------ target-i386/cpu.h
> > |    5 +- target-i386/kvm.c     |   72 ++++++++++
> >  4 files changed, 232 insertions(+), 217 deletions(-)
> > 
> [...]
> > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> > index 48e6b54..c8f320d 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-kvm-" TYPE_X86_CPU
> > +
> [...]
> >      if (name == NULL) {
> > -        return -1;
> > -    }
> > -    if (kvm_enabled() && strcmp(name, "host") == 0) {
> > -        kvm_cpu_fill_host(x86_cpu_def);
> > -        return 0;
> > +        return NULL;
> >      }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> > -        def = &builtin_x86_defs[i];
> > -        if (strcmp(name, def->name) == 0) {
> > -            memcpy(x86_cpu_def, def, sizeof(*def));
> > -            /* 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
> > -             */
> > -            if (kvm_enabled()) {
> > -                uint32_t  ebx = 0, ecx = 0, edx = 0;
> > -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> > -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx,
> > ecx);
> > -            }
> > -            return 0;
> > -        }
> > +    if (kvm_enabled()) {
> > +        typename = g_strdup_printf("%s-kvm-" TYPE_X86_CPU, name);
> 
> * Could we use the "-tcg" suffix for the non-KVM mode?
sure, I'll add it for the next re-spin.

> * Can we make the code fall back to no-suffix CPU names? We need to add
>   the -kvm suffix (and maybe a -tcg suffix) to keep compatibility with
>   existing CPU models, but maybe we should deprecate the automatic
>   -tcg/-kvm suffixing and ask users to specify the full CPU name.
I'm not sure that I got you right, Have you meant something like this:

if (kvm) {
    typename = name-kvm-...
} else {
    typename = name-tcg-...
}

oc = object_class_by_name(typename)
if (!oc) {
    oc = object_class_by_name(name)
}

> 
> 
> [...]
> > @@ -2234,7 +2075,57 @@ static void x86_cpu_initfn(Object *obj)
> >          cpu_set_debug_excp_handler(breakpoint_handler);
> >  #endif
> >      }
> > +
> > +    if (error) {
> > +        fprintf(stderr, "%s\n", error_get_pretty(error));
> > +        error_free(error);
> > +        abort();
> > +    }
> 
> Good idea, we should have done that a long time ago, to avoid surprises
> if one day the property setting fails.
> 
> > +
> > +}
> > +
> > +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> > +{
> > +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > +    x86_def_t *def = data;
> > +    int i;
> > +    static const char *versioned_models[] = { "qemu32", "qemu64",
> > "athlon" }; +
> > +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > +    xcc->info.ext_features |= CPUID_EXT_HYPERVISOR;
> 
> If this is TCG-specific now, we could make the class match the reality
> and mask TCG_FEATURES/TCG_EXT_FEATURES/etc here. That's what happens in
> practice, e.g.: "-cpu SandyBridge" in TCG mode is a completely different
> CPU, today.
well, this function is shared between TCG and KVM, I mean, it's common
code for both. Which asks for one more TCG class_init function for TCG
specific behavior.
But could it be a separate patch (i.e. fixing TCG filtering), I think
just moving tcg filtering might cause regression. And need to worked on
separately.

> > +
> > +    /* 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;
> > +        }
> > +    }
> > +}
> > +
> > +#ifdef CONFIG_KVM
> > +static void x86_cpu_kvm_def_class_init(ObjectClass *oc, void *data)
> > +{
> > +    uint32_t eax, ebx, ecx, edx;
> > +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > +
> > +    x86_cpu_def_class_init(oc, data);
> > +    /* 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
> > +     */
> > +    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> 
> Cool, this is exactly what I was going to suggest, to avoid depending on
> the "host" CPU class and KVM initialization.
> 
> I see two options when making the "vendor" property static, and I don't
> know which one is preferable:
> 
> One solution is the one in this patch: to call host_cpuid() here in
> class_init, and have a different property default depending on the host
> CPU.
I prefer this one ^^^.

> Another solution is to have default to vendor="host", and have
> instance_init understand vendor="host" as "use the host CPU vendor".
> This would make the property default really static (not depending on the
> host CPU).
> 


> I am inclined for the latter, because I am assuming that the QOM design
> expects class_init results to be completely static. This is not as bad
> as depending on KVM initialization, but it may be an unexpected
> surprise, or even something not allowed by QOM.
That's where I have to disagree :)

You might have a point with 'static' if our goal is for introspection for
source level to work. But we have a number of issues with that:

* model_id is not static for some cpus, 'vendor' is the same just for
  different set of classes
* we generate sub-classes at runtime dynamically, which makes source level
  introspection impossible for them.

I think that QOM is aiming towards run-time introspection of
classes/objects. And that allows us to define defaults (even generate
them dynamically) at class_init() time, they don't have to be static
constants.
But main point of putting defaults in class_init is because they are class
wide, whether instance_init is for instance specific settings.

Anthony probably could judge us and suggest which way to move.

And I intend to use this feature with static properties defaults after
static properties + subclasses are in tree:

"Dynamically create (copy) static properties definitions for CPU subclass"
https://github.com/imammedo/qemu/commit/ed506d354688ea00212cf5f76caf08932c20d0aa

"set per subclass defaults of static properties in class_init()"
https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4f9517d0a25e5c

Whole tree to play with is here:
https://github.com/imammedo/qemu/commits/x86-cpu-hot-add.WIP

> This doesn't matter today, because there's no "vendor" property default
> being set here, but it may be an issue when we introduce the static
> properties.
Could you describe what problems it creates after we have static properties?

> 
> > +    x86_cpu_vendor_words2str(xcc->info.vendor, ebx, edx, ecx);
> > +
> > +    xcc->info.kvm_features |= kvm_default_features;
> >  }
> > +#endif
> >  
> >  static void x86_cpu_common_class_init(ObjectClass *oc, void *data)
> >  {
> > @@ -2247,6 +2138,28 @@ static void x86_cpu_common_class_init(ObjectClass
> > *oc, void *data) 
> >      xcc->parent_reset = cc->reset;
> >      cc->reset = x86_cpu_reset;
> > +
> > +    cc->class_by_name = x86_cpu_class_by_name;
> > +}
> > +
> > +static void x86_register_cpu_type(const x86_def_t *def)
> > +{
> > +    TypeInfo type_info = {
> > +        .parent = TYPE_X86_CPU,
> > +        .class_init = x86_cpu_def_class_init,
> > +        .class_data = (void *)def,
> > +    };
> > +
> > +    type_info.name = g_strdup_printf("%s-" TYPE_X86_CPU, def->name);
> > +    type_register(&type_info);
> > +    g_free((void *)type_info.name);
> > +
> > +#ifdef CONFIG_KVM
> > +    type_info.name = g_strdup_printf("%s-kvm-" TYPE_X86_CPU, def->name);
> > +    type_info.class_init = x86_cpu_kvm_def_class_init;
> > +    type_register(&type_info);
> > +    g_free((void *)type_info.name);
> > +#endif
> 
> Like I said above, I would prefer to have both "-tcg" and "-kvm"
> suffixes.
sure, np.

> 
> > [...]
> 
> Overall, I really like how simple this solution ended up. I had issues
> with "class hierarchy explosion", but as I said before: in practice we
> already have different CPU models in TCG and KVM mode, because of the
> silent feature-masking done in TCG mode. So it just makes sense to have
> those different CPU models represented by different classes.
> 

Thanks for reviewing!