[RFC,v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses

From: Andreas Färber <afaerber@suse.de>

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 <afaerber@suse.de>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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(-)

On Wed, Feb 13, 2013 at 04:33:33PM +0100, Igor Mammedov wrote:
> From: Andreas Färber <afaerber@suse.de>
> 
> 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 <afaerber@suse.de>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> 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.

On Thu, 14 Feb 2013 09:44:59 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Feb 13, 2013 at 04:33:33PM +0100, Igor Mammedov wrote:
> > From: Andreas Färber <afaerber@suse.de>
> > 
> > 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 <afaerber@suse.de>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > 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!

On Thu, Feb 14, 2013 at 04:13:22PM +0100, Igor Mammedov wrote:
> On Thu, 14 Feb 2013 09:44:59 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
[...]
> > > +    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)
> }

Yes, exactly.


[...]
> > > +
> > > +}
> > > +
> > > +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.

Having both TCG-specific and KVM-specific subclasses instead of making
the KVM class inherit from the TCG class would make sense to me, as we
may have TCG-specific behavior in other places too.

Or we could do memcpy() again on the KVM class_init function. Or we
could set a different void* pointer on the TCG class, with
already-filtered x86_def_t object. There are multiple possible
solutions.


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

I'm OK with doing that in a separate patch, as it is not a bug fix or
behavior change. But it would be nice to do that before we introduce the
feature properties, to make the reported defaults right since the
beginning.

[...]
> > > +#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

model_id is static for a given QEMU version, isn't it? This may be OK
(but I am _also_ not sure about this case).


> * we generate sub-classes at runtime dynamically, which makes source level
>   introspection impossible for them.

It's not source-level introspection I am looking for, but runtime
introspection that won't surprise other components in the system by
changing unexpectedly (e.g. when the host CPU changes).

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

Yes, QOM introspection is run-time based. What I don't know is: what can
be the expectations about the stability of the class data that was just
introspected? How much this information can change when you run QEMU
again? How much can it change if you run the same QEMU binary on a
different host CPU? How much can it change when you run the same QEMU
binary using a different host kernel? How much can it change between
QEMU versions?

(I don't know the answer to any of those questions).


> But main point of putting defaults in class_init is because they are class
> wide, whether instance_init is for instance specific settings.

We're back to the difference between "property defaults" vs "how cpuid_*
will look like after instance_init is called". Property defaults are
"static data" (considering some definition of "static", at least) and
will always be in class_init, but there's no requirement that the value
of every single bit inside struct X86CPU has be initialized inside
class_init (otherwise instance_init wouldn't even exist!).

In other words "the value of X86CPU.cpuid_vendor after instance_init
runs" (that may depend on the host CPU) may be different from "the
default value of the 'vendor' property" (that could be statically set to
"host" if we choose that solution). The latter _must_ be defined by
class_init. The former is simply an internal field calculated by
instance_init, like lots of other X86CPU fields.

To give another example:

* In the future, we may have properties for reading CPU registers values;
* The EIP register is set to 0xfff0 on CPU reset;
* The CPU will probably be reset on instance_init;
* But that doesn't mean we have to set a 0xfff0 default for the
  "register-EIP" property on class_init.

I agree it is nice when we have properties that more closely match
what's the internal state of the object after instance_init. But _if_ we
have restrictions on how the defaults can change between runs of the
same QEMU binary (which I don't know yet), we may have to hide some
specifics behind an abstraction (like having vendor="host" meaning "use
the host CPU vendor").

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

Thanks! I will take a look later.

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

The problem is that the default value of the "vendor" property will
change when you run the same QEMU binary on a different CPU. I don't
know if we can do that, or not (see my question about "expections about
the stability of data" above).


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

On Thu, 14 Feb 2013 13:52:59 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 14, 2013 at 04:13:22PM +0100, Igor Mammedov wrote:
> > On Thu, 14 Feb 2013 09:44:59 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> [...]
> > > > +    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)
> > }
> 
> Yes, exactly.
> 
> 
> [...]
> > > > +
> > > > +}
> > > > +
> > > > +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.
> 
> Having both TCG-specific and KVM-specific subclasses instead of making
> the KVM class inherit from the TCG class would make sense to me, as we
> may have TCG-specific behavior in other places too.
> 
> Or we could do memcpy() again on the KVM class_init function. Or we
> could set a different void* pointer on the TCG class, with
> already-filtered x86_def_t object. There are multiple possible
> solutions.
> 
> 
> > 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.
> 
> I'm OK with doing that in a separate patch, as it is not a bug fix or
> behavior change. But it would be nice to do that before we introduce the
> feature properties, to make the reported defaults right since the
> beginning.
It's behavior change, if we move filtering from realizefn to class_init, user
would be able to add features not visible now to guest.

ordering now:
class_init, initfn, setting defautls, custom features, realizefn(filtering)

would be ordering with static properties:
clas_init, static props defaults, global properties(custom features),
x86_cpu_initfn, realizefn

in would be scenario filtering comes before custom features, which is not
necessarily bad and may be nice to consciously add/test features before
enabling them in filter for everyone, but it's behavior change.

> [...]
> > > > +#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
> 
> model_id is static for a given QEMU version, isn't it? This may be OK
> (but I am _also_ not sure about this case).
I guess you mean constant when using static. 

> 
> 
> > * we generate sub-classes at runtime dynamically, which makes source level
> >   introspection impossible for them.
> 
> It's not source-level introspection I am looking for, but runtime
> introspection that won't surprise other components in the system by
> changing unexpectedly (e.g. when the host CPU changes).
And I do not think that it must be constant until class is registered.
Actually as a user during class introspection, I'd like to see vendor
value that will be used, not a placeholder 'host', so I won't have to
guess/get from somewhere else what that 'host' actually means.

> 
> > 
> > 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.
> 
> Yes, QOM introspection is run-time based. What I don't know is: what can
> be the expectations about the stability of the class data that was just
> introspected? How much this information can change when you run QEMU
> again? How much can it change if you run the same QEMU binary on a
> different host CPU? How much can it change when you run the same QEMU
> binary using a different host kernel? How much can it change between
> QEMU versions?
> 
> (I don't know the answer to any of those questions).
I don't know answers to these Q either, it probably will be individual on per
case basis. But in general due to introspection being runtime why should we 
corner us to only constants, I'd expect that data might change for above cases
between different qemu runs, it should reflect environment we are running on.
for 'cpuid.vendor' it is current behavior, and I don't see the reason why we
should change it.

> 
> 
> > But main point of putting defaults in class_init is because they are class
> > wide, whether instance_init is for instance specific settings.
> 
> We're back to the difference between "property defaults" vs "how cpuid_*
> will look like after instance_init is called". Property defaults are
> "static data" (considering some definition of "static", at least) and
> will always be in class_init, but there's no requirement that the value
> of every single bit inside struct X86CPU has be initialized inside
> class_init (otherwise instance_init wouldn't even exist!).
> 
> In other words "the value of X86CPU.cpuid_vendor after instance_init
> runs" (that may depend on the host CPU) may be different from "the
> default value of the 'vendor' property" (that could be statically set to
> "host" if we choose that solution). The latter _must_ be defined by
> class_init. The former is simply an internal field calculated by
> instance_init, like lots of other X86CPU fields.
No, I'm not. I'm not talking about every field in X86CPU. CPUID is
architectural constant/default depending on CPU model. In case of KVM
CPUID.vendor is just another constant values from POV of consumer. And I
propose to treat it the same way as the other defaults (i.e. set it in
class_init). (i.e. no special casing unless we have to)
Yes it can change if you run qemu on different hosts, but it won't do so if
you run it on the same host.

> 
> To give another example:
> 
> * In the future, we may have properties for reading CPU registers values;
> * The EIP register is set to 0xfff0 on CPU reset;
> * The CPU will probably be reset on instance_init;
> * But that doesn't mean we have to set a 0xfff0 default for the
>   "register-EIP" property on class_init.
It's not valid comparison, register is 'VARIABLE' when CPUID is 'CONSANT'.
PS: 'constant' here means constant after class is registered.

> I agree it is nice when we have properties that more closely match
> what's the internal state of the object after instance_init. But _if_ we
> have restrictions on how the defaults can change between runs of the
> same QEMU binary (which I don't know yet), we may have to hide some
> specifics behind an abstraction (like having vendor="host" meaning "use
> the host CPU vendor").
Well, I'm no aware about such restrictions. But I'm planing re-factoring with
keeping current behavior, unless there is reasons to change it.
Presently cpu_x86_find_by_name() that overrides 'vendor' is called before
object_new(), and it's sort of class_init(), so I'm trying fit it to new model
and do it uniformly for all cpuid properties. I don't see why 'vendor' is any
different from other properties, when you get x86_cpu_def from
cpu_x86_find_by_name() it already has correct vendor values.
From my pov vendor="host" introduces only problems and is not obvious to users
and isn't easy to use.
 
> 
> > 
> > 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
> 
> Thanks! I will take a look later.
> 
> > 
> > > 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?
> 
> The problem is that the default value of the "vendor" property will
> change when you run the same QEMU binary on a different CPU. I don't
> know if we can do that, or not (see my question about "expections about
> the stability of data" above).
> 
> 
> > 
> > > 
> > > > +    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!
> 
> -- 
> Eduardo
>

On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
> > [...]
> > > > > +
> > > > > +}
> > > > > +
> > > > > +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.
> > 
> > Having both TCG-specific and KVM-specific subclasses instead of making
> > the KVM class inherit from the TCG class would make sense to me, as we
> > may have TCG-specific behavior in other places too.
> > 
> > Or we could do memcpy() again on the KVM class_init function. Or we
> > could set a different void* pointer on the TCG class, with
> > already-filtered x86_def_t object. There are multiple possible
> > solutions.
> > 
> > 
> > > 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.
> > 
> > I'm OK with doing that in a separate patch, as it is not a bug fix or
> > behavior change. But it would be nice to do that before we introduce the
> > feature properties, to make the reported defaults right since the
> > beginning.
> It's behavior change, if we move filtering from realizefn to class_init, user
> would be able to add features not visible now to guest.
> 
> ordering now:
> class_init, initfn, setting defautls, custom features, realizefn(filtering)
> 
> would be ordering with static properties:
> clas_init, static props defaults, global properties(custom features),
> x86_cpu_initfn, realizefn
> 
> in would be scenario filtering comes before custom features, which is not
> necessarily bad and may be nice to consciously add/test features before
> enabling them in filter for everyone, but it's behavior change.
> 

We should keep the filtering on realize because of custom -cpu strings,
but if we filter on class_init too, we will make the class introspection
reflect reality more closely. Wasn't this one the main reasons you
argued (and convinced me) for having separate subclasses?

Also, if we do this we will be able to make "enforce" work for TCG too.
For example: if "enforce" worked for TCG today, people who want to work
around the existing n270 MOVBE bug could use "-cpu n270,+movbe,enforce"
to make sure the QEMU version they are using really accepts movbe.


> > [...]
> > > > > +#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
> > 
> > model_id is static for a given QEMU version, isn't it? This may be OK
> > (but I am _also_ not sure about this case).
> I guess you mean constant when using static. 

Yes, I mean "constant" if by "constant" you mean "you can have
reasonable expectations that it won't change under certain conditions".
If the data you got from class introspection could change at any moment
without any warning, it would be random, useless information.

What I don't know is: what are reasonable conditions where the class
data may change, and what are the reasonable conditions where it should
_not_ change?


> 
> > 
> > 
> > > * we generate sub-classes at runtime dynamically, which makes source level
> > >   introspection impossible for them.
> > 
> > It's not source-level introspection I am looking for, but runtime
> > introspection that won't surprise other components in the system by
> > changing unexpectedly (e.g. when the host CPU changes).
> And I do not think that it must be constant until class is registered.
> Actually as a user during class introspection, I'd like to see vendor
> value that will be used, not a placeholder 'host', so I won't have to
> guess/get from somewhere else what that 'host' actually means.

What if you run QEMU once for introspection/probing and then run it once
again? I believe the other components can expect the values won't change
just if you re-run the same QEMU binary.

This is not just about keeping it constant during a single QEMU proces
lifetime. Under certain conditions, the class data has to keep constant
between QEMU runs, too.


> 
> > 
> > > 
> > > 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.
> > 
> > Yes, QOM introspection is run-time based. What I don't know is: what can
> > be the expectations about the stability of the class data that was just
> > introspected? How much this information can change when you run QEMU
> > again? How much can it change if you run the same QEMU binary on a
> > different host CPU? How much can it change when you run the same QEMU
> > binary using a different host kernel? How much can it change between
> > QEMU versions?
> > 
> > (I don't know the answer to any of those questions).
> I don't know answers to these Q either, it probably will be individual on per
> case basis. But in general due to introspection being runtime why should we 
> corner us to only constants, I'd expect that data might change for above cases
> between different qemu runs, it should reflect environment we are running on.
> for 'cpuid.vendor' it is current behavior, and I don't see the reason why we
> should change it.

The data may change, yes, but I want to know when it is reasonable to
change this data.

I am sure of one thing: if somebody is running the same QEMU vesion on
the same system (same QEMU binary, same CPU, same kernel, same
hardware), the data absolutely shouldn't change between QEMU runs, or
the data would be useless for probing QEMU capabilities.

And I also think (but I am not sure) we _can_ change the defaults when
updating to a new QEMU binary. What I don't know is about the
intermediate cases (changing host CPU, changing host kernel, etc).


> 
> > 
> > 
> > > But main point of putting defaults in class_init is because they are class
> > > wide, whether instance_init is for instance specific settings.
> > 
> > We're back to the difference between "property defaults" vs "how cpuid_*
> > will look like after instance_init is called". Property defaults are
> > "static data" (considering some definition of "static", at least) and
> > will always be in class_init, but there's no requirement that the value
> > of every single bit inside struct X86CPU has be initialized inside
> > class_init (otherwise instance_init wouldn't even exist!).
> > 
> > In other words "the value of X86CPU.cpuid_vendor after instance_init
> > runs" (that may depend on the host CPU) may be different from "the
> > default value of the 'vendor' property" (that could be statically set to
> > "host" if we choose that solution). The latter _must_ be defined by
> > class_init. The former is simply an internal field calculated by
> > instance_init, like lots of other X86CPU fields.
> No, I'm not. I'm not talking about every field in X86CPU. CPUID is
> architectural constant/default depending on CPU model. In case of KVM
> CPUID.vendor is just another constant values from POV of consumer. And I
> propose to treat it the same way as the other defaults (i.e. set it in
> class_init). (i.e. no special casing unless we have to)

It's great when we can keep properties constant for a long time, after
the class is registered and even during instance_init. The problem is
that to reach this goal you are making class introspection report "less
constant" data (making it less useful and less reliable).


> Yes it can change if you run qemu on different hosts, but it won't do so if
> you run it on the same host.

This is what I would like to avoid.


> 
> > 
> > To give another example:
> > 
> > * In the future, we may have properties for reading CPU registers values;
> > * The EIP register is set to 0xfff0 on CPU reset;
> > * The CPU will probably be reset on instance_init;
> > * But that doesn't mean we have to set a 0xfff0 default for the
> >   "register-EIP" property on class_init.
> It's not valid comparison, register is 'VARIABLE' when CPUID is 'CONSANT'.
> PS: 'constant' here means constant after class is registered.

Well, it is "constant after class is registered" just because you want
to make it so. What I am proposing is to make vendor "variable" during
the instance_init() call. _If_ we can't change "vendor" in class_init
depending on the host CPU, we may _have to_ change it on instance_init,
we would have no other choice.

The "vendor" property is already variable between class registration and
the start of instance_init(), because it can be set using "-cpu
...,.vendor=...". I am just asking to allow it to change during a few
additional nanoseconds, while instance_init() runs. There are lots of
fields that can change during instance_init and even _after_
instance_init run. I really don't see what's the big deal in changing
the value of cpuid_vendor inside instance_init.

Objects are supposed to be dynamic, after all. You are trying to make
the object "more static" during instance_init (for a reason I don't
understand) by making the class (that is really supposed to be "more
static") change depending on the host CPU.


> 
> > I agree it is nice when we have properties that more closely match
> > what's the internal state of the object after instance_init. But _if_ we
> > have restrictions on how the defaults can change between runs of the
> > same QEMU binary (which I don't know yet), we may have to hide some
> > specifics behind an abstraction (like having vendor="host" meaning "use
> > the host CPU vendor").
> Well, I'm no aware about such restrictions. But I'm planing re-factoring with
> keeping current behavior, unless there is reasons to change it.
> Presently cpu_x86_find_by_name() that overrides 'vendor' is called before
> object_new(), and it's sort of class_init(), so I'm trying fit it to new model
> and do it uniformly for all cpuid properties. I don't see why 'vendor' is any
> different from other properties, when you get x86_cpu_def from
> cpu_x86_find_by_name() it already has correct vendor values.

The current code would OK by now because the "vendor" property is not
static yet. But we will have a problem once we start registering static
properties, because static properties may have additional design
requirements.


> From my pov vendor="host" introduces only problems and is not obvious to users
> and isn't easy to use.

I believe vendor="host" is very obvious and easy to use, and I really
don't see any problem this would introduce. What I think is not obvious
and not intuitive is seeing QEMU report the existence of classes with
the same name but with different defaults, depending on the host CPU you
are running on.


> [...]

On Thu, 14 Feb 2013 17:56:58 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
> > > [...]
> > > > > > +
> > > > > > +}
> > > > > > +
> > > > > > +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.
> > > 
> > > Having both TCG-specific and KVM-specific subclasses instead of making
> > > the KVM class inherit from the TCG class would make sense to me, as we
> > > may have TCG-specific behavior in other places too.
> > > 
> > > Or we could do memcpy() again on the KVM class_init function. Or we
> > > could set a different void* pointer on the TCG class, with
> > > already-filtered x86_def_t object. There are multiple possible
> > > solutions.
> > > 
> > > 
> > > > 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.
> > > 
> > > I'm OK with doing that in a separate patch, as it is not a bug fix or
> > > behavior change. But it would be nice to do that before we introduce the
> > > feature properties, to make the reported defaults right since the
> > > beginning.
> > It's behavior change, if we move filtering from realizefn to class_init,
> > user would be able to add features not visible now to guest.
> > 
> > ordering now:
> > class_init, initfn, setting defautls, custom features,
> > realizefn(filtering)
> > 
> > would be ordering with static properties:
> > clas_init, static props defaults, global properties(custom features),
> > x86_cpu_initfn, realizefn
> > 
> > in would be scenario filtering comes before custom features, which is not
> > necessarily bad and may be nice to consciously add/test features before
> > enabling them in filter for everyone, but it's behavior change.
> > 
> 
> We should keep the filtering on realize because of custom -cpu strings,
> but if we filter on class_init too, we will make the class introspection
> reflect reality more closely. Wasn't this one the main reasons you
> argued (and convinced me) for having separate subclasses?
> 
> Also, if we do this we will be able to make "enforce" work for TCG too.
> For example: if "enforce" worked for TCG today, people who want to work
> around the existing n270 MOVBE bug could use "-cpu n270,+movbe,enforce"
> to make sure the QEMU version they are using really accepts movbe.
I've thought you proposed to move it, if we duplicate it then it should be
fine.

> 
> 
> > > [...]
> > > > > > +#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
> > > 
> > > model_id is static for a given QEMU version, isn't it? This may be OK
> > > (but I am _also_ not sure about this case).
> > I guess you mean constant when using static. 
> 
> Yes, I mean "constant" if by "constant" you mean "you can have
> reasonable expectations that it won't change under certain conditions".
> If the data you got from class introspection could change at any moment
> without any warning, it would be random, useless information.
> 
> What I don't know is: what are reasonable conditions where the class
> data may change, and what are the reasonable conditions where it should
> _not_ change?
> 
> 
> > 
> > > 
> > > 
> > > > * we generate sub-classes at runtime dynamically, which makes source
> > > > level introspection impossible for them.
> > > 
> > > It's not source-level introspection I am looking for, but runtime
> > > introspection that won't surprise other components in the system by
> > > changing unexpectedly (e.g. when the host CPU changes).
> > And I do not think that it must be constant until class is registered.
> > Actually as a user during class introspection, I'd like to see vendor
> > value that will be used, not a placeholder 'host', so I won't have to
> > guess/get from somewhere else what that 'host' actually means.
> 
> What if you run QEMU once for introspection/probing and then run it once
> again? I believe the other components can expect the values won't change
> just if you re-run the same QEMU binary.
> 
> This is not just about keeping it constant during a single QEMU proces
> lifetime. Under certain conditions, the class data has to keep constant
> between QEMU runs, too.
I'm not arguing that they shouldn't stay the same, provided 4-same rule,
you've been talking below, are kept.
For the rest of permutations it's probably too much expectations and should
be considered when problem arises, instead of cornering us into doing hacks.
To keep current behavior we just need obey 4-same rule.

With proposed impl. class defaults will stay the same, unless one
changes to a hardware with different CPU vendor, but that would be violation
4-same rule. And user should be able to see it at once instead of guessing
what 'host' might mean.

> 
> 
> > 
> > > 
> > > > 
> > > > 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.
> > > 
> > > Yes, QOM introspection is run-time based. What I don't know is: what can
> > > be the expectations about the stability of the class data that was just
> > > introspected? How much this information can change when you run QEMU
> > > again? How much can it change if you run the same QEMU binary on a
> > > different host CPU? How much can it change when you run the same QEMU
> > > binary using a different host kernel? How much can it change between
> > > QEMU versions?
> > > 
> > > (I don't know the answer to any of those questions).
> > I don't know answers to these Q either, it probably will be individual on
> > per case basis. But in general due to introspection being runtime why
> > should we corner us to only constants, I'd expect that data might change
> > for above cases between different qemu runs, it should reflect
> > environment we are running on. for 'cpuid.vendor' it is current behavior,
> > and I don't see the reason why we should change it.
> 
> The data may change, yes, but I want to know when it is reasonable to
> change this data.
> 
> I am sure of one thing: if somebody is running the same QEMU vesion on
> the same system (same QEMU binary, same CPU, same kernel, same
> hardware), the data absolutely shouldn't change between QEMU runs, or
> the data would be useless for probing QEMU capabilities.
Agreed.

> 
> And I also think (but I am not sure) we _can_ change the defaults when
> updating to a new QEMU binary. What I don't know is about the
> intermediate cases (changing host CPU, changing host kernel, etc).
host CPUID is the same variable like QEMU version, it's constants embedded in
hardware instead of QEMU binary.

> 
> 
> > 
> > > 
> > > 
> > > > But main point of putting defaults in class_init is because they are
> > > > class wide, whether instance_init is for instance specific settings.
> > > 
> > > We're back to the difference between "property defaults" vs "how cpuid_*
> > > will look like after instance_init is called". Property defaults are
> > > "static data" (considering some definition of "static", at least) and
> > > will always be in class_init, but there's no requirement that the value
> > > of every single bit inside struct X86CPU has be initialized inside
> > > class_init (otherwise instance_init wouldn't even exist!).
> > > 
> > > In other words "the value of X86CPU.cpuid_vendor after instance_init
> > > runs" (that may depend on the host CPU) may be different from "the
> > > default value of the 'vendor' property" (that could be statically set to
> > > "host" if we choose that solution). The latter _must_ be defined by
> > > class_init. The former is simply an internal field calculated by
> > > instance_init, like lots of other X86CPU fields.
> > No, I'm not. I'm not talking about every field in X86CPU. CPUID is
> > architectural constant/default depending on CPU model. In case of KVM
> > CPUID.vendor is just another constant values from POV of consumer. And I
> > propose to treat it the same way as the other defaults (i.e. set it in
> > class_init). (i.e. no special casing unless we have to)
> 
> It's great when we can keep properties constant for a long time, after
> the class is registered and even during instance_init. The problem is
> that to reach this goal you are making class introspection report "less
> constant" data (making it less useful and less reliable).
It's opposite, It makes introspection more useful/reliable since it reflects
CPUID data corresponding to 4-same combination.

> 
> 
> > Yes it can change if you run qemu on different hosts, but it won't do so
> > if you run it on the same host.
> 
> This is what I would like to avoid.
> 
> 
> > 
> > > 
> > > To give another example:
> > > 
> > > * In the future, we may have properties for reading CPU registers
> > > values;
> > > * The EIP register is set to 0xfff0 on CPU reset;
> > > * The CPU will probably be reset on instance_init;
> > > * But that doesn't mean we have to set a 0xfff0 default for the
> > >   "register-EIP" property on class_init.
> > It's not valid comparison, register is 'VARIABLE' when CPUID is 'CONSANT'.
> > PS: 'constant' here means constant after class is registered.
> 
> Well, it is "constant after class is registered" just because you want
> to make it so. What I am proposing is to make vendor "variable" during
> the instance_init() call. _If_ we can't change "vendor" in class_init
> depending on the host CPU, we may _have to_ change it on instance_init,
> we would have no other choice.
> 
> The "vendor" property is already variable between class registration and
> the start of instance_init(), because it can be set using "-cpu
> ...,.vendor=...". I am just asking to allow it to change during a few
> additional nanoseconds, while instance_init() runs. There are lots of
> fields that can change during instance_init and even _after_
> instance_init run. I really don't see what's the big deal in changing
> the value of cpuid_vendor inside instance_init.
cpuid_vendor itself is a variable, which can change, but I prefer it to be
changed via compat/global properties only.
But we are arguing here about default value of vendor.

> 
> Objects are supposed to be dynamic, after all. You are trying to make
> the object "more static" during instance_init (for a reason I don't
> understand) by making the class (that is really supposed to be "more
> static") change depending on the host CPU.
> 
> 
> > 
> > > I agree it is nice when we have properties that more closely match
> > > what's the internal state of the object after instance_init. But _if_ we
> > > have restrictions on how the defaults can change between runs of the
> > > same QEMU binary (which I don't know yet), we may have to hide some
> > > specifics behind an abstraction (like having vendor="host" meaning "use
> > > the host CPU vendor").
> > Well, I'm no aware about such restrictions. But I'm planing re-factoring
> > with keeping current behavior, unless there is reasons to change it.
> > Presently cpu_x86_find_by_name() that overrides 'vendor' is called before
> > object_new(), and it's sort of class_init(), so I'm trying fit it to new
> > model and do it uniformly for all cpuid properties. I don't see why
> > 'vendor' is any different from other properties, when you get x86_cpu_def
> > from cpu_x86_find_by_name() it already has correct vendor values.
> 
> The current code would OK by now because the "vendor" property is not
> static yet. But we will have a problem once we start registering static
> properties, because static properties may have additional design
> requirements.
Could you point out in code to such requirements pls. From my experiments
static properties do not have a technical problems with 'vendor' 'or model_id'
being dynamically generated and set in class_init.

> 
> 
> > From my pov vendor="host" introduces only problems and is not obvious to
> > users and isn't easy to use.
> 
> I believe vendor="host" is very obvious and easy to use, and I really
> don't see any problem this would introduce. What I think is not obvious
> and not intuitive is seeing QEMU report the existence of classes with
> the same name but with different defaults, depending on the host CPU you
> are running on.
Defaults are different because resulting CPUs are different. It's the same
with model_id only in this case instead of host CPU changes QEMU version, but
still the same CPU reports different default value between different QEMU
blobs. Having vendor='host' doesn't make much sense since user won't have a
clue what it really means, which means extra work to be done.

Let's me illustrate it. Imagine user introspected class on remote host and:
 1: got vendor=host, it is NOT obvious what that 'host' means and requires
 extra interface to query what that 'host' means

 2: if QEMU returns a real host's vendor value, user will get it at once and
 won't have to invent/use extra interfaces.

IMHO, 2nd is much more obvious that 1st and less complex.
In 1st case user will have to do twice more work to get the same result as the
2nd provides, and that work only multiplies by amount of cpu_models and hosts
participating in query.

> 
> 
> > [...]
>

I'm happy because the replies are getting shorter!  :-)

(But I'm unhappy because my questions about QOM design requirements are
not being answered yet.)


On Fri, Feb 15, 2013 at 12:49:46PM +0100, Igor Mammedov wrote:
> On Thu, 14 Feb 2013 17:56:58 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
> > > > [...]
> > > > > > > +
> > > > > > > +}
> > > > > > > +
> > > > > > > +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.
> > > > 
> > > > Having both TCG-specific and KVM-specific subclasses instead of making
> > > > the KVM class inherit from the TCG class would make sense to me, as we
> > > > may have TCG-specific behavior in other places too.
> > > > 
> > > > Or we could do memcpy() again on the KVM class_init function. Or we
> > > > could set a different void* pointer on the TCG class, with
> > > > already-filtered x86_def_t object. There are multiple possible
> > > > solutions.
> > > > 
> > > > 
> > > > > 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.
> > > > 
> > > > I'm OK with doing that in a separate patch, as it is not a bug fix or
> > > > behavior change. But it would be nice to do that before we introduce the
> > > > feature properties, to make the reported defaults right since the
> > > > beginning.
> > > It's behavior change, if we move filtering from realizefn to class_init,
> > > user would be able to add features not visible now to guest.
> > > 
> > > ordering now:
> > > class_init, initfn, setting defautls, custom features,
> > > realizefn(filtering)
> > > 
> > > would be ordering with static properties:
> > > clas_init, static props defaults, global properties(custom features),
> > > x86_cpu_initfn, realizefn
> > > 
> > > in would be scenario filtering comes before custom features, which is not
> > > necessarily bad and may be nice to consciously add/test features before
> > > enabling them in filter for everyone, but it's behavior change.
> > > 
> > 
> > We should keep the filtering on realize because of custom -cpu strings,
> > but if we filter on class_init too, we will make the class introspection
> > reflect reality more closely. Wasn't this one the main reasons you
> > argued (and convinced me) for having separate subclasses?
> > 
> > Also, if we do this we will be able to make "enforce" work for TCG too.
> > For example: if "enforce" worked for TCG today, people who want to work
> > around the existing n270 MOVBE bug could use "-cpu n270,+movbe,enforce"
> > to make sure the QEMU version they are using really accepts movbe.
> I've thought you proposed to move it, if we duplicate it then it should be
> fine.

I was, but then you pointed out that it would change behavior.  :-)


> > > > [...]
> > > > > > > +#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
> > > > 
> > > > model_id is static for a given QEMU version, isn't it? This may be OK
> > > > (but I am _also_ not sure about this case).
> > > I guess you mean constant when using static. 
> > 
> > Yes, I mean "constant" if by "constant" you mean "you can have
> > reasonable expectations that it won't change under certain conditions".
> > If the data you got from class introspection could change at any moment
> > without any warning, it would be random, useless information.
> > 
> > What I don't know is: what are reasonable conditions where the class
> > data may change, and what are the reasonable conditions where it should
> > _not_ change?
> > 
> > 
> > > 
> > > > 
> > > > 
> > > > > * we generate sub-classes at runtime dynamically, which makes source
> > > > > level introspection impossible for them.
> > > > 
> > > > It's not source-level introspection I am looking for, but runtime
> > > > introspection that won't surprise other components in the system by
> > > > changing unexpectedly (e.g. when the host CPU changes).
> > > And I do not think that it must be constant until class is registered.
> > > Actually as a user during class introspection, I'd like to see vendor
> > > value that will be used, not a placeholder 'host', so I won't have to
> > > guess/get from somewhere else what that 'host' actually means.
> > 
> > What if you run QEMU once for introspection/probing and then run it once
> > again? I believe the other components can expect the values won't change
> > just if you re-run the same QEMU binary.
> > 
> > This is not just about keeping it constant during a single QEMU proces
> > lifetime. Under certain conditions, the class data has to keep constant
> > between QEMU runs, too.
> I'm not arguing that they shouldn't stay the same, provided 4-same rule,
> you've been talking below, are kept.
> For the rest of permutations it's probably too much expectations and should
> be considered when problem arises, instead of cornering us into doing hacks.
> To keep current behavior we just need obey 4-same rule.
> 
> With proposed impl. class defaults will stay the same, unless one
> changes to a hardware with different CPU vendor, but that would be violation
> 4-same rule. And user should be able to see it at once instead of guessing
> what 'host' might mean.

So, that's the main unanswered questions: the property defaults for a
class should be kept stable in some conditions, but _which conditions_
exactly? You believe it should be constant if keeping the "4-same" rule.


> > > > > 
> > > > > 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.
> > > > 
> > > > Yes, QOM introspection is run-time based. What I don't know is: what can
> > > > be the expectations about the stability of the class data that was just
> > > > introspected? How much this information can change when you run QEMU
> > > > again? How much can it change if you run the same QEMU binary on a
> > > > different host CPU? How much can it change when you run the same QEMU
> > > > binary using a different host kernel? How much can it change between
> > > > QEMU versions?
> > > > 
> > > > (I don't know the answer to any of those questions).
> > > I don't know answers to these Q either, it probably will be individual on
> > > per case basis. But in general due to introspection being runtime why
> > > should we corner us to only constants, I'd expect that data might change
> > > for above cases between different qemu runs, it should reflect
> > > environment we are running on. for 'cpuid.vendor' it is current behavior,
> > > and I don't see the reason why we should change it.
> > 
> > The data may change, yes, but I want to know when it is reasonable to
> > change this data.
> > 
> > I am sure of one thing: if somebody is running the same QEMU vesion on
> > the same system (same QEMU binary, same CPU, same kernel, same
> > hardware), the data absolutely shouldn't change between QEMU runs, or
> > the data would be useless for probing QEMU capabilities.
> Agreed.
> 
> > 
> > And I also think (but I am not sure) we _can_ change the defaults when
> > updating to a new QEMU binary. What I don't know is about the
> > intermediate cases (changing host CPU, changing host kernel, etc).
> host CPUID is the same variable like QEMU version, it's constants embedded in
> hardware instead of QEMU binary.

That's what I am unsure of.


> > > > 
> > > > > But main point of putting defaults in class_init is because they are
> > > > > class wide, whether instance_init is for instance specific settings.
> > > > 
> > > > We're back to the difference between "property defaults" vs "how cpuid_*
> > > > will look like after instance_init is called". Property defaults are
> > > > "static data" (considering some definition of "static", at least) and
> > > > will always be in class_init, but there's no requirement that the value
> > > > of every single bit inside struct X86CPU has be initialized inside
> > > > class_init (otherwise instance_init wouldn't even exist!).
> > > > 
> > > > In other words "the value of X86CPU.cpuid_vendor after instance_init
> > > > runs" (that may depend on the host CPU) may be different from "the
> > > > default value of the 'vendor' property" (that could be statically set to
> > > > "host" if we choose that solution). The latter _must_ be defined by
> > > > class_init. The former is simply an internal field calculated by
> > > > instance_init, like lots of other X86CPU fields.
> > > No, I'm not. I'm not talking about every field in X86CPU. CPUID is
> > > architectural constant/default depending on CPU model. In case of KVM
> > > CPUID.vendor is just another constant values from POV of consumer. And I
> > > propose to treat it the same way as the other defaults (i.e. set it in
> > > class_init). (i.e. no special casing unless we have to)
> > 
> > It's great when we can keep properties constant for a long time, after
> > the class is registered and even during instance_init. The problem is
> > that to reach this goal you are making class introspection report "less
> > constant" data (making it less useful and less reliable).
> It's opposite, It makes introspection more useful/reliable since it reflects
> CPUID data corresponding to 4-same combination.

OK, the "usefulness" of the dta is all about assumptions: I am assuming
that other components expect the data is expected to reflect
capabilities/behavior of a specific QEMU binary/version, not the of the
binary + its environment. If somebody guarantees me that it is 100% OK
to make the class data be a function of the QEMU binary + host kernel +
host hardware, then I will be OK with your proposal.


> 
> > 
> > 
> > > Yes it can change if you run qemu on different hosts, but it won't do so
> > > if you run it on the same host.
> > 
> > This is what I would like to avoid.
> > 
> > 
> > > 
> > > > 
> > > > To give another example:
> > > > 
> > > > * In the future, we may have properties for reading CPU registers
> > > > values;
> > > > * The EIP register is set to 0xfff0 on CPU reset;
> > > > * The CPU will probably be reset on instance_init;
> > > > * But that doesn't mean we have to set a 0xfff0 default for the
> > > >   "register-EIP" property on class_init.
> > > It's not valid comparison, register is 'VARIABLE' when CPUID is 'CONSANT'.
> > > PS: 'constant' here means constant after class is registered.
> > 
> > Well, it is "constant after class is registered" just because you want
> > to make it so. What I am proposing is to make vendor "variable" during
> > the instance_init() call. _If_ we can't change "vendor" in class_init
> > depending on the host CPU, we may _have to_ change it on instance_init,
> > we would have no other choice.
> > 
> > The "vendor" property is already variable between class registration and
> > the start of instance_init(), because it can be set using "-cpu
> > ...,.vendor=...". I am just asking to allow it to change during a few
> > additional nanoseconds, while instance_init() runs. There are lots of
> > fields that can change during instance_init and even _after_
> > instance_init run. I really don't see what's the big deal in changing
> > the value of cpuid_vendor inside instance_init.
> cpuid_vendor itself is a variable, which can change, but I prefer it to be
> changed via compat/global properties only.
> But we are arguing here about default value of vendor.

Yes, and about the meaning of the default value of vendor.


> 
> > 
> > Objects are supposed to be dynamic, after all. You are trying to make
> > the object "more static" during instance_init (for a reason I don't
> > understand) by making the class (that is really supposed to be "more
> > static") change depending on the host CPU.
> > 
> > 
> > > 
> > > > I agree it is nice when we have properties that more closely match
> > > > what's the internal state of the object after instance_init. But _if_ we
> > > > have restrictions on how the defaults can change between runs of the
> > > > same QEMU binary (which I don't know yet), we may have to hide some
> > > > specifics behind an abstraction (like having vendor="host" meaning "use
> > > > the host CPU vendor").
> > > Well, I'm no aware about such restrictions. But I'm planing re-factoring
> > > with keeping current behavior, unless there is reasons to change it.
> > > Presently cpu_x86_find_by_name() that overrides 'vendor' is called before
> > > object_new(), and it's sort of class_init(), so I'm trying fit it to new
> > > model and do it uniformly for all cpuid properties. I don't see why
> > > 'vendor' is any different from other properties, when you get x86_cpu_def
> > > from cpu_x86_find_by_name() it already has correct vendor values.
> > 
> > The current code would OK by now because the "vendor" property is not
> > static yet. But we will have a problem once we start registering static
> > properties, because static properties may have additional design
> > requirements.
> Could you point out in code to such requirements pls. From my experiments
> static properties do not have a technical problems with 'vendor' 'or model_id'
> being dynamically generated and set in class_init.

The requirements are not on the code, but on what I believe is the
reasoning behind the existence of static properties in the QOM design.


> 
> > 
> > 
> > > From my pov vendor="host" introduces only problems and is not obvious to
> > > users and isn't easy to use.
> > 
> > I believe vendor="host" is very obvious and easy to use, and I really
> > don't see any problem this would introduce. What I think is not obvious
> > and not intuitive is seeing QEMU report the existence of classes with
> > the same name but with different defaults, depending on the host CPU you
> > are running on.
> Defaults are different because resulting CPUs are different. It's the same
> with model_id only in this case instead of host CPU changes QEMU version, but
> still the same CPU reports different default value between different QEMU
> blobs. Having vendor='host' doesn't make much sense since user won't have a
> clue what it really means, which means extra work to be done.
> 
> Let's me illustrate it. Imagine user introspected class on remote host and:
>  1: got vendor=host, it is NOT obvious what that 'host' means and requires
>  extra interface to query what that 'host' means
> 
>  2: if QEMU returns a real host's vendor value, user will get it at once and
>  won't have to invent/use extra interfaces.
> 
> IMHO, 2nd is much more obvious that 1st and less complex.
> In 1st case user will have to do twice more work to get the same result as the
> 2nd provides, and that work only multiplies by amount of cpu_models and hosts
> participating in query.

Well, it is all about expectations. I find it less intuitive because I
never expected the introspectable class data to depend on the
environment where QEMU is running, but to just reflect the capabilities
of the QEMU binary being queried and nothing else.

If we get confirmation that the introspectable class data may change
depending on the host CPU, then I will agree 100% with you.


> > 
> > 
> > > [...]
> > 
>