From patchwork Fri Feb 8 11:16:17 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Andreas_F=C3=A4rber?= X-Patchwork-Id: 219113 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 783BD2C0092 for ; Fri, 8 Feb 2013 22:16:50 +1100 (EST) Received: from localhost ([::1]:55999 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U3lwm-0000d5-JS for incoming@patchwork.ozlabs.org; Fri, 08 Feb 2013 06:16:48 -0500 Received: from eggs.gnu.org ([208.118.235.92]:47396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U3lwd-0000cn-VI for qemu-devel@nongnu.org; Fri, 08 Feb 2013 06:16:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U3lwa-0001mz-BJ for qemu-devel@nongnu.org; Fri, 08 Feb 2013 06:16:39 -0500 Received: from cantor2.suse.de ([195.135.220.15]:54665 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U3lwZ-0001mk-SA for qemu-devel@nongnu.org; Fri, 08 Feb 2013 06:16:36 -0500 Received: from relay1.suse.de (unknown [195.135.220.254]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx2.suse.de (Postfix) with ESMTP id 87397A3B99; Fri, 8 Feb 2013 12:16:34 +0100 (CET) Message-ID: <5114DE81.2000501@suse.de> Date: Fri, 08 Feb 2013 12:16:17 +0100 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Organization: SUSE LINUX Products GmbH User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130105 Thunderbird/17.0.2 MIME-Version: 1.0 To: Igor Mammedov 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> In-Reply-To: <20130208100329.2105a649@thinkpad.mammed.net> X-Enigmail-Version: 1.5 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.4.x X-Received-From: 195.135.220.15 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 Subject: Re: [Qemu-devel] [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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: >>> From: Andreas Färber >>> >>> 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 >>> 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 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: 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 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);