Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 13/24] ppc: spapr: define core types statically

[Greg Kurz <groug@kaod.org>]

On Tue, 10 Oct 2017 09:29:46 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Mon,  9 Oct 2017 21:51:00 +0200
> Igor Mammedov <imammedo@redhat.com> wrote:
> 
> > spapr core type definition doesn't have any fields that
> > require it to be defined at runtime. So replace code
> > that fills in TypeInfo at runtime with static TypeInfo
> > array that does the same at complie time.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> >   - fix rebase conflict: add newly added power9_v2 core
> > ---  
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 

Oops, this patch causes a regression when you try to pass a sPAPR core to
a non-pseries machine.

Without this patch:

$ ppc64-softmmu/qemu-system-ppc64 -M none -device host-spapr-cpu-core 
qemu-system-ppc64: -device host-spapr-cpu-core:
 'host-spapr-cpu-core' is not a valid device model name

With this patch:

$ ppc64-softmmu/qemu-system-ppc64 -M none -device host-spapr-cpu-core 
**
ERROR:qom/object.c:217:object_type_get_instance_size:
 assertion failed: (type != NULL)
Aborted

#0  0x00007ffff760eff0 in raise () from /lib64/libc.so.6
#1  0x00007ffff761136c in abort () from /lib64/libc.so.6
#2  0x00007ffff78caa04 in g_assertion_message () from /lib64/libglib-2.0.so.0
#3  0x00007ffff78cab0c in g_assertion_message_expr () from /lib64/libglib-2.0.so.0
#4  0x00000000106abaac in object_type_get_instance_size (typename=0x108a3ac0 "host-powerpc64-cpu") at qom/object.c:217
#5  0x00000000101bb018 in spapr_cpu_core_realize (dev=0x111bcc10, errp=0x7fffffffe730) at hw/ppc/spapr_cpu_core.c:154
#6  0x000000001043e8b4 in device_set_realized (obj=0x111bcc10, value=true, errp=0x7fffffffe9f0) at hw/core/qdev.c:914
#7  0x00000000106b1390 in property_set_bool (obj=0x111bcc10, v=0x111c16a0, name=0x108cd520 "realized", opaque=0x111bcb60, errp=0x7fffffffe9f0) at qom/object.c:1906
#8  0x00000000106aeab8 in object_property_set (obj=0x111bcc10, v=0x111c16a0, name=0x108cd520 "realized", errp=0x7fffffffe9f0) at qom/object.c:1102
#9  0x00000000106b3068 in object_property_set_qobject (obj=0x111bcc10, value=0x111c1660, name=0x108cd520 "realized", errp=0x7fffffffe9f0) at qom/qom-qobject.c:27
#10 0x00000000106aee5c in object_property_set_bool (obj=0x111bcc10, value=true, name=0x108cd520 "realized", errp=0x7fffffffe9f0) at qom/object.c:1171
#11 0x000000001036b8cc in qdev_device_add (opts=0x110f4b30, errp=0x7fffffffea78) at qdev-monitor.c:630
#12 0x0000000010378e40 in device_init_func (opaque=0x0, opts=0x110f4b30, errp=0x0) at vl.c:2432
#13 0x0000000010850464 in qemu_opts_foreach (list=0x10abbaa8 <qemu_device_opts>, func=0x10378df4 <device_init_func>, opaque=0x0, errp=0x0) at util/qemu-option.c:1104
#14 0x000000001037fe0c in main (argc=5, argv=0x7ffffffff298, envp=0x7ffffffff2c8) at vl.c:4767

We're crashing because the host CPU type wasn't registered... Before, we would
gently fail to add the CPU core because its type wouldn't be registered either.

There's a strong correlation between the CPU type and CPU core type. I don't
believe we can handle this cleanly in the sPAPR code.

So I only see two options here: either drop this patch, or find a way to register
the host CPU type at QOM init time as well. It would be really unfortunate to bury
Igor's cleanup... I'd rather go forward so I started looking at the latter.

As explained in another mail recently, we cannot rely on the object class hierarchy
to do that since object_class_get_list() cannot be safely called during the QOM init
phase. It really needs all the parent types to be registered, because it will try
to initialize them at some point (look for type_initialize in qom/object.c).

https://lists.nongnu.org/archive/html/qemu-ppc/2017-09/msg00794.html

But since we only need CPU model names to register CPU types, building a PVR<->CPU model
table in the PPC code seem to be enough. Some more tweaking in the POWERPC_FAMILY() magic
is also needed to support migration on heterogeneous-but-compatible HW (eg, POWER8 <->
POWER8E).

I think that this patch should be reverted from David's staging tree for now, I'll
repost it when I have a proposal for the host CPU.

Cheers,

--
Greg

> >  include/hw/ppc/spapr_cpu_core.h |  2 +
> >  hw/ppc/spapr_cpu_core.c         | 87 +++++++++++++----------------------------
> >  2 files changed, 30 insertions(+), 59 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 93051e9..66dcf52 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -21,6 +21,8 @@
> >  #define SPAPR_CPU_CORE_GET_CLASS(obj) \
> >       OBJECT_GET_CLASS(sPAPRCPUCoreClass, (obj), TYPE_SPAPR_CPU_CORE)
> >  
> > +#define SPAPR_CPU_CORE_TYPE_NAME(model) model "-" TYPE_SPAPR_CPU_CORE
> > +
> >  typedef struct sPAPRCPUCore {
> >      /*< private >*/
> >      CPUCore parent_obj;
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index b6610dd..550d320 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -217,37 +217,6 @@ err:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > -static const char *spapr_core_models[] = {
> > -    /* 970 */
> > -    "970_v2.2",
> > -
> > -    /* 970MP variants */
> > -    "970mp_v1.0",
> > -    "970mp_v1.1",
> > -
> > -    /* POWER5+ */
> > -    "power5+_v2.1",
> > -
> > -    /* POWER7 */
> > -    "power7_v2.3",
> > -
> > -    /* POWER7+ */
> > -    "power7+_v2.1",
> > -
> > -    /* POWER8 */
> > -    "power8_v2.0",
> > -
> > -    /* POWER8E */
> > -    "power8e_v2.1",
> > -
> > -    /* POWER8NVL */
> > -    "power8nvl_v1.0",
> > -
> > -    /* POWER9 */
> > -    "power9_v1.0",
> > -    "power9_v2.0",
> > -};
> > -
> >  static Property spapr_cpu_core_properties[] = {
> >      DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id, CPU_UNSET_NUMA_NODE_ID),
> >      DEFINE_PROP_END_OF_LIST()
> > @@ -265,33 +234,33 @@ void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> >      g_assert(scc->cpu_class);
> >  }
> >  
> > -static const TypeInfo spapr_cpu_core_type_info = {
> > -    .name = TYPE_SPAPR_CPU_CORE,
> > -    .parent = TYPE_CPU_CORE,
> > -    .abstract = true,
> > -    .instance_size = sizeof(sPAPRCPUCore),
> > -    .class_size = sizeof(sPAPRCPUCoreClass),
> > -};
> > -
> > -static void spapr_cpu_core_register_types(void)
> > -{
> > -    int i;
> > -
> > -    type_register_static(&spapr_cpu_core_type_info);
> > -
> > -    for (i = 0; i < ARRAY_SIZE(spapr_core_models); i++) {
> > -        TypeInfo type_info = {
> > -            .parent = TYPE_SPAPR_CPU_CORE,
> > -            .instance_size = sizeof(sPAPRCPUCore),
> > -            .class_init = spapr_cpu_core_class_init,
> > -            .class_data = (void *) spapr_core_models[i],
> > -        };
> > -
> > -        type_info.name = g_strdup_printf("%s-" TYPE_SPAPR_CPU_CORE,
> > -                                         spapr_core_models[i]);
> > -        type_register(&type_info);
> > -        g_free((void *)type_info.name);
> > +#define DEFINE_SPAPR_CPU_CORE_TYPE(cpu_model) \
> > +    {                                                   \
> > +        .parent = TYPE_SPAPR_CPU_CORE,                  \587
> > +        .class_data = (void *) cpu_model,               \
> > +        .class_init = spapr_cpu_core_class_init,        \
> > +        .name = SPAPR_CPU_CORE_TYPE_NAME(cpu_model),    \
> >      }
> > -}
> >  
> > -type_init(spapr_cpu_core_register_types)
> > +static const TypeInfo spapr_cpu_core_type_infos[] = {
> > +    {
> > +        .name = TYPE_SPAPR_CPU_CORE,
> > +        .parent = TYPE_CPU_CORE,
> > +        .abstract = true,
> > +        .instance_size = sizeof(sPAPRCPUCore),
> > +        .class_size = sizeof(sPAPRCPUCoreClass),
> > +    },
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("970_v2.2"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.0"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("970mp_v1.1"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power5+_v2.1"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power7_v2.3"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power7+_v2.1"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power8_v2.0"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power8e_v2.1"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power8nvl_v1.0"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power9_v1.0"),
> > +    DEFINE_SPAPR_CPU_CORE_TYPE("power9_v2.0"),
> > +};
> > +
> > +DEFINE_TYPES(spapr_cpu_core_type_infos)  
> 
>