From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= Subject: Re: [Qemu-devel] [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses Date: Fri, 08 Feb 2013 17:54:50 +0100 Message-ID: <51152DDA.5070309@suse.de> References: <1360082364-12475-1-git-send-email-imammedo@redhat.com> <1360082364-12475-4-git-send-email-imammedo@redhat.com> <20130207150819.GD9964@otherpad.lan.raisama.net> <20130208100329.2105a649@thinkpad.mammed.net> <5114DE81.2000501@suse.de> <20130208135842.10dd34a5@thinkpad.mammed.net> <20130208145231.GK4138@otherpad.lan.raisama.net> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: aliguori@us.ibm.com, gleb@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, anthony@codemonkey.ws, "kvm@vger.kernel.org list" , pbonzini@redhat.com, rth@twiddle.net To: Eduardo Habkost , Igor Mammedov Return-path: Received: from cantor2.suse.de ([195.135.220.15]:40013 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753029Ab3BHQy7 (ORCPT ); Fri, 8 Feb 2013 11:54:59 -0500 In-Reply-To: <20130208145231.GK4138@otherpad.lan.raisama.net> Sender: kvm-owner@vger.kernel.org List-ID: 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=E4rber wrote: >>> Am 08.02.2013 10:03, schrieb Igor Mammedov: >>>> On Thu, 7 Feb 2013 13:08:19 -0200 >>>> Eduardo Habkost 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) >>>>>> } >>>>>> } >>>>>> =20 >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void *data) >>>>>> +{ >>>>>> + X86CPUClass *xcc =3D X86_CPU_CLASS(oc); >>>>>> + ObjectClass *hoc =3D object_class_by_name(TYPE_HOST_X86_CPU= ); >>>>>> + X86CPUClass *hostcc; >>>>>> + x86_def_t *def =3D data; >>>>>> + int i; >>>>>> + static const char *versioned_models[] =3D { "qemu32", "qemu= 64", "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 =3D X86_CPU_CLASS(hoc); >>>>>> + /* sysenter isn't supported in compatibility mode on AM= D, >>>>>> + * syscall isn't supported in compatibility mode on Int= el. >>>>>> + * Normally we advertise the actual CPU vendor, but you= can >>>>>> + * override this using the 'vendor' property if you wan= t to use >>>>>> + * KVM's sysenter/syscall emulation in compatibility mo= de 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 ini= tialized >>>> 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 orde= r without >>>> hack. >=20 > 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 ma= in > code, but I believe we want to get rid of this limitation eventually, > instead of making it harder to get rid of. >=20 > If we could initialize KVM before QMP is initialized, we could simply > initialize KVM before class_init is called, instead. It would be easi= er > 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=3D. >>> >>> 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 i= n >>> 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? >=20 > 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 callin= g 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 KV= M or >>> kvm module not being loaded or the user having insufficient privile= dges >>> to access /dev/kvm. >> working without KVM is not issue, it just works with normal defaults= =2E Applying >> KVM specific defaults to already initialized classes is. Right, but applying KVM-specific defaults is much easier once KVM is initialized. :) >=20 > My big question is: why exactly we want to initialize this stuff insi= de > 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 or= der 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_witho= ut_kvm) { >>>> x86_cpu_builtin_class_initialized_without_kvm =3D 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 in= itfn >>> time (only ever getting set to true by the pre-defined models). The= n it >>> would be per model. And if we add a prototype for the ..._class_ini= t 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 be= fore >> 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 defa= ults is to >> iterate over them in kvm_arch_init() and possibly calling their clas= s_init() >> again to reinitialize. It's still hack (due fixing something up), bu= t it would >> give at least correct KVM mode defaults, regardless of the order cla= sses are >> initialized. >=20 > 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 introspectio= n > easy. If we make the classes change how they look like depending on t= he > 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 fix= ups, i.e. >> create extra KVM-builtin-CPU-classes that are derived from host clas= s. >> and in object_class_by_name() lookup for them if kvm is enabled. But= we could >> do this as follow-up to #1. >> =20 >>> precautions and add explanatory comments. >>> >>>>> If we still want the default vendor to be a static property in th= e >>>>> class, we can do that if we set the default in a "tcg-vendor" pro= perty >>>>> instead of a "vendor" property (that would be empty/unset by defa= ult), >>>>> and x86_cpu_initfn() could do this: >>>>> >>>>> vendor =3D object_property_get_str(cpu, "vendor"); >>>>> tcg_vendor =3D object_property_get_str(cpu, "tcg-vendor"); >>>>> if (vendor && vendor[0]) { >>>>> cpu->cpuid_vendor =3D vendor; >>>>> } else if (kvm_enabled()) { >>>>> cpu->cpuid_vendor =3D get_host_vendor(); >>>>> } else { >>>>> cpu->cpuid_vendor =3D tcg_vendor; >>>>> } >>>>> >>>>>> + >>>>>> + /* Look for specific models that have the QEMU version in .= model_id */ >>>>>> + for (i =3D 0; i < ARRAY_SIZE(versioned_models); i++) { >>>>>> + if (strcmp(versioned_models[i], def->name) =3D=3D 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; >>>>>> } >>>>>> =20 >>>> [...] >>>> >>>>>> int kvm_arch_init(KVMState *s) >>>>>> { >>>>>> QemuOptsList *list =3D qemu_find_opts("machine"); >>>>>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s) >>>>>> int ret; >>>>>> struct utsname utsname; >>>>>> =20 >>>>>> + static const TypeInfo host_x86_cpu_type_info =3D { >>>>>> + .name =3D TYPE_HOST_X86_CPU, >>>>>> + .parent =3D TYPE_X86_CPU, >>>>>> + .class_init =3D kvm_host_cpu_class_init, >>>>>> + }; >>>>>> + >>>>>> ret =3D 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 com= pletely >>>>> useless anyway (because all data in the "host" class depend on da= ta >>>>> 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 ex= perts. >>>> >>>> Antnony, Paolo, Andreas >>>> what do you think? >>> >>> I already answered Eduardo on IRC that in general I see no restrict= ion >>> not to register a type late. >>> >>> The issue is that in this case we cannot rely on accessing the clas= s >>> from another class_init that is registered before it, which you wer= e >>> 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 acce= ss 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 no= t 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. >=20 > (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=3Dkvm. 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg