Re: [Qemu-devel] [PATCH qom-cpu-next v3 3/4] target-i386: Slim conversion to X86CPU subclasses

[Igor Mammedov]

On Mon, 04 Feb 2013 13:52:32 +0100
Andreas Färber <address@hidden> wrote:

> Am 04.02.2013 12:08, schrieb Igor Mammedov:
> > On Sat,  2 Feb 2013 01:37:07 +0100
> > Andreas Färber <address@hidden> wrote:
> > 
> >> Move x86_def_t definition to header and embed into X86CPUClass.
> >> Register types per built-in model definition.
> >>
> >> Move version initialization from x86_cpudef_setup() to class_init.
> >>
> >> Inline cpu_x86_register() into the X86CPU initfn.
> >> Since instance_init cannot reports errors, drop error handling.
> >>
> >> Replace cpu_x86_find_by_name() with x86_cpu_class_by_name().
> >> Move KVM host vendor override from cpu_x86_find_by_name() to the initfn.
> >>
> >> Register host-{i386,x86_64}-cpu type from KVM code to avoid #ifdefs.
> >> Make kvm_cpu_fill_host() a class_init and inline cpu_x86_fill_model_id().
> >>
> >> Let kvm_check_features_against_host() obtain host-{i386,86_64}-cpu for
> >> comparison.
> >>
> >> Signed-off-by: Andreas Färber <address@hidden>
> >> ---
> >>  target-i386/cpu-qom.h |   24 ++++
> >>  target-i386/cpu.c     |  324
> >> +++++++++++++++++-------------------------------- target-i386/cpu.h
> >> |    2 - target-i386/kvm.c     |   93 ++++++++++++++
> >>  4 Dateien geändert, 228 Zeilen hinzugefügt(+), 215 Zeilen entfernt(-)
> >>
> >> diff --git a/target-i386/cpu-qom.h b/target-i386/cpu-qom.h
> >> index 48e6b54..80bf72d 100644
> >> --- a/target-i386/cpu-qom.h
> >> +++ b/target-i386/cpu-qom.h
[...]

> >>   * An x86 CPU model or family.
> >>   */
> >> @@ -51,6 +73,8 @@ typedef struct X86CPUClass {
> >>  
> >>      DeviceRealize parent_realize;
> >>      void (*parent_reset)(CPUState *cpu);
> >> +
> >> +    x86_def_t info;
> > Is it necessary to embed it here, could pointer to corresponding static
> > array be used here?
> > That way one could avoid extra memcpy in class_init().
> 
> That would require an allocation for -cpu host, which is undesired.
not necessary, it could be local static x86_def_t in target-i386/kvm.c in
host class_init(). Any way change is cosmetic and I do not insist on it.

> x86_def_t is supposed to die, forgot to add a TODO comment like on ppc.
> Patches to do that are being being polished on qom-cpu-ppc.
+1

> 
> >>  } X86CPUClass;
> >>  
> >>  /**
[...]

> >> *name) +static ObjectClass *x86_cpu_class_by_name(const char *name)
> >>  {
> >> -    x86_def_t *def;
> >> -    int i;
> >> +    ObjectClass *oc;
> >> +    char *typename;
> >>  
> >>      if (name == NULL) {
> >> -        return -1;
> >> -    }
> >> -    if (kvm_enabled() && strcmp(name, "host") == 0) {
> >> -        kvm_cpu_fill_host(x86_cpu_def);
> >> -        return 0;
> >> +        return NULL;
> >>      }
> >>  
> >> -    for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); i++) {
> >> -        def = &builtin_x86_defs[i];
> >> -        if (strcmp(name, def->name) == 0) {
> >> -            memcpy(x86_cpu_def, def, sizeof(*def));
> >> -            /* 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
> >> -             */
> >> -            if (kvm_enabled()) {
> >> -                uint32_t  ebx = 0, ecx = 0, edx = 0;
> >> -                host_cpuid(0, 0, NULL, &ebx, &ecx, &edx);
> >> -                x86_cpu_vendor_words2str(x86_cpu_def->vendor, ebx, edx,
> >> ecx);
> >> -            }
> >> -            return 0;
> >> +    if (strcmp(name, "host") == 0) {
> >> +        if (kvm_enabled()) {
> >> +            return object_class_by_name(TYPE_HOST_X86_CPU);
> >>          }
> >> +        return NULL;
> >>      }
> > block is not necessary, the following block could yield the same result.
> 
> No, it doesn't in the !kvm_enabled() case. I tripped over this when I
> left the kvm_enabled() check where it was before, falling back to the
> below lookup, leading to -cpu host being instantiated for TCG and
> originally not asserting (that's why I added the -cpu host initfn).
Maybe TYPE_HOST_X86_CPU shouldn't be registered at all if kvm_enabled() == 0.

> 
> Symptoms were x86_64 openSUSE .iso claiming no support for 486(?)
> processors.
> 
> >>  
> >> -    return -1;
> >> +    typename = g_strdup_printf("%s-" TYPE_X86_CPU, name);
> >> +    oc = object_class_by_name(typename);
> >> +    g_free(typename);
> >> +    if (oc != NULL && (!object_class_dynamic_cast(oc, TYPE_X86_CPU) ||
> >> +                       object_class_is_abstract(oc))) {
> >> +        oc = NULL;
> >> +    }
> >> +    return oc;
> >>  }
> >>  
[...]

> >> @@ -1580,13 +1424,13 @@ X86CPU *cpu_x86_init(const char *cpu_model)
> >>      name = model_pieces[0];
> >>      features = model_pieces[1];
> >>  
> >> -    cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >> -    env = &cpu->env;
> >> -    env->cpu_model_str = cpu_model;
> >> -
> >> -    if (cpu_x86_register(cpu, name) < 0) {
> >> +    oc = x86_cpu_class_by_name(name);
> >> +    if (oc == NULL) {
> > make sure error is reported when cpu is not found:
> >            error_setg(&error, "Can't find CPU: %s", name);
> 
> Why? The callers do that when NULL is returned.
The callers do not exactly know why NULL is returned, wouldn't it better
to specify error message at the point of origin.

> 
> >>          goto error;
> >>      }
> >> +    cpu = X86_CPU(object_new(object_class_get_name(oc)));
> >> +    env = &cpu->env;
> >> +    env->cpu_model_str = cpu_model;
> >>  
> >>      cpu_x86_parse_featurestr(cpu, features, &error);
> >>      if (error) {
[...]

> >> @@ -2198,6 +2018,8 @@ static void x86_cpu_initfn(Object *obj)
> >>      CPUState *cs = CPU(obj);
> >>      X86CPU *cpu = X86_CPU(obj);
> >>      CPUX86State *env = &cpu->env;
> >> +    X86CPUClass *xcc = X86_CPU_GET_CLASS(obj);
> >> +    const x86_def_t *def = &xcc->info;
> >>      static int inited;
> >>  
> >>      cpu_exec_init(env);
> >> @@ -2227,6 +2049,41 @@ static void x86_cpu_initfn(Object *obj)
> >>                          x86_cpuid_get_tsc_freq,
> >>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> >>  
> >> +    /* 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
> >> +     */
> >> +    if (kvm_enabled()) {
> >> +        host_cpuid(0, 0, NULL, &env->cpuid_vendor1, &env->cpuid_vendor2,
> >> +                   &env->cpuid_vendor3);
> > This is not per instance specific, this override would be better done to
> > sub-classes in kvm_arch_init().
> 
> Hmm... I think the issue I addresses this way was that kvm.c didn't have
> access to your static vendor_words2str() helper. Writing the words into
> the word fields is more direct than converting them to a string and
> writing them back into the words.
you have these correct vendor words ready after you called
kvm_host_cpu_class_init() in kvm_arch_init(), so just direct copy from host
class would do.

> 
> > As bonus we would get actual 'vendor' when
> > doing class introspection provided it's done after kvm_init() is called.
> 
> Doing that would mean that we force class_init of every CPU class to run
> every time we start with KVM enabled.
I'd ask if there is any particular downside in calling every class_init()?

> Is there any practical downside to
> doing it at instance_init time? Properties are set afterwards, so it
> doesn't override anything. Where would we want to inspect an X86CPUClass
> to have an AMD model say Intel or vice versa? -cpu ? doesn't look at it
> today, nor does QMP.
Presently nothing changes if we do it in kvm_init() or initfn() time since
none of users displays actual vendor value, but it looks more like a bug. 

Downside of doing it initfn() is that static properties defaults and global
properties are set by device_initfn() before cpu_initfn() is called, so
it renders all that was set via it overridden by a later x86_cpu_initfn().

So if there is no really big drawback in calling every x86 cpu class_init(),
I'd prefer to utilize features provided by static properties, than hacking
class wide vendor override per each cpu instance.

As alternative x86 cpu could have a hook that is called from
x86_cpu_class_init() that does kvm specific overrides and set by
kvm_arch_init(). Then there is no need to initialize all types in
kvm_arch_init(). The problem with it is if class is initialized before
kvm_init(), then override won't be applied ever.

> 
> >> +    } else {
> >> +        object_property_set_str(OBJECT(cpu), def->vendor, "vendor",
> >> NULL);
> >> +    }
> >> +
> >> +    object_property_set_int(OBJECT(cpu), def->level, "level", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->family, "family", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->model, "model", NULL);
> >> +    object_property_set_int(OBJECT(cpu), def->stepping, "stepping",
> >> NULL);
> >> +    env->cpuid_features = def->features;
> >> +    env->cpuid_ext_features = def->ext_features;
> >> +    env->cpuid_ext_features |= CPUID_EXT_HYPERVISOR;
> >> +    env->cpuid_ext2_features = def->ext2_features;
> >> +    env->cpuid_ext3_features = def->ext3_features;
> >> +    object_property_set_int(OBJECT(cpu), def->xlevel, "xlevel", NULL);
> >> +    env->cpuid_kvm_features = def->kvm_features;
> >> +    if (kvm_enabled()) {
> >> +        env->cpuid_kvm_features |= kvm_default_features;
> >> +    }
> >> +    env->cpuid_svm_features = def->svm_features;
> >> +    env->cpuid_ext4_features = def->ext4_features;
> >> +    env->cpuid_7_0_ebx_features = def->cpuid_7_0_ebx_features;
> >> +    env->cpuid_xlevel2 = def->xlevel2;
> >> +
> >> +    object_property_set_str(OBJECT(cpu), def->model_id, "model-id",
> >> NULL);
> > All this feature initialization in initfn() is not compatible with
> > static/global properties because they are set in device_initfn(). But I
> > can remove properties/features from here as they are converted to static
> > properties and incrementally move their defaults initialization into
> > new x86_cpu_def_class_init().
> 
> Would it help to call the old registration function from here? Or what
> exactly would you need on top?
Nope, see previous answer to why setting defaults here is not desired.
After conversion, defaults will be set via static properties defaults and then
on top of it, global properties will be applied in device_initfn().
Eventually -cpu xxx,foo should set global properties for cpu type and cpu
type name for machine to use. no manual property setting between object_new()
and realize() unless it is instance specific.

[...]
> >> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> >> index 9ebf181..dbfa670 100644
> >> --- a/target-i386/kvm.c
> >> +++ b/target-i386/kvm.c
> >> @@ -735,6 +735,80 @@ static int kvm_get_supported_msrs(KVMState *s)
> >>      return ret;
> >>  }
> >>  
> >> +static void kvm_host_cpu_initfn(Object *obj)
> >> +{
> >> +    assert(kvm_enabled());
> >> +}
> >> +
> >> +static bool kvm_host_cpu_needs_class_init;
> >> +
> >> +static void kvm_host_cpu_class_init(ObjectClass *oc, void *data)
> >> +{
> >> +    X86CPUClass *xcc = X86_CPU_CLASS(oc);
> >> +    x86_def_t *x86_cpu_def = &xcc->info;
> >> +    KVMState *s = kvm_state;
> >> +    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> >> +    int i;
> >> +
> >> +    if (!kvm_enabled()) {
> >> +        kvm_host_cpu_needs_class_init = true;
> >> +        return;
> >> +    }
> >> +
> >> +    xcc->info.name = "host";
> >> +
> >> +    /* Vendor will be set in initfn if kvm_enabled() */
> >> +
> >> +    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
> >> +    x86_cpu_def->family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> >> +    x86_cpu_def->model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
> >> +    x86_cpu_def->stepping = eax & 0x0F;
> >> +
> >> +    x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> >> +    x86_cpu_def->features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> >> R_EDX);
> >> +    x86_cpu_def->ext_features = kvm_arch_get_supported_cpuid(s, 0x1, 0,
> >> R_ECX); +
> >> +    if (x86_cpu_def->level >= 7) {
> >> +        x86_cpu_def->cpuid_7_0_ebx_features =
> >> +                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
> >> +    } else {
> >> +        x86_cpu_def->cpuid_7_0_ebx_features = 0;
> >> +    }
> >> +
> >> +    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0,
> >> R_EAX);
> >> +    x86_cpu_def->ext2_features =
> >> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
> >> +    x86_cpu_def->ext3_features =
> >> +                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
> >> +
> >> +    for (i = 0; i < 3; i++) {
> >> +        host_cpuid(0x80000002 + i, 0, &eax, &ebx, &ecx, &edx);
> >> +        memcpy(xcc->info.model_id + i * 16 +  0, &eax, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 +  4, &ebx, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 +  8, &ecx, 4);
> >> +        memcpy(xcc->info.model_id + i * 16 + 12, &edx, 4);
> >> +    }
> >> +
> >> +    /* Call Centaur's CPUID instruction. */
> >> +    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
> >> +        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
> >> +        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> >> +        if (eax >= 0xC0000001) {
> >> +            /* Support VIA max extended level */
> >> +            x86_cpu_def->xlevel2 = eax;
> >> +            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
> >> +            x86_cpu_def->ext4_features =
> >> +                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0,
> >> R_EDX);
> >> +        }
> >> +    }
> >> +
> >> +    /* Other KVM-specific feature fields: */
> >> +    x86_cpu_def->svm_features =
> >> +        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
> >> +    x86_cpu_def->kvm_features =
> >> +        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
> >> +}
> >> +
> >>  int kvm_arch_init(KVMState *s)
> >>  {
> >>      QemuOptsList *list = qemu_find_opts("machine");
> >> @@ -797,6 +871,11 @@ int kvm_arch_init(KVMState *s)
> >>              }
> >>          }
> >>      }
> >> +
> >> +    if (kvm_host_cpu_needs_class_init) {
> >> +        kvm_host_cpu_class_init(object_class_by_name(TYPE_HOST_X86_CPU),
> >> NULL);
> >> +    }
> > kvm_host_cpu_needs_class_init is set to true in kvm_host_cpu_class_init()
> > so block is nop.
> 
> Nope. It's a question of initialization ordering:
Yep, I see now, type_register_static(&host_x86_cpu_type_info) creates
fake(uninitialized) host class and then it is initialized when
kvm_arch_init() is called.

> 
> > Why kvm_host_cpu_needs_class_init is needed anyway, why not just call
> > kvm_host_cpu_class_init() here?
> 
> i) class_init may be called before kvm_enabled() evaluates to true, such
> as for -cpu ?, so we need to handle that case gracefully.
We do not print host cpu type at -cpu ? now, it's implied that host cpu is
available if started with --enable-kvm.

> 
> ii) The regular case is that the class_init is run in response to
> object_new() from pc.c, in which case kvm_enabled() evaluates to true
> and the needs_class_init remains false.
could we register host type in kvm_arch_init() time, and avoid calling
kvm_host_cpu_class_init() twice?
It also would allow to remove host look-up hack in x86_cpu_class_by_name().

> 
> Thus, both cases are used in practice.
> 
> Regards,
> Andreas
> 
> >> +
> >>      return 0;
> >>  }
> >>  
> >> @@ -2330,3 +2409,17 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t
> >> dev_id) return kvm_deassign_irq_internal(s, dev_id,
> >> KVM_DEV_IRQ_GUEST_MSIX | KVM_DEV_IRQ_HOST_MSIX);
> >>  }
> >> +
> >> +static const TypeInfo host_x86_cpu_type_info = {
> >> +    .name = TYPE_HOST_X86_CPU,
> >> +    .parent = TYPE_X86_CPU,
> >> +    .instance_init = kvm_host_cpu_initfn,
> >> +    .class_init = kvm_host_cpu_class_init,
> >> +};
> >> +
> >> +static void kvm_register_types(void)
> >> +{
> >> +    type_register_static(&host_x86_cpu_type_info);
> >> +}
> >> +
> >> +type_init(kvm_register_types)
>