Re: [Qemu-devel] [RFC v5] target-i386: Slim conversion to X86CPU subclasses + KVM subclasses

[Eduardo Habkost]

On Thu, Feb 14, 2013 at 04:13:22PM +0100, Igor Mammedov wrote:
> On Thu, 14 Feb 2013 09:44:59 -0200
> Eduardo Habkost <address@hidden> wrote:
[...]
> > > +    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)
> }

Yes, exactly.


[...]
> > > +
> > > +}
> > > +
> > > +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.

Having both TCG-specific and KVM-specific subclasses instead of making
the KVM class inherit from the TCG class would make sense to me, as we
may have TCG-specific behavior in other places too.

Or we could do memcpy() again on the KVM class_init function. Or we
could set a different void* pointer on the TCG class, with
already-filtered x86_def_t object. There are multiple possible
solutions.


> 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.

I'm OK with doing that in a separate patch, as it is not a bug fix or
behavior change. But it would be nice to do that before we introduce the
feature properties, to make the reported defaults right since the
beginning.

[...]
> > > +#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

model_id is static for a given QEMU version, isn't it? This may be OK
(but I am _also_ not sure about this case).


> * we generate sub-classes at runtime dynamically, which makes source level
>   introspection impossible for them.

It's not source-level introspection I am looking for, but runtime
introspection that won't surprise other components in the system by
changing unexpectedly (e.g. when the host CPU changes).

> 
> 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.

Yes, QOM introspection is run-time based. What I don't know is: what can
be the expectations about the stability of the class data that was just
introspected? How much this information can change when you run QEMU
again? How much can it change if you run the same QEMU binary on a
different host CPU? How much can it change when you run the same QEMU
binary using a different host kernel? How much can it change between
QEMU versions?

(I don't know the answer to any of those questions).


> But main point of putting defaults in class_init is because they are class
> wide, whether instance_init is for instance specific settings.

We're back to the difference between "property defaults" vs "how cpuid_*
will look like after instance_init is called". Property defaults are
"static data" (considering some definition of "static", at least) and
will always be in class_init, but there's no requirement that the value
of every single bit inside struct X86CPU has be initialized inside
class_init (otherwise instance_init wouldn't even exist!).

In other words "the value of X86CPU.cpuid_vendor after instance_init
runs" (that may depend on the host CPU) may be different from "the
default value of the 'vendor' property" (that could be statically set to
"host" if we choose that solution). The latter _must_ be defined by
class_init. The former is simply an internal field calculated by
instance_init, like lots of other X86CPU fields.

To give another example:

* In the future, we may have properties for reading CPU registers values;
* The EIP register is set to 0xfff0 on CPU reset;
* The CPU will probably be reset on instance_init;
* But that doesn't mean we have to set a 0xfff0 default for the
  "register-EIP" property on class_init.

I agree it is nice when we have properties that more closely match
what's the internal state of the object after instance_init. But _if_ we
have restrictions on how the defaults can change between runs of the
same QEMU binary (which I don't know yet), we may have to hide some
specifics behind an abstraction (like having vendor="host" meaning "use
the host CPU vendor").

> 
> 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

Thanks! I will take a look later.

> 
> > 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?

The problem is that the default value of the "vendor" property will
change when you run the same QEMU binary on a different CPU. I don't
know if we can do that, or not (see my question about "expections about
the stability of data" above).


> 
> > 
> > > +    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!

-- 
Eduardo