[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclas
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses |
Date: |
Thu, 14 Feb 2013 16:13:22 +0100 |
On Thu, 14 Feb 2013 09:44:59 -0200
Eduardo Habkost <address@hidden> wrote:
> On Wed, Feb 13, 2013 at 04:33:33PM +0100, Igor Mammedov wrote:
> > From: Andreas Färber <address@hidden>
> >
> > 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 <address@hidden>
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > 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!
- [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Igor Mammedov, 2013/02/13
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Eduardo Habkost, 2013/02/14
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses,
Igor Mammedov <=
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Eduardo Habkost, 2013/02/14
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Igor Mammedov, 2013/02/14
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Eduardo Habkost, 2013/02/14
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Igor Mammedov, 2013/02/15
- Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses, Eduardo Habkost, 2013/02/15