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 <afaer...@suse.de> wrote: >>> Am 08.02.2013 10:03, schrieb Igor Mammedov: >>>> On Thu, 7 Feb 2013 13:08:19 -0200 >>>> Eduardo Habkost <ehabk...@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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html