From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Subject: Re: [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses Date: Fri, 08 Feb 2013 12:16:17 +0100 Message-ID: <5114DE81.2000501@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> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: aliguori@us.ibm.com, Eduardo Habkost , 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: Igor Mammedov Return-path: In-Reply-To: <20130208100329.2105a649@thinkpad.mammed.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org Am 08.02.2013 10:03, schrieb Igor Mammedov: > On Thu, 7 Feb 2013 13:08:19 -0200 > Eduardo Habkost wrote: >=20 >> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote: >>> From: Andreas F=C3=A4rber >>> >>> 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 fo= r >>> comparison. >>> >>> Signed-off-by: Andreas F=C3=A4rber >>> Signed-off-by: Igor Mammedov >>> --- >>> 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 CPU= s. >>> --- >>> 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 >>> =20 >>> +#define TYPE_HOST_X86_CPU "host-" TYPE_X86_CPU >> >> Can we introduce a X86_CPU_CLASS_NAME() macro to help us make the rule= s >> 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 =3D 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. >=20 >> >> >> >>> + >>> +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=3D7,ECX=3D0].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 { >>> =20 >>> 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. >=20 > 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; >>> =20 >>> /** >>> 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 @@ > [...] >=20 >>> @@ -2195,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj) >>> CPUState *cs =3D CPU(obj); >>> X86CPU *cpu =3D X86_CPU(obj); >>> CPUX86State *env =3D &cpu->env; >>> + X86CPUClass *xcc =3D X86_CPU_GET_CLASS(obj); >>> + const x86_def_t *def =3D &xcc->info; >>> static int inited; >>> =20 >>> 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); >>> =20 >>> + 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 =3D def->features; >>> + env->cpuid_ext_features =3D def->ext_features; >>> + env->cpuid_ext_features |=3D CPUID_EXT_HYPERVISOR; >>> + env->cpuid_ext2_features =3D def->ext2_features; >>> + env->cpuid_ext3_features =3D def->ext3_features; >>> + object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL= ); >>> + env->cpuid_kvm_features =3D def->kvm_features; >>> + if (kvm_enabled()) { >>> + env->cpuid_kvm_features |=3D 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 wi= th >> this patch. >> > whould be moving it in x86_cpu_def_class_init() suitable solution? >=20 >> >>> + env->cpuid_svm_features =3D def->svm_features; >>> + env->cpuid_ext4_features =3D def->ext4_features; >>> + env->cpuid_7_0_ebx_features =3D def->cpuid_7_0_ebx_features; >>> + env->cpuid_xlevel2 =3D def->xlevel2; >>> + >>> + object_property_set_str(OBJECT(cpu), def->model_id, "model-id", = NULL); >>> + >>> env->cpuid_apic_id =3D x86_cpu_apic_id_from_index(cs->cpu_index)= ; >>> =20 >>> /* init various static tables used in TCG mode */ >>> @@ -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", "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 =3D 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 an= d >>> + * 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 initiali= zed > before KVM "-cpu ?" we do not care what their defaults are, since we on= ly > would use class names there and then exit. >=20 > For case where classes could be inspected over QMP, OQM, KVM would be a= lready > initialized if enabled and we would get proper initialization order wit= hout > 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 a= nd > abort in initfn() if type was initialized without KVM present and KVM i= s > enabled at initfn() time. Something along the lines: >=20 > static x86_cpu_builtin_class_initialized_without_kvm; >=20 > x86_cpu_def_class_init() { > ... > if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kv= m) { > x86_cpu_builtin_class_initialized_without_kvm =3D true; > } > ... > } >=20 > 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 =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; >>> + } >>> + } >>> +} >>> + > [...] >=20 >>> --- 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 > [...] >=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 complete= ly >> useless anyway (because all data in the "host" class depend on data >> available only post-KVM-init anyway). >=20 > Looking at type_register() impl. it seems safe to do so + relying on QB= L for > type_table_add() safety. So it's really design question for QOM experts= . >=20 > 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg