[Qemu-devel] [PATCH v2 0/7] ppc: compute cpu_dt_id in the machine code

[Qemu-devel] [PATCH v2 0/7] ppc: compute cpu_dt_id in the machine code

[Greg Kurz]

This series is a sequel to the discussion on a patch from Ben's powernv
patchset:

        http://patchwork.ozlabs.org/patch/597153/

Indeed, since the DT is a machine abstraction, it should definitely sit
under hw/ppc and not in the target code:
- all machine types are forced to share the same numbering logic
- user mode does not need that => there are #ifdef everywhere

So this series moves all the current numbering logic to the machine
code.

The patchset was completely re-written according to Igor's valuable
suggestions. The main change is that cpu_dt_id is now computed out
of a cpu_index provided by the machine, instead of bending the code
to use cs->cpu_index.

I resend Bharata's patch without any modification because patch 6 is
based on it (pseries-2.7 support).

I did not see any regression on pseries-2.6, pseries-2.7 and CPU hotplug.

Migration is not impacted because the new cpu_index has the same value
as cs->cpu_index, and thus gives the same guest visible cpu_dt_id.

TODO: find a way for machines to provide their own cpu_dt_id logic.

---

Bharata B Rao (1):
      spapr: Ensure thread0 of CPU core is always realized first

Greg Kurz (6):
      ppc: simplify max_smt initialization in ppc_cpu_realizefn()
      ppc: different creation paths for cpus in system and user mode
      ppc: open code cpu creation for machine types
      ppc: introduce ppc_set_vcpu_dt_id()
      spapr: use ppc_set_vcpu_dt_id() in CPU hotplug code
      ppc: move the cpu_dt_id logic to machine code


 hw/ppc/e500.c               |    2 +
 hw/ppc/mac_newworld.c       |    2 +
 hw/ppc/mac_oldworld.c       |    2 +
 hw/ppc/ppc.c                |   78 +++++++++++++++++++++++++++++++++++++++++++
 hw/ppc/ppc440_bamboo.c      |    2 +
 hw/ppc/ppc4xx_devs.c        |    2 +
 hw/ppc/prep.c               |    2 +
 hw/ppc/spapr.c              |    2 +
 hw/ppc/spapr_cpu_core.c     |   36 +++++++++++++-------
 hw/ppc/virtex_ml507.c       |    2 +
 include/hw/ppc/ppc.h        |    2 +
 target-ppc/cpu.h            |    5 ++-
 target-ppc/translate_init.c |   35 -------------------
 13 files changed, 114 insertions(+), 58 deletions(-)

--
Greg

[Qemu-devel] [PATCH v2 1/7] spapr: Ensure thread0 of CPU core is always realized first

[Greg Kurz]

From: Bharata B Rao <bharata@linux.vnet.ibm.com>

During CPU core realization, we create all the thread objects and parent
them to the core object in a loop. However, the realization of thread
objects is done separately by walking the threads of a core using
object_child_foreach(). With this, there is no guarantee on the order
in which the child thread objects get realized. Since CPU device tree
properties are currently derived from the CPU thread object, we assume
thread0 of the core to be the representative thread of the core when
creating device tree properties for the core. If thread0 is not the
first thread that gets realized, then we would end up having an
incorrect dt_id for the core and this causes hotplug failures from
the guest.

Fix this by realizing each thread object by walking the core's thread
object list thereby ensuring that thread0 and other threads are always
realized in the correct order.

Future TODO: CPU DT nodes are per-core properties and we should
ideally base the creation of CPU DT nodes on core objects rather than
the thread objects.

Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr_cpu_core.c |   29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index a384db5204ac..70b6b0b5ee17 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -259,9 +259,9 @@ out:
     error_propagate(errp, local_err);
 }
 
-static int spapr_cpu_core_realize_child(Object *child, void *opaque)
+static void spapr_cpu_core_realize_child(Object *child, Error **errp)
 {
-    Error **errp = opaque, *local_err = NULL;
+    Error *local_err = NULL;
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -269,15 +269,14 @@ static int spapr_cpu_core_realize_child(Object *child, void *opaque)
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return 1;
+        return;
     }
 
     spapr_cpu_init(spapr, cpu, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        return 1;
+        return;
     }
-    return 0;
 }
 
 static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
@@ -287,13 +286,13 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     const char *typename = object_class_get_name(sc->cpu_class);
     size_t size = object_type_get_instance_size(typename);
     Error *local_err = NULL;
-    Object *obj;
-    int i;
+    void *obj;
+    int i, j;
 
     sc->threads = g_malloc0(size * cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
         char id[32];
-        void *obj = sc->threads + i * size;
+        obj = sc->threads + i * size;
 
         object_initialize(obj, size, typename);
         snprintf(id, sizeof(id), "thread[%d]", i);
@@ -303,12 +302,16 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
         }
         object_unref(obj);
     }
-    object_child_foreach(OBJECT(dev), spapr_cpu_core_realize_child, &local_err);
-    if (local_err) {
-        goto err;
-    } else {
-        return;
+
+    for (j = 0; j < cc->nr_threads; j++) {
+        obj = sc->threads + j * size;
+
+        spapr_cpu_core_realize_child(obj, &local_err);
+        if (local_err) {
+            goto err;
+        }
     }
+    return;
 
 err:
     while (--i >= 0) {

[Qemu-devel] [PATCH v2 2/7] ppc: simplify max_smt initialization in ppc_cpu_realizefn()

[Greg Kurz]

kvmppc_smt_threads() returns 1 if KVM is not enabled.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target-ppc/translate_init.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 843f19b748fb..a06bf50b65d4 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9516,7 +9516,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
 #if !defined(CONFIG_USER_ONLY)
-    int max_smt = kvm_enabled() ? kvmppc_smt_threads() : 1;
+    int max_smt = kvmppc_smt_threads();
 #endif
 
 #if !defined(CONFIG_USER_ONLY)

[Qemu-devel] [PATCH v2 3/7] ppc: different creation paths for cpus in system and user mode

[Greg Kurz]

The machine code currently uses the same cpu_ppc_init() function to
create cpus as the user mode. This function also triggers the cpu
realization.

It is okay for user mode but with system mode we may want to do other
things between initialization and realization, like generating cpu
ids for the DT for example.

With this patch, each mode has its own creation helper:
- ppc_cpu_init() is for system mode only
- cpu_init() is for user mode only

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/e500.c               |    2 +-
 hw/ppc/mac_newworld.c       |    2 +-
 hw/ppc/mac_oldworld.c       |    2 +-
 hw/ppc/ppc.c                |    5 +++++
 hw/ppc/ppc440_bamboo.c      |    2 +-
 hw/ppc/ppc4xx_devs.c        |    2 +-
 hw/ppc/prep.c               |    2 +-
 hw/ppc/spapr.c              |    2 +-
 hw/ppc/virtex_ml507.c       |    2 +-
 include/hw/ppc/ppc.h        |    1 +
 target-ppc/cpu.h            |    5 +++--
 target-ppc/translate_init.c |    5 -----
 12 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 0cd534df55f8..ff5d92e48dd9 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         CPUState *cs;
         qemu_irq *input;
 
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to initialize CPU!\n");
             exit(1);
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 32e88b378687..6ab675c498d0 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
 #endif
     }
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 447948746b1a..77fbdfffd4e2 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "G3";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index e4252528a69d..dc3d214009c5 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1350,3 +1350,8 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
     return NULL;
 }
+
+PowerPCCPU *ppc_cpu_init(const char *cpu_model)
+{
+    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+}
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 5c535b18a20d..7f22433c8e91 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "440EP";
     }
-    cpu = cpu_ppc_init(machine->cpu_model);
+    cpu = ppc_cpu_init(machine->cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index e7f413e49d08..94a24243af70 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -56,7 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
     CPUPPCState *env;
 
     /* init CPUs */
-    cpu = cpu_ppc_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
                 cpu_model);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 054af1e8b481..e62fe643f492 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "602";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 78ebd9ee38ce..690ee486aa07 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1828,7 +1828,7 @@ static void ppc_spapr_init(MachineState *machine)
         g_free(type);
     } else {
         for (i = 0; i < smp_cpus; i++) {
-            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
+            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
             if (cpu == NULL) {
                 error_report("Unable to find PowerPC CPU definition");
                 exit(1);
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index b97d96685cf1..8d350fb98b2c 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -96,7 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
     CPUPPCState *env;
     qemu_irq *irqs;
 
-    cpu = cpu_ppc_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 5617dc4a2c04..a4db1db82e1b 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -106,4 +106,5 @@ enum {
 /* ppc_booke.c */
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
+PowerPCCPU *ppc_cpu_init(const char *cpu_model);
 #endif
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index af73bced9f70..440309a68006 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1196,7 +1196,6 @@ extern const struct VMStateDescription vmstate_ppc_cpu;
 #endif
 
 /*****************************************************************************/
-PowerPCCPU *cpu_ppc_init(const char *cpu_model);
 void ppc_translate_init(void);
 void gen_update_current_nip(void *opaque);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
@@ -1275,7 +1274,9 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
 int ppc_dcr_read (ppc_dcr_t *dcr_env, int dcrn, uint32_t *valp);
 int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
 
-#define cpu_init(cpu_model) CPU(cpu_ppc_init(cpu_model))
+#if defined(CONFIG_USER_ONLY)
+#define cpu_init(cpu_model) cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)
+#endif
 
 #define cpu_signal_handler cpu_ppc_signal_handler
 #define cpu_list ppc_cpu_list
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index a06bf50b65d4..6706787b41a1 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10000,11 +10000,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
     return NULL;
 }
 
-PowerPCCPU *cpu_ppc_init(const char *cpu_model)
-{
-    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
-}
-
 /* Sort by PVR, ordering special case "host" last. */
 static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
 {

[Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

If we want to generate cpu_dt_id in the machine code, this must occur
before the cpu gets realized. We must open code the cpu creation to be
able to do this.

This patch just does that. It borrows some lines from previous work
from Bharata to handle the feature parsing.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index dc3d214009c5..57f4ddd073d0 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -32,6 +32,7 @@
 #include "sysemu/cpus.h"
 #include "hw/timer/m48t59.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "hw/loader.h"
 #include "sysemu/kvm.h"
@@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
 PowerPCCPU *ppc_cpu_init(const char *cpu_model)
 {
-    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+    PowerPCCPU *cpu;
+    CPUClass *cc;
+    ObjectClass *oc;
+    gchar **model_pieces;
+    Error *err = NULL;
+
+    model_pieces = g_strsplit(cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        return NULL;
+    }
+
+    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        return NULL;
+    }
+
+    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
+
+    cc = CPU_CLASS(oc);
+    cc->parse_features(CPU(cpu), model_pieces[1], &err);
+    g_strfreev(model_pieces);
+    if (err != NULL) {
+        goto out;
+    }
+
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+
+out:
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    }
+
+    return cpu;
 }

[Qemu-devel] [PATCH v2 5/7] ppc: introduce ppc_set_vcpu_dt_id()

[Greg Kurz]

This patch introduces the ppc_set_vcpu_dt_id() function. It is
currently empty but it will be used to generate cpu_dt_id out of
a cpu_index provided by the machine.

It also changes the machine types to provide cpu_index. Since all
of them keep the cpus in an array, cpu_index is simply the index
in the array.

The only exception is pseries-2.7 which supports hotplug of cpu
cores and already open codes the cpu creation. Its case will be
covered in follow-up patch.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/e500.c          |    2 +-
 hw/ppc/mac_newworld.c  |    2 +-
 hw/ppc/mac_oldworld.c  |    2 +-
 hw/ppc/ppc.c           |   12 +++++++++++-
 hw/ppc/ppc440_bamboo.c |    2 +-
 hw/ppc/prep.c          |    2 +-
 hw/ppc/spapr.c         |    2 +-
 include/hw/ppc/ppc.h   |    2 +-
 8 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ff5d92e48dd9..461dcdc031b0 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         CPUState *cs;
         qemu_irq *input;
 
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to initialize CPU!\n");
             exit(1);
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6ab675c498d0..888f448796e7 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
 #endif
     }
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 77fbdfffd4e2..ceb92f820dde 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "G3";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 57f4ddd073d0..dbc8ac7b3a9b 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1352,7 +1352,12 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
     return NULL;
 }
 
-PowerPCCPU *ppc_cpu_init(const char *cpu_model)
+static void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
+{
+    ;
+}
+
+PowerPCCPU *ppc_cpu_init(const char *cpu_model, int cpu_index)
 {
     PowerPCCPU *cpu;
     CPUClass *cc;
@@ -1374,6 +1379,11 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
 
     cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
 
+    ppc_set_vcpu_dt_id(cpu, cpu_index, &err);
+    if (err != NULL) {
+        goto out;
+    }
+
     cc = CPU_CLASS(oc);
     cc->parse_features(CPU(cpu), model_pieces[1], &err);
     g_strfreev(model_pieces);
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 7f22433c8e91..86c453542a94 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "440EP";
     }
-    cpu = ppc_cpu_init(machine->cpu_model);
+    cpu = ppc_cpu_init(machine->cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index e62fe643f492..d81d0675255e 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "602";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 690ee486aa07..57dbac2106d2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1828,7 +1828,7 @@ static void ppc_spapr_init(MachineState *machine)
         g_free(type);
     } else {
         for (i = 0; i < smp_cpus; i++) {
-            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
+            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model, i);
             if (cpu == NULL) {
                 error_report("Unable to find PowerPC CPU definition");
                 exit(1);
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index a4db1db82e1b..647451c9b6ac 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -106,5 +106,5 @@ enum {
 /* ppc_booke.c */
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
-PowerPCCPU *ppc_cpu_init(const char *cpu_model);
+PowerPCCPU *ppc_cpu_init(const char *cpu_model, int cpu_index);
 #endif

[Qemu-devel] [PATCH v2 6/7] spapr: use ppc_set_vcpu_dt_id() in CPU hotplug code

[Greg Kurz]

Starting with version 2.7, pseries machine now support hotplug of
cpu cores. The implementation requires to open code cpu creation
and thus does not call ppc_cpu_init().

This patch does all the plumbing to allow pseries machine types
with version >= 2.7 to generate cpu DT ids out of the indexes
of the cores and threads in their respective arrays.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/ppc.c            |    2 +-
 hw/ppc/spapr_cpu_core.c |   11 +++++++++--
 include/hw/ppc/ppc.h    |    1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index dbc8ac7b3a9b..12de255fb211 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1352,7 +1352,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
     return NULL;
 }
 
-static void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
+void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
 {
     ;
 }
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 70b6b0b5ee17..475c8063f086 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -259,13 +259,20 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void spapr_cpu_core_realize_child(Object *child, Error **errp)
+static void spapr_cpu_core_realize_child(Object *child, int cpu_index,
+                                         Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
 
+    ppc_set_vcpu_dt_id(cpu, cpu_index, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -306,7 +313,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     for (j = 0; j < cc->nr_threads; j++) {
         obj = sc->threads + j * size;
 
-        spapr_cpu_core_realize_child(obj, &local_err);
+        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
         if (local_err) {
             goto err;
         }
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 647451c9b6ac..a9067f45b3f4 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -107,4 +107,5 @@ enum {
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
 PowerPCCPU *ppc_cpu_init(const char *cpu_model, int cpu_index);
+void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp);
 #endif

[Qemu-devel] [PATCH v2 7/7] ppc: move the cpu_dt_id logic to machine code

[Greg Kurz]

Now that every supported machine type is able to provide a cpu_index, we
can safely move all the cpu_dt_id bits to the machine code.

TODO: the cpu_dt_id logic remains the same wannabe generic one as before
because of its target code background: machine types should provide their
own cpu_dt_id logic (it is required by the future powernv machine type for
example).

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/ppc.c                |   28 +++++++++++++++++++++++++++-
 target-ppc/translate_init.c |   30 ------------------------------
 2 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 12de255fb211..506b493bf43b 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1354,7 +1354,33 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
 void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
 {
-    ;
+    int max_smt = kvmppc_smt_threads();
+    int vcpu_dt_id;
+
+    if (smp_threads > max_smt) {
+        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
+                   max_smt, kvm_enabled() ? "KVM" : "TCG");
+        return;
+    }
+    if (!is_power_of_2(smp_threads)) {
+        error_setg(errp, "Cannot support %d threads on PPC with %s, "
+                   "threads count must be a power of 2.",
+                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
+        return;
+    }
+
+    vcpu_dt_id = (cpu_index / smp_threads) * max_smt
+        + (cpu_index % smp_threads);
+
+    if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_dt_id)) {
+        error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_dt_id);
+        error_append_hint(errp, "Adjust the number of cpus to %d "
+                          "or try to raise the number of threads per core\n",
+                          vcpu_dt_id * smp_threads / max_smt);
+        return;
+    }
+
+    cpu->cpu_dt_id = vcpu_dt_id;
 }
 
 PowerPCCPU *ppc_cpu_init(const char *cpu_model, int cpu_index)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 6706787b41a1..a54845a5be8f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9515,23 +9515,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     PowerPCCPU *cpu = POWERPC_CPU(dev);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
-#if !defined(CONFIG_USER_ONLY)
-    int max_smt = kvmppc_smt_threads();
-#endif
-
-#if !defined(CONFIG_USER_ONLY)
-    if (smp_threads > max_smt) {
-        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
-                   max_smt, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-    if (!is_power_of_2(smp_threads)) {
-        error_setg(errp, "Cannot support %d threads on PPC with %s, "
-                   "threads count must be a power of 2.",
-                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-#endif
 
     cpu_exec_init(cs, &local_err);
     if (local_err != NULL) {
@@ -9539,19 +9522,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-#if !defined(CONFIG_USER_ONLY)
-    cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
-        + (cs->cpu_index % smp_threads);
-
-    if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
-        error_setg(errp, "Can't create CPU with id %d in KVM", cpu->cpu_dt_id);
-        error_append_hint(errp, "Adjust the number of cpus to %d "
-                          "or try to raise the number of threads per core\n",
-                          cpu->cpu_dt_id * smp_threads / max_smt);
-        return;
-    }
-#endif
-
     if (tcg_enabled()) {
         if (ppc_fixup_cpu(cpu) != 0) {
             error_setg(errp, "Unable to emulate selected CPU with TCG");

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Bharata B Rao]

On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:
> If we want to generate cpu_dt_id in the machine code, this must occur
> before the cpu gets realized. We must open code the cpu creation to be
> able to do this.
> 
> This patch just does that. It borrows some lines from previous work
> from Bharata to handle the feature parsing.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index dc3d214009c5..57f4ddd073d0 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/cpus.h"
>  #include "hw/timer/m48t59.h"
>  #include "qemu/log.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "hw/loader.h"
>  #include "sysemu/kvm.h"
> @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> 
>  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
>  {
> -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> +    PowerPCCPU *cpu;
> +    CPUClass *cc;
> +    ObjectClass *oc;
> +    gchar **model_pieces;
> +    Error *err = NULL;
> +
> +    model_pieces = g_strsplit(cpu_model, ",", 2);
> +    if (!model_pieces[0]) {
> +        error_report("Invalid/empty CPU model name");
> +        return NULL;
> +    }
> +
> +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> +    if (oc == NULL) {
> +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> +        return NULL;
> +    }
> +
> +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> +
> +    cc = CPU_CLASS(oc);
> +    cc->parse_features(CPU(cpu), model_pieces[1], &err);

Igor is working on a patchset to convert -cpu features into global properties.
IIUC, after that patchset, it is not recommended to parse the -cpu features
for every CPU but do it only once.

That is what I attempted here in the context of supporting compat cpu type
for pseries-2.7:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 6/7] spapr: use ppc_set_vcpu_dt_id() in CPU hotplug code

[Bharata B Rao]

On Sat, Jul 02, 2016 at 12:42:04AM +0200, Greg Kurz wrote:
> Starting with version 2.7, pseries machine now support hotplug of
> cpu cores. The implementation requires to open code cpu creation
> and thus does not call ppc_cpu_init().
> 
> This patch does all the plumbing to allow pseries machine types
> with version >= 2.7 to generate cpu DT ids out of the indexes
> of the cores and threads in their respective arrays.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/ppc.c            |    2 +-
>  hw/ppc/spapr_cpu_core.c |   11 +++++++++--
>  include/hw/ppc/ppc.h    |    1 +
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index dbc8ac7b3a9b..12de255fb211 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1352,7 +1352,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>      return NULL;
>  }
> 
> -static void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
> +void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
>  {
>      ;
>  }
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 70b6b0b5ee17..475c8063f086 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -259,13 +259,20 @@ out:
>      error_propagate(errp, local_err);
>  }
> 
> -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> +static void spapr_cpu_core_realize_child(Object *child, int cpu_index,
> +                                         Error **errp)
>  {
>      Error *local_err = NULL;
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> 
> +    ppc_set_vcpu_dt_id(cpu, cpu_index, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -306,7 +313,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      for (j = 0; j < cc->nr_threads; j++) {
>          obj = sc->threads + j * size;
> 
> -        spapr_cpu_core_realize_child(obj, &local_err);
> +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);

cc->core_id is essentially the cpu_dt_id. For boot time cores, we set this
(via core-id prop) explicitly. For hotplugged cores, we expect user to
set this mandatorily on -device/device_add. The value that the
user is expected to provide as core-id is in fact supplied by us via
query-hotpluggable-cpus. So there are two places (ppc_spapr_init &
spapr_query_hotpluggable_cpus) where we generate the cpu_dt_id by
open coding as (core_index * smt). Can we have consolidate this logic
into some well defined routine like ppc_core_index_to_dt_id() ?

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 7/7] ppc: move the cpu_dt_id logic to machine code

[Bharata B Rao]

On Sat, Jul 02, 2016 at 12:42:12AM +0200, Greg Kurz wrote:
> Now that every supported machine type is able to provide a cpu_index, we
> can safely move all the cpu_dt_id bits to the machine code.
> 
> TODO: the cpu_dt_id logic remains the same wannabe generic one as before
> because of its target code background: machine types should provide their
> own cpu_dt_id logic (it is required by the future powernv machine type for
> example).
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/ppc.c                |   28 +++++++++++++++++++++++++++-
>  target-ppc/translate_init.c |   30 ------------------------------
>  2 files changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 12de255fb211..506b493bf43b 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1354,7 +1354,33 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> 
>  void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
>  {
> -    ;
> +    int max_smt = kvmppc_smt_threads();
> +    int vcpu_dt_id;
> +
> +    if (smp_threads > max_smt) {
> +        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> +                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> +        return;
> +    }
> +    if (!is_power_of_2(smp_threads)) {
> +        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> +                   "threads count must be a power of 2.",
> +                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> +        return;
> +    }

Not sure if the above two checks belong here in the routine which
set the cpu_dt_id.

Regards,
Bharata.

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

On Sat, 2 Jul 2016 13:36:22 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:
> > If we want to generate cpu_dt_id in the machine code, this must occur
> > before the cpu gets realized. We must open code the cpu creation to be
> > able to do this.
> > 
> > This patch just does that. It borrows some lines from previous work
> > from Bharata to handle the feature parsing.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index dc3d214009c5..57f4ddd073d0 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -32,6 +32,7 @@
> >  #include "sysemu/cpus.h"
> >  #include "hw/timer/m48t59.h"
> >  #include "qemu/log.h"
> > +#include "qapi/error.h"
> >  #include "qemu/error-report.h"
> >  #include "hw/loader.h"
> >  #include "sysemu/kvm.h"
> > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > 
> >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> >  {
> > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> > +    PowerPCCPU *cpu;
> > +    CPUClass *cc;
> > +    ObjectClass *oc;
> > +    gchar **model_pieces;
> > +    Error *err = NULL;
> > +
> > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > +    if (!model_pieces[0]) {
> > +        error_report("Invalid/empty CPU model name");
> > +        return NULL;
> > +    }
> > +
> > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > +    if (oc == NULL) {
> > +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> > +        return NULL;
> > +    }
> > +
> > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > +
> > +    cc = CPU_CLASS(oc);
> > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);  
> 
> Igor is working on a patchset to convert -cpu features into global properties.
> IIUC, after that patchset, it is not recommended to parse the -cpu features
> for every CPU but do it only once.
> 

cpu_generic_init() in the current code also does the parsing, and as the title
says, this patch is just about open coding the creation. I don't want to
change behavior yet.

But yes, I agree that we should only parse features once and I'll be more than
happy to fix this in a followup patch, based on Igor's work.

In the meantime, maybe I can add a comment stating that the parsing should go
away ?

> That is what I attempted here in the context of supporting compat cpu type
> for pseries-2.7:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html
> 

Yeah and this is where I borrowed some lines. :)

> Regards,
> Bharata.
> 

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v2 6/7] spapr: use ppc_set_vcpu_dt_id() in CPU hotplug code

[Greg Kurz]

On Sat, 2 Jul 2016 13:44:08 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Sat, Jul 02, 2016 at 12:42:04AM +0200, Greg Kurz wrote:
> > Starting with version 2.7, pseries machine now support hotplug of
> > cpu cores. The implementation requires to open code cpu creation
> > and thus does not call ppc_cpu_init().
> > 
> > This patch does all the plumbing to allow pseries machine types
> > with version >= 2.7 to generate cpu DT ids out of the indexes
> > of the cores and threads in their respective arrays.
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/ppc.c            |    2 +-
> >  hw/ppc/spapr_cpu_core.c |   11 +++++++++--
> >  include/hw/ppc/ppc.h    |    1 +
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index dbc8ac7b3a9b..12de255fb211 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -1352,7 +1352,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> >      return NULL;
> >  }
> > 
> > -static void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
> > +void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
> >  {
> >      ;
> >  }
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 70b6b0b5ee17..475c8063f086 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -259,13 +259,20 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> > 
> > -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> > +static void spapr_cpu_core_realize_child(Object *child, int cpu_index,
> > +                                         Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUState *cs = CPU(child);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > 
> > +    ppc_set_vcpu_dt_id(cpu, cpu_index, &local_err);
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        return;
> > +    }
> > +
> >      object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> > @@ -306,7 +313,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (j = 0; j < cc->nr_threads; j++) {
> >          obj = sc->threads + j * size;
> > 
> > -        spapr_cpu_core_realize_child(obj, &local_err);
> > +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);  
> 
> cc->core_id is essentially the cpu_dt_id. For boot time cores, we set this
> (via core-id prop) explicitly. For hotplugged cores, we expect user to
> set this mandatorily on -device/device_add. The value that the
> user is expected to provide as core-id is in fact supplied by us via
> query-hotpluggable-cpus. So there are two places (ppc_spapr_init &
> spapr_query_hotpluggable_cpus) where we generate the cpu_dt_id by
> open coding as (core_index * smt). Can we have consolidate this logic
> into some well defined routine like ppc_core_index_to_dt_id() ?
> 

Sure ! I'll have a closer look.

> Regards,
> Bharata.
> 

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v2 7/7] ppc: move the cpu_dt_id logic to machine code

[Greg Kurz]

On Sat, 2 Jul 2016 13:45:44 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Sat, Jul 02, 2016 at 12:42:12AM +0200, Greg Kurz wrote:
> > Now that every supported machine type is able to provide a cpu_index, we
> > can safely move all the cpu_dt_id bits to the machine code.
> > 
> > TODO: the cpu_dt_id logic remains the same wannabe generic one as before
> > because of its target code background: machine types should provide their
> > own cpu_dt_id logic (it is required by the future powernv machine type for
> > example).
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/ppc.c                |   28 +++++++++++++++++++++++++++-
> >  target-ppc/translate_init.c |   30 ------------------------------
> >  2 files changed, 27 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 12de255fb211..506b493bf43b 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -1354,7 +1354,33 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > 
> >  void ppc_set_vcpu_dt_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
> >  {
> > -    ;
> > +    int max_smt = kvmppc_smt_threads();
> > +    int vcpu_dt_id;
> > +
> > +    if (smp_threads > max_smt) {
> > +        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> > +                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> > +        return;
> > +    }
> > +    if (!is_power_of_2(smp_threads)) {
> > +        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> > +                   "threads count must be a power of 2.",
> > +                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > +        return;
> > +    }  
> 
> Not sure if the above two checks belong here in the routine which
> set the cpu_dt_id.
> 

I'm pretty sure they don't belong here :) ! But again, this is the very same
code that we currently have in the target and I don't want to change behavior
here (maybe I should mention it in the changelog).

This can be fixed in a followup patch.

> Regards,
> Bharata.
> 

Thanks for your feedback Bharata !

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v2 0/7] ppc: compute cpu_dt_id in the machine code

[Bharata B Rao]

On Sat, Jul 02, 2016 at 12:41:14AM +0200, Greg Kurz wrote:
> This series is a sequel to the discussion on a patch from Ben's powernv
> patchset:
> 
>         http://patchwork.ozlabs.org/patch/597153/
> 
> Indeed, since the DT is a machine abstraction, it should definitely sit
> under hw/ppc and not in the target code:
> - all machine types are forced to share the same numbering logic
> - user mode does not need that => there are #ifdef everywhere
> 
> So this series moves all the current numbering logic to the machine
> code.
> 
> The patchset was completely re-written according to Igor's valuable
> suggestions. The main change is that cpu_dt_id is now computed out
> of a cpu_index provided by the machine, instead of bending the code
> to use cs->cpu_index.
> 
> I resend Bharata's patch without any modification because patch 6 is
> based on it (pseries-2.7 support).
> 
> I did not see any regression on pseries-2.6, pseries-2.7 and CPU hotplug.

Haven't had a chance to debug yet, but just noticed that CPU hotplug
seems to have issues after this patchset on mainline.

-smp 8,cores=1,threads=4,maxcpus=32
(qemu) device_add host-spapr-cpu-core,id=core1,core-id=16
(qemu) info cpus
* CPU #0: nip=0xc00000000007dc6c thread_id=10865
  CPU #1: nip=0xc00000000007dc6c thread_id=10866
  CPU #2: nip=0xc00000000007dc6c thread_id=10867
  CPU #3: nip=0xc00000000007dc6c thread_id=10868
  CPU #4: nip=0xc00000000007dc6c thread_id=10869
  CPU #5: nip=0xc00000000007dc6c thread_id=10870
  CPU #6: nip=0xc00000000007dc6c thread_id=10871
  CPU #7: nip=0xc00000000007dc6c thread_id=10872
  CPU #8: nip=0x0000000000000000 (halted) thread_id=10948
  CPU #9: nip=0x0000000000000000 (halted) thread_id=10949
  CPU #10: nip=0x0000000000000000 (halted) thread_id=10950
  CPU #11: nip=0x0000000000000000 (halted) thread_id=10951

[root@localhost cpus]# ls
#address-cells   ibm,drc-power-domains  name               #size-cells
ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0
ibm,drc-names    linux,phandle          PowerPC,POWER8@10

[root@localhost cpus]# DEBUG: read_rtas_events(): Received RTAS event 1
DEBUG: handle_rtas_event(): Handling RTAS event 1
DEBUG: handle_rtas_event(): Entering check_platform_dump()
DEBUG: print_rtas_event(): Writing RTAS event 1 to /var/log/platform
DEBUG: handle_rtas_event(): Entering Hotplug handler
DEBUG: handle_hotplug_event(): Build drmgr command

DEBUG: handle_hotplug_event(): run: drmgr -c cpu -a -s 0x10000010 (null)

DEBUG: handle_hotplug_event(): Invoke drmgr command

Validating CPU DLPAR capability...yes.
Requested CPU with drc index 10000010 is already present.
DEBUG: handle_hotplug_event(): drmgr call exited with 1

If instead of core-id=16, if I use core-id=24, then hotplug succeeds but
the CPU gets unexpected dt_id(0x30) as shown below:

(qemu) device_add host-spapr-cpu-core,id=core2,core-id=24

[root@localhost cpus]# ls
#address-cells   ibm,drc-power-domains  name               #size-cells
ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0
ibm,drc-names    linux,phandle          PowerPC,POWER8@10

[root@localhost cpus]# DEBUG: read_rtas_events(): Received RTAS event 1
DEBUG: handle_rtas_event(): Handling RTAS event 1
DEBUG: handle_rtas_event(): Entering check_platform_dump()
DEBUG: print_rtas_event(): Writing RTAS event 1 to /var/log/platform
DEBUG: handle_rtas_event(): Entering Hotplug handler
DEBUG: handle_hotplug_event(): Build drmgr command

DEBUG: handle_hotplug_event(): run: drmgr -c cpu -a -s 0x10000018 (null)

DEBUG: handle_hotplug_event(): Invoke drmgr command

Validating CPU DLPAR capability...yes.
Could not retrieve ibm,ppc-interrupt-server#s property for 
CPU 24
Rotating logs...
DEBUG: handle_hotplug_event(): drmgr call exited with 0

DEBUG: process_v6(): Processing version 6 event
DEBUG: report_menugoal(): menugoal: number = 651301, message = "Platform
Firmware Not applicable."
DEBUG: (Sequence #1) servicelog key 96.

[root@localhost cpus]# ls
#address-cells   ibm,drc-power-domains  name               PowerPC,POWER8@30
ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0   #size-cells
ibm,drc-names    linux,phandle          PowerPC,POWER8@10

Re: [Qemu-devel] [PATCH v2 0/7] ppc: compute cpu_dt_id in the machine code

[Greg Kurz]

On Sat, 2 Jul 2016 15:25:07 +0530
Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:

> On Sat, Jul 02, 2016 at 12:41:14AM +0200, Greg Kurz wrote:
> > This series is a sequel to the discussion on a patch from Ben's powernv
> > patchset:
> > 
> >         http://patchwork.ozlabs.org/patch/597153/
> > 
> > Indeed, since the DT is a machine abstraction, it should definitely sit
> > under hw/ppc and not in the target code:
> > - all machine types are forced to share the same numbering logic
> > - user mode does not need that => there are #ifdef everywhere
> > 
> > So this series moves all the current numbering logic to the machine
> > code.
> > 
> > The patchset was completely re-written according to Igor's valuable
> > suggestions. The main change is that cpu_dt_id is now computed out
> > of a cpu_index provided by the machine, instead of bending the code
> > to use cs->cpu_index.
> > 
> > I resend Bharata's patch without any modification because patch 6 is
> > based on it (pseries-2.7 support).
> > 
> > I did not see any regression on pseries-2.6, pseries-2.7 and CPU hotplug.  
> 
> Haven't had a chance to debug yet, but just noticed that CPU hotplug
> seems to have issues after this patchset on mainline.
> 

I forgot to mention this patchset is based on David's ppc-for-2.7 tree. I'll
give a try with mainline.

> -smp 8,cores=1,threads=4,maxcpus=32
> (qemu) device_add host-spapr-cpu-core,id=core1,core-id=16
> (qemu) info cpus
> * CPU #0: nip=0xc00000000007dc6c thread_id=10865
>   CPU #1: nip=0xc00000000007dc6c thread_id=10866
>   CPU #2: nip=0xc00000000007dc6c thread_id=10867
>   CPU #3: nip=0xc00000000007dc6c thread_id=10868
>   CPU #4: nip=0xc00000000007dc6c thread_id=10869
>   CPU #5: nip=0xc00000000007dc6c thread_id=10870
>   CPU #6: nip=0xc00000000007dc6c thread_id=10871
>   CPU #7: nip=0xc00000000007dc6c thread_id=10872
>   CPU #8: nip=0x0000000000000000 (halted) thread_id=10948
>   CPU #9: nip=0x0000000000000000 (halted) thread_id=10949
>   CPU #10: nip=0x0000000000000000 (halted) thread_id=10950
>   CPU #11: nip=0x0000000000000000 (halted) thread_id=10951
> 
> [root@localhost cpus]# ls
> #address-cells   ibm,drc-power-domains  name               #size-cells
> ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0
> ibm,drc-names    linux,phandle          PowerPC,POWER8@10
> 
> [root@localhost cpus]# DEBUG: read_rtas_events(): Received RTAS event 1
> DEBUG: handle_rtas_event(): Handling RTAS event 1
> DEBUG: handle_rtas_event(): Entering check_platform_dump()
> DEBUG: print_rtas_event(): Writing RTAS event 1 to /var/log/platform
> DEBUG: handle_rtas_event(): Entering Hotplug handler
> DEBUG: handle_hotplug_event(): Build drmgr command
> 
> DEBUG: handle_hotplug_event(): run: drmgr -c cpu -a -s 0x10000010 (null)
> 
> DEBUG: handle_hotplug_event(): Invoke drmgr command
> 
> Validating CPU DLPAR capability...yes.
> Requested CPU with drc index 10000010 is already present.
> DEBUG: handle_hotplug_event(): drmgr call exited with 1
> 
> If instead of core-id=16, if I use core-id=24, then hotplug succeeds but
> the CPU gets unexpected dt_id(0x30) as shown below:
> 
> (qemu) device_add host-spapr-cpu-core,id=core2,core-id=24
> 
> [root@localhost cpus]# ls
> #address-cells   ibm,drc-power-domains  name               #size-cells
> ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0
> ibm,drc-names    linux,phandle          PowerPC,POWER8@10
> 
> [root@localhost cpus]# DEBUG: read_rtas_events(): Received RTAS event 1
> DEBUG: handle_rtas_event(): Handling RTAS event 1
> DEBUG: handle_rtas_event(): Entering check_platform_dump()
> DEBUG: print_rtas_event(): Writing RTAS event 1 to /var/log/platform
> DEBUG: handle_rtas_event(): Entering Hotplug handler
> DEBUG: handle_hotplug_event(): Build drmgr command
> 
> DEBUG: handle_hotplug_event(): run: drmgr -c cpu -a -s 0x10000018 (null)
> 
> DEBUG: handle_hotplug_event(): Invoke drmgr command
> 
> Validating CPU DLPAR capability...yes.
> Could not retrieve ibm,ppc-interrupt-server#s property for 
> CPU 24
> Rotating logs...
> DEBUG: handle_hotplug_event(): drmgr call exited with 0
> 
> DEBUG: process_v6(): Processing version 6 event
> DEBUG: report_menugoal(): menugoal: number = 651301, message = "Platform
> Firmware Not applicable."
> DEBUG: (Sequence #1) servicelog key 96.
> 
> [root@localhost cpus]# ls
> #address-cells   ibm,drc-power-domains  name               PowerPC,POWER8@30
> ibm,drc-indexes  ibm,drc-types          PowerPC,POWER8@0   #size-cells
> ibm,drc-names    linux,phandle          PowerPC,POWER8@10
> 

Re: [Qemu-devel] [PATCH v2 2/7] ppc: simplify max_smt initialization in ppc_cpu_realizefn()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1061 bytes --]

On Sat, Jul 02, 2016 at 12:41:32AM +0200, Greg Kurz wrote:
> kvmppc_smt_threads() returns 1 if KVM is not enabled.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

Applied to ppc-for-2.7

> ---
>  target-ppc/translate_init.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 843f19b748fb..a06bf50b65d4 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9516,7 +9516,7 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      Error *local_err = NULL;
>  #if !defined(CONFIG_USER_ONLY)
> -    int max_smt = kvm_enabled() ? kvmppc_smt_threads() : 1;
> +    int max_smt = kvmppc_smt_threads();
>  #endif
>  
>  #if !defined(CONFIG_USER_ONLY)
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3763 bytes --]

On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote:
> On Sat, 2 Jul 2016 13:36:22 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:
> > > If we want to generate cpu_dt_id in the machine code, this must occur
> > > before the cpu gets realized. We must open code the cpu creation to be
> > > able to do this.
> > > 
> > > This patch just does that. It borrows some lines from previous work
> > > from Bharata to handle the feature parsing.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > index dc3d214009c5..57f4ddd073d0 100644
> > > --- a/hw/ppc/ppc.c
> > > +++ b/hw/ppc/ppc.c
> > > @@ -32,6 +32,7 @@
> > >  #include "sysemu/cpus.h"
> > >  #include "hw/timer/m48t59.h"
> > >  #include "qemu/log.h"
> > > +#include "qapi/error.h"
> > >  #include "qemu/error-report.h"
> > >  #include "hw/loader.h"
> > >  #include "sysemu/kvm.h"
> > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > > 
> > >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > >  {
> > > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> > > +    PowerPCCPU *cpu;
> > > +    CPUClass *cc;
> > > +    ObjectClass *oc;
> > > +    gchar **model_pieces;
> > > +    Error *err = NULL;
> > > +
> > > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > > +    if (!model_pieces[0]) {
> > > +        error_report("Invalid/empty CPU model name");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > > +    if (oc == NULL) {
> > > +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> > > +        return NULL;
> > > +    }
> > > +
> > > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > +
> > > +    cc = CPU_CLASS(oc);
> > > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);  
> > 
> > Igor is working on a patchset to convert -cpu features into global properties.
> > IIUC, after that patchset, it is not recommended to parse the -cpu features
> > for every CPU but do it only once.
> > 
> 
> cpu_generic_init() in the current code also does the parsing, and as the title
> says, this patch is just about open coding the creation. I don't want to
> change behavior yet.
> 
> But yes, I agree that we should only parse features once and I'll be more than
> happy to fix this in a followup patch, based on Igor's work.
> 
> In the meantime, maybe I can add a comment stating that the parsing should go
> away ?

Right.  But the thing is by open coding here, you're making two copies
that need to be fixed instead of one, which increases the chances of
error.

It seems like it would be safer to change the generic code so there's
a new generic function which doesn't do the realize which we can use
on ppc (and other platforms when/if they need it).

Doing the change just on ppc by making our own copy of
cpu_generic_init() seems more like to lead to future mistakes.

> > That is what I attempted here in the context of supporting compat cpu type
> > for pseries-2.7:
> > 
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html
> > 
> 
> Yeah and this is where I borrowed some lines. :)
> 
> > Regards,
> > Bharata.
> > 
> 
> Cheers.
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 3966 bytes --]

On Mon, 4 Jul 2016 13:54:55 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote:
> > On Sat, 2 Jul 2016 13:36:22 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> >   
> > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:  
> > > > If we want to generate cpu_dt_id in the machine code, this must occur
> > > > before the cpu gets realized. We must open code the cpu creation to be
> > > > able to do this.
> > > > 
> > > > This patch just does that. It borrows some lines from previous work
> > > > from Bharata to handle the feature parsing.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > > index dc3d214009c5..57f4ddd073d0 100644
> > > > --- a/hw/ppc/ppc.c
> > > > +++ b/hw/ppc/ppc.c
> > > > @@ -32,6 +32,7 @@
> > > >  #include "sysemu/cpus.h"
> > > >  #include "hw/timer/m48t59.h"
> > > >  #include "qemu/log.h"
> > > > +#include "qapi/error.h"
> > > >  #include "qemu/error-report.h"
> > > >  #include "hw/loader.h"
> > > >  #include "sysemu/kvm.h"
> > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > > > 
> > > >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > >  {
> > > > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> > > > +    PowerPCCPU *cpu;
> > > > +    CPUClass *cc;
> > > > +    ObjectClass *oc;
> > > > +    gchar **model_pieces;
> > > > +    Error *err = NULL;
> > > > +
> > > > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > > > +    if (!model_pieces[0]) {
> > > > +        error_report("Invalid/empty CPU model name");
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > > > +    if (oc == NULL) {
> > > > +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > > +
> > > > +    cc = CPU_CLASS(oc);
> > > > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);    
> > > 
> > > Igor is working on a patchset to convert -cpu features into global properties.
> > > IIUC, after that patchset, it is not recommended to parse the -cpu features
> > > for every CPU but do it only once.
> > >   
> > 
> > cpu_generic_init() in the current code also does the parsing, and as the title
> > says, this patch is just about open coding the creation. I don't want to
> > change behavior yet.
> > 
> > But yes, I agree that we should only parse features once and I'll be more than
> > happy to fix this in a followup patch, based on Igor's work.
> > 
> > In the meantime, maybe I can add a comment stating that the parsing should go
> > away ?  
> 
> Right.  But the thing is by open coding here, you're making two copies
> that need to be fixed instead of one, which increases the chances of
> error.
> 
> It seems like it would be safer to change the generic code so there's
> a new generic function which doesn't do the realize which we can use
> on ppc (and other platforms when/if they need it).
> 
> Doing the change just on ppc by making our own copy of
> cpu_generic_init() seems more like to lead to future mistakes.
> 

I had this in v1:

http://patchwork.ozlabs.org/patch/642216/

I'll repost it in v3.

> > > That is what I attempted here in the context of supporting compat cpu type
> > > for pseries-2.7:
> > > 
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html
> > >   
> > 
> > Yeah and this is where I borrowed some lines. :)
> >   
> > > Regards,
> > > Bharata.
> > >   
> > 
> > Cheers.
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v2 3/7] ppc: different creation paths for cpus in system and user mode

[Igor Mammedov]

On Sat, 02 Jul 2016 00:41:40 +0200
Greg Kurz <groug@kaod.org> wrote:

> The machine code currently uses the same cpu_ppc_init() function to
> create cpus as the user mode. This function also triggers the cpu
> realization.
> 
> It is okay for user mode but with system mode we may want to do other
> things between initialization and realization, like generating cpu
> ids for the DT for example.
> 
> With this patch, each mode has its own creation helper:
> - ppc_cpu_init() is for system mode only
> - cpu_init() is for user mode only
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/e500.c               |    2 +-
>  hw/ppc/mac_newworld.c       |    2 +-
>  hw/ppc/mac_oldworld.c       |    2 +-
>  hw/ppc/ppc.c                |    5 +++++
>  hw/ppc/ppc440_bamboo.c      |    2 +-
>  hw/ppc/ppc4xx_devs.c        |    2 +-
>  hw/ppc/prep.c               |    2 +-
>  hw/ppc/spapr.c              |    2 +-
>  hw/ppc/virtex_ml507.c       |    2 +-
>  include/hw/ppc/ppc.h        |    1 +
>  target-ppc/cpu.h            |    5 +++--
>  target-ppc/translate_init.c |    5 -----
>  12 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 0cd534df55f8..ff5d92e48dd9 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine,
> PPCE500Params *params) CPUState *cs;
>          qemu_irq *input;
>  
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to initialize CPU!\n");
>              exit(1);
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 32e88b378687..6ab675c498d0 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
>  #endif
>      }
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU
> definition\n"); exit(1);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 447948746b1a..77fbdfffd4e2 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState
> *machine) if (machine->cpu_model == NULL)
>          machine->cpu_model = "G3";
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU
> definition\n"); exit(1);
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index e4252528a69d..dc3d214009c5 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1350,3 +1350,8 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>  
>      return NULL;
>  }
> +
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> +{
> +    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> cpu_model)); +}
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index 5c535b18a20d..7f22433c8e91 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = "440EP";
>      }
> -    cpu = cpu_ppc_init(machine->cpu_model);
> +    cpu = ppc_cpu_init(machine->cpu_model);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index e7f413e49d08..94a24243af70 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -56,7 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
>      CPUPPCState *env;
>  
>      /* init CPUs */
> -    cpu = cpu_ppc_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model, 0);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
>                  cpu_model);
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 054af1e8b481..e62fe643f492 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
>      if (machine->cpu_model == NULL)
>          machine->cpu_model = "602";
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU
> definition\n"); exit(1);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 78ebd9ee38ce..690ee486aa07 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1828,7 +1828,7 @@ static void ppc_spapr_init(MachineState
> *machine) g_free(type);
>      } else {
>          for (i = 0; i < smp_cpus; i++) {
> -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
>              if (cpu == NULL) {
>                  error_report("Unable to find PowerPC CPU
> definition"); exit(1);
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index b97d96685cf1..8d350fb98b2c 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -96,7 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t
> *ram_size, CPUPPCState *env;
>      qemu_irq *irqs;
>  
> -    cpu = cpu_ppc_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model, 0);
shouldn't it have only 1 argument?

>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index 5617dc4a2c04..a4db1db82e1b 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -106,4 +106,5 @@ enum {
>  /* ppc_booke.c */
>  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t
> flags); 
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model);
>  #endif
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index af73bced9f70..440309a68006 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1196,7 +1196,6 @@ extern const struct VMStateDescription
> vmstate_ppc_cpu; #endif
>  
>  /*****************************************************************************/
> -PowerPCCPU *cpu_ppc_init(const char *cpu_model);
>  void ppc_translate_init(void);
>  void gen_update_current_nip(void *opaque);
>  /* you can call this signal handler from your SIGBUS and SIGSEGV
> @@ -1275,7 +1274,9 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState
> *env, int gprn) int ppc_dcr_read (ppc_dcr_t *dcr_env, int dcrn,
> uint32_t *valp); int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn,
> uint32_t val); 
> -#define cpu_init(cpu_model) CPU(cpu_ppc_init(cpu_model))
> +#if defined(CONFIG_USER_ONLY)
> +#define cpu_init(cpu_model) cpu_generic_init(TYPE_POWERPC_CPU,
> cpu_model) +#endif
>  
>  #define cpu_signal_handler cpu_ppc_signal_handler
>  #define cpu_list ppc_cpu_list
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index a06bf50b65d4..6706787b41a1 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10000,11 +10000,6 @@ static ObjectClass
> *ppc_cpu_class_by_name(const char *name) return NULL;
>  }
>  
> -PowerPCCPU *cpu_ppc_init(const char *cpu_model)
> -{
> -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> cpu_model)); -}
> -
>  /* Sort by PVR, ordering special case "host" last. */
>  static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> 

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Igor Mammedov]

On Mon, 4 Jul 2016 13:54:55 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote:
> > On Sat, 2 Jul 2016 13:36:22 +0530
> > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > 
> > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:
> > > > If we want to generate cpu_dt_id in the machine code, this must
> > > > occur before the cpu gets realized. We must open code the cpu
> > > > creation to be able to do this.
> > > > 
> > > > This patch just does that. It borrows some lines from previous
> > > > work from Bharata to handle the feature parsing.
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > > index dc3d214009c5..57f4ddd073d0 100644
> > > > --- a/hw/ppc/ppc.c
> > > > +++ b/hw/ppc/ppc.c
> > > > @@ -32,6 +32,7 @@
> > > >  #include "sysemu/cpus.h"
> > > >  #include "hw/timer/m48t59.h"
> > > >  #include "qemu/log.h"
> > > > +#include "qapi/error.h"
> > > >  #include "qemu/error-report.h"
> > > >  #include "hw/loader.h"
> > > >  #include "sysemu/kvm.h"
> > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int
> > > > cpu_dt_id)
> > > > 
> > > >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > >  {
> > > > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> > > > cpu_model));
> > > > +    PowerPCCPU *cpu;
> > > > +    CPUClass *cc;
> > > > +    ObjectClass *oc;
> > > > +    gchar **model_pieces;
> > > > +    Error *err = NULL;
> > > > +
> > > > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > > > +    if (!model_pieces[0]) {
> > > > +        error_report("Invalid/empty CPU model name");
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > > > +    if (oc == NULL) {
> > > > +        error_report("Unable to find CPU definition: %s",
> > > > model_pieces[0]);
> > > > +        return NULL;
> > > > +    }
> > > > +
> > > > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > > +
> > > > +    cc = CPU_CLASS(oc);
> > > > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);  
> > > 
> > > Igor is working on a patchset to convert -cpu features into
> > > global properties. IIUC, after that patchset, it is not
> > > recommended to parse the -cpu features for every CPU but do it
> > > only once.
> > > 
> > 
> > cpu_generic_init() in the current code also does the parsing, and
> > as the title says, this patch is just about open coding the
> > creation. I don't want to change behavior yet.
> > 
> > But yes, I agree that we should only parse features once and I'll
> > be more than happy to fix this in a followup patch, based on Igor's
> > work.
> > 
> > In the meantime, maybe I can add a comment stating that the parsing
> > should go away ?
> 
> Right.  But the thing is by open coding here, you're making two copies
> that need to be fixed instead of one, which increases the chances of
> error.
this patch matches what has been done for x86 target as a pert of
decoupling *-user mode from machine emulation.

> It seems like it would be safer to change the generic code so there's
> a new generic function which doesn't do the realize which we can use
> on ppc (and other platforms when/if they need it).
We've had it in x86 until recently but I've replaced it
cpu_generic_init(), please don't go that route.

There is not much to generalize here so far it's basically following
code
   cpu = object_new(cpu-type)
   parse-features(cpu)

   set_properties(cpu) /* optional machine specific */

   cpu->realize()

once parse-features refactoring is merged, there won't be anything cpu
specific left as parse-features(cpu) could be moved to generic code
and only very machine specific set_properties would be left which are
not generalizable.

> 
> Doing the change just on ppc by making our own copy of
> cpu_generic_init() seems more like to lead to future mistakes.
I wouldn't touch cpu_init()/cpu_generic_init() and leave it only for
*-user users (as it's been done for x86).

> 
> > > That is what I attempted here in the context of supporting compat
> > > cpu type for pseries-2.7:
> > > 
> > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html
> > > 
> > 
> > Yeah and this is where I borrowed some lines. :)
> > 
> > > Regards,
> > > Bharata.
> > > 
> > 
> > Cheers.
> > 
> 

Re: [Qemu-devel] [PATCH v2 3/7] ppc: different creation paths for cpus in system and user mode

[Greg Kurz]

On Mon, 4 Jul 2016 09:14:43 +0200
Igor Mammedov <imammedo@redhat.com> wrote:

> On Sat, 02 Jul 2016 00:41:40 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > The machine code currently uses the same cpu_ppc_init() function to
> > create cpus as the user mode. This function also triggers the cpu
> > realization.
> > 
> > It is okay for user mode but with system mode we may want to do other
> > things between initialization and realization, like generating cpu
> > ids for the DT for example.
> > 
> > With this patch, each mode has its own creation helper:
> > - ppc_cpu_init() is for system mode only
> > - cpu_init() is for user mode only
> > 
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/e500.c               |    2 +-
> >  hw/ppc/mac_newworld.c       |    2 +-
> >  hw/ppc/mac_oldworld.c       |    2 +-
> >  hw/ppc/ppc.c                |    5 +++++
> >  hw/ppc/ppc440_bamboo.c      |    2 +-
> >  hw/ppc/ppc4xx_devs.c        |    2 +-
> >  hw/ppc/prep.c               |    2 +-
> >  hw/ppc/spapr.c              |    2 +-
> >  hw/ppc/virtex_ml507.c       |    2 +-
> >  include/hw/ppc/ppc.h        |    1 +
> >  target-ppc/cpu.h            |    5 +++--
> >  target-ppc/translate_init.c |    5 -----
> >  12 files changed, 17 insertions(+), 15 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index 0cd534df55f8..ff5d92e48dd9 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine,
> > PPCE500Params *params) CPUState *cs;
> >          qemu_irq *input;
> >  
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to initialize CPU!\n");
> >              exit(1);
> > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > index 32e88b378687..6ab675c498d0 100644
> > --- a/hw/ppc/mac_newworld.c
> > +++ b/hw/ppc/mac_newworld.c
> > @@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
> >  #endif
> >      }
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU
> > definition\n"); exit(1);
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index 447948746b1a..77fbdfffd4e2 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState
> > *machine) if (machine->cpu_model == NULL)
> >          machine->cpu_model = "G3";
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU
> > definition\n"); exit(1);
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index e4252528a69d..dc3d214009c5 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -1350,3 +1350,8 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> >  
> >      return NULL;
> >  }
> > +
> > +PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > +{
> > +    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> > cpu_model)); +}
> > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> > index 5c535b18a20d..7f22433c8e91 100644
> > --- a/hw/ppc/ppc440_bamboo.c
> > +++ b/hw/ppc/ppc440_bamboo.c
> > @@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
> >      if (machine->cpu_model == NULL) {
> >          machine->cpu_model = "440EP";
> >      }
> > -    cpu = cpu_ppc_init(machine->cpu_model);
> > +    cpu = ppc_cpu_init(machine->cpu_model);
> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to initialize CPU!\n");
> >          exit(1);
> > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > index e7f413e49d08..94a24243af70 100644
> > --- a/hw/ppc/ppc4xx_devs.c
> > +++ b/hw/ppc/ppc4xx_devs.c
> > @@ -56,7 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
> >      CPUPPCState *env;
> >  
> >      /* init CPUs */
> > -    cpu = cpu_ppc_init(cpu_model);
> > +    cpu = ppc_cpu_init(cpu_model, 0);
> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
> >                  cpu_model);
> > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > index 054af1e8b481..e62fe643f492 100644
> > --- a/hw/ppc/prep.c
> > +++ b/hw/ppc/prep.c
> > @@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
> >      if (machine->cpu_model == NULL)
> >          machine->cpu_model = "602";
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = cpu_ppc_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU
> > definition\n"); exit(1);
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 78ebd9ee38ce..690ee486aa07 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1828,7 +1828,7 @@ static void ppc_spapr_init(MachineState
> > *machine) g_free(type);
> >      } else {
> >          for (i = 0; i < smp_cpus; i++) {
> > -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> > +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
> >              if (cpu == NULL) {
> >                  error_report("Unable to find PowerPC CPU
> > definition"); exit(1);
> > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > index b97d96685cf1..8d350fb98b2c 100644
> > --- a/hw/ppc/virtex_ml507.c
> > +++ b/hw/ppc/virtex_ml507.c
> > @@ -96,7 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t
> > *ram_size, CPUPPCState *env;
> >      qemu_irq *irqs;
> >  
> > -    cpu = cpu_ppc_init(cpu_model);
> > +    cpu = ppc_cpu_init(cpu_model, 0);  
> shouldn't it have only 1 argument?
> 

Yes ! And the build breaks... :-\

> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to initialize CPU!\n");
> >          exit(1);
> > diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> > index 5617dc4a2c04..a4db1db82e1b 100644
> > --- a/include/hw/ppc/ppc.h
> > +++ b/include/hw/ppc/ppc.h
> > @@ -106,4 +106,5 @@ enum {
> >  /* ppc_booke.c */
> >  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t
> > flags); 
> > +PowerPCCPU *ppc_cpu_init(const char *cpu_model);
> >  #endif
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index af73bced9f70..440309a68006 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -1196,7 +1196,6 @@ extern const struct VMStateDescription
> > vmstate_ppc_cpu; #endif
> >  
> >  /*****************************************************************************/
> > -PowerPCCPU *cpu_ppc_init(const char *cpu_model);
> >  void ppc_translate_init(void);
> >  void gen_update_current_nip(void *opaque);
> >  /* you can call this signal handler from your SIGBUS and SIGSEGV
> > @@ -1275,7 +1274,9 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState
> > *env, int gprn) int ppc_dcr_read (ppc_dcr_t *dcr_env, int dcrn,
> > uint32_t *valp); int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn,
> > uint32_t val); 
> > -#define cpu_init(cpu_model) CPU(cpu_ppc_init(cpu_model))
> > +#if defined(CONFIG_USER_ONLY)
> > +#define cpu_init(cpu_model) cpu_generic_init(TYPE_POWERPC_CPU,
> > cpu_model) +#endif
> >  
> >  #define cpu_signal_handler cpu_ppc_signal_handler
> >  #define cpu_list ppc_cpu_list
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index a06bf50b65d4..6706787b41a1 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -10000,11 +10000,6 @@ static ObjectClass
> > *ppc_cpu_class_by_name(const char *name) return NULL;
> >  }
> >  
> > -PowerPCCPU *cpu_ppc_init(const char *cpu_model)
> > -{
> > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> > cpu_model)); -}
> > -
> >  /* Sort by PVR, ordering special case "host" last. */
> >  static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
> >  {
> >   
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4448 bytes --]

On Mon, 4 Jul 2016 08:32:04 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Mon, 4 Jul 2016 13:54:55 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote:  
> > > On Sat, 2 Jul 2016 13:36:22 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > >     
> > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:    
> > > > > If we want to generate cpu_dt_id in the machine code, this must occur
> > > > > before the cpu gets realized. We must open code the cpu creation to be
> > > > > able to do this.
> > > > > 
> > > > > This patch just does that. It borrows some lines from previous work
> > > > > from Bharata to handle the feature parsing.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > > > index dc3d214009c5..57f4ddd073d0 100644
> > > > > --- a/hw/ppc/ppc.c
> > > > > +++ b/hw/ppc/ppc.c
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include "sysemu/cpus.h"
> > > > >  #include "hw/timer/m48t59.h"
> > > > >  #include "qemu/log.h"
> > > > > +#include "qapi/error.h"
> > > > >  #include "qemu/error-report.h"
> > > > >  #include "hw/loader.h"
> > > > >  #include "sysemu/kvm.h"
> > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > > > > 
> > > > >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > > >  {
> > > > > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> > > > > +    PowerPCCPU *cpu;
> > > > > +    CPUClass *cc;
> > > > > +    ObjectClass *oc;
> > > > > +    gchar **model_pieces;
> > > > > +    Error *err = NULL;
> > > > > +
> > > > > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > > > > +    if (!model_pieces[0]) {
> > > > > +        error_report("Invalid/empty CPU model name");
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > > > > +    if (oc == NULL) {
> > > > > +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > > > +
> > > > > +    cc = CPU_CLASS(oc);
> > > > > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);      
> > > > 
> > > > Igor is working on a patchset to convert -cpu features into global properties.
> > > > IIUC, after that patchset, it is not recommended to parse the -cpu features
> > > > for every CPU but do it only once.
> > > >     
> > > 
> > > cpu_generic_init() in the current code also does the parsing, and as the title
> > > says, this patch is just about open coding the creation. I don't want to
> > > change behavior yet.
> > > 
> > > But yes, I agree that we should only parse features once and I'll be more than
> > > happy to fix this in a followup patch, based on Igor's work.
> > > 
> > > In the meantime, maybe I can add a comment stating that the parsing should go
> > > away ?    
> > 
> > Right.  But the thing is by open coding here, you're making two copies
> > that need to be fixed instead of one, which increases the chances of
> > error.
> > 
> > It seems like it would be safer to change the generic code so there's
> > a new generic function which doesn't do the realize which we can use
> > on ppc (and other platforms when/if they need it).
> > 
> > Doing the change just on ppc by making our own copy of
> > cpu_generic_init() seems more like to lead to future mistakes.
> >   
> 
> I had this in v1:
> 
> http://patchwork.ozlabs.org/patch/642216/
> 
> I'll repost it in v3.
> 

I tend to agree with Igor's latest feeback:

https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00381.html

I'll keep the open coded version in v3.

> > > > That is what I attempted here in the context of supporting compat cpu type
> > > > for pseries-2.7:
> > > > 
> > > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg381660.html
> > > >     
> > > 
> > > Yeah and this is where I borrowed some lines. :)
> > >     
> > > > Regards,
> > > > Bharata.
> > > >     
> > > 
> > > Cheers.
> > >     
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [Qemu-devel] [PATCH v2 4/7] ppc: open code cpu creation for machine types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4607 bytes --]

On Mon, Jul 04, 2016 at 09:37:47AM +0200, Igor Mammedov wrote:
> On Mon, 4 Jul 2016 13:54:55 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Sat, Jul 02, 2016 at 10:33:33AM +0200, Greg Kurz wrote:
> > > On Sat, 2 Jul 2016 13:36:22 +0530
> > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> > > 
> > > > On Sat, Jul 02, 2016 at 12:41:48AM +0200, Greg Kurz wrote:
> > > > > If we want to generate cpu_dt_id in the machine code, this must
> > > > > occur before the cpu gets realized. We must open code the cpu
> > > > > creation to be able to do this.
> > > > > 
> > > > > This patch just does that. It borrows some lines from previous
> > > > > work from Bharata to handle the feature parsing.
> > > > > 
> > > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > > ---
> > > > >  hw/ppc/ppc.c |   39 ++++++++++++++++++++++++++++++++++++++-
> > > > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > > > index dc3d214009c5..57f4ddd073d0 100644
> > > > > --- a/hw/ppc/ppc.c
> > > > > +++ b/hw/ppc/ppc.c
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include "sysemu/cpus.h"
> > > > >  #include "hw/timer/m48t59.h"
> > > > >  #include "qemu/log.h"
> > > > > +#include "qapi/error.h"
> > > > >  #include "qemu/error-report.h"
> > > > >  #include "hw/loader.h"
> > > > >  #include "sysemu/kvm.h"
> > > > > @@ -1353,5 +1354,41 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int
> > > > > cpu_dt_id)
> > > > > 
> > > > >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > > >  {
> > > > > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU,
> > > > > cpu_model));
> > > > > +    PowerPCCPU *cpu;
> > > > > +    CPUClass *cc;
> > > > > +    ObjectClass *oc;
> > > > > +    gchar **model_pieces;
> > > > > +    Error *err = NULL;
> > > > > +
> > > > > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > > > > +    if (!model_pieces[0]) {
> > > > > +        error_report("Invalid/empty CPU model name");
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > > > > +    if (oc == NULL) {
> > > > > +        error_report("Unable to find CPU definition: %s",
> > > > > model_pieces[0]);
> > > > > +        return NULL;
> > > > > +    }
> > > > > +
> > > > > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > > > +
> > > > > +    cc = CPU_CLASS(oc);
> > > > > +    cc->parse_features(CPU(cpu), model_pieces[1], &err);  
> > > > 
> > > > Igor is working on a patchset to convert -cpu features into
> > > > global properties. IIUC, after that patchset, it is not
> > > > recommended to parse the -cpu features for every CPU but do it
> > > > only once.
> > > > 
> > > 
> > > cpu_generic_init() in the current code also does the parsing, and
> > > as the title says, this patch is just about open coding the
> > > creation. I don't want to change behavior yet.
> > > 
> > > But yes, I agree that we should only parse features once and I'll
> > > be more than happy to fix this in a followup patch, based on Igor's
> > > work.
> > > 
> > > In the meantime, maybe I can add a comment stating that the parsing
> > > should go away ?
> > 
> > Right.  But the thing is by open coding here, you're making two copies
> > that need to be fixed instead of one, which increases the chances of
> > error.
> this patch matches what has been done for x86 target as a pert of
> decoupling *-user mode from machine emulation.
> 
> > It seems like it would be safer to change the generic code so there's
> > a new generic function which doesn't do the realize which we can use
> > on ppc (and other platforms when/if they need it).
> We've had it in x86 until recently but I've replaced it
> cpu_generic_init(), please don't go that route.
> 
> There is not much to generalize here so far it's basically following
> code
>    cpu = object_new(cpu-type)
>    parse-features(cpu)
> 
>    set_properties(cpu) /* optional machine specific */
> 
>    cpu->realize()
> 
> once parse-features refactoring is merged, there won't be anything cpu
> specific left as parse-features(cpu) could be moved to generic code
> and only very machine specific set_properties would be left which are
> not generalizable.

Ok.

Greg, belay that suggestion :).

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]