Am 08.02.2013 10:03, schrieb Igor Mammedov:
> On Thu, 7 Feb 2013 13:08:19 -0200
> Eduardo Habkost <ehabkost@xxxxxxxxxx> wrote:
>
>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
>>> From: Andreas Färber <afaerber@xxxxxxx>
>>>
>>> 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@xxxxxxx>
>>> Signed-off-by: Igor Mammedov <imammedo@xxxxxxxxxx>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
