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

[Igor Mammedov]

On Tue, 12 Feb 2013 12:48:47 -0200
Eduardo Habkost <address@hidden> wrote:

> On Mon, Feb 11, 2013 at 02:52:49AM +0100, Igor Mammedov wrote:
> > On Fri, 8 Feb 2013 16:13:02 -0200
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Fri, Feb 08, 2013 at 05:54:50PM +0100, Andreas Färber wrote:
> > > > Am 08.02.2013 15:52, schrieb Eduardo Habkost:
> > > > > On Fri, Feb 08, 2013 at 01:58:42PM +0100, Igor Mammedov wrote:
> > > > >> On Fri, 08 Feb 2013 12:16:17 +0100
> > > > >> Andreas Färber <address@hidden> wrote:
> > > > >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > > > >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> > > > >>>> Eduardo Habkost <address@hidden> wrote:
> > > > >>>>
> > > > >>>>> On Tue, Feb 05, 2013 at 05:39:22PM +0100, Igor Mammedov wrote:
> > > > >>>>>> @@ -2236,6 +2083,44 @@ static void x86_cpu_initfn(Object *obj)
> > > > >>>>>>      }
> > > > >>>>>>  }
> > > > >>>>>>  
> > > > >>>>>> +static void x86_cpu_def_class_init(ObjectClass *oc, void
> > > > >>>>>> *data) +{
> > > > >>>>>> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> > > > >>>>>> +    ObjectClass *hoc =
> > > > >>>>>> object_class_by_name(TYPE_HOST_X86_CPU);
> > > > >>>>>> +    X86CPUClass *hostcc;
> > > > >>>>>> +    x86_def_t *def = data;
> > > > >>>>>> +    int i;
> > > > >>>>>> +    static const char *versioned_models[] = { "qemu32",
> > > > >>>>>> "qemu64", "athlon" }; +
> > > > >>>>>> +    memcpy(&xcc->info, def, sizeof(x86_def_t));
> > > > >>>>>> +
> > > > >>>>>> +    /* host cpu class is available if KVM is enabled,
> > > > >>>>>> +     * get kvm overrides from it */
> > > > >>>>>> +    if (hoc) {
> > > > >>>>>> +        hostcc = X86_CPU_CLASS(hoc);
> > > > >>>>>> +        /* 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
> > > > >>>>>> +         */
> > > > >>>>>> +        memcpy(xcc->info.vendor, hostcc->info.vendor,
> > > > >>>>>> +               sizeof(xcc->info.vendor));
> > > > >>>>>> +    }
> > > > >>>>>
> > > > >>>>> Again, we have the same problem we had before, but now in the
> > > > >>>>> non-host classes. What if class_init is called before KVM is
> > > > >>>>> initialized? I believe we will be forced to move this hack to
> > > > >>>>> the instance init function.
> > > > >>>> I believe, the in the case where non-host CPU classes might be
> > > > >>>> initialized before KVM "-cpu ?" we do not care what their
> > > > >>>> defaults are, since we only would use class names there and then
> > > > >>>> exit.
> > > > >>>>
> > > > >>>> For case where classes could be inspected over QMP, OQM, KVM
> > > > >>>> would be already initialized if enabled and we would get proper
> > > > >>>> initialization order without hack.
> > > > > 
> > > > > Who guarantees that KVM will be already initialized when we get a
> > > > > QMP monitor? We can't do that today because of limitations in the
> > > > > QEMU main code, but I believe we want to get rid of this limitation
> > > > > eventually, instead of making it harder to get rid of.
> > > > > 
> > > > > If we could initialize KVM before QMP is initialized, we could
> > > > > simply initialize KVM before class_init is called, instead. It
> > > > > would be easier to reason about, and it would make the limitations
> > > > > of our code very clear to anybody reading the code in main().
> > > > That wouldn't work (currently) due to -device and -object being
> > > > command line options just like -enable-kvm, -disable-kvm and -machine
> > > > accel=.
> > > 
> > > Well, we could loop over the command-line options twice.
> > > 
> > > It is just an alternative that would be better than making class_init
> > > unreliable. I don't think it would be a great solution anyway.
> > > 
> > > > 
> > > > >>>
> > > > >>> I think you're missing Eduardo's and my point:
> > > > >>>
> > > > >>> diff --git a/vl.c b/vl.c
> > > > >>> index a8dc73d..6b9378e 100644
> > > > >>> --- a/vl.c
> > > > >>> +++ b/vl.c
> > > > >>> @@ -2844,6 +2844,7 @@ int main(int argc, char **argv, char **envp)
> > > > >>>      }
> > > > >>>
> > > > >>>      module_call_init(MODULE_INIT_QOM);
> > > > >>> +    object_class_foreach(walkerfn, TYPE_OBJECT, false, NULL);
> > > > >>>
> > > > >>>      qemu_add_opts(&qemu_drive_opts);
> > > > >>>      qemu_add_opts(&qemu_chardev_opts);
> > > > >>>
> > > > >>> Anyone may iterate over QOM classes at any time after their type
> > > > >>> registration, which is before the first round of option parsing.
> > > > >>> Sometime later, after option parsing, there's the -cpu ? handling
> > > > >>> in vl.c:3854, then vl.c:4018:configure_accelerator().
> > > > >>>
> > > > >>> Like I said, mostly a theoretical issue today.
> > > > >> Question is if we should drop this theoretical issue for 1.5?
> > > > > 
> > > > > I wouldn't call it just theoretical. It is something that will
> > > > > surely hit us back. The people working on QMP or on the main() code
> > > > > 6 months from now will no idea that our class_init code is broken
> > > > > and will explode if class_init is called too early.
> > > > 
> > > > We should try to find a reliable solution here or at least add
> > > > appropriate comments to the module_call_init() call in vl.c.
> > > 
> > > Agreed. But I believe there are tons of other solutions that don't
> > > require making class_init broken.
> > > 
> > > 
> > > > 
> > > > >>> Originally I had considered making kvm_init() re-entrant and
> > > > >>> calling it from the offending class_init. But we must support the
> > > > >>> distro case of compiling with CONFIG_KVM but the user's hardware
> > > > >>> not supporting KVM or kvm module not being loaded or the user
> > > > >>> having insufficient priviledges to access /dev/kvm.
> > > > >> working without KVM is not issue, it just works with normal
> > > > >> defaults. Applying KVM specific defaults to already initialized
> > > > >> classes is.
> > > > 
> > > > Right, but applying KVM-specific defaults is much easier once KVM is
> > > > initialized. :)
> > > > 
> > > > > 
> > > > > 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.

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

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

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

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

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

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

> 
> 
> > 
> > 
> > > > 
> > > > Then we're pretty much back to my v3 plus Igor's error handling
> > > > change, right? Modulo whether to register the host class in
> > > > kvm_arch_init() or always.
> > > 
> > > I don't know, I would need to take a look at your v3. I don't remember
> > > how many of the code in this version was already in v3.
> > > 
> > > > 
> > > > > If "default vendor set in in built-in CPU model table" (TCG-only)
> > > > > has a different meaning from "vendor set by
> > > > > command-line/global-property" (affects TCG and KVM), it means we
> > > > > have two different knobs with two diferent semantics. Hence my
> > > > > suggestion of adding two properties: "tcg-vendor" and "vendor".
> > > > 
> > > > I don't quite understand why we would need a "tcg-vendor" property.
> > > > Are you trying to address setting or getting the value? If it's
> > > > setting we should just bypass the property in our internal code,
> > > > using Igor's vendor_str2words helper.
> > > 
> > > Just for setting (more exactly, just for carrying the TCG-only default
> > > values on the built-in classes, that are currently inside the x86_def_t
> > > table). The tricky part here is that we need to differentiate the
> > > default vendor value from class_init/x86_def_t that is valid only for
> > > TCG (on KVM, the default vendor is always the host CPU vendor), and
> > > "vendor" property set using global-properties/command-line is different
> > > and should be used on both TCG and KVM modes.
> > > 
> > > 
> > > 
> > > > 
> > > [...]
> > > > 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.

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

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

> 
> > 
> > > > 
> > > > 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?
> 
> 
> > > > 
> > > > Regards,
> > > > Andreas
> > > > 
> > > > -- 
> > > > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG
> > > > Nürnberg
> > > 
> > > -- 
> > > Eduardo
> > 
> > 
> > -- 
> > Regards,
> >   Igor
>