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