Re: [Qemu-devel] [PATCH 3/5] target-i386: Slim conversion to X86CPU subclasses

[Eduardo Habkost]

TL;DR: I still disagree about some points, but those points aren't so
relevant anymore because I am starting to like having
KVM-specific/TCG-specific subclasses (because of other problems that
would be solved by them).


On Wed, Feb 13, 2013 at 04:20:43PM +0100, Igor Mammedov wrote:
[...]
> > > > > > 
> > > > > > My big question is: why exactly we want to initialize this stuff
> > > > > > inside class_init? Can't we (please!) put the KVM-specific logic
> > > > > > inside instance_init?
> > > I see 2 issues with it:
> > > 1. a rather abstract introspection. defaults are belong to class data and
> > >    user who introspected class would expect to get CPU he saw during it,
> > > which he won't get if instance_init() will set another defaults, It will
> > > be already another CPU. So introspection becomes useless here.
> > 
> > Really, it does not become useless, it has very clear semantics. The
> > difference is that now the defaults won't depend on the kvm=on/off
> > configuration.
> >
> > I believe strongly that one of the purposes of class-based introspection
> > is to have introspectable _static_ data that do not depend on any
> > configuration data.
> It isn't static (look at model_id in class_init), I'd say it should be
> immutable after class is initialized, which means it could be determined
> dynamically in class_init.

That's were I believe we disagree. It's not about being immutable, it's
about being introspectable before anything is configured.

We could change the values between QEMU versions (it would be nice to
avoid it, but not a must), but we can't make depend on configuration
data unless we find a solution to the dependency/ordering problem.

Note that I would be happy enough if the QEMU maintainers decided that
CPUs are special for QEMU and the CPU classes must be initialized very
late, or that accelerator initialization is special and can happen
before any class is initialized. But we must make an explicit decision
about that, if we want to. That's why I listed the posible options I
see, below (after the paragraph I say "I believe the current dependency
chain is").


> > > 
> > > 2. more close to code:
> > >     * vendor property, you offering to add a new tcg-vendor property. If
> > > we are dumping goal [1] then it might work.
> > 
> > We're now dumping goal [1], we would be enabling it to be achieved,
> > because the default values of "vendor" and "tcg-vendor" would be
> > completely static.
> > 
> > The assumption you seem to be unwilling to drop is that cpuid_vendor
> > should always unconditionally correspond to the value of the "vendor"
> > property set by the class. But it doesn't have to. The CPU class may
> > choose to do anything with cpuid_vendor on instance_init, including
> > using the "tcg-vendor" property to set it if KVM is disabled and
> > "vendor" is empty.
> > 
> > 
> > > But that new property is just
> > >       another reincarnation of vendor_override field in CPUState that
> > > we've just gotten rid of and brings back hack of selecting what guest os
> > > will see.
> > 
> > In a way, yes. But it looks like it is necessary. But at least is a very
> > understandable model that can be seen from the outside. "tcg-vendor" is
> > obviously tcg-only, and "vendor" overrides everything if set.
> I'm not sure it's necessary and I dislike adding extra semantics to CPU
> unless it's how it's made in real hardware or we have to due to lack of
> another way to implement it correctly. CPUID has only one 'vendor' so lets
> try to make it work instead of working around deficiencies of current
> modeling.

We are not adding "extra" semantics, we are just modelling the existing
(weird) KVM vs. TCG semantics, unfortunately. The class model is not
just a hardware model, but a "configuration data" model.

AFAIU, not every property is about modelling hardware, but also about
modelling the knobs we ahve to configure the software. The block device
"cache" option is not about modelling hardware, the virtio-net "timer"
option is not about modelling hardware.

So, if we have to keep the weird semantics, the point is that the weird
semantics belong to instance_init and not class_init (because of the
ordering/dependency problem we have).

> 
> > 
> > 
> > >     * cpuid_kvm_features defaults also different for KVM and TCG. Which
> > > also makes 2 different CPUs, and makes guest behave differently.
> > >       If we set defaults in instance_init(), we would loose possibility
> > > to use global(cmd line) and compat(pv_eoi) properties with respective
> > >       feature bits or invent another special case to detect that
> > >       global/compat properties were used and workaround it.
> > 
> > I don't propose setting property defaults on instance_init. I propose
> > using the _static_ defaults set by class_init to initialize the CPU
> > state on instance_init. Just like nobody said that the CPU _must_
> > initialize cpuid_vendor using the "vendor" property only, nobody said
> > that the CPU can't set cpuid_kvm_features to zero on instance_init if
> > KVM is disabled.
> whether defaults are static or dynamic doesn't matter, it doesn't change
> the fact that it's defaults nonetheless. And setting one defaults in
> class_init and others in instance_init reminds of the split brain syndrome.
> It's much better to do it one place if possible.

It would be wonderful if we could. But we have the dependency/ordering
problem.

> kvm_features is just simplified case of 'vendor' with only 2 default states.
> It could be workarounded by clearing it in instance_init() in TCG mode, but
> that would mean:
>  1. reporting this features as available at class-level when
>     they aren't available for this class in TCG mode. which doesn't look
>     correct.

We have lots of features that may be reported as available but can't be
enabled because of lack of host support.

>  2. clearing would silently override whatever user set via global properties
>     which isn't ok even if user shoots in his own leg setting them

That's what we already do on TCG today, for every single feature. The
KVM features are not an exception, they are just following the same
pattern: TCG code silently disables the features that are not supported
by TCG. See the usage of TCG_FEATURES on x86_cpu_realize().

But you have a point here: if TCG behavior is so so weird regarding
_every_ feature (not just the KVM ones!), maybe splitting in two class
hierarchies will be simpler. I still would like to avoid it if possible,
but maybe it is a reasonable solution.


> 
> > 
> > You seem to be confusing "how the CPU object struct fields will look
> > like just after instance_init is called" with "property defaults". They
> > are different things.
> I think it should be equal to property defaults if there weren't
> compat/global properties provided/applied to alter it. From my POV,
> instance_init is for initializing instance specific extra state based
> on defaults it got before it was called. (whether vendor and kvm_features
> defaults are class wide settings).

instance_init is where we implement things that can't be represented as
simple data and needs code. My reasoning/assumptions are:

0) We can't check kvm_enabled() on class_init (because of the
   ordering/dependency problem);
1) class_init is the only place we can initialize QOM-property defaults
   before instance_init is called.
2) instance_init is the only place we can check kvm_enabled().

So, the only solutions I see that match all the requirements above are:

1) Having separate TCG-specific/KVM-specific properties because we
   actually have two different knobs/defaults for KVM and TCG;
2) Having separate subclasses for KVM and TCG;
3) Change the ordering/dependency requirements to allow class_init to
   depend on kvm_enabled.


> 
> > 
> > 
> > >     * that pile of special cases will only grow with time, adding likes
> > >       disable_pv_eoi() hooks and who knows what else.
> > 
> > I don't see how disable_pv_eoi() is related to that. disable_pv_eoi()
> > can easily be converted to global properties once we implement feature
> > properties. instance_init can then simply zero all KVM features on
> > instance_init if KVM is disabled.
> For the price of showing wrong defaults at class level for TCG mode CPU.

True. But as I said above, this is already the case for every other
feature that's not inside TCG_FEATURES/TCG_EXT_FEATURES/etc.

(But as I said above, this is a point for making separate subclasses)

> 
> > 
> > > 
> > > To use benefits provided by static properties we need to follow rule that
> > > defaults are not set in instance_init(), instead of they should be set
> > > using defaults of static properties. Unconditional calls to set them in
> > > this patch could be eventually converted to class_init() defaults. But
> > > kvm_enabled() conditioned ones like vendor and cpuid_kvm_features, rise
> > > chicken or egg problem we are trying to solve.
> > 
> > We _are_ setting defaults on class_init, the difference is that
> > instance_init choose which property to obey ("vendor" or "tcg-vendor")
> > depending on configuration. Doing that would _fix_ global properties,
> > not break it (because now we can change the tcg-only default-vendor of a
> > CPU model using global properties easily. See my "486" example/question
> > below).
> Why fix anything if it could be done earlier without fixing ever.

I couldn't parse what you meant here.

The current solution that checks kvm_enabled() in class_init doesn't
work with global properties. It doesn't allow us to set compat global
properties to allow us to change the default vendor only in TCG mode
(see the "486" example).

> 
> > 
> > > 
> > > From reading above I've got that fixing up built-in CPUs class data is not
> > > perfect idea and might not work after all.
> > > Then lets consider alternative of creating a bunch of KVM-based built-in
> > > CPU sub-classes that are added at kvm_init() time.
> > 
> > Separate KVM-based subclasses would work, too, but it looks like
> > overkill if we can simply have two properties.
> I prefer KVM-based subclasses correctness and uniform handling of defaults
> without exceptions with clear properties semantic to splitting defaults
> handling to two places and working around limitations it incurs (introducing
> properties not existing in real hw along the way, without must have need in
> it).

I understand.

Just to make it clear: I am not against having separate TCG/KVM
subclasses, I just would like to avoid them if possible.

Checking kvm_enabled() in class_init, on the other hand, isn't just
something I would like to avoid, it's something that I believe is broken
(unless we decide to change the main code ordering/dependencies).

[...]
> > > > > I've been thinking about whether this is a more general issue that we
> > > > > could solve at QOM level, like the base_class_init for qdev props,
> > > > > but I haven't come up with a usable idea. (Paolo?)
> > > > 
> > > > I don't see any simple solution involving extending the QOM design,
> > > > except that we need to decide on a general dependency/layering model and
> > > > stick with it instead of trying to break the model without admitting
> > > > we're breaking it.
> > > > 
> > > > I believe the current dependency chain is:
> > > > 
> > > >  - class data initialization
> > > >     -> handling of configuration options
> > > >        -> accelerator initialization
> > > >           -> machine and CPU instances initialization
> > > > 
> > > > That means:
> > > > 
> > > >  - Handling of configuration options (via command-line, QMP, config
> > > >    file, whatever method) need classes to be initialized first.
> > > >  - Accelerator initialization depends on config options to be handled
> > > >    first.
> > > >  - Hence, we can't make class data depend on accelerator initialization.
> > > >    Period.
> > > > 
> > > > 
> > > > What we _could_ do (but I don't think is the best solution) is:
> > > > 
> > > >  - handling of accelerator options
> > > >    -> accelerator initialization
> > > >       -> class data
> > > >          -> handling of remaining configuration options
> > > >             -> machine and CPU instances initialization
> > > > 
> > > > Such a change would be introsive, so I believe we can try to fix it by
> > > > simply changing our CPU class model so that class data don't depend on
> > > > accelerator data[1].
> > > > 
> > > > An intermediate solution could be registering and initializing all the
> > > > X86CPU classes later. e.g.:
> > > > 
> > > >  - General class data initialization
> > > >    -> handling of configuration options
> > > >       -> accelerator initialization
> > > >          -> X86CPU class data initialization
> > > >             -> machine and CPU instances initialization
> > > +1 if KVM based sub-classes idea is not acceptable, this could work for me
> > > as compromise provided defaults are initialized in class_init()
> > > 
> > > > 
> > > > The problem with this approach is that it would be impossible to
> > > > implement "-cpu ?" and "query-cpu-definitions" without at least handling
> > > "-cpu ?" can be moved after kvm_init() and we could put comment that
> > > QMP should be accessed after it.
> > 
> > No, we can't. QMP should be able to handle the "handling of
> > configuration options" part. And "-cpu ?"/query-cpu-definitions doesn't
> > require the KVM option to be set.
> > 
> > > 
> > > > the accelerator configuration options first. This would probably break
> > > > (or require hacks for) "-device" too.
> > > Why break? -device cpu doesn't work now and I'm not sure it should/would
> > > in oversee-able future.
> > 
> > Well, it doesn't work today, but we plan to make it work, right? So we
> > must choose a design that allows us to make it work instead of making
> > our life difficult in the near future. This solution has "handling of
> > configuration options" before "X86CPU class data initialization", so it
> > will prevent us from making -device work with CPUs.
> > 
> > We are making huge efforts to make CPUs devices like any other, I
> > believe we should avoid introducing _additional_ exceptions and
> > special-cases for the CPU devices.
> Agreed. That looks like class dependence issue. Where kmv should be enabled
> explicitly before any -device are handled or -device should gracefully fail
> if not kvm present and kvm dependent CPU is requested. Or implicitly which
> would require introducing class dependencies and their implicit
> initialization.
> 
> > 
> > > 
> > > > 
> > > > The latter is already the solution we are trying for "-cpu host", but at
> > > > least we know that "-cpu host" is special and we can work around it in
> > > > the query-cpu-definitions and "-cpu ?" code[2].
> > > > 
> > > > 
> > > > [1] My main point is: we're trying to force accelerator-provided data to
> > > >     be stored in the class, when we really don't need that data to be
> > > >     stored in the class. We just need property-value semantics that make
> > > >     the object use the accelerator-provided data later, but without
> > > >     storing the accelerator-provided data itself inside the class.
> > > that complicates property handling a lot to a point of not able to use
> > > global/compat properties.
> > 
> > I believe it _simplifies_ property handling a lot and allows us to
> > easily use global/compat properties even if we want a KVM-only
> > global-property.
> > 
> > e.g. assume we made a mistake and we have an Intel CPU (e.g. 486) with
> > vendor=AMD in the builtin CPU list. With your solution, how would you
> > set global properties to fix this and make 486 have vendor=Intel without
> > affecting KVM at all?
> global_properties isn't for fixing, but lets imagine we do it:
> > 
> > With my solution it is simple:
> > 
> >  { "486-x86_64-cpu", "tcg-vendor", "GenuineIntel" }
> It would be:
>  { "486-x86_64-cpu", "vendor", "GenuineIntel" }
> 
> > 
> > I don't see how you could do that with the current patch.
> Current patch doesn't support static properties and their features.
> We need this intermediate subclasses + convertion to static properties +
> moving setting defaults from instance_init to class_init using static
> properties default values approach.

Replace "current patch" with "a property-based solution that involves
checking kvm_enabled() in class_init before initializing the vendor
property".

> 
> > 
> > 
> > > Setting defaults in class_init() using static
> > > property defaults doesn't have such limitations + keeps property semantics
> > > simple and working the same way regardless accelerator.
> > 
> > This is true. And making a default that depends on kvm_enabled() is
> > what? A _non-static_ property default.
> > 
> > 
> > > Extra data in form of
> > > class_init()s looks more preferable than complicated code/semantics.
> > 
> > Exactly! That's why a simple tcg-vendor/vendor property pair is simple
> > and straightforward. Checking the KVM configuration in class_init is
> > what makes the semantics more complicated.
> > 
> > class_init initializes static data that shouldn't depend on
> > initialization of other parts of QEMU. If we want any extra logic, it
> > belongs to instance_init.
> I can't agree with this, my POV on claas_init vs instance_init is stated
> earlier.
> And if type requires other parts of qemu to be correctly intialized, it would
> mean that it should be initialized even registered after that parts are
> initialized.
> If something attempts to access it before than that something should be fixed
> not type.
> 

I don't think that's how QOM is designed. But you don't need to convince
me of this, you need to convince the maintainers of QOM, QMP, and the
main() code.

> > 
> > > 
> > > > 
> > > > [2] It is possible to fix the problem with "-cpu host" later if we
> > > >     decide on property/value semantics that allow the "the CPU will
> > > >     simply copy features from the host when initialized" mode to be
> > > >     expressed in the class (without requiring KVM to be initialized
> > > >     first).
> > > > 
> > > >     A possible solution is having a "copy-all-host-features=yes"
> > > >     property in the -cpu host class. But that would probably break "-cpu
> > > >     host,+foo,-bar".
> > > > 
> > > >     Another solution would be to make the "f-*" flags tristate,
> > > >     accepting "on", "off" and "host". The host class could then simply
> > > >     set the default for all feature properties to "host", and the
> > > >     instance init function (not class_init) would understand that "host"
> > > >     means "ask KVM for host features".
> > > > 
> > > > 
> > > > > 
> > > > > I agree with Eduardo that we should not try to override class values
> > > > > based on accel=kvm. Global properties don't operate on classes but on
> > > > > the properties, which get set up at device-instance_init time only.
> > > > > If there's an issue with the vendor it seems easier to fix that than
> > > > > to play games with class_init as we seem to be going in circles...
> > > properties definitions are part of classes (including properties
> > > defaults), and defaults are set before global properties at the same
> > > place.
> > 
> > Exactly. And if we make class data depend on configuration option
> > parsing, we are making CPUs require lots of exceptions in the QEMU
> > main() code.
> I'll send a RFC using KVM based sublcasses that won't impose additional
> dependency compared to current code.

Cool. :-)

>  
> > Please don't confuse "default value for the 'vendor' property" with "how
> > the cpuid_vendor struct field will look like just after we called
> > instance_init". They can be different things. I am arguing that the
> > former should be static, but the latter can look different depending on
> > kvm_enabled().
> > 
> > 
> > > It's not only
> > > vendor, setting default cpuid_kvm_features in x86_cpu_initfn() will
> > > overwrite anything set before via global properties (i.e. +foo,-foo).
> > 
> > It won't, if we have two properties (tcg-vendor/vendor). And in the case
> > of cpuid_kvm_features, we can simply zero it if kvm_enabled() is false,
> > and that's it.
> > 
> > See my question about changing the "486" vendor above. 
> >
> > > 
> > > > > 
> > > > > That would leave us with the problematic -cpu host class and with
> > > > > analyzing any remaining instance_init problems.
> > > Than lets double built-in CPUs with KVM bases sub-classes. That well give
> > > us constant classes that reliably describe KVM specific and reduce amount
> > > of kvm_enabled() conditions in initfn(), hence later allow to replace
> > > setting defaults with static properties (making initfn() even more
> > > simpler).
> > 
> > It would work, but why the extra complexity? We just need two very small
> > additional properties, and that's it. Because the vendor string is the
> > only part where KVM differ from TCG. The KVM feature list doesn't matter
> > because enabling a KVM feature on a TCG feature has no effect at all.
> Because it's more correct, doesn't require workarounds in instance_init(),
> doesn't introduce extra (fake)properties/interfaces with related extra
> semantic to maintain, allows to treat all properties uniformly without
> exceptions, doesn't break class-level introspection ( it really reflects
> default state of properties for a class in question), not more complex in
> terms amount of code than extra ifs in instance_init() and extra properties.

I am becoming more inclined for the separate-subclasses solution because
of the other TCG-specific stuff we have in the code today (the silent
TCG_FEATURES filtering, specifically). With separate subclasses we could
report simply clear TCG_FEATURES in the property default-values
themselves (and not so late in the initialization). In practice we
_already_ have different TCG and KVM CPU models because of the
silent TCG feature-filtering.

I plan to take a look at your subclass-based patch tomorrow.


> 
> > 
> > > 
> > > > > 
> > > > > And not to forget -object and, once Anthony drops his unloved no_user
> > > > > flag, -device.
> > > And KVM based sub-classes won't break it, they actually wouldn't be
> > > available till kvm_init() is completed successfully.
> > > I understand -object and -device are more like API calls that couldn't be
> > > called arbitrarily, one has to provide dependencies first. So something
> > > like -object kvm should go before -object kvm-cpu if caller expects it to
> > > work.
> > > 
> > 
> > KVM-based subclasses would work. A solution requiring a "KVM" object to
> > be created first would work, too.
> > 
> > But I still think we can simply handle KVM-specific behavior in
> > instance_init (using a separate tcg-specific and kvm-specific properties
> > when their defaults need to be different) instead of multiplying our
> > number of classes by 2.
> > 
> > It's all about how we want to model this to the outside: when we
> > introduced vhost, did we introduce additional classes for vhost-based
> > configuration? What about the defaults for vhost-specific options? How
> > are they set? Why can't we follow the same pattern for CPUs?
> > 
> > 

-- 
Eduardo