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

[Igor Mammedov]

On Thu, 14 Feb 2013 13:52:59 -0200
Eduardo Habkost <address@hidden> wrote:

> 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.
It's behavior change, if we move filtering from realizefn to class_init, user
would be able to add features not visible now to guest.

ordering now:
class_init, initfn, setting defautls, custom features, realizefn(filtering)

would be ordering with static properties:
clas_init, static props defaults, global properties(custom features),
x86_cpu_initfn, realizefn

in would be scenario filtering comes before custom features, which is not
necessarily bad and may be nice to consciously add/test features before
enabling them in filter for everyone, but it's behavior change.

> [...]
> > > > +#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).
I guess you mean constant when using static. 

> 
> 
> > * 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).
And I do not think that it must be constant until class is registered.
Actually as a user during class introspection, I'd like to see vendor
value that will be used, not a placeholder 'host', so I won't have to
guess/get from somewhere else what that 'host' actually means.

> 
> > 
> > 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).
I don't know answers to these Q either, it probably will be individual on per
case basis. But in general due to introspection being runtime why should we 
corner us to only constants, I'd expect that data might change for above cases
between different qemu runs, it should reflect environment we are running on.
for 'cpuid.vendor' it is current behavior, and I don't see the reason why we
should change it.

> 
> 
> > 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.
No, I'm not. I'm not talking about every field in X86CPU. CPUID is
architectural constant/default depending on CPU model. In case of KVM
CPUID.vendor is just another constant values from POV of consumer. And I
propose to treat it the same way as the other defaults (i.e. set it in
class_init). (i.e. no special casing unless we have to)
Yes it can change if you run qemu on different hosts, but it won't do so if
you run it on the same host.

> 
> 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.
It's not valid comparison, register is 'VARIABLE' when CPUID is 'CONSANT'.
PS: 'constant' here means constant after class is registered.

> 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").
Well, I'm no aware about such restrictions. But I'm planing re-factoring with
keeping current behavior, unless there is reasons to change it.
Presently cpu_x86_find_by_name() that overrides 'vendor' is called before
object_new(), and it's sort of class_init(), so I'm trying fit it to new model
and do it uniformly for all cpuid properties. I don't see why 'vendor' is any
different from other properties, when you get x86_cpu_def from
cpu_x86_find_by_name() it already has correct vendor values.
From my pov vendor="host" introduces only problems and is not obvious to users
and isn't easy to use.
 
> 
> > 
> > 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
> 


-- 
Regards,
  Igor