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

[Andreas Färber]

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

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

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

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

>>>> Instead of adding hack, I'd rather enforce valid initialization order and
>>>> abort in initfn() if type was initialized without KVM present and KVM is
>>>> enabled at initfn() time. Something along the lines:
>>>>
>>>> static x86_cpu_builtin_class_initialized_without_kvm;
>>>>
>>>> x86_cpu_def_class_init() {
>>>>     ...
>>>>     if (!kvm_enabled() && !x86_cpu_builtin_class_initialized_without_kvm) {
>>>>         x86_cpu_builtin_class_initialized_without_kvm = true;
>>>>     }
>>>>     ...
>>>> }
>>>>
>>>> initfn() {
>>>>     ...
>>>>     if (kvm_enabled() && x86_cpu_builtin_class_initialized_without_kvm) {
>>>>         abort();
>>>>     }
>>>>     ...
>>>> }
>>>
>> Continuing on theoretical issue:
>>> We could add an inited field to X86CPUClass that gets checked at initfn
>>> time (only ever getting set to true by the pre-defined models). Then it
>>> would be per model. And if we add a prototype for the ..._class_init we
>>> could even call it late as proposed for -cpu host if we take some
>>              ^^^^^^^^^^^^ is a tricky part, for global properties to work it
>> would require, calling this hook after kvm_init() is succeeds and before
>> any initfn() is called in general or as minimum before Device.initfn(). And 
>> it
>> anyway will not make all CPU classes to have correct defaults in KVM mode,
>> since only CPU class of created CPU instance will be fixed up.
>>
>> 1. One way to make sure that built-in CPU classes have fixed up defaults is 
>> to
>> iterate over them in kvm_arch_init() and possibly calling their class_init()
>> again to reinitialize. It's still hack (due fixing something up), but it 
>> would
>> give at least correct KVM mode defaults, regardless of the order classes are
>> initialized.
> 
> Can't we do that more easily with the tcg-vendor/vendor properties?

> It looks we are burning too much brain cycles trying to force a model
> that's really unintuitive to the outside, where the default-value of a
> class property depends on the options given to the QEMU command-line. We
> don't have to do that.
>
> The point of initializing stuff in class_init is to make introspection
> easy. If we make the classes change how they look like depending on the
> command-line configuration, the classes and the class introspection
> system get less useful.

+1

>> 2. But more correct way from POV of OOP would be one without any fixups, i.e.
>> create extra KVM-builtin-CPU-classes that are derived from host class.
>> and in object_class_by_name() lookup for them if kvm is enabled. But we could
>> do this as follow-up to #1.
>>  
>>> precautions and add explanatory comments.
>>>
>>>>> If we still want the default vendor to be a static property in the
>>>>> class, we can do that if we set the default in a "tcg-vendor" property
>>>>> instead of a "vendor" property (that would be empty/unset by default),
>>>>> and x86_cpu_initfn() could do this:
>>>>>
>>>>>     vendor = object_property_get_str(cpu, "vendor");
>>>>>     tcg_vendor = object_property_get_str(cpu, "tcg-vendor");
>>>>>     if (vendor && vendor[0]) {
>>>>>       cpu->cpuid_vendor = vendor;
>>>>>     } else if (kvm_enabled()) {
>>>>>       cpu->cpuid_vendor = get_host_vendor();
>>>>>     } else {
>>>>>       cpu->cpuid_vendor = tcg_vendor;
>>>>>     }
>>>>>
>>>>>> +
>>>>>> +    /* Look for specific models that have the QEMU version in .model_id 
>>>>>> */
>>>>>> +    for (i = 0; i < ARRAY_SIZE(versioned_models); i++) {
>>>>>> +        if (strcmp(versioned_models[i], def->name) == 0) {
>>>>>> +            pstrcpy(xcc->info.model_id, sizeof(xcc->info.model_id),
>>>>>> +                    "QEMU Virtual CPU version ");
>>>>>> +            pstrcat(xcc->info.model_id, sizeof(xcc->info.model_id),
>>>>>> +                    qemu_get_version());
>>>>>> +            break;
>>>>>> +        }
>>>>>> +    }
>>>>>> +}
>>>>>> +
>>>> [...]
>>>>
>>>>>> --- a/target-i386/kvm.c
>>>>>> +++ b/target-i386/kvm.c
>>>>>> @@ -735,6 +735,69 @@ static int kvm_get_supported_msrs(KVMState *s)
>>>>>>      return ret;
>>>>>>  }
>>>>>>  
>>>> [...]
>>>>
>>>>>>  int kvm_arch_init(KVMState *s)
>>>>>>  {
>>>>>>      QemuOptsList *list = qemu_find_opts("machine");
>>>>>> @@ -743,6 +806,12 @@ int kvm_arch_init(KVMState *s)
>>>>>>      int ret;
>>>>>>      struct utsname utsname;
>>>>>>  
>>>>>> +    static const TypeInfo host_x86_cpu_type_info = {
>>>>>> +        .name = TYPE_HOST_X86_CPU,
>>>>>> +        .parent = TYPE_X86_CPU,
>>>>>> +        .class_init = kvm_host_cpu_class_init,
>>>>>> +    };
>>>>>> +
>>>>>>      ret = kvm_get_supported_msrs(s);
>>>>>>      if (ret < 0) {
>>>>>>          return ret;
>>>>>> @@ -797,6 +866,9 @@ int kvm_arch_init(KVMState *s)
>>>>>>              }
>>>>>>          }
>>>>>>      }
>>>>>> +
>>>>>> +    type_register(&host_x86_cpu_type_info);
>>>>>
>>>>> Are we really allowed to register QOM classes that late?
>>>>>
>>>>> If QOM design allows us to register the class very late (I would like to
>>>>> confirm that), this approach sounds really clean and sane to me.
>>>>> Pre-KVM-init class introspection of the "host" class would be completely
>>>>> useless anyway (because all data in the "host" class depend on data
>>>>> available only post-KVM-init anyway).
>>>>
>>>> Looking at type_register() impl. it seems safe to do so + relying on QBL 
>>>> for
>>>> type_table_add() safety. So it's really design question for QOM experts.
>>>>
>>>> Antnony, Paolo, Andreas
>>>>  what do you think?
>>>
>>> I already answered Eduardo on IRC that in general I see no restriction
>>> not to register a type late.
>>>
>>> The issue is that in this case we cannot rely on accessing the class
>>> from another class_init that is registered before it, which you were
>>> doing somewhere for hoc etc. (BTW please rename to host_oc if we go that
>>> route).
>> If we are accessing host class somewhere, then we would like to access its
>> initialized state, not a dummy state which gives us nothing.

No one is doubting that. It in turn means that either class_init may not
fail/skip parts, or we need to restrict access to such classes to a
mechanism that ensures they get fully initialized if they weren't.

> Absolutely.
> 
> (Just like the built-in classes, that should be always properly
> initialized by class_init.  ;-)

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