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 <afaerber@xxxxxxx> wrote:
> >>> Am 08.02.2013 10:03, schrieb Igor Mammedov:
> >>>> On Thu, 7 Feb 2013 13:08:19 -0200
> >>>> Eduardo Habkost <ehabkost@xxxxxxxxxx> 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?
>
> 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
The problem with this approach is that it would be impossible to
implement "-cpu ?" and "query-cpu-definitions" without at least handling
the accelerator configuration options first. This would probably break
(or require hacks for) "-device" too.
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.
[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...
>
> That would leave us with the problematic -cpu host class and with
> analyzing any remaining instance_init problems.
>
> And not to forget -object and, once Anthony drops his unloved no_user
> flag, -device.
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
