[RFC,v1,05/10] cpu: Abstract CPU core type

Commit Message

Bharata B Rao March 4, 2016, 6:54 a.m. UTC

Add an abstract CPU core type that could be used by machines that want
to define and hotplug CPUs in core granularity.

Signed-off-by: Bharata B Rao <[e-mail]>
---
 hw/cpu/Makefile.objs  |  1 +
 hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 hw/cpu/core.c
 create mode 100644 include/hw/cpu/core.h

Comments

Igor Mammedov March 4, 2016, 10:38 a.m. UTC | #1

On Fri,  4 Mar 2016 12:24:16 +0530
Bharata B Rao <[e-mail]> wrote:

> Add an abstract CPU core type that could be used by machines that want
> to define and hotplug CPUs in core granularity.
> 
> Signed-off-by: Bharata B Rao <[e-mail]>
> ---
>  hw/cpu/Makefile.objs  |  1 +
>  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 75 insertions(+)
>  create mode 100644 hw/cpu/core.c
>  create mode 100644 include/hw/cpu/core.h
> 
> diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> index 0954a18..942a4bb 100644
> --- a/hw/cpu/Makefile.objs
> +++ b/hw/cpu/Makefile.objs
> @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
>  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
>  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
>  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> +obj-y += core.o
>  
> diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> new file mode 100644
> index 0000000..d8caf37
> --- /dev/null
> +++ b/hw/cpu/core.c
> @@ -0,0 +1,44 @@
> +/*
> + * CPU core abstract device
> + *
> + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "hw/cpu/core.h"
> +
> +static char *core_prop_get_slot(Object *obj, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +
> +    return g_strdup(core->slot);
> +}
> +
> +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> +{
> +    CPUCore *core = CPU_CORE(obj);
> +
> +    core->slot = g_strdup(val);
> +}
> +
> +static void cpu_core_instance_init(Object *obj)
> +{
> +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> +                            NULL);
> +}
> +
> +static const TypeInfo cpu_core_type_info = {
> +    .name = TYPE_CPU_CORE,
> +    .parent = TYPE_DEVICE,
> +    .abstract = true,
> +    .instance_size = sizeof(CPUCore),
> +    .instance_init = cpu_core_instance_init,
> +};
> +
> +static void cpu_core_register_types(void)
> +{
> +    type_register_static(&cpu_core_type_info);
> +}
> +
> +type_init(cpu_core_register_types)
> diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> new file mode 100644
> index 0000000..2daa724
> --- /dev/null
> +++ b/include/hw/cpu/core.h
> @@ -0,0 +1,30 @@
> +/*
> + * CPU core abstract device
> + *
> + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef HW_CPU_CORE_H
> +#define HW_CPU_CORE_H
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +
> +#define TYPE_CPU_CORE "cpu-core"
> +
> +#define CPU_CORE(obj) \
> +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> +
> +typedef struct CPUCore {
> +    /*< private >*/
> +    DeviceState parent_obj;
> +
> +    /*< public >*/
> +    char *slot;
> +} CPUCore;
> +
> +#define CPU_CORE_SLOT_PROP "slot"
as it's generic property I'd rename to 'core' so it would fit all users


on top of that I'd add numeric 'threads' property to base class so
all derived cores would inherit it.

Then as easy integration with -smp threads=x, a machine could push
a global variable 'cpu-core.threads=[smp_threads]' which would
make every created cpu-core object to have threads set
at instance_init() time (device_init).

That way user won't have to specify 'threads=y' for every
  device_add spapr-core,core=x
as it will be taken from global property 'cpu-core.threads'
but if user wishes he/she still could override global by explicitly
providing thread property at device_add time:
  device_add spapr-core,core=x,threads=y

wrt this series it would mean, instead of creating threads in property
setter, delaying threads creation to core.realize() time,
but since realize is allowed to fail it should be fine do so.

> +
> +#endif

Bharata B Rao March 4, 2016, 11:02 a.m. UTC | #2

On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:
> On Fri,  4 Mar 2016 12:24:16 +0530
> Bharata B Rao <[e-mail]> wrote:
> 
> > Add an abstract CPU core type that could be used by machines that want
> > to define and hotplug CPUs in core granularity.
> > 
> > Signed-off-by: Bharata B Rao <[e-mail]>
> > ---
> >  hw/cpu/Makefile.objs  |  1 +
> >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> >  3 files changed, 75 insertions(+)
> >  create mode 100644 hw/cpu/core.c
> >  create mode 100644 include/hw/cpu/core.h
> > 
> > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > index 0954a18..942a4bb 100644
> > --- a/hw/cpu/Makefile.objs
> > +++ b/hw/cpu/Makefile.objs
> > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > +obj-y += core.o
> >  
> > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > new file mode 100644
> > index 0000000..d8caf37
> > --- /dev/null
> > +++ b/hw/cpu/core.c
> > @@ -0,0 +1,44 @@
> > +/*
> > + * CPU core abstract device
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#include "hw/cpu/core.h"
> > +
> > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > +{
> > +    CPUCore *core = CPU_CORE(obj);
> > +
> > +    return g_strdup(core->slot);
> > +}
> > +
> > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > +{
> > +    CPUCore *core = CPU_CORE(obj);
> > +
> > +    core->slot = g_strdup(val);
> > +}
> > +
> > +static void cpu_core_instance_init(Object *obj)
> > +{
> > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > +                            NULL);
> > +}
> > +
> > +static const TypeInfo cpu_core_type_info = {
> > +    .name = TYPE_CPU_CORE,
> > +    .parent = TYPE_DEVICE,
> > +    .abstract = true,
> > +    .instance_size = sizeof(CPUCore),
> > +    .instance_init = cpu_core_instance_init,
> > +};
> > +
> > +static void cpu_core_register_types(void)
> > +{
> > +    type_register_static(&cpu_core_type_info);
> > +}
> > +
> > +type_init(cpu_core_register_types)
> > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > new file mode 100644
> > index 0000000..2daa724
> > --- /dev/null
> > +++ b/include/hw/cpu/core.h
> > @@ -0,0 +1,30 @@
> > +/*
> > + * CPU core abstract device
> > + *
> > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +#ifndef HW_CPU_CORE_H
> > +#define HW_CPU_CORE_H
> > +
> > +#include "qemu/osdep.h"
> > +#include "hw/qdev.h"
> > +
> > +#define TYPE_CPU_CORE "cpu-core"
> > +
> > +#define CPU_CORE(obj) \
> > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > +
> > +typedef struct CPUCore {
> > +    /*< private >*/
> > +    DeviceState parent_obj;
> > +
> > +    /*< public >*/
> > +    char *slot;
> > +} CPUCore;
> > +
> > +#define CPU_CORE_SLOT_PROP "slot"
> as it's generic property I'd rename to 'core' so it would fit all users

Ok. Also note that this is a string property which is associated with the
link name (string) that we created from machine object to this core. I think
it would be ideal if this becomes an interger  property in which case it
becomes easier to feed the core location into your CPUSlotProperties.core.

> on top of that I'd add numeric 'threads' property to base class so
> all derived cores would inherit it.
> 
> Then as easy integration with -smp threads=x, a machine could push
> a global variable 'cpu-core.threads=[smp_threads]' which would
> make every created cpu-core object to have threads set
> at instance_init() time (device_init).
> 
> That way user won't have to specify 'threads=y' for every
>   device_add spapr-core,core=x
> as it will be taken from global property 'cpu-core.threads'
> but if user wishes he/she still could override global by explicitly
> providing thread property at device_add time:
>   device_add spapr-core,core=x,threads=y
> 
> wrt this series it would mean, instead of creating threads in property
> setter, delaying threads creation to core.realize() time,
> but since realize is allowed to fail it should be fine do so.

Ok that would suit us as there are two properties on which thread creation
is dependent upon: nr_threads and cpu_model. If thread objects can be
created at core realize time, then we don't have to resort to the ugliness
of creating the threads from either of the property setters. I always
assumed that we shouldn't be creating objects from realize, but if that
is fine, it is good.

Regards,
Bharata.

Igor Mammedov March 4, 2016, 6:07 p.m. UTC | #3

On Fri, 4 Mar 2016 16:32:53 +0530
Bharata B Rao <[e-mail]> wrote:

> On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:
> > On Fri,  4 Mar 2016 12:24:16 +0530
> > Bharata B Rao <[e-mail]> wrote:
> >   
> > > Add an abstract CPU core type that could be used by machines that want
> > > to define and hotplug CPUs in core granularity.
> > > 
> > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > ---
> > >  hw/cpu/Makefile.objs  |  1 +
> > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > >  3 files changed, 75 insertions(+)
> > >  create mode 100644 hw/cpu/core.c
> > >  create mode 100644 include/hw/cpu/core.h
> > > 
> > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > index 0954a18..942a4bb 100644
> > > --- a/hw/cpu/Makefile.objs
> > > +++ b/hw/cpu/Makefile.objs
> > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > +obj-y += core.o
> > >  
> > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > new file mode 100644
> > > index 0000000..d8caf37
> > > --- /dev/null
> > > +++ b/hw/cpu/core.c
> > > @@ -0,0 +1,44 @@
> > > +/*
> > > + * CPU core abstract device
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#include "hw/cpu/core.h"
> > > +
> > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > +{
> > > +    CPUCore *core = CPU_CORE(obj);
> > > +
> > > +    return g_strdup(core->slot);
> > > +}
> > > +
> > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > +{
> > > +    CPUCore *core = CPU_CORE(obj);
> > > +
> > > +    core->slot = g_strdup(val);
> > > +}
> > > +
> > > +static void cpu_core_instance_init(Object *obj)
> > > +{
> > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > +                            NULL);
> > > +}
> > > +
> > > +static const TypeInfo cpu_core_type_info = {
> > > +    .name = TYPE_CPU_CORE,
> > > +    .parent = TYPE_DEVICE,
> > > +    .abstract = true,
> > > +    .instance_size = sizeof(CPUCore),
> > > +    .instance_init = cpu_core_instance_init,
> > > +};
> > > +
> > > +static void cpu_core_register_types(void)
> > > +{
> > > +    type_register_static(&cpu_core_type_info);
> > > +}
> > > +
> > > +type_init(cpu_core_register_types)
> > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > new file mode 100644
> > > index 0000000..2daa724
> > > --- /dev/null
> > > +++ b/include/hw/cpu/core.h
> > > @@ -0,0 +1,30 @@
> > > +/*
> > > + * CPU core abstract device
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > + *
> > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > + * See the COPYING file in the top-level directory.
> > > + */
> > > +#ifndef HW_CPU_CORE_H
> > > +#define HW_CPU_CORE_H
> > > +
> > > +#include "qemu/osdep.h"
> > > +#include "hw/qdev.h"
> > > +
> > > +#define TYPE_CPU_CORE "cpu-core"
> > > +
> > > +#define CPU_CORE(obj) \
> > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > +
> > > +typedef struct CPUCore {
> > > +    /*< private >*/
> > > +    DeviceState parent_obj;
> > > +
> > > +    /*< public >*/
> > > +    char *slot;
> > > +} CPUCore;
> > > +
> > > +#define CPU_CORE_SLOT_PROP "slot"  
> > as it's generic property I'd rename to 'core' so it would fit all users  
> 
> Ok. Also note that this is a string property which is associated with the
> link name (string) that we created from machine object to this core. I think
> it would be ideal if this becomes an interger  property in which case it
> becomes easier to feed the core location into your CPUSlotProperties.core.
agreed, it should be core number.


> > on top of that I'd add numeric 'threads' property to base class so
> > all derived cores would inherit it.
> > 
> > Then as easy integration with -smp threads=x, a machine could push
> > a global variable 'cpu-core.threads=[smp_threads]' which would
> > make every created cpu-core object to have threads set
> > at instance_init() time (device_init).
> > 
> > That way user won't have to specify 'threads=y' for every
> >   device_add spapr-core,core=x
> > as it will be taken from global property 'cpu-core.threads'
> > but if user wishes he/she still could override global by explicitly
> > providing thread property at device_add time:
> >   device_add spapr-core,core=x,threads=y
> > 
> > wrt this series it would mean, instead of creating threads in property
> > setter, delaying threads creation to core.realize() time,
> > but since realize is allowed to fail it should be fine do so.  
> 
> Ok that would suit us as there are two properties on which thread creation
> is dependent upon: nr_threads and cpu_model. If thread objects can be
> created at core realize time, then we don't have to resort to the ugliness
> of creating the threads from either of the property setters. I always
> assumed that we shouldn't be creating objects from realize, but if that
> is fine, it is good.
since realize is allowed to fail, it should be safe from hotplug pov
to create internal objects there, as far as proper cleanups are done
for failure path.

> 
> Regards,
> Bharata.
>

David Gibson March 7, 2016, 3:36 a.m. UTC | #4

On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:
> On Fri, 4 Mar 2016 16:32:53 +0530
> Bharata B Rao <[e-mail]> wrote:
> 
> > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:
> > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > Bharata B Rao <[e-mail]> wrote:
> > >   
> > > > Add an abstract CPU core type that could be used by machines that want
> > > > to define and hotplug CPUs in core granularity.
> > > > 
> > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > ---
> > > >  hw/cpu/Makefile.objs  |  1 +
> > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > >  3 files changed, 75 insertions(+)
> > > >  create mode 100644 hw/cpu/core.c
> > > >  create mode 100644 include/hw/cpu/core.h
> > > > 
> > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > index 0954a18..942a4bb 100644
> > > > --- a/hw/cpu/Makefile.objs
> > > > +++ b/hw/cpu/Makefile.objs
> > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > +obj-y += core.o
> > > >  
> > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > new file mode 100644
> > > > index 0000000..d8caf37
> > > > --- /dev/null
> > > > +++ b/hw/cpu/core.c
> > > > @@ -0,0 +1,44 @@
> > > > +/*
> > > > + * CPU core abstract device
> > > > + *
> > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +#include "hw/cpu/core.h"
> > > > +
> > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > +{
> > > > +    CPUCore *core = CPU_CORE(obj);
> > > > +
> > > > +    return g_strdup(core->slot);
> > > > +}
> > > > +
> > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > +{
> > > > +    CPUCore *core = CPU_CORE(obj);
> > > > +
> > > > +    core->slot = g_strdup(val);
> > > > +}
> > > > +
> > > > +static void cpu_core_instance_init(Object *obj)
> > > > +{
> > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > +                            NULL);
> > > > +}
> > > > +
> > > > +static const TypeInfo cpu_core_type_info = {
> > > > +    .name = TYPE_CPU_CORE,
> > > > +    .parent = TYPE_DEVICE,
> > > > +    .abstract = true,
> > > > +    .instance_size = sizeof(CPUCore),
> > > > +    .instance_init = cpu_core_instance_init,
> > > > +};
> > > > +
> > > > +static void cpu_core_register_types(void)
> > > > +{
> > > > +    type_register_static(&cpu_core_type_info);
> > > > +}
> > > > +
> > > > +type_init(cpu_core_register_types)
> > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > new file mode 100644
> > > > index 0000000..2daa724
> > > > --- /dev/null
> > > > +++ b/include/hw/cpu/core.h
> > > > @@ -0,0 +1,30 @@
> > > > +/*
> > > > + * CPU core abstract device
> > > > + *
> > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > + *
> > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > + * See the COPYING file in the top-level directory.
> > > > + */
> > > > +#ifndef HW_CPU_CORE_H
> > > > +#define HW_CPU_CORE_H
> > > > +
> > > > +#include "qemu/osdep.h"
> > > > +#include "hw/qdev.h"
> > > > +
> > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > +
> > > > +#define CPU_CORE(obj) \
> > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > +
> > > > +typedef struct CPUCore {
> > > > +    /*< private >*/
> > > > +    DeviceState parent_obj;
> > > > +
> > > > +    /*< public >*/
> > > > +    char *slot;
> > > > +} CPUCore;
> > > > +
> > > > +#define CPU_CORE_SLOT_PROP "slot"  
> > > as it's generic property I'd rename to 'core' so it would fit all users  
> > 
> > Ok. Also note that this is a string property which is associated with the
> > link name (string) that we created from machine object to this core. I think
> > it would be ideal if this becomes an interger  property in which case it
> > becomes easier to feed the core location into your CPUSlotProperties.core.
> agreed, it should be core number.

The slot stuff is continuing to confuse me a bit.  I see that we need
some kind of "address" value, but how best to do it is not clear to
me.

Changing this to an integer sounds like it's probably a good idea.
I'm a bit wary of just calling it "core" though.  Do all platforms
even necessarily have a core id?

I'm wondering if the addressing is something that needs to move the
the platform specific subtypes, while some other stuff can move to the
generic base type.

> > > on top of that I'd add numeric 'threads' property to base class so
> > > all derived cores would inherit it.
> > > 
> > > Then as easy integration with -smp threads=x, a machine could push
> > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > make every created cpu-core object to have threads set
> > > at instance_init() time (device_init).
> > > 
> > > That way user won't have to specify 'threads=y' for every
> > >   device_add spapr-core,core=x
> > > as it will be taken from global property 'cpu-core.threads'
> > > but if user wishes he/she still could override global by explicitly
> > > providing thread property at device_add time:
> > >   device_add spapr-core,core=x,threads=y
> > > 
> > > wrt this series it would mean, instead of creating threads in property
> > > setter, delaying threads creation to core.realize() time,
> > > but since realize is allowed to fail it should be fine do so.  
> > 
> > Ok that would suit us as there are two properties on which thread creation
> > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > created at core realize time, then we don't have to resort to the ugliness
> > of creating the threads from either of the property setters. I always
> > assumed that we shouldn't be creating objects from realize, but if that
> > is fine, it is good.
> since realize is allowed to fail, it should be safe from hotplug pov
> to create internal objects there, as far as proper cleanups are done
> for failure path.

Right, moving the "nr_threads" property to the base type seems like a
good idea to me.  Even for subtypes which have a fixed number of
threads we should be able to handle by just verifying and rejecting in
the subtype's realize.  And even for those core types, being able to read
the number of threads in a generic way is a good thing.

I'm not clear from the above if you're also intending to move at least
the adding of the threads as child properties is supposed to go into
the base type, but that also sounds like a good idea, again for
consistency.

Bharata B Rao March 7, 2016, 8:31 a.m. UTC | #5

On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:
> On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:
> > On Fri, 4 Mar 2016 16:32:53 +0530
> > Bharata B Rao <[e-mail]> wrote:
> > 
> > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:
> > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > Bharata B Rao <[e-mail]> wrote:
> > > >   
> > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > to define and hotplug CPUs in core granularity.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > ---
> > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > >  3 files changed, 75 insertions(+)
> > > > >  create mode 100644 hw/cpu/core.c
> > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > 
> > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > index 0954a18..942a4bb 100644
> > > > > --- a/hw/cpu/Makefile.objs
> > > > > +++ b/hw/cpu/Makefile.objs
> > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > +obj-y += core.o
> > > > >  
> > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > new file mode 100644
> > > > > index 0000000..d8caf37
> > > > > --- /dev/null
> > > > > +++ b/hw/cpu/core.c
> > > > > @@ -0,0 +1,44 @@
> > > > > +/*
> > > > > + * CPU core abstract device
> > > > > + *
> > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +#include "hw/cpu/core.h"
> > > > > +
> > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > +{
> > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > +
> > > > > +    return g_strdup(core->slot);
> > > > > +}
> > > > > +
> > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > +{
> > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > +
> > > > > +    core->slot = g_strdup(val);
> > > > > +}
> > > > > +
> > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > +{
> > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > +                            NULL);
> > > > > +}
> > > > > +
> > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > +    .name = TYPE_CPU_CORE,
> > > > > +    .parent = TYPE_DEVICE,
> > > > > +    .abstract = true,
> > > > > +    .instance_size = sizeof(CPUCore),
> > > > > +    .instance_init = cpu_core_instance_init,
> > > > > +};
> > > > > +
> > > > > +static void cpu_core_register_types(void)
> > > > > +{
> > > > > +    type_register_static(&cpu_core_type_info);
> > > > > +}
> > > > > +
> > > > > +type_init(cpu_core_register_types)
> > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > new file mode 100644
> > > > > index 0000000..2daa724
> > > > > --- /dev/null
> > > > > +++ b/include/hw/cpu/core.h
> > > > > @@ -0,0 +1,30 @@
> > > > > +/*
> > > > > + * CPU core abstract device
> > > > > + *
> > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +#ifndef HW_CPU_CORE_H
> > > > > +#define HW_CPU_CORE_H
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "hw/qdev.h"
> > > > > +
> > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > +
> > > > > +#define CPU_CORE(obj) \
> > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > +
> > > > > +typedef struct CPUCore {
> > > > > +    /*< private >*/
> > > > > +    DeviceState parent_obj;
> > > > > +
> > > > > +    /*< public >*/
> > > > > +    char *slot;
> > > > > +} CPUCore;
> > > > > +
> > > > > +#define CPU_CORE_SLOT_PROP "slot"  
> > > > as it's generic property I'd rename to 'core' so it would fit all users  
> > > 
> > > Ok. Also note that this is a string property which is associated with the
> > > link name (string) that we created from machine object to this core. I think
> > > it would be ideal if this becomes an interger  property in which case it
> > > becomes easier to feed the core location into your CPUSlotProperties.core.
> > agreed, it should be core number.
> 
> The slot stuff is continuing to confuse me a bit.  I see that we need
> some kind of "address" value, but how best to do it is not clear to
> me.
> 
> Changing this to an integer sounds like it's probably a good idea.
> I'm a bit wary of just calling it "core" though.  Do all platforms
> even necessarily have a core id?
> 
> I'm wondering if the addressing is something that needs to move the
> the platform specific subtypes, while some other stuff can move to the
> generic base type.
> 
> > > > on top of that I'd add numeric 'threads' property to base class so
> > > > all derived cores would inherit it.
> > > > 
> > > > Then as easy integration with -smp threads=x, a machine could push
> > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > make every created cpu-core object to have threads set
> > > > at instance_init() time (device_init).
> > > > 
> > > > That way user won't have to specify 'threads=y' for every
> > > >   device_add spapr-core,core=x
> > > > as it will be taken from global property 'cpu-core.threads'
> > > > but if user wishes he/she still could override global by explicitly
> > > > providing thread property at device_add time:
> > > >   device_add spapr-core,core=x,threads=y
> > > > 
> > > > wrt this series it would mean, instead of creating threads in property
> > > > setter, delaying threads creation to core.realize() time,
> > > > but since realize is allowed to fail it should be fine do so.  
> > > 
> > > Ok that would suit us as there are two properties on which thread creation
> > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > created at core realize time, then we don't have to resort to the ugliness
> > > of creating the threads from either of the property setters. I always
> > > assumed that we shouldn't be creating objects from realize, but if that
> > > is fine, it is good.
> > since realize is allowed to fail, it should be safe from hotplug pov
> > to create internal objects there, as far as proper cleanups are done
> > for failure path.
> 
> Right, moving the "nr_threads" property to the base type seems like a
> good idea to me.

And we will also move the cpu_model property (now being tracked by
an ObjectClass pointer) to the base type ?

Regards,
Bharata.

Igor Mammedov March 7, 2016, 10:29 a.m. UTC | #6

On Mon, 7 Mar 2016 14:36:55 +1100
David Gibson <[e-mail]> wrote:

> On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:
> > On Fri, 4 Mar 2016 16:32:53 +0530
> > Bharata B Rao <[e-mail]> wrote:
> >   
> > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:  
> > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > Bharata B Rao <[e-mail]> wrote:
> > > >     
> > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > to define and hotplug CPUs in core granularity.
> > > > > 
> > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > ---
> > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > >  3 files changed, 75 insertions(+)
> > > > >  create mode 100644 hw/cpu/core.c
> > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > 
> > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > index 0954a18..942a4bb 100644
> > > > > --- a/hw/cpu/Makefile.objs
> > > > > +++ b/hw/cpu/Makefile.objs
> > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > +obj-y += core.o
> > > > >  
> > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > new file mode 100644
> > > > > index 0000000..d8caf37
> > > > > --- /dev/null
> > > > > +++ b/hw/cpu/core.c
> > > > > @@ -0,0 +1,44 @@
> > > > > +/*
> > > > > + * CPU core abstract device
> > > > > + *
> > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +#include "hw/cpu/core.h"
> > > > > +
> > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > +{
> > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > +
> > > > > +    return g_strdup(core->slot);
> > > > > +}
> > > > > +
> > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > +{
> > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > +
> > > > > +    core->slot = g_strdup(val);
> > > > > +}
> > > > > +
> > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > +{
> > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > +                            NULL);
> > > > > +}
> > > > > +
> > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > +    .name = TYPE_CPU_CORE,
> > > > > +    .parent = TYPE_DEVICE,
> > > > > +    .abstract = true,
> > > > > +    .instance_size = sizeof(CPUCore),
> > > > > +    .instance_init = cpu_core_instance_init,
> > > > > +};
> > > > > +
> > > > > +static void cpu_core_register_types(void)
> > > > > +{
> > > > > +    type_register_static(&cpu_core_type_info);
> > > > > +}
> > > > > +
> > > > > +type_init(cpu_core_register_types)
> > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > new file mode 100644
> > > > > index 0000000..2daa724
> > > > > --- /dev/null
> > > > > +++ b/include/hw/cpu/core.h
> > > > > @@ -0,0 +1,30 @@
> > > > > +/*
> > > > > + * CPU core abstract device
> > > > > + *
> > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > + *
> > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > + * See the COPYING file in the top-level directory.
> > > > > + */
> > > > > +#ifndef HW_CPU_CORE_H
> > > > > +#define HW_CPU_CORE_H
> > > > > +
> > > > > +#include "qemu/osdep.h"
> > > > > +#include "hw/qdev.h"
> > > > > +
> > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > +
> > > > > +#define CPU_CORE(obj) \
> > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > +
> > > > > +typedef struct CPUCore {
> > > > > +    /*< private >*/
> > > > > +    DeviceState parent_obj;
> > > > > +
> > > > > +    /*< public >*/
> > > > > +    char *slot;
> > > > > +} CPUCore;
> > > > > +
> > > > > +#define CPU_CORE_SLOT_PROP "slot"    
> > > > as it's generic property I'd rename to 'core' so it would fit all users    
> > > 
> > > Ok. Also note that this is a string property which is associated with the
> > > link name (string) that we created from machine object to this core. I think
> > > it would be ideal if this becomes an interger  property in which case it
> > > becomes easier to feed the core location into your CPUSlotProperties.core.  
> > agreed, it should be core number.  
> 
> The slot stuff is continuing to confuse me a bit.  I see that we need
> some kind of "address" value, but how best to do it is not clear to
> me.
> 
> Changing this to an integer sounds like it's probably a good idea.
> I'm a bit wary of just calling it "core" though.  Do all platforms
> even necessarily have a core id?
platform's that don't have core concept could or even should
use its own base type (i.e. not cpu-core).
Numeric code id should work for x86, ARM and Power.

> I'm wondering if the addressing is something that needs to move the
> the platform specific subtypes, while some other stuff can move to the
> generic base type.
core id looks to me as cpu-core property but I won't object if
it will be moved to subtype as so far only Power would have it.

What I'd prefer to keep is consistent naming of properties
if it's possible, i.e. property 'core' which makes sense for x86, ARM, and Power
from enduser point of view.

> 
> > > > on top of that I'd add numeric 'threads' property to base class so
> > > > all derived cores would inherit it.
> > > > 
> > > > Then as easy integration with -smp threads=x, a machine could push
> > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > make every created cpu-core object to have threads set
> > > > at instance_init() time (device_init).
> > > > 
> > > > That way user won't have to specify 'threads=y' for every
> > > >   device_add spapr-core,core=x
> > > > as it will be taken from global property 'cpu-core.threads'
> > > > but if user wishes he/she still could override global by explicitly
> > > > providing thread property at device_add time:
> > > >   device_add spapr-core,core=x,threads=y
> > > > 
> > > > wrt this series it would mean, instead of creating threads in property
> > > > setter, delaying threads creation to core.realize() time,
> > > > but since realize is allowed to fail it should be fine do so.    
> > > 
> > > Ok that would suit us as there are two properties on which thread creation
> > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > created at core realize time, then we don't have to resort to the ugliness
> > > of creating the threads from either of the property setters. I always
> > > assumed that we shouldn't be creating objects from realize, but if that
> > > is fine, it is good.  
> > since realize is allowed to fail, it should be safe from hotplug pov
> > to create internal objects there, as far as proper cleanups are done
> > for failure path.  
> 
[...]
> I'm not clear from the above if you're also intending to move at least
> the adding of the threads as child properties is supposed to go into
> the base type,
I'm not sure that I've got question, could you please rephrase?

> but that also sounds like a good idea, again for
> consistency.
>

Igor Mammedov March 7, 2016, 10:40 a.m. UTC | #7

On Mon, 7 Mar 2016 14:01:55 +0530
Bharata B Rao <[e-mail]> wrote:

> On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:
> > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:  
> > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > Bharata B Rao <[e-mail]> wrote:
> > >   
> > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:  
> > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > Bharata B Rao <[e-mail]> wrote:
> > > > >     
> > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > to define and hotplug CPUs in core granularity.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > ---
> > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 75 insertions(+)
> > > > > >  create mode 100644 hw/cpu/core.c
> > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > 
> > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > index 0954a18..942a4bb 100644
> > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > +obj-y += core.o
> > > > > >  
> > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > new file mode 100644
> > > > > > index 0000000..d8caf37
> > > > > > --- /dev/null
> > > > > > +++ b/hw/cpu/core.c
> > > > > > @@ -0,0 +1,44 @@
> > > > > > +/*
> > > > > > + * CPU core abstract device
> > > > > > + *
> > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +#include "hw/cpu/core.h"
> > > > > > +
> > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > +{
> > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > +
> > > > > > +    return g_strdup(core->slot);
> > > > > > +}
> > > > > > +
> > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > +{
> > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > +
> > > > > > +    core->slot = g_strdup(val);
> > > > > > +}
> > > > > > +
> > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > +{
> > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > +                            NULL);
> > > > > > +}
> > > > > > +
> > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > +    .parent = TYPE_DEVICE,
> > > > > > +    .abstract = true,
> > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > +};
> > > > > > +
> > > > > > +static void cpu_core_register_types(void)
> > > > > > +{
> > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > +}
> > > > > > +
> > > > > > +type_init(cpu_core_register_types)
> > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > new file mode 100644
> > > > > > index 0000000..2daa724
> > > > > > --- /dev/null
> > > > > > +++ b/include/hw/cpu/core.h
> > > > > > @@ -0,0 +1,30 @@
> > > > > > +/*
> > > > > > + * CPU core abstract device
> > > > > > + *
> > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > +#define HW_CPU_CORE_H
> > > > > > +
> > > > > > +#include "qemu/osdep.h"
> > > > > > +#include "hw/qdev.h"
> > > > > > +
> > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > +
> > > > > > +#define CPU_CORE(obj) \
> > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > +
> > > > > > +typedef struct CPUCore {
> > > > > > +    /*< private >*/
> > > > > > +    DeviceState parent_obj;
> > > > > > +
> > > > > > +    /*< public >*/
> > > > > > +    char *slot;
> > > > > > +} CPUCore;
> > > > > > +
> > > > > > +#define CPU_CORE_SLOT_PROP "slot"    
> > > > > as it's generic property I'd rename to 'core' so it would fit all users    
> > > > 
> > > > Ok. Also note that this is a string property which is associated with the
> > > > link name (string) that we created from machine object to this core. I think
> > > > it would be ideal if this becomes an interger  property in which case it
> > > > becomes easier to feed the core location into your CPUSlotProperties.core.  
> > > agreed, it should be core number.  
> > 
> > The slot stuff is continuing to confuse me a bit.  I see that we need
> > some kind of "address" value, but how best to do it is not clear to
> > me.
> > 
> > Changing this to an integer sounds like it's probably a good idea.
> > I'm a bit wary of just calling it "core" though.  Do all platforms
> > even necessarily have a core id?
> > 
> > I'm wondering if the addressing is something that needs to move the
> > the platform specific subtypes, while some other stuff can move to the
> > generic base type.
> >   
> > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > all derived cores would inherit it.
> > > > > 
> > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > make every created cpu-core object to have threads set
> > > > > at instance_init() time (device_init).
> > > > > 
> > > > > That way user won't have to specify 'threads=y' for every
> > > > >   device_add spapr-core,core=x
> > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > but if user wishes he/she still could override global by explicitly
> > > > > providing thread property at device_add time:
> > > > >   device_add spapr-core,core=x,threads=y
> > > > > 
> > > > > wrt this series it would mean, instead of creating threads in property
> > > > > setter, delaying threads creation to core.realize() time,
> > > > > but since realize is allowed to fail it should be fine do so.    
> > > > 
> > > > Ok that would suit us as there are two properties on which thread creation
> > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > created at core realize time, then we don't have to resort to the ugliness
> > > > of creating the threads from either of the property setters. I always
> > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > is fine, it is good.  
> > > since realize is allowed to fail, it should be safe from hotplug pov
> > > to create internal objects there, as far as proper cleanups are done
> > > for failure path.  
> > 
> > Right, moving the "nr_threads" property to the base type seems like a
> > good idea to me.  
> 
> And we will also move the cpu_model property (now being tracked by
> an ObjectClass pointer) to the base type ?
I'm not sure that moving cpu_model to the base class is the right thing,
I'd keep it local to platform for now.

Could you have several spapr core types? One per CPU model?
That way you won't need to track cpu_model when using device_add.

> 
> Regards,
> Bharata.
>

David Gibson March 8, 2016, 3:57 a.m. UTC | #8

On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:
> On Mon, 7 Mar 2016 14:01:55 +0530
> Bharata B Rao <[e-mail]> wrote:
> 
> > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:
> > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > Bharata B Rao <[e-mail]> wrote:
> > > >   
> > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:  
> > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > >     
> > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > ---
> > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 75 insertions(+)
> > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > 
> > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > index 0954a18..942a4bb 100644
> > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > +obj-y += core.o
> > > > > > >  
> > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..d8caf37
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/cpu/core.c
> > > > > > > @@ -0,0 +1,44 @@
> > > > > > > +/*
> > > > > > > + * CPU core abstract device
> > > > > > > + *
> > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > + *
> > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > + */
> > > > > > > +#include "hw/cpu/core.h"
> > > > > > > +
> > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > +{
> > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    return g_strdup(core->slot);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > +{
> > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    core->slot = g_strdup(val);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > +{
> > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > +                            NULL);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > +    .abstract = true,
> > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static void cpu_core_register_types(void)
> > > > > > > +{
> > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > +}
> > > > > > > +
> > > > > > > +type_init(cpu_core_register_types)
> > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > new file mode 100644
> > > > > > > index 0000000..2daa724
> > > > > > > --- /dev/null
> > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > @@ -0,0 +1,30 @@
> > > > > > > +/*
> > > > > > > + * CPU core abstract device
> > > > > > > + *
> > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > + *
> > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > + */
> > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > +#define HW_CPU_CORE_H
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "hw/qdev.h"
> > > > > > > +
> > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > +
> > > > > > > +#define CPU_CORE(obj) \
> > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > +
> > > > > > > +typedef struct CPUCore {
> > > > > > > +    /*< private >*/
> > > > > > > +    DeviceState parent_obj;
> > > > > > > +
> > > > > > > +    /*< public >*/
> > > > > > > +    char *slot;
> > > > > > > +} CPUCore;
> > > > > > > +
> > > > > > > +#define CPU_CORE_SLOT_PROP "slot"    
> > > > > > as it's generic property I'd rename to 'core' so it would fit all users    
> > > > > 
> > > > > Ok. Also note that this is a string property which is associated with the
> > > > > link name (string) that we created from machine object to this core. I think
> > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > becomes easier to feed the core location into your CPUSlotProperties.core.  
> > > > agreed, it should be core number.  
> > > 
> > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > some kind of "address" value, but how best to do it is not clear to
> > > me.
> > > 
> > > Changing this to an integer sounds like it's probably a good idea.
> > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > even necessarily have a core id?
> > > 
> > > I'm wondering if the addressing is something that needs to move the
> > > the platform specific subtypes, while some other stuff can move to the
> > > generic base type.
> > >   
> > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > all derived cores would inherit it.
> > > > > > 
> > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > make every created cpu-core object to have threads set
> > > > > > at instance_init() time (device_init).
> > > > > > 
> > > > > > That way user won't have to specify 'threads=y' for every
> > > > > >   device_add spapr-core,core=x
> > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > providing thread property at device_add time:
> > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > 
> > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > but since realize is allowed to fail it should be fine do so.    
> > > > > 
> > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > of creating the threads from either of the property setters. I always
> > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > is fine, it is good.  
> > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > to create internal objects there, as far as proper cleanups are done
> > > > for failure path.  
> > > 
> > > Right, moving the "nr_threads" property to the base type seems like a
> > > good idea to me.  
> > 
> > And we will also move the cpu_model property (now being tracked by
> > an ObjectClass pointer) to the base type ?
> I'm not sure that moving cpu_model to the base class is the right thing,
> I'd keep it local to platform for now.

I tend to agree, although I'm not sure that I could really explain why
:/

> Could you have several spapr core types? One per CPU model?
> That way you won't need to track cpu_model when using device_add.

We could in theory, but it would be pretty inconvenient.  Because this
is a paravirt platform, there really can't be any core-level
difference between them, and it would mean creating a fair batch of
core types for the various minor POWER7 and POWER8 variants - and
needing to update this whenever IBM makes a new version.  I suspect it
would also introduce more wrinkles in order to have a correct
"spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
(HV) only supports the HOST thread type, that's a fairly big issue.

David Gibson March 8, 2016, 4:26 a.m. UTC | #9

On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:
> On Mon, 7 Mar 2016 14:36:55 +1100
> David Gibson <[e-mail]> wrote:
> 
> > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:
> > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > Bharata B Rao <[e-mail]> wrote:
> > >   
> > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:  
> > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > Bharata B Rao <[e-mail]> wrote:
> > > > >     
> > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > to define and hotplug CPUs in core granularity.
> > > > > > 
> > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > ---
> > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 75 insertions(+)
> > > > > >  create mode 100644 hw/cpu/core.c
> > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > 
> > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > index 0954a18..942a4bb 100644
> > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > +obj-y += core.o
> > > > > >  
> > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > new file mode 100644
> > > > > > index 0000000..d8caf37
> > > > > > --- /dev/null
> > > > > > +++ b/hw/cpu/core.c
> > > > > > @@ -0,0 +1,44 @@
> > > > > > +/*
> > > > > > + * CPU core abstract device
> > > > > > + *
> > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +#include "hw/cpu/core.h"
> > > > > > +
> > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > +{
> > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > +
> > > > > > +    return g_strdup(core->slot);
> > > > > > +}
> > > > > > +
> > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > +{
> > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > +
> > > > > > +    core->slot = g_strdup(val);
> > > > > > +}
> > > > > > +
> > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > +{
> > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > +                            NULL);
> > > > > > +}
> > > > > > +
> > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > +    .parent = TYPE_DEVICE,
> > > > > > +    .abstract = true,
> > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > +};
> > > > > > +
> > > > > > +static void cpu_core_register_types(void)
> > > > > > +{
> > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > +}
> > > > > > +
> > > > > > +type_init(cpu_core_register_types)
> > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > new file mode 100644
> > > > > > index 0000000..2daa724
> > > > > > --- /dev/null
> > > > > > +++ b/include/hw/cpu/core.h
> > > > > > @@ -0,0 +1,30 @@
> > > > > > +/*
> > > > > > + * CPU core abstract device
> > > > > > + *
> > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > + *
> > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > + * See the COPYING file in the top-level directory.
> > > > > > + */
> > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > +#define HW_CPU_CORE_H
> > > > > > +
> > > > > > +#include "qemu/osdep.h"
> > > > > > +#include "hw/qdev.h"
> > > > > > +
> > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > +
> > > > > > +#define CPU_CORE(obj) \
> > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > +
> > > > > > +typedef struct CPUCore {
> > > > > > +    /*< private >*/
> > > > > > +    DeviceState parent_obj;
> > > > > > +
> > > > > > +    /*< public >*/
> > > > > > +    char *slot;
> > > > > > +} CPUCore;
> > > > > > +
> > > > > > +#define CPU_CORE_SLOT_PROP "slot"    
> > > > > as it's generic property I'd rename to 'core' so it would fit all users    
> > > > 
> > > > Ok. Also note that this is a string property which is associated with the
> > > > link name (string) that we created from machine object to this core. I think
> > > > it would be ideal if this becomes an interger  property in which case it
> > > > becomes easier to feed the core location into your CPUSlotProperties.core.  
> > > agreed, it should be core number.  
> > 
> > The slot stuff is continuing to confuse me a bit.  I see that we need
> > some kind of "address" value, but how best to do it is not clear to
> > me.
> > 
> > Changing this to an integer sounds like it's probably a good idea.
> > I'm a bit wary of just calling it "core" though.  Do all platforms
> > even necessarily have a core id?
> platform's that don't have core concept could or even should
> use its own base type (i.e. not cpu-core).

Hmm.. that's a good point.  And actually makes me inclined to
suggest including the cpu model property in the base type, contrary to
my suggestion earlier.

I can think of (somewhat contrived) cases of cpu packages where
cpu_model doesn't make sense (e.g. a multi-chip bigLITTLE system,
since there are multiple CPU types in a package), but in that case the
package doesn't really resemble a "core" in any normal sense.

> Numeric code id should work for x86, ARM and Power.

Yes.  I think a numeric id should be fine in general.  Whether it's
actually meaningful with regard to platform docs, or completely
arbitrary might vary by platform, but it should be possible to create
something.

> > I'm wondering if the addressing is something that needs to move the
> > the platform specific subtypes, while some other stuff can move to the
> > generic base type.
> core id looks to me as cpu-core property but I won't object if
> it will be moved to subtype as so far only Power would have it.
> 
> What I'd prefer to keep is consistent naming of properties
> if it's possible, i.e. property 'core' which makes sense for x86, ARM, and Power
> from enduser point of view.
> 
> > 
> > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > all derived cores would inherit it.
> > > > > 
> > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > make every created cpu-core object to have threads set
> > > > > at instance_init() time (device_init).
> > > > > 
> > > > > That way user won't have to specify 'threads=y' for every
> > > > >   device_add spapr-core,core=x
> > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > but if user wishes he/she still could override global by explicitly
> > > > > providing thread property at device_add time:
> > > > >   device_add spapr-core,core=x,threads=y
> > > > > 
> > > > > wrt this series it would mean, instead of creating threads in property
> > > > > setter, delaying threads creation to core.realize() time,
> > > > > but since realize is allowed to fail it should be fine do so.    
> > > > 
> > > > Ok that would suit us as there are two properties on which thread creation
> > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > created at core realize time, then we don't have to resort to the ugliness
> > > > of creating the threads from either of the property setters. I always
> > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > is fine, it is good.  
> > > since realize is allowed to fail, it should be safe from hotplug pov
> > > to create internal objects there, as far as proper cleanups are done
> > > for failure path.  
> > 
> [...]
> > I'm not clear from the above if you're also intending to move at least
> > the adding of the threads as child properties is supposed to go into
> > the base type,
> I'm not sure that I've got question, could you please rephrase?

So, it seems like we're agreed that moving the nr_threads property to
the base type is a good idea.

My question is, do we also move the object_property_add_child() calls
for each thread to the base type (possibly via a helper function or
method the base type provides to derived types)?

> > but that also sounds like a good idea, again for
> > consistency.

Igor Mammedov March 8, 2016, 9:11 a.m. UTC | #10

On Tue, 8 Mar 2016 14:57:10 +1100
David Gibson <[e-mail]> wrote:

> On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:
> > On Mon, 7 Mar 2016 14:01:55 +0530
> > Bharata B Rao <[e-mail]> wrote:
> >   
> > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:  
> > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:    
> > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > Bharata B Rao <[e-mail]> wrote:
> > > > >     
> > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:    
> > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > >       
> > > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > 
> > > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > > ---
> > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > 
> > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > +obj-y += core.o
> > > > > > > >  
> > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..d8caf37
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > +/*
> > > > > > > > + * CPU core abstract device
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > + *
> > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > + */
> > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > +
> > > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > +{
> > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > +
> > > > > > > > +    return g_strdup(core->slot);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > > +{
> > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > +
> > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > +{
> > > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > > +                            NULL);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > > +    .abstract = true,
> > > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > +{
> > > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..2daa724
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > > @@ -0,0 +1,30 @@
> > > > > > > > +/*
> > > > > > > > + * CPU core abstract device
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > + *
> > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > + */
> > > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > > +#define HW_CPU_CORE_H
> > > > > > > > +
> > > > > > > > +#include "qemu/osdep.h"
> > > > > > > > +#include "hw/qdev.h"
> > > > > > > > +
> > > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > > +
> > > > > > > > +#define CPU_CORE(obj) \
> > > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > > +
> > > > > > > > +typedef struct CPUCore {
> > > > > > > > +    /*< private >*/
> > > > > > > > +    DeviceState parent_obj;
> > > > > > > > +
> > > > > > > > +    /*< public >*/
> > > > > > > > +    char *slot;
> > > > > > > > +} CPUCore;
> > > > > > > > +
> > > > > > > > +#define CPU_CORE_SLOT_PROP "slot"      
> > > > > > > as it's generic property I'd rename to 'core' so it would fit all users      
> > > > > > 
> > > > > > Ok. Also note that this is a string property which is associated with the
> > > > > > link name (string) that we created from machine object to this core. I think
> > > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > > becomes easier to feed the core location into your CPUSlotProperties.core.    
> > > > > agreed, it should be core number.    
> > > > 
> > > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > > some kind of "address" value, but how best to do it is not clear to
> > > > me.
> > > > 
> > > > Changing this to an integer sounds like it's probably a good idea.
> > > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > > even necessarily have a core id?
> > > > 
> > > > I'm wondering if the addressing is something that needs to move the
> > > > the platform specific subtypes, while some other stuff can move to the
> > > > generic base type.
> > > >     
> > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > all derived cores would inherit it.
> > > > > > > 
> > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > make every created cpu-core object to have threads set
> > > > > > > at instance_init() time (device_init).
> > > > > > > 
> > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > >   device_add spapr-core,core=x
> > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > providing thread property at device_add time:
> > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > 
> > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > but since realize is allowed to fail it should be fine do so.      
> > > > > > 
> > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > of creating the threads from either of the property setters. I always
> > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > is fine, it is good.    
> > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > to create internal objects there, as far as proper cleanups are done
> > > > > for failure path.    
> > > > 
> > > > Right, moving the "nr_threads" property to the base type seems like a
> > > > good idea to me.    
> > > 
> > > And we will also move the cpu_model property (now being tracked by
> > > an ObjectClass pointer) to the base type ?  
> > I'm not sure that moving cpu_model to the base class is the right thing,
> > I'd keep it local to platform for now.  
> 
> I tend to agree, although I'm not sure that I could really explain why
> :/
> 
> > Could you have several spapr core types? One per CPU model?
> > That way you won't need to track cpu_model when using device_add.  
> 
> We could in theory, but it would be pretty inconvenient.  Because this
> is a paravirt platform, there really can't be any core-level
> difference between them, and it would mean creating a fair batch of
> core types for the various minor POWER7 and POWER8 variants - and
> needing to update this whenever IBM makes a new version.  I suspect it
> would also introduce more wrinkles in order to have a correct
> "spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
> (HV) only supports the HOST thread type, that's a fairly big issue.
Welcome to x86 world, that's roughly what we have there.

David Gibson March 9, 2016, 2:55 a.m. UTC | #11

On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 14:57:10 +1100
> David Gibson <[e-mail]> wrote:
> 
> > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:
> > > On Mon, 7 Mar 2016 14:01:55 +0530
> > > Bharata B Rao <[e-mail]> wrote:
> > >   
> > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:  
> > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:    
> > > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > >     
> > > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:    
> > > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > >       
> > > > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > > > ---
> > > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > > 
> > > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > > +obj-y += core.o
> > > > > > > > >  
> > > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > > new file mode 100644
> > > > > > > > > index 0000000..d8caf37
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > +/*
> > > > > > > > > + * CPU core abstract device
> > > > > > > > > + *
> > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > + *
> > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > + */
> > > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > > +
> > > > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > > +{
> > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > +
> > > > > > > > > +    return g_strdup(core->slot);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > > > +{
> > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > +
> > > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > > +{
> > > > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > > > +                            NULL);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > > > +    .abstract = true,
> > > > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > > > +};
> > > > > > > > > +
> > > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > > +{
> > > > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > > new file mode 100644
> > > > > > > > > index 0000000..2daa724
> > > > > > > > > --- /dev/null
> > > > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > > > @@ -0,0 +1,30 @@
> > > > > > > > > +/*
> > > > > > > > > + * CPU core abstract device
> > > > > > > > > + *
> > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > + *
> > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > + */
> > > > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > > > +#define HW_CPU_CORE_H
> > > > > > > > > +
> > > > > > > > > +#include "qemu/osdep.h"
> > > > > > > > > +#include "hw/qdev.h"
> > > > > > > > > +
> > > > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > > > +
> > > > > > > > > +#define CPU_CORE(obj) \
> > > > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > > > +
> > > > > > > > > +typedef struct CPUCore {
> > > > > > > > > +    /*< private >*/
> > > > > > > > > +    DeviceState parent_obj;
> > > > > > > > > +
> > > > > > > > > +    /*< public >*/
> > > > > > > > > +    char *slot;
> > > > > > > > > +} CPUCore;
> > > > > > > > > +
> > > > > > > > > +#define CPU_CORE_SLOT_PROP "slot"      
> > > > > > > > as it's generic property I'd rename to 'core' so it would fit all users      
> > > > > > > 
> > > > > > > Ok. Also note that this is a string property which is associated with the
> > > > > > > link name (string) that we created from machine object to this core. I think
> > > > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > > > becomes easier to feed the core location into your CPUSlotProperties.core.    
> > > > > > agreed, it should be core number.    
> > > > > 
> > > > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > > > some kind of "address" value, but how best to do it is not clear to
> > > > > me.
> > > > > 
> > > > > Changing this to an integer sounds like it's probably a good idea.
> > > > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > > > even necessarily have a core id?
> > > > > 
> > > > > I'm wondering if the addressing is something that needs to move the
> > > > > the platform specific subtypes, while some other stuff can move to the
> > > > > generic base type.
> > > > >     
> > > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > > all derived cores would inherit it.
> > > > > > > > 
> > > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > at instance_init() time (device_init).
> > > > > > > > 
> > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > >   device_add spapr-core,core=x
> > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > > providing thread property at device_add time:
> > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > 
> > > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > but since realize is allowed to fail it should be fine do so.      
> > > > > > > 
> > > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > > of creating the threads from either of the property setters. I always
> > > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > > is fine, it is good.    
> > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > for failure path.    
> > > > > 
> > > > > Right, moving the "nr_threads" property to the base type seems like a
> > > > > good idea to me.    
> > > > 
> > > > And we will also move the cpu_model property (now being tracked by
> > > > an ObjectClass pointer) to the base type ?  
> > > I'm not sure that moving cpu_model to the base class is the right thing,
> > > I'd keep it local to platform for now.  
> > 
> > I tend to agree, although I'm not sure that I could really explain why
> > :/
> > 
> > > Could you have several spapr core types? One per CPU model?
> > > That way you won't need to track cpu_model when using device_add.  
> > 
> > We could in theory, but it would be pretty inconvenient.  Because this
> > is a paravirt platform, there really can't be any core-level
> > difference between them, and it would mean creating a fair batch of
> > core types for the various minor POWER7 and POWER8 variants - and
> > needing to update this whenever IBM makes a new version.  I suspect it
> > would also introduce more wrinkles in order to have a correct
> > "spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
> > (HV) only supports the HOST thread type, that's a fairly big issue.
> Welcome to x86 world, that's roughly what we have there.

I don't really follow you.  x86 doesn't have core devices at all for
the moment.

What I'm saying here is that using different core subtypes for every
cpu subtype on power would mean 2 types for each minor variant, rather
than just 1.

Igor Mammedov March 9, 2016, 10:32 a.m. UTC | #12

On Wed, 9 Mar 2016 13:55:51 +1100
David Gibson <[e-mail]> wrote:

> On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Mar 2016 14:57:10 +1100
> > David Gibson <[e-mail]> wrote:
> >   
> > > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:  
> > > > On Mon, 7 Mar 2016 14:01:55 +0530
> > > > Bharata B Rao <[e-mail]> wrote:
> > > >     
> > > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:    
> > > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:      
> > > > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > >       
> > > > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:      
> > > > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > > >         
> > > > > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > > > > ---
> > > > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > > > 
> > > > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > > > +obj-y += core.o
> > > > > > > > > >  
> > > > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 0000000..d8caf37
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > > +/*
> > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > + *
> > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > + *
> > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > + */
> > > > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > > > +
> > > > > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > > > +{
> > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > +
> > > > > > > > > > +    return g_strdup(core->slot);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > > > > +{
> > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > +
> > > > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > > > +{
> > > > > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > > > > +                            NULL);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > > > > +    .abstract = true,
> > > > > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > > > > +};
> > > > > > > > > > +
> > > > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > > > +{
> > > > > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 0000000..2daa724
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > > > > @@ -0,0 +1,30 @@
> > > > > > > > > > +/*
> > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > + *
> > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > + *
> > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > + */
> > > > > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > > > > +#define HW_CPU_CORE_H
> > > > > > > > > > +
> > > > > > > > > > +#include "qemu/osdep.h"
> > > > > > > > > > +#include "hw/qdev.h"
> > > > > > > > > > +
> > > > > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > > > > +
> > > > > > > > > > +#define CPU_CORE(obj) \
> > > > > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > > > > +
> > > > > > > > > > +typedef struct CPUCore {
> > > > > > > > > > +    /*< private >*/
> > > > > > > > > > +    DeviceState parent_obj;
> > > > > > > > > > +
> > > > > > > > > > +    /*< public >*/
> > > > > > > > > > +    char *slot;
> > > > > > > > > > +} CPUCore;
> > > > > > > > > > +
> > > > > > > > > > +#define CPU_CORE_SLOT_PROP "slot"        
> > > > > > > > > as it's generic property I'd rename to 'core' so it would fit all users        
> > > > > > > > 
> > > > > > > > Ok. Also note that this is a string property which is associated with the
> > > > > > > > link name (string) that we created from machine object to this core. I think
> > > > > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > > > > becomes easier to feed the core location into your CPUSlotProperties.core.      
> > > > > > > agreed, it should be core number.      
> > > > > > 
> > > > > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > > > > some kind of "address" value, but how best to do it is not clear to
> > > > > > me.
> > > > > > 
> > > > > > Changing this to an integer sounds like it's probably a good idea.
> > > > > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > > > > even necessarily have a core id?
> > > > > > 
> > > > > > I'm wondering if the addressing is something that needs to move the
> > > > > > the platform specific subtypes, while some other stuff can move to the
> > > > > > generic base type.
> > > > > >       
> > > > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > > > all derived cores would inherit it.
> > > > > > > > > 
> > > > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > > at instance_init() time (device_init).
> > > > > > > > > 
> > > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > > >   device_add spapr-core,core=x
> > > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > > > providing thread property at device_add time:
> > > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > > 
> > > > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > > but since realize is allowed to fail it should be fine do so.        
> > > > > > > > 
> > > > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > > > of creating the threads from either of the property setters. I always
> > > > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > > > is fine, it is good.      
> > > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > > for failure path.      
> > > > > > 
> > > > > > Right, moving the "nr_threads" property to the base type seems like a
> > > > > > good idea to me.      
> > > > > 
> > > > > And we will also move the cpu_model property (now being tracked by
> > > > > an ObjectClass pointer) to the base type ?    
> > > > I'm not sure that moving cpu_model to the base class is the right thing,
> > > > I'd keep it local to platform for now.    
> > > 
> > > I tend to agree, although I'm not sure that I could really explain why
> > > :/
> > >   
> > > > Could you have several spapr core types? One per CPU model?
> > > > That way you won't need to track cpu_model when using device_add.    
> > > 
> > > We could in theory, but it would be pretty inconvenient.  Because this
> > > is a paravirt platform, there really can't be any core-level
> > > difference between them, and it would mean creating a fair batch of
> > > core types for the various minor POWER7 and POWER8 variants - and
> > > needing to update this whenever IBM makes a new version.  I suspect it
> > > would also introduce more wrinkles in order to have a correct
> > > "spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
> > > (HV) only supports the HOST thread type, that's a fairly big issue.  
> > Welcome to x86 world, that's roughly what we have there.  
> 
> I don't really follow you.  x86 doesn't have core devices at all for
> the moment.
> 
> What I'm saying here is that using different core subtypes for every
> cpu subtype on power would mean 2 types for each minor variant, rather
> than just 1.
I think the same applies to cpu_model and transitioning CPUs to -device.
X86 also has a bunch of cpu_model-s which have some minor variations
but it still generates a type per cpu_model.
If some day we are to implement socket objects for x86 that would also
mean to have a socket types per each cpu model.
As I understand cpu_model is a legacy option which should translate to
a corresponding QOM type (CPU device) which could be used with
-device/device_add.

As analogy, QEMU has legacy -net model=foo[1234] option, but when network cards
were converted to -device interface that in the end became a set of QOM types
like -device foo[1234], it was easier in case of network cards as
they where a separate devices models to begin with, the thing to note
here is that they weren't converted to a single 'network_card' type with
'model' property.

Igor Mammedov March 9, 2016, 10:40 a.m. UTC | #13

On Tue, 8 Mar 2016 15:26:27 +1100
David Gibson <[e-mail]> wrote:

> On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:
> > On Mon, 7 Mar 2016 14:36:55 +1100
> > David Gibson <[e-mail]> wrote:
> >   
> > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:  
> > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > Bharata B Rao <[e-mail]> wrote:
> > > >     
> > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:    
> > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > >       
> > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > 
> > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > ---
> > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 75 insertions(+)
> > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > 
> > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > index 0954a18..942a4bb 100644
> > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > +obj-y += core.o
> > > > > > >  
> > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > new file mode 100644
> > > > > > > index 0000000..d8caf37
> > > > > > > --- /dev/null
> > > > > > > +++ b/hw/cpu/core.c
> > > > > > > @@ -0,0 +1,44 @@
> > > > > > > +/*
> > > > > > > + * CPU core abstract device
> > > > > > > + *
> > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > + *
> > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > + */
> > > > > > > +#include "hw/cpu/core.h"
> > > > > > > +
> > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > +{
> > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    return g_strdup(core->slot);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > +{
> > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > +
> > > > > > > +    core->slot = g_strdup(val);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > +{
> > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > +                            NULL);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > +    .abstract = true,
> > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static void cpu_core_register_types(void)
> > > > > > > +{
> > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > +}
> > > > > > > +
> > > > > > > +type_init(cpu_core_register_types)
> > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > new file mode 100644
> > > > > > > index 0000000..2daa724
> > > > > > > --- /dev/null
> > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > @@ -0,0 +1,30 @@
> > > > > > > +/*
> > > > > > > + * CPU core abstract device
> > > > > > > + *
> > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > + *
> > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > + */
> > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > +#define HW_CPU_CORE_H
> > > > > > > +
> > > > > > > +#include "qemu/osdep.h"
> > > > > > > +#include "hw/qdev.h"
> > > > > > > +
> > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > +
> > > > > > > +#define CPU_CORE(obj) \
> > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > +
> > > > > > > +typedef struct CPUCore {
> > > > > > > +    /*< private >*/
> > > > > > > +    DeviceState parent_obj;
> > > > > > > +
> > > > > > > +    /*< public >*/
> > > > > > > +    char *slot;
> > > > > > > +} CPUCore;
> > > > > > > +
> > > > > > > +#define CPU_CORE_SLOT_PROP "slot"      
> > > > > > as it's generic property I'd rename to 'core' so it would fit all users      
> > > > > 
> > > > > Ok. Also note that this is a string property which is associated with the
> > > > > link name (string) that we created from machine object to this core. I think
> > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > becomes easier to feed the core location into your CPUSlotProperties.core.    
> > > > agreed, it should be core number.    
> > > 
> > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > some kind of "address" value, but how best to do it is not clear to
> > > me.
> > > 
> > > Changing this to an integer sounds like it's probably a good idea.
> > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > even necessarily have a core id?  
> > platform's that don't have core concept could or even should
> > use its own base type (i.e. not cpu-core).  
> 
> Hmm.. that's a good point.  And actually makes me inclined to
> suggest including the cpu model property in the base type, contrary to
> my suggestion earlier.
I've answered wrt cpu_model in "v1 05/10] cpu: Abstract CPU core type" thread.
You can try to ping/ask Andreas on IRC what he thinks about it,
but I think he would be against cpu_model property in core.

> 
> I can think of (somewhat contrived) cases of cpu packages where
> cpu_model doesn't make sense (e.g. a multi-chip bigLITTLE system,
> since there are multiple CPU types in a package), but in that case the
> package doesn't really resemble a "core" in any normal sense.
> 
> > Numeric code id should work for x86, ARM and Power.  
> 
> Yes.  I think a numeric id should be fine in general.  Whether it's
> actually meaningful with regard to platform docs, or completely
> arbitrary might vary by platform, but it should be possible to create
> something.
> 
> > > I'm wondering if the addressing is something that needs to move the
> > > the platform specific subtypes, while some other stuff can move to the
> > > generic base type.  
> > core id looks to me as cpu-core property but I won't object if
> > it will be moved to subtype as so far only Power would have it.
> > 
> > What I'd prefer to keep is consistent naming of properties
> > if it's possible, i.e. property 'core' which makes sense for x86, ARM, and Power
> > from enduser point of view.
> >   
> > >   
> > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > all derived cores would inherit it.
> > > > > > 
> > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > make every created cpu-core object to have threads set
> > > > > > at instance_init() time (device_init).
> > > > > > 
> > > > > > That way user won't have to specify 'threads=y' for every
> > > > > >   device_add spapr-core,core=x
> > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > providing thread property at device_add time:
> > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > 
> > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > but since realize is allowed to fail it should be fine do so.      
> > > > > 
> > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > of creating the threads from either of the property setters. I always
> > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > is fine, it is good.    
> > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > to create internal objects there, as far as proper cleanups are done
> > > > for failure path.    
> > >   
> > [...]  
> > > I'm not clear from the above if you're also intending to move at least
> > > the adding of the threads as child properties is supposed to go into
> > > the base type,  
> > I'm not sure that I've got question, could you please rephrase?  
> 
> So, it seems like we're agreed that moving the nr_threads property to
> the base type is a good idea.
> 
> My question is, do we also move the object_property_add_child() calls
> for each thread to the base type (possibly via a helper function or
> method the base type provides to derived types)?
I can't think of a reason to do so,
why can't subtype-core.realize() do it?
What would one gain creating callbacks and calling them from base class?

> 
> > > but that also sounds like a good idea, again for
> > > consistency.  
>

David Gibson March 9, 2016, 11:42 p.m. UTC | #14

On Wed, Mar 09, 2016 at 11:40:53AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 15:26:27 +1100
> David Gibson <[e-mail]> wrote:
> > On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:
> > > On Mon, 7 Mar 2016 14:36:55 +1100
> > > David Gibson <[e-mail]> wrote:
[snip]
> > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > all derived cores would inherit it.
> > > > > > > 
> > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > make every created cpu-core object to have threads set
> > > > > > > at instance_init() time (device_init).
> > > > > > > 
> > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > >   device_add spapr-core,core=x
> > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > providing thread property at device_add time:
> > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > 
> > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > but since realize is allowed to fail it should be fine do so.      
> > > > > > 
> > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > of creating the threads from either of the property setters. I always
> > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > is fine, it is good.    
> > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > to create internal objects there, as far as proper cleanups are done
> > > > > for failure path.    
> > > >   
> > > [...]  
> > > > I'm not clear from the above if you're also intending to move at least
> > > > the adding of the threads as child properties is supposed to go into
> > > > the base type,  
> > > I'm not sure that I've got question, could you please rephrase?  
> > 
> > So, it seems like we're agreed that moving the nr_threads property to
> > the base type is a good idea.
> > 
> > My question is, do we also move the object_property_add_child() calls
> > for each thread to the base type (possibly via a helper function or
> > method the base type provides to derived types)?
> I can't think of a reason to do so,
> why can't subtype-core.realize() do it?

It can, but I'm always suspicious of boilerplate stuff that every
subtype *has* to do in order to work properly.

> What would one gain creating callbacks and calling them from base class?

Enforcing - or at least making as easy as possible - consistency in
the child object naming.

David Gibson March 10, 2016, 5:04 a.m. UTC | #15

On Wed, Mar 09, 2016 at 11:32:28AM +0100, Igor Mammedov wrote:
> On Wed, 9 Mar 2016 13:55:51 +1100
> David Gibson <[e-mail]> wrote:
> 
> > On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote:
> > > On Tue, 8 Mar 2016 14:57:10 +1100
> > > David Gibson <[e-mail]> wrote:
> > >   
> > > > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:  
> > > > > On Mon, 7 Mar 2016 14:01:55 +0530
> > > > > Bharata B Rao <[e-mail]> wrote:
> > > > >     
> > > > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:    
> > > > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:      
> > > > > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > >       
> > > > > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:      
> > > > > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > > > >         
> > > > > > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > > > > > ---
> > > > > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > > > > 
> > > > > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > > > > +obj-y += core.o
> > > > > > > > > > >  
> > > > > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 0000000..d8caf37
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > > > +/*
> > > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > > + *
> > > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > > + *
> > > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > > + */
> > > > > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > > > > +
> > > > > > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > > > > +{
> > > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > +
> > > > > > > > > > > +    return g_strdup(core->slot);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > > > > > +{
> > > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > +
> > > > > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > > > > +{
> > > > > > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > > > > > +                            NULL);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > > > > > +    .abstract = true,
> > > > > > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > > > > > +};
> > > > > > > > > > > +
> > > > > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > > > > +{
> > > > > > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > > > > new file mode 100644
> > > > > > > > > > > index 0000000..2daa724
> > > > > > > > > > > --- /dev/null
> > > > > > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > > > > > @@ -0,0 +1,30 @@
> > > > > > > > > > > +/*
> > > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > > + *
> > > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > > + *
> > > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > > + */
> > > > > > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > > > > > +#define HW_CPU_CORE_H
> > > > > > > > > > > +
> > > > > > > > > > > +#include "qemu/osdep.h"
> > > > > > > > > > > +#include "hw/qdev.h"
> > > > > > > > > > > +
> > > > > > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > > > > > +
> > > > > > > > > > > +#define CPU_CORE(obj) \
> > > > > > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > > > > > +
> > > > > > > > > > > +typedef struct CPUCore {
> > > > > > > > > > > +    /*< private >*/
> > > > > > > > > > > +    DeviceState parent_obj;
> > > > > > > > > > > +
> > > > > > > > > > > +    /*< public >*/
> > > > > > > > > > > +    char *slot;
> > > > > > > > > > > +} CPUCore;
> > > > > > > > > > > +
> > > > > > > > > > > +#define CPU_CORE_SLOT_PROP "slot"        
> > > > > > > > > > as it's generic property I'd rename to 'core' so it would fit all users        
> > > > > > > > > 
> > > > > > > > > Ok. Also note that this is a string property which is associated with the
> > > > > > > > > link name (string) that we created from machine object to this core. I think
> > > > > > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > > > > > becomes easier to feed the core location into your CPUSlotProperties.core.      
> > > > > > > > agreed, it should be core number.      
> > > > > > > 
> > > > > > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > > > > > some kind of "address" value, but how best to do it is not clear to
> > > > > > > me.
> > > > > > > 
> > > > > > > Changing this to an integer sounds like it's probably a good idea.
> > > > > > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > > > > > even necessarily have a core id?
> > > > > > > 
> > > > > > > I'm wondering if the addressing is something that needs to move the
> > > > > > > the platform specific subtypes, while some other stuff can move to the
> > > > > > > generic base type.
> > > > > > >       
> > > > > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > > > > all derived cores would inherit it.
> > > > > > > > > > 
> > > > > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > > > at instance_init() time (device_init).
> > > > > > > > > > 
> > > > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > > > >   device_add spapr-core,core=x
> > > > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > > > > providing thread property at device_add time:
> > > > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > > > 
> > > > > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > > > but since realize is allowed to fail it should be fine do so.        
> > > > > > > > > 
> > > > > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > > > > of creating the threads from either of the property setters. I always
> > > > > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > > > > is fine, it is good.      
> > > > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > > > for failure path.      
> > > > > > > 
> > > > > > > Right, moving the "nr_threads" property to the base type seems like a
> > > > > > > good idea to me.      
> > > > > > 
> > > > > > And we will also move the cpu_model property (now being tracked by
> > > > > > an ObjectClass pointer) to the base type ?    
> > > > > I'm not sure that moving cpu_model to the base class is the right thing,
> > > > > I'd keep it local to platform for now.    
> > > > 
> > > > I tend to agree, although I'm not sure that I could really explain why
> > > > :/
> > > >   
> > > > > Could you have several spapr core types? One per CPU model?
> > > > > That way you won't need to track cpu_model when using device_add.    
> > > > 
> > > > We could in theory, but it would be pretty inconvenient.  Because this
> > > > is a paravirt platform, there really can't be any core-level
> > > > difference between them, and it would mean creating a fair batch of
> > > > core types for the various minor POWER7 and POWER8 variants - and
> > > > needing to update this whenever IBM makes a new version.  I suspect it
> > > > would also introduce more wrinkles in order to have a correct
> > > > "spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
> > > > (HV) only supports the HOST thread type, that's a fairly big issue.  
> > > Welcome to x86 world, that's roughly what we have there.  
> > 
> > I don't really follow you.  x86 doesn't have core devices at all for
> > the moment.
> > 
> > What I'm saying here is that using different core subtypes for every
> > cpu subtype on power would mean 2 types for each minor variant, rather
> > than just 1.
> I think the same applies to cpu_model and transitioning CPUs to -device.
> X86 also has a bunch of cpu_model-s which have some minor variations
> but it still generates a type per cpu_model.
> If some day we are to implement socket objects for x86 that would also
> mean to have a socket types per each cpu model.

Hmm.  I guess.

What concerns me is the possible combinatorial explosion of # cpu
models * # of machine types leading to an enormous number of
core/socket types.

The other thing is that the platform specific core types belong with
the machine type code, which means they can't naturally use macro
magic or whatever to generate them in parallel with the thread device
types in the core target-ppc code.

> As I understand cpu_model is a legacy option which should translate to
> a corresponding QOM type (CPU device) which could be used with
> -device/device_add.

Sure, but the "cpu_model" parameter could be "thread type name" just
as easily.

> As analogy, QEMU has legacy -net model=foo[1234] option, but when network cards
> were converted to -device interface that in the end became a set of QOM types
> like -device foo[1234], it was easier in case of network cards as
> they where a separate devices models to begin with, the thing to note
> here is that they weren't converted to a single 'network_card' type with
> 'model' property.

Igor Mammedov March 10, 2016, 9:35 a.m. UTC | #16

On Thu, 10 Mar 2016 16:04:29 +1100
David Gibson <[e-mail]> wrote:

> On Wed, Mar 09, 2016 at 11:32:28AM +0100, Igor Mammedov wrote:
> > On Wed, 9 Mar 2016 13:55:51 +1100
> > David Gibson <[e-mail]> wrote:
> >   
> > > On Tue, Mar 08, 2016 at 10:11:17AM +0100, Igor Mammedov wrote:  
> > > > On Tue, 8 Mar 2016 14:57:10 +1100
> > > > David Gibson <[e-mail]> wrote:
> > > >     
> > > > > On Mon, Mar 07, 2016 at 11:40:11AM +0100, Igor Mammedov wrote:    
> > > > > > On Mon, 7 Mar 2016 14:01:55 +0530
> > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > >       
> > > > > > > On Mon, Mar 07, 2016 at 02:36:55PM +1100, David Gibson wrote:      
> > > > > > > > On Fri, Mar 04, 2016 at 07:07:20PM +0100, Igor Mammedov wrote:        
> > > > > > > > > On Fri, 4 Mar 2016 16:32:53 +0530
> > > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > > >         
> > > > > > > > > > On Fri, Mar 04, 2016 at 11:38:45AM +0100, Igor Mammedov wrote:        
> > > > > > > > > > > On Fri,  4 Mar 2016 12:24:16 +0530
> > > > > > > > > > > Bharata B Rao <[e-mail]> wrote:
> > > > > > > > > > >           
> > > > > > > > > > > > Add an abstract CPU core type that could be used by machines that want
> > > > > > > > > > > > to define and hotplug CPUs in core granularity.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Bharata B Rao <[e-mail]>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  hw/cpu/Makefile.objs  |  1 +
> > > > > > > > > > > >  hw/cpu/core.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > > > > > >  include/hw/cpu/core.h | 30 ++++++++++++++++++++++++++++++
> > > > > > > > > > > >  3 files changed, 75 insertions(+)
> > > > > > > > > > > >  create mode 100644 hw/cpu/core.c
> > > > > > > > > > > >  create mode 100644 include/hw/cpu/core.h
> > > > > > > > > > > > 
> > > > > > > > > > > > diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
> > > > > > > > > > > > index 0954a18..942a4bb 100644
> > > > > > > > > > > > --- a/hw/cpu/Makefile.objs
> > > > > > > > > > > > +++ b/hw/cpu/Makefile.objs
> > > > > > > > > > > > @@ -2,4 +2,5 @@ obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
> > > > > > > > > > > >  obj-$(CONFIG_REALVIEW) += realview_mpcore.o
> > > > > > > > > > > >  obj-$(CONFIG_A9MPCORE) += a9mpcore.o
> > > > > > > > > > > >  obj-$(CONFIG_A15MPCORE) += a15mpcore.o
> > > > > > > > > > > > +obj-y += core.o
> > > > > > > > > > > >  
> > > > > > > > > > > > diff --git a/hw/cpu/core.c b/hw/cpu/core.c
> > > > > > > > > > > > new file mode 100644
> > > > > > > > > > > > index 0000000..d8caf37
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/hw/cpu/core.c
> > > > > > > > > > > > @@ -0,0 +1,44 @@
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > > > + */
> > > > > > > > > > > > +#include "hw/cpu/core.h"
> > > > > > > > > > > > +
> > > > > > > > > > > > +static char *core_prop_get_slot(Object *obj, Error **errp)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > > +
> > > > > > > > > > > > +    return g_strdup(core->slot);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    CPUCore *core = CPU_CORE(obj);
> > > > > > > > > > > > +
> > > > > > > > > > > > +    core->slot = g_strdup(val);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +static void cpu_core_instance_init(Object *obj)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
> > > > > > > > > > > > +                            NULL);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +static const TypeInfo cpu_core_type_info = {
> > > > > > > > > > > > +    .name = TYPE_CPU_CORE,
> > > > > > > > > > > > +    .parent = TYPE_DEVICE,
> > > > > > > > > > > > +    .abstract = true,
> > > > > > > > > > > > +    .instance_size = sizeof(CPUCore),
> > > > > > > > > > > > +    .instance_init = cpu_core_instance_init,
> > > > > > > > > > > > +};
> > > > > > > > > > > > +
> > > > > > > > > > > > +static void cpu_core_register_types(void)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +    type_register_static(&cpu_core_type_info);
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +type_init(cpu_core_register_types)
> > > > > > > > > > > > diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
> > > > > > > > > > > > new file mode 100644
> > > > > > > > > > > > index 0000000..2daa724
> > > > > > > > > > > > --- /dev/null
> > > > > > > > > > > > +++ b/include/hw/cpu/core.h
> > > > > > > > > > > > @@ -0,0 +1,30 @@
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * CPU core abstract device
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * Copyright (C) 2016 Bharata B Rao <[e-mail]>
> > > > > > > > > > > > + *
> > > > > > > > > > > > + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> > > > > > > > > > > > + * See the COPYING file in the top-level directory.
> > > > > > > > > > > > + */
> > > > > > > > > > > > +#ifndef HW_CPU_CORE_H
> > > > > > > > > > > > +#define HW_CPU_CORE_H
> > > > > > > > > > > > +
> > > > > > > > > > > > +#include "qemu/osdep.h"
> > > > > > > > > > > > +#include "hw/qdev.h"
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define TYPE_CPU_CORE "cpu-core"
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define CPU_CORE(obj) \
> > > > > > > > > > > > +    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
> > > > > > > > > > > > +
> > > > > > > > > > > > +typedef struct CPUCore {
> > > > > > > > > > > > +    /*< private >*/
> > > > > > > > > > > > +    DeviceState parent_obj;
> > > > > > > > > > > > +
> > > > > > > > > > > > +    /*< public >*/
> > > > > > > > > > > > +    char *slot;
> > > > > > > > > > > > +} CPUCore;
> > > > > > > > > > > > +
> > > > > > > > > > > > +#define CPU_CORE_SLOT_PROP "slot"          
> > > > > > > > > > > as it's generic property I'd rename to 'core' so it would fit all users          
> > > > > > > > > > 
> > > > > > > > > > Ok. Also note that this is a string property which is associated with the
> > > > > > > > > > link name (string) that we created from machine object to this core. I think
> > > > > > > > > > it would be ideal if this becomes an interger  property in which case it
> > > > > > > > > > becomes easier to feed the core location into your CPUSlotProperties.core.        
> > > > > > > > > agreed, it should be core number.        
> > > > > > > > 
> > > > > > > > The slot stuff is continuing to confuse me a bit.  I see that we need
> > > > > > > > some kind of "address" value, but how best to do it is not clear to
> > > > > > > > me.
> > > > > > > > 
> > > > > > > > Changing this to an integer sounds like it's probably a good idea.
> > > > > > > > I'm a bit wary of just calling it "core" though.  Do all platforms
> > > > > > > > even necessarily have a core id?
> > > > > > > > 
> > > > > > > > I'm wondering if the addressing is something that needs to move the
> > > > > > > > the platform specific subtypes, while some other stuff can move to the
> > > > > > > > generic base type.
> > > > > > > >         
> > > > > > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > > > > > all derived cores would inherit it.
> > > > > > > > > > > 
> > > > > > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > > > > at instance_init() time (device_init).
> > > > > > > > > > > 
> > > > > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > > > > >   device_add spapr-core,core=x
> > > > > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > > > > > providing thread property at device_add time:
> > > > > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > > > > 
> > > > > > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > > > > but since realize is allowed to fail it should be fine do so.          
> > > > > > > > > > 
> > > > > > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > > > > > of creating the threads from either of the property setters. I always
> > > > > > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > > > > > is fine, it is good.        
> > > > > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > > > > for failure path.        
> > > > > > > > 
> > > > > > > > Right, moving the "nr_threads" property to the base type seems like a
> > > > > > > > good idea to me.        
> > > > > > > 
> > > > > > > And we will also move the cpu_model property (now being tracked by
> > > > > > > an ObjectClass pointer) to the base type ?      
> > > > > > I'm not sure that moving cpu_model to the base class is the right thing,
> > > > > > I'd keep it local to platform for now.      
> > > > > 
> > > > > I tend to agree, although I'm not sure that I could really explain why
> > > > > :/
> > > > >     
> > > > > > Could you have several spapr core types? One per CPU model?
> > > > > > That way you won't need to track cpu_model when using device_add.      
> > > > > 
> > > > > We could in theory, but it would be pretty inconvenient.  Because this
> > > > > is a paravirt platform, there really can't be any core-level
> > > > > difference between them, and it would mean creating a fair batch of
> > > > > core types for the various minor POWER7 and POWER8 variants - and
> > > > > needing to update this whenever IBM makes a new version.  I suspect it
> > > > > would also introduce more wrinkles in order to have a correct
> > > > > "spapr-core-host" type matching the "HOST" cpu thread type.  Since KVM
> > > > > (HV) only supports the HOST thread type, that's a fairly big issue.    
> > > > Welcome to x86 world, that's roughly what we have there.    
> > > 
> > > I don't really follow you.  x86 doesn't have core devices at all for
> > > the moment.
> > > 
> > > What I'm saying here is that using different core subtypes for every
> > > cpu subtype on power would mean 2 types for each minor variant, rather
> > > than just 1.  
> > I think the same applies to cpu_model and transitioning CPUs to -device.
> > X86 also has a bunch of cpu_model-s which have some minor variations
> > but it still generates a type per cpu_model.
> > If some day we are to implement socket objects for x86 that would also
> > mean to have a socket types per each cpu model.  
> 
> Hmm.  I guess.
> 
> What concerns me is the possible combinatorial explosion of # cpu
> models * # of machine types leading to an enormous number of
> core/socket types.
>
> The other thing is that the platform specific core types belong with
> the machine type code, which means they can't naturally use macro
> magic or whatever to generate them in parallel with the thread device
> types in the core target-ppc code.
on x86 when introducing a new feature or changing behavior 
of existing cpu_model, cpu type stays the same but we amend some of
it's properties using compat machine infrastructure.
Though I'm not sure if it could help in SPAPR case.
 

> > As I understand cpu_model is a legacy option which should translate to
> > a corresponding QOM type (CPU device) which could be used with
> > -device/device_add.  
> 
> Sure, but the "cpu_model" parameter could be "thread type name" just
> as easily.
> 
> > As analogy, QEMU has legacy -net model=foo[1234] option, but when network cards
> > were converted to -device interface that in the end became a set of QOM types
> > like -device foo[1234], it was easier in case of network cards as
> > they where a separate devices models to begin with, the thing to note
> > here is that they weren't converted to a single 'network_card' type with
> > 'model' property.  
>

Igor Mammedov March 10, 2016, 9:39 a.m. UTC | #17

On Thu, 10 Mar 2016 10:42:26 +1100
David Gibson <[e-mail]> wrote:

> On Wed, Mar 09, 2016 at 11:40:53AM +0100, Igor Mammedov wrote:
> > On Tue, 8 Mar 2016 15:26:27 +1100
> > David Gibson <[e-mail]> wrote:  
> > > On Mon, Mar 07, 2016 at 11:29:29AM +0100, Igor Mammedov wrote:  
> > > > On Mon, 7 Mar 2016 14:36:55 +1100
> > > > David Gibson <[e-mail]> wrote:  
> [snip]
> > > > > > > > on top of that I'd add numeric 'threads' property to base class so
> > > > > > > > all derived cores would inherit it.
> > > > > > > > 
> > > > > > > > Then as easy integration with -smp threads=x, a machine could push
> > > > > > > > a global variable 'cpu-core.threads=[smp_threads]' which would
> > > > > > > > make every created cpu-core object to have threads set
> > > > > > > > at instance_init() time (device_init).
> > > > > > > > 
> > > > > > > > That way user won't have to specify 'threads=y' for every
> > > > > > > >   device_add spapr-core,core=x
> > > > > > > > as it will be taken from global property 'cpu-core.threads'
> > > > > > > > but if user wishes he/she still could override global by explicitly
> > > > > > > > providing thread property at device_add time:
> > > > > > > >   device_add spapr-core,core=x,threads=y
> > > > > > > > 
> > > > > > > > wrt this series it would mean, instead of creating threads in property
> > > > > > > > setter, delaying threads creation to core.realize() time,
> > > > > > > > but since realize is allowed to fail it should be fine do so.        
> > > > > > > 
> > > > > > > Ok that would suit us as there are two properties on which thread creation
> > > > > > > is dependent upon: nr_threads and cpu_model. If thread objects can be
> > > > > > > created at core realize time, then we don't have to resort to the ugliness
> > > > > > > of creating the threads from either of the property setters. I always
> > > > > > > assumed that we shouldn't be creating objects from realize, but if that
> > > > > > > is fine, it is good.      
> > > > > > since realize is allowed to fail, it should be safe from hotplug pov
> > > > > > to create internal objects there, as far as proper cleanups are done
> > > > > > for failure path.      
> > > > >     
> > > > [...]    
> > > > > I'm not clear from the above if you're also intending to move at least
> > > > > the adding of the threads as child properties is supposed to go into
> > > > > the base type,    
> > > > I'm not sure that I've got question, could you please rephrase?    
> > > 
> > > So, it seems like we're agreed that moving the nr_threads property to
> > > the base type is a good idea.
> > > 
> > > My question is, do we also move the object_property_add_child() calls
> > > for each thread to the base type (possibly via a helper function or
> > > method the base type provides to derived types)?  
> > I can't think of a reason to do so,
> > why can't subtype-core.realize() do it?  
> 
> It can, but I'm always suspicious of boilerplate stuff that every
> subtype *has* to do in order to work properly.
> 
> > What would one gain creating callbacks and calling them from base class?  
> 
> Enforcing - or at least making as easy as possible - consistency in
> the child object naming.
> 

I guess a patch would help to understand if it's worth of effort of adding
extra callbacks in base class.

Patch

diff --git a/hw/cpu/Makefile.objs b/hw/cpu/Makefile.objs
index 0954a18..942a4bb 100644
--- a/hw/cpu/Makefile.objs
+++ b/hw/cpu/Makefile.objs
@@ -2,4 +2,5 @@  obj-$(CONFIG_ARM11MPCORE) += arm11mpcore.o
 obj-$(CONFIG_REALVIEW) += realview_mpcore.o
 obj-$(CONFIG_A9MPCORE) += a9mpcore.o
 obj-$(CONFIG_A15MPCORE) += a15mpcore.o
+obj-y += core.o
 
diff --git a/hw/cpu/core.c b/hw/cpu/core.c
new file mode 100644
index 0000000..d8caf37
--- /dev/null
+++ b/hw/cpu/core.c
@@ -0,0 +1,44 @@ 
+/*
+ * CPU core abstract device
+ *
+ * Copyright (C) 2016 Bharata B Rao <[e-mail]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "hw/cpu/core.h"
+
+static char *core_prop_get_slot(Object *obj, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+
+    return g_strdup(core->slot);
+}
+
+static void core_prop_set_slot(Object *obj, const char *val, Error **errp)
+{
+    CPUCore *core = CPU_CORE(obj);
+
+    core->slot = g_strdup(val);
+}
+
+static void cpu_core_instance_init(Object *obj)
+{
+    object_property_add_str(obj, "slot", core_prop_get_slot, core_prop_set_slot,
+                            NULL);
+}
+
+static const TypeInfo cpu_core_type_info = {
+    .name = TYPE_CPU_CORE,
+    .parent = TYPE_DEVICE,
+    .abstract = true,
+    .instance_size = sizeof(CPUCore),
+    .instance_init = cpu_core_instance_init,
+};
+
+static void cpu_core_register_types(void)
+{
+    type_register_static(&cpu_core_type_info);
+}
+
+type_init(cpu_core_register_types)
diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
new file mode 100644
index 0000000..2daa724
--- /dev/null
+++ b/include/hw/cpu/core.h
@@ -0,0 +1,30 @@ 
+/*
+ * CPU core abstract device
+ *
+ * Copyright (C) 2016 Bharata B Rao <[e-mail]>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef HW_CPU_CORE_H
+#define HW_CPU_CORE_H
+
+#include "qemu/osdep.h"
+#include "hw/qdev.h"
+
+#define TYPE_CPU_CORE "cpu-core"
+
+#define CPU_CORE(obj) \
+    OBJECT_CHECK(CPUCore, (obj), TYPE_CPU_CORE)
+
+typedef struct CPUCore {
+    /*< private >*/
+    DeviceState parent_obj;
+
+    /*< public >*/
+    char *slot;
+} CPUCore;
+
+#define CPU_CORE_SLOT_PROP "slot"
+
+#endif