| Message ID | 1463024905-28401-7-git-send-email-bharata@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 12, 2016 at 09:18:16AM +0530, Bharata B Rao 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 <bharata@linux.vnet.ibm.com> > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > [Integer core property] > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> Igor, Eduardo, Do you think we're comfortable enough with this abstract core concept to merge it now? If so which tree should it go through? > --- > hw/cpu/Makefile.objs | 1 + > hw/cpu/core.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/cpu/core.h | 31 ++++++++++++++++++ > 3 files changed, 120 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..fa5bc82 > --- /dev/null > +++ b/hw/cpu/core.c > @@ -0,0 +1,88 @@ > +/* > + * CPU core abstract device > + * > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > + * > + * 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" > +#include "qapi/visitor.h" > +#include "qapi/error.h" > +#include "sysemu/cpus.h" > + > +static void core_prop_get_core(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + CPUCore *core = CPU_CORE(obj); > + int64_t value = core->core; > + > + visit_type_int(v, name, &value, errp); > +} > + > +static void core_prop_set_core(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + CPUCore *core = CPU_CORE(obj); > + Error *local_err = NULL; > + int64_t value; > + > + visit_type_int(v, name, &value, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + core->core = value; > +} > + > +static void core_prop_get_threads(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + CPUCore *core = CPU_CORE(obj); > + int64_t value = core->threads; > + > + visit_type_int(v, name, &value, errp); > +} > + > +static void core_prop_set_threads(Object *obj, Visitor *v, const char *name, > + void *opaque, Error **errp) > +{ > + CPUCore *core = CPU_CORE(obj); > + Error *local_err = NULL; > + int64_t value; > + > + visit_type_int(v, name, &value, &local_err); > + if (local_err) { > + error_propagate(errp, local_err); > + return; > + } > + > + core->threads = value; > +} > + > +static void cpu_core_instance_init(Object *obj) > +{ > + CPUCore *core = CPU_CORE(obj); > + > + object_property_add(obj, "core", "int", core_prop_get_core, > + core_prop_set_core, NULL, NULL, NULL); > + object_property_add(obj, "threads", "int", core_prop_get_threads, > + core_prop_set_threads, NULL, NULL, NULL); > + core->threads = smp_threads; > +} > + > +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..a2a5a04 > --- /dev/null > +++ b/include/hw/cpu/core.h > @@ -0,0 +1,31 @@ > +/* > + * CPU core abstract device > + * > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > + * > + * 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 >*/ > + int core; > + int threads; > +} CPUCore; > + > +#define CPU_CORE_PROP_CORE "core" > + > +#endif
On Thu, 2 Jun 2016 13:38:58 +1000 David Gibson <david@gibson.dropbear.id.au> wrote: > On Thu, May 12, 2016 at 09:18:16AM +0530, Bharata B Rao 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 <bharata@linux.vnet.ibm.com> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > [Integer core property] > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Igor, Eduardo, > > Do you think we're comfortable enough with this abstract core concept > to merge it now? If so which tree should it go through? spapr is the only user of it, and it makes sense for patch to be merged as part of series that actually uses it. Reviewed-by: Igor Mammedov <imammedo@redhat.com> > > > --- > > hw/cpu/Makefile.objs | 1 + > > hw/cpu/core.c | 88 > > +++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/hw/cpu/core.h | 31 ++++++++++++++++++ 3 files changed, 120 > > 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..fa5bc82 > > --- /dev/null > > +++ b/hw/cpu/core.c > > @@ -0,0 +1,88 @@ > > +/* > > + * CPU core abstract device > > + * > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > > + * > > + * 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" > > +#include "qapi/visitor.h" > > +#include "qapi/error.h" > > +#include "sysemu/cpus.h" > > + > > +static void core_prop_get_core(Object *obj, Visitor *v, const char > > *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + int64_t value = core->core; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void core_prop_set_core(Object *obj, Visitor *v, const char > > *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + core->core = value; > > +} > > + > > +static void core_prop_get_threads(Object *obj, Visitor *v, const > > char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + int64_t value = core->threads; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void core_prop_set_threads(Object *obj, Visitor *v, const > > char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + core->threads = value; > > +} > > + > > +static void cpu_core_instance_init(Object *obj) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + > > + object_property_add(obj, "core", "int", core_prop_get_core, > > + core_prop_set_core, NULL, NULL, NULL); > > + object_property_add(obj, "threads", "int", > > core_prop_get_threads, > > + core_prop_set_threads, NULL, NULL, NULL); > > + core->threads = smp_threads; > > +} > > + > > +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..a2a5a04 > > --- /dev/null > > +++ b/include/hw/cpu/core.h > > @@ -0,0 +1,31 @@ > > +/* > > + * CPU core abstract device > > + * > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > > + * > > + * 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 >*/ > > + int core; > > + int threads; > > +} CPUCore; > > + > > +#define CPU_CORE_PROP_CORE "core" > > + > > +#endif >
On Thu, Jun 02, 2016 at 01:38:58PM +1000, David Gibson wrote: > On Thu, May 12, 2016 at 09:18:16AM +0530, Bharata B Rao 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 <bharata@linux.vnet.ibm.com> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > [Integer core property] > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > Igor, Eduardo, > > Do you think we're comfortable enough with this abstract core concept > to merge it now? If so which tree should it go through? TBH, I haven't reviewed the concept carefully. I hoped that people that spent more time thinking about long-term plans (especially Andreas) would help move the discussion forward, but Andreas is moving away from QOM CPU. I need to review the previous discussions more carefully, but the concept looks simple enough to me, and I don't think we should hold spapr work because we want a Grand Plan for generic CPU abstractions to be finished first. If David, Igor, and the people working on spapr are happy with it, I trust their judgement. I just wish the interface was better documented, especially the meaning of the "core" and "threads" properties. I would prefer to have "core-id" as the property name instead of "core" (most of the error messages related to it (in patch 08/15) say "core id"). > > > --- > > hw/cpu/Makefile.objs | 1 + > > hw/cpu/core.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++ > > include/hw/cpu/core.h | 31 ++++++++++++++++++ > > 3 files changed, 120 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..fa5bc82 > > --- /dev/null > > +++ b/hw/cpu/core.c > > @@ -0,0 +1,88 @@ > > +/* > > + * CPU core abstract device > > + * > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > > + * > > + * 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" > > +#include "qapi/visitor.h" > > +#include "qapi/error.h" > > +#include "sysemu/cpus.h" > > + > > +static void core_prop_get_core(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + int64_t value = core->core; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void core_prop_set_core(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + core->core = value; > > +} > > + > > +static void core_prop_get_threads(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + int64_t value = core->threads; > > + > > + visit_type_int(v, name, &value, errp); > > +} > > + > > +static void core_prop_set_threads(Object *obj, Visitor *v, const char *name, > > + void *opaque, Error **errp) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + Error *local_err = NULL; > > + int64_t value; > > + > > + visit_type_int(v, name, &value, &local_err); > > + if (local_err) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + core->threads = value; > > +} > > + > > +static void cpu_core_instance_init(Object *obj) > > +{ > > + CPUCore *core = CPU_CORE(obj); > > + > > + object_property_add(obj, "core", "int", core_prop_get_core, > > + core_prop_set_core, NULL, NULL, NULL); > > + object_property_add(obj, "threads", "int", core_prop_get_threads, > > + core_prop_set_threads, NULL, NULL, NULL); > > + core->threads = smp_threads; > > +} > > + > > +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..a2a5a04 > > --- /dev/null > > +++ b/include/hw/cpu/core.h > > @@ -0,0 +1,31 @@ > > +/* > > + * CPU core abstract device > > + * > > + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> > > + * > > + * 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 >*/ > > + int core; > > + int threads; > > +} CPUCore; > > + > > +#define CPU_CORE_PROP_CORE "core" > > + > > +#endif > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Thu, Jun 02, 2016 at 03:12:09PM -0300, Eduardo Habkost wrote: > On Thu, Jun 02, 2016 at 01:38:58PM +1000, David Gibson wrote: > > On Thu, May 12, 2016 at 09:18:16AM +0530, Bharata B Rao 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 <bharata@linux.vnet.ibm.com> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > [Integer core property] > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > > Igor, Eduardo, > > > > Do you think we're comfortable enough with this abstract core concept > > to merge it now? If so which tree should it go through? > > TBH, I haven't reviewed the concept carefully. I hoped that > people that spent more time thinking about long-term plans > (especially Andreas) would help move the discussion forward, but > Andreas is moving away from QOM CPU. > > I need to review the previous discussions more carefully, but the > concept looks simple enough to me, and I don't think we should > hold spapr work because we want a Grand Plan for generic CPU > abstractions to be finished first. If David, Igor, and the people > working on spapr are happy with it, I trust their judgement. > > I just wish the interface was better documented, especially the > meaning of the "core" and "threads" properties. I would prefer to > have "core-id" as the property name instead of "core" (most of > the error messages related to it (in patch 08/15) say "core id"). Ok, I've renamed the properties to 'core-id' and 'nr-threads' as I merged, which I think is clearer.
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..fa5bc82 --- /dev/null +++ b/hw/cpu/core.c @@ -0,0 +1,88 @@ +/* + * CPU core abstract device + * + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> + * + * 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" +#include "qapi/visitor.h" +#include "qapi/error.h" +#include "sysemu/cpus.h" + +static void core_prop_get_core(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + CPUCore *core = CPU_CORE(obj); + int64_t value = core->core; + + visit_type_int(v, name, &value, errp); +} + +static void core_prop_set_core(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + CPUCore *core = CPU_CORE(obj); + Error *local_err = NULL; + int64_t value; + + visit_type_int(v, name, &value, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + core->core = value; +} + +static void core_prop_get_threads(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + CPUCore *core = CPU_CORE(obj); + int64_t value = core->threads; + + visit_type_int(v, name, &value, errp); +} + +static void core_prop_set_threads(Object *obj, Visitor *v, const char *name, + void *opaque, Error **errp) +{ + CPUCore *core = CPU_CORE(obj); + Error *local_err = NULL; + int64_t value; + + visit_type_int(v, name, &value, &local_err); + if (local_err) { + error_propagate(errp, local_err); + return; + } + + core->threads = value; +} + +static void cpu_core_instance_init(Object *obj) +{ + CPUCore *core = CPU_CORE(obj); + + object_property_add(obj, "core", "int", core_prop_get_core, + core_prop_set_core, NULL, NULL, NULL); + object_property_add(obj, "threads", "int", core_prop_get_threads, + core_prop_set_threads, NULL, NULL, NULL); + core->threads = smp_threads; +} + +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..a2a5a04 --- /dev/null +++ b/include/hw/cpu/core.h @@ -0,0 +1,31 @@ +/* + * CPU core abstract device + * + * Copyright (C) 2016 Bharata B Rao <bharata@linux.vnet.ibm.com> + * + * 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 >*/ + int core; + int threads; +} CPUCore; + +#define CPU_CORE_PROP_CORE "core" + +#endif