On Thu, 14 Feb 2013 09:44:59 -0200 Eduardo Habkost <ehabk...@redhat.com> wrote:
> On Wed, Feb 13, 2013 at 04:33:33PM +0100, Igor Mammedov wrote: > > From: Andreas Färber <afaer...@suse.de> > > > > Depends on > > http://lists.gnu.org/archive/html/qemu-devel/2013-02/msg00677.html > > > > 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(). > > > > Move default setting of CPUID_EXT_HYPERVISOR to class_init(). > > > > Move KVM specific built-in CPU defaults overrides in a kvm specific > > x86_cpu_kvm_def_class_init(). And select TCG vs KVM class of CPU > > to create at runtime in x86_cpu_class_by_name() when kvm_enable() > > is available. > > > > Inline cpu_x86_register() into the X86CPU initfn. > > Since instance_init cannot reports errors, die there if some > > of default values are incorrect, instead of ignoring errors. > > > > 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-kvm-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs > > and only when KVM is enabled to avoid workarounds in name to class > > lookup code in x86_cpu_class_by_name(). > > 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-kvm-{i386,86_64}-cpu > > for comparison. > > > > Signed-off-by: Andreas Färber <afaer...@suse.de> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > --- > > v5: > > * 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. > > * register KVM specific subclasses for built-in CPU models. > > * abort() in instance_init() if property setter fails to set default > > value. > > 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. > > --- > > target-i386/cpu-qom.h | 24 ++++ > > target-i386/cpu.c | 348 > > +++++++++++++++++++------------------------------ target-i386/cpu.h > > | 5 +- target-i386/kvm.c | 72 ++++++++++ > > 4 files changed, 232 insertions(+), 217 deletions(-) > > > [...] > > diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h > > index 48e6b54..c8f320d 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-kvm-" TYPE_X86_CPU > > + > [...] > > if (name == NULL) { > > - return -1; > > - } > > - if (kvm_enabled() && strcmp(name, "host") == 0) { > > - kvm_cpu_fill_host(x86_cpu_def); > > - return 0; > > + return NULL; > > } > > > > - for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) { > > - def = &builtin_x86_defs[i]; > > - if (strcmp(name, def->name) == 0) { > > - memcpy(x86_cpu_def, def, sizeof(*def)); > > - /* 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 > > - */ > > - if (kvm_enabled()) { > > - uint32_t ebx = 0, ecx = 0, edx = 0; > > - host_cpuid(0, 0, NULL, &ebx, &ecx, &edx); > > - x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx, > > ecx); > > - } > > - return 0; > > - } > > + if (kvm_enabled()) { > > + typename = g_strdup_printf("%s-kvm-" TYPE_X86_CPU, name); > > * Could we use the "-tcg" suffix for the non-KVM mode? sure, I'll add it for the next re-spin. > * Can we make the code fall back to no-suffix CPU names? We need to add > the -kvm suffix (and maybe a -tcg suffix) to keep compatibility with > existing CPU models, but maybe we should deprecate the automatic > -tcg/-kvm suffixing and ask users to specify the full CPU name. I'm not sure that I got you right, Have you meant something like this: if (kvm) { typename = name-kvm-... } else { typename = name-tcg-... } oc = object_class_by_name(typename) if (!oc) { oc = object_class_by_name(name) } > > > [...] > > @@ -2234,7 +2075,57 @@ static void x86_cpu_initfn(Object *obj) > > cpu_set_debug_excp_handler(breakpoint_handler); > > #endif > > } > > + > > + if (error) { > > + fprintf(stderr, "%s\n", error_get_pretty(error)); > > + error_free(error); > > + abort(); > > + } > > Good idea, we should have done that a long time ago, to avoid surprises > if one day the property setting fails. > > > + > > +} > > + > > +static void x86_cpu_def_class_init(ObjectClass *oc, void *data) > > +{ > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > + x86_def_t *def = data; > > + int i; > > + static const char *versioned_models[] = { "qemu32", "qemu64", > > "athlon" }; + > > + memcpy(&xcc->info, def, sizeof(x86_def_t)); > > + xcc->info.ext_features |= CPUID_EXT_HYPERVISOR; > > If this is TCG-specific now, we could make the class match the reality > and mask TCG_FEATURES/TCG_EXT_FEATURES/etc here. That's what happens in > practice, e.g.: "-cpu SandyBridge" in TCG mode is a completely different > CPU, today. well, this function is shared between TCG and KVM, I mean, it's common code for both. Which asks for one more TCG class_init function for TCG specific behavior. But could it be a separate patch (i.e. fixing TCG filtering), I think just moving tcg filtering might cause regression. And need to worked on separately. > > + > > + /* 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; > > + } > > + } > > +} > > + > > +#ifdef CONFIG_KVM > > +static void x86_cpu_kvm_def_class_init(ObjectClass *oc, void *data) > > +{ > > + uint32_t eax, ebx, ecx, edx; > > + X86CPUClass *xcc = X86_CPU_CLASS(oc); > > + > > + x86_cpu_def_class_init(oc, data); > > + /* 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 > > + */ > > + host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx); > > Cool, this is exactly what I was going to suggest, to avoid depending on > the "host" CPU class and KVM initialization. > > I see two options when making the "vendor" property static, and I don't > know which one is preferable: > > One solution is the one in this patch: to call host_cpuid() here in > class_init, and have a different property default depending on the host > CPU. I prefer this one ^^^. > Another solution is to have default to vendor="host", and have > instance_init understand vendor="host" as "use the host CPU vendor". > This would make the property default really static (not depending on the > host CPU). > > I am inclined for the latter, because I am assuming that the QOM design > expects class_init results to be completely static. This is not as bad > as depending on KVM initialization, but it may be an unexpected > surprise, or even something not allowed by QOM. That's where I have to disagree :) You might have a point with 'static' if our goal is for introspection for source level to work. But we have a number of issues with that: * model_id is not static for some cpus, 'vendor' is the same just for different set of classes * we generate sub-classes at runtime dynamically, which makes source level introspection impossible for them. I think that QOM is aiming towards run-time introspection of classes/objects. And that allows us to define defaults (even generate them dynamically) at class_init() time, they don't have to be static constants. But main point of putting defaults in class_init is because they are class wide, whether instance_init is for instance specific settings. Anthony probably could judge us and suggest which way to move. And I intend to use this feature with static properties defaults after static properties + subclasses are in tree: "Dynamically create (copy) static properties definitions for CPU subclass" https://github.com/imammedo/qemu/commit/ed506d354688ea00212cf5f76caf08932c20d0aa "set per subclass defaults of static properties in class_init()" https://github.com/imammedo/qemu/commit/a48e252a2800bf8dd56320e68e4f9517d0a25e5c Whole tree to play with is here: https://github.com/imammedo/qemu/commits/x86-cpu-hot-add.WIP > This doesn't matter today, because there's no "vendor" property default > being set here, but it may be an issue when we introduce the static > properties. Could you describe what problems it creates after we have static properties? > > > + x86_cpu_vendor_words2str(xcc->info.vendor, ebx, edx, ecx); > > + > > + xcc->info.kvm_features |= kvm_default_features; > > } > > +#endif > > > > static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > > { > > @@ -2247,6 +2138,28 @@ static void x86_cpu_common_class_init(ObjectClass > > *oc, void *data) > > xcc->parent_reset = cc->reset; > > cc->reset = x86_cpu_reset; > > + > > + cc->class_by_name = x86_cpu_class_by_name; > > +} > > + > > +static void x86_register_cpu_type(const x86_def_t *def) > > +{ > > + TypeInfo type_info = { > > + .parent = TYPE_X86_CPU, > > + .class_init = x86_cpu_def_class_init, > > + .class_data = (void *)def, > > + }; > > + > > + type_info.name = g_strdup_printf("%s-" TYPE_X86_CPU, def->name); > > + type_register(&type_info); > > + g_free((void *)type_info.name); > > + > > +#ifdef CONFIG_KVM > > + type_info.name = g_strdup_printf("%s-kvm-" TYPE_X86_CPU, def->name); > > + type_info.class_init = x86_cpu_kvm_def_class_init; > > + type_register(&type_info); > > + g_free((void *)type_info.name); > > +#endif > > Like I said above, I would prefer to have both "-tcg" and "-kvm" > suffixes. sure, np. > > > [...] > > Overall, I really like how simple this solution ended up. I had issues > with "class hierarchy explosion", but as I said before: in practice we > already have different CPU models in TCG and KVM mode, because of the > silent feature-masking done in TCG mode. So it just makes sense to have > those different CPU models represented by different classes. > Thanks for reviewing!
