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? > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html