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

[Eduardo Habkost]

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?
* 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.


[...]
> @@ -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.

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

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.

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.

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

> [...]

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.

-- 
Eduardo