On Mon, Feb 11, 2013 at 02:52:49AM +0100, Igor Mammedov wrote:
> On Fri, 8 Feb 2013 16:13:02 -0200
> Eduardo Habkost <ehabk...@redhat.com> 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 <afaer...@suse.de> wrote:
> > > >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> > > >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> > > >>>> Eduardo Habkost <ehabk...@redhat.com> 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.
>
> 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.
> * 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.
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.
> * 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.
>
> 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).
>
> 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.
>
>
> > >
> > > 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.
>
> >
> > 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?
With my solution it is simple:
{ "486-x86_64-cpu", "tcg-vendor", "GenuineIntel" }
I don't see how you could do that with the current patch.
> 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.
>
> >
> > [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.
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.
>
> > >
> > > 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
--
Eduardo
