Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

2016-06-08 Thread David Gibson 
On Wed, Jun 08, 2016 at 03:12:42PM +0530, Bharata B Rao wrote:
> On Fri, Jun 03, 2016 at 03:25:08PM +1000, David Gibson wrote:
> > On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> > > Add sPAPR specific abastract CPU core device that is based on generic
> > > CPU core device. Use this as base type to create sPAPR CPU specific core
> > > devices.
> > > 
> > > TODO:
> > > - Add core types for other remaining CPU types
> > > - Handle CPU model alias correctly
> > > 
> > > Signed-off-by: Bharata B Rao 
> > 
> > This is looking pretty cood, but there's some minor changes I'd like
> > to see.
> > 
> > > ---
> > >  hw/ppc/Makefile.objs|   1 +
> > >  hw/ppc/spapr.c  |   3 +-
> > >  hw/ppc/spapr_cpu_core.c | 168 
> > > 
> > >  include/hw/ppc/spapr.h  |   1 +
> > >  include/hw/ppc/spapr_cpu_core.h |  28 +++
> > >  5 files changed, 199 insertions(+), 2 deletions(-)
> > >  create mode 100644 hw/ppc/spapr_cpu_core.c
> > >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > > 
> > > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > > index c1ffc77..5cc6608 100644
> > > --- a/hw/ppc/Makefile.objs
> > > +++ b/hw/ppc/Makefile.objs
> > > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> > >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> > >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> > >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> > >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> > >  obj-y += spapr_pci_vfio.o
> > >  endif
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index b69995e..95db047 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1605,8 +1605,7 @@ static void spapr_boot_set(void *opaque, const char 
> > > *boot_device,
> > >  machine->boot_order = g_strdup(boot_device);
> > >  }
> > >  
> > > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > > -   Error **errp)
> > > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error 
> > > **errp)
> > 
> > I think this function should actually move into spapr_cpu_core.c
> 
> Actually, this is a CPU thread specific routine and will be called from
> spapr.c too. But I moved this to spapr_cpu_core.c as you suggest.

Yes, I realize it will be called from spapr.c as well.  The idea is to
make the core based initialization the clean path, even if the
thread-based initialization needed for backwards compat needs some
awkward cross-module calls and exports.

> > >  {
> > >  CPUPPCState *env = >env;
> > >  
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > new file mode 100644
> > > index 000..af63ed9
> > > --- /dev/null
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -0,0 +1,168 @@
> > > +/*
> > > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > > + *
> > > + * Copyright (C) 2016 Bharata B Rao 
> > > + *
> > > + * 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 "hw/ppc/spapr_cpu_core.h"
> > > +#include "hw/ppc/spapr.h"
> > > +#include "hw/boards.h"
> > > +#include "qapi/error.h"
> > > +#include 
> > > +#include "target-ppc/kvm_ppc.h"
> > > +
> > > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> > > +  Error **errp)
> > 
> > This function could be folded into spapr_cpu_core_realize(), that's
> > the only caller and they're both fairly short.
> 
> Ok done.
> 
> > 
> > > +{
> > > +int i;
> > > +Error *local_err = NULL;
> > > +sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > > +
> > > +for (i = 0; i < threads; i++) {
> > > +char id[32];
> > > +
> > > +object_initialize(>threads[i], sizeof(core->threads[i]),
> > > +  object_class_get_name(core->cpu));
> > 
> > Given we have to go from the class pointer to the class name to
> > actually use it here, maybe we would be better off storing the name
> > rather than a class pointer.  Up to you, I'm happy either way.
> 
> I was doing typename initially and there were suggestions to move to
> ObjectClass. May be I will leave it like this while I fix all the other
> things and finally revisit this ?

Yes, that makes sense.  As I say it's not really a big deal either
way.

Could you use object_initialize_with_type() to avoid going via the
type name?

> > > +snprintf(id, sizeof(id), "thread[%d]", i);
> > > +object_property_add_child(OBJECT(core), id, 
> > > OBJECT(>threads[i]),
> > > +  _err);
> > > +if (local_err) {
> > > +goto err;
> > > +}
> > >

Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

2016-06-08 Thread Bharata B Rao 
On Fri, Jun 03, 2016 at 03:25:08PM +1000, David Gibson wrote:
> On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> > Add sPAPR specific abastract CPU core device that is based on generic
> > CPU core device. Use this as base type to create sPAPR CPU specific core
> > devices.
> > 
> > TODO:
> > - Add core types for other remaining CPU types
> > - Handle CPU model alias correctly
> > 
> > Signed-off-by: Bharata B Rao 
> 
> This is looking pretty cood, but there's some minor changes I'd like
> to see.
> 
> > ---
> >  hw/ppc/Makefile.objs|   1 +
> >  hw/ppc/spapr.c  |   3 +-
> >  hw/ppc/spapr_cpu_core.c | 168 
> > 
> >  include/hw/ppc/spapr.h  |   1 +
> >  include/hw/ppc/spapr_cpu_core.h |  28 +++
> >  5 files changed, 199 insertions(+), 2 deletions(-)
> >  create mode 100644 hw/ppc/spapr_cpu_core.c
> >  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> > 
> > diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> > index c1ffc77..5cc6608 100644
> > --- a/hw/ppc/Makefile.objs
> > +++ b/hw/ppc/Makefile.objs
> > @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
> >  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
> >  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
> >  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> > +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
> >  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
> >  obj-y += spapr_pci_vfio.o
> >  endif
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index b69995e..95db047 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1605,8 +1605,7 @@ static void spapr_boot_set(void *opaque, const char 
> > *boot_device,
> >  machine->boot_order = g_strdup(boot_device);
> >  }
> >  
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > -   Error **errp)
> > +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error 
> > **errp)
> 
> I think this function should actually move into spapr_cpu_core.c

Actually, this is a CPU thread specific routine and will be called from
spapr.c too. But I moved this to spapr_cpu_core.c as you suggest.

> 
> >  {
> >  CPUPPCState *env = >env;
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > new file mode 100644
> > index 000..af63ed9
> > --- /dev/null
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -0,0 +1,168 @@
> > +/*
> > + * sPAPR CPU core device, acts as container of CPU thread devices.
> > + *
> > + * Copyright (C) 2016 Bharata B Rao 
> > + *
> > + * 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 "hw/ppc/spapr_cpu_core.h"
> > +#include "hw/ppc/spapr.h"
> > +#include "hw/boards.h"
> > +#include "qapi/error.h"
> > +#include 
> > +#include "target-ppc/kvm_ppc.h"
> > +
> > +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> > +  Error **errp)
> 
> This function could be folded into spapr_cpu_core_realize(), that's
> the only caller and they're both fairly short.

Ok done.

> 
> > +{
> > +int i;
> > +Error *local_err = NULL;
> > +sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +
> > +for (i = 0; i < threads; i++) {
> > +char id[32];
> > +
> > +object_initialize(>threads[i], sizeof(core->threads[i]),
> > +  object_class_get_name(core->cpu));
> 
> Given we have to go from the class pointer to the class name to
> actually use it here, maybe we would be better off storing the name
> rather than a class pointer.  Up to you, I'm happy either way.

I was doing typename initially and there were suggestions to move to
ObjectClass. May be I will leave it like this while I fix all the other
things and finally revisit this ?

> 
> > +snprintf(id, sizeof(id), "thread[%d]", i);
> > +object_property_add_child(OBJECT(core), id, 
> > OBJECT(>threads[i]),
> > +  _err);
> > +if (local_err) {
> > +goto err;
> > +}
> > +}
> > +return;
> > +
> > +err:
> > +while (--i) {
> > +object_unparent(OBJECT(>threads[i]));
> 
> Is this safe if some of the threads haven't been initialized?

This is in the error path and only those threads which have been
initialized will be unparented.

> 
> > +}
> > +error_propagate(errp, local_err);
> > +}
> > +
> > +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> > +{
> > +Error **errp = opaque;
> > +sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +CPUState *cs = CPU(child);
> > +PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +object_property_set_bool(child, true, "realized", errp);

Re: [Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

2016-06-02 Thread David Gibson 
On Thu, May 12, 2016 at 09:18:17AM +0530, Bharata B Rao wrote:
> Add sPAPR specific abastract CPU core device that is based on generic
> CPU core device. Use this as base type to create sPAPR CPU specific core
> devices.
> 
> TODO:
> - Add core types for other remaining CPU types
> - Handle CPU model alias correctly
> 
> Signed-off-by: Bharata B Rao 

This is looking pretty cood, but there's some minor changes I'd like
to see.

> ---
>  hw/ppc/Makefile.objs|   1 +
>  hw/ppc/spapr.c  |   3 +-
>  hw/ppc/spapr_cpu_core.c | 168 
> 
>  include/hw/ppc/spapr.h  |   1 +
>  include/hw/ppc/spapr_cpu_core.h |  28 +++
>  5 files changed, 199 insertions(+), 2 deletions(-)
>  create mode 100644 hw/ppc/spapr_cpu_core.c
>  create mode 100644 include/hw/ppc/spapr_cpu_core.h
> 
> diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
> index c1ffc77..5cc6608 100644
> --- a/hw/ppc/Makefile.objs
> +++ b/hw/ppc/Makefile.objs
> @@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
>  obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
>  obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
>  obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
> +obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
>  ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
>  obj-y += spapr_pci_vfio.o
>  endif
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b69995e..95db047 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1605,8 +1605,7 @@ static void spapr_boot_set(void *opaque, const char 
> *boot_device,
>  machine->boot_order = g_strdup(boot_device);
>  }
>  
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -   Error **errp)
> +void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)

I think this function should actually move into spapr_cpu_core.c

>  {
>  CPUPPCState *env = >env;
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> new file mode 100644
> index 000..af63ed9
> --- /dev/null
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -0,0 +1,168 @@
> +/*
> + * sPAPR CPU core device, acts as container of CPU thread devices.
> + *
> + * Copyright (C) 2016 Bharata B Rao 
> + *
> + * 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 "hw/ppc/spapr_cpu_core.h"
> +#include "hw/ppc/spapr.h"
> +#include "hw/boards.h"
> +#include "qapi/error.h"
> +#include 
> +#include "target-ppc/kvm_ppc.h"
> +
> +static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
> +  Error **errp)

This function could be folded into spapr_cpu_core_realize(), that's
the only caller and they're both fairly short.

> +{
> +int i;
> +Error *local_err = NULL;
> +sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> +
> +for (i = 0; i < threads; i++) {
> +char id[32];
> +
> +object_initialize(>threads[i], sizeof(core->threads[i]),
> +  object_class_get_name(core->cpu));

Given we have to go from the class pointer to the class name to
actually use it here, maybe we would be better off storing the name
rather than a class pointer.  Up to you, I'm happy either way.

> +snprintf(id, sizeof(id), "thread[%d]", i);
> +object_property_add_child(OBJECT(core), id, 
> OBJECT(>threads[i]),
> +  _err);
> +if (local_err) {
> +goto err;
> +}
> +}
> +return;
> +
> +err:
> +while (--i) {
> +object_unparent(OBJECT(>threads[i]));

Is this safe if some of the threads haven't been initialized?

> +}
> +error_propagate(errp, local_err);
> +}
> +
> +static int spapr_cpu_core_realize_child(Object *child, void *opaque)
> +{
> +Error **errp = opaque;
> +sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> +CPUState *cs = CPU(child);
> +PowerPCCPU *cpu = POWERPC_CPU(cs);
> +
> +object_property_set_bool(child, true, "realized", errp);
> +if (*errp) {
> +return 1;
> +}
> +
> +spapr_cpu_init(spapr, cpu, errp);
> +if (*errp) {
> +return 1;
> +}
> +return 0;
> +}
> +
> +static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> +{
> +sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> +CPUCore *cc = CPU_CORE(OBJECT(dev));
> +Error *local_err = NULL;
> +
> +sc->threads = g_new0(PowerPCCPU, cc->threads);

This isn't quite safe, because it assume the structure size for all
the threads is that of PowerPCCPU.  That's true now, but cpu thread
subtypes could in theory extend that structure with subtype specific
fields.  I think we need to actually look up the class instance size
here.

> +

[Qemu-devel] [for-2.7 PATCH v3 07/15] spapr: Abstract CPU core device and type specific core devices

2016-05-11 Thread Bharata B Rao 
Add sPAPR specific abastract CPU core device that is based on generic
CPU core device. Use this as base type to create sPAPR CPU specific core
devices.

TODO:
- Add core types for other remaining CPU types
- Handle CPU model alias correctly

Signed-off-by: Bharata B Rao 
---
 hw/ppc/Makefile.objs|   1 +
 hw/ppc/spapr.c  |   3 +-
 hw/ppc/spapr_cpu_core.c | 168 
 include/hw/ppc/spapr.h  |   1 +
 include/hw/ppc/spapr_cpu_core.h |  28 +++
 5 files changed, 199 insertions(+), 2 deletions(-)
 create mode 100644 hw/ppc/spapr_cpu_core.c
 create mode 100644 include/hw/ppc/spapr_cpu_core.h

diff --git a/hw/ppc/Makefile.objs b/hw/ppc/Makefile.objs
index c1ffc77..5cc6608 100644
--- a/hw/ppc/Makefile.objs
+++ b/hw/ppc/Makefile.objs
@@ -4,6 +4,7 @@ obj-y += ppc.o ppc_booke.o
 obj-$(CONFIG_PSERIES) += spapr.o spapr_vio.o spapr_events.o
 obj-$(CONFIG_PSERIES) += spapr_hcall.o spapr_iommu.o spapr_rtas.o
 obj-$(CONFIG_PSERIES) += spapr_pci.o spapr_rtc.o spapr_drc.o spapr_rng.o
+obj-$(CONFIG_PSERIES) += spapr_cpu_core.o
 ifeq ($(CONFIG_PCI)$(CONFIG_PSERIES)$(CONFIG_LINUX), yyy)
 obj-y += spapr_pci_vfio.o
 endif
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index b69995e..95db047 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1605,8 +1605,7 @@ static void spapr_boot_set(void *opaque, const char 
*boot_device,
 machine->boot_order = g_strdup(boot_device);
 }
 
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
-   Error **errp)
+void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu, Error **errp)
 {
 CPUPPCState *env = >env;
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
new file mode 100644
index 000..af63ed9
--- /dev/null
+++ b/hw/ppc/spapr_cpu_core.c
@@ -0,0 +1,168 @@
+/*
+ * sPAPR CPU core device, acts as container of CPU thread devices.
+ *
+ * Copyright (C) 2016 Bharata B Rao 
+ *
+ * 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 "hw/ppc/spapr_cpu_core.h"
+#include "hw/ppc/spapr.h"
+#include "hw/boards.h"
+#include "qapi/error.h"
+#include 
+#include "target-ppc/kvm_ppc.h"
+
+static void spapr_cpu_core_create_threads(DeviceState *dev, int threads,
+  Error **errp)
+{
+int i;
+Error *local_err = NULL;
+sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
+
+for (i = 0; i < threads; i++) {
+char id[32];
+
+object_initialize(>threads[i], sizeof(core->threads[i]),
+  object_class_get_name(core->cpu));
+snprintf(id, sizeof(id), "thread[%d]", i);
+object_property_add_child(OBJECT(core), id, OBJECT(>threads[i]),
+  _err);
+if (local_err) {
+goto err;
+}
+}
+return;
+
+err:
+while (--i) {
+object_unparent(OBJECT(>threads[i]));
+}
+error_propagate(errp, local_err);
+}
+
+static int spapr_cpu_core_realize_child(Object *child, void *opaque)
+{
+Error **errp = opaque;
+sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+CPUState *cs = CPU(child);
+PowerPCCPU *cpu = POWERPC_CPU(cs);
+
+object_property_set_bool(child, true, "realized", errp);
+if (*errp) {
+return 1;
+}
+
+spapr_cpu_init(spapr, cpu, errp);
+if (*errp) {
+return 1;
+}
+return 0;
+}
+
+static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
+{
+sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
+CPUCore *cc = CPU_CORE(OBJECT(dev));
+Error *local_err = NULL;
+
+sc->threads = g_new0(PowerPCCPU, cc->threads);
+spapr_cpu_core_create_threads(dev, cc->threads, _err);
+if (local_err) {
+goto out;
+}
+object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, 
_err);
+
+out:
+if (local_err) {
+g_free(sc->threads);
+error_propagate(errp, local_err);
+}
+}
+
+static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(oc);
+dc->realize = spapr_cpu_core_realize;
+}
+
+/*
+ * instance_init routines from different flavours of sPAPR CPU cores.
+ * TODO: Add support for 'host' core type.
+ */
+#define SPAPR_CPU_CORE_INITFN(_type, _fname) \
+static void glue(glue(spapr_cpu_core_, _fname), _initfn(Object *obj)) \
+{ \
+sPAPRCPUCore *core = SPAPR_CPU_CORE(obj); \
+char *name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, stringify(_type)); \
+ObjectClass *oc = object_class_by_name(name); \
+g_assert(oc); \
+g_free((void *)name); \
+core->cpu = oc; \
+}
+
+SPAPR_CPU_CORE_INITFN(POWER7_v2.3, POWER7);
+SPAPR_CPU_CORE_INITFN(POWER7+_v2.1, POWER7plus);
+SPAPR_CPU_CORE_INITFN(POWER8_v2.0, POWER8);
+SPAPR_CPU_CORE_INITFN(POWER8E_v2.1,