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

[Igor Mammedov]

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.

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. 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.
    * 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.
    * that pile of special cases will only grow with time, adding likes
      disable_pv_eoi() hooks and who knows what else.

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.

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.


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

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

> 
> 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. 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. Extra data in form of
class_init()s looks more preferable than complicated code/semantics.

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

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

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

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