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

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

[Andreas Färber]

Am 08.02.2013 10:03, schrieb Igor Mammedov:
> On Thu, 7 Feb 2013 13:08:19 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
>>> From: Andreas Färber <afaerber@suse.de>
>>>
>>> Move x86_def_t definition to header and embed into X86CPUClass.
>>> Register types per built-in model definition.
>>>
>>> Move version initialization from x86_cpudef_setup() to class_init.
>>>
>>> Inline cpu_x86_register() into the X86CPU initfn.
>>> Since instance_init cannot reports errors, drop error handling.
>>>
>>> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
>>> Move handling of KVM host vendor override from cpu_x86_find_by_name()
>>> to the kvm_arch_init() and class_init(). Use TYPE_X86_CPU class to
>>> communicate kvm specific defaults to other sub-classes.
>>>
>>> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs
>>> and only when KVM is enabled to avoid hacks in CPU code.
>>> Make kvm_cpu_fill_host() into a host specific class_init and inline
>>> cpu_x86_fill_model_id().
>>>
>>> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
>>> comparison.
>>>
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> v4:
>>>   * set error if cpu model is not found and goto out;
>>>   * copy vendor override from 'host' CPU class in sub-class'es
>>>     class_init() if 'host' CPU class is available.
>>>   * register type TYPE_HOST_X86_CPU in kvm_arch_init(), this type
>>>     should be available only in KVM mode and we haven't printed it in
>>>     -cpu ? output so far, so we can continue doing so. It's not
>>>     really confusing to show 'host' cpu (even if we do it) when KVM
>>>     is not enabled.
>>>   * remove special case for 'host' CPU check in x86_cpu_class_by_name(),
>>>     due to 'host' CPU will not find anything if not in KVM mode or
>>>     return 'host' CPU class in KVM mode, i.e. treat it as regular CPUs.
>>> ---
>>>  target-i386/cpu-qom.h |   24 ++++
>>>  target-i386/cpu.c     |  331 ++++++++++++++++++-------------------------------
>>>  target-i386/cpu.h     |    5 +-
>>>  target-i386/kvm.c     |   72 +++++++++++
>>>  4 files changed, 217 insertions(+), 215 deletions(-)
>>>
>>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
>>> index 48e6b54..80bf72d 100644
>>> --- a/target-i386/cpu-qom.h
>>> +++ b/target-i386/cpu-qom.h
>>> @@ -30,6 +30,27 @@
>>>  #define TYPE_X86_CPU "i386-cpu"
>>>  #endif
>>>  
>>> +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
>>
>> Can we introduce a X86_CPU_CLASS_NAME() macro to help us make the rules
>> to generate CPU class names clearer?
>>
>> e.g.:
>>
>> #define X86_CPU_CLASS_NAME(s) s "-" TYPE_X86_CPU
>> [...]
>> #define TYPE_HOST_X86_CPU X86_CPU_CLASS_NAME("host")
>> [...]
>>   /* (at the class lookup code) */
>>   typename = g_strdup_printf(X86_CPU_CLASS_NAME("%s"), name);
> I kind of like Andreas' variant not hiding format string in macro, for
> one doesn't have look-up what macro does to see how name will look.
> Especially since it's called in only 2 places.
> 
>>
>>
>>
>>> +
>>> +typedef struct x86_def_t {
>>> +    const char *name;
>>> +    uint32_t level;
>>> +    /* vendor is zero-terminated, 12 character ASCII string */
>>> +    char vendor[CPUID_VENDOR_SZ + 1];
>>> +    int family;
>>> +    int model;
>>> +    int stepping;
>>> +    uint32_t features, ext_features, ext2_features, ext3_features;
>>> +    uint32_t kvm_features, svm_features;
>>> +    uint32_t xlevel;
>>> +    char model_id[48];
>>> +    /* Store the results of Centaur's CPUID instructions */
>>> +    uint32_t ext4_features;
>>> +    uint32_t xlevel2;
>>> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
>>> +    uint32_t cpuid_7_0_ebx_features;
>>> +} x86_def_t;
>>> +
>>>  #define X86_CPU_CLASS(klass) \
>>>      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
>>>  #define X86_CPU(obj) \
>>> @@ -41,6 +62,7 @@
>>>   * X86CPUClass:
>>>   * @parent_realize: The parent class' realize handler.
>>>   * @parent_reset: The parent class' reset handler.
>>> + * @info: Model-specific data.
>>>   *
>>>   * An x86 CPU model or family.
>>>   */
>>> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
>>>  
>>>      DeviceRealize parent_realize;
>>>      void (*parent_reset)(CPUState *cpu);
>>> +
>>> +    x86_def_t info;
>>
>> I thought you had suggesting making it a pointer. If you made it a
>> pointer, you wouldn't need to move the x86_def_t declaration to
>> cpu-qom.h.
> 
> x86_def_t is needed in kvm.c for host class. So there is no much point
> in changing info into pointer, considering it's temporary solution.

The main reason I did it this way was to avoid a g_malloc0() in a
non-failing class_init. Also it is closer to having class fields sit
directly in X86CPUClass.

>>>  } X86CPUClass;
>>>  
>>>  /**
>>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>>> index 1aee097..62fdc84 100644
>>> --- a/target-i386/cpu.c
>>> +++ b/target-i386/cpu.c
>>> @@ -47,8 +47,8 @@
> [...]
> 
>>> @@ -2195,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
>>>      CPUState *cs = CPU(obj);
>>>      X86CPU *cpu = X86_CPU(obj);
>>>      CPUX86State *env = &cpu->env;
>>> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
>>> +    const x86_def_t *def = &xcc->info;
>>>      static int inited;
>>>  
>>>      cpu_exec_init(env);
>>> @@ -2224,6 +2049,28 @@ static void x86_cpu_initfn(Object *obj)
>>>                          x86_cpuid_get_tsc_freq,
>>>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
>>>  
>>> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
>>> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
>>> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
>>> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
>>> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
>>> +    env->cpuid_features = def->features;
>>> +    env->cpuid_ext_features = def->ext_features;
>>> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
>>> +    env->cpuid_ext2_features = def->ext2_features;
>>> +    env->cpuid_ext3_features = def->ext3_features;
>>> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
>>> +    env->cpuid_kvm_features = def->kvm_features;
>>> +    if (kvm_enabled()) {
>>> +        env->cpuid_kvm_features |= kvm_default_features;
>>> +    }
>>
>> "-cpu host,enforce" is supposed to never fail. What if the host doesn't
>> support some of the features present in kvm_default_features? We need to
>> use kvm_default_features only if the CPU model is not "host".
>>
>> But this is an existing bug in the code, you are not introducing it with
>> this patch.
>>
> whould be moving it in x86_cpu_def_class_init() suitable solution?
> 
>>
>>> +    env->cpuid_svm_features = def->svm_features;
>>> +    env->cpuid_ext4_features = def->ext4_features;
>>> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
>>> +    env->cpuid_xlevel2 = def->xlevel2;
>>> +
>>> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
>>> +
>>>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
>>>  
>>>      /* init various static tables used in TCG mode */
>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
>>>      }
>>>  }
>>>  
>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
>>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
>>> +    X86CPUClass *hostcc;
>>> +    x86_def_t *def = data;
>>> +    int i;
>>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
>>> +
>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
>>> +
>>> +    /* host cpu class is available if KVM is enabled,
>>> +     * get kvm overrides from it */
>>> +    if (hoc) {
>>> +        hostcc = X86_CPU_CLASS(hoc);
>>> +        /* sysenter isn't supported in compatibility mode on AMD,
>>> +         * syscall isn't supported in compatibility mode on Intel.
>>> +         * Normally we advertise the actual CPU vendor, but you can
>>> +         * override this using the 'vendor' property if you want to use
>>> +         * KVM's sysenter/syscall emulation in compatibility mode and
>>> +         * when doing cross vendor migration
>>> +         */
>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
>>> +               sizeof(xcc->info.vendor));
>>> +    }
>>
>> Again, we have the same problem we had before, but now in the non-host
>> classes. What if class_init is called before KVM is initialized? I
>> believe we will be forced to move this hack to the instance init
>> function.
> I believe, the in the case where non-host CPU classes might be initialized
> before KVM "-cpu ?" we do not care what their defaults are, since we only
> would use class names there and then exit.
> 
> For case where classes could be inspected over QMP, OQM, KVM would be already
> initialized if enabled and we would get proper initialization order without
> hack.

I think you're missing Eduardo's and my point:

diff --git a/vl.c b/vl.c
index a8dc73d..6b9378e 100644
--- a/vl.c
+++ b/vl.c
@@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
     }

     module_call_init(MODULE_INIT_QOM);
+    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);

     qemu_add_opts(&qemu_drive_opts);
     qemu_add_opts(&qemu_chardev_opts);

Anyone may iterate over QOM classes at any time after their type
registration, which is before the first round of option parsing.
Sometime later, after option parsing, there's the -cpu ? handling in
vl.c:3854, then vl.c:4018:configure_accelerator().

Like I said, mostly a theoretical issue today.

Originally I had considered making kvm_init() re-entrant and calling it
from the offending class_init. But we must support the distro case of
compiling with CONFIG_KVM but the user's hardware not supporting KVM or
kvm module not being loaded or the user having insufficient priviledges
to access /dev/kvm.

> Instead of adding hack, I'd rather enforce valid initialization order and
> abort in initfn() if type was initialized without KVM present and KVM is
> enabled at initfn() time. Something along the lines:
> 
> static x86_cpu_builtin_class_initialized_without_kvm;
> 
> x86_cpu_def_class_init() {
>     ...
>     if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kvm) {
>         x86_cpu_builtin_class_initialized_without_kvm = true;
>     }
>     ...
> }
> 
> initfn() {
>     ...
>     if (kvm_enabled() && x86_cpu_builtin_class_initialized_without_kvm) {
>         abort();
>     }
>     ...
> }

We could add an inited field to X86CPUClass that gets checked at initfn
time (only ever getting set to true by the pre-defined models). Then it
would be per model. And if we add a prototype for the ..._class_init we
could even call it late as proposed for -cpu host if we take some
precautions and add explanatory comments.

>> If we still want the default vendor to be a static property in the
>> class, we can do that if we set the default in a "tcg-vendor" property
>> instead of a "vendor" property (that would be empty/unset by default),
>> and x86_cpu_initfn() could do this:
>>
>>     vendor = object_property_get_str(cpu, "vendor");
>>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
>>     if (vendor && vendor[0]) {
>>       cpu->cpuid_vendor = vendor;
>>     } else if (kvm_enabled()) {
>>       cpu->cpuid_vendor = get_host_vendor();
>>     } else {
>>       cpu->cpuid_vendor = tcg_vendor;
>>     }
>>
>>> +
>>> +    /* Look for specific models that have the QEMU version in .model_id */
>>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
>>> +        if (strcmp(versioned_models[i], def->name) == 0) {
>>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
>>> +                    "QEMU Virtual CPU version ");
>>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
>>> +                    qemu_get_version());
>>> +            break;
>>> +        }
>>> +    }
>>> +}
>>> +
> [...]
> 
>>> --- a/target-i386/kvm.c
>>> +++ b/target-i386/kvm.c
>>> @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
>>>      return ret;
>>>  }
>>>  
> [...]
> 
>>>  int kvm_arch_init(KVMState *s)
>>>  {
>>>      QemuOptsList *list = qemu_find_opts("machine");
>>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
>>>      int ret;
>>>      struct utsname utsname;
>>>  
>>> +    static const TypeInfo host_x86_cpu_type_info = {
>>> +        .name = TYPE_HOST_X86_CPU,
>>> +        .parent = TYPE_X86_CPU,
>>> +        .class_init = kvm_host_cpu_class_init,
>>> +    };
>>> +
>>>      ret = kvm_get_supported_msrs(s);
>>>      if (ret < 0) {
>>>          return ret;
>>> @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
>>>              }
>>>          }
>>>      }
>>> +
>>> +    type_register(&host_x86_cpu_type_info);
>>
>> Are we really allowed to register QOM classes that late?
>>
>> If QOM design allows us to register the class very late (I would like to
>> confirm that), this approach sounds really clean and sane to me.
>> Pre-KVM-init class introspection of the "host" class would be completely
>> useless anyway (because all data in the "host" class depend on data
>> available only post-KVM-init anyway).
> 
> Looking at type_register() impl. it seems safe to do so + relying on QBL for
> type_table_add() safety. So it's really design question for QOM experts.
> 
> Antnony, Paolo, Andreas
>  what do you think?

I already answered Eduardo on IRC that in general I see no restriction
not to register a type late.

The issue is that in this case we cannot rely on accessing the class
from another class_init that is registered before it, which you were
doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
route).

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Igor Mammedov]

On Fri, 08 Feb 2013 12:16:17 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > On Thu, 7 Feb 2013 13:08:19 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> >> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> >>> From: Andreas Färber <afaerber@suse.de>
> >>>
> >>> Move x86_def_t definition to header and embed into X86CPUClass.
> >>> Register types per built-in model definition.
> >>>
> >>> Move version initialization from x86_cpudef_setup() to class_init.
> >>>
> >>> Inline cpu_x86_register() into the X86CPU initfn.
> >>> Since instance_init cannot reports errors, drop error handling.
> >>>
> >>> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
> >>> Move handling of KVM host vendor override from cpu_x86_find_by_name()
> >>> to the kvm_arch_init() and class_init(). Use TYPE_X86_CPU class to
> >>> communicate kvm specific defaults to other sub-classes.
> >>>
> >>> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs
> >>> and only when KVM is enabled to avoid hacks in CPU code.
> >>> Make kvm_cpu_fill_host() into a host specific class_init and inline
> >>> cpu_x86_fill_model_id().
> >>>
> >>> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
> >>> comparison.
> >>>
> >>> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>> ---
> >>> v4:
> >>>   * set error if cpu model is not found and goto out;
> >>>   * copy vendor override from 'host' CPU class in sub-class'es
> >>>     class_init() if 'host' CPU class is available.
> >>>   * register type TYPE_HOST_X86_CPU in kvm_arch_init(), this type
> >>>     should be available only in KVM mode and we haven't printed it in
> >>>     -cpu ? output so far, so we can continue doing so. It's not
> >>>     really confusing to show 'host' cpu (even if we do it) when KVM
> >>>     is not enabled.
> >>>   * remove special case for 'host' CPU check in x86_cpu_class_by_name(),
> >>>     due to 'host' CPU will not find anything if not in KVM mode or
> >>>     return 'host' CPU class in KVM mode, i.e. treat it as regular CPUs.
> >>> ---
> >>>  target-i386/cpu-qom.h |   24 ++++
> >>>  target-i386/cpu.c     |  331 ++++++++++++++++++-------------------------------
> >>>  target-i386/cpu.h     |    5 +-
> >>>  target-i386/kvm.c     |   72 +++++++++++
> >>>  4 files changed, 217 insertions(+), 215 deletions(-)
> >>>
> >>> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >>> index 48e6b54..80bf72d 100644
> >>> --- a/target-i386/cpu-qom.h
> >>> +++ b/target-i386/cpu-qom.h
> >>> @@ -30,6 +30,27 @@
> >>>  #define TYPE_X86_CPU "i386-cpu"
> >>>  #endif
> >>>  
> >>> +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU
> >>
> >> Can we introduce a X86_CPU_CLASS_NAME() macro to help us make the rules
> >> to generate CPU class names clearer?
> >>
> >> e.g.:
> >>
> >> #define X86_CPU_CLASS_NAME(s) s "-" TYPE_X86_CPU
> >> [...]
> >> #define TYPE_HOST_X86_CPU X86_CPU_CLASS_NAME("host")
> >> [...]
> >>   /* (at the class lookup code) */
> >>   typename = g_strdup_printf(X86_CPU_CLASS_NAME("%s"), name);
> > I kind of like Andreas' variant not hiding format string in macro, for
> > one doesn't have look-up what macro does to see how name will look.
> > Especially since it's called in only 2 places.
> > 
> >>
> >>
> >>
> >>> +
> >>> +typedef struct x86_def_t {
> >>> +    const char *name;
> >>> +    uint32_t level;
> >>> +    /* vendor is zero-terminated, 12 character ASCII string */
> >>> +    char vendor[CPUID_VENDOR_SZ + 1];
> >>> +    int family;
> >>> +    int model;
> >>> +    int stepping;
> >>> +    uint32_t features, ext_features, ext2_features, ext3_features;
> >>> +    uint32_t kvm_features, svm_features;
> >>> +    uint32_t xlevel;
> >>> +    char model_id[48];
> >>> +    /* Store the results of Centaur's CPUID instructions */
> >>> +    uint32_t ext4_features;
> >>> +    uint32_t xlevel2;
> >>> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> >>> +    uint32_t cpuid_7_0_ebx_features;
> >>> +} x86_def_t;
> >>> +
> >>>  #define X86_CPU_CLASS(klass) \
> >>>      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
> >>>  #define X86_CPU(obj) \
> >>> @@ -41,6 +62,7 @@
> >>>   * X86CPUClass:
> >>>   * @parent_realize: The parent class' realize handler.
> >>>   * @parent_reset: The parent class' reset handler.
> >>> + * @info: Model-specific data.
> >>>   *
> >>>   * An x86 CPU model or family.
> >>>   */
> >>> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
> >>>  
> >>>      DeviceRealize parent_realize;
> >>>      void (*parent_reset)(CPUState *cpu);
> >>> +
> >>> +    x86_def_t info;
> >>
> >> I thought you had suggesting making it a pointer. If you made it a
> >> pointer, you wouldn't need to move the x86_def_t declaration to
> >> cpu-qom.h.
> > 
> > x86_def_t is needed in kvm.c for host class. So there is no much point
> > in changing info into pointer, considering it's temporary solution.
> 
> The main reason I did it this way was to avoid a g_malloc0() in a
> non-failing class_init. Also it is closer to having class fields sit
> directly in X86CPUClass.
> 
> >>>  } X86CPUClass;
> >>>  
> >>>  /**
> >>> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> >>> index 1aee097..62fdc84 100644
> >>> --- a/target-i386/cpu.c
> >>> +++ b/target-i386/cpu.c
> >>> @@ -47,8 +47,8 @@
> > [...]
> > 
> >>> @@ -2195,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
> >>>      CPUState *cs = CPU(obj);
> >>>      X86CPU *cpu = X86_CPU(obj);
> >>>      CPUX86State *env = &cpu->env;
> >>> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> >>> +    const x86_def_t *def = &xcc->info;
> >>>      static int inited;
> >>>  
> >>>      cpu_exec_init(env);
> >>> @@ -2224,6 +2049,28 @@ static void x86_cpu_initfn(Object *obj)
> >>>                          x86_cpuid_get_tsc_freq,
> >>>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> >>>  
> >>> +    object_property_set_str(OBJECT(cpu), def->vendor, "vendor", NULL);
> >>> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
> >>> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
> >>> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
> >>> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping", NULL);
> >>> +    env->cpuid_features = def->features;
> >>> +    env->cpuid_ext_features = def->ext_features;
> >>> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
> >>> +    env->cpuid_ext2_features = def->ext2_features;
> >>> +    env->cpuid_ext3_features = def->ext3_features;
> >>> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
> >>> +    env->cpuid_kvm_features = def->kvm_features;
> >>> +    if (kvm_enabled()) {
> >>> +        env->cpuid_kvm_features |= kvm_default_features;
> >>> +    }
> >>
> >> "-cpu host,enforce" is supposed to never fail. What if the host doesn't
> >> support some of the features present in kvm_default_features? We need to
> >> use kvm_default_features only if the CPU model is not "host".
> >>
> >> But this is an existing bug in the code, you are not introducing it with
> >> this patch.
> >>
> > whould be moving it in x86_cpu_def_class_init() suitable solution?
> > 
> >>
> >>> +    env->cpuid_svm_features = def->svm_features;
> >>> +    env->cpuid_ext4_features = def->ext4_features;
> >>> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> >>> +    env->cpuid_xlevel2 = def->xlevel2;
> >>> +
> >>> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id", NULL);
> >>> +
> >>>      env->cpuid_apic_id = x86_cpu_apic_id_from_index(cs->cpu_index);
> >>>  
> >>>      /* init various static tables used in TCG mode */
> >>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> >>>      }
> >>>  }
> >>>  
> >>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> >>> +{
> >>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> >>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
> >>> +    X86CPUClass *hostcc;
> >>> +    x86_def_t *def = data;
> >>> +    int i;
> >>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
> >>> +
> >>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> >>> +
> >>> +    /* host cpu class is available if KVM is enabled,
> >>> +     * get kvm overrides from it */
> >>> +    if (hoc) {
> >>> +        hostcc = X86_CPU_CLASS(hoc);
> >>> +        /* sysenter isn't supported in compatibility mode on AMD,
> >>> +         * syscall isn't supported in compatibility mode on Intel.
> >>> +         * Normally we advertise the actual CPU vendor, but you can
> >>> +         * override this using the 'vendor' property if you want to use
> >>> +         * KVM's sysenter/syscall emulation in compatibility mode and
> >>> +         * when doing cross vendor migration
> >>> +         */
> >>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> >>> +               sizeof(xcc->info.vendor));
> >>> +    }
> >>
> >> Again, we have the same problem we had before, but now in the non-host
> >> classes. What if class_init is called before KVM is initialized? I
> >> believe we will be forced to move this hack to the instance init
> >> function.
> > I believe, the in the case where non-host CPU classes might be initialized
> > before KVM "-cpu ?" we do not care what their defaults are, since we only
> > would use class names there and then exit.
> > 
> > For case where classes could be inspected over QMP, OQM, KVM would be already
> > initialized if enabled and we would get proper initialization order without
> > hack.
> 
> I think you're missing Eduardo's and my point:
> 
> diff --git a/vl.c b/vl.c
> index a8dc73d..6b9378e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
>      }
> 
>      module_call_init(MODULE_INIT_QOM);
> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> 
>      qemu_add_opts(&qemu_drive_opts);
>      qemu_add_opts(&qemu_chardev_opts);
> 
> Anyone may iterate over QOM classes at any time after their type
> registration, which is before the first round of option parsing.
> Sometime later, after option parsing, there's the -cpu ? handling in
> vl.c:3854, then vl.c:4018:configure_accelerator().
> 
> Like I said, mostly a theoretical issue today.
Question is if we should drop this theoretical issue for 1.5?

> 
> Originally I had considered making kvm_init() re-entrant and calling it
> from the offending class_init. But we must support the distro case of
> compiling with CONFIG_KVM but the user's hardware not supporting KVM or
> kvm module not being loaded or the user having insufficient priviledges
> to access /dev/kvm.
working without KVM is not issue, it just works with normal defaults. Applying
KVM specific defaults to already initialized classes is.
 
> 
> > Instead of adding hack, I'd rather enforce valid initialization order and
> > abort in initfn() if type was initialized without KVM present and KVM is
> > enabled at initfn() time. Something along the lines:
> > 
> > static x86_cpu_builtin_class_initialized_without_kvm;
> > 
> > x86_cpu_def_class_init() {
> >     ...
> >     if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kvm) {
> >         x86_cpu_builtin_class_initialized_without_kvm = true;
> >     }
> >     ...
> > }
> > 
> > initfn() {
> >     ...
> >     if (kvm_enabled() && x86_cpu_builtin_class_initialized_without_kvm) {
> >         abort();
> >     }
> >     ...
> > }
> 
Continuing on theoretical issue:
> We could add an inited field to X86CPUClass that gets checked at initfn
> time (only ever getting set to true by the pre-defined models). Then it
> would be per model. And if we add a prototype for the ..._class_init we
> could even call it late as proposed for -cpu host if we take some
             ^^^^^^^^^^^^ is a tricky part, for global properties to work it
would require, calling this hook after kvm_init() is succeeds and before
any initfn() is called in general or as minimum before Device.initfn(). And it
anyway will not make all CPU classes to have correct defaults in KVM mode,
since only CPU class of created CPU instance will be fixed up.

1. One way to make sure that built-in CPU classes have fixed up defaults is to
iterate over them in kvm_arch_init() and possibly calling their class_init()
again to reinitialize. It's still hack (due fixing something up), but it would
give at least correct KVM mode defaults, regardless of the order classes are
initialized.

2. But more correct way from POV of OOP would be one without any fixups, i.e.
create extra KVM-builtin-CPU-classes that are derived from host class.
and in object_class_by_name() lookup for them if kvm is enabled. But we could
do this as follow-up to #1.
 
> precautions and add explanatory comments.
> 
> >> If we still want the default vendor to be a static property in the
> >> class, we can do that if we set the default in a "tcg-vendor" property
> >> instead of a "vendor" property (that would be empty/unset by default),
> >> and x86_cpu_initfn() could do this:
> >>
> >>     vendor = object_property_get_str(cpu, "vendor");
> >>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
> >>     if (vendor && vendor[0]) {
> >>       cpu->cpuid_vendor = vendor;
> >>     } else if (kvm_enabled()) {
> >>       cpu->cpuid_vendor = get_host_vendor();
> >>     } else {
> >>       cpu->cpuid_vendor = tcg_vendor;
> >>     }
> >>
> >>> +
> >>> +    /* Look for specific models that have the QEMU version in .model_id */
> >>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
> >>> +        if (strcmp(versioned_models[i], def->name) == 0) {
> >>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
> >>> +                    "QEMU Virtual CPU version ");
> >>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
> >>> +                    qemu_get_version());
> >>> +            break;
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> > [...]
> > 
> >>> --- a/target-i386/kvm.c
> >>> +++ b/target-i386/kvm.c
> >>> @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
> >>>      return ret;
> >>>  }
> >>>  
> > [...]
> > 
> >>>  int kvm_arch_init(KVMState *s)
> >>>  {
> >>>      QemuOptsList *list = qemu_find_opts("machine");
> >>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
> >>>      int ret;
> >>>      struct utsname utsname;
> >>>  
> >>> +    static const TypeInfo host_x86_cpu_type_info = {
> >>> +        .name = TYPE_HOST_X86_CPU,
> >>> +        .parent = TYPE_X86_CPU,
> >>> +        .class_init = kvm_host_cpu_class_init,
> >>> +    };
> >>> +
> >>>      ret = kvm_get_supported_msrs(s);
> >>>      if (ret < 0) {
> >>>          return ret;
> >>> @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
> >>>              }
> >>>          }
> >>>      }
> >>> +
> >>> +    type_register(&host_x86_cpu_type_info);
> >>
> >> Are we really allowed to register QOM classes that late?
> >>
> >> If QOM design allows us to register the class very late (I would like to
> >> confirm that), this approach sounds really clean and sane to me.
> >> Pre-KVM-init class introspection of the "host" class would be completely
> >> useless anyway (because all data in the "host" class depend on data
> >> available only post-KVM-init anyway).
> > 
> > Looking at type_register() impl. it seems safe to do so + relying on QBL for
> > type_table_add() safety. So it's really design question for QOM experts.
> > 
> > Antnony, Paolo, Andreas
> >  what do you think?
> 
> I already answered Eduardo on IRC that in general I see no restriction
> not to register a type late.
> 
> The issue is that in this case we cannot rely on accessing the class
> from another class_init that is registered before it, which you were
> doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
> route).
If we are accessing host class somewhere, then we would like to access its
initialized state, not a dummy state which gives us nothing.

sure, I'll rename hoc => host_oc.

> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 


-- 
Regards,
  Igor

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

[Eduardo Habkost]

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

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

But I am not completely against the current version.


> > > 
> > >>
> > >>
> > >>
> > >>> +
> > >>> +typedef struct x86_def_t {
> > >>> +    const char *name;
> > >>> +    uint32_t level;
> > >>> +    /* vendor is zero-terminated, 12 character ASCII string */
> > >>> +    char vendor[CPUID_VENDOR_SZ + 1];
> > >>> +    int family;
> > >>> +    int model;
> > >>> +    int stepping;
> > >>> +    uint32_t features, ext_features, ext2_features, ext3_features;
> > >>> +    uint32_t kvm_features, svm_features;
> > >>> +    uint32_t xlevel;
> > >>> +    char model_id[48];
> > >>> +    /* Store the results of Centaur's CPUID instructions */
> > >>> +    uint32_t ext4_features;
> > >>> +    uint32_t xlevel2;
> > >>> +    /* The feature bits on CPUID[EAX=7,ECX=0].EBX */
> > >>> +    uint32_t cpuid_7_0_ebx_features;
> > >>> +} x86_def_t;
> > >>> +
> > >>>  #define X86_CPU_CLASS(klass) \
> > >>>      OBJECT_CLASS_CHECK(X86CPUClass, (klass), TYPE_X86_CPU)
> > >>>  #define X86_CPU(obj) \
> > >>> @@ -41,6 +62,7 @@
> > >>>   * X86CPUClass:
> > >>>   * @parent_realize: The parent class' realize handler.
> > >>>   * @parent_reset: The parent class' reset handler.
> > >>> + * @info: Model-specific data.
> > >>>   *
> > >>>   * An x86 CPU model or family.
> > >>>   */
> > >>> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
> > >>>  
> > >>>      DeviceRealize parent_realize;
> > >>>      void (*parent_reset)(CPUState *cpu);
> > >>> +
> > >>> +    x86_def_t info;
> > >>
> > >> I thought you had suggesting making it a pointer. If you made it a
> > >> pointer, you wouldn't need to move the x86_def_t declaration to
> > >> cpu-qom.h.
> > > 
> > > x86_def_t is needed in kvm.c for host class. So there is no much point
> > > in changing info into pointer, considering it's temporary solution.
> > 
> > The main reason I did it this way was to avoid a g_malloc0() in a
> > non-failing class_init. Also it is closer to having class fields sit
> > directly in X86CPUClass.

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


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

Probably, yes.

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



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

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

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

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

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


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

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

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


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

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

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

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

> 
> 2. But more correct way from POV of OOP would be one without any fixups, i.e.
> create extra KVM-builtin-CPU-classes that are derived from host class.
> and in object_class_by_name() lookup for them if kvm is enabled. But we could
> do this as follow-up to #1.
>  
> > precautions and add explanatory comments.
> > 
> > >> If we still want the default vendor to be a static property in the
> > >> class, we can do that if we set the default in a "tcg-vendor" property
> > >> instead of a "vendor" property (that would be empty/unset by default),
> > >> and x86_cpu_initfn() could do this:
> > >>
> > >>     vendor = object_property_get_str(cpu, "vendor");
> > >>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
> > >>     if (vendor && vendor[0]) {
> > >>       cpu->cpuid_vendor = vendor;
> > >>     } else if (kvm_enabled()) {
> > >>       cpu->cpuid_vendor = get_host_vendor();
> > >>     } else {
> > >>       cpu->cpuid_vendor = tcg_vendor;
> > >>     }
> > >>
> > >>> +
> > >>> +    /* Look for specific models that have the QEMU version in .model_id */
> > >>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
> > >>> +        if (strcmp(versioned_models[i], def->name) == 0) {
> > >>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
> > >>> +                    "QEMU Virtual CPU version ");
> > >>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
> > >>> +                    qemu_get_version());
> > >>> +            break;
> > >>> +        }
> > >>> +    }
> > >>> +}
> > >>> +
> > > [...]
> > > 
> > >>> --- a/target-i386/kvm.c
> > >>> +++ b/target-i386/kvm.c
> > >>> @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
> > >>>      return ret;
> > >>>  }
> > >>>  
> > > [...]
> > > 
> > >>>  int kvm_arch_init(KVMState *s)
> > >>>  {
> > >>>      QemuOptsList *list = qemu_find_opts("machine");
> > >>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
> > >>>      int ret;
> > >>>      struct utsname utsname;
> > >>>  
> > >>> +    static const TypeInfo host_x86_cpu_type_info = {
> > >>> +        .name = TYPE_HOST_X86_CPU,
> > >>> +        .parent = TYPE_X86_CPU,
> > >>> +        .class_init = kvm_host_cpu_class_init,
> > >>> +    };
> > >>> +
> > >>>      ret = kvm_get_supported_msrs(s);
> > >>>      if (ret < 0) {
> > >>>          return ret;
> > >>> @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
> > >>>              }
> > >>>          }
> > >>>      }
> > >>> +
> > >>> +    type_register(&host_x86_cpu_type_info);
> > >>
> > >> Are we really allowed to register QOM classes that late?
> > >>
> > >> If QOM design allows us to register the class very late (I would like to
> > >> confirm that), this approach sounds really clean and sane to me.
> > >> Pre-KVM-init class introspection of the "host" class would be completely
> > >> useless anyway (because all data in the "host" class depend on data
> > >> available only post-KVM-init anyway).
> > > 
> > > Looking at type_register() impl. it seems safe to do so + relying on QBL for
> > > type_table_add() safety. So it's really design question for QOM experts.
> > > 
> > > Antnony, Paolo, Andreas
> > >  what do you think?
> > 
> > I already answered Eduardo on IRC that in general I see no restriction
> > not to register a type late.
> > 
> > The issue is that in this case we cannot rely on accessing the class
> > from another class_init that is registered before it, which you were
> > doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
> > route).
> If we are accessing host class somewhere, then we would like to access its
> initialized state, not a dummy state which gives us nothing.

Absolutely.

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


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

-- 
Eduardo

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

[Eduardo Habkost]

On Fri, Feb 08, 2013 at 12:52:31PM -0200, Eduardo Habkost wrote:
> On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
[...]
> > Continuing on theoretical issue:
> > > We could add an inited field to X86CPUClass that gets checked at initfn
> > > time (only ever getting set to true by the pre-defined models). Then it
> > > would be per model. And if we add a prototype for the ..._class_init we
> > > could even call it late as proposed for -cpu host if we take some
> >              ^^^^^^^^^^^^ is a tricky part, for global properties to work it
> > would require, calling this hook after kvm_init() is succeeds and before
> > any initfn() is called in general or as minimum before Device.initfn(). And it
> > anyway will not make all CPU classes to have correct defaults in KVM mode,
> > since only CPU class of created CPU instance will be fixed up.
> > 
> > 1. One way to make sure that built-in CPU classes have fixed up defaults is to
> > iterate over them in kvm_arch_init() and possibly calling their class_init()
> > again to reinitialize. It's still hack (due fixing something up), but it would
> > give at least correct KVM mode defaults, regardless of the order classes are
> > initialized.
> 
> Can't we do that more easily with the tcg-vendor/vendor properties?
> 
> It looks we are burning too much brain cycles trying to force a model
> that's really unintuitive to the outside, where the default-value of a
> class property depends on the options given to the QEMU command-line. We
> don't have to do that.
> 
> The point of initializing stuff in class_init is to make introspection
> easy. If we make the classes change how they look like depending on the
> command-line configuration, the classes and the class introspection
> system get less useful.
> 

Sorry for replying to myself, but extending my answer:

> > 
> > 2. But more correct way from POV of OOP would be one without any fixups, i.e.
> > create extra KVM-builtin-CPU-classes that are derived from host class.
> > and in object_class_by_name() lookup for them if kvm is enabled. But we could
> > do this as follow-up to #1.

Solution #2 would be 100% correct, strictly speaking, but isn't it
overkill to create separate classes if we could just add one additional
property in X86CPUClass, and let the CPU object choose which property is
important for the CPUID setup, depending if KVM is enabled or not?

-- 
Eduardo

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

[Andreas Färber]

Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
>> On Fri, 08 Feb 2013 12:16:17 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
>>>> On Thu, 7 Feb 2013 13:08:19 -0200
>>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>>>
>>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
>>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
>>>>>>      }
>>>>>>  }
>>>>>>  
>>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
>>>>>> +{
>>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
>>>>>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
>>>>>> +    X86CPUClass *hostcc;
>>>>>> +    x86_def_t *def = data;
>>>>>> +    int i;
>>>>>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
>>>>>> +
>>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
>>>>>> +
>>>>>> +    /* host cpu class is available if KVM is enabled,
>>>>>> +     * get kvm overrides from it */
>>>>>> +    if (hoc) {
>>>>>> +        hostcc = X86_CPU_CLASS(hoc);
>>>>>> +        /* sysenter isn't supported in compatibility mode on AMD,
>>>>>> +         * syscall isn't supported in compatibility mode on Intel.
>>>>>> +         * Normally we advertise the actual CPU vendor, but you can
>>>>>> +         * override this using the 'vendor' property if you want to use
>>>>>> +         * KVM's sysenter/syscall emulation in compatibility mode and
>>>>>> +         * when doing cross vendor migration
>>>>>> +         */
>>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
>>>>>> +               sizeof(xcc->info.vendor));
>>>>>> +    }
>>>>>
>>>>> Again, we have the same problem we had before, but now in the non-host
>>>>> classes. What if class_init is called before KVM is initialized? I
>>>>> believe we will be forced to move this hack to the instance init
>>>>> function.
>>>> I believe, the in the case where non-host CPU classes might be initialized
>>>> before KVM "-cpu ?" we do not care what their defaults are, since we only
>>>> would use class names there and then exit.
>>>>
>>>> For case where classes could be inspected over QMP, OQM, KVM would be already
>>>> initialized if enabled and we would get proper initialization order without
>>>> hack.
> 
> Who guarantees that KVM will be already initialized when we get a QMP
> monitor? We can't do that today because of limitations in the QEMU main
> code, but I believe we want to get rid of this limitation eventually,
> instead of making it harder to get rid of.
> 
> If we could initialize KVM before QMP is initialized, we could simply
> initialize KVM before class_init is called, instead. It would be easier
> to reason about, and it would make the limitations of our code very
> clear to anybody reading the code in main().

That wouldn't work (currently) due to -device and -object being command
line options just like -enable-kvm, -disable-kvm and -machine accel=.

>>>
>>> I think you're missing Eduardo's and my point:
>>>
>>> diff --git a/vl.c b/vl.c
>>> index a8dc73d..6b9378e 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
>>>      }
>>>
>>>      module_call_init(MODULE_INIT_QOM);
>>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
>>>
>>>      qemu_add_opts(&qemu_drive_opts);
>>>      qemu_add_opts(&qemu_chardev_opts);
>>>
>>> Anyone may iterate over QOM classes at any time after their type
>>> registration, which is before the first round of option parsing.
>>> Sometime later, after option parsing, there's the -cpu ? handling in
>>> vl.c:3854, then vl.c:4018:configure_accelerator().
>>>
>>> Like I said, mostly a theoretical issue today.
>> Question is if we should drop this theoretical issue for 1.5?
> 
> I wouldn't call it just theoretical. It is something that will surely
> hit us back. The people working on QMP or on the main() code 6 months
> from now will no idea that our class_init code is broken and will
> explode if class_init is called too early.

We should try to find a reliable solution here or at least add
appropriate comments to the module_call_init() call in vl.c.

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

Right, but applying KVM-specific defaults is much easier once KVM is
initialized. :)

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

Then we're pretty much back to my v3 plus Igor's error handling change,
right? Modulo whether to register the host class in kvm_arch_init() or
always.

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

I don't quite understand why we would need a "tcg-vendor" property. Are
you trying to address setting or getting the value? If it's setting we
should just bypass the property in our internal code, using Igor's
vendor_str2words helper.

>>>> Instead of adding hack, I'd rather enforce valid initialization order and
>>>> abort in initfn() if type was initialized without KVM present and KVM is
>>>> enabled at initfn() time. Something along the lines:
>>>>
>>>> static x86_cpu_builtin_class_initialized_without_kvm;
>>>>
>>>> x86_cpu_def_class_init() {
>>>>     ...
>>>>     if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kvm) {
>>>>         x86_cpu_builtin_class_initialized_without_kvm = true;
>>>>     }
>>>>     ...
>>>> }
>>>>
>>>> initfn() {
>>>>     ...
>>>>     if (kvm_enabled() && x86_cpu_builtin_class_initialized_without_kvm) {
>>>>         abort();
>>>>     }
>>>>     ...
>>>> }
>>>
>> Continuing on theoretical issue:
>>> We could add an inited field to X86CPUClass that gets checked at initfn
>>> time (only ever getting set to true by the pre-defined models). Then it
>>> would be per model. And if we add a prototype for the ..._class_init we
>>> could even call it late as proposed for -cpu host if we take some
>>              ^^^^^^^^^^^^ is a tricky part, for global properties to work it
>> would require, calling this hook after kvm_init() is succeeds and before
>> any initfn() is called in general or as minimum before Device.initfn(). And it
>> anyway will not make all CPU classes to have correct defaults in KVM mode,
>> since only CPU class of created CPU instance will be fixed up.
>>
>> 1. One way to make sure that built-in CPU classes have fixed up defaults is to
>> iterate over them in kvm_arch_init() and possibly calling their class_init()
>> again to reinitialize. It's still hack (due fixing something up), but it would
>> give at least correct KVM mode defaults, regardless of the order classes are
>> initialized.
> 
> Can't we do that more easily with the tcg-vendor/vendor properties?

> It looks we are burning too much brain cycles trying to force a model
> that's really unintuitive to the outside, where the default-value of a
> class property depends on the options given to the QEMU command-line. We
> don't have to do that.
>
> The point of initializing stuff in class_init is to make introspection
> easy. If we make the classes change how they look like depending on the
> command-line configuration, the classes and the class introspection
> system get less useful.

+1

>> 2. But more correct way from POV of OOP would be one without any fixups, i.e.
>> create extra KVM-builtin-CPU-classes that are derived from host class.
>> and in object_class_by_name() lookup for them if kvm is enabled. But we could
>> do this as follow-up to #1.
>>  
>>> precautions and add explanatory comments.
>>>
>>>>> If we still want the default vendor to be a static property in the
>>>>> class, we can do that if we set the default in a "tcg-vendor" property
>>>>> instead of a "vendor" property (that would be empty/unset by default),
>>>>> and x86_cpu_initfn() could do this:
>>>>>
>>>>>     vendor = object_property_get_str(cpu, "vendor");
>>>>>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
>>>>>     if (vendor && vendor[0]) {
>>>>>       cpu->cpuid_vendor = vendor;
>>>>>     } else if (kvm_enabled()) {
>>>>>       cpu->cpuid_vendor = get_host_vendor();
>>>>>     } else {
>>>>>       cpu->cpuid_vendor = tcg_vendor;
>>>>>     }
>>>>>
>>>>>> +
>>>>>> +    /* Look for specific models that have the QEMU version in .model_id */
>>>>>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
>>>>>> +        if (strcmp(versioned_models[i], def->name) == 0) {
>>>>>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
>>>>>> +                    "QEMU Virtual CPU version ");
>>>>>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
>>>>>> +                    qemu_get_version());
>>>>>> +            break;
>>>>>> +        }
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>> [...]
>>>>
>>>>>> --- a/target-i386/kvm.c
>>>>>> +++ b/target-i386/kvm.c
>>>>>> @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
>>>>>>      return ret;
>>>>>>  }
>>>>>>  
>>>> [...]
>>>>
>>>>>>  int kvm_arch_init(KVMState *s)
>>>>>>  {
>>>>>>      QemuOptsList *list = qemu_find_opts("machine");
>>>>>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
>>>>>>      int ret;
>>>>>>      struct utsname utsname;
>>>>>>  
>>>>>> +    static const TypeInfo host_x86_cpu_type_info = {
>>>>>> +        .name = TYPE_HOST_X86_CPU,
>>>>>> +        .parent = TYPE_X86_CPU,
>>>>>> +        .class_init = kvm_host_cpu_class_init,
>>>>>> +    };
>>>>>> +
>>>>>>      ret = kvm_get_supported_msrs(s);
>>>>>>      if (ret < 0) {
>>>>>>          return ret;
>>>>>> @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
>>>>>>              }
>>>>>>          }
>>>>>>      }
>>>>>> +
>>>>>> +    type_register(&host_x86_cpu_type_info);
>>>>>
>>>>> Are we really allowed to register QOM classes that late?
>>>>>
>>>>> If QOM design allows us to register the class very late (I would like to
>>>>> confirm that), this approach sounds really clean and sane to me.
>>>>> Pre-KVM-init class introspection of the "host" class would be completely
>>>>> useless anyway (because all data in the "host" class depend on data
>>>>> available only post-KVM-init anyway).
>>>>
>>>> Looking at type_register() impl. it seems safe to do so + relying on QBL for
>>>> type_table_add() safety. So it's really design question for QOM experts.
>>>>
>>>> Antnony, Paolo, Andreas
>>>>  what do you think?
>>>
>>> I already answered Eduardo on IRC that in general I see no restriction
>>> not to register a type late.
>>>
>>> The issue is that in this case we cannot rely on accessing the class
>>> from another class_init that is registered before it, which you were
>>> doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
>>> route).
>> If we are accessing host class somewhere, then we would like to access its
>> initialized state, not a dummy state which gives us nothing.

No one is doubting that. It in turn means that either class_init may not
fail/skip parts, or we need to restrict access to such classes to a
mechanism that ensures they get fully initialized if they weren't.

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

I've been thinking about whether this is a more general issue that we
could solve at QOM level, like the base_class_init for qdev props, but I
haven't come up with a usable idea. (Paolo?)

I agree with Eduardo that we should not try to override class values
based on accel=kvm. Global properties don't operate on classes but on
the properties, which get set up at device-instance_init time only.
If there's an issue with the vendor it seems easier to fix that than to
play games with class_init as we seem to be going in circles...

That would leave us with the problematic -cpu host class and with
analyzing any remaining instance_init problems.

And not to forget -object and, once Anthony drops his unloved no_user
flag, -device.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Eduardo Habkost]

On Fri, Feb 08, 2013 at 05:54:50PM +0100, Andreas Färber wrote:
> Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> > On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> >> On Fri, 08 Feb 2013 12:16:17 +0100
> >> Andreas Färber <afaerber@suse.de> wrote:
> >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>>>
> >>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> >>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> >>>>>>      }
> >>>>>>  }
> >>>>>>  
> >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> >>>>>> +{
> >>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> >>>>>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
> >>>>>> +    X86CPUClass *hostcc;
> >>>>>> +    x86_def_t *def = data;
> >>>>>> +    int i;
> >>>>>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
> >>>>>> +
> >>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> >>>>>> +
> >>>>>> +    /* host cpu class is available if KVM is enabled,
> >>>>>> +     * get kvm overrides from it */
> >>>>>> +    if (hoc) {
> >>>>>> +        hostcc = X86_CPU_CLASS(hoc);
> >>>>>> +        /* sysenter isn't supported in compatibility mode on AMD,
> >>>>>> +         * syscall isn't supported in compatibility mode on Intel.
> >>>>>> +         * Normally we advertise the actual CPU vendor, but you can
> >>>>>> +         * override this using the 'vendor' property if you want to use
> >>>>>> +         * KVM's sysenter/syscall emulation in compatibility mode and
> >>>>>> +         * when doing cross vendor migration
> >>>>>> +         */
> >>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> >>>>>> +               sizeof(xcc->info.vendor));
> >>>>>> +    }
> >>>>>
> >>>>> Again, we have the same problem we had before, but now in the non-host
> >>>>> classes. What if class_init is called before KVM is initialized? I
> >>>>> believe we will be forced to move this hack to the instance init
> >>>>> function.
> >>>> I believe, the in the case where non-host CPU classes might be initialized
> >>>> before KVM "-cpu ?" we do not care what their defaults are, since we only
> >>>> would use class names there and then exit.
> >>>>
> >>>> For case where classes could be inspected over QMP, OQM, KVM would be already
> >>>> initialized if enabled and we would get proper initialization order without
> >>>> hack.
> > 
> > Who guarantees that KVM will be already initialized when we get a QMP
> > monitor? We can't do that today because of limitations in the QEMU main
> > code, but I believe we want to get rid of this limitation eventually,
> > instead of making it harder to get rid of.
> > 
> > If we could initialize KVM before QMP is initialized, we could simply
> > initialize KVM before class_init is called, instead. It would be easier
> > to reason about, and it would make the limitations of our code very
> > clear to anybody reading the code in main().
> 
> That wouldn't work (currently) due to -device and -object being command
> line options just like -enable-kvm, -disable-kvm and -machine accel=.

Well, we could loop over the command-line options twice.

It is just an alternative that would be better than making class_init
unreliable. I don't think it would be a great solution anyway.

> 
> >>>
> >>> I think you're missing Eduardo's and my point:
> >>>
> >>> diff --git a/vl.c b/vl.c
> >>> index a8dc73d..6b9378e 100644
> >>> --- a/vl.c
> >>> +++ b/vl.c
> >>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> >>>      }
> >>>
> >>>      module_call_init(MODULE_INIT_QOM);
> >>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> >>>
> >>>      qemu_add_opts(&qemu_drive_opts);
> >>>      qemu_add_opts(&qemu_chardev_opts);
> >>>
> >>> Anyone may iterate over QOM classes at any time after their type
> >>> registration, which is before the first round of option parsing.
> >>> Sometime later, after option parsing, there's the -cpu ? handling in
> >>> vl.c:3854, then vl.c:4018:configure_accelerator().
> >>>
> >>> Like I said, mostly a theoretical issue today.
> >> Question is if we should drop this theoretical issue for 1.5?
> > 
> > I wouldn't call it just theoretical. It is something that will surely
> > hit us back. The people working on QMP or on the main() code 6 months
> > from now will no idea that our class_init code is broken and will
> > explode if class_init is called too early.
> 
> We should try to find a reliable solution here or at least add
> appropriate comments to the module_call_init() call in vl.c.

Agreed. But I believe there are tons of other solutions that don't
require making class_init broken.


> 
> >>> Originally I had considered making kvm_init() re-entrant and calling it
> >>> from the offending class_init. But we must support the distro case of
> >>> compiling with CONFIG_KVM but the user's hardware not supporting KVM or
> >>> kvm module not being loaded or the user having insufficient priviledges
> >>> to access /dev/kvm.
> >> working without KVM is not issue, it just works with normal defaults. Applying
> >> KVM specific defaults to already initialized classes is.
> 
> Right, but applying KVM-specific defaults is much easier once KVM is
> initialized. :)
> 
> > 
> > My big question is: why exactly we want to initialize this stuff inside
> > class_init? Can't we (please!) put the KVM-specific logic inside
> > instance_init?
> 
> Then we're pretty much back to my v3 plus Igor's error handling change,
> right? Modulo whether to register the host class in kvm_arch_init() or
> always.

I don't know, I would need to take a look at your v3. I don't remember
how many of the code in this version was already in v3.

> 
> > If "default vendor set in in built-in CPU model table" (TCG-only) has a
> > different meaning from "vendor set by command-line/global-property"
> > (affects TCG and KVM), it means we have two different knobs with two
> > diferent semantics. Hence my suggestion of adding two properties:
> > "tcg-vendor" and "vendor".
> 
> I don't quite understand why we would need a "tcg-vendor" property. Are
> you trying to address setting or getting the value? If it's setting we
> should just bypass the property in our internal code, using Igor's
> vendor_str2words helper.

Just for setting (more exactly, just for carrying the TCG-only default
values on the built-in classes, that are currently inside the x86_def_t
table). The tricky part here is that we need to differentiate the
default vendor value from class_init/x86_def_t that is valid only for
TCG (on KVM, the default vendor is always the host CPU vendor), and
"vendor" property set using global-properties/command-line is different
and should be used on both TCG and KVM modes.



> 
[...]
> I've been thinking about whether this is a more general issue that we
> could solve at QOM level, like the base_class_init for qdev props, but I
> haven't come up with a usable idea. (Paolo?)

I don't see any simple solution involving extending the QOM design,
except that we need to decide on a general dependency/layering model and
stick with it instead of trying to break the model without admitting
we're breaking it.

I believe the current dependency chain is:

 - class data initialization
    -> handling of configuration options
       -> accelerator initialization
          -> machine and CPU instances initialization

That means:

 - Handling of configuration options (via command-line, QMP, config
   file, whatever method) need classes to be initialized first.
 - Accelerator initialization depends on config options to be handled
   first.
 - Hence, we can't make class data depend on accelerator initialization.
   Period.


What we _could_ do (but I don't think is the best solution) is:

 - handling of accelerator options
   -> accelerator initialization
      -> class data
         -> handling of remaining configuration options
            -> machine and CPU instances initialization

Such a change would be introsive, so I believe we can try to fix it by
simply changing our CPU class model so that class data don't depend on
accelerator data[1].

An intermediate solution could be registering and initializing all the
X86CPU classes later. e.g.:

 - General class data initialization
   -> handling of configuration options
      -> accelerator initialization
         -> X86CPU class data initialization
            -> machine and CPU instances initialization

The problem with this approach is that it would be impossible to
implement "-cpu ?" and "query-cpu-definitions" without at least handling
the accelerator configuration options first. This would probably break
(or require hacks for) "-device" too.

The latter is already the solution we are trying for "-cpu host", but at
least we know that "-cpu host" is special and we can work around it in
the query-cpu-definitions and "-cpu ?" code[2].


[1] My main point is: we're trying to force accelerator-provided data to
    be stored in the class, when we really don't need that data to be
    stored in the class. We just need property-value semantics that make
    the object use the accelerator-provided data later, but without
    storing the accelerator-provided data itself inside the class.

[2] It is possible to fix the problem with "-cpu host" later if we
    decide on property/value semantics that allow the "the CPU will
    simply copy features from the host when initialized" mode to be
    expressed in the class (without requiring KVM to be initialized
    first).

    A possible solution is having a "copy-all-host-features=yes"
    property in the -cpu host class. But that would probably break "-cpu
    host,+foo,-bar".

    Another solution would be to make the "f-*" flags tristate,
    accepting "on", "off" and "host". The host class could then simply
    set the default for all feature properties to "host", and the
    instance init function (not class_init) would understand that "host"
    means "ask KVM for host features".


> 
> I agree with Eduardo that we should not try to override class values
> based on accel=kvm. Global properties don't operate on classes but on
> the properties, which get set up at device-instance_init time only.
> If there's an issue with the vendor it seems easier to fix that than to
> play games with class_init as we seem to be going in circles...
> 
> That would leave us with the problematic -cpu host class and with
> analyzing any remaining instance_init problems.
> 
> And not to forget -object and, once Anthony drops his unloved no_user
> flag, -device.
> 
> Regards,
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

-- 
Eduardo

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

[Igor Mammedov]

On Fri, 8 Feb 2013 16:13:02 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Feb 08, 2013 at 05:54:50PM +0100, Andreas Färber wrote:
> > Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> > > On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> > >> On Fri, 08 Feb 2013 12:16:17 +0100
> > >> Andreas Färber <afaerber@suse.de> wrote:
> > >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > >>>>
> > >>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> > >>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> > >>>>>>      }
> > >>>>>>  }
> > >>>>>>  
> > >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> > >>>>>> +{
> > >>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > >>>>>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
> > >>>>>> +    X86CPUClass *hostcc;
> > >>>>>> +    x86_def_t *def = data;
> > >>>>>> +    int i;
> > >>>>>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
> > >>>>>> +
> > >>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > >>>>>> +
> > >>>>>> +    /* host cpu class is available if KVM is enabled,
> > >>>>>> +     * get kvm overrides from it */
> > >>>>>> +    if (hoc) {
> > >>>>>> +        hostcc = X86_CPU_CLASS(hoc);
> > >>>>>> +        /* sysenter isn't supported in compatibility mode on AMD,
> > >>>>>> +         * syscall isn't supported in compatibility mode on Intel.
> > >>>>>> +         * Normally we advertise the actual CPU vendor, but you can
> > >>>>>> +         * override this using the 'vendor' property if you want to use
> > >>>>>> +         * KVM's sysenter/syscall emulation in compatibility mode and
> > >>>>>> +         * when doing cross vendor migration
> > >>>>>> +         */
> > >>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> > >>>>>> +               sizeof(xcc->info.vendor));
> > >>>>>> +    }
> > >>>>>
> > >>>>> Again, we have the same problem we had before, but now in the non-host
> > >>>>> classes. What if class_init is called before KVM is initialized? I
> > >>>>> believe we will be forced to move this hack to the instance init
> > >>>>> function.
> > >>>> I believe, the in the case where non-host CPU classes might be initialized
> > >>>> before KVM "-cpu ?" we do not care what their defaults are, since we only
> > >>>> would use class names there and then exit.
> > >>>>
> > >>>> For case where classes could be inspected over QMP, OQM, KVM would be already
> > >>>> initialized if enabled and we would get proper initialization order without
> > >>>> hack.
> > > 
> > > Who guarantees that KVM will be already initialized when we get a QMP
> > > monitor? We can't do that today because of limitations in the QEMU main
> > > code, but I believe we want to get rid of this limitation eventually,
> > > instead of making it harder to get rid of.
> > > 
> > > If we could initialize KVM before QMP is initialized, we could simply
> > > initialize KVM before class_init is called, instead. It would be easier
> > > to reason about, and it would make the limitations of our code very
> > > clear to anybody reading the code in main().
> > That wouldn't work (currently) due to -device and -object being command
> > line options just like -enable-kvm, -disable-kvm and -machine accel=.
> 
> Well, we could loop over the command-line options twice.
> 
> It is just an alternative that would be better than making class_init
> unreliable. I don't think it would be a great solution anyway.
> 
> > 
> > >>>
> > >>> I think you're missing Eduardo's and my point:
> > >>>
> > >>> diff --git a/vl.c b/vl.c
> > >>> index a8dc73d..6b9378e 100644
> > >>> --- a/vl.c
> > >>> +++ b/vl.c
> > >>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> > >>>      }
> > >>>
> > >>>      module_call_init(MODULE_INIT_QOM);
> > >>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> > >>>
> > >>>      qemu_add_opts(&qemu_drive_opts);
> > >>>      qemu_add_opts(&qemu_chardev_opts);
> > >>>
> > >>> Anyone may iterate over QOM classes at any time after their type
> > >>> registration, which is before the first round of option parsing.
> > >>> Sometime later, after option parsing, there's the -cpu ? handling in
> > >>> vl.c:3854, then vl.c:4018:configure_accelerator().
> > >>>
> > >>> Like I said, mostly a theoretical issue today.
> > >> Question is if we should drop this theoretical issue for 1.5?
> > > 
> > > I wouldn't call it just theoretical. It is something that will surely
> > > hit us back. The people working on QMP or on the main() code 6 months
> > > from now will no idea that our class_init code is broken and will
> > > explode if class_init is called too early.
> > 
> > We should try to find a reliable solution here or at least add
> > appropriate comments to the module_call_init() call in vl.c.
> 
> Agreed. But I believe there are tons of other solutions that don't
> require making class_init broken.
> 
> 
> > 
> > >>> Originally I had considered making kvm_init() re-entrant and calling it
> > >>> from the offending class_init. But we must support the distro case of
> > >>> compiling with CONFIG_KVM but the user's hardware not supporting KVM or
> > >>> kvm module not being loaded or the user having insufficient priviledges
> > >>> to access /dev/kvm.
> > >> working without KVM is not issue, it just works with normal defaults. Applying
> > >> KVM specific defaults to already initialized classes is.
> > 
> > Right, but applying KVM-specific defaults is much easier once KVM is
> > initialized. :)
> > 
> > > 
> > > My big question is: why exactly we want to initialize this stuff inside
> > > class_init? Can't we (please!) put the KVM-specific logic inside
> > > instance_init?
I see 2 issues with it:
1. a rather abstract introspection. defaults are belong to class data and
   user who introspected class would expect to get CPU he saw during it, which
   he won't get if instance_init() will set another defaults, It will be
   already another CPU. So introspection becomes useless here.

2. more close to code:
    * vendor property, you offering to add a new tcg-vendor property. If we
      are dumping goal [1] then it might work. But that new property is just
      another reincarnation of vendor_override field in CPUState that we've
      just gotten rid of and brings back hack of selecting what guest os will see.
    * cpuid_kvm_features defaults also different for KVM and TCG. Which also
      makes 2 different CPUs, and makes guest behave differently.
      If we set defaults in instance_init(), we would loose possibility to use
      global(cmd line) and compat(pv_eoi) properties with respective
      feature bits or invent another special case to detect that
      global/compat properties were used and workaround it.
    * that pile of special cases will only grow with time, adding likes
      disable_pv_eoi() hooks and who knows what else.

To use benefits provided by static properties we need to follow rule that
defaults are not set in instance_init(), instead of they should be set using
defaults of static properties. Unconditional calls to set them in this patch
could be eventually converted to class_init() defaults. But kvm_enabled()
conditioned ones like vendor and cpuid_kvm_features, rise chicken or egg
problem we are trying to solve.

From reading above I've got that fixing up built-in CPUs class data is not
perfect idea and might not work after all.
Then lets consider alternative of creating a bunch of KVM-based built-in CPU
sub-classes that are added at kvm_init() time.


> > 
> > Then we're pretty much back to my v3 plus Igor's error handling change,
> > right? Modulo whether to register the host class in kvm_arch_init() or
> > always.
> 
> I don't know, I would need to take a look at your v3. I don't remember
> how many of the code in this version was already in v3.
> 
> > 
> > > If "default vendor set in in built-in CPU model table" (TCG-only) has a
> > > different meaning from "vendor set by command-line/global-property"
> > > (affects TCG and KVM), it means we have two different knobs with two
> > > diferent semantics. Hence my suggestion of adding two properties:
> > > "tcg-vendor" and "vendor".
> > 
> > I don't quite understand why we would need a "tcg-vendor" property. Are
> > you trying to address setting or getting the value? If it's setting we
> > should just bypass the property in our internal code, using Igor's
> > vendor_str2words helper.
> 
> Just for setting (more exactly, just for carrying the TCG-only default
> values on the built-in classes, that are currently inside the x86_def_t
> table). The tricky part here is that we need to differentiate the
> default vendor value from class_init/x86_def_t that is valid only for
> TCG (on KVM, the default vendor is always the host CPU vendor), and
> "vendor" property set using global-properties/command-line is different
> and should be used on both TCG and KVM modes.
> 
> 
> 
> > 
> [...]
> > I've been thinking about whether this is a more general issue that we
> > could solve at QOM level, like the base_class_init for qdev props, but I
> > haven't come up with a usable idea. (Paolo?)
> 
> I don't see any simple solution involving extending the QOM design,
> except that we need to decide on a general dependency/layering model and
> stick with it instead of trying to break the model without admitting
> we're breaking it.
> 
> I believe the current dependency chain is:
> 
>  - class data initialization
>     -> handling of configuration options
>        -> accelerator initialization
>           -> machine and CPU instances initialization
> 
> That means:
> 
>  - Handling of configuration options (via command-line, QMP, config
>    file, whatever method) need classes to be initialized first.
>  - Accelerator initialization depends on config options to be handled
>    first.
>  - Hence, we can't make class data depend on accelerator initialization.
>    Period.
> 
> 
> What we _could_ do (but I don't think is the best solution) is:
> 
>  - handling of accelerator options
>    -> accelerator initialization
>       -> class data
>          -> handling of remaining configuration options
>             -> machine and CPU instances initialization
> 
> Such a change would be introsive, so I believe we can try to fix it by
> simply changing our CPU class model so that class data don't depend on
> accelerator data[1].
> 
> An intermediate solution could be registering and initializing all the
> X86CPU classes later. e.g.:
> 
>  - General class data initialization
>    -> handling of configuration options
>       -> accelerator initialization
>          -> X86CPU class data initialization
>             -> machine and CPU instances initialization
+1 if KVM based sub-classes idea is not acceptable, this could work for me
as compromise provided defaults are initialized in class_init()

> 
> The problem with this approach is that it would be impossible to
> implement "-cpu ?" and "query-cpu-definitions" without at least handling
"-cpu ?" can be moved after kvm_init() and we could put comment that
QMP should be accessed after it.

> the accelerator configuration options first. This would probably break
> (or require hacks for) "-device" too.
Why break? -device cpu doesn't work now and I'm not sure it should/would in
oversee-able future.

> 
> The latter is already the solution we are trying for "-cpu host", but at
> least we know that "-cpu host" is special and we can work around it in
> the query-cpu-definitions and "-cpu ?" code[2].
> 
> 
> [1] My main point is: we're trying to force accelerator-provided data to
>     be stored in the class, when we really don't need that data to be
>     stored in the class. We just need property-value semantics that make
>     the object use the accelerator-provided data later, but without
>     storing the accelerator-provided data itself inside the class.
that complicates property handling a lot to a point of not able to use
global/compat properties. Setting defaults in class_init() using static
property defaults doesn't have such limitations + keeps property semantics
simple and working the same way regardless accelerator. Extra data in form of
class_init()s looks more preferable than complicated code/semantics.

> 
> [2] It is possible to fix the problem with "-cpu host" later if we
>     decide on property/value semantics that allow the "the CPU will
>     simply copy features from the host when initialized" mode to be
>     expressed in the class (without requiring KVM to be initialized
>     first).
> 
>     A possible solution is having a "copy-all-host-features=yes"
>     property in the -cpu host class. But that would probably break "-cpu
>     host,+foo,-bar".
> 
>     Another solution would be to make the "f-*" flags tristate,
>     accepting "on", "off" and "host". The host class could then simply
>     set the default for all feature properties to "host", and the
>     instance init function (not class_init) would understand that "host"
>     means "ask KVM for host features".
> 
> 
> > 
> > I agree with Eduardo that we should not try to override class values
> > based on accel=kvm. Global properties don't operate on classes but on
> > the properties, which get set up at device-instance_init time only.
> > If there's an issue with the vendor it seems easier to fix that than to
> > play games with class_init as we seem to be going in circles...
properties definitions are part of classes (including properties defaults),
and defaults are set before global properties at the same place. It's not only
vendor, setting default cpuid_kvm_features in x86_cpu_initfn() will
overwrite anything set before via global properties (i.e. +foo,-foo).

> > 
> > That would leave us with the problematic -cpu host class and with
> > analyzing any remaining instance_init problems.
Than lets double built-in CPUs with KVM bases sub-classes. That well give us
constant classes that reliably describe KVM specific and reduce amount of
kvm_enabled() conditions in initfn(), hence later allow to replace setting
defaults with static properties (making initfn() even more simpler).

> > 
> > And not to forget -object and, once Anthony drops his unloved no_user
> > flag, -device.
And KVM based sub-classes won't break it, they actually wouldn't be available
till kvm_init() is completed successfully.
I understand -object and -device are more like API calls that couldn't be
called arbitrarily, one has to provide dependencies first. So something like
-object kvm should go before -object kvm-cpu if caller expects it to work.

> > 
> > Regards,
> > Andreas
> > 
> > -- 
> > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> 
> -- 
> Eduardo


-- 
Regards,
  Igor

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

[Eduardo Habkost]

On Mon, Feb 11, 2013 at 02:52:49AM +0100, Igor Mammedov wrote:
> On Fri, 8 Feb 2013 16:13:02 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Feb 08, 2013 at 05:54:50PM +0100, Andreas Färber wrote:
> > > Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> > > > On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> > > >> On Fri, 08 Feb 2013 12:16:17 +0100
> > > >> Andreas Färber <afaerber@suse.de> wrote:
> > > >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > > >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> > > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > >>>>
> > > >>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> > > >>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> > > >>>>>>      }
> > > >>>>>>  }
> > > >>>>>>  
> > > >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data)
> > > >>>>>> +{
> > > >>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > >>>>>> +    ObjectClass *hoc = object_class_by_name(TYPE_HOST_X86_CPU);
> > > >>>>>> +    X86CPUClass *hostcc;
> > > >>>>>> +    x86_def_t *def = data;
> > > >>>>>> +    int i;
> > > >>>>>> +    static const char *versioned_models[] = { "qemu32", "qemu64", "athlon" };
> > > >>>>>> +
> > > >>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > > >>>>>> +
> > > >>>>>> +    /* host cpu class is available if KVM is enabled,
> > > >>>>>> +     * get kvm overrides from it */
> > > >>>>>> +    if (hoc) {
> > > >>>>>> +        hostcc = X86_CPU_CLASS(hoc);
> > > >>>>>> +        /* sysenter isn't supported in compatibility mode on AMD,
> > > >>>>>> +         * syscall isn't supported in compatibility mode on Intel.
> > > >>>>>> +         * Normally we advertise the actual CPU vendor, but you can
> > > >>>>>> +         * override this using the 'vendor' property if you want to use
> > > >>>>>> +         * KVM's sysenter/syscall emulation in compatibility mode and
> > > >>>>>> +         * when doing cross vendor migration
> > > >>>>>> +         */
> > > >>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> > > >>>>>> +               sizeof(xcc->info.vendor));
> > > >>>>>> +    }
> > > >>>>>
> > > >>>>> Again, we have the same problem we had before, but now in the non-host
> > > >>>>> classes. What if class_init is called before KVM is initialized? I
> > > >>>>> believe we will be forced to move this hack to the instance init
> > > >>>>> function.
> > > >>>> I believe, the in the case where non-host CPU classes might be initialized
> > > >>>> before KVM "-cpu ?" we do not care what their defaults are, since we only
> > > >>>> would use class names there and then exit.
> > > >>>>
> > > >>>> For case where classes could be inspected over QMP, OQM, KVM would be already
> > > >>>> initialized if enabled and we would get proper initialization order without
> > > >>>> hack.
> > > > 
> > > > Who guarantees that KVM will be already initialized when we get a QMP
> > > > monitor? We can't do that today because of limitations in the QEMU main
> > > > code, but I believe we want to get rid of this limitation eventually,
> > > > instead of making it harder to get rid of.
> > > > 
> > > > If we could initialize KVM before QMP is initialized, we could simply
> > > > initialize KVM before class_init is called, instead. It would be easier
> > > > to reason about, and it would make the limitations of our code very
> > > > clear to anybody reading the code in main().
> > > That wouldn't work (currently) due to -device and -object being command
> > > line options just like -enable-kvm, -disable-kvm and -machine accel=.
> > 
> > Well, we could loop over the command-line options twice.
> > 
> > It is just an alternative that would be better than making class_init
> > unreliable. I don't think it would be a great solution anyway.
> > 
> > > 
> > > >>>
> > > >>> I think you're missing Eduardo's and my point:
> > > >>>
> > > >>> diff --git a/vl.c b/vl.c
> > > >>> index a8dc73d..6b9378e 100644
> > > >>> --- a/vl.c
> > > >>> +++ b/vl.c
> > > >>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> > > >>>      }
> > > >>>
> > > >>>      module_call_init(MODULE_INIT_QOM);
> > > >>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> > > >>>
> > > >>>      qemu_add_opts(&qemu_drive_opts);
> > > >>>      qemu_add_opts(&qemu_chardev_opts);
> > > >>>
> > > >>> Anyone may iterate over QOM classes at any time after their type
> > > >>> registration, which is before the first round of option parsing.
> > > >>> Sometime later, after option parsing, there's the -cpu ? handling in
> > > >>> vl.c:3854, then vl.c:4018:configure_accelerator().
> > > >>>
> > > >>> Like I said, mostly a theoretical issue today.
> > > >> Question is if we should drop this theoretical issue for 1.5?
> > > > 
> > > > I wouldn't call it just theoretical. It is something that will surely
> > > > hit us back. The people working on QMP or on the main() code 6 months
> > > > from now will no idea that our class_init code is broken and will
> > > > explode if class_init is called too early.
> > > 
> > > We should try to find a reliable solution here or at least add
> > > appropriate comments to the module_call_init() call in vl.c.
> > 
> > Agreed. But I believe there are tons of other solutions that don't
> > require making class_init broken.
> > 
> > 
> > > 
> > > >>> Originally I had considered making kvm_init() re-entrant and calling it
> > > >>> from the offending class_init. But we must support the distro case of
> > > >>> compiling with CONFIG_KVM but the user's hardware not supporting KVM or
> > > >>> kvm module not being loaded or the user having insufficient priviledges
> > > >>> to access /dev/kvm.
> > > >> working without KVM is not issue, it just works with normal defaults. Applying
> > > >> KVM specific defaults to already initialized classes is.
> > > 
> > > Right, but applying KVM-specific defaults is much easier once KVM is
> > > initialized. :)
> > > 
> > > > 
> > > > My big question is: why exactly we want to initialize this stuff inside
> > > > class_init? Can't we (please!) put the KVM-specific logic inside
> > > > instance_init?
> I see 2 issues with it:
> 1. a rather abstract introspection. defaults are belong to class data and
>    user who introspected class would expect to get CPU he saw during it, which
>    he won't get if instance_init() will set another defaults, It will be
>    already another CPU. So introspection becomes useless here.

Really, it does not become useless, it has very clear semantics. The
difference is that now the defaults won't depend on the kvm=on/off
configuration.

I believe strongly that one of the purposes of class-based introspection
is to have introspectable _static_ data that do not depend on any
configuration data.

> 
> 2. more close to code:
>     * vendor property, you offering to add a new tcg-vendor property. If we
>       are dumping goal [1] then it might work.

We're now dumping goal [1], we would be enabling it to be achieved,
because the default values of "vendor" and "tcg-vendor" would be
completely static.

The assumption you seem to be unwilling to drop is that cpuid_vendor
should always unconditionally correspond to the value of the "vendor"
property set by the class. But it doesn't have to. The CPU class may
choose to do anything with cpuid_vendor on instance_init, including
using the "tcg-vendor" property to set it if KVM is disabled and
"vendor" is empty.


> But that new property is just
>       another reincarnation of vendor_override field in CPUState that we've
>       just gotten rid of and brings back hack of selecting what guest os will see.

In a way, yes. But it looks like it is necessary. But at least is a very
understandable model that can be seen from the outside. "tcg-vendor" is
obviously tcg-only, and "vendor" overrides everything if set.


>     * cpuid_kvm_features defaults also different for KVM and TCG. Which also
>       makes 2 different CPUs, and makes guest behave differently.
>       If we set defaults in instance_init(), we would loose possibility to use
>       global(cmd line) and compat(pv_eoi) properties with respective
>       feature bits or invent another special case to detect that
>       global/compat properties were used and workaround it.

I don't propose setting property defaults on instance_init. I propose
using the _static_ defaults set by class_init to initialize the CPU
state on instance_init. Just like nobody said that the CPU _must_
initialize cpuid_vendor using the "vendor" property only, nobody said
that the CPU can't set cpuid_kvm_features to zero on instance_init if
KVM is disabled.

You seem to be confusing "how the CPU object struct fields will look
like just after instance_init is called" with "property defaults". They
are different things.


>     * that pile of special cases will only grow with time, adding likes
>       disable_pv_eoi() hooks and who knows what else.

I don't see how disable_pv_eoi() is related to that. disable_pv_eoi()
can easily be converted to global properties once we implement feature
properties. instance_init can then simply zero all KVM features on
instance_init if KVM is disabled.

> 
> To use benefits provided by static properties we need to follow rule that
> defaults are not set in instance_init(), instead of they should be set using
> defaults of static properties. Unconditional calls to set them in this patch
> could be eventually converted to class_init() defaults. But kvm_enabled()
> conditioned ones like vendor and cpuid_kvm_features, rise chicken or egg
> problem we are trying to solve.

We _are_ setting defaults on class_init, the difference is that
instance_init choose which property to obey ("vendor" or "tcg-vendor")
depending on configuration. Doing that would _fix_ global properties,
not break it (because now we can change the tcg-only default-vendor of a
CPU model using global properties easily. See my "486" example/question
below).

> 
> From reading above I've got that fixing up built-in CPUs class data is not
> perfect idea and might not work after all.
> Then lets consider alternative of creating a bunch of KVM-based built-in CPU
> sub-classes that are added at kvm_init() time.

Separate KVM-based subclasses would work, too, but it looks like
overkill if we can simply have two properties.


> 
> 
> > > 
> > > Then we're pretty much back to my v3 plus Igor's error handling change,
> > > right? Modulo whether to register the host class in kvm_arch_init() or
> > > always.
> > 
> > I don't know, I would need to take a look at your v3. I don't remember
> > how many of the code in this version was already in v3.
> > 
> > > 
> > > > If "default vendor set in in built-in CPU model table" (TCG-only) has a
> > > > different meaning from "vendor set by command-line/global-property"
> > > > (affects TCG and KVM), it means we have two different knobs with two
> > > > diferent semantics. Hence my suggestion of adding two properties:
> > > > "tcg-vendor" and "vendor".
> > > 
> > > I don't quite understand why we would need a "tcg-vendor" property. Are
> > > you trying to address setting or getting the value? If it's setting we
> > > should just bypass the property in our internal code, using Igor's
> > > vendor_str2words helper.
> > 
> > Just for setting (more exactly, just for carrying the TCG-only default
> > values on the built-in classes, that are currently inside the x86_def_t
> > table). The tricky part here is that we need to differentiate the
> > default vendor value from class_init/x86_def_t that is valid only for
> > TCG (on KVM, the default vendor is always the host CPU vendor), and
> > "vendor" property set using global-properties/command-line is different
> > and should be used on both TCG and KVM modes.
> > 
> > 
> > 
> > > 
> > [...]
> > > I've been thinking about whether this is a more general issue that we
> > > could solve at QOM level, like the base_class_init for qdev props, but I
> > > haven't come up with a usable idea. (Paolo?)
> > 
> > I don't see any simple solution involving extending the QOM design,
> > except that we need to decide on a general dependency/layering model and
> > stick with it instead of trying to break the model without admitting
> > we're breaking it.
> > 
> > I believe the current dependency chain is:
> > 
> >  - class data initialization
> >     -> handling of configuration options
> >        -> accelerator initialization
> >           -> machine and CPU instances initialization
> > 
> > That means:
> > 
> >  - Handling of configuration options (via command-line, QMP, config
> >    file, whatever method) need classes to be initialized first.
> >  - Accelerator initialization depends on config options to be handled
> >    first.
> >  - Hence, we can't make class data depend on accelerator initialization.
> >    Period.
> > 
> > 
> > What we _could_ do (but I don't think is the best solution) is:
> > 
> >  - handling of accelerator options
> >    -> accelerator initialization
> >       -> class data
> >          -> handling of remaining configuration options
> >             -> machine and CPU instances initialization
> > 
> > Such a change would be introsive, so I believe we can try to fix it by
> > simply changing our CPU class model so that class data don't depend on
> > accelerator data[1].
> > 
> > An intermediate solution could be registering and initializing all the
> > X86CPU classes later. e.g.:
> > 
> >  - General class data initialization
> >    -> handling of configuration options
> >       -> accelerator initialization
> >          -> X86CPU class data initialization
> >             -> machine and CPU instances initialization
> +1 if KVM based sub-classes idea is not acceptable, this could work for me
> as compromise provided defaults are initialized in class_init()
> 
> > 
> > The problem with this approach is that it would be impossible to
> > implement "-cpu ?" and "query-cpu-definitions" without at least handling
> "-cpu ?" can be moved after kvm_init() and we could put comment that
> QMP should be accessed after it.

No, we can't. QMP should be able to handle the "handling of
configuration options" part. And "-cpu ?"/query-cpu-definitions doesn't
require the KVM option to be set.

> 
> > the accelerator configuration options first. This would probably break
> > (or require hacks for) "-device" too.
> Why break? -device cpu doesn't work now and I'm not sure it should/would in
> oversee-able future.

Well, it doesn't work today, but we plan to make it work, right? So we
must choose a design that allows us to make it work instead of making
our life difficult in the near future. This solution has "handling of
configuration options" before "X86CPU class data initialization", so it
will prevent us from making -device work with CPUs.

We are making huge efforts to make CPUs devices like any other, I
believe we should avoid introducing _additional_ exceptions and
special-cases for the CPU devices.

> 
> > 
> > The latter is already the solution we are trying for "-cpu host", but at
> > least we know that "-cpu host" is special and we can work around it in
> > the query-cpu-definitions and "-cpu ?" code[2].
> > 
> > 
> > [1] My main point is: we're trying to force accelerator-provided data to
> >     be stored in the class, when we really don't need that data to be
> >     stored in the class. We just need property-value semantics that make
> >     the object use the accelerator-provided data later, but without
> >     storing the accelerator-provided data itself inside the class.
> that complicates property handling a lot to a point of not able to use
> global/compat properties.

I believe it _simplifies_ property handling a lot and allows us to
easily use global/compat properties even if we want a KVM-only
global-property.

e.g. assume we made a mistake and we have an Intel CPU (e.g. 486) with
vendor=AMD in the builtin CPU list. With your solution, how would you
set global properties to fix this and make 486 have vendor=Intel without
affecting KVM at all?

With my solution it is simple:

 { "486-x86_64-cpu", "tcg-vendor", "GenuineIntel" }

I don't see how you could do that with the current patch.


> Setting defaults in class_init() using static
> property defaults doesn't have such limitations + keeps property semantics
> simple and working the same way regardless accelerator.

This is true. And making a default that depends on kvm_enabled() is
what? A _non-static_ property default.


> Extra data in form of
> class_init()s looks more preferable than complicated code/semantics.

Exactly! That's why a simple tcg-vendor/vendor property pair is simple
and straightforward. Checking the KVM configuration in class_init is
what makes the semantics more complicated.

class_init initializes static data that shouldn't depend on
initialization of other parts of QEMU. If we want any extra logic, it
belongs to instance_init.

> 
> > 
> > [2] It is possible to fix the problem with "-cpu host" later if we
> >     decide on property/value semantics that allow the "the CPU will
> >     simply copy features from the host when initialized" mode to be
> >     expressed in the class (without requiring KVM to be initialized
> >     first).
> > 
> >     A possible solution is having a "copy-all-host-features=yes"
> >     property in the -cpu host class. But that would probably break "-cpu
> >     host,+foo,-bar".
> > 
> >     Another solution would be to make the "f-*" flags tristate,
> >     accepting "on", "off" and "host". The host class could then simply
> >     set the default for all feature properties to "host", and the
> >     instance init function (not class_init) would understand that "host"
> >     means "ask KVM for host features".
> > 
> > 
> > > 
> > > I agree with Eduardo that we should not try to override class values
> > > based on accel=kvm. Global properties don't operate on classes but on
> > > the properties, which get set up at device-instance_init time only.
> > > If there's an issue with the vendor it seems easier to fix that than to
> > > play games with class_init as we seem to be going in circles...
> properties definitions are part of classes (including properties defaults),
> and defaults are set before global properties at the same place.

Exactly. And if we make class data depend on configuration option
parsing, we are making CPUs require lots of exceptions in the QEMU
main() code.

Please don't confuse "default value for the 'vendor' property" with "how
the cpuid_vendor struct field will look like just after we called
instance_init". They can be different things. I am arguing that the
former should be static, but the latter can look different depending on
kvm_enabled().


> It's not only
> vendor, setting default cpuid_kvm_features in x86_cpu_initfn() will
> overwrite anything set before via global properties (i.e. +foo,-foo).

It won't, if we have two properties (tcg-vendor/vendor). And in the case
of cpuid_kvm_features, we can simply zero it if kvm_enabled() is false,
and that's it.

See my question about changing the "486" vendor above. 

> 
> > > 
> > > That would leave us with the problematic -cpu host class and with
> > > analyzing any remaining instance_init problems.
> Than lets double built-in CPUs with KVM bases sub-classes. That well give us
> constant classes that reliably describe KVM specific and reduce amount of
> kvm_enabled() conditions in initfn(), hence later allow to replace setting
> defaults with static properties (making initfn() even more simpler).

It would work, but why the extra complexity? We just need two very small
additional properties, and that's it. Because the vendor string is the
only part where KVM differ from TCG. The KVM feature list doesn't matter
because enabling a KVM feature on a TCG feature has no effect at all.

> 
> > > 
> > > And not to forget -object and, once Anthony drops his unloved no_user
> > > flag, -device.
> And KVM based sub-classes won't break it, they actually wouldn't be available
> till kvm_init() is completed successfully.
> I understand -object and -device are more like API calls that couldn't be
> called arbitrarily, one has to provide dependencies first. So something like
> -object kvm should go before -object kvm-cpu if caller expects it to work.
> 

KVM-based subclasses would work. A solution requiring a "KVM" object to
be created first would work, too.

But I still think we can simply handle KVM-specific behavior in
instance_init (using a separate tcg-specific and kvm-specific properties
when their defaults need to be different) instead of multiplying our
number of classes by 2.

It's all about how we want to model this to the outside: when we
introduced vhost, did we introduce additional classes for vhost-based
configuration? What about the defaults for vhost-specific options? How
are they set? Why can't we follow the same pattern for CPUs?


> > > 
> > > Regards,
> > > Andreas
> > > 
> > > -- 
> > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
> > 
> > -- 
> > Eduardo
> 
> 
> -- 
> Regards,
>   Igor

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 12 Feb 2013 12:48:47 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Feb 11, 2013 at 02:52:49AM +0100, Igor Mammedov wrote:
> > On Fri, 8 Feb 2013 16:13:02 -0200
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > On Fri, Feb 08, 2013 at 05:54:50PM +0100, Andreas Färber wrote:
> > > > Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> > > > > On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> > > > >> On Fri, 08 Feb 2013 12:16:17 +0100
> > > > >> Andreas Färber <afaerber@suse.de> wrote:
> > > > >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > > > >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> > > > >>>> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > > > >>>>
> > > > >>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> > > > >>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> > > > >>>>>>      }
> > > > >>>>>>  }
> > > > >>>>>>  
> > > > >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void
> > > > >>>>>> *data) +{
> > > > >>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > >>>>>> +    ObjectClass *hoc =
> > > > >>>>>> object_class_by_name(TYPE_HOST_X86_CPU);
> > > > >>>>>> +    X86CPUClass *hostcc;
> > > > >>>>>> +    x86_def_t *def = data;
> > > > >>>>>> +    int i;
> > > > >>>>>> +    static const char *versioned_models[] = { "qemu32",
> > > > >>>>>> "qemu64", "athlon" }; +
> > > > >>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > > > >>>>>> +
> > > > >>>>>> +    /* host cpu class is available if KVM is enabled,
> > > > >>>>>> +     * get kvm overrides from it */
> > > > >>>>>> +    if (hoc) {
> > > > >>>>>> +        hostcc = X86_CPU_CLASS(hoc);
> > > > >>>>>> +        /* sysenter isn't supported in compatibility mode on
> > > > >>>>>> AMD,
> > > > >>>>>> +         * syscall isn't supported in compatibility mode on
> > > > >>>>>> Intel.
> > > > >>>>>> +         * Normally we advertise the actual CPU vendor, but
> > > > >>>>>> you can
> > > > >>>>>> +         * override this using the 'vendor' property if you
> > > > >>>>>> want to use
> > > > >>>>>> +         * KVM's sysenter/syscall emulation in compatibility
> > > > >>>>>> mode and
> > > > >>>>>> +         * when doing cross vendor migration
> > > > >>>>>> +         */
> > > > >>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> > > > >>>>>> +               sizeof(xcc->info.vendor));
> > > > >>>>>> +    }
> > > > >>>>>
> > > > >>>>> Again, we have the same problem we had before, but now in the
> > > > >>>>> non-host classes. What if class_init is called before KVM is
> > > > >>>>> initialized? I believe we will be forced to move this hack to
> > > > >>>>> the instance init function.
> > > > >>>> I believe, the in the case where non-host CPU classes might be
> > > > >>>> initialized before KVM "-cpu ?" we do not care what their
> > > > >>>> defaults are, since we only would use class names there and then
> > > > >>>> exit.
> > > > >>>>
> > > > >>>> For case where classes could be inspected over QMP, OQM, KVM
> > > > >>>> would be already initialized if enabled and we would get proper
> > > > >>>> initialization order without hack.
> > > > > 
> > > > > Who guarantees that KVM will be already initialized when we get a
> > > > > QMP monitor? We can't do that today because of limitations in the
> > > > > QEMU main code, but I believe we want to get rid of this limitation
> > > > > eventually, instead of making it harder to get rid of.
> > > > > 
> > > > > If we could initialize KVM before QMP is initialized, we could
> > > > > simply initialize KVM before class_init is called, instead. It
> > > > > would be easier to reason about, and it would make the limitations
> > > > > of our code very clear to anybody reading the code in main().
> > > > That wouldn't work (currently) due to -device and -object being
> > > > command line options just like -enable-kvm, -disable-kvm and -machine
> > > > accel=.
> > > 
> > > Well, we could loop over the command-line options twice.
> > > 
> > > It is just an alternative that would be better than making class_init
> > > unreliable. I don't think it would be a great solution anyway.
> > > 
> > > > 
> > > > >>>
> > > > >>> I think you're missing Eduardo's and my point:
> > > > >>>
> > > > >>> diff --git a/vl.c b/vl.c
> > > > >>> index a8dc73d..6b9378e 100644
> > > > >>> --- a/vl.c
> > > > >>> +++ b/vl.c
> > > > >>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> > > > >>>      }
> > > > >>>
> > > > >>>      module_call_init(MODULE_INIT_QOM);
> > > > >>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> > > > >>>
> > > > >>>      qemu_add_opts(&qemu_drive_opts);
> > > > >>>      qemu_add_opts(&qemu_chardev_opts);
> > > > >>>
> > > > >>> Anyone may iterate over QOM classes at any time after their type
> > > > >>> registration, which is before the first round of option parsing.
> > > > >>> Sometime later, after option parsing, there's the -cpu ? handling
> > > > >>> in vl.c:3854, then vl.c:4018:configure_accelerator().
> > > > >>>
> > > > >>> Like I said, mostly a theoretical issue today.
> > > > >> Question is if we should drop this theoretical issue for 1.5?
> > > > > 
> > > > > I wouldn't call it just theoretical. It is something that will
> > > > > surely hit us back. The people working on QMP or on the main() code
> > > > > 6 months from now will no idea that our class_init code is broken
> > > > > and will explode if class_init is called too early.
> > > > 
> > > > We should try to find a reliable solution here or at least add
> > > > appropriate comments to the module_call_init() call in vl.c.
> > > 
> > > Agreed. But I believe there are tons of other solutions that don't
> > > require making class_init broken.
> > > 
> > > 
> > > > 
> > > > >>> Originally I had considered making kvm_init() re-entrant and
> > > > >>> calling it from the offending class_init. But we must support the
> > > > >>> distro case of compiling with CONFIG_KVM but the user's hardware
> > > > >>> not supporting KVM or kvm module not being loaded or the user
> > > > >>> having insufficient priviledges to access /dev/kvm.
> > > > >> working without KVM is not issue, it just works with normal
> > > > >> defaults. Applying KVM specific defaults to already initialized
> > > > >> classes is.
> > > > 
> > > > Right, but applying KVM-specific defaults is much easier once KVM is
> > > > initialized. :)
> > > > 
> > > > > 
> > > > > My big question is: why exactly we want to initialize this stuff
> > > > > inside class_init? Can't we (please!) put the KVM-specific logic
> > > > > inside instance_init?
> > I see 2 issues with it:
> > 1. a rather abstract introspection. defaults are belong to class data and
> >    user who introspected class would expect to get CPU he saw during it,
> > which he won't get if instance_init() will set another defaults, It will
> > be already another CPU. So introspection becomes useless here.
> 
> Really, it does not become useless, it has very clear semantics. The
> difference is that now the defaults won't depend on the kvm=on/off
> configuration.
>
> I believe strongly that one of the purposes of class-based introspection
> is to have introspectable _static_ data that do not depend on any
> configuration data.
It isn't static (look at model_id in class_init), I'd say it should be
immutable after class is initialized, which means it could be determined
dynamically in class_init.

> 
> > 
> > 2. more close to code:
> >     * vendor property, you offering to add a new tcg-vendor property. If
> > we are dumping goal [1] then it might work.
> 
> We're now dumping goal [1], we would be enabling it to be achieved,
> because the default values of "vendor" and "tcg-vendor" would be
> completely static.
> 
> The assumption you seem to be unwilling to drop is that cpuid_vendor
> should always unconditionally correspond to the value of the "vendor"
> property set by the class. But it doesn't have to. The CPU class may
> choose to do anything with cpuid_vendor on instance_init, including
> using the "tcg-vendor" property to set it if KVM is disabled and
> "vendor" is empty.
> 
> 
> > But that new property is just
> >       another reincarnation of vendor_override field in CPUState that
> > we've just gotten rid of and brings back hack of selecting what guest os
> > will see.
> 
> In a way, yes. But it looks like it is necessary. But at least is a very
> understandable model that can be seen from the outside. "tcg-vendor" is
> obviously tcg-only, and "vendor" overrides everything if set.
I'm not sure it's necessary and I dislike adding extra semantics to CPU
unless it's how it's made in real hardware or we have to due to lack of
another way to implement it correctly. CPUID has only one 'vendor' so lets
try to make it work instead of working around deficiencies of current
modeling.

> 
> 
> >     * cpuid_kvm_features defaults also different for KVM and TCG. Which
> > also makes 2 different CPUs, and makes guest behave differently.
> >       If we set defaults in instance_init(), we would loose possibility
> > to use global(cmd line) and compat(pv_eoi) properties with respective
> >       feature bits or invent another special case to detect that
> >       global/compat properties were used and workaround it.
> 
> I don't propose setting property defaults on instance_init. I propose
> using the _static_ defaults set by class_init to initialize the CPU
> state on instance_init. Just like nobody said that the CPU _must_
> initialize cpuid_vendor using the "vendor" property only, nobody said
> that the CPU can't set cpuid_kvm_features to zero on instance_init if
> KVM is disabled.
whether defaults are static or dynamic doesn't matter, it doesn't change
the fact that it's defaults nonetheless. And setting one defaults in
class_init and others in instance_init reminds of the split brain syndrome.
It's much better to do it one place if possible.
kvm_features is just simplified case of 'vendor' with only 2 default states.
It could be workarounded by clearing it in instance_init() in TCG mode, but
that would mean:
 1. reporting this features as available at class-level when
    they aren't available for this class in TCG mode. which doesn't look
    correct.
 2. clearing would silently override whatever user set via global properties
    which isn't ok even if user shoots in his own leg setting them

> 
> You seem to be confusing "how the CPU object struct fields will look
> like just after instance_init is called" with "property defaults". They
> are different things.
I think it should be equal to property defaults if there weren't
compat/global properties provided/applied to alter it. From my POV,
instance_init is for initializing instance specific extra state based
on defaults it got before it was called. (whether vendor and kvm_features
defaults are class wide settings).

> 
> 
> >     * that pile of special cases will only grow with time, adding likes
> >       disable_pv_eoi() hooks and who knows what else.
> 
> I don't see how disable_pv_eoi() is related to that. disable_pv_eoi()
> can easily be converted to global properties once we implement feature
> properties. instance_init can then simply zero all KVM features on
> instance_init if KVM is disabled.
For the price of showing wrong defaults at class level for TCG mode CPU.

> 
> > 
> > To use benefits provided by static properties we need to follow rule that
> > defaults are not set in instance_init(), instead of they should be set
> > using defaults of static properties. Unconditional calls to set them in
> > this patch could be eventually converted to class_init() defaults. But
> > kvm_enabled() conditioned ones like vendor and cpuid_kvm_features, rise
> > chicken or egg problem we are trying to solve.
> 
> We _are_ setting defaults on class_init, the difference is that
> instance_init choose which property to obey ("vendor" or "tcg-vendor")
> depending on configuration. Doing that would _fix_ global properties,
> not break it (because now we can change the tcg-only default-vendor of a
> CPU model using global properties easily. See my "486" example/question
> below).
Why fix anything if it could be done earlier without fixing ever.

> 
> > 
> > From reading above I've got that fixing up built-in CPUs class data is not
> > perfect idea and might not work after all.
> > Then lets consider alternative of creating a bunch of KVM-based built-in
> > CPU sub-classes that are added at kvm_init() time.
> 
> Separate KVM-based subclasses would work, too, but it looks like
> overkill if we can simply have two properties.
I prefer KVM-based subclasses correctness and uniform handling of defaults
without exceptions with clear properties semantic to splitting defaults
handling to two places and working around limitations it incurs (introducing
properties not existing in real hw along the way, without must have need in
it).

> 
> 
> > 
> > 
> > > > 
> > > > Then we're pretty much back to my v3 plus Igor's error handling
> > > > change, right? Modulo whether to register the host class in
> > > > kvm_arch_init() or always.
> > > 
> > > I don't know, I would need to take a look at your v3. I don't remember
> > > how many of the code in this version was already in v3.
> > > 
> > > > 
> > > > > If "default vendor set in in built-in CPU model table" (TCG-only)
> > > > > has a different meaning from "vendor set by
> > > > > command-line/global-property" (affects TCG and KVM), it means we
> > > > > have two different knobs with two diferent semantics. Hence my
> > > > > suggestion of adding two properties: "tcg-vendor" and "vendor".
> > > > 
> > > > I don't quite understand why we would need a "tcg-vendor" property.
> > > > Are you trying to address setting or getting the value? If it's
> > > > setting we should just bypass the property in our internal code,
> > > > using Igor's vendor_str2words helper.
> > > 
> > > Just for setting (more exactly, just for carrying the TCG-only default
> > > values on the built-in classes, that are currently inside the x86_def_t
> > > table). The tricky part here is that we need to differentiate the
> > > default vendor value from class_init/x86_def_t that is valid only for
> > > TCG (on KVM, the default vendor is always the host CPU vendor), and
> > > "vendor" property set using global-properties/command-line is different
> > > and should be used on both TCG and KVM modes.
> > > 
> > > 
> > > 
> > > > 
> > > [...]
> > > > I've been thinking about whether this is a more general issue that we
> > > > could solve at QOM level, like the base_class_init for qdev props,
> > > > but I haven't come up with a usable idea. (Paolo?)
> > > 
> > > I don't see any simple solution involving extending the QOM design,
> > > except that we need to decide on a general dependency/layering model and
> > > stick with it instead of trying to break the model without admitting
> > > we're breaking it.
> > > 
> > > I believe the current dependency chain is:
> > > 
> > >  - class data initialization
> > >     -> handling of configuration options
> > >        -> accelerator initialization
> > >           -> machine and CPU instances initialization
> > > 
> > > That means:
> > > 
> > >  - Handling of configuration options (via command-line, QMP, config
> > >    file, whatever method) need classes to be initialized first.
> > >  - Accelerator initialization depends on config options to be handled
> > >    first.
> > >  - Hence, we can't make class data depend on accelerator initialization.
> > >    Period.
> > > 
> > > 
> > > What we _could_ do (but I don't think is the best solution) is:
> > > 
> > >  - handling of accelerator options
> > >    -> accelerator initialization
> > >       -> class data
> > >          -> handling of remaining configuration options
> > >             -> machine and CPU instances initialization
> > > 
> > > Such a change would be introsive, so I believe we can try to fix it by
> > > simply changing our CPU class model so that class data don't depend on
> > > accelerator data[1].
> > > 
> > > An intermediate solution could be registering and initializing all the
> > > X86CPU classes later. e.g.:
> > > 
> > >  - General class data initialization
> > >    -> handling of configuration options
> > >       -> accelerator initialization
> > >          -> X86CPU class data initialization
> > >             -> machine and CPU instances initialization
> > +1 if KVM based sub-classes idea is not acceptable, this could work for me
> > as compromise provided defaults are initialized in class_init()
> > 
> > > 
> > > The problem with this approach is that it would be impossible to
> > > implement "-cpu ?" and "query-cpu-definitions" without at least handling
> > "-cpu ?" can be moved after kvm_init() and we could put comment that
> > QMP should be accessed after it.
> 
> No, we can't. QMP should be able to handle the "handling of
> configuration options" part. And "-cpu ?"/query-cpu-definitions doesn't
> require the KVM option to be set.
> 
> > 
> > > the accelerator configuration options first. This would probably break
> > > (or require hacks for) "-device" too.
> > Why break? -device cpu doesn't work now and I'm not sure it should/would
> > in oversee-able future.
> 
> Well, it doesn't work today, but we plan to make it work, right? So we
> must choose a design that allows us to make it work instead of making
> our life difficult in the near future. This solution has "handling of
> configuration options" before "X86CPU class data initialization", so it
> will prevent us from making -device work with CPUs.
> 
> We are making huge efforts to make CPUs devices like any other, I
> believe we should avoid introducing _additional_ exceptions and
> special-cases for the CPU devices.
Agreed. That looks like class dependence issue. Where kmv should be enabled
explicitly before any -device are handled or -device should gracefully fail
if not kvm present and kvm dependent CPU is requested. Or implicitly which
would require introducing class dependencies and their implicit
initialization.

> 
> > 
> > > 
> > > The latter is already the solution we are trying for "-cpu host", but at
> > > least we know that "-cpu host" is special and we can work around it in
> > > the query-cpu-definitions and "-cpu ?" code[2].
> > > 
> > > 
> > > [1] My main point is: we're trying to force accelerator-provided data to
> > >     be stored in the class, when we really don't need that data to be
> > >     stored in the class. We just need property-value semantics that make
> > >     the object use the accelerator-provided data later, but without
> > >     storing the accelerator-provided data itself inside the class.
> > that complicates property handling a lot to a point of not able to use
> > global/compat properties.
> 
> I believe it _simplifies_ property handling a lot and allows us to
> easily use global/compat properties even if we want a KVM-only
> global-property.
> 
> e.g. assume we made a mistake and we have an Intel CPU (e.g. 486) with
> vendor=AMD in the builtin CPU list. With your solution, how would you
> set global properties to fix this and make 486 have vendor=Intel without
> affecting KVM at all?
global_properties isn't for fixing, but lets imagine we do it:
> 
> With my solution it is simple:
> 
>  { "486-x86_64-cpu", "tcg-vendor", "GenuineIntel" }
It would be:
 { "486-x86_64-cpu", "vendor", "GenuineIntel" }

> 
> I don't see how you could do that with the current patch.
Current patch doesn't support static properties and their features.
We need this intermediate subclasses + convertion to static properties +
moving setting defaults from instance_init to class_init using static
properties default values approach.

> 
> 
> > Setting defaults in class_init() using static
> > property defaults doesn't have such limitations + keeps property semantics
> > simple and working the same way regardless accelerator.
> 
> This is true. And making a default that depends on kvm_enabled() is
> what? A _non-static_ property default.
> 
> 
> > Extra data in form of
> > class_init()s looks more preferable than complicated code/semantics.
> 
> Exactly! That's why a simple tcg-vendor/vendor property pair is simple
> and straightforward. Checking the KVM configuration in class_init is
> what makes the semantics more complicated.
> 
> class_init initializes static data that shouldn't depend on
> initialization of other parts of QEMU. If we want any extra logic, it
> belongs to instance_init.
I can't agree with this, my POV on claas_init vs instance_init is stated
earlier.
And if type requires other parts of qemu to be correctly intialized, it would
mean that it should be initialized even registered after that parts are
initialized.
If something attempts to access it before than that something should be fixed
not type.

> 
> > 
> > > 
> > > [2] It is possible to fix the problem with "-cpu host" later if we
> > >     decide on property/value semantics that allow the "the CPU will
> > >     simply copy features from the host when initialized" mode to be
> > >     expressed in the class (without requiring KVM to be initialized
> > >     first).
> > > 
> > >     A possible solution is having a "copy-all-host-features=yes"
> > >     property in the -cpu host class. But that would probably break "-cpu
> > >     host,+foo,-bar".
> > > 
> > >     Another solution would be to make the "f-*" flags tristate,
> > >     accepting "on", "off" and "host". The host class could then simply
> > >     set the default for all feature properties to "host", and the
> > >     instance init function (not class_init) would understand that "host"
> > >     means "ask KVM for host features".
> > > 
> > > 
> > > > 
> > > > I agree with Eduardo that we should not try to override class values
> > > > based on accel=kvm. Global properties don't operate on classes but on
> > > > the properties, which get set up at device-instance_init time only.
> > > > If there's an issue with the vendor it seems easier to fix that than
> > > > to play games with class_init as we seem to be going in circles...
> > properties definitions are part of classes (including properties
> > defaults), and defaults are set before global properties at the same
> > place.
> 
> Exactly. And if we make class data depend on configuration option
> parsing, we are making CPUs require lots of exceptions in the QEMU
> main() code.
I'll send a RFC using KVM based sublcasses that won't impose additional
dependency compared to current code.
 
> Please don't confuse "default value for the 'vendor' property" with "how
> the cpuid_vendor struct field will look like just after we called
> instance_init". They can be different things. I am arguing that the
> former should be static, but the latter can look different depending on
> kvm_enabled().
> 
> 
> > It's not only
> > vendor, setting default cpuid_kvm_features in x86_cpu_initfn() will
> > overwrite anything set before via global properties (i.e. +foo,-foo).
> 
> It won't, if we have two properties (tcg-vendor/vendor). And in the case
> of cpuid_kvm_features, we can simply zero it if kvm_enabled() is false,
> and that's it.
> 
> See my question about changing the "486" vendor above. 
>
> > 
> > > > 
> > > > That would leave us with the problematic -cpu host class and with
> > > > analyzing any remaining instance_init problems.
> > Than lets double built-in CPUs with KVM bases sub-classes. That well give
> > us constant classes that reliably describe KVM specific and reduce amount
> > of kvm_enabled() conditions in initfn(), hence later allow to replace
> > setting defaults with static properties (making initfn() even more
> > simpler).
> 
> It would work, but why the extra complexity? We just need two very small
> additional properties, and that's it. Because the vendor string is the
> only part where KVM differ from TCG. The KVM feature list doesn't matter
> because enabling a KVM feature on a TCG feature has no effect at all.
Because it's more correct, doesn't require workarounds in instance_init(),
doesn't introduce extra (fake)properties/interfaces with related extra
semantic to maintain, allows to treat all properties uniformly without
exceptions, doesn't break class-level introspection ( it really reflects
default state of properties for a class in question), not more complex in
terms amount of code than extra ifs in instance_init() and extra properties.

> 
> > 
> > > > 
> > > > And not to forget -object and, once Anthony drops his unloved no_user
> > > > flag, -device.
> > And KVM based sub-classes won't break it, they actually wouldn't be
> > available till kvm_init() is completed successfully.
> > I understand -object and -device are more like API calls that couldn't be
> > called arbitrarily, one has to provide dependencies first. So something
> > like -object kvm should go before -object kvm-cpu if caller expects it to
> > work.
> > 
> 
> KVM-based subclasses would work. A solution requiring a "KVM" object to
> be created first would work, too.
> 
> But I still think we can simply handle KVM-specific behavior in
> instance_init (using a separate tcg-specific and kvm-specific properties
> when their defaults need to be different) instead of multiplying our
> number of classes by 2.
> 
> It's all about how we want to model this to the outside: when we
> introduced vhost, did we introduce additional classes for vhost-based
> configuration? What about the defaults for vhost-specific options? How
> are they set? Why can't we follow the same pattern for CPUs?
> 
> 
> > > > 
> > > > Regards,
> > > > Andreas
> > > > 
> > > > -- 
> > > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG
> > > > Nürnberg
> > > 
> > > -- 
> > > Eduardo
> > 
> > 
> > -- 
> > Regards,
> >   Igor
> 

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

[Eduardo Habkost]

TL;DR: I still disagree about some points, but those points aren't so
relevant anymore because I am starting to like having
KVM-specific/TCG-specific subclasses (because of other problems that
would be solved by them).


On Wed, Feb 13, 2013 at 04:20:43PM +0100, Igor Mammedov wrote:
[...]
> > > > > > 
> > > > > > My big question is: why exactly we want to initialize this stuff
> > > > > > inside class_init? Can't we (please!) put the KVM-specific logic
> > > > > > inside instance_init?
> > > I see 2 issues with it:
> > > 1. a rather abstract introspection. defaults are belong to class data and
> > >    user who introspected class would expect to get CPU he saw during it,
> > > which he won't get if instance_init() will set another defaults, It will
> > > be already another CPU. So introspection becomes useless here.
> > 
> > Really, it does not become useless, it has very clear semantics. The
> > difference is that now the defaults won't depend on the kvm=on/off
> > configuration.
> >
> > I believe strongly that one of the purposes of class-based introspection
> > is to have introspectable _static_ data that do not depend on any
> > configuration data.
> It isn't static (look at model_id in class_init), I'd say it should be
> immutable after class is initialized, which means it could be determined
> dynamically in class_init.

That's were I believe we disagree. It's not about being immutable, it's
about being introspectable before anything is configured.

We could change the values between QEMU versions (it would be nice to
avoid it, but not a must), but we can't make depend on configuration
data unless we find a solution to the dependency/ordering problem.

Note that I would be happy enough if the QEMU maintainers decided that
CPUs are special for QEMU and the CPU classes must be initialized very
late, or that accelerator initialization is special and can happen
before any class is initialized. But we must make an explicit decision
about that, if we want to. That's why I listed the posible options I
see, below (after the paragraph I say "I believe the current dependency
chain is").


> > > 
> > > 2. more close to code:
> > >     * vendor property, you offering to add a new tcg-vendor property. If
> > > we are dumping goal [1] then it might work.
> > 
> > We're now dumping goal [1], we would be enabling it to be achieved,
> > because the default values of "vendor" and "tcg-vendor" would be
> > completely static.
> > 
> > The assumption you seem to be unwilling to drop is that cpuid_vendor
> > should always unconditionally correspond to the value of the "vendor"
> > property set by the class. But it doesn't have to. The CPU class may
> > choose to do anything with cpuid_vendor on instance_init, including
> > using the "tcg-vendor" property to set it if KVM is disabled and
> > "vendor" is empty.
> > 
> > 
> > > But that new property is just
> > >       another reincarnation of vendor_override field in CPUState that
> > > we've just gotten rid of and brings back hack of selecting what guest os
> > > will see.
> > 
> > In a way, yes. But it looks like it is necessary. But at least is a very
> > understandable model that can be seen from the outside. "tcg-vendor" is
> > obviously tcg-only, and "vendor" overrides everything if set.
> I'm not sure it's necessary and I dislike adding extra semantics to CPU
> unless it's how it's made in real hardware or we have to due to lack of
> another way to implement it correctly. CPUID has only one 'vendor' so lets
> try to make it work instead of working around deficiencies of current
> modeling.

We are not adding "extra" semantics, we are just modelling the existing
(weird) KVM vs. TCG semantics, unfortunately. The class model is not
just a hardware model, but a "configuration data" model.

AFAIU, not every property is about modelling hardware, but also about
modelling the knobs we ahve to configure the software. The block device
"cache" option is not about modelling hardware, the virtio-net "timer"
option is not about modelling hardware.

So, if we have to keep the weird semantics, the point is that the weird
semantics belong to instance_init and not class_init (because of the
ordering/dependency problem we have).

> 
> > 
> > 
> > >     * cpuid_kvm_features defaults also different for KVM and TCG. Which
> > > also makes 2 different CPUs, and makes guest behave differently.
> > >       If we set defaults in instance_init(), we would loose possibility
> > > to use global(cmd line) and compat(pv_eoi) properties with respective
> > >       feature bits or invent another special case to detect that
> > >       global/compat properties were used and workaround it.
> > 
> > I don't propose setting property defaults on instance_init. I propose
> > using the _static_ defaults set by class_init to initialize the CPU
> > state on instance_init. Just like nobody said that the CPU _must_
> > initialize cpuid_vendor using the "vendor" property only, nobody said
> > that the CPU can't set cpuid_kvm_features to zero on instance_init if
> > KVM is disabled.
> whether defaults are static or dynamic doesn't matter, it doesn't change
> the fact that it's defaults nonetheless. And setting one defaults in
> class_init and others in instance_init reminds of the split brain syndrome.
> It's much better to do it one place if possible.

It would be wonderful if we could. But we have the dependency/ordering
problem.

> kvm_features is just simplified case of 'vendor' with only 2 default states.
> It could be workarounded by clearing it in instance_init() in TCG mode, but
> that would mean:
>  1. reporting this features as available at class-level when
>     they aren't available for this class in TCG mode. which doesn't look
>     correct.

We have lots of features that may be reported as available but can't be
enabled because of lack of host support.

>  2. clearing would silently override whatever user set via global properties
>     which isn't ok even if user shoots in his own leg setting them

That's what we already do on TCG today, for every single feature. The
KVM features are not an exception, they are just following the same
pattern: TCG code silently disables the features that are not supported
by TCG. See the usage of TCG_FEATURES on x86_cpu_realize().

But you have a point here: if TCG behavior is so so weird regarding
_every_ feature (not just the KVM ones!), maybe splitting in two class
hierarchies will be simpler. I still would like to avoid it if possible,
but maybe it is a reasonable solution.


> 
> > 
> > You seem to be confusing "how the CPU object struct fields will look
> > like just after instance_init is called" with "property defaults". They
> > are different things.
> I think it should be equal to property defaults if there weren't
> compat/global properties provided/applied to alter it. From my POV,
> instance_init is for initializing instance specific extra state based
> on defaults it got before it was called. (whether vendor and kvm_features
> defaults are class wide settings).

instance_init is where we implement things that can't be represented as
simple data and needs code. My reasoning/assumptions are:

0) We can't check kvm_enabled() on class_init (because of the
   ordering/dependency problem);
1) class_init is the only place we can initialize QOM-property defaults
   before instance_init is called.
2) instance_init is the only place we can check kvm_enabled().

So, the only solutions I see that match all the requirements above are:

1) Having separate TCG-specific/KVM-specific properties because we
   actually have two different knobs/defaults for KVM and TCG;
2) Having separate subclasses for KVM and TCG;
3) Change the ordering/dependency requirements to allow class_init to
   depend on kvm_enabled.


> 
> > 
> > 
> > >     * that pile of special cases will only grow with time, adding likes
> > >       disable_pv_eoi() hooks and who knows what else.
> > 
> > I don't see how disable_pv_eoi() is related to that. disable_pv_eoi()
> > can easily be converted to global properties once we implement feature
> > properties. instance_init can then simply zero all KVM features on
> > instance_init if KVM is disabled.
> For the price of showing wrong defaults at class level for TCG mode CPU.

True. But as I said above, this is already the case for every other
feature that's not inside TCG_FEATURES/TCG_EXT_FEATURES/etc.

(But as I said above, this is a point for making separate subclasses)

> 
> > 
> > > 
> > > To use benefits provided by static properties we need to follow rule that
> > > defaults are not set in instance_init(), instead of they should be set
> > > using defaults of static properties. Unconditional calls to set them in
> > > this patch could be eventually converted to class_init() defaults. But
> > > kvm_enabled() conditioned ones like vendor and cpuid_kvm_features, rise
> > > chicken or egg problem we are trying to solve.
> > 
> > We _are_ setting defaults on class_init, the difference is that
> > instance_init choose which property to obey ("vendor" or "tcg-vendor")
> > depending on configuration. Doing that would _fix_ global properties,
> > not break it (because now we can change the tcg-only default-vendor of a
> > CPU model using global properties easily. See my "486" example/question
> > below).
> Why fix anything if it could be done earlier without fixing ever.

I couldn't parse what you meant here.

The current solution that checks kvm_enabled() in class_init doesn't
work with global properties. It doesn't allow us to set compat global
properties to allow us to change the default vendor only in TCG mode
(see the "486" example).

> 
> > 
> > > 
> > > From reading above I've got that fixing up built-in CPUs class data is not
> > > perfect idea and might not work after all.
> > > Then lets consider alternative of creating a bunch of KVM-based built-in
> > > CPU sub-classes that are added at kvm_init() time.
> > 
> > Separate KVM-based subclasses would work, too, but it looks like
> > overkill if we can simply have two properties.
> I prefer KVM-based subclasses correctness and uniform handling of defaults
> without exceptions with clear properties semantic to splitting defaults
> handling to two places and working around limitations it incurs (introducing
> properties not existing in real hw along the way, without must have need in
> it).

I understand.

Just to make it clear: I am not against having separate TCG/KVM
subclasses, I just would like to avoid them if possible.

Checking kvm_enabled() in class_init, on the other hand, isn't just
something I would like to avoid, it's something that I believe is broken
(unless we decide to change the main code ordering/dependencies).

[...]
> > > > > I've been thinking about whether this is a more general issue that we
> > > > > could solve at QOM level, like the base_class_init for qdev props,
> > > > > but I haven't come up with a usable idea. (Paolo?)
> > > > 
> > > > I don't see any simple solution involving extending the QOM design,
> > > > except that we need to decide on a general dependency/layering model and
> > > > stick with it instead of trying to break the model without admitting
> > > > we're breaking it.
> > > > 
> > > > I believe the current dependency chain is:
> > > > 
> > > >  - class data initialization
> > > >     -> handling of configuration options
> > > >        -> accelerator initialization
> > > >           -> machine and CPU instances initialization
> > > > 
> > > > That means:
> > > > 
> > > >  - Handling of configuration options (via command-line, QMP, config
> > > >    file, whatever method) need classes to be initialized first.
> > > >  - Accelerator initialization depends on config options to be handled
> > > >    first.
> > > >  - Hence, we can't make class data depend on accelerator initialization.
> > > >    Period.
> > > > 
> > > > 
> > > > What we _could_ do (but I don't think is the best solution) is:
> > > > 
> > > >  - handling of accelerator options
> > > >    -> accelerator initialization
> > > >       -> class data
> > > >          -> handling of remaining configuration options
> > > >             -> machine and CPU instances initialization
> > > > 
> > > > Such a change would be introsive, so I believe we can try to fix it by
> > > > simply changing our CPU class model so that class data don't depend on
> > > > accelerator data[1].
> > > > 
> > > > An intermediate solution could be registering and initializing all the
> > > > X86CPU classes later. e.g.:
> > > > 
> > > >  - General class data initialization
> > > >    -> handling of configuration options
> > > >       -> accelerator initialization
> > > >          -> X86CPU class data initialization
> > > >             -> machine and CPU instances initialization
> > > +1 if KVM based sub-classes idea is not acceptable, this could work for me
> > > as compromise provided defaults are initialized in class_init()
> > > 
> > > > 
> > > > The problem with this approach is that it would be impossible to
> > > > implement "-cpu ?" and "query-cpu-definitions" without at least handling
> > > "-cpu ?" can be moved after kvm_init() and we could put comment that
> > > QMP should be accessed after it.
> > 
> > No, we can't. QMP should be able to handle the "handling of
> > configuration options" part. And "-cpu ?"/query-cpu-definitions doesn't
> > require the KVM option to be set.
> > 
> > > 
> > > > the accelerator configuration options first. This would probably break
> > > > (or require hacks for) "-device" too.
> > > Why break? -device cpu doesn't work now and I'm not sure it should/would
> > > in oversee-able future.
> > 
> > Well, it doesn't work today, but we plan to make it work, right? So we
> > must choose a design that allows us to make it work instead of making
> > our life difficult in the near future. This solution has "handling of
> > configuration options" before "X86CPU class data initialization", so it
> > will prevent us from making -device work with CPUs.
> > 
> > We are making huge efforts to make CPUs devices like any other, I
> > believe we should avoid introducing _additional_ exceptions and
> > special-cases for the CPU devices.
> Agreed. That looks like class dependence issue. Where kmv should be enabled
> explicitly before any -device are handled or -device should gracefully fail
> if not kvm present and kvm dependent CPU is requested. Or implicitly which
> would require introducing class dependencies and their implicit
> initialization.
> 
> > 
> > > 
> > > > 
> > > > The latter is already the solution we are trying for "-cpu host", but at
> > > > least we know that "-cpu host" is special and we can work around it in
> > > > the query-cpu-definitions and "-cpu ?" code[2].
> > > > 
> > > > 
> > > > [1] My main point is: we're trying to force accelerator-provided data to
> > > >     be stored in the class, when we really don't need that data to be
> > > >     stored in the class. We just need property-value semantics that make
> > > >     the object use the accelerator-provided data later, but without
> > > >     storing the accelerator-provided data itself inside the class.
> > > that complicates property handling a lot to a point of not able to use
> > > global/compat properties.
> > 
> > I believe it _simplifies_ property handling a lot and allows us to
> > easily use global/compat properties even if we want a KVM-only
> > global-property.
> > 
> > e.g. assume we made a mistake and we have an Intel CPU (e.g. 486) with
> > vendor=AMD in the builtin CPU list. With your solution, how would you
> > set global properties to fix this and make 486 have vendor=Intel without
> > affecting KVM at all?
> global_properties isn't for fixing, but lets imagine we do it:
> > 
> > With my solution it is simple:
> > 
> >  { "486-x86_64-cpu", "tcg-vendor", "GenuineIntel" }
> It would be:
>  { "486-x86_64-cpu", "vendor", "GenuineIntel" }
> 
> > 
> > I don't see how you could do that with the current patch.
> Current patch doesn't support static properties and their features.
> We need this intermediate subclasses + convertion to static properties +
> moving setting defaults from instance_init to class_init using static
> properties default values approach.

Replace "current patch" with "a property-based solution that involves
checking kvm_enabled() in class_init before initializing the vendor
property".

> 
> > 
> > 
> > > Setting defaults in class_init() using static
> > > property defaults doesn't have such limitations + keeps property semantics
> > > simple and working the same way regardless accelerator.
> > 
> > This is true. And making a default that depends on kvm_enabled() is
> > what? A _non-static_ property default.
> > 
> > 
> > > Extra data in form of
> > > class_init()s looks more preferable than complicated code/semantics.
> > 
> > Exactly! That's why a simple tcg-vendor/vendor property pair is simple
> > and straightforward. Checking the KVM configuration in class_init is
> > what makes the semantics more complicated.
> > 
> > class_init initializes static data that shouldn't depend on
> > initialization of other parts of QEMU. If we want any extra logic, it
> > belongs to instance_init.
> I can't agree with this, my POV on claas_init vs instance_init is stated
> earlier.
> And if type requires other parts of qemu to be correctly intialized, it would
> mean that it should be initialized even registered after that parts are
> initialized.
> If something attempts to access it before than that something should be fixed
> not type.
> 

I don't think that's how QOM is designed. But you don't need to convince
me of this, you need to convince the maintainers of QOM, QMP, and the
main() code.

> > 
> > > 
> > > > 
> > > > [2] It is possible to fix the problem with "-cpu host" later if we
> > > >     decide on property/value semantics that allow the "the CPU will
> > > >     simply copy features from the host when initialized" mode to be
> > > >     expressed in the class (without requiring KVM to be initialized
> > > >     first).
> > > > 
> > > >     A possible solution is having a "copy-all-host-features=yes"
> > > >     property in the -cpu host class. But that would probably break "-cpu
> > > >     host,+foo,-bar".
> > > > 
> > > >     Another solution would be to make the "f-*" flags tristate,
> > > >     accepting "on", "off" and "host". The host class could then simply
> > > >     set the default for all feature properties to "host", and the
> > > >     instance init function (not class_init) would understand that "host"
> > > >     means "ask KVM for host features".
> > > > 
> > > > 
> > > > > 
> > > > > I agree with Eduardo that we should not try to override class values
> > > > > based on accel=kvm. Global properties don't operate on classes but on
> > > > > the properties, which get set up at device-instance_init time only.
> > > > > If there's an issue with the vendor it seems easier to fix that than
> > > > > to play games with class_init as we seem to be going in circles...
> > > properties definitions are part of classes (including properties
> > > defaults), and defaults are set before global properties at the same
> > > place.
> > 
> > Exactly. And if we make class data depend on configuration option
> > parsing, we are making CPUs require lots of exceptions in the QEMU
> > main() code.
> I'll send a RFC using KVM based sublcasses that won't impose additional
> dependency compared to current code.

Cool. :-)

>  
> > Please don't confuse "default value for the 'vendor' property" with "how
> > the cpuid_vendor struct field will look like just after we called
> > instance_init". They can be different things. I am arguing that the
> > former should be static, but the latter can look different depending on
> > kvm_enabled().
> > 
> > 
> > > It's not only
> > > vendor, setting default cpuid_kvm_features in x86_cpu_initfn() will
> > > overwrite anything set before via global properties (i.e. +foo,-foo).
> > 
> > It won't, if we have two properties (tcg-vendor/vendor). And in the case
> > of cpuid_kvm_features, we can simply zero it if kvm_enabled() is false,
> > and that's it.
> > 
> > See my question about changing the "486" vendor above. 
> >
> > > 
> > > > > 
> > > > > That would leave us with the problematic -cpu host class and with
> > > > > analyzing any remaining instance_init problems.
> > > Than lets double built-in CPUs with KVM bases sub-classes. That well give
> > > us constant classes that reliably describe KVM specific and reduce amount
> > > of kvm_enabled() conditions in initfn(), hence later allow to replace
> > > setting defaults with static properties (making initfn() even more
> > > simpler).
> > 
> > It would work, but why the extra complexity? We just need two very small
> > additional properties, and that's it. Because the vendor string is the
> > only part where KVM differ from TCG. The KVM feature list doesn't matter
> > because enabling a KVM feature on a TCG feature has no effect at all.
> Because it's more correct, doesn't require workarounds in instance_init(),
> doesn't introduce extra (fake)properties/interfaces with related extra
> semantic to maintain, allows to treat all properties uniformly without
> exceptions, doesn't break class-level introspection ( it really reflects
> default state of properties for a class in question), not more complex in
> terms amount of code than extra ifs in instance_init() and extra properties.

I am becoming more inclined for the separate-subclasses solution because
of the other TCG-specific stuff we have in the code today (the silent
TCG_FEATURES filtering, specifically). With separate subclasses we could
report simply clear TCG_FEATURES in the property default-values
themselves (and not so late in the initialization). In practice we
_already_ have different TCG and KVM CPU models because of the
silent TCG feature-filtering.

I plan to take a look at your subclass-based patch tomorrow.


> 
> > 
> > > 
> > > > > 
> > > > > And not to forget -object and, once Anthony drops his unloved no_user
> > > > > flag, -device.
> > > And KVM based sub-classes won't break it, they actually wouldn't be
> > > available till kvm_init() is completed successfully.
> > > I understand -object and -device are more like API calls that couldn't be
> > > called arbitrarily, one has to provide dependencies first. So something
> > > like -object kvm should go before -object kvm-cpu if caller expects it to
> > > work.
> > > 
> > 
> > KVM-based subclasses would work. A solution requiring a "KVM" object to
> > be created first would work, too.
> > 
> > But I still think we can simply handle KVM-specific behavior in
> > instance_init (using a separate tcg-specific and kvm-specific properties
> > when their defaults need to be different) instead of multiplying our
> > number of classes by 2.
> > 
> > It's all about how we want to model this to the outside: when we
> > introduced vhost, did we introduce additional classes for vhost-based
> > configuration? What about the defaults for vhost-specific options? How
> > are they set? Why can't we follow the same pattern for CPUs?
> > 
> > 

-- 
Eduardo