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

[Eduardo Habkost]

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