[Qemu-devel] [PATCH 0/6] ppc/pnv: new Pnv8Chip and Pnv9Chip models

[Qemu-devel] [PATCH 0/6] ppc/pnv: new Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

Hello,

First are some cleanups around the ISA bus of the machine. Then
additions of new chip models for the different processor the PowerNV
machine supports, which come with their respective "powernv8" and
"powernv9" machines.


For some obscure reasons, this patchset breaks 'make check' :

  TEST: tests/spapr-phb-test... (pid=8196)
  qemu-system-ppc64: -device spapr-pci-host-bridge,index=30: spapr-pci-host-bridge needs a pseries machine
  Broken pipe
 FAIL: tests/spapr-phb-test

Any idea ? I guess this is because of the new machines. 

Thanks,

C. 

Cédric Le Goater (6):
  ppc/pnv: introduce a 'primary' field under the LPC model
  ppc/pnv: move the details of the ISA bus creation under the LPC model
  ppc/pnv: introduce an 'isa_bus_name' field under the LPC model
  ppc/pnv: introduce a pnv_chip_core_realize() routine
  ppc/pnv: introduce a new intc_create() operation to the chip model
  ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

 include/hw/ppc/pnv.h     |  29 +++-
 include/hw/ppc/pnv_lpc.h |   6 +-
 hw/ppc/pnv.c             | 380 +++++++++++++++++++++++++++++------------------
 hw/ppc/pnv_core.c        |  18 +--
 hw/ppc/pnv_lpc.c         |  55 +++++--
 5 files changed, 322 insertions(+), 166 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/6] ppc/pnv: introduce a 'primary' field under the LPC model

[Cédric Le Goater]

When a PowerNV system is started, the firmware (skiboot) looks for a
"primary" property to determine which LPC bus is the default on a
multichip system. This property is currently populated in the main
routine creating the device tree of a chip, which is the not the right
place to do so.

Check the chip id to flag the LPC controller as "primary", or not, and
use that to add the property in the LPC device tree routine.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv_lpc.h |  2 ++
 hw/ppc/pnv.c             | 19 ++++++-------------
 hw/ppc/pnv_lpc.c         | 29 ++++++++++++++++++++---------
 3 files changed, 28 insertions(+), 22 deletions(-)

diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index 53fdd5bb6450..fddcb1c054b3 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -68,6 +68,8 @@ typedef struct PnvLpcController {
 
     /* PSI to generate interrupts */
     PnvPsi *psi;
+
+    bool   primary;
 } PnvLpcController;
 
 qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 031488131629..b419d3323100 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -285,16 +285,6 @@ static void pnv_dt_chip(PnvChip *chip, void *fdt)
 
     pnv_dt_xscom(chip, fdt, 0);
 
-    /* The default LPC bus of a multichip system is on chip 0. It's
-     * recognized by the firmware (skiboot) using a "primary"
-     * property.
-     */
-    if (chip->chip_id == 0x0) {
-        int lpc_offset = pnv_chip_lpc_offset(chip, fdt);
-
-        _FDT((fdt_setprop(fdt, lpc_offset, "primary", NULL, 0)));
-    }
-
     for (i = 0; i < chip->nr_cores; i++) {
         PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
 
@@ -814,9 +804,12 @@ static void pnv_chip_init(Object *obj)
     object_property_add_const_link(OBJECT(&chip->occ), "psi",
                                    OBJECT(&chip->psi), &error_abort);
 
-    /* The LPC controller needs PSI to generate interrupts */
-    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
-                                   OBJECT(&chip->psi), &error_abort);
+    /*
+     * The LPC controller needs a few things from the chip : to know
+     * if it's primary and PSI to generate interrupts
+     */
+    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
+                                   OBJECT(chip), &error_abort);
 }
 
 static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 402c4fefa886..1e70c8c19d52 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -113,6 +113,14 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
     _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
     _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
     _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
+
+    /*
+     * The default LPC bus of a multichip system is on chip 0. It's
+     * recognized by the firmware (skiboot) using a "primary" property.
+     */
+    if (PNV_LPC(dev)->primary) {
+        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
+    }
     return 0;
 }
 
@@ -416,6 +424,18 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
     PnvLpcController *lpc = PNV_LPC(dev);
     Object *obj;
     Error *error = NULL;
+    PnvChip *chip;
+
+    /* get PSI object from chip */
+    obj = object_property_get_link(OBJECT(dev), "chip", &error);
+    if (!obj) {
+        error_propagate(errp, error);
+        error_prepend(errp, "required link 'chip' not found: ");
+        return;
+    }
+    chip = PNV_CHIP(obj);
+    lpc->psi = &chip->psi;
+    lpc->primary = chip->chip_id == 0;
 
     /* Reg inits */
     lpc->lpc_hc_fw_rd_acc_size = LPC_HC_FW_RD_4B;
@@ -460,15 +480,6 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
                           &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
                           PNV_XSCOM_LPC_SIZE);
-
-    /* get PSI object from chip */
-    obj = object_property_get_link(OBJECT(dev), "psi", &error);
-    if (!obj) {
-        error_setg(errp, "%s: required link 'psi' not found: %s",
-                   __func__, error_get_pretty(error));
-        return;
-    }
-    lpc->psi = PNV_PSI(obj);
 }
 
 static void pnv_lpc_class_init(ObjectClass *klass, void *data)
-- 
2.13.6

[Qemu-devel] [PATCH 2/6] ppc/pnv: move the details of the ISA bus creation under the LPC model

[Cédric Le Goater]

This is a small cleanup to hide to the machine the gory details of the
creation of the ISA bus. When time comes, the 'qemu_irq_handler' should
become a LPC controller class attribute.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv_lpc.h |  3 +--
 hw/ppc/pnv.c             | 15 +--------------
 hw/ppc/pnv_lpc.c         | 24 ++++++++++++++++++++----
 3 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index fddcb1c054b3..fb4b7b83d798 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -72,7 +72,6 @@ typedef struct PnvLpcController {
     bool   primary;
 } PnvLpcController;
 
-qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
-                                 int nirqs);
+ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type);
 
 #endif /* _PPC_PNV_LPC_H */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index b419d3323100..d2126ee4affc 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -521,22 +521,9 @@ static void pnv_reset(void)
 
 static ISABus *pnv_isa_create(PnvChip *chip)
 {
-    PnvLpcController *lpc = &chip->lpc;
-    ISABus *isa_bus;
-    qemu_irq *irqs;
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
-    /* let isa_bus_new() create its own bridge on SysBus otherwise
-     * devices speficied on the command line won't find the bus and
-     * will fail to create.
-     */
-    isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io,
-                          &error_fatal);
-
-    irqs = pnv_lpc_isa_irq_create(lpc, pcc->chip_type, ISA_NUM_IRQS);
-
-    isa_bus_irqs(isa_bus, irqs);
-    return isa_bus;
+    return pnv_lpc_isa_create(&chip->lpc, pcc->chip_type);
 }
 
 static void pnv_init(MachineState *machine)
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 1e70c8c19d52..7c6c012d5176 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -22,6 +22,7 @@
 #include "target/ppc/cpu.h"
 #include "qapi/error.h"
 #include "qemu/log.h"
+#include "hw/isa/isa.h"
 
 #include "hw/ppc/pnv.h"
 #include "hw/ppc/pnv_lpc.h"
@@ -546,16 +547,31 @@ static void pnv_lpc_isa_irq_handler(void *opaque, int n, int level)
     }
 }
 
-qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
-                                 int nirqs)
+ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type)
 {
+    ISABus *isa_bus;
+    qemu_irq *irqs;
+    qemu_irq_handler handler;
+
+    /* let isa_bus_new() create its own bridge on SysBus otherwise
+     * devices speficied on the command line won't find the bus and
+     * will fail to create.
+     */
+    isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io,
+                          &error_fatal);
+
     /* Not all variants have a working serial irq decoder. If not,
      * handling of LPC interrupts becomes a platform issue (some
      * platforms have a CPLD to do it).
      */
     if (chip_type == PNV_CHIP_POWER8NVL) {
-        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler, lpc, nirqs);
+        handler = pnv_lpc_isa_irq_handler;
     } else {
-        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler_cpld, lpc, nirqs);
+        handler = pnv_lpc_isa_irq_handler_cpld;
     }
+
+    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
+
+    isa_bus_irqs(isa_bus, irqs);
+    return isa_bus;
 }
-- 
2.13.6

[Qemu-devel] [PATCH 3/6] ppc/pnv: introduce an 'isa_bus_name' field under the LPC model

[Cédric Le Goater]

This is again a small cleanup to hide to the machine the details of
the ISA bus. The ISA bus device tree nodename will be different on
Power9.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv_lpc.h | 1 +
 hw/ppc/pnv.c             | 9 +--------
 hw/ppc/pnv_lpc.c         | 4 ++++
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index fb4b7b83d798..e8f7dcb9bfe9 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -70,6 +70,7 @@ typedef struct PnvLpcController {
     PnvPsi *psi;
 
     bool   primary;
+    char   *isa_bus_name;
 } PnvLpcController;
 
 ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index d2126ee4affc..72cfe4c2627c 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -267,14 +267,7 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 
 static int pnv_chip_lpc_offset(PnvChip *chip, void *fdt)
 {
-    char *name;
-    int offset;
-
-    name = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
-                           (uint64_t) PNV_XSCOM_BASE(chip), PNV_XSCOM_LPC_BASE);
-    offset = fdt_path_offset(fdt, name);
-    g_free(name);
-    return offset;
+    return fdt_path_offset(fdt, chip->lpc.isa_bus_name);
 }
 
 static void pnv_dt_chip(PnvChip *chip, void *fdt)
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 7c6c012d5176..7f13c4bcf52c 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -481,6 +481,10 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
     pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
                           &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
                           PNV_XSCOM_LPC_SIZE);
+
+    lpc->isa_bus_name = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
+                                        (uint64_t) PNV_XSCOM_BASE(chip),
+                                        PNV_XSCOM_LPC_BASE);
 }
 
 static void pnv_lpc_class_init(ObjectClass *klass, void *data)
-- 
2.13.6

[Qemu-devel] [PATCH 4/6] ppc/pnv: introduce a pnv_chip_core_realize() routine

[Cédric Le Goater]

This extracts from the PvChip realize routine the part creating the
cores. On Power9, we will need to create the cores after the Xive
interrupt controller is created.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/ppc/pnv.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 72cfe4c2627c..b3b0dd44582f 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -822,9 +822,8 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
     }
 }
 
-static void pnv_chip_realize(DeviceState *dev, Error **errp)
+static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
 {
-    PnvChip *chip = PNV_CHIP(dev);
     Error *error = NULL;
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
     const char *typename = pnv_chip_core_typename(chip);
@@ -836,14 +835,6 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* XSCOM bridge */
-    pnv_xscom_realize(chip, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 0, PNV_XSCOM_BASE(chip));
-
     /* Cores */
     pnv_chip_core_sanitize(chip, &error);
     if (error) {
@@ -891,6 +882,27 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
                                 &PNV_CORE(pnv_core)->xscom_regs);
         i++;
     }
+}
+
+static void pnv_chip_realize(DeviceState *dev, Error **errp)
+{
+    PnvChip *chip = PNV_CHIP(dev);
+    Error *error = NULL;
+
+    /* XSCOM bridge */
+    pnv_xscom_realize(chip, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 0, PNV_XSCOM_BASE(chip));
+
+    /* Cores */
+    pnv_chip_core_realize(chip, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
 
     /* Create LPC controller */
     object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
-- 
2.13.6

[Qemu-devel] [PATCH 5/6] ppc/pnv: introduce a new intc_create() operation to the chip model

[Cédric Le Goater]

On Power9, the thread interrupt presenter has a different type and is
linked to the chip owning the cores.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h |  1 +
 hw/ppc/pnv.c         | 21 +++++++++++++++++++--
 hw/ppc/pnv_core.c    | 18 +++++++++---------
 3 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 90759240a7b1..e934e84f555e 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -76,6 +76,7 @@ typedef struct PnvChipClass {
     hwaddr       xscom_base;
 
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
+    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
 } PnvChipClass;
 
 #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index b3b0dd44582f..7d99366daf90 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -641,6 +641,13 @@ static uint32_t pnv_chip_core_pir_p8(PnvChip *chip, uint32_t core_id)
     return (chip->chip_id << 7) | (core_id << 3);
 }
 
+static Object *pnv_chip_power8_intc_create(PnvChip *chip, Object *child,
+                                           Error **errp)
+{
+    return icp_create(child, TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
+                      errp);
+}
+
 /*
  *    0:48  Reserved - Read as zeroes
  *   49:52  Node ID
@@ -656,6 +663,12 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
     return (chip->chip_id << 8) | (core_id << 2);
 }
 
+static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
+                                           Error **errp)
+{
+    return NULL;
+}
+
 /* Allowed core identifiers on a POWER8 Processor Chip :
  *
  * <EX0 reserved>
@@ -691,6 +704,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8E";
 }
@@ -704,6 +718,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
@@ -717,6 +732,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
+    k->intc_create = pnv_chip_power8_intc_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
@@ -730,6 +746,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
+    k->intc_create = pnv_chip_power9_intc_create;
     k->xscom_base = 0x00603fc00000000ull;
     dc->desc = "PowerNV Chip POWER9";
 }
@@ -865,8 +882,8 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
         object_property_set_int(OBJECT(pnv_core),
                                 pcc->core_pir(chip, core_hwid),
                                 "pir", &error_fatal);
-        object_property_add_const_link(OBJECT(pnv_core), "xics",
-                                       qdev_get_machine(), &error_fatal);
+        object_property_add_const_link(OBJECT(pnv_core), "chip",
+                                       OBJECT(chip), &error_fatal);
         object_property_set_bool(OBJECT(pnv_core), true, "realized",
                                  &error_fatal);
         object_unref(OBJECT(pnv_core));
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 13ad7d9e0470..5805bcd10abf 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -121,11 +121,12 @@ 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_core_realize_child(Object *child, PnvChip *chip, Error **errp)
 {
     Error *local_err = NULL;
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
@@ -133,7 +134,7 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
         return;
     }
 
-    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
+    cpu->intc = pcc->intc_create(chip, child, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -156,13 +157,12 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     void *obj;
     int i, j;
     char name[32];
-    Object *xi;
+    Object *chip;
 
-    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
-    if (!xi) {
-        error_setg(errp, "%s: required link 'xics' not found: %s",
-                   __func__, error_get_pretty(local_err));
-        return;
+    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
+    if (!chip) {
+        error_propagate(errp, local_err);
+        error_prepend(errp, "required link 'chip' not found: ");
     }
 
     pc->threads = g_malloc0(size * cc->nr_threads);
@@ -184,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;
 
-        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
+        pnv_core_realize_child(obj, PNV_CHIP(chip), &local_err);
         if (local_err) {
             goto err;
         }
-- 
2.13.6

[Qemu-devel] [PATCH 6/6] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

This is a major reshuffle of the PowerNV machine and chip models to
introduce a machine type per processor. It is quite noisy but it
doesn't change much the code flow.

It introduces a base PnvChip class from which the specific processor
chip classes, Pnv8Chip and Pnv9Chip, inherit. Each of them needs to
define an init and a realize routine which will create the controllers
of the target processor.

For the moment, the base PnvChip class handles the XSCOM bus and the
cores but the core creation will surely move to the specific processor
chip classes because of the new XIVE interrupt controller in Power9.
The base class also has pointers on the main controllers of the chip
which are common to all processors : PSI, LPC, OCC. These controllers
have some subtil differences in each processor version, but, globally,
they are very similar and provide the same feature. This is how we
have a console for instance.

>From there, we introduce two different machines : "powernv8" and
"powernv9" but, a part from the XICSFabric interface, this is not
strictly needed as it is the cpu type which determines the PnvChip
class. Something to discuss.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 include/hw/ppc/pnv.h |  28 ++++-
 hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
 hw/ppc/pnv_lpc.c     |   2 +-
 3 files changed, 236 insertions(+), 116 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index e934e84f555e..4942add9458a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -57,12 +57,37 @@ typedef struct PnvChip {
     MemoryRegion xscom_mmio;
     MemoryRegion xscom;
     AddressSpace xscom_as;
+
+    /* Base class controllers */
+    PnvLpcController *lpc;
+    PnvPsi       *psi;
+    PnvOCC       *occ;
+} PnvChip;
+
+#define TYPE_PNV8_CHIP "pnv8-chip"
+#define PNV8_CHIP(obj) OBJECT_CHECK(Pnv8Chip, (obj), TYPE_PNV8_CHIP)
+
+typedef struct Pnv8Chip {
+    /*< private >*/
+    PnvChip      parent_obj;
+
+    /*< public >*/
     MemoryRegion icp_mmio;
 
     PnvLpcController lpc;
     PnvPsi       psi;
     PnvOCC       occ;
-} PnvChip;
+} Pnv8Chip;
+
+#define TYPE_PNV9_CHIP "pnv9-chip"
+#define PNV9_CHIP(obj) OBJECT_CHECK(Pnv9Chip, (obj), TYPE_PNV9_CHIP)
+
+typedef struct Pnv9Chip {
+    /*< private >*/
+    PnvChip      parent_obj;
+
+    /*< public >*/
+} Pnv9Chip;
 
 typedef struct PnvChipClass {
     /*< private >*/
@@ -75,6 +100,7 @@ typedef struct PnvChipClass {
 
     hwaddr       xscom_base;
 
+    void (*realize)(PnvChip *chip, Error **errp);
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
     Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
 } PnvChipClass;
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index 7d99366daf90..60b56c7fe07b 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -267,7 +267,7 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
 
 static int pnv_chip_lpc_offset(PnvChip *chip, void *fdt)
 {
-    return fdt_path_offset(fdt, chip->lpc.isa_bus_name);
+    return fdt_path_offset(fdt, chip->lpc->isa_bus_name);
 }
 
 static void pnv_dt_chip(PnvChip *chip, void *fdt)
@@ -516,7 +516,7 @@ static ISABus *pnv_isa_create(PnvChip *chip)
 {
     PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
-    return pnv_lpc_isa_create(&chip->lpc, pcc->chip_type);
+    return pnv_lpc_isa_create(chip->lpc, pcc->chip_type);
 }
 
 static void pnv_init(MachineState *machine)
@@ -695,6 +695,106 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
  */
 #define POWER9_CORE_MASK   (0xffffffffffffffull)
 
+static void pnv_chip_power8_initfn(Object *obj)
+{
+    Pnv8Chip *chip8 = PNV8_CHIP(obj);
+    PnvChip *chip = PNV_CHIP(obj);
+
+    object_initialize(&chip8->lpc, sizeof(chip8->lpc), TYPE_PNV_LPC);
+    object_property_add_child(obj, "lpc", OBJECT(&chip8->lpc), NULL);
+
+    object_initialize(&chip8->psi, sizeof(chip8->psi), TYPE_PNV_PSI);
+    object_property_add_child(obj, "psi", OBJECT(&chip8->psi), NULL);
+    object_property_add_const_link(OBJECT(&chip8->psi), "xics",
+                                   OBJECT(qdev_get_machine()), &error_abort);
+
+    object_initialize(&chip8->occ, sizeof(chip8->occ), TYPE_PNV_OCC);
+    object_property_add_child(obj, "occ", OBJECT(&chip8->occ), NULL);
+    object_property_add_const_link(OBJECT(&chip8->occ), "psi",
+                                   OBJECT(&chip8->psi), &error_abort);
+
+    /*
+     * The LPC controller needs a few things from the chip : to know
+     * if it's primary and PSI to generate interrupts
+     */
+    object_property_add_const_link(OBJECT(&chip8->lpc), "chip",
+                                   OBJECT(chip8), &error_abort);
+
+    /* Intialize the controllers in the base class also */
+    chip->lpc = &chip8->lpc;
+    chip->psi = &chip8->psi;
+    chip->occ = &chip8->occ;
+}
+
+static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
+ {
+    PnvChip *chip = PNV_CHIP(chip8);
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
+    const char *typename = pnv_chip_core_typename(chip);
+    size_t typesize = object_type_get_instance_size(typename);
+    int i, j;
+    char *name;
+    XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
+
+    name = g_strdup_printf("icp-%x", chip->chip_id);
+    memory_region_init(&chip8->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip8->icp_mmio);
+    g_free(name);
+
+    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 1, PNV_ICP_BASE(chip));
+
+    /* Map the ICP registers for each thread */
+    for (i = 0; i < chip->nr_cores; i++) {
+        PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
+        int core_hwid = CPU_CORE(pnv_core)->core_id;
+
+        for (j = 0; j < CPU_CORE(pnv_core)->nr_threads; j++) {
+            uint32_t pir = pcc->core_pir(chip, core_hwid) + j;
+            PnvICPState *icp = PNV_ICP(xics_icp_get(xi, pir));
+
+            memory_region_add_subregion(&chip8->icp_mmio, pir << 12,
+                                        &icp->mmio);
+        }
+    }
+}
+
+static void pnv_chip_power8_realize(PnvChip *chip, Error **errp)
+ {
+    Pnv8Chip *chip8 = PNV8_CHIP(chip);
+    Error *error = NULL;
+
+    /* Create LPC controller */
+    object_property_set_bool(OBJECT(&chip8->lpc), true, "realized",
+                             &error_fatal);
+    pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
+
+    /* Interrupt Management Area. This is the memory region holding
+     * all the Interrupt Control Presenter (ICP) registers */
+    pnv_chip_icp_realize(chip8, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+
+    /* Processor Service Interface (PSI) Host Bridge */
+    object_property_set_int(OBJECT(&chip8->psi), PNV_PSIHB_BASE(chip),
+                            "bar", &error_fatal);
+    object_property_set_bool(OBJECT(&chip8->psi), true, "realized", &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    pnv_xscom_add_subregion(chip, PNV_XSCOM_PSIHB_BASE, &chip8->psi.xscom_regs);
+
+    /* Create the simplified OCC model */
+    object_property_set_bool(OBJECT(&chip8->occ), true, "realized", &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    pnv_xscom_add_subregion(chip, PNV_XSCOM_OCC_BASE, &chip8->occ.xscom_regs);
+}
+
 static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -719,6 +819,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->realize  = pnv_chip_power8_realize;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
@@ -733,10 +834,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->realize  = pnv_chip_power8_realize;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
 
+static void pnv_chip_power9_initfn(Object *obj)
+{
+}
+
+static void pnv_chip_power9_realize(PnvChip *chip, Error **errp)
+{
+
+}
+
 static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -747,6 +858,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER9_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p9;
     k->intc_create = pnv_chip_power9_intc_create;
+    k->realize  = pnv_chip_power9_realize;
     k->xscom_base = 0x00603fc00000000ull;
     dc->desc = "PowerNV Chip POWER9";
 }
@@ -781,62 +893,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
     }
 }
 
-static void pnv_chip_init(Object *obj)
+static void pnv_chip_initfn(Object *obj)
 {
-    PnvChip *chip = PNV_CHIP(obj);
-    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
-
-    chip->xscom_base = pcc->xscom_base;
-
-    object_initialize(&chip->lpc, sizeof(chip->lpc), TYPE_PNV_LPC);
-    object_property_add_child(obj, "lpc", OBJECT(&chip->lpc), NULL);
-
-    object_initialize(&chip->psi, sizeof(chip->psi), TYPE_PNV_PSI);
-    object_property_add_child(obj, "psi", OBJECT(&chip->psi), NULL);
-    object_property_add_const_link(OBJECT(&chip->psi), "xics",
-                                   OBJECT(qdev_get_machine()), &error_abort);
-
-    object_initialize(&chip->occ, sizeof(chip->occ), TYPE_PNV_OCC);
-    object_property_add_child(obj, "occ", OBJECT(&chip->occ), NULL);
-    object_property_add_const_link(OBJECT(&chip->occ), "psi",
-                                   OBJECT(&chip->psi), &error_abort);
-
-    /*
-     * The LPC controller needs a few things from the chip : to know
-     * if it's primary and PSI to generate interrupts
-     */
-    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
-                                   OBJECT(chip), &error_abort);
-}
-
-static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
-{
-    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
-    const char *typename = pnv_chip_core_typename(chip);
-    size_t typesize = object_type_get_instance_size(typename);
-    int i, j;
-    char *name;
-    XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
-
-    name = g_strdup_printf("icp-%x", chip->chip_id);
-    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
-    sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
-    g_free(name);
-
-    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 1, PNV_ICP_BASE(chip));
-
-    /* Map the ICP registers for each thread */
-    for (i = 0; i < chip->nr_cores; i++) {
-        PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
-        int core_hwid = CPU_CORE(pnv_core)->core_id;
-
-        for (j = 0; j < CPU_CORE(pnv_core)->nr_threads; j++) {
-            uint32_t pir = pcc->core_pir(chip, core_hwid) + j;
-            PnvICPState *icp = PNV_ICP(xics_icp_get(xi, pir));
-
-            memory_region_add_subregion(&chip->icp_mmio, pir << 12, &icp->mmio);
-        }
-    }
+    PNV_CHIP(obj)->xscom_base = PNV_CHIP_GET_CLASS(obj)->xscom_base;
 }
 
 static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
@@ -904,6 +963,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
 static void pnv_chip_realize(DeviceState *dev, Error **errp)
 {
     PnvChip *chip = PNV_CHIP(dev);
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
     Error *error = NULL;
 
     /* XSCOM bridge */
@@ -921,36 +981,7 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    /* Create LPC controller */
-    object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
-                             &error_fatal);
-    pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
-
-    /* Interrupt Management Area. This is the memory region holding
-     * all the Interrupt Control Presenter (ICP) registers */
-    pnv_chip_icp_realize(chip, &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-
-    /* Processor Service Interface (PSI) Host Bridge */
-    object_property_set_int(OBJECT(&chip->psi), PNV_PSIHB_BASE(chip),
-                            "bar", &error_fatal);
-    object_property_set_bool(OBJECT(&chip->psi), true, "realized", &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-    pnv_xscom_add_subregion(chip, PNV_XSCOM_PSIHB_BASE, &chip->psi.xscom_regs);
-
-    /* Create the simplified OCC model */
-    object_property_set_bool(OBJECT(&chip->occ), true, "realized", &error);
-    if (error) {
-        error_propagate(errp, error);
-        return;
-    }
-    pnv_xscom_add_subregion(chip, PNV_XSCOM_OCC_BASE, &chip->occ.xscom_regs);
+    pcc->realize(chip, errp);
 }
 
 static Property pnv_chip_properties[] = {
@@ -972,26 +1003,29 @@ static void pnv_chip_class_init(ObjectClass *klass, void *data)
     dc->desc = "PowerNV Chip";
 }
 
-static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
+static ICSState *pnv8_ics_get(XICSFabric *xi, int irq)
 {
     PnvMachineState *pnv = PNV_MACHINE(xi);
     int i;
 
     for (i = 0; i < pnv->num_chips; i++) {
-        if (ics_valid_irq(&pnv->chips[i]->psi.ics, irq)) {
-            return &pnv->chips[i]->psi.ics;
+        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
+
+        if (ics_valid_irq(&chip->psi.ics, irq)) {
+            return &chip->psi.ics;
         }
     }
     return NULL;
 }
 
-static void pnv_ics_resend(XICSFabric *xi)
+static void pnv8_ics_resend(XICSFabric *xi)
 {
     PnvMachineState *pnv = PNV_MACHINE(xi);
     int i;
 
     for (i = 0; i < pnv->num_chips; i++) {
-        ics_resend(&pnv->chips[i]->psi.ics);
+        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
+        ics_resend(&chip->psi.ics);
     }
 }
 
@@ -1011,15 +1045,14 @@ static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
     return NULL;
 }
 
-static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
+static ICPState *pnv8_icp_get(XICSFabric *xi, int pir)
 {
     PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
 
     return cpu ? ICP(cpu->intc) : NULL;
 }
 
-static void pnv_pic_print_info(InterruptStatsProvider *obj,
-                               Monitor *mon)
+static void pnv8_pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
 {
     PnvMachineState *pnv = PNV_MACHINE(obj);
     int i;
@@ -1032,7 +1065,8 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
     }
 
     for (i = 0; i < pnv->num_chips; i++) {
-        ics_pic_print_info(&pnv->chips[i]->psi.ics, mon);
+        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
+        ics_pic_print_info(&chip->psi.ics, mon);
     }
 }
 
@@ -1086,8 +1120,6 @@ static void pnv_machine_class_props_init(ObjectClass *oc)
 static void pnv_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
-    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
-    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
 
     mc->desc = "IBM PowerNV (Non-Virtualized)";
     mc->init = pnv_init;
@@ -1099,48 +1131,110 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
     mc->default_boot_order = NULL;
     mc->default_ram_size = 1 * G_BYTE;
-    xic->icp_get = pnv_icp_get;
-    xic->ics_get = pnv_ics_get;
-    xic->ics_resend = pnv_ics_resend;
-    ispc->print_info = pnv_pic_print_info;
 
     pnv_machine_class_props_init(oc);
 }
 
-#define DEFINE_PNV_CHIP_TYPE(type, class_initfn) \
-    {                                            \
-        .name          = type,                   \
-        .class_init    = class_initfn,           \
-        .parent        = TYPE_PNV_CHIP,          \
+static void pnv8_machine_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
+    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
+
+    /* Power8 is the default */
+    mc->alias = "powernv";
+    mc->is_default = 1;
+
+    xic->icp_get = pnv8_icp_get;
+    xic->ics_get = pnv8_ics_get;
+    xic->ics_resend = pnv8_ics_resend;
+    ispc->print_info = pnv8_pic_print_info;
+}
+
+static void pnv9_machine_class_init(ObjectClass *oc, void *data)
+{
+}
+
+#define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \
+    {                                             \
+        .name          = type,                    \
+        .class_init    = class_initfn,            \
+        .parent        = TYPE_PNV8_CHIP,          \
+    }
+
+#define DEFINE_PNV9_CHIP_TYPE(type, class_initfn) \
+    {                                             \
+        .name          = type,                    \
+        .class_init    = class_initfn,            \
+        .parent        = TYPE_PNV9_CHIP,          \
     }
 
 static const TypeInfo types[] = {
+    /*
+     * PowerNV machines and variants
+     */
     {
         .name          = TYPE_PNV_MACHINE,
         .parent        = TYPE_MACHINE,
+        .abstract      = true,
         .instance_size = sizeof(PnvMachineState),
         .instance_init = pnv_machine_initfn,
         .class_init    = pnv_machine_class_init,
         .interfaces = (InterfaceInfo[]) {
-            { TYPE_XICS_FABRIC },
             { TYPE_INTERRUPT_STATS_PROVIDER },
             { },
         },
     },
     {
+        .name          = MACHINE_TYPE_NAME("powernv9"),
+        .parent        = TYPE_PNV_MACHINE,
+        .class_init    = pnv9_machine_class_init,
+    },
+    {
+        .name          = MACHINE_TYPE_NAME("powernv8"),
+        .parent        = TYPE_PNV_MACHINE,
+        .class_init    = pnv8_machine_class_init,
+        .interfaces    = (InterfaceInfo[]) {
+            { TYPE_XICS_FABRIC },
+            { },
+        },
+    },
+
+    /* Power Chip */
+    {
         .name          = TYPE_PNV_CHIP,
         .parent        = TYPE_SYS_BUS_DEVICE,
         .class_init    = pnv_chip_class_init,
-        .instance_init = pnv_chip_init,
+        .instance_init = pnv_chip_initfn,
         .instance_size = sizeof(PnvChip),
         .class_size    = sizeof(PnvChipClass),
         .abstract      = true,
     },
-    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER9, pnv_chip_power9_class_init),
-    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8, pnv_chip_power8_class_init),
-    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8E, pnv_chip_power8e_class_init),
-    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8NVL,
-                         pnv_chip_power8nvl_class_init),
+
+    /*
+     * P9 chips and variants
+     */
+    {
+        .name          = TYPE_PNV9_CHIP,
+        .parent        = TYPE_PNV_CHIP,
+        .instance_init = pnv_chip_power9_initfn,
+        .instance_size = sizeof(Pnv9Chip),
+    },
+    DEFINE_PNV9_CHIP_TYPE(TYPE_PNV_CHIP_POWER9, pnv_chip_power9_class_init),
+
+    /*
+     * P8 chips and variants
+     */
+    {
+        .name          = TYPE_PNV8_CHIP,
+        .parent        = TYPE_PNV_CHIP,
+        .instance_init = pnv_chip_power8_initfn,
+        .instance_size = sizeof(Pnv8Chip),
+    },
+    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8, pnv_chip_power8_class_init),
+    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8E, pnv_chip_power8e_class_init),
+    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8NVL,
+                          pnv_chip_power8nvl_class_init),
 };
 
 DEFINE_TYPES(types)
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 7f13c4bcf52c..265d94c3240d 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -435,7 +435,7 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
         return;
     }
     chip = PNV_CHIP(obj);
-    lpc->psi = &chip->psi;
+    lpc->psi = chip->psi;
     lpc->primary = chip->chip_id == 0;
 
     /* Reg inits */
-- 
2.13.6

Re: [Qemu-devel] [PATCH 1/6] ppc/pnv: introduce a 'primary' field under the LPC model

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:38PM +0200, Cédric Le Goater wrote:
> When a PowerNV system is started, the firmware (skiboot) looks for a
> "primary" property to determine which LPC bus is the default on a
> multichip system. This property is currently populated in the main
> routine creating the device tree of a chip, which is the not the right
> place to do so.
> 
> Check the chip id to flag the LPC controller as "primary", or not, and
> use that to add the property in the LPC device tree routine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

This doesn't seem particularly useful to me.  Is there ever likely to
be a case where we want the primary LPC to be something other than the
one on chip 0?

If not, we seem to be just creating properties and states and a bunch
of stuff just to cache a (chip# == 0) test.

> ---
>  include/hw/ppc/pnv_lpc.h |  2 ++
>  hw/ppc/pnv.c             | 19 ++++++-------------
>  hw/ppc/pnv_lpc.c         | 29 ++++++++++++++++++++---------
>  3 files changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 53fdd5bb6450..fddcb1c054b3 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -68,6 +68,8 @@ typedef struct PnvLpcController {
>  
>      /* PSI to generate interrupts */
>      PnvPsi *psi;
> +
> +    bool   primary;
>  } PnvLpcController;
>  
>  qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..b419d3323100 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -285,16 +285,6 @@ static void pnv_dt_chip(PnvChip *chip, void *fdt)
>  
>      pnv_dt_xscom(chip, fdt, 0);
>  
> -    /* The default LPC bus of a multichip system is on chip 0. It's
> -     * recognized by the firmware (skiboot) using a "primary"
> -     * property.
> -     */
> -    if (chip->chip_id == 0x0) {
> -        int lpc_offset = pnv_chip_lpc_offset(chip, fdt);
> -
> -        _FDT((fdt_setprop(fdt, lpc_offset, "primary", NULL, 0)));
> -    }
> -
>      for (i = 0; i < chip->nr_cores; i++) {
>          PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
>  
> @@ -814,9 +804,12 @@ static void pnv_chip_init(Object *obj)
>      object_property_add_const_link(OBJECT(&chip->occ), "psi",
>                                     OBJECT(&chip->psi), &error_abort);
>  
> -    /* The LPC controller needs PSI to generate interrupts */
> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
> -                                   OBJECT(&chip->psi), &error_abort);
> +    /*
> +     * The LPC controller needs a few things from the chip : to know
> +     * if it's primary and PSI to generate interrupts
> +     */
> +    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
> +                                   OBJECT(chip), &error_abort);
>  }
>  
>  static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 402c4fefa886..1e70c8c19d52 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -113,6 +113,14 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
>      _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
> +
> +    /*
> +     * The default LPC bus of a multichip system is on chip 0. It's
> +     * recognized by the firmware (skiboot) using a "primary" property.
> +     */
> +    if (PNV_LPC(dev)->primary) {
> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
> +    }
>      return 0;
>  }
>  
> @@ -416,6 +424,18 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      PnvLpcController *lpc = PNV_LPC(dev);
>      Object *obj;
>      Error *error = NULL;
> +    PnvChip *chip;
> +
> +    /* get PSI object from chip */
> +    obj = object_property_get_link(OBJECT(dev), "chip", &error);
> +    if (!obj) {
> +        error_propagate(errp, error);
> +        error_prepend(errp, "required link 'chip' not found: ");
> +        return;
> +    }
> +    chip = PNV_CHIP(obj);
> +    lpc->psi = &chip->psi;
> +    lpc->primary = chip->chip_id == 0;
>  
>      /* Reg inits */
>      lpc->lpc_hc_fw_rd_acc_size = LPC_HC_FW_RD_4B;
> @@ -460,15 +480,6 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
>                            &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
>                            PNV_XSCOM_LPC_SIZE);
> -
> -    /* get PSI object from chip */
> -    obj = object_property_get_link(OBJECT(dev), "psi", &error);
> -    if (!obj) {
> -        error_setg(errp, "%s: required link 'psi' not found: %s",
> -                   __func__, error_get_pretty(error));
> -        return;
> -    }
> -    lpc->psi = PNV_PSI(obj);
>  }
>  
>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)

-- 
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/6] ppc/pnv: move the details of the ISA bus creation under the LPC model

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:39PM +0200, Cédric Le Goater wrote:
> This is a small cleanup to hide to the machine the gory details of the
> creation of the ISA bus. When time comes, the 'qemu_irq_handler' should
> become a LPC controller class attribute.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/pnv_lpc.h |  3 +--
>  hw/ppc/pnv.c             | 15 +--------------
>  hw/ppc/pnv_lpc.c         | 24 ++++++++++++++++++++----
>  3 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index fddcb1c054b3..fb4b7b83d798 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -72,7 +72,6 @@ typedef struct PnvLpcController {
>      bool   primary;
>  } PnvLpcController;
>  
> -qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
> -                                 int nirqs);
> +ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type);
>  
>  #endif /* _PPC_PNV_LPC_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b419d3323100..d2126ee4affc 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -521,22 +521,9 @@ static void pnv_reset(void)
>  
>  static ISABus *pnv_isa_create(PnvChip *chip)
>  {
> -    PnvLpcController *lpc = &chip->lpc;
> -    ISABus *isa_bus;
> -    qemu_irq *irqs;
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
> -    /* let isa_bus_new() create its own bridge on SysBus otherwise
> -     * devices speficied on the command line won't find the bus and
> -     * will fail to create.
> -     */
> -    isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io,
> -                          &error_fatal);
> -
> -    irqs = pnv_lpc_isa_irq_create(lpc, pcc->chip_type, ISA_NUM_IRQS);
> -
> -    isa_bus_irqs(isa_bus, irqs);
> -    return isa_bus;
> +    return pnv_lpc_isa_create(&chip->lpc, pcc->chip_type);
>  }

This 1-line wrapper doesn't look useful.  Why not just call
pnv_lpc_isa_create() from this thing's caller?

>  
>  static void pnv_init(MachineState *machine)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 1e70c8c19d52..7c6c012d5176 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -22,6 +22,7 @@
>  #include "target/ppc/cpu.h"
>  #include "qapi/error.h"
>  #include "qemu/log.h"
> +#include "hw/isa/isa.h"
>  
>  #include "hw/ppc/pnv.h"
>  #include "hw/ppc/pnv_lpc.h"
> @@ -546,16 +547,31 @@ static void pnv_lpc_isa_irq_handler(void *opaque, int n, int level)
>      }
>  }
>  
> -qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
> -                                 int nirqs)
> +ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type)
>  {
> +    ISABus *isa_bus;
> +    qemu_irq *irqs;
> +    qemu_irq_handler handler;
> +
> +    /* let isa_bus_new() create its own bridge on SysBus otherwise
> +     * devices speficied on the command line won't find the bus and
> +     * will fail to create.
> +     */
> +    isa_bus = isa_bus_new(NULL, &lpc->isa_mem, &lpc->isa_io,
> +                          &error_fatal);
> +
>      /* Not all variants have a working serial irq decoder. If not,
>       * handling of LPC interrupts becomes a platform issue (some
>       * platforms have a CPLD to do it).
>       */
>      if (chip_type == PNV_CHIP_POWER8NVL) {
> -        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler, lpc, nirqs);
> +        handler = pnv_lpc_isa_irq_handler;
>      } else {
> -        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler_cpld, lpc, nirqs);
> +        handler = pnv_lpc_isa_irq_handler_cpld;
>      }
> +
> +    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
> +
> +    isa_bus_irqs(isa_bus, irqs);
> +    return isa_bus;
>  }

-- 
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/6] ppc/pnv: introduce an 'isa_bus_name' field under the LPC model

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:40PM +0200, Cédric Le Goater wrote:
> This is again a small cleanup to hide to the machine the details of
> the ISA bus. The ISA bus device tree nodename will be different on
> Power9.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>



> ---
>  include/hw/ppc/pnv_lpc.h | 1 +
>  hw/ppc/pnv.c             | 9 +--------
>  hw/ppc/pnv_lpc.c         | 4 ++++
>  3 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index fb4b7b83d798..e8f7dcb9bfe9 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -70,6 +70,7 @@ typedef struct PnvLpcController {
>      PnvPsi *psi;
>  
>      bool   primary;
> +    char   *isa_bus_name;

I'd suggest putting 'dt' somewhere in the field to make it more
obvious that this is about the bus's path in the device tree, rather
than say the bus's QOM path name.

>  } PnvLpcController;
>  
>  ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, int chip_type);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index d2126ee4affc..72cfe4c2627c 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -267,14 +267,7 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>  
>  static int pnv_chip_lpc_offset(PnvChip *chip, void *fdt)
>  {
> -    char *name;
> -    int offset;
> -
> -    name = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
> -                           (uint64_t) PNV_XSCOM_BASE(chip), PNV_XSCOM_LPC_BASE);
> -    offset = fdt_path_offset(fdt, name);
> -    g_free(name);
> -    return offset;
> +    return fdt_path_offset(fdt, chip->lpc.isa_bus_name);
>  }

Having reduced this wrapper to 1 line, I'm not sure there is any
remaining point to it.

>  
>  static void pnv_dt_chip(PnvChip *chip, void *fdt)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 7c6c012d5176..7f13c4bcf52c 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -481,6 +481,10 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>      pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
>                            &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
>                            PNV_XSCOM_LPC_SIZE);
> +
> +    lpc->isa_bus_name = g_strdup_printf("/xscom@%" PRIx64 "/isa@%x",
> +                                        (uint64_t) PNV_XSCOM_BASE(chip),
> +                                        PNV_XSCOM_LPC_BASE);
>  }
>  
>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)

-- 
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 4/6] ppc/pnv: introduce a pnv_chip_core_realize() routine

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:41PM +0200, Cédric Le Goater wrote:
> This extracts from the PvChip realize routine the part creating the
> cores. On Power9, we will need to create the cores after the Xive
> interrupt controller is created.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
>  hw/ppc/pnv.c | 32 ++++++++++++++++++++++----------
>  1 file changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 72cfe4c2627c..b3b0dd44582f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -822,9 +822,8 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>      }
>  }
>  
> -static void pnv_chip_realize(DeviceState *dev, Error **errp)
> +static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>  {
> -    PnvChip *chip = PNV_CHIP(dev);
>      Error *error = NULL;
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>      const char *typename = pnv_chip_core_typename(chip);
> @@ -836,14 +835,6 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    /* XSCOM bridge */
> -    pnv_xscom_realize(chip, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 0, PNV_XSCOM_BASE(chip));
> -
>      /* Cores */
>      pnv_chip_core_sanitize(chip, &error);
>      if (error) {
> @@ -891,6 +882,27 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>                                  &PNV_CORE(pnv_core)->xscom_regs);
>          i++;
>      }
> +}
> +
> +static void pnv_chip_realize(DeviceState *dev, Error **errp)
> +{
> +    PnvChip *chip = PNV_CHIP(dev);
> +    Error *error = NULL;
> +
> +    /* XSCOM bridge */
> +    pnv_xscom_realize(chip, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 0, PNV_XSCOM_BASE(chip));
> +
> +    /* Cores */
> +    pnv_chip_core_realize(chip, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
>  
>      /* Create LPC controller */
>      object_property_set_bool(OBJECT(&chip->lpc), true, "realized",

-- 
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 5/6] ppc/pnv: introduce a new intc_create() operation to the chip model

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:42PM +0200, Cédric Le Goater wrote:
> On Power9, the thread interrupt presenter has a different type and is
> linked to the chip owning the cores.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

(but not applied for now, since it depends on earlier patches I had
comments on)

> ---
>  include/hw/ppc/pnv.h |  1 +
>  hw/ppc/pnv.c         | 21 +++++++++++++++++++--
>  hw/ppc/pnv_core.c    | 18 +++++++++---------
>  3 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7b1..e934e84f555e 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -76,6 +76,7 @@ typedef struct PnvChipClass {
>      hwaddr       xscom_base;
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> +    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index b3b0dd44582f..7d99366daf90 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -641,6 +641,13 @@ static uint32_t pnv_chip_core_pir_p8(PnvChip *chip, uint32_t core_id)
>      return (chip->chip_id << 7) | (core_id << 3);
>  }
>  
> +static Object *pnv_chip_power8_intc_create(PnvChip *chip, Object *child,
> +                                           Error **errp)
> +{
> +    return icp_create(child, TYPE_PNV_ICP, XICS_FABRIC(qdev_get_machine()),
> +                      errp);
> +}
> +
>  /*
>   *    0:48  Reserved - Read as zeroes
>   *   49:52  Node ID
> @@ -656,6 +663,12 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>      return (chip->chip_id << 8) | (core_id << 2);
>  }
>  
> +static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
> +                                           Error **errp)
> +{
> +    return NULL;
> +}
> +
>  /* Allowed core identifiers on a POWER8 Processor Chip :
>   *
>   * <EX0 reserved>
> @@ -691,6 +704,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x221ef04980000000ull;  /* P8 Murano DD2.1 */
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
> @@ -704,6 +718,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8";
>  }
> @@ -717,6 +732,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x120d304980000000ull;  /* P8 Naples DD1.0 */
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
> +    k->intc_create = pnv_chip_power8_intc_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
> @@ -730,6 +746,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
> +    k->intc_create = pnv_chip_power9_intc_create;
>      k->xscom_base = 0x00603fc00000000ull;
>      dc->desc = "PowerNV Chip POWER9";
>  }
> @@ -865,8 +882,8 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>          object_property_set_int(OBJECT(pnv_core),
>                                  pcc->core_pir(chip, core_hwid),
>                                  "pir", &error_fatal);
> -        object_property_add_const_link(OBJECT(pnv_core), "xics",
> -                                       qdev_get_machine(), &error_fatal);
> +        object_property_add_const_link(OBJECT(pnv_core), "chip",
> +                                       OBJECT(chip), &error_fatal);
>          object_property_set_bool(OBJECT(pnv_core), true, "realized",
>                                   &error_fatal);
>          object_unref(OBJECT(pnv_core));
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 13ad7d9e0470..5805bcd10abf 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -121,11 +121,12 @@ 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_core_realize_child(Object *child, PnvChip *chip, Error **errp)
>  {
>      Error *local_err = NULL;
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
> @@ -133,7 +134,7 @@ static void pnv_core_realize_child(Object *child, XICSFabric *xi, Error **errp)
>          return;
>      }
>  
> -    cpu->intc = icp_create(child, TYPE_PNV_ICP, xi, &local_err);
> +    cpu->intc = pcc->intc_create(chip, child, &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -156,13 +157,12 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      void *obj;
>      int i, j;
>      char name[32];
> -    Object *xi;
> +    Object *chip;
>  
> -    xi = object_property_get_link(OBJECT(dev), "xics", &local_err);
> -    if (!xi) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> -                   __func__, error_get_pretty(local_err));
> -        return;
> +    chip = object_property_get_link(OBJECT(dev), "chip", &local_err);
> +    if (!chip) {
> +        error_propagate(errp, local_err);
> +        error_prepend(errp, "required link 'chip' not found: ");
>      }
>  
>      pc->threads = g_malloc0(size * cc->nr_threads);
> @@ -184,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;
>  
> -        pnv_core_realize_child(obj, XICS_FABRIC(xi), &local_err);
> +        pnv_core_realize_child(obj, PNV_CHIP(chip), &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 6/6] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[David Gibson]

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

On Thu, Jun 14, 2018 at 04:00:43PM +0200, Cédric Le Goater wrote:
> This is a major reshuffle of the PowerNV machine and chip models to
> introduce a machine type per processor. It is quite noisy but it
> doesn't change much the code flow.
> 
> It introduces a base PnvChip class from which the specific processor
> chip classes, Pnv8Chip and Pnv9Chip, inherit. Each of them needs to
> define an init and a realize routine which will create the controllers
> of the target processor.
> 
> For the moment, the base PnvChip class handles the XSCOM bus and the
> cores but the core creation will surely move to the specific processor
> chip classes because of the new XIVE interrupt controller in Power9.
> The base class also has pointers on the main controllers of the chip
> which are common to all processors : PSI, LPC, OCC. These controllers
> have some subtil differences in each processor version, but, globally,
> they are very similar and provide the same feature. This is how we
> have a console for instance.
> 
> >From there, we introduce two different machines : "powernv8" and
> "powernv9" but, a part from the XICSFabric interface, this is not
> strictly needed as it is the cpu type which determines the PnvChip
> class. Something to discuss.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/pnv.h |  28 ++++-
>  hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
>  hw/ppc/pnv_lpc.c     |   2 +-
>  3 files changed, 236 insertions(+), 116 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index e934e84f555e..4942add9458a 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -57,12 +57,37 @@ typedef struct PnvChip {
>      MemoryRegion xscom_mmio;
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
> +
> +    /* Base class controllers */
> +    PnvLpcController *lpc;
> +    PnvPsi       *psi;
> +    PnvOCC       *occ;

Having pointers here to full structures in the subclass seems
bizarrely pointless.

> +} PnvChip;
> +
> +#define TYPE_PNV8_CHIP "pnv8-chip"
> +#define PNV8_CHIP(obj) OBJECT_CHECK(Pnv8Chip, (obj), TYPE_PNV8_CHIP)
> +
> +typedef struct Pnv8Chip {
> +    /*< private >*/
> +    PnvChip      parent_obj;
> +
> +    /*< public >*/
>      MemoryRegion icp_mmio;
>  
>      PnvLpcController lpc;
>      PnvPsi       psi;
>      PnvOCC       occ;
> -} PnvChip;
> +} Pnv8Chip;
> +
> +#define TYPE_PNV9_CHIP "pnv9-chip"
> +#define PNV9_CHIP(obj) OBJECT_CHECK(Pnv9Chip, (obj), TYPE_PNV9_CHIP)
> +
> +typedef struct Pnv9Chip {
> +    /*< private >*/
> +    PnvChip      parent_obj;
> +
> +    /*< public >*/
> +} Pnv9Chip;
>  
>  typedef struct PnvChipClass {
>      /*< private >*/
> @@ -75,6 +100,7 @@ typedef struct PnvChipClass {
>  
>      hwaddr       xscom_base;
>  
> +    void (*realize)(PnvChip *chip, Error **errp);
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>      Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>  } PnvChipClass;
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 7d99366daf90..60b56c7fe07b 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -267,7 +267,7 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
>  
>  static int pnv_chip_lpc_offset(PnvChip *chip, void *fdt)
>  {
> -    return fdt_path_offset(fdt, chip->lpc.isa_bus_name);
> +    return fdt_path_offset(fdt, chip->lpc->isa_bus_name);
>  }
>  
>  static void pnv_dt_chip(PnvChip *chip, void *fdt)
> @@ -516,7 +516,7 @@ static ISABus *pnv_isa_create(PnvChip *chip)
>  {
>      PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
> -    return pnv_lpc_isa_create(&chip->lpc, pcc->chip_type);
> +    return pnv_lpc_isa_create(chip->lpc, pcc->chip_type);
>  }
>  
>  static void pnv_init(MachineState *machine)
> @@ -695,6 +695,106 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
>   */
>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>  
> +static void pnv_chip_power8_initfn(Object *obj)

I'm trying to standardize on *_instance_init() for instance_init functions.

> +{
> +    Pnv8Chip *chip8 = PNV8_CHIP(obj);
> +    PnvChip *chip = PNV_CHIP(obj);
> +
> +    object_initialize(&chip8->lpc, sizeof(chip8->lpc), TYPE_PNV_LPC);
> +    object_property_add_child(obj, "lpc", OBJECT(&chip8->lpc), NULL);

Especially when you *also* have these child link properties which
would let you get to the sub-objects from the base ckass,

> +    object_initialize(&chip8->psi, sizeof(chip8->psi), TYPE_PNV_PSI);
> +    object_property_add_child(obj, "psi", OBJECT(&chip8->psi), NULL);
> +    object_property_add_const_link(OBJECT(&chip8->psi), "xics",
> +                                   OBJECT(qdev_get_machine()), &error_abort);
> +
> +    object_initialize(&chip8->occ, sizeof(chip8->occ), TYPE_PNV_OCC);
> +    object_property_add_child(obj, "occ", OBJECT(&chip8->occ), NULL);
> +    object_property_add_const_link(OBJECT(&chip8->occ), "psi",
> +                                   OBJECT(&chip8->psi), &error_abort);
> +
> +    /*
> +     * The LPC controller needs a few things from the chip : to know
> +     * if it's primary and PSI to generate interrupts
> +     */
> +    object_property_add_const_link(OBJECT(&chip8->lpc), "chip",
> +                                   OBJECT(chip8), &error_abort);
> +
> +    /* Intialize the controllers in the base class also */
> +    chip->lpc = &chip8->lpc;
> +    chip->psi = &chip8->psi;
> +    chip->occ = &chip8->occ;
> +}
> +
> +static void pnv_chip_icp_realize(Pnv8Chip *chip8, Error **errp)
> + {
> +    PnvChip *chip = PNV_CHIP(chip8);
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const char *typename = pnv_chip_core_typename(chip);
> +    size_t typesize = object_type_get_instance_size(typename);
> +    int i, j;
> +    char *name;
> +    XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
> +
> +    name = g_strdup_printf("icp-%x", chip->chip_id);
> +    memory_region_init(&chip8->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip8->icp_mmio);
> +    g_free(name);
> +
> +    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 1, PNV_ICP_BASE(chip));
> +
> +    /* Map the ICP registers for each thread */
> +    for (i = 0; i < chip->nr_cores; i++) {
> +        PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
> +        int core_hwid = CPU_CORE(pnv_core)->core_id;
> +
> +        for (j = 0; j < CPU_CORE(pnv_core)->nr_threads; j++) {
> +            uint32_t pir = pcc->core_pir(chip, core_hwid) + j;
> +            PnvICPState *icp = PNV_ICP(xics_icp_get(xi, pir));
> +
> +            memory_region_add_subregion(&chip8->icp_mmio, pir << 12,
> +                                        &icp->mmio);
> +        }
> +    }
> +}
> +
> +static void pnv_chip_power8_realize(PnvChip *chip, Error **errp)
> + {
> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +    Error *error = NULL;
> +
> +    /* Create LPC controller */
> +    object_property_set_bool(OBJECT(&chip8->lpc), true, "realized",
> +                             &error_fatal);
> +    pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip8->lpc.xscom_regs);
> +
> +    /* Interrupt Management Area. This is the memory region holding
> +     * all the Interrupt Control Presenter (ICP) registers */
> +    pnv_chip_icp_realize(chip8, &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +
> +    /* Processor Service Interface (PSI) Host Bridge */
> +    object_property_set_int(OBJECT(&chip8->psi), PNV_PSIHB_BASE(chip),
> +                            "bar", &error_fatal);
> +    object_property_set_bool(OBJECT(&chip8->psi), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    pnv_xscom_add_subregion(chip, PNV_XSCOM_PSIHB_BASE, &chip8->psi.xscom_regs);
> +
> +    /* Create the simplified OCC model */
> +    object_property_set_bool(OBJECT(&chip8->occ), true, "realized", &error);
> +    if (error) {
> +        error_propagate(errp, error);
> +        return;
> +    }
> +    pnv_xscom_add_subregion(chip, PNV_XSCOM_OCC_BASE, &chip8->occ.xscom_regs);
> +}
> +
>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -719,6 +819,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->realize  = pnv_chip_power8_realize;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8";
>  }
> @@ -733,10 +834,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->realize  = pnv_chip_power8_realize;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
>  
> +static void pnv_chip_power9_initfn(Object *obj)
> +{
> +}
> +
> +static void pnv_chip_power9_realize(PnvChip *chip, Error **errp)
> +{
> +
> +}
> +
>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -747,6 +858,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER9_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p9;
>      k->intc_create = pnv_chip_power9_intc_create;
> +    k->realize  = pnv_chip_power9_realize;
>      k->xscom_base = 0x00603fc00000000ull;
>      dc->desc = "PowerNV Chip POWER9";
>  }
> @@ -781,62 +893,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>      }
>  }
>  
> -static void pnv_chip_init(Object *obj)
> +static void pnv_chip_initfn(Object *obj)
>  {
> -    PnvChip *chip = PNV_CHIP(obj);
> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> -
> -    chip->xscom_base = pcc->xscom_base;
> -
> -    object_initialize(&chip->lpc, sizeof(chip->lpc), TYPE_PNV_LPC);
> -    object_property_add_child(obj, "lpc", OBJECT(&chip->lpc), NULL);
> -
> -    object_initialize(&chip->psi, sizeof(chip->psi), TYPE_PNV_PSI);
> -    object_property_add_child(obj, "psi", OBJECT(&chip->psi), NULL);
> -    object_property_add_const_link(OBJECT(&chip->psi), "xics",
> -                                   OBJECT(qdev_get_machine()), &error_abort);
> -
> -    object_initialize(&chip->occ, sizeof(chip->occ), TYPE_PNV_OCC);
> -    object_property_add_child(obj, "occ", OBJECT(&chip->occ), NULL);
> -    object_property_add_const_link(OBJECT(&chip->occ), "psi",
> -                                   OBJECT(&chip->psi), &error_abort);
> -
> -    /*
> -     * The LPC controller needs a few things from the chip : to know
> -     * if it's primary and PSI to generate interrupts
> -     */
> -    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
> -                                   OBJECT(chip), &error_abort);
> -}
> -
> -static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
> -{
> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> -    const char *typename = pnv_chip_core_typename(chip);
> -    size_t typesize = object_type_get_instance_size(typename);
> -    int i, j;
> -    char *name;
> -    XICSFabric *xi = XICS_FABRIC(qdev_get_machine());
> -
> -    name = g_strdup_printf("icp-%x", chip->chip_id);
> -    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE);
> -    sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
> -    g_free(name);
> -
> -    sysbus_mmio_map(SYS_BUS_DEVICE(chip), 1, PNV_ICP_BASE(chip));
> -
> -    /* Map the ICP registers for each thread */
> -    for (i = 0; i < chip->nr_cores; i++) {
> -        PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
> -        int core_hwid = CPU_CORE(pnv_core)->core_id;
> -
> -        for (j = 0; j < CPU_CORE(pnv_core)->nr_threads; j++) {
> -            uint32_t pir = pcc->core_pir(chip, core_hwid) + j;
> -            PnvICPState *icp = PNV_ICP(xics_icp_get(xi, pir));
> -
> -            memory_region_add_subregion(&chip->icp_mmio, pir << 12, &icp->mmio);
> -        }
> -    }
> +    PNV_CHIP(obj)->xscom_base = PNV_CHIP_GET_CLASS(obj)->xscom_base;
>  }
>  
>  static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
> @@ -904,6 +963,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp)
>  static void pnv_chip_realize(DeviceState *dev, Error **errp)
>  {
>      PnvChip *chip = PNV_CHIP(dev);
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>      Error *error = NULL;
>  
>      /* XSCOM bridge */
> @@ -921,36 +981,7 @@ static void pnv_chip_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    /* Create LPC controller */
> -    object_property_set_bool(OBJECT(&chip->lpc), true, "realized",
> -                             &error_fatal);
> -    pnv_xscom_add_subregion(chip, PNV_XSCOM_LPC_BASE, &chip->lpc.xscom_regs);
> -
> -    /* Interrupt Management Area. This is the memory region holding
> -     * all the Interrupt Control Presenter (ICP) registers */
> -    pnv_chip_icp_realize(chip, &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -
> -    /* Processor Service Interface (PSI) Host Bridge */
> -    object_property_set_int(OBJECT(&chip->psi), PNV_PSIHB_BASE(chip),
> -                            "bar", &error_fatal);
> -    object_property_set_bool(OBJECT(&chip->psi), true, "realized", &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -    pnv_xscom_add_subregion(chip, PNV_XSCOM_PSIHB_BASE, &chip->psi.xscom_regs);
> -
> -    /* Create the simplified OCC model */
> -    object_property_set_bool(OBJECT(&chip->occ), true, "realized", &error);
> -    if (error) {
> -        error_propagate(errp, error);
> -        return;
> -    }
> -    pnv_xscom_add_subregion(chip, PNV_XSCOM_OCC_BASE, &chip->occ.xscom_regs);
> +    pcc->realize(chip, errp);
>  }
>  
>  static Property pnv_chip_properties[] = {
> @@ -972,26 +1003,29 @@ static void pnv_chip_class_init(ObjectClass *klass, void *data)
>      dc->desc = "PowerNV Chip";
>  }
>  
> -static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
> +static ICSState *pnv8_ics_get(XICSFabric *xi, int irq)
>  {
>      PnvMachineState *pnv = PNV_MACHINE(xi);
>      int i;
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> -        if (ics_valid_irq(&pnv->chips[i]->psi.ics, irq)) {
> -            return &pnv->chips[i]->psi.ics;
> +        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
> +
> +        if (ics_valid_irq(&chip->psi.ics, irq)) {
> +            return &chip->psi.ics;
>          }
>      }
>      return NULL;
>  }
>  
> -static void pnv_ics_resend(XICSFabric *xi)
> +static void pnv8_ics_resend(XICSFabric *xi)
>  {
>      PnvMachineState *pnv = PNV_MACHINE(xi);
>      int i;
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> -        ics_resend(&pnv->chips[i]->psi.ics);
> +        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
> +        ics_resend(&chip->psi.ics);
>      }
>  }
>  
> @@ -1011,15 +1045,14 @@ static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
>      return NULL;
>  }
>  
> -static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> +static ICPState *pnv8_icp_get(XICSFabric *xi, int pir)
>  {
>      PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
>  
>      return cpu ? ICP(cpu->intc) : NULL;
>  }
>  
> -static void pnv_pic_print_info(InterruptStatsProvider *obj,
> -                               Monitor *mon)
> +static void pnv8_pic_print_info(InterruptStatsProvider *obj, Monitor *mon)
>  {
>      PnvMachineState *pnv = PNV_MACHINE(obj);
>      int i;
> @@ -1032,7 +1065,8 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj,
>      }
>  
>      for (i = 0; i < pnv->num_chips; i++) {
> -        ics_pic_print_info(&pnv->chips[i]->psi.ics, mon);
> +        Pnv8Chip *chip = PNV8_CHIP(pnv->chips[i]);
> +        ics_pic_print_info(&chip->psi.ics, mon);
>      }
>  }
>  
> @@ -1086,8 +1120,6 @@ static void pnv_machine_class_props_init(ObjectClass *oc)
>  static void pnv_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> -    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> -    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
>  
>      mc->desc = "IBM PowerNV (Non-Virtualized)";
>      mc->init = pnv_init;
> @@ -1099,48 +1131,110 @@ static void pnv_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_boot_order = NULL;
>      mc->default_ram_size = 1 * G_BYTE;
> -    xic->icp_get = pnv_icp_get;
> -    xic->ics_get = pnv_ics_get;
> -    xic->ics_resend = pnv_ics_resend;
> -    ispc->print_info = pnv_pic_print_info;
>  
>      pnv_machine_class_props_init(oc);
>  }
>  
> -#define DEFINE_PNV_CHIP_TYPE(type, class_initfn) \
> -    {                                            \
> -        .name          = type,                   \
> -        .class_init    = class_initfn,           \
> -        .parent        = TYPE_PNV_CHIP,          \
> +static void pnv8_machine_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    XICSFabricClass *xic = XICS_FABRIC_CLASS(oc);
> +    InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc);
> +
> +    /* Power8 is the default */
> +    mc->alias = "powernv";
> +    mc->is_default = 1;
> +
> +    xic->icp_get = pnv8_icp_get;
> +    xic->ics_get = pnv8_ics_get;
> +    xic->ics_resend = pnv8_ics_resend;
> +    ispc->print_info = pnv8_pic_print_info;
> +}
> +
> +static void pnv9_machine_class_init(ObjectClass *oc, void *data)
> +{
> +}
> +
> +#define DEFINE_PNV8_CHIP_TYPE(type, class_initfn) \
> +    {                                             \
> +        .name          = type,                    \
> +        .class_init    = class_initfn,            \
> +        .parent        = TYPE_PNV8_CHIP,          \
> +    }
> +
> +#define DEFINE_PNV9_CHIP_TYPE(type, class_initfn) \
> +    {                                             \
> +        .name          = type,                    \
> +        .class_init    = class_initfn,            \
> +        .parent        = TYPE_PNV9_CHIP,          \
>      }
>  
>  static const TypeInfo types[] = {
> +    /*
> +     * PowerNV machines and variants
> +     */
>      {
>          .name          = TYPE_PNV_MACHINE,
>          .parent        = TYPE_MACHINE,
> +        .abstract      = true,
>          .instance_size = sizeof(PnvMachineState),
>          .instance_init = pnv_machine_initfn,
>          .class_init    = pnv_machine_class_init,
>          .interfaces = (InterfaceInfo[]) {
> -            { TYPE_XICS_FABRIC },
>              { TYPE_INTERRUPT_STATS_PROVIDER },
>              { },
>          },
>      },
>      {
> +        .name          = MACHINE_TYPE_NAME("powernv9"),
> +        .parent        = TYPE_PNV_MACHINE,
> +        .class_init    = pnv9_machine_class_init,
> +    },
> +    {
> +        .name          = MACHINE_TYPE_NAME("powernv8"),
> +        .parent        = TYPE_PNV_MACHINE,
> +        .class_init    = pnv8_machine_class_init,
> +        .interfaces    = (InterfaceInfo[]) {
> +            { TYPE_XICS_FABRIC },
> +            { },
> +        },
> +    },
> +
> +    /* Power Chip */
> +    {
>          .name          = TYPE_PNV_CHIP,
>          .parent        = TYPE_SYS_BUS_DEVICE,
>          .class_init    = pnv_chip_class_init,
> -        .instance_init = pnv_chip_init,
> +        .instance_init = pnv_chip_initfn,
>          .instance_size = sizeof(PnvChip),
>          .class_size    = sizeof(PnvChipClass),
>          .abstract      = true,
>      },
> -    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER9, pnv_chip_power9_class_init),
> -    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8, pnv_chip_power8_class_init),
> -    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8E, pnv_chip_power8e_class_init),
> -    DEFINE_PNV_CHIP_TYPE(TYPE_PNV_CHIP_POWER8NVL,
> -                         pnv_chip_power8nvl_class_init),
> +
> +    /*
> +     * P9 chips and variants
> +     */
> +    {
> +        .name          = TYPE_PNV9_CHIP,
> +        .parent        = TYPE_PNV_CHIP,
> +        .instance_init = pnv_chip_power9_initfn,
> +        .instance_size = sizeof(Pnv9Chip),
> +    },
> +    DEFINE_PNV9_CHIP_TYPE(TYPE_PNV_CHIP_POWER9, pnv_chip_power9_class_init),
> +
> +    /*
> +     * P8 chips and variants
> +     */
> +    {
> +        .name          = TYPE_PNV8_CHIP,
> +        .parent        = TYPE_PNV_CHIP,
> +        .instance_init = pnv_chip_power8_initfn,
> +        .instance_size = sizeof(Pnv8Chip),
> +    },
> +    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8, pnv_chip_power8_class_init),
> +    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8E, pnv_chip_power8e_class_init),
> +    DEFINE_PNV8_CHIP_TYPE(TYPE_PNV_CHIP_POWER8NVL,
> +                          pnv_chip_power8nvl_class_init),
>  };
>  
>  DEFINE_TYPES(types)
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 7f13c4bcf52c..265d94c3240d 100644
> --- a/hw/ppc/pnv_lpc.c
> +++ b/hw/ppc/pnv_lpc.c
> @@ -435,7 +435,7 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>      chip = PNV_CHIP(obj);
> -    lpc->psi = &chip->psi;
> +    lpc->psi = chip->psi;
>      lpc->primary = chip->chip_id == 0;
>  
>      /* Reg inits */

-- 
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 1/6] ppc/pnv: introduce a 'primary' field under the LPC model

[Cédric Le Goater]

On 06/15/2018 04:38 AM, David Gibson wrote:
> On Thu, Jun 14, 2018 at 04:00:38PM +0200, Cédric Le Goater wrote:
>> When a PowerNV system is started, the firmware (skiboot) looks for a
>> "primary" property to determine which LPC bus is the default on a
>> multichip system. This property is currently populated in the main
>> routine creating the device tree of a chip, which is the not the right
>> place to do so.
>>
>> Check the chip id to flag the LPC controller as "primary", or not, and
>> use that to add the property in the LPC device tree routine.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> This doesn't seem particularly useful to me.  Is there ever likely to
> be a case where we want the primary LPC to be something other than the
> one on chip 0?
> 
> If not, we seem to be just creating properties and states and a bunch
> of stuff just to cache a (chip# == 0) test.

knowing that the LPC controller is on chip0 is important, that's how
we build the ISA bus of the machine and I am trying to untangle this
relation : 

	pnv->isa_bus <-> pnv->chip->lpc->regions 

The chip is in the middle and it is problematic to introduce an 
abstract PnvChip class. 

Still working on it.

Thanks,

C. 


>> ---
>>  include/hw/ppc/pnv_lpc.h |  2 ++
>>  hw/ppc/pnv.c             | 19 ++++++-------------
>>  hw/ppc/pnv_lpc.c         | 29 ++++++++++++++++++++---------
>>  3 files changed, 28 insertions(+), 22 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
>> index 53fdd5bb6450..fddcb1c054b3 100644
>> --- a/include/hw/ppc/pnv_lpc.h
>> +++ b/include/hw/ppc/pnv_lpc.h
>> @@ -68,6 +68,8 @@ typedef struct PnvLpcController {
>>  
>>      /* PSI to generate interrupts */
>>      PnvPsi *psi;
>> +
>> +    bool   primary;
>>  } PnvLpcController;
>>  
>>  qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 031488131629..b419d3323100 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -285,16 +285,6 @@ static void pnv_dt_chip(PnvChip *chip, void *fdt)
>>  
>>      pnv_dt_xscom(chip, fdt, 0);
>>  
>> -    /* The default LPC bus of a multichip system is on chip 0. It's
>> -     * recognized by the firmware (skiboot) using a "primary"
>> -     * property.
>> -     */
>> -    if (chip->chip_id == 0x0) {
>> -        int lpc_offset = pnv_chip_lpc_offset(chip, fdt);
>> -
>> -        _FDT((fdt_setprop(fdt, lpc_offset, "primary", NULL, 0)));
>> -    }
>> -
>>      for (i = 0; i < chip->nr_cores; i++) {
>>          PnvCore *pnv_core = PNV_CORE(chip->cores + i * typesize);
>>  
>> @@ -814,9 +804,12 @@ static void pnv_chip_init(Object *obj)
>>      object_property_add_const_link(OBJECT(&chip->occ), "psi",
>>                                     OBJECT(&chip->psi), &error_abort);
>>  
>> -    /* The LPC controller needs PSI to generate interrupts */
>> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
>> -                                   OBJECT(&chip->psi), &error_abort);
>> +    /*
>> +     * The LPC controller needs a few things from the chip : to know
>> +     * if it's primary and PSI to generate interrupts
>> +     */
>> +    object_property_add_const_link(OBJECT(&chip->lpc), "chip",
>> +                                   OBJECT(chip), &error_abort);
>>  }
>>  
>>  static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
>> index 402c4fefa886..1e70c8c19d52 100644
>> --- a/hw/ppc/pnv_lpc.c
>> +++ b/hw/ppc/pnv_lpc.c
>> @@ -113,6 +113,14 @@ static int pnv_lpc_dt_xscom(PnvXScomInterface *dev, void *fdt, int xscom_offset)
>>      _FDT((fdt_setprop_cell(fdt, offset, "#address-cells", 2)));
>>      _FDT((fdt_setprop_cell(fdt, offset, "#size-cells", 1)));
>>      _FDT((fdt_setprop(fdt, offset, "compatible", compat, sizeof(compat))));
>> +
>> +    /*
>> +     * The default LPC bus of a multichip system is on chip 0. It's
>> +     * recognized by the firmware (skiboot) using a "primary" property.
>> +     */
>> +    if (PNV_LPC(dev)->primary) {
>> +        _FDT((fdt_setprop(fdt, offset, "primary", NULL, 0)));
>> +    }
>>      return 0;
>>  }
>>  
>> @@ -416,6 +424,18 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>>      PnvLpcController *lpc = PNV_LPC(dev);
>>      Object *obj;
>>      Error *error = NULL;
>> +    PnvChip *chip;
>> +
>> +    /* get PSI object from chip */
>> +    obj = object_property_get_link(OBJECT(dev), "chip", &error);
>> +    if (!obj) {
>> +        error_propagate(errp, error);
>> +        error_prepend(errp, "required link 'chip' not found: ");
>> +        return;
>> +    }
>> +    chip = PNV_CHIP(obj);
>> +    lpc->psi = &chip->psi;
>> +    lpc->primary = chip->chip_id == 0;
>>  
>>      /* Reg inits */
>>      lpc->lpc_hc_fw_rd_acc_size = LPC_HC_FW_RD_4B;
>> @@ -460,15 +480,6 @@ static void pnv_lpc_realize(DeviceState *dev, Error **errp)
>>      pnv_xscom_region_init(&lpc->xscom_regs, OBJECT(dev),
>>                            &pnv_lpc_xscom_ops, lpc, "xscom-lpc",
>>                            PNV_XSCOM_LPC_SIZE);
>> -
>> -    /* get PSI object from chip */
>> -    obj = object_property_get_link(OBJECT(dev), "psi", &error);
>> -    if (!obj) {
>> -        error_setg(errp, "%s: required link 'psi' not found: %s",
>> -                   __func__, error_get_pretty(error));
>> -        return;
>> -    }
>> -    lpc->psi = PNV_PSI(obj);
>>  }
>>  
>>  static void pnv_lpc_class_init(ObjectClass *klass, void *data)
>