[Qemu-devel] [PATCH 0/7] Better handling of machine specific per-cpu information

[Qemu-devel] [PATCH 0/7] Better handling of machine specific per-cpu information

[David Gibson]

It's moderately common for a machine type to need to keep track of
information that is specific to the platform it implements, but
per-cpu.

While it could keep such information inside the MachineState, this
makes lookup from the CPUState awkward.  So, this series adds a
standard way to stash machine-specific per-cpu information using a
void pointer in the PowerPCCPU object.  The machine is responsible for
alloc()ing, free()ing and (if applicable) migrating this state.

The meat of the series is the last two patches.  The first 5 clean up
a number of minor uglies I encountered while implementing.

David Gibson (7):
  spapr: Clean up cpu realize/unrealize paths
  pnv: Add missing error check during cpu realize()
  pnv_core: Allocate cpu thread objects individually
  pnv: Clean up cpu realize path
  pnv: Add cpu unrealize path
  target/ppc: Replace intc pointer with a general machine_data pointer
  target/ppc, spapr: Move VPA information to machine_data

 hw/intc/xics.c                  |  5 +-
 hw/intc/xics_spapr.c            | 16 ++++--
 hw/ppc/pnv.c                    |  8 +--
 hw/ppc/pnv_core.c               | 96 +++++++++++++++++++--------------
 hw/ppc/spapr.c                  |  8 +--
 hw/ppc/spapr_cpu_core.c         | 85 ++++++++++++++---------------
 hw/ppc/spapr_hcall.c            | 77 ++++++++++++++------------
 include/hw/ppc/pnv_core.h       | 11 +++-
 include/hw/ppc/spapr_cpu_core.h | 13 +++++
 include/hw/ppc/xics.h           |  4 +-
 target/ppc/cpu.h                |  8 +--
 target/ppc/translate_init.inc.c |  8 ---
 12 files changed, 186 insertions(+), 153 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/7] spapr: Clean up cpu realize/unrealize paths

[David Gibson]

spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
cpu core realize/unrealize paths, and really can only be called from there.

Those are all short functions, so fold the pairs together for simplicity.
While we're there rename some functions and change some parameter types
for brevity and clarity.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c | 69 +++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 44 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index f3e9b879b2..7fdb3b6667 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -83,26 +83,6 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
     ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
 }
 
-static void spapr_cpu_destroy(PowerPCCPU *cpu)
-{
-    qemu_unregister_reset(spapr_cpu_reset, cpu);
-}
-
-static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
-                           Error **errp)
-{
-    CPUPPCState *env = &cpu->env;
-
-    /* Set time-base frequency to 512 MHz */
-    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
-
-    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
-    kvmppc_set_papr(cpu);
-
-    qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
-}
-
 /*
  * Return the sPAPR CPU core type for @model which essentially is the CPU
  * model specified with -cpu cmdline option.
@@ -122,44 +102,47 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
     return object_class_get_name(oc);
 }
 
-static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
+static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
+{
+    qemu_unregister_reset(spapr_cpu_reset, cpu);
+    object_unparent(cpu->intc);
+    cpu_remove_sync(CPU(cpu));
+    object_unparent(OBJECT(cpu));
+}
+
+static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
 {
     sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(dev);
     int i;
 
     for (i = 0; i < cc->nr_threads; i++) {
-        Object *obj = OBJECT(sc->threads[i]);
-        DeviceState *dev = DEVICE(obj);
-        CPUState *cs = CPU(dev);
-        PowerPCCPU *cpu = POWERPC_CPU(cs);
-
-        spapr_cpu_destroy(cpu);
-        object_unparent(cpu->intc);
-        cpu_remove_sync(cs);
-        object_unparent(obj);
+        spapr_unrealize_vcpu(sc->threads[i]);
     }
     g_free(sc->threads);
 }
 
-static void spapr_cpu_core_realize_child(Object *child,
-                                         sPAPRMachineState *spapr, Error **errp)
+static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
+                               Error **errp)
 {
+    CPUPPCState *env = &cpu->env;
     Error *local_err = NULL;
-    CPUState *cs = CPU(child);
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
-    spapr_cpu_init(spapr, cpu, &local_err);
-    if (local_err) {
-        goto error;
-    }
+    /* Set time-base frequency to 512 MHz */
+    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
+
+    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
+    kvmppc_set_papr(cpu);
 
-    cpu->intc = icp_create(child, spapr->icp_type, XICS_FABRIC(spapr),
+    qemu_register_reset(spapr_cpu_reset, cpu);
+    spapr_cpu_reset(cpu);
+
+    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
                            &local_err);
     if (local_err) {
         goto error;
@@ -220,9 +203,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        obj = OBJECT(sc->threads[j]);
-
-        spapr_cpu_core_realize_child(obj, spapr, &local_err);
+        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
         if (local_err) {
             goto err;
         }
@@ -249,7 +230,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
     sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
 
     dc->realize = spapr_cpu_core_realize;
-    dc->unrealize = spapr_cpu_core_unrealizefn;
+    dc->unrealize = spapr_cpu_core_unrealize;
     dc->props = spapr_cpu_core_properties;
     scc->cpu_type = data;
 }
-- 
2.17.1

[Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[David Gibson]

In pnv_core_realize() we call two functions with an Error * parameter in
succession, which means if they both cause errors we'll lose the first one.
Add an extra test/escape to fix this.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv_core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 13ad7d9e04..efb68226bb 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
 
         snprintf(name, sizeof(name), "thread[%d]", i);
         object_property_add_child(OBJECT(pc), name, obj, &local_err);
+        if (local_err) {
+            goto err;
+        }
         object_property_add_alias(obj, "core-pir", OBJECT(pc),
                                   "pir", &local_err);
         if (local_err) {
-- 
2.17.1

[Qemu-devel] [PATCH 3/7] pnv_core: Allocate cpu thread objects individually

[David Gibson]

Currently, we allocate space for all the cpu objects within a single core
in one big block.  This was copied from an older version of the spapr code
and requires some ugly pointer manipulation to extract the individual
objects.

This design was due to a misunderstanding of qemu lifetime conventions and
has already been changed in spapr (in 94ad93bd "spapr_cpu_core: instantiate
CPUs separately".

Make an equivalent change in pnv_core to get rid of the nasty pointer
arithmetic.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv.c              |  4 ++--
 hw/ppc/pnv_core.c         | 11 +++++------
 include/hw/ppc/pnv_core.h |  2 +-
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0314881316..0b9508d94d 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -121,9 +121,9 @@ static int get_cpus_node(void *fdt)
  */
 static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
 {
-    CPUState *cs = CPU(DEVICE(pc->threads));
+    PowerPCCPU *cpu = pc->threads[0];
+    CPUState *cs = CPU(cpu);
     DeviceClass *dc = DEVICE_GET_CLASS(cs);
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
     int smt_threads = CPU_CORE(pc)->nr_threads;
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index efb68226bb..59309e149c 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -151,7 +151,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     PnvCore *pc = PNV_CORE(OBJECT(dev));
     CPUCore *cc = CPU_CORE(OBJECT(dev));
     const char *typename = pnv_core_cpu_typename(pc);
-    size_t size = object_type_get_instance_size(typename);
     Error *local_err = NULL;
     void *obj;
     int i, j;
@@ -165,11 +164,11 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    pc->threads = g_malloc0(size * cc->nr_threads);
+    pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
     for (i = 0; i < cc->nr_threads; i++) {
-        obj = pc->threads + i * size;
+        obj = object_new(typename);
 
-        object_initialize(obj, size, typename);
+        pc->threads[i] = POWERPC_CPU(obj);
 
         snprintf(name, sizeof(name), "thread[%d]", i);
         object_property_add_child(OBJECT(pc), name, obj, &local_err);
@@ -185,7 +184,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        obj = pc->threads + j * size;
+        obj = OBJECT(pc->threads[j]);
 
         pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
         if (local_err) {
@@ -200,7 +199,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
 
 err:
     while (--i >= 0) {
-        obj = pc->threads + i * size;
+        obj = OBJECT(pc->threads[i]);
         object_unparent(obj);
     }
     g_free(pc->threads);
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index e337af7a3a..447ae761f7 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -34,7 +34,7 @@ typedef struct PnvCore {
     CPUCore parent_obj;
 
     /*< public >*/
-    void *threads;
+    PowerPCCPU **threads;
     uint32_t pir;
 
     MemoryRegion xscom_regs;
-- 
2.17.1

[Qemu-devel] [PATCH 4/7] pnv: Clean up cpu realize path

[David Gibson]

pnv_cpu_init() is only called from the the pnv cpu core realize path, and
really only can be called from there.  So fold it into its caller, which
we also rename for brevity.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv_core.c | 56 ++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 59309e149c..c9648fd1ad 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -54,28 +54,6 @@ static void pnv_cpu_reset(void *opaque)
     env->msr |= MSR_HVB; /* Hypervisor mode */
 }
 
-static void pnv_cpu_init(PowerPCCPU *cpu, Error **errp)
-{
-    CPUPPCState *env = &cpu->env;
-    int core_pir;
-    int thread_index = 0; /* TODO: TCG supports only one thread */
-    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
-
-    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
-
-    /*
-     * The PIR of a thread is the core PIR + the thread index. We will
-     * need to find a way to get the thread index when TCG supports
-     * more than 1. We could use the object name ?
-     */
-    pir->default_value = core_pir + thread_index;
-
-    /* Set time-base frequency to 512 MHz */
-    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
-
-    qemu_register_reset(pnv_cpu_reset, cpu);
-}
-
 /*
  * These values are read by the PowerNV HW monitors under Linux
  */
@@ -121,29 +99,39 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
+static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
 {
+    CPUPPCState *env = &cpu->env;
+    int core_pir;
+    int thread_index = 0; /* TODO: TCG supports only one thread */
+    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
     Error *local_err = NULL;
-    CPUState *cs = CPU(child);
-    PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-    object_property_set_bool(child, true, "realized", &local_err);
+    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
+    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
     }
 
-    pnv_cpu_init(cpu, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
+    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
+
+    /*
+     * The PIR of a thread is the core PIR + the thread index. We will
+     * need to find a way to get the thread index when TCG supports
+     * more than 1. We could use the object name ?
+     */
+    pir->default_value = core_pir + thread_index;
+
+    /* Set time-base frequency to 512 MHz */
+    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
+
+    qemu_register_reset(pnv_cpu_reset, cpu);
 }
 
 static void pnv_core_realize(DeviceState *dev, Error **errp)
@@ -184,9 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        obj = OBJECT(pc->threads[j]);
-
-        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
+        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
         if (local_err) {
             goto err;
         }
-- 
2.17.1

[Qemu-devel] [PATCH 5/7] pnv: Add cpu unrealize path

[David Gibson]

Currently we don't have any unrealize path for pnv cpu cores.  We get away
with this because we don't yet support cpu hotplug for pnv.

However, we're going to want it eventually, and in the meantime, it makes
it non-obvious why there are a bunch of allocations on the realize() path
that don't have matching frees.

So, implement the missing unrealize path.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/pnv_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index c9648fd1ad..c70dbbe056 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -192,6 +192,26 @@ err:
     error_propagate(errp, local_err);
 }
 
+static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
+{
+    qemu_unregister_reset(pnv_cpu_reset, cpu);
+    object_unparent(cpu->intc);
+    cpu_remove_sync(CPU(cpu));
+    object_unparent(OBJECT(cpu));
+}
+
+static void pnv_core_unrealize(DeviceState *dev, Error **errp)
+{
+    PnvCore *pc = PNV_CORE(dev);
+    CPUCore *cc = CPU_CORE(dev);
+    int i;
+
+    for (i = 0; i < cc->nr_threads; i++) {
+        pnv_unrealize_vcpu(pc->threads[i]);
+    }
+    g_free(pc->threads);
+}
+
 static Property pnv_core_properties[] = {
     DEFINE_PROP_UINT32("pir", PnvCore, pir, 0),
     DEFINE_PROP_END_OF_LIST(),
@@ -202,6 +222,7 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
     DeviceClass *dc = DEVICE_CLASS(oc);
 
     dc->realize = pnv_core_realize;
+    dc->unrealize = pnv_core_unrealize;
     dc->props = pnv_core_properties;
 }
 
-- 
2.17.1

[Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer

[David Gibson]

PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
controller.  Or more precisely to the "presentation" component of the
interrupt controller relevant to this cpu.

Really, this field is machine specific.  The machines which use it can
point it to different types of object depending on their needs, and most
machines don't use it at all (since they have older style PICs which don't
have per-cpu presentation components).

There's also other information that's per-cpu, but platform/machine
specific.  So replace the intc pointer with a (void *)machine_data which
can be managed as the machine type likes to conveniently store per cpu
information.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/intc/xics.c                  |  5 +++--
 hw/intc/xics_spapr.c            | 16 +++++++++++-----
 hw/ppc/pnv.c                    |  4 ++--
 hw/ppc/pnv_core.c               | 11 +++++++++--
 hw/ppc/spapr.c                  |  8 ++++----
 hw/ppc/spapr_cpu_core.c         | 13 ++++++++++---
 include/hw/ppc/pnv_core.h       |  9 +++++++++
 include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
 include/hw/ppc/xics.h           |  4 ++--
 target/ppc/cpu.h                |  2 +-
 10 files changed, 61 insertions(+), 21 deletions(-)

diff --git a/hw/intc/xics.c b/hw/intc/xics.c
index e73e623e3b..689ad44e5f 100644
--- a/hw/intc/xics.c
+++ b/hw/intc/xics.c
@@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
     .class_size = sizeof(ICPStateClass),
 };
 
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
+ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
+                     Error **errp)
 {
     Error *local_err = NULL;
     Object *obj;
@@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
         obj = NULL;
     }
 
-    return obj;
+    return ICP(obj);
 }
 
 /*
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 2e27b92b87..01c76717cf 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -31,6 +31,7 @@
 #include "trace.h"
 #include "qemu/timer.h"
 #include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_cpu_core.h"
 #include "hw/ppc/xics.h"
 #include "hw/ppc/fdt.h"
 #include "qapi/visitor.h"
@@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
     target_ulong cppr = args[0];
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
 
-    icp_set_cppr(ICP(cpu->intc), cppr);
+    icp_set_cppr(spapr_cpu->icp, cppr);
     return H_SUCCESS;
 }
 
@@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                            target_ulong opcode, target_ulong *args)
 {
-    uint32_t xirr = icp_accept(ICP(cpu->intc));
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+    uint32_t xirr = icp_accept(spapr_cpu->icp);
 
     args[0] = xirr;
     return H_SUCCESS;
@@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                              target_ulong opcode, target_ulong *args)
 {
-    uint32_t xirr = icp_accept(ICP(cpu->intc));
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+    uint32_t xirr = icp_accept(spapr_cpu->icp);
 
     args[0] = xirr;
     args[1] = cpu_get_host_ticks();
@@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                           target_ulong opcode, target_ulong *args)
 {
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong xirr = args[0];
 
-    icp_eoi(ICP(cpu->intc), xirr);
+    icp_eoi(spapr_cpu->icp, xirr);
     return H_SUCCESS;
 }
 
@@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
                             target_ulong opcode, target_ulong *args)
 {
     uint32_t mfrr;
-    uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+    uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
 
     args[0] = xirr;
     args[1] = mfrr;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 0b9508d94d..3a36c6ac6a 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
 {
     PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
 
-    return cpu ? ICP(cpu->intc) : NULL;
+    return cpu ? pnv_cpu_state(cpu)->icp : NULL;
 }
 
 static void pnv_pic_print_info(InterruptStatsProvider *obj,
@@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        icp_pic_print_info(ICP(cpu->intc), mon);
+        icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
     }
 
     for (i = 0; i < pnv->num_chips; i++) {
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index c70dbbe056..86448ade87 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
     int core_pir;
     int thread_index = 0; /* TODO: TCG supports only one thread */
     ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
+    PnvCPUState *pnv_cpu;
     Error *local_err = NULL;
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
@@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
         return;
     }
 
-    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
+    cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
+
+    pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -194,8 +197,12 @@ err:
 
 static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
 {
+    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
+
     qemu_unregister_reset(pnv_cpu_reset, cpu);
-    object_unparent(cpu->intc);
+    object_unparent(OBJECT(pnv_cpu->icp));
+    cpu->machine_data = NULL;
+    g_free(pnv_cpu);
     cpu_remove_sync(CPU(cpu));
     object_unparent(OBJECT(cpu));
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index f59999daac..cbab6b6b7e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
     if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
         CPUState *cs;
         CPU_FOREACH(cs) {
-            PowerPCCPU *cpu = POWERPC_CPU(cs);
-            icp_resend(ICP(cpu->intc));
+            sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
+            icp_resend(spapr_cpu->icp);
         }
     }
 
@@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
 {
     PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
 
-    return cpu ? ICP(cpu->intc) : NULL;
+    return cpu ? spapr_cpu_state(cpu)->icp : NULL;
 }
 
 #define ICS_IRQ_FREE(ics, srcno)   \
@@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
     CPU_FOREACH(cs) {
         PowerPCCPU *cpu = POWERPC_CPU(cs);
 
-        icp_pic_print_info(ICP(cpu->intc), mon);
+        icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
     }
 
     ics_pic_print_info(spapr->ics, mon);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 7fdb3b6667..544bda93e2 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
 
 static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
 {
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
     qemu_unregister_reset(spapr_cpu_reset, cpu);
-    object_unparent(cpu->intc);
+    object_unparent(OBJECT(spapr_cpu->icp));
+    cpu->machine_data = NULL;
+    g_free(spapr_cpu);
     cpu_remove_sync(CPU(cpu));
     object_unparent(OBJECT(cpu));
 }
@@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 {
     CPUPPCState *env = &cpu->env;
     Error *local_err = NULL;
+    sPAPRCPUState *spapr_cpu;
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
     if (local_err) {
         goto error;
     }
 
+    spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
+
     /* Set time-base frequency to 512 MHz */
     cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
 
@@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     qemu_register_reset(spapr_cpu_reset, cpu);
     spapr_cpu_reset(cpu);
 
-    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
-                           &local_err);
+    spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
+                                XICS_FABRIC(spapr), &local_err);
     if (local_err) {
         goto error;
     }
diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
index 447ae761f7..f81dff28aa 100644
--- a/include/hw/ppc/pnv_core.h
+++ b/include/hw/ppc/pnv_core.h
@@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
 #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
 #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
 
+typedef struct PnvCPUState {
+    ICPState *icp;
+} PnvCPUState;
+
+static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
+{
+    return cpu->machine_data;
+}
+
 #endif /* _PPC_PNV_CORE_H */
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 47dcfda12b..e3d2aa45a4 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
 const char *spapr_get_cpu_core_type(const char *cpu_type);
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
 
+typedef struct ICPState ICPState;
+typedef struct sPAPRCPUState {
+    ICPState *icp;
+} sPAPRCPUState;
+
+static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
+{
+    return (sPAPRCPUState *)cpu->machine_data;
+}
+
 #endif
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 6cebff47a7..48930d91e5 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
 int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
 void xics_spapr_init(sPAPRMachineState *spapr);
 
-Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
-                   Error **errp);
+ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
+                     Error **errp);
 
 #endif /* XICS_H */
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index a91f1a8777..abf0bf0224 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1204,7 +1204,7 @@ struct PowerPCCPU {
     int vcpu_id;
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
-    Object *intc;
+    void *machine_data;
     int32_t node_id; /* NUMA node this CPU belongs to */
     PPCHash64Options *hash64_opts;
 
-- 
2.17.1

[Qemu-devel] [PATCH 7/7] target/ppc, spapr: Move VPA information to machine_data

[David Gibson]

CPUPPCState currently contains a number of fields containing the state of
the VPA.  The VPA is a PAPR specific concept covering several guest/host
shared memory areas used to communicate some information with the
hypervisor.

As a PAPR concept this is really machine specific information, although it
is per-cpu, so it doesn't really belong in the core CPU state structure.
So, move it to the PAPR specific 'machine_data' structure.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_cpu_core.c         |  7 +++
 hw/ppc/spapr_hcall.c            | 77 ++++++++++++++++++---------------
 include/hw/ppc/spapr_cpu_core.h |  3 ++
 target/ppc/cpu.h                |  6 ---
 target/ppc/translate_init.inc.c |  8 ----
 5 files changed, 52 insertions(+), 49 deletions(-)

diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 544bda93e2..f642c95967 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong lpcr;
 
     cpu_reset(cs);
@@ -69,6 +70,12 @@ static void spapr_cpu_reset(void *opaque)
 
     /* Set a full AMOR so guest can use the AMR as it sees fit */
     env->spr[SPR_AMOR] = 0xffffffffffffffffull;
+
+    spapr_cpu->vpa_addr = 0;
+    spapr_cpu->slb_shadow_addr = 0;
+    spapr_cpu->slb_shadow_size = 0;
+    spapr_cpu->dtl_addr = 0;
+    spapr_cpu->dtl_size = 0;
 }
 
 void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 8b9a4b577f..ae913d070f 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -8,6 +8,7 @@
 #include "exec/exec-all.h"
 #include "helper_regs.h"
 #include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_cpu_core.h"
 #include "mmu-hash64.h"
 #include "cpu-models.h"
 #include "trace.h"
@@ -908,9 +909,11 @@ unmap_out:
 #define VPA_SHARED_PROC_OFFSET 0x9
 #define VPA_SHARED_PROC_VAL    0x2
 
-static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
+static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
 {
-    CPUState *cs = CPU(ppc_env_get_cpu(env));
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
     uint16_t size;
     uint8_t tmp;
 
@@ -935,32 +938,34 @@ static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
         return H_PARAMETER;
     }
 
-    env->vpa_addr = vpa;
+    spapr_cpu->vpa_addr = vpa;
 
-    tmp = ldub_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET);
+    tmp = ldub_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET);
     tmp |= VPA_SHARED_PROC_VAL;
-    stb_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
+    stb_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
 
     return H_SUCCESS;
 }
 
-static target_ulong deregister_vpa(CPUPPCState *env, target_ulong vpa)
+static target_ulong deregister_vpa(PowerPCCPU *cpu, target_ulong vpa)
 {
-    if (env->slb_shadow_addr) {
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+    if (spapr_cpu->slb_shadow_addr) {
         return H_RESOURCE;
     }
 
-    if (env->dtl_addr) {
+    if (spapr_cpu->dtl_addr) {
         return H_RESOURCE;
     }
 
-    env->vpa_addr = 0;
+    spapr_cpu->vpa_addr = 0;
     return H_SUCCESS;
 }
 
-static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
+static target_ulong register_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
 {
-    CPUState *cs = CPU(ppc_env_get_cpu(env));
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
     uint32_t size;
 
     if (addr == 0) {
@@ -968,7 +973,7 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
         return H_HARDWARE;
     }
 
-    size = ldl_be_phys(cs->as, addr + 0x4);
+    size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
     if (size < 0x8) {
         return H_PARAMETER;
     }
@@ -977,26 +982,28 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
         return H_PARAMETER;
     }
 
-    if (!env->vpa_addr) {
+    if (!spapr_cpu->vpa_addr) {
         return H_RESOURCE;
     }
 
-    env->slb_shadow_addr = addr;
-    env->slb_shadow_size = size;
+    spapr_cpu->slb_shadow_addr = addr;
+    spapr_cpu->slb_shadow_size = size;
 
     return H_SUCCESS;
 }
 
-static target_ulong deregister_slb_shadow(CPUPPCState *env, target_ulong addr)
+static target_ulong deregister_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
 {
-    env->slb_shadow_addr = 0;
-    env->slb_shadow_size = 0;
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+    spapr_cpu->slb_shadow_addr = 0;
+    spapr_cpu->slb_shadow_size = 0;
     return H_SUCCESS;
 }
 
-static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
+static target_ulong register_dtl(PowerPCCPU *cpu, target_ulong addr)
 {
-    CPUState *cs = CPU(ppc_env_get_cpu(env));
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
     uint32_t size;
 
     if (addr == 0) {
@@ -1004,26 +1011,28 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
         return H_HARDWARE;
     }
 
-    size = ldl_be_phys(cs->as, addr + 0x4);
+    size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
 
     if (size < 48) {
         return H_PARAMETER;
     }
 
-    if (!env->vpa_addr) {
+    if (!spapr_cpu->vpa_addr) {
         return H_RESOURCE;
     }
 
-    env->dtl_addr = addr;
-    env->dtl_size = size;
+    spapr_cpu->dtl_addr = addr;
+    spapr_cpu->dtl_size = size;
 
     return H_SUCCESS;
 }
 
-static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
+static target_ulong deregister_dtl(PowerPCCPU *cpu, target_ulong addr)
 {
-    env->dtl_addr = 0;
-    env->dtl_size = 0;
+    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
+
+    spapr_cpu->dtl_addr = 0;
+    spapr_cpu->dtl_size = 0;
 
     return H_SUCCESS;
 }
@@ -1035,38 +1044,36 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     target_ulong procno = args[1];
     target_ulong vpa = args[2];
     target_ulong ret = H_PARAMETER;
-    CPUPPCState *tenv;
     PowerPCCPU *tcpu;
 
     tcpu = spapr_find_cpu(procno);
     if (!tcpu) {
         return H_PARAMETER;
     }
-    tenv = &tcpu->env;
 
     switch (flags) {
     case FLAGS_REGISTER_VPA:
-        ret = register_vpa(tenv, vpa);
+        ret = register_vpa(tcpu, vpa);
         break;
 
     case FLAGS_DEREGISTER_VPA:
-        ret = deregister_vpa(tenv, vpa);
+        ret = deregister_vpa(tcpu, vpa);
         break;
 
     case FLAGS_REGISTER_SLBSHADOW:
-        ret = register_slb_shadow(tenv, vpa);
+        ret = register_slb_shadow(tcpu, vpa);
         break;
 
     case FLAGS_DEREGISTER_SLBSHADOW:
-        ret = deregister_slb_shadow(tenv, vpa);
+        ret = deregister_slb_shadow(tcpu, vpa);
         break;
 
     case FLAGS_REGISTER_DTL:
-        ret = register_dtl(tenv, vpa);
+        ret = register_dtl(tcpu, vpa);
         break;
 
     case FLAGS_DEREGISTER_DTL:
-        ret = deregister_dtl(tenv, vpa);
+        ret = deregister_dtl(tcpu, vpa);
         break;
     }
 
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index e3d2aa45a4..40129cc452 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -44,6 +44,9 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
 typedef struct ICPState ICPState;
 typedef struct sPAPRCPUState {
     ICPState *icp;
+    uint64_t vpa_addr;
+    uint64_t slb_shadow_addr, slb_shadow_size;
+    uint64_t dtl_addr, dtl_size;
 } sPAPRCPUState;
 
 static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index abf0bf0224..6c2f4d29f2 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1091,12 +1091,6 @@ struct CPUPPCState {
     target_ulong rmls;
 #endif
 
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
-    uint64_t vpa_addr;
-    uint64_t slb_shadow_addr, slb_shadow_size;
-    uint64_t dtl_addr, dtl_size;
-#endif /* TARGET_PPC64 */
-
     int error_code;
     uint32_t pending_interrupts;
 #if !defined(CONFIG_USER_ONLY)
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index bb9296f5a3..76d6f3fd5e 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -10316,14 +10316,6 @@ static void ppc_cpu_reset(CPUState *s)
     s->exception_index = POWERPC_EXCP_NONE;
     env->error_code = 0;
 
-#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
-    env->vpa_addr = 0;
-    env->slb_shadow_addr = 0;
-    env->slb_shadow_size = 0;
-    env->dtl_addr = 0;
-    env->dtl_size = 0;
-#endif /* TARGET_PPC64 */
-
     for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
         ppc_spr_t *spr = &env->spr_cb[i];
 
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/7] spapr: Clean up cpu realize/unrealize paths

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
> cpu core realize/unrealize paths, and really can only be called from there.
> 
> Those are all short functions, so fold the pairs together for simplicity.
> While we're there rename some functions and change some parameter types
> for brevity and clarity.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Still a call to spapr_cpu_reset(cpu). We should try to get rid of it
one day.
 
Thanks,

C.

> ---
>  hw/ppc/spapr_cpu_core.c | 69 +++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index f3e9b879b2..7fdb3b6667 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -83,26 +83,6 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
>      ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  }
>  
> -static void spapr_cpu_destroy(PowerPCCPU *cpu)
> -{
> -    qemu_unregister_reset(spapr_cpu_reset, cpu);
> -}
> -
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -
> -    /* Set time-base frequency to 512 MHz */
> -    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> -
> -    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> -    kvmppc_set_papr(cpu);
> -
> -    qemu_register_reset(spapr_cpu_reset, cpu);
> -    spapr_cpu_reset(cpu);
> -}
> -
>  /*
>   * Return the sPAPR CPU core type for @model which essentially is the CPU
>   * model specified with -cpu cmdline option.
> @@ -122,44 +102,47 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>      return object_class_get_name(oc);
>  }
>  
> -static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> +static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> +{
> +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> +    object_unparent(cpu->intc);
> +    cpu_remove_sync(CPU(cpu));
> +    object_unparent(OBJECT(cpu));
> +}
> +
> +static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(dev);
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        Object *obj = OBJECT(sc->threads[i]);
> -        DeviceState *dev = DEVICE(obj);
> -        CPUState *cs = CPU(dev);
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> -        spapr_cpu_destroy(cpu);
> -        object_unparent(cpu->intc);
> -        cpu_remove_sync(cs);
> -        object_unparent(obj);
> +        spapr_unrealize_vcpu(sc->threads[i]);
>      }
>      g_free(sc->threads);
>  }
>  
> -static void spapr_cpu_core_realize_child(Object *child,
> -                                         sPAPRMachineState *spapr, Error **errp)
> +static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                               Error **errp)
>  {
> +    CPUPPCState *env = &cpu->env;
>      Error *local_err = NULL;
> -    CPUState *cs = CPU(child);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> -    if (local_err) {
> -        goto error;
> -    }
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> +
> +    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    kvmppc_set_papr(cpu);
>  
> -    cpu->intc = icp_create(child, spapr->icp_type, XICS_FABRIC(spapr),
> +    qemu_register_reset(spapr_cpu_reset, cpu);
> +    spapr_cpu_reset(cpu);
> +
> +    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
>                             &local_err);
>      if (local_err) {
>          goto error;
> @@ -220,9 +203,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = OBJECT(sc->threads[j]);
> -
> -        spapr_cpu_core_realize_child(obj, spapr, &local_err);
> +        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
>          if (local_err) {
>              goto err;
>          }
> @@ -249,7 +230,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
>  
>      dc->realize = spapr_cpu_core_realize;
> -    dc->unrealize = spapr_cpu_core_unrealizefn;
> +    dc->unrealize = spapr_cpu_core_unrealize;
>      dc->props = spapr_cpu_core_properties;
>      scc->cpu_type = data;
>  }
> 

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> In pnv_core_realize() we call two functions with an Error * parameter in
> succession, which means if they both cause errors we'll lose the first one.
> Add an extra test/escape to fix this.

I tend now to pass just NULL or &error_abort to object_property_add_child() 
and object_property_add_const_link(). These calls should just not fail.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C. 
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 13ad7d9e04..efb68226bb 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>  
>          snprintf(name, sizeof(name), "thread[%d]", i);
>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
>                                    "pir", &local_err);
>          if (local_err) {
> 

Re: [Qemu-devel] [PATCH 3/7] pnv_core: Allocate cpu thread objects individually

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> Currently, we allocate space for all the cpu objects within a single core
> in one big block.  This was copied from an older version of the spapr code
> and requires some ugly pointer manipulation to extract the individual
> objects.
> 
> This design was due to a misunderstanding of qemu lifetime conventions and
> has already been changed in spapr (in 94ad93bd "spapr_cpu_core: instantiate
> CPUs separately".
> 
> Make an equivalent change in pnv_core to get rid of the nasty pointer
> arithmetic.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Ah nice cleanup :)

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/ppc/pnv.c              |  4 ++--
>  hw/ppc/pnv_core.c         | 11 +++++------
>  include/hw/ppc/pnv_core.h |  2 +-
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0314881316..0b9508d94d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -121,9 +121,9 @@ static int get_cpus_node(void *fdt)
>   */
>  static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
>  {
> -    CPUState *cs = CPU(DEVICE(pc->threads));
> +    PowerPCCPU *cpu = pc->threads[0];
> +    CPUState *cs = CPU(cpu);
>      DeviceClass *dc = DEVICE_GET_CLASS(cs);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>      int smt_threads = CPU_CORE(pc)->nr_threads;
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index efb68226bb..59309e149c 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -151,7 +151,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      PnvCore *pc = PNV_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      const char *typename = pnv_core_cpu_typename(pc);
> -    size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
>      void *obj;
>      int i, j;
> @@ -165,11 +164,11 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    pc->threads = g_malloc0(size * cc->nr_threads);
> +    pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        obj = pc->threads + i * size;
> +        obj = object_new(typename);
>  
> -        object_initialize(obj, size, typename);
> +        pc->threads[i] = POWERPC_CPU(obj);
>  
>          snprintf(name, sizeof(name), "thread[%d]", i);
>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> @@ -185,7 +184,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = pc->threads + j * size;
> +        obj = OBJECT(pc->threads[j]);
>  
>          pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
>          if (local_err) {
> @@ -200,7 +199,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>  
>  err:
>      while (--i >= 0) {
> -        obj = pc->threads + i * size;
> +        obj = OBJECT(pc->threads[i]);
>          object_unparent(obj);
>      }
>      g_free(pc->threads);
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index e337af7a3a..447ae761f7 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -34,7 +34,7 @@ typedef struct PnvCore {
>      CPUCore parent_obj;
>  
>      /*< public >*/
> -    void *threads;
> +    PowerPCCPU **threads;
>      uint32_t pir;
>  
>      MemoryRegion xscom_regs;
> 

Re: [Qemu-devel] [PATCH 4/7] pnv: Clean up cpu realize path

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> pnv_cpu_init() is only called from the the pnv cpu core realize path, and
> really only can be called from there.  So fold it into its caller, which
> we also rename for brevity.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

I think we should set the default CPU settings (PIR) before creating
the 'intc' object. I have cleanup for that in the pnv patchset. 
Nevertheless, 

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>  hw/ppc/pnv_core.c | 56 ++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 59309e149c..c9648fd1ad 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -54,28 +54,6 @@ static void pnv_cpu_reset(void *opaque)
>      env->msr |= MSR_HVB; /* Hypervisor mode */
>  }
>  
> -static void pnv_cpu_init(PowerPCCPU *cpu, Error **errp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    int core_pir;
> -    int thread_index = 0; /* TODO: TCG supports only one thread */
> -    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> -
> -    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> -
> -    /*
> -     * The PIR of a thread is the core PIR + the thread index. We will
> -     * need to find a way to get the thread index when TCG supports
> -     * more than 1. We could use the object name ?
> -     */
> -    pir->default_value = core_pir + thread_index;
> -
> -    /* Set time-base frequency to 512 MHz */
> -    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> -
> -    qemu_register_reset(pnv_cpu_reset, cpu);
> -}
> -
>  /*
>   * These values are read by the PowerNV HW monitors under Linux
>   */
> @@ -121,29 +99,39 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> +static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>  {
> +    CPUPPCState *env = &cpu->env;
> +    int core_pir;
> +    int thread_index = 0; /* TODO: TCG supports only one thread */
> +    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
>      Error *local_err = NULL;
> -    CPUState *cs = CPU(child);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
> +    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    pnv_cpu_init(cpu, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> +
> +    /*
> +     * The PIR of a thread is the core PIR + the thread index. We will
> +     * need to find a way to get the thread index when TCG supports
> +     * more than 1. We could use the object name ?
> +     */
> +    pir->default_value = core_pir + thread_index;
> +
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> +
> +    qemu_register_reset(pnv_cpu_reset, cpu);
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> @@ -184,9 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = OBJECT(pc->threads[j]);
> -
> -        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
>          if (local_err) {
>              goto err;
>          }
> 

Re: [Qemu-devel] [PATCH 5/7] pnv: Add cpu unrealize path

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> Currently we don't have any unrealize path for pnv cpu cores.  We get away
> with this because we don't yet support cpu hotplug for pnv.
> 
> However, we're going to want it eventually, and in the meantime, it makes
> it non-obvious why there are a bunch of allocations on the realize() path
> that don't have matching frees.
> 
> So, implement the missing unrealize path.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>  hw/ppc/pnv_core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index c9648fd1ad..c70dbbe056 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -192,6 +192,26 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> +static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> +{
> +    qemu_unregister_reset(pnv_cpu_reset, cpu);
> +    object_unparent(cpu->intc);
> +    cpu_remove_sync(CPU(cpu));
> +    object_unparent(OBJECT(cpu));
> +}
> +
> +static void pnv_core_unrealize(DeviceState *dev, Error **errp)
> +{
> +    PnvCore *pc = PNV_CORE(dev);
> +    CPUCore *cc = CPU_CORE(dev);
> +    int i;
> +
> +    for (i = 0; i < cc->nr_threads; i++) {
> +        pnv_unrealize_vcpu(pc->threads[i]);
> +    }
> +    g_free(pc->threads);
> +}
> +
>  static Property pnv_core_properties[] = {
>      DEFINE_PROP_UINT32("pir", PnvCore, pir, 0),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -202,6 +222,7 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      dc->realize = pnv_core_realize;
> +    dc->unrealize = pnv_core_unrealize;
>      dc->props = pnv_core_properties;
>  }
>  
> 

Re: [Qemu-devel] [PATCH 1/7] spapr: Clean up cpu realize/unrealize paths

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
> cpu core realize/unrealize paths, and really can only be called from there.
> 
> Those are all short functions, so fold the pairs together for simplicity.
> While we're there rename some functions and change some parameter types
> for brevity and clarity.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c | 69 +++++++++++++++--------------------------
>  1 file changed, 25 insertions(+), 44 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index f3e9b879b2..7fdb3b6667 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -83,26 +83,6 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
>      ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
>  }
>  
> -static void spapr_cpu_destroy(PowerPCCPU *cpu)
> -{
> -    qemu_unregister_reset(spapr_cpu_reset, cpu);
> -}
> -
> -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> -                           Error **errp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -
> -    /* Set time-base frequency to 512 MHz */
> -    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> -
> -    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> -    kvmppc_set_papr(cpu);
> -
> -    qemu_register_reset(spapr_cpu_reset, cpu);
> -    spapr_cpu_reset(cpu);
> -}
> -
>  /*
>   * Return the sPAPR CPU core type for @model which essentially is the CPU
>   * model specified with -cpu cmdline option.
> @@ -122,44 +102,47 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>      return object_class_get_name(oc);
>  }
>  
> -static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> +static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> +{
> +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> +    object_unparent(cpu->intc);
> +    cpu_remove_sync(CPU(cpu));
> +    object_unparent(OBJECT(cpu));
> +}
> +
> +static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
>  {
>      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(dev);
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> -        Object *obj = OBJECT(sc->threads[i]);
> -        DeviceState *dev = DEVICE(obj);
> -        CPUState *cs = CPU(dev);
> -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> -
> -        spapr_cpu_destroy(cpu);
> -        object_unparent(cpu->intc);
> -        cpu_remove_sync(cs);
> -        object_unparent(obj);
> +        spapr_unrealize_vcpu(sc->threads[i]);
>      }
>      g_free(sc->threads);
>  }
>  
> -static void spapr_cpu_core_realize_child(Object *child,
> -                                         sPAPRMachineState *spapr, Error **errp)
> +static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> +                               Error **errp)
>  {
> +    CPUPPCState *env = &cpu->env;
>      Error *local_err = NULL;
> -    CPUState *cs = CPU(child);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> -    spapr_cpu_init(spapr, cpu, &local_err);
> -    if (local_err) {
> -        goto error;
> -    }
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> +
> +    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> +    kvmppc_set_papr(cpu);
>  
> -    cpu->intc = icp_create(child, spapr->icp_type, XICS_FABRIC(spapr),
> +    qemu_register_reset(spapr_cpu_reset, cpu);
> +    spapr_cpu_reset(cpu);
> +
> +    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
>                             &local_err);
>      if (local_err) {
>          goto error;
> @@ -220,9 +203,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = OBJECT(sc->threads[j]);
> -
> -        spapr_cpu_core_realize_child(obj, spapr, &local_err);
> +        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
>          if (local_err) {
>              goto err;
>          }
> @@ -249,7 +230,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
>  
>      dc->realize = spapr_cpu_core_realize;
> -    dc->unrealize = spapr_cpu_core_unrealizefn;
> +    dc->unrealize = spapr_cpu_core_unrealize;
>      dc->props = spapr_cpu_core_properties;
>      scc->cpu_type = data;
>  }

Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer

[Cédric Le Goater]

On 06/13/2018 08:57 AM, David Gibson wrote:
> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller. Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.

yes and that made sense in terms of modeling because you actually have a 
set of wires between the presenter and the cores of a system.

> Really, this field is machine specific.  The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
> 
> There's also other information that's per-cpu, but platform/machine
> specific.  So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.

ah. so you have something else the store in the machine_data. 

If you were defining a type, we would have some more checks when 
casting the machine_data field. We also could parent the object 
to the CPU also. This is minor.


The change should be compatible with the XIVE change which need 
to allocate a different type of presenter. So, sPAPRCPUState and 
PnvCPUState would look like :

	typedef struct sPAPRCPUState {
	    ICPState *icp;
	    XiveTCTX *tctx;
	} sPAPRCPUState;

and the call to ipc_create() will move in an operation of the 
sPAPR IRQ backend, if that exists oneday, and in an operation of 
the PnvChip to handle the differences in the interrupt controller
in use by the machine. 

So no big difference, but the cpu machine_data won't be populated
from the core but from the machine. I hope this is compatible
with the next changes.

Thanks,

C.

> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/intc/xics.c                  |  5 +++--
>  hw/intc/xics_spapr.c            | 16 +++++++++++-----
>  hw/ppc/pnv.c                    |  4 ++--
>  hw/ppc/pnv_core.c               | 11 +++++++++--
>  hw/ppc/spapr.c                  |  8 ++++----
>  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++---
>  include/hw/ppc/pnv_core.h       |  9 +++++++++
>  include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
>  include/hw/ppc/xics.h           |  4 ++--
>  target/ppc/cpu.h                |  2 +-
>  10 files changed, 61 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
>      .class_size = sizeof(ICPStateClass),
>  };
>  
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> +                     Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
>          obj = NULL;
>      }
>  
> -    return obj;
> +    return ICP(obj);
>  }
>  
>  /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
>  #include "trace.h"
>  #include "qemu/timer.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/fdt.h"
>  #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
>      target_ulong cppr = args[0];
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>  
> -    icp_set_cppr(ICP(cpu->intc), cppr);
> +    icp_set_cppr(spapr_cpu->icp, cppr);
>      return H_SUCCESS;
>  }
>  
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_accept(spapr_cpu->icp);
>  
>      args[0] = xirr;
>      return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                               target_ulong opcode, target_ulong *args)
>  {
> -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_accept(spapr_cpu->icp);
>  
>      args[0] = xirr;
>      args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong xirr = args[0];
>  
> -    icp_eoi(ICP(cpu->intc), xirr);
> +    icp_eoi(spapr_cpu->icp, xirr);
>      return H_SUCCESS;
>  }
>  
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
>      uint32_t mfrr;
> -    uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
>  
>      args[0] = xirr;
>      args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
>  {
>      PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>  
> -    return cpu ? ICP(cpu->intc) : NULL;
> +    return cpu ? pnv_cpu_state(cpu)->icp : NULL;
>  }
>  
>  static void pnv_pic_print_info(InterruptStatsProvider *obj,
> @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        icp_pic_print_info(ICP(cpu->intc), mon);
> +        icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
>      }
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index c70dbbe056..86448ade87 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>      int core_pir;
>      int thread_index = 0; /* TODO: TCG supports only one thread */
>      ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> +    PnvCPUState *pnv_cpu;
>      Error *local_err = NULL;
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>          return;
>      }
>  
> -    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> +    cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> +
> +    pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -194,8 +197,12 @@ err:
>  
>  static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
>  {
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
>      qemu_unregister_reset(pnv_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    object_unparent(OBJECT(pnv_cpu->icp));
> +    cpu->machine_data = NULL;
> +    g_free(pnv_cpu);
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..cbab6b6b7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
>          CPUState *cs;
>          CPU_FOREACH(cs) {
> -            PowerPCCPU *cpu = POWERPC_CPU(cs);
> -            icp_resend(ICP(cpu->intc));
> +            sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> +            icp_resend(spapr_cpu->icp);
>          }
>      }
>  
> @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
>  {
>      PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
>  
> -    return cpu ? ICP(cpu->intc) : NULL;
> +    return cpu ? spapr_cpu_state(cpu)->icp : NULL;
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        icp_pic_print_info(ICP(cpu->intc), mon);
> +        icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
>      }
>  
>      ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7fdb3b6667..544bda93e2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>  
>  static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
>  {
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    object_unparent(OBJECT(spapr_cpu->icp));
> +    cpu->machine_data = NULL;
> +    g_free(spapr_cpu);
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  {
>      CPUPPCState *env = &cpu->env;
>      Error *local_err = NULL;
> +    sPAPRCPUState *spapr_cpu;
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> +    spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> +
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>  
> @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
>  
> -    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> -                           &local_err);
> +    spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> +                                XICS_FABRIC(spapr), &local_err);
>      if (local_err) {
>          goto error;
>      }
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 447ae761f7..f81dff28aa 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
>  #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
>  #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
>  
> +typedef struct PnvCPUState {
> +    ICPState *icp;
> +} PnvCPUState;
> +
> +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> +{
> +    return cpu->machine_data;
> +}
> +
>  #endif /* _PPC_PNV_CORE_H */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 47dcfda12b..e3d2aa45a4 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
>  const char *spapr_get_cpu_core_type(const char *cpu_type);
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
>  
> +typedef struct ICPState ICPState;
> +typedef struct sPAPRCPUState {
> +    ICPState *icp;
> +} sPAPRCPUState;
> +
> +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> +{
> +    return (sPAPRCPUState *)cpu->machine_data;
> +}
> +
>  #endif
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7..48930d91e5 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
>  void xics_spapr_init(sPAPRMachineState *spapr);
>  
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> -                   Error **errp);
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> +                     Error **errp);
>  
>  #endif /* XICS_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777..abf0bf0224 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    Object *intc;
> +    void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
>  
> 

Re: [Qemu-devel] [PATCH 1/7] spapr: Clean up cpu realize/unrealize paths

[Greg Kurz]

On Wed, 13 Jun 2018 10:11:45 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> On 06/13/2018 08:57 AM, David Gibson wrote:
> > spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
> > cpu core realize/unrealize paths, and really can only be called from there.
> > 
> > Those are all short functions, so fold the pairs together for simplicity.
> > While we're there rename some functions and change some parameter types
> > for brevity and clarity.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>  
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Still a call to spapr_cpu_reset(cpu). We should try to get rid of it
> one day.
>  

Yeah, CPU reset should ideally be triggered during machine reset or during
hotplug. There have been several tries to do so but we hit an issue with
hotplug... If we don't call spapr_cpu_reset() here then CPUState::halted
isn't set and the CPU can start executing before the hotplug path has a
chance to reset it...

https://lists.nongnu.org/archive/html/qemu-ppc/2018-04/msg00126.html

This requires a deeper investigation.

> Thanks,
> 
> C.
> 
> > ---
> >  hw/ppc/spapr_cpu_core.c | 69 +++++++++++++++--------------------------
> >  1 file changed, 25 insertions(+), 44 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index f3e9b879b2..7fdb3b6667 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -83,26 +83,6 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
> >      ppc_store_lpcr(cpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> >  }
> >  
> > -static void spapr_cpu_destroy(PowerPCCPU *cpu)
> > -{
> > -    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > -}
> > -
> > -static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > -                           Error **errp)
> > -{
> > -    CPUPPCState *env = &cpu->env;
> > -
> > -    /* Set time-base frequency to 512 MHz */
> > -    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> > -
> > -    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> > -    kvmppc_set_papr(cpu);
> > -
> > -    qemu_register_reset(spapr_cpu_reset, cpu);
> > -    spapr_cpu_reset(cpu);
> > -}
> > -
> >  /*
> >   * Return the sPAPR CPU core type for @model which essentially is the CPU
> >   * model specified with -cpu cmdline option.
> > @@ -122,44 +102,47 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
> >      return object_class_get_name(oc);
> >  }
> >  
> > -static void spapr_cpu_core_unrealizefn(DeviceState *dev, Error **errp)
> > +static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> > +{
> > +    qemu_unregister_reset(spapr_cpu_reset, cpu);
> > +    object_unparent(cpu->intc);
> > +    cpu_remove_sync(CPU(cpu));
> > +    object_unparent(OBJECT(cpu));
> > +}
> > +
> > +static void spapr_cpu_core_unrealize(DeviceState *dev, Error **errp)
> >  {
> >      sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> >      CPUCore *cc = CPU_CORE(dev);
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > -        Object *obj = OBJECT(sc->threads[i]);
> > -        DeviceState *dev = DEVICE(obj);
> > -        CPUState *cs = CPU(dev);
> > -        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -
> > -        spapr_cpu_destroy(cpu);
> > -        object_unparent(cpu->intc);
> > -        cpu_remove_sync(cs);
> > -        object_unparent(obj);
> > +        spapr_unrealize_vcpu(sc->threads[i]);
> >      }
> >      g_free(sc->threads);
> >  }
> >  
> > -static void spapr_cpu_core_realize_child(Object *child,
> > -                                         sPAPRMachineState *spapr, Error **errp)
> > +static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> > +                               Error **errp)
> >  {
> > +    CPUPPCState *env = &cpu->env;
> >      Error *local_err = NULL;
> > -    CPUState *cs = CPU(child);
> > -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > -    spapr_cpu_init(spapr, cpu, &local_err);
> > -    if (local_err) {
> > -        goto error;
> > -    }
> > +    /* Set time-base frequency to 512 MHz */
> > +    cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> > +
> > +    cpu_ppc_set_vhyp(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> > +    kvmppc_set_papr(cpu);
> >  
> > -    cpu->intc = icp_create(child, spapr->icp_type, XICS_FABRIC(spapr),
> > +    qemu_register_reset(spapr_cpu_reset, cpu);
> > +    spapr_cpu_reset(cpu);
> > +
> > +    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> >                             &local_err);
> >      if (local_err) {
> >          goto error;
> > @@ -220,9 +203,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        obj = OBJECT(sc->threads[j]);
> > -
> > -        spapr_cpu_core_realize_child(obj, spapr, &local_err);
> > +        spapr_realize_vcpu(sc->threads[j], spapr, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> > @@ -249,7 +230,7 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> >      sPAPRCPUCoreClass *scc = SPAPR_CPU_CORE_CLASS(oc);
> >  
> >      dc->realize = spapr_cpu_core_realize;
> > -    dc->unrealize = spapr_cpu_core_unrealizefn;
> > +    dc->unrealize = spapr_cpu_core_unrealize;
> >      dc->props = spapr_cpu_core_properties;
> >      scc->cpu_type = data;
> >  }
> >   
> 

Re: [Qemu-devel] [PATCH 1/7] spapr: Clean up cpu realize/unrealize paths

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:11:45AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 08:57 AM, David Gibson wrote:
> > spapr_cpu_init() and spapr_cpu_destroy() are only called from the spapr
> > cpu core realize/unrealize paths, and really can only be called from there.
> > 
> > Those are all short functions, so fold the pairs together for simplicity.
> > While we're there rename some functions and change some parameter types
> > for brevity and clarity.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Still a call to spapr_cpu_reset(cpu). We should try to get rid of it
> one day.

Yeah, I know.  I'm wrestling with that along with some of the pagesize
stuff.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> In pnv_core_realize() we call two functions with an Error * parameter in
> succession, which means if they both cause errors we'll lose the first one.

Not exactly. The error code doesn't allow that and QEMU will abort.

static void error_setv(Error **errp,
                       const char *src, int line, const char *func,
                       ErrorClass err_class, const char *fmt, va_list ap,
                       const char *suffix)
{
    Error *err;
    int saved_errno = errno;

    if (errp == NULL) {
        return;
    }
    assert(*errp == NULL);


> Add an extra test/escape to fix this.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/pnv_core.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 13ad7d9e04..efb68226bb 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>  
>          snprintf(name, sizeof(name), "thread[%d]", i);
>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> +        if (local_err) {
> +            goto err;
> +        }
>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
>                                    "pir", &local_err);
>          if (local_err) {

Hmm... the current error path seems to assume failures to be
caused by object_property_add_child(). It hence unparents the
previously parented CPUs, but not the current one. So we'll
miss one call to object_unparent() if object_property_add_alias()
fails.

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:15:09AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 08:57 AM, David Gibson wrote:
> > In pnv_core_realize() we call two functions with an Error * parameter in
> > succession, which means if they both cause errors we'll lose the first one.
> > Add an extra test/escape to fix this.
> 
> I tend now to pass just NULL or &error_abort to object_property_add_child() 
> and object_property_add_const_link(). These calls should just not
> fail.

Hm, good point. Another day.

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C. 
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/pnv_core.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 13ad7d9e04..efb68226bb 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >  
> >          snprintf(name, sizeof(name), "thread[%d]", i);
> >          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> > +        if (local_err) {
> > +            goto err;
> > +        }
> >          object_property_add_alias(obj, "core-pir", OBJECT(pc),
> >                                    "pir", &local_err);
> >          if (local_err) {
> > 
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/7] pnv_core: Allocate cpu thread objects individually

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:03 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently, we allocate space for all the cpu objects within a single core
> in one big block.  This was copied from an older version of the spapr code
> and requires some ugly pointer manipulation to extract the individual
> objects.
> 
> This design was due to a misunderstanding of qemu lifetime conventions and
> has already been changed in spapr (in 94ad93bd "spapr_cpu_core: instantiate
> CPUs separately".
> 
> Make an equivalent change in pnv_core to get rid of the nasty pointer
> arithmetic.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/pnv.c              |  4 ++--
>  hw/ppc/pnv_core.c         | 11 +++++------
>  include/hw/ppc/pnv_core.h |  2 +-
>  3 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0314881316..0b9508d94d 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -121,9 +121,9 @@ static int get_cpus_node(void *fdt)
>   */
>  static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
>  {
> -    CPUState *cs = CPU(DEVICE(pc->threads));
> +    PowerPCCPU *cpu = pc->threads[0];
> +    CPUState *cs = CPU(cpu);
>      DeviceClass *dc = DEVICE_GET_CLASS(cs);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>      int smt_threads = CPU_CORE(pc)->nr_threads;
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cs);
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index efb68226bb..59309e149c 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -151,7 +151,6 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      PnvCore *pc = PNV_CORE(OBJECT(dev));
>      CPUCore *cc = CPU_CORE(OBJECT(dev));
>      const char *typename = pnv_core_cpu_typename(pc);
> -    size_t size = object_type_get_instance_size(typename);
>      Error *local_err = NULL;
>      void *obj;
>      int i, j;
> @@ -165,11 +164,11 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    pc->threads = g_malloc0(size * cc->nr_threads);
> +    pc->threads = g_new(PowerPCCPU *, cc->nr_threads);
>      for (i = 0; i < cc->nr_threads; i++) {
> -        obj = pc->threads + i * size;
> +        obj = object_new(typename);
>  
> -        object_initialize(obj, size, typename);
> +        pc->threads[i] = POWERPC_CPU(obj);
>  
>          snprintf(name, sizeof(name), "thread[%d]", i);
>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> @@ -185,7 +184,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = pc->threads + j * size;
> +        obj = OBJECT(pc->threads[j]);
>  
>          pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
>          if (local_err) {
> @@ -200,7 +199,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>  
>  err:
>      while (--i >= 0) {
> -        obj = pc->threads + i * size;
> +        obj = OBJECT(pc->threads[i]);
>          object_unparent(obj);
>      }
>      g_free(pc->threads);
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index e337af7a3a..447ae761f7 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -34,7 +34,7 @@ typedef struct PnvCore {
>      CPUCore parent_obj;
>  
>      /*< public >*/
> -    void *threads;
> +    PowerPCCPU **threads;
>      uint32_t pir;
>  
>      MemoryRegion xscom_regs;

Re: [Qemu-devel] [PATCH 4/7] pnv: Clean up cpu realize path

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:20:43AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 08:57 AM, David Gibson wrote:
> > pnv_cpu_init() is only called from the the pnv cpu core realize path, and
> > really only can be called from there.  So fold it into its caller, which
> > we also rename for brevity.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> I think we should set the default CPU settings (PIR) before creating
> the 'intc' object. I have cleanup for that in the pnv patchset. 
> Nevertheless,

Ok.

> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks,
> 
> C.
> 
> > ---
> >  hw/ppc/pnv_core.c | 56 ++++++++++++++++++-----------------------------
> >  1 file changed, 21 insertions(+), 35 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index 59309e149c..c9648fd1ad 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -54,28 +54,6 @@ static void pnv_cpu_reset(void *opaque)
> >      env->msr |= MSR_HVB; /* Hypervisor mode */
> >  }
> >  
> > -static void pnv_cpu_init(PowerPCCPU *cpu, Error **errp)
> > -{
> > -    CPUPPCState *env = &cpu->env;
> > -    int core_pir;
> > -    int thread_index = 0; /* TODO: TCG supports only one thread */
> > -    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> > -
> > -    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> > -
> > -    /*
> > -     * The PIR of a thread is the core PIR + the thread index. We will
> > -     * need to find a way to get the thread index when TCG supports
> > -     * more than 1. We could use the object name ?
> > -     */
> > -    pir->default_value = core_pir + thread_index;
> > -
> > -    /* Set time-base frequency to 512 MHz */
> > -    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> > -
> > -    qemu_register_reset(pnv_cpu_reset, cpu);
> > -}
> > -
> >  /*
> >   * These values are read by the PowerNV HW monitors under Linux
> >   */
> > @@ -121,29 +99,39 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
> >      .endianness = DEVICE_BIG_ENDIAN,
> >  };
> >  
> > -static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> > +static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> >  {
> > +    CPUPPCState *env = &cpu->env;
> > +    int core_pir;
> > +    int thread_index = 0; /* TODO: TCG supports only one thread */
> > +    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> >      Error *local_err = NULL;
> > -    CPUState *cs = CPU(child);
> > -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -    object_property_set_bool(child, true, "realized", &local_err);
> > +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> >  
> > -    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
> > +    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> >      }
> >  
> > -    pnv_cpu_init(cpu, &local_err);
> > -    if (local_err) {
> > -        error_propagate(errp, local_err);
> > -        return;
> > -    }
> > +    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> > +
> > +    /*
> > +     * The PIR of a thread is the core PIR + the thread index. We will
> > +     * need to find a way to get the thread index when TCG supports
> > +     * more than 1. We could use the object name ?
> > +     */
> > +    pir->default_value = core_pir + thread_index;
> > +
> > +    /* Set time-base frequency to 512 MHz */
> > +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> > +
> > +    qemu_register_reset(pnv_cpu_reset, cpu);
> >  }
> >  
> >  static void pnv_core_realize(DeviceState *dev, Error **errp)
> > @@ -184,9 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >      }
> >  
> >      for (j = 0; j < cc->nr_threads; j++) {
> > -        obj = OBJECT(pc->threads[j]);
> > -
> > -        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
> > +        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> > 
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[Cédric Le Goater]

>> index 13ad7d9e04..efb68226bb 100644
>> --- a/hw/ppc/pnv_core.c
>> +++ b/hw/ppc/pnv_core.c
>> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>>  
>>          snprintf(name, sizeof(name), "thread[%d]", i);
>>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
>> +        if (local_err) {
>> +            goto err;
>> +        }
>>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
>>                                    "pir", &local_err);
>>          if (local_err) {
> 
> Hmm... the current error path seems to assume failures to be
> caused by object_property_add_child(). It hence unparents the
> previously parented CPUs, but not the current one. So we'll
> miss one call to object_unparent() if object_property_add_alias()
> fails.

yes, let's just put NULL or &error_abort instead.

C. 

Re: [Qemu-devel] [PATCH 4/7] pnv: Clean up cpu realize path

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:04 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> pnv_cpu_init() is only called from the the pnv cpu core realize path, and
> really only can be called from there.  So fold it into its caller, which
> we also rename for brevity.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/pnv_core.c | 56 ++++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 59309e149c..c9648fd1ad 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -54,28 +54,6 @@ static void pnv_cpu_reset(void *opaque)
>      env->msr |= MSR_HVB; /* Hypervisor mode */
>  }
>  
> -static void pnv_cpu_init(PowerPCCPU *cpu, Error **errp)
> -{
> -    CPUPPCState *env = &cpu->env;
> -    int core_pir;
> -    int thread_index = 0; /* TODO: TCG supports only one thread */
> -    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> -
> -    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> -
> -    /*
> -     * The PIR of a thread is the core PIR + the thread index. We will
> -     * need to find a way to get the thread index when TCG supports
> -     * more than 1. We could use the object name ?
> -     */
> -    pir->default_value = core_pir + thread_index;
> -
> -    /* Set time-base frequency to 512 MHz */
> -    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> -
> -    qemu_register_reset(pnv_cpu_reset, cpu);
> -}
> -
>  /*
>   * These values are read by the PowerNV HW monitors under Linux
>   */
> @@ -121,29 +99,39 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
> +static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>  {
> +    CPUPPCState *env = &cpu->env;
> +    int core_pir;
> +    int thread_index = 0; /* TODO: TCG supports only one thread */
> +    ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
>      Error *local_err = NULL;
> -    CPUState *cs = CPU(child);
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -    object_property_set_bool(child, true, "realized", &local_err);
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
> +    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
>      }
>  
> -    pnv_cpu_init(cpu, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -        return;
> -    }
> +    core_pir = object_property_get_uint(OBJECT(cpu), "core-pir", &error_abort);
> +
> +    /*
> +     * The PIR of a thread is the core PIR + the thread index. We will
> +     * need to find a way to get the thread index when TCG supports
> +     * more than 1. We could use the object name ?
> +     */
> +    pir->default_value = core_pir + thread_index;
> +
> +    /* Set time-base frequency to 512 MHz */
> +    cpu_ppc_tb_init(env, PNV_TIMEBASE_FREQ);
> +
> +    qemu_register_reset(pnv_cpu_reset, cpu);
>  }
>  
>  static void pnv_core_realize(DeviceState *dev, Error **errp)
> @@ -184,9 +172,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        obj = OBJECT(pc->threads[j]);
> -
> -        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
>          if (local_err) {
>              goto err;
>          }

Re: [Qemu-devel] [PATCH 5/7] pnv: Add cpu unrealize path

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:05 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we don't have any unrealize path for pnv cpu cores.  We get away
> with this because we don't yet support cpu hotplug for pnv.
> 
> However, we're going to want it eventually, and in the meantime, it makes
> it non-obvious why there are a bunch of allocations on the realize() path
> that don't have matching frees.
> 
> So, implement the missing unrealize path.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/pnv_core.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index c9648fd1ad..c70dbbe056 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -192,6 +192,26 @@ err:
>      error_propagate(errp, local_err);
>  }
>  
> +static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> +{
> +    qemu_unregister_reset(pnv_cpu_reset, cpu);
> +    object_unparent(cpu->intc);
> +    cpu_remove_sync(CPU(cpu));
> +    object_unparent(OBJECT(cpu));
> +}
> +
> +static void pnv_core_unrealize(DeviceState *dev, Error **errp)
> +{
> +    PnvCore *pc = PNV_CORE(dev);
> +    CPUCore *cc = CPU_CORE(dev);
> +    int i;
> +
> +    for (i = 0; i < cc->nr_threads; i++) {
> +        pnv_unrealize_vcpu(pc->threads[i]);
> +    }
> +    g_free(pc->threads);
> +}
> +
>  static Property pnv_core_properties[] = {
>      DEFINE_PROP_UINT32("pir", PnvCore, pir, 0),
>      DEFINE_PROP_END_OF_LIST(),
> @@ -202,6 +222,7 @@ static void pnv_core_class_init(ObjectClass *oc, void *data)
>      DeviceClass *dc = DEVICE_CLASS(oc);
>  
>      dc->realize = pnv_core_realize;
> +    dc->unrealize = pnv_core_unrealize;
>      dc->props = pnv_core_properties;
>  }
>  

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[Greg Kurz]

On Wed, 13 Jun 2018 11:14:57 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> >> index 13ad7d9e04..efb68226bb 100644
> >> --- a/hw/ppc/pnv_core.c
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >>  
> >>          snprintf(name, sizeof(name), "thread[%d]", i);
> >>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> >> +        if (local_err) {
> >> +            goto err;
> >> +        }
> >>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
> >>                                    "pir", &local_err);
> >>          if (local_err) {  
> > 
> > Hmm... the current error path seems to assume failures to be
> > caused by object_property_add_child(). It hence unparents the
> > previously parented CPUs, but not the current one. So we'll
> > miss one call to object_unparent() if object_property_add_alias()
> > fails.  
> 
> yes, let's just put NULL or &error_abort instead.
> 

NULL means we really don't care if the call fails or succeeds.

&error_abort means we consider a failure to be a unrecoverable bug.

So I would rather pass &error_abort here.

But if the guest is already running and functional, and we hit
the error during hotplug, does the guest really deserve to be
aborted or should we just fail the hotplug ?

> C. 

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[David Gibson]

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

On Wed, Jun 13, 2018 at 11:14:57AM +0200, Cédric Le Goater wrote:
> >> index 13ad7d9e04..efb68226bb 100644
> >> --- a/hw/ppc/pnv_core.c
> >> +++ b/hw/ppc/pnv_core.c
> >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> >>  
> >>          snprintf(name, sizeof(name), "thread[%d]", i);
> >>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> >> +        if (local_err) {
> >> +            goto err;
> >> +        }
> >>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
> >>                                    "pir", &local_err);
> >>          if (local_err) {
> > 
> > Hmm... the current error path seems to assume failures to be
> > caused by object_property_add_child(). It hence unparents the
> > previously parented CPUs, but not the current one. So we'll
> > miss one call to object_unparent() if object_property_add_alias()
> > fails.
> 
> yes, let's just put NULL or &error_abort instead.

Yeah, good idea, I'll change it in a new spin.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer

[David Gibson]

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

On Wed, Jun 13, 2018 at 10:46:02AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 08:57 AM, David Gibson wrote:
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller. Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
> 
> yes and that made sense in terms of modeling because you actually have a 
> set of wires between the presenter and the cores of a system.
> 
> > Really, this field is machine specific.  The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> > 
> > There's also other information that's per-cpu, but platform/machine
> > specific.  So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
> 
> ah. so you have something else the store in the machine_data. 
> 
> If you were defining a type, we would have some more checks when 
> casting the machine_data field. We also could parent the object 
> to the CPU also. This is minor.

My intention is that machine_data be a "passive" structure, not a QOM
object.  Lifetime and type management are all up to the machine.

> The change should be compatible with the XIVE change which need 
> to allocate a different type of presenter. So, sPAPRCPUState and 
> PnvCPUState would look like :
> 
> 	typedef struct sPAPRCPUState {
> 	    ICPState *icp;
> 	    XiveTCTX *tctx;
> 	} sPAPRCPUState;

Exactly.

> and the call to ipc_create() will move in an operation of the 
> sPAPR IRQ backend, if that exists oneday, and in an operation of 
> the PnvChip to handle the differences in the interrupt controller
> in use by the machine. 
> 
> So no big difference, but the cpu machine_data won't be populated
> from the core but from the machine. I hope this is compatible
> with the next changes.

intc was already populated from the machine.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[David Gibson]

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

On Wed, Jun 13, 2018 at 11:42:07AM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 11:14:57 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
> > >> index 13ad7d9e04..efb68226bb 100644
> > >> --- a/hw/ppc/pnv_core.c
> > >> +++ b/hw/ppc/pnv_core.c
> > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> > >>  
> > >>          snprintf(name, sizeof(name), "thread[%d]", i);
> > >>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> > >> +        if (local_err) {
> > >> +            goto err;
> > >> +        }
> > >>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
> > >>                                    "pir", &local_err);
> > >>          if (local_err) {  
> > > 
> > > Hmm... the current error path seems to assume failures to be
> > > caused by object_property_add_child(). It hence unparents the
> > > previously parented CPUs, but not the current one. So we'll
> > > miss one call to object_unparent() if object_property_add_alias()
> > > fails.  
> > 
> > yes, let's just put NULL or &error_abort instead.
> > 
> 
> NULL means we really don't care if the call fails or succeeds.
> 
> &error_abort means we consider a failure to be a unrecoverable bug.
> 
> So I would rather pass &error_abort here.
> 
> But if the guest is already running and functional, and we hit
> the error during hotplug, does the guest really deserve to be
> aborted or should we just fail the hotplug ?

Ah, dammit, that's why it wasn't an abort in the first place.  Yeah,
we'd better propagate the errors.

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:06 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> controller.  Or more precisely to the "presentation" component of the
> interrupt controller relevant to this cpu.
> 
> Really, this field is machine specific.  The machines which use it can
> point it to different types of object depending on their needs, and most
> machines don't use it at all (since they have older style PICs which don't
> have per-cpu presentation components).
> 
> There's also other information that's per-cpu, but platform/machine
> specific.  So replace the intc pointer with a (void *)machine_data which
> can be managed as the machine type likes to conveniently store per cpu
> information.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

This looks good, just one question below...

>  hw/intc/xics.c                  |  5 +++--
>  hw/intc/xics_spapr.c            | 16 +++++++++++-----
>  hw/ppc/pnv.c                    |  4 ++--
>  hw/ppc/pnv_core.c               | 11 +++++++++--
>  hw/ppc/spapr.c                  |  8 ++++----
>  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++---
>  include/hw/ppc/pnv_core.h       |  9 +++++++++
>  include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
>  include/hw/ppc/xics.h           |  4 ++--
>  target/ppc/cpu.h                |  2 +-
>  10 files changed, 61 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> index e73e623e3b..689ad44e5f 100644
> --- a/hw/intc/xics.c
> +++ b/hw/intc/xics.c
> @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
>      .class_size = sizeof(ICPStateClass),
>  };
>  
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> +                     Error **errp)
>  {
>      Error *local_err = NULL;
>      Object *obj;
> @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
>          obj = NULL;
>      }
>  
> -    return obj;
> +    return ICP(obj);
>  }
>  
>  /*
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 2e27b92b87..01c76717cf 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -31,6 +31,7 @@
>  #include "trace.h"
>  #include "qemu/timer.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/fdt.h"
>  #include "qapi/visitor.h"
> @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
>      target_ulong cppr = args[0];
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>  
> -    icp_set_cppr(ICP(cpu->intc), cppr);
> +    icp_set_cppr(spapr_cpu->icp, cppr);
>      return H_SUCCESS;
>  }
>  
> @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                             target_ulong opcode, target_ulong *args)
>  {
> -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_accept(spapr_cpu->icp);
>  
>      args[0] = xirr;
>      return H_SUCCESS;
> @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                               target_ulong opcode, target_ulong *args)
>  {
> -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_accept(spapr_cpu->icp);
>  
>      args[0] = xirr;
>      args[1] = cpu_get_host_ticks();
> @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                            target_ulong opcode, target_ulong *args)
>  {
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong xirr = args[0];
>  
> -    icp_eoi(ICP(cpu->intc), xirr);
> +    icp_eoi(spapr_cpu->icp, xirr);
>      return H_SUCCESS;
>  }
>  
> @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>                              target_ulong opcode, target_ulong *args)
>  {
>      uint32_t mfrr;
> -    uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +    uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
>  
>      args[0] = xirr;
>      args[1] = mfrr;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 0b9508d94d..3a36c6ac6a 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
>  {
>      PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>  
> -    return cpu ? ICP(cpu->intc) : NULL;
> +    return cpu ? pnv_cpu_state(cpu)->icp : NULL;
>  }
>  
>  static void pnv_pic_print_info(InterruptStatsProvider *obj,
> @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        icp_pic_print_info(ICP(cpu->intc), mon);
> +        icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
>      }
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index c70dbbe056..86448ade87 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>      int core_pir;
>      int thread_index = 0; /* TODO: TCG supports only one thread */
>      ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> +    PnvCPUState *pnv_cpu;
>      Error *local_err = NULL;
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
>          return;
>      }
>  
> -    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> +    cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> +
> +    pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -194,8 +197,12 @@ err:
>  
>  static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
>  {
> +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> +
>      qemu_unregister_reset(pnv_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    object_unparent(OBJECT(pnv_cpu->icp));
> +    cpu->machine_data = NULL;

Is this really needed ? I mean cpu is supposed to be freed by
object_unparent() below, right ? Is there a case where we would
dereference this anyway ?

In all cases, better safe than sorry, so:

Reviewed-by: Greg Kurz <groug@kaod.org>

> +    g_free(pnv_cpu);
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index f59999daac..cbab6b6b7e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
>      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
>          CPUState *cs;
>          CPU_FOREACH(cs) {
> -            PowerPCCPU *cpu = POWERPC_CPU(cs);
> -            icp_resend(ICP(cpu->intc));
> +            sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> +            icp_resend(spapr_cpu->icp);
>          }
>      }
>  
> @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
>  {
>      PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
>  
> -    return cpu ? ICP(cpu->intc) : NULL;
> +    return cpu ? spapr_cpu_state(cpu)->icp : NULL;
>  }
>  
>  #define ICS_IRQ_FREE(ics, srcno)   \
> @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
>      CPU_FOREACH(cs) {
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> -        icp_pic_print_info(ICP(cpu->intc), mon);
> +        icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
>      }
>  
>      ics_pic_print_info(spapr->ics, mon);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 7fdb3b6667..544bda93e2 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
>  
>  static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
>  {
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
>      qemu_unregister_reset(spapr_cpu_reset, cpu);
> -    object_unparent(cpu->intc);
> +    object_unparent(OBJECT(spapr_cpu->icp));
> +    cpu->machine_data = NULL;
> +    g_free(spapr_cpu);
>      cpu_remove_sync(CPU(cpu));
>      object_unparent(OBJECT(cpu));
>  }
> @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>  {
>      CPUPPCState *env = &cpu->env;
>      Error *local_err = NULL;
> +    sPAPRCPUState *spapr_cpu;
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
>          goto error;
>      }
>  
> +    spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> +
>      /* Set time-base frequency to 512 MHz */
>      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
>  
> @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      qemu_register_reset(spapr_cpu_reset, cpu);
>      spapr_cpu_reset(cpu);
>  
> -    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> -                           &local_err);
> +    spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> +                                XICS_FABRIC(spapr), &local_err);
>      if (local_err) {
>          goto error;
>      }
> diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> index 447ae761f7..f81dff28aa 100644
> --- a/include/hw/ppc/pnv_core.h
> +++ b/include/hw/ppc/pnv_core.h
> @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
>  #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
>  #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
>  
> +typedef struct PnvCPUState {
> +    ICPState *icp;
> +} PnvCPUState;
> +
> +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> +{
> +    return cpu->machine_data;
> +}
> +
>  #endif /* _PPC_PNV_CORE_H */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 47dcfda12b..e3d2aa45a4 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
>  const char *spapr_get_cpu_core_type(const char *cpu_type);
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
>  
> +typedef struct ICPState ICPState;
> +typedef struct sPAPRCPUState {
> +    ICPState *icp;
> +} sPAPRCPUState;
> +
> +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> +{
> +    return (sPAPRCPUState *)cpu->machine_data;
> +}
> +
>  #endif
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 6cebff47a7..48930d91e5 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
>  void xics_spapr_init(sPAPRMachineState *spapr);
>  
> -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> -                   Error **errp);
> +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> +                     Error **errp);
>  
>  #endif /* XICS_H */
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index a91f1a8777..abf0bf0224 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    Object *intc;
> +    void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;
>  

Re: [Qemu-devel] [PATCH 6/7] target/ppc: Replace intc pointer with a general machine_data pointer

[David Gibson]

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

On Wed, Jun 13, 2018 at 12:11:12PM +0200, Greg Kurz wrote:
> On Wed, 13 Jun 2018 16:57:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > PowerPCCPU contains an (Object *)intc used to point to the cpu's interrupt
> > controller.  Or more precisely to the "presentation" component of the
> > interrupt controller relevant to this cpu.
> > 
> > Really, this field is machine specific.  The machines which use it can
> > point it to different types of object depending on their needs, and most
> > machines don't use it at all (since they have older style PICs which don't
> > have per-cpu presentation components).
> > 
> > There's also other information that's per-cpu, but platform/machine
> > specific.  So replace the intc pointer with a (void *)machine_data which
> > can be managed as the machine type likes to conveniently store per cpu
> > information.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> 
> This looks good, just one question below...
> 
> >  hw/intc/xics.c                  |  5 +++--
> >  hw/intc/xics_spapr.c            | 16 +++++++++++-----
> >  hw/ppc/pnv.c                    |  4 ++--
> >  hw/ppc/pnv_core.c               | 11 +++++++++--
> >  hw/ppc/spapr.c                  |  8 ++++----
> >  hw/ppc/spapr_cpu_core.c         | 13 ++++++++++---
> >  include/hw/ppc/pnv_core.h       |  9 +++++++++
> >  include/hw/ppc/spapr_cpu_core.h | 10 ++++++++++
> >  include/hw/ppc/xics.h           |  4 ++--
> >  target/ppc/cpu.h                |  2 +-
> >  10 files changed, 61 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/intc/xics.c b/hw/intc/xics.c
> > index e73e623e3b..689ad44e5f 100644
> > --- a/hw/intc/xics.c
> > +++ b/hw/intc/xics.c
> > @@ -383,7 +383,8 @@ static const TypeInfo icp_info = {
> >      .class_size = sizeof(ICPStateClass),
> >  };
> >  
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> > +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > +                     Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      Object *obj;
> > @@ -401,7 +402,7 @@ Object *icp_create(Object *cpu, const char *type, XICSFabric *xi, Error **errp)
> >          obj = NULL;
> >      }
> >  
> > -    return obj;
> > +    return ICP(obj);
> >  }
> >  
> >  /*
> > diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> > index 2e27b92b87..01c76717cf 100644
> > --- a/hw/intc/xics_spapr.c
> > +++ b/hw/intc/xics_spapr.c
> > @@ -31,6 +31,7 @@
> >  #include "trace.h"
> >  #include "qemu/timer.h"
> >  #include "hw/ppc/spapr.h"
> > +#include "hw/ppc/spapr_cpu_core.h"
> >  #include "hw/ppc/xics.h"
> >  #include "hw/ppc/fdt.h"
> >  #include "qapi/visitor.h"
> > @@ -43,8 +44,9 @@ static target_ulong h_cppr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                             target_ulong opcode, target_ulong *args)
> >  {
> >      target_ulong cppr = args[0];
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> >  
> > -    icp_set_cppr(ICP(cpu->intc), cppr);
> > +    icp_set_cppr(spapr_cpu->icp, cppr);
> >      return H_SUCCESS;
> >  }
> >  
> > @@ -65,7 +67,8 @@ static target_ulong h_ipi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >  static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                             target_ulong opcode, target_ulong *args)
> >  {
> > -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +    uint32_t xirr = icp_accept(spapr_cpu->icp);
> >  
> >      args[0] = xirr;
> >      return H_SUCCESS;
> > @@ -74,7 +77,8 @@ static target_ulong h_xirr(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >  static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                               target_ulong opcode, target_ulong *args)
> >  {
> > -    uint32_t xirr = icp_accept(ICP(cpu->intc));
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +    uint32_t xirr = icp_accept(spapr_cpu->icp);
> >  
> >      args[0] = xirr;
> >      args[1] = cpu_get_host_ticks();
> > @@ -84,9 +88,10 @@ static target_ulong h_xirr_x(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >  static target_ulong h_eoi(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                            target_ulong opcode, target_ulong *args)
> >  {
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> >      target_ulong xirr = args[0];
> >  
> > -    icp_eoi(ICP(cpu->intc), xirr);
> > +    icp_eoi(spapr_cpu->icp, xirr);
> >      return H_SUCCESS;
> >  }
> >  
> > @@ -94,7 +99,8 @@ static target_ulong h_ipoll(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >                              target_ulong opcode, target_ulong *args)
> >  {
> >      uint32_t mfrr;
> > -    uint32_t xirr = icp_ipoll(ICP(cpu->intc), &mfrr);
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +    uint32_t xirr = icp_ipoll(spapr_cpu->icp, &mfrr);
> >  
> >      args[0] = xirr;
> >      args[1] = mfrr;
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 0b9508d94d..3a36c6ac6a 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -1013,7 +1013,7 @@ static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> >  {
> >      PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> >  
> > -    return cpu ? ICP(cpu->intc) : NULL;
> > +    return cpu ? pnv_cpu_state(cpu)->icp : NULL;
> >  }
> >  
> >  static void pnv_pic_print_info(InterruptStatsProvider *obj,
> > @@ -1026,7 +1026,7 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
> >      CPU_FOREACH(cs) {
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -        icp_pic_print_info(ICP(cpu->intc), mon);
> > +        icp_pic_print_info(pnv_cpu_state(cpu)->icp, mon);
> >      }
> >  
> >      for (i = 0; i < pnv->num_chips; i++) {
> > diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> > index c70dbbe056..86448ade87 100644
> > --- a/hw/ppc/pnv_core.c
> > +++ b/hw/ppc/pnv_core.c
> > @@ -105,6 +105,7 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> >      int core_pir;
> >      int thread_index = 0; /* TODO: TCG supports only one thread */
> >      ppc_spr_t *pir = &env->spr_cb[SPR_PIR];
> > +    PnvCPUState *pnv_cpu;
> >      Error *local_err = NULL;
> >  
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> > @@ -113,7 +114,9 @@ static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> >          return;
> >      }
> >  
> > -    cpu->intc = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> > +    cpu->machine_data = pnv_cpu = g_new0(PnvCPUState, 1);
> > +
> > +    pnv_cpu->icp = icp_create(OBJECT(cpu), TYPE_PNV_ICP, xi, &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> >          return;
> > @@ -194,8 +197,12 @@ err:
> >  
> >  static void pnv_unrealize_vcpu(PowerPCCPU *cpu)
> >  {
> > +    PnvCPUState *pnv_cpu = pnv_cpu_state(cpu);
> > +
> >      qemu_unregister_reset(pnv_cpu_reset, cpu);
> > -    object_unparent(cpu->intc);
> > +    object_unparent(OBJECT(pnv_cpu->icp));
> > +    cpu->machine_data = NULL;
> 
> Is this really needed ? I mean cpu is supposed to be freed by
> object_unparent() below, right ? Is there a case where we would
> dereference this anyway ?

It's probably not necessary.  But, it's not a hot path, so I figured
might as well be paranoid.  If it goes horribly wrong, a NULL
dereference is probably easier to debug than a random stale pointer
dereference.

> In all cases, better safe than sorry, so:
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> > +    g_free(pnv_cpu);
> >      cpu_remove_sync(CPU(cpu));
> >      object_unparent(OBJECT(cpu));
> >  }
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index f59999daac..cbab6b6b7e 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1745,8 +1745,8 @@ static int spapr_post_load(void *opaque, int version_id)
> >      if (!object_dynamic_cast(OBJECT(spapr->ics), TYPE_ICS_KVM)) {
> >          CPUState *cs;
> >          CPU_FOREACH(cs) {
> > -            PowerPCCPU *cpu = POWERPC_CPU(cs);
> > -            icp_resend(ICP(cpu->intc));
> > +            sPAPRCPUState *spapr_cpu = spapr_cpu_state(POWERPC_CPU(cs));
> > +            icp_resend(spapr_cpu->icp);
> >          }
> >      }
> >  
> > @@ -3783,7 +3783,7 @@ static ICPState *spapr_icp_get(XICSFabric *xi, int vcpu_id)
> >  {
> >      PowerPCCPU *cpu = spapr_find_cpu(vcpu_id);
> >  
> > -    return cpu ? ICP(cpu->intc) : NULL;
> > +    return cpu ? spapr_cpu_state(cpu)->icp : NULL;
> >  }
> >  
> >  #define ICS_IRQ_FREE(ics, srcno)   \
> > @@ -3925,7 +3925,7 @@ static void spapr_pic_print_info(InterruptStatsProvider *obj,
> >      CPU_FOREACH(cs) {
> >          PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > -        icp_pic_print_info(ICP(cpu->intc), mon);
> > +        icp_pic_print_info(spapr_cpu_state(cpu)->icp, mon);
> >      }
> >  
> >      ics_pic_print_info(spapr->ics, mon);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 7fdb3b6667..544bda93e2 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -104,8 +104,12 @@ const char *spapr_get_cpu_core_type(const char *cpu_type)
> >  
> >  static void spapr_unrealize_vcpu(PowerPCCPU *cpu)
> >  {
> > +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> > +
> >      qemu_unregister_reset(spapr_cpu_reset, cpu);
> > -    object_unparent(cpu->intc);
> > +    object_unparent(OBJECT(spapr_cpu->icp));
> > +    cpu->machine_data = NULL;
> > +    g_free(spapr_cpu);
> >      cpu_remove_sync(CPU(cpu));
> >      object_unparent(OBJECT(cpu));
> >  }
> > @@ -127,12 +131,15 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >  {
> >      CPUPPCState *env = &cpu->env;
> >      Error *local_err = NULL;
> > +    sPAPRCPUState *spapr_cpu;
> >  
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> >  
> > +    spapr_cpu = cpu->machine_data = g_new0(sPAPRCPUState, 1);
> > +
> >      /* Set time-base frequency to 512 MHz */
> >      cpu_ppc_tb_init(env, SPAPR_TIMEBASE_FREQ);
> >  
> > @@ -142,8 +149,8 @@ static void spapr_realize_vcpu(PowerPCCPU *cpu, sPAPRMachineState *spapr,
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> >      spapr_cpu_reset(cpu);
> >  
> > -    cpu->intc = icp_create(OBJECT(cpu), spapr->icp_type, XICS_FABRIC(spapr),
> > -                           &local_err);
> > +    spapr_cpu->icp = icp_create(OBJECT(cpu), spapr->icp_type,
> > +                                XICS_FABRIC(spapr), &local_err);
> >      if (local_err) {
> >          goto error;
> >      }
> > diff --git a/include/hw/ppc/pnv_core.h b/include/hw/ppc/pnv_core.h
> > index 447ae761f7..f81dff28aa 100644
> > --- a/include/hw/ppc/pnv_core.h
> > +++ b/include/hw/ppc/pnv_core.h
> > @@ -47,4 +47,13 @@ typedef struct PnvCoreClass {
> >  #define PNV_CORE_TYPE_SUFFIX "-" TYPE_PNV_CORE
> >  #define PNV_CORE_TYPE_NAME(cpu_model) cpu_model PNV_CORE_TYPE_SUFFIX
> >  
> > +typedef struct PnvCPUState {
> > +    ICPState *icp;
> > +} PnvCPUState;
> > +
> > +static inline PnvCPUState *pnv_cpu_state(PowerPCCPU *cpu)
> > +{
> > +    return cpu->machine_data;
> > +}
> > +
> >  #endif /* _PPC_PNV_CORE_H */
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 47dcfda12b..e3d2aa45a4 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -41,4 +41,14 @@ typedef struct sPAPRCPUCoreClass {
> >  const char *spapr_get_cpu_core_type(const char *cpu_type);
> >  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3);
> >  
> > +typedef struct ICPState ICPState;
> > +typedef struct sPAPRCPUState {
> > +    ICPState *icp;
> > +} sPAPRCPUState;
> > +
> > +static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> > +{
> > +    return (sPAPRCPUState *)cpu->machine_data;
> > +}
> > +
> >  #endif
> > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> > index 6cebff47a7..48930d91e5 100644
> > --- a/include/hw/ppc/xics.h
> > +++ b/include/hw/ppc/xics.h
> > @@ -207,7 +207,7 @@ typedef struct sPAPRMachineState sPAPRMachineState;
> >  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> >  void xics_spapr_init(sPAPRMachineState *spapr);
> >  
> > -Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > -                   Error **errp);
> > +ICPState *icp_create(Object *cpu, const char *type, XICSFabric *xi,
> > +                     Error **errp);
> >  
> >  #endif /* XICS_H */
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index a91f1a8777..abf0bf0224 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1204,7 +1204,7 @@ struct PowerPCCPU {
> >      int vcpu_id;
> >      uint32_t compat_pvr;
> >      PPCVirtualHypervisor *vhyp;
> > -    Object *intc;
> > +    void *machine_data;
> >      int32_t node_id; /* NUMA node this CPU belongs to */
> >      PPCHash64Options *hash64_opts;
> >  
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 7/7] target/ppc, spapr: Move VPA information to machine_data

[Greg Kurz]

On Wed, 13 Jun 2018 16:57:07 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> CPUPPCState currently contains a number of fields containing the state of
> the VPA.  The VPA is a PAPR specific concept covering several guest/host
> shared memory areas used to communicate some information with the
> hypervisor.
> 
> As a PAPR concept this is really machine specific information, although it
> is per-cpu, so it doesn't really belong in the core CPU state structure.
> So, move it to the PAPR specific 'machine_data' structure.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---

Nice ! I'll rework VPA migration on top of that.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/ppc/spapr_cpu_core.c         |  7 +++
>  hw/ppc/spapr_hcall.c            | 77 ++++++++++++++++++---------------
>  include/hw/ppc/spapr_cpu_core.h |  3 ++
>  target/ppc/cpu.h                |  6 ---
>  target/ppc/translate_init.inc.c |  8 ----
>  5 files changed, 52 insertions(+), 49 deletions(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 544bda93e2..f642c95967 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -28,6 +28,7 @@ static void spapr_cpu_reset(void *opaque)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong lpcr;
>  
>      cpu_reset(cs);
> @@ -69,6 +70,12 @@ static void spapr_cpu_reset(void *opaque)
>  
>      /* Set a full AMOR so guest can use the AMR as it sees fit */
>      env->spr[SPR_AMOR] = 0xffffffffffffffffull;
> +
> +    spapr_cpu->vpa_addr = 0;
> +    spapr_cpu->slb_shadow_addr = 0;
> +    spapr_cpu->slb_shadow_size = 0;
> +    spapr_cpu->dtl_addr = 0;
> +    spapr_cpu->dtl_size = 0;
>  }
>  
>  void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r3)
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 8b9a4b577f..ae913d070f 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -8,6 +8,7 @@
>  #include "exec/exec-all.h"
>  #include "helper_regs.h"
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_cpu_core.h"
>  #include "mmu-hash64.h"
>  #include "cpu-models.h"
>  #include "trace.h"
> @@ -908,9 +909,11 @@ unmap_out:
>  #define VPA_SHARED_PROC_OFFSET 0x9
>  #define VPA_SHARED_PROC_VAL    0x2
>  
> -static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
> +static target_ulong register_vpa(PowerPCCPU *cpu, target_ulong vpa)
>  {
> -    CPUState *cs = CPU(ppc_env_get_cpu(env));
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      uint16_t size;
>      uint8_t tmp;
>  
> @@ -935,32 +938,34 @@ static target_ulong register_vpa(CPUPPCState *env, target_ulong vpa)
>          return H_PARAMETER;
>      }
>  
> -    env->vpa_addr = vpa;
> +    spapr_cpu->vpa_addr = vpa;
>  
> -    tmp = ldub_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET);
> +    tmp = ldub_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET);
>      tmp |= VPA_SHARED_PROC_VAL;
> -    stb_phys(cs->as, env->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
> +    stb_phys(cs->as, spapr_cpu->vpa_addr + VPA_SHARED_PROC_OFFSET, tmp);
>  
>      return H_SUCCESS;
>  }
>  
> -static target_ulong deregister_vpa(CPUPPCState *env, target_ulong vpa)
> +static target_ulong deregister_vpa(PowerPCCPU *cpu, target_ulong vpa)
>  {
> -    if (env->slb_shadow_addr) {
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    if (spapr_cpu->slb_shadow_addr) {
>          return H_RESOURCE;
>      }
>  
> -    if (env->dtl_addr) {
> +    if (spapr_cpu->dtl_addr) {
>          return H_RESOURCE;
>      }
>  
> -    env->vpa_addr = 0;
> +    spapr_cpu->vpa_addr = 0;
>      return H_SUCCESS;
>  }
>  
> -static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
> +static target_ulong register_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
>  {
> -    CPUState *cs = CPU(ppc_env_get_cpu(env));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      uint32_t size;
>  
>      if (addr == 0) {
> @@ -968,7 +973,7 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
>          return H_HARDWARE;
>      }
>  
> -    size = ldl_be_phys(cs->as, addr + 0x4);
> +    size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
>      if (size < 0x8) {
>          return H_PARAMETER;
>      }
> @@ -977,26 +982,28 @@ static target_ulong register_slb_shadow(CPUPPCState *env, target_ulong addr)
>          return H_PARAMETER;
>      }
>  
> -    if (!env->vpa_addr) {
> +    if (!spapr_cpu->vpa_addr) {
>          return H_RESOURCE;
>      }
>  
> -    env->slb_shadow_addr = addr;
> -    env->slb_shadow_size = size;
> +    spapr_cpu->slb_shadow_addr = addr;
> +    spapr_cpu->slb_shadow_size = size;
>  
>      return H_SUCCESS;
>  }
>  
> -static target_ulong deregister_slb_shadow(CPUPPCState *env, target_ulong addr)
> +static target_ulong deregister_slb_shadow(PowerPCCPU *cpu, target_ulong addr)
>  {
> -    env->slb_shadow_addr = 0;
> -    env->slb_shadow_size = 0;
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    spapr_cpu->slb_shadow_addr = 0;
> +    spapr_cpu->slb_shadow_size = 0;
>      return H_SUCCESS;
>  }
>  
> -static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
> +static target_ulong register_dtl(PowerPCCPU *cpu, target_ulong addr)
>  {
> -    CPUState *cs = CPU(ppc_env_get_cpu(env));
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
>      uint32_t size;
>  
>      if (addr == 0) {
> @@ -1004,26 +1011,28 @@ static target_ulong register_dtl(CPUPPCState *env, target_ulong addr)
>          return H_HARDWARE;
>      }
>  
> -    size = ldl_be_phys(cs->as, addr + 0x4);
> +    size = ldl_be_phys(CPU(cpu)->as, addr + 0x4);
>  
>      if (size < 48) {
>          return H_PARAMETER;
>      }
>  
> -    if (!env->vpa_addr) {
> +    if (!spapr_cpu->vpa_addr) {
>          return H_RESOURCE;
>      }
>  
> -    env->dtl_addr = addr;
> -    env->dtl_size = size;
> +    spapr_cpu->dtl_addr = addr;
> +    spapr_cpu->dtl_size = size;
>  
>      return H_SUCCESS;
>  }
>  
> -static target_ulong deregister_dtl(CPUPPCState *env, target_ulong addr)
> +static target_ulong deregister_dtl(PowerPCCPU *cpu, target_ulong addr)
>  {
> -    env->dtl_addr = 0;
> -    env->dtl_size = 0;
> +    sPAPRCPUState *spapr_cpu = spapr_cpu_state(cpu);
> +
> +    spapr_cpu->dtl_addr = 0;
> +    spapr_cpu->dtl_size = 0;
>  
>      return H_SUCCESS;
>  }
> @@ -1035,38 +1044,36 @@ static target_ulong h_register_vpa(PowerPCCPU *cpu, sPAPRMachineState *spapr,
>      target_ulong procno = args[1];
>      target_ulong vpa = args[2];
>      target_ulong ret = H_PARAMETER;
> -    CPUPPCState *tenv;
>      PowerPCCPU *tcpu;
>  
>      tcpu = spapr_find_cpu(procno);
>      if (!tcpu) {
>          return H_PARAMETER;
>      }
> -    tenv = &tcpu->env;
>  
>      switch (flags) {
>      case FLAGS_REGISTER_VPA:
> -        ret = register_vpa(tenv, vpa);
> +        ret = register_vpa(tcpu, vpa);
>          break;
>  
>      case FLAGS_DEREGISTER_VPA:
> -        ret = deregister_vpa(tenv, vpa);
> +        ret = deregister_vpa(tcpu, vpa);
>          break;
>  
>      case FLAGS_REGISTER_SLBSHADOW:
> -        ret = register_slb_shadow(tenv, vpa);
> +        ret = register_slb_shadow(tcpu, vpa);
>          break;
>  
>      case FLAGS_DEREGISTER_SLBSHADOW:
> -        ret = deregister_slb_shadow(tenv, vpa);
> +        ret = deregister_slb_shadow(tcpu, vpa);
>          break;
>  
>      case FLAGS_REGISTER_DTL:
> -        ret = register_dtl(tenv, vpa);
> +        ret = register_dtl(tcpu, vpa);
>          break;
>  
>      case FLAGS_DEREGISTER_DTL:
> -        ret = deregister_dtl(tenv, vpa);
> +        ret = deregister_dtl(tcpu, vpa);
>          break;
>      }
>  
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index e3d2aa45a4..40129cc452 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -44,6 +44,9 @@ void spapr_cpu_set_entry_state(PowerPCCPU *cpu, target_ulong nip, target_ulong r
>  typedef struct ICPState ICPState;
>  typedef struct sPAPRCPUState {
>      ICPState *icp;
> +    uint64_t vpa_addr;
> +    uint64_t slb_shadow_addr, slb_shadow_size;
> +    uint64_t dtl_addr, dtl_size;
>  } sPAPRCPUState;
>  
>  static inline sPAPRCPUState *spapr_cpu_state(PowerPCCPU *cpu)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index abf0bf0224..6c2f4d29f2 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1091,12 +1091,6 @@ struct CPUPPCState {
>      target_ulong rmls;
>  #endif
>  
> -#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> -    uint64_t vpa_addr;
> -    uint64_t slb_shadow_addr, slb_shadow_size;
> -    uint64_t dtl_addr, dtl_size;
> -#endif /* TARGET_PPC64 */
> -
>      int error_code;
>      uint32_t pending_interrupts;
>  #if !defined(CONFIG_USER_ONLY)
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index bb9296f5a3..76d6f3fd5e 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -10316,14 +10316,6 @@ static void ppc_cpu_reset(CPUState *s)
>      s->exception_index = POWERPC_EXCP_NONE;
>      env->error_code = 0;
>  
> -#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> -    env->vpa_addr = 0;
> -    env->slb_shadow_addr = 0;
> -    env->slb_shadow_size = 0;
> -    env->dtl_addr = 0;
> -    env->dtl_size = 0;
> -#endif /* TARGET_PPC64 */
> -
>      for (i = 0; i < ARRAY_SIZE(env->spr_cb); i++) {
>          ppc_spr_t *spr = &env->spr_cb[i];
>  

Re: [Qemu-devel] [PATCH 2/7] pnv: Add missing error check during cpu realize()

[David Gibson]

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

On Wed, Jun 13, 2018 at 07:53:29PM +1000, David Gibson wrote:
> On Wed, Jun 13, 2018 at 11:42:07AM +0200, Greg Kurz wrote:
> > On Wed, 13 Jun 2018 11:14:57 +0200
> > Cédric Le Goater <clg@kaod.org> wrote:
> > 
> > > >> index 13ad7d9e04..efb68226bb 100644
> > > >> --- a/hw/ppc/pnv_core.c
> > > >> +++ b/hw/ppc/pnv_core.c
> > > >> @@ -173,6 +173,9 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
> > > >>  
> > > >>          snprintf(name, sizeof(name), "thread[%d]", i);
> > > >>          object_property_add_child(OBJECT(pc), name, obj, &local_err);
> > > >> +        if (local_err) {
> > > >> +            goto err;
> > > >> +        }
> > > >>          object_property_add_alias(obj, "core-pir", OBJECT(pc),
> > > >>                                    "pir", &local_err);
> > > >>          if (local_err) {  
> > > > 
> > > > Hmm... the current error path seems to assume failures to be
> > > > caused by object_property_add_child(). It hence unparents the
> > > > previously parented CPUs, but not the current one. So we'll
> > > > miss one call to object_unparent() if object_property_add_alias()
> > > > fails.  
> > > 
> > > yes, let's just put NULL or &error_abort instead.
> > > 
> > 
> > NULL means we really don't care if the call fails or succeeds.
> > 
> > &error_abort means we consider a failure to be a unrecoverable bug.
> > 
> > So I would rather pass &error_abort here.
> > 
> > But if the guest is already running and functional, and we hit
> > the error during hotplug, does the guest really deserve to be
> > aborted or should we just fail the hotplug ?
> 
> Ah, dammit, that's why it wasn't an abort in the first place.  Yeah,
> we'd better propagate the errors.

No.. thinking about this yet again, we should be ok with error_abort.
These really aren't things that should fail.  If they do something has
gone so horribly wrong, that I think an abort() is a reasonable
reaction, even on hotplug.

-- 
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: 833 bytes --]