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

[Eduardo Habkost]

On Thu, Feb 14, 2013 at 08:16:32PM +0100, Igor Mammedov wrote:
> > [...]
> > > > > +
> > > > > +}
> > > > > +
> > > > > +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.
> 

We should keep the filtering on realize because of custom -cpu strings,
but if we filter on class_init too, we will make the class introspection
reflect reality more closely. Wasn't this one the main reasons you
argued (and convinced me) for having separate subclasses?

Also, if we do this we will be able to make "enforce" work for TCG too.
For example: if "enforce" worked for TCG today, people who want to work
around the existing n270 MOVBE bug could use "-cpu n270,+movbe,enforce"
to make sure the QEMU version they are using really accepts movbe.


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

Yes, I mean "constant" if by "constant" you mean "you can have
reasonable expectations that it won't change under certain conditions".
If the data you got from class introspection could change at any moment
without any warning, it would be random, useless information.

What I don't know is: what are reasonable conditions where the class
data may change, and what are the reasonable conditions where it should
_not_ change?


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

What if you run QEMU once for introspection/probing and then run it once
again? I believe the other components can expect the values won't change
just if you re-run the same QEMU binary.

This is not just about keeping it constant during a single QEMU proces
lifetime. Under certain conditions, the class data has to keep constant
between QEMU runs, too.


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

The data may change, yes, but I want to know when it is reasonable to
change this data.

I am sure of one thing: if somebody is running the same QEMU vesion on
the same system (same QEMU binary, same CPU, same kernel, same
hardware), the data absolutely shouldn't change between QEMU runs, or
the data would be useless for probing QEMU capabilities.

And I also think (but I am not sure) we _can_ change the defaults when
updating to a new QEMU binary. What I don't know is about the
intermediate cases (changing host CPU, changing host kernel, etc).


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

It's great when we can keep properties constant for a long time, after
the class is registered and even during instance_init. The problem is
that to reach this goal you are making class introspection report "less
constant" data (making it less useful and less reliable).


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

This is what I would like to avoid.


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

Well, it is "constant after class is registered" just because you want
to make it so. What I am proposing is to make vendor "variable" during
the instance_init() call. _If_ we can't change "vendor" in class_init
depending on the host CPU, we may _have to_ change it on instance_init,
we would have no other choice.

The "vendor" property is already variable between class registration and
the start of instance_init(), because it can be set using "-cpu
...,.vendor=...". I am just asking to allow it to change during a few
additional nanoseconds, while instance_init() runs. There are lots of
fields that can change during instance_init and even _after_
instance_init run. I really don't see what's the big deal in changing
the value of cpuid_vendor inside instance_init.

Objects are supposed to be dynamic, after all. You are trying to make
the object "more static" during instance_init (for a reason I don't
understand) by making the class (that is really supposed to be "more
static") change depending on the host CPU.


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

The current code would OK by now because the "vendor" property is not
static yet. But we will have a problem once we start registering static
properties, because static properties may have additional design
requirements.


> From my pov vendor="host" introduces only problems and is not obvious to users
> and isn't easy to use.

I believe vendor="host" is very obvious and easy to use, and I really
don't see any problem this would introduce. What I think is not obvious
and not intuitive is seeing QEMU report the existence of classes with
the same name but with different defaults, depending on the host CPU you
are running on.


> [...]

-- 
Eduardo