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

[Qemu-devel] [PATCH v2 0/4] 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. 

Changes since v1:

 - reworked the ISABus creation interface with the chip, the machine
   is not aware anymore of the chip controllers.
 - removed the bizarre controllers under the PnvChip base class.
 - kept back some changes on the ISA device tree name. They will come
   in time with the LPC controller for P9

Cédric Le Goater (4):
  ppc/pnv: introduce a new intc_create() operation to the chip model
  ppc/pnv: introduce a new isa_create() operation to the chip model
  ppc/pnv: introduce Pnv8Chip and Pnv9Chip models
  ppc/pnv: consolidate the creation of the ISA bus device tree

 include/hw/ppc/pnv.h     |  25 ++-
 include/hw/ppc/pnv_lpc.h |   3 +-
 hw/ppc/pnv.c             | 424 ++++++++++++++++++++++++++++++-----------------
 hw/ppc/pnv_core.c        |  18 +-
 hw/ppc/pnv_lpc.c         |  30 +++-
 5 files changed, 327 insertions(+), 173 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH v2 1/4] 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 0d2b79f7980f..c7e127ae97db 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -671,6 +671,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
@@ -686,6 +693,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>
@@ -721,6 +734,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";
 }
@@ -734,6 +748,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";
 }
@@ -747,6 +762,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";
 }
@@ -760,6 +776,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";
 }
@@ -892,8 +909,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 f7cf33f547a5..a9f129fc2c5f 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -99,13 +99,14 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
+static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, 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;
+    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 
     object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
     if (local_err) {
@@ -113,7 +114,7 @@ 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->intc = pcc->intc_create(chip, OBJECT(cpu), &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         return;
@@ -143,13 +144,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_new(PowerPCCPU *, cc->nr_threads);
@@ -166,7 +166,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
     }
 
     for (j = 0; j < cc->nr_threads; j++) {
-        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
+        pnv_realize_vcpu(pc->threads[j], PNV_CHIP(chip), &local_err);
         if (local_err) {
             goto err;
         }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 1/4] ppc/pnv: introduce a new intc_create() operation to the chip model

[David Gibson]

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

On Fri, Jun 15, 2018 at 05:25:33PM +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>

Applied to ppc-for-3.0, thanks.

> ---
>  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 0d2b79f7980f..c7e127ae97db 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -671,6 +671,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
> @@ -686,6 +693,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>
> @@ -721,6 +734,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";
>  }
> @@ -734,6 +748,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";
>  }
> @@ -747,6 +762,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";
>  }
> @@ -760,6 +776,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";
>  }
> @@ -892,8 +909,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 f7cf33f547a5..a9f129fc2c5f 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -99,13 +99,14 @@ static const MemoryRegionOps pnv_core_xscom_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>  };
>  
> -static void pnv_realize_vcpu(PowerPCCPU *cpu, XICSFabric *xi, Error **errp)
> +static void pnv_realize_vcpu(PowerPCCPU *cpu, PnvChip *chip, 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;
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>  
>      object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
>      if (local_err) {
> @@ -113,7 +114,7 @@ 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->intc = pcc->intc_create(chip, OBJECT(cpu), &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
>          return;
> @@ -143,13 +144,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_new(PowerPCCPU *, cc->nr_threads);
> @@ -166,7 +166,7 @@ static void pnv_core_realize(DeviceState *dev, Error **errp)
>      }
>  
>      for (j = 0; j < cc->nr_threads; j++) {
> -        pnv_realize_vcpu(pc->threads[j], XICS_FABRIC(xi), &local_err);
> +        pnv_realize_vcpu(pc->threads[j], 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 --]

[Qemu-devel] [PATCH v2 2/4] ppc/pnv: introduce a new isa_create() operation to the chip model

[Cédric Le Goater]

This moves the details of the ISA bus creation under the LPC model but
more important, the new PnvChip operation will let us choose the chip
class to use when we introduce the different chip classes for Power9
and Power8. It hides away the processor chip controllers from the
machine.

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

 pnv_isa_create() is a onliner but it looks better like that than to
 use it directly with pnv->chips[0]

 include/hw/ppc/pnv.h     |  1 +
 include/hw/ppc/pnv_lpc.h |  3 +--
 hw/ppc/pnv.c             | 34 +++++++++++++++++++---------------
 hw/ppc/pnv_lpc.c         | 30 +++++++++++++++++++++++++-----
 4 files changed, 46 insertions(+), 22 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index e934e84f555e..563279f3e00c 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -77,6 +77,7 @@ typedef struct PnvChipClass {
 
     uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
     Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
+    ISABus *(*isa_create)(PnvChip *chip, Error **errp);
 } PnvChipClass;
 
 #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
index 53fdd5bb6450..d657489b07ce 100644
--- a/include/hw/ppc/pnv_lpc.h
+++ b/include/hw/ppc/pnv_lpc.h
@@ -70,7 +70,6 @@ typedef struct PnvLpcController {
     PnvPsi *psi;
 } PnvLpcController;
 
-qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
-                                 int nirqs);
+ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp);
 
 #endif /* _PPC_PNV_LPC_H */
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index c7e127ae97db..ac828d133173 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -529,24 +529,24 @@ static void pnv_reset(void)
     cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
 }
 
-static ISABus *pnv_isa_create(PnvChip *chip)
+static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
 {
-    PnvLpcController *lpc = &chip->lpc;
-    ISABus *isa_bus;
-    qemu_irq *irqs;
-    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
+    return pnv_lpc_isa_create(&chip->lpc, true, errp);
+}
 
-    /* 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);
+static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
+{
+    return pnv_lpc_isa_create(&chip->lpc, false, errp);
+}
 
-    irqs = pnv_lpc_isa_irq_create(lpc, pcc->chip_type, ISA_NUM_IRQS);
+static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
+{
+    return NULL;
+}
 
-    isa_bus_irqs(isa_bus, irqs);
-    return isa_bus;
+static ISABus *pnv_isa_create(PnvChip *chip, Error **errp)
+{
+    return PNV_CHIP_GET_CLASS(chip)->isa_create(chip, errp);
 }
 
 static void pnv_init(MachineState *machine)
@@ -646,7 +646,7 @@ static void pnv_init(MachineState *machine)
     g_free(chip_typename);
 
     /* Instantiate ISA bus on chip 0 */
-    pnv->isa_bus = pnv_isa_create(pnv->chips[0]);
+    pnv->isa_bus = pnv_isa_create(pnv->chips[0], &error_fatal);
 
     /* Create serial port */
     serial_hds_isa_init(pnv->isa_bus, 0, MAX_ISA_SERIAL_PORTS);
@@ -735,6 +735,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->cores_mask = POWER8E_CORE_MASK;
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
+    k->isa_create = pnv_chip_power8_isa_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8E";
 }
@@ -749,6 +750,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->isa_create = pnv_chip_power8_isa_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
@@ -763,6 +765,7 @@ 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->isa_create = pnv_chip_power8nvl_isa_create;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
@@ -777,6 +780,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->isa_create = pnv_chip_power9_isa_create;
     k->xscom_base = 0x00603fc00000000ull;
     dc->desc = "PowerNV Chip POWER9";
 }
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 402c4fefa886..d7721320a25b 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"
@@ -535,16 +536,35 @@ 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, bool use_cpld, Error **errp)
 {
+    Error *local_err = NULL;
+    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, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
     /* 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);
+    if (use_cpld) {
+        handler = pnv_lpc_isa_irq_handler_cpld;
     } else {
-        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler_cpld, lpc, nirqs);
+        handler = pnv_lpc_isa_irq_handler;
     }
+
+    irqs = qemu_allocate_irqs(handler, lpc, ISA_NUM_IRQS);
+
+    isa_bus_irqs(isa_bus, irqs);
+    return isa_bus;
 }
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 2/4] ppc/pnv: introduce a new isa_create() operation to the chip model

[David Gibson]

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

On Fri, Jun 15, 2018 at 05:25:34PM +0200, Cédric Le Goater wrote:
> This moves the details of the ISA bus creation under the LPC model but
> more important, the new PnvChip operation will let us choose the chip
> class to use when we introduce the different chip classes for Power9
> and Power8. It hides away the processor chip controllers from the
> machine.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Applied to ppc-for-3.0, thanks.

> ---
> 
>  pnv_isa_create() is a onliner but it looks better like that than to
>  use it directly with pnv->chips[0]
> 
>  include/hw/ppc/pnv.h     |  1 +
>  include/hw/ppc/pnv_lpc.h |  3 +--
>  hw/ppc/pnv.c             | 34 +++++++++++++++++++---------------
>  hw/ppc/pnv_lpc.c         | 30 +++++++++++++++++++++++++-----
>  4 files changed, 46 insertions(+), 22 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index e934e84f555e..563279f3e00c 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -77,6 +77,7 @@ typedef struct PnvChipClass {
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>      Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
> +    ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> diff --git a/include/hw/ppc/pnv_lpc.h b/include/hw/ppc/pnv_lpc.h
> index 53fdd5bb6450..d657489b07ce 100644
> --- a/include/hw/ppc/pnv_lpc.h
> +++ b/include/hw/ppc/pnv_lpc.h
> @@ -70,7 +70,6 @@ typedef struct PnvLpcController {
>      PnvPsi *psi;
>  } PnvLpcController;
>  
> -qemu_irq *pnv_lpc_isa_irq_create(PnvLpcController *lpc, int chip_type,
> -                                 int nirqs);
> +ISABus *pnv_lpc_isa_create(PnvLpcController *lpc, bool use_cpld, Error **errp);
>  
>  #endif /* _PPC_PNV_LPC_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index c7e127ae97db..ac828d133173 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -529,24 +529,24 @@ static void pnv_reset(void)
>      cpu_physical_memory_write(PNV_FDT_ADDR, fdt, fdt_totalsize(fdt));
>  }
>  
> -static ISABus *pnv_isa_create(PnvChip *chip)
> +static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
>  {
> -    PnvLpcController *lpc = &chip->lpc;
> -    ISABus *isa_bus;
> -    qemu_irq *irqs;
> -    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    return pnv_lpc_isa_create(&chip->lpc, true, errp);
> +}
>  
> -    /* 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);
> +static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
> +{
> +    return pnv_lpc_isa_create(&chip->lpc, false, errp);
> +}
>  
> -    irqs = pnv_lpc_isa_irq_create(lpc, pcc->chip_type, ISA_NUM_IRQS);
> +static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
> +{
> +    return NULL;
> +}
>  
> -    isa_bus_irqs(isa_bus, irqs);
> -    return isa_bus;
> +static ISABus *pnv_isa_create(PnvChip *chip, Error **errp)
> +{
> +    return PNV_CHIP_GET_CLASS(chip)->isa_create(chip, errp);
>  }
>  
>  static void pnv_init(MachineState *machine)
> @@ -646,7 +646,7 @@ static void pnv_init(MachineState *machine)
>      g_free(chip_typename);
>  
>      /* Instantiate ISA bus on chip 0 */
> -    pnv->isa_bus = pnv_isa_create(pnv->chips[0]);
> +    pnv->isa_bus = pnv_isa_create(pnv->chips[0], &error_fatal);
>  
>      /* Create serial port */
>      serial_hds_isa_init(pnv->isa_bus, 0, MAX_ISA_SERIAL_PORTS);
> @@ -735,6 +735,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->cores_mask = POWER8E_CORE_MASK;
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
> +    k->isa_create = pnv_chip_power8_isa_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
> @@ -749,6 +750,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->isa_create = pnv_chip_power8_isa_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8";
>  }
> @@ -763,6 +765,7 @@ 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->isa_create = pnv_chip_power8nvl_isa_create;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
> @@ -777,6 +780,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->isa_create = pnv_chip_power9_isa_create;
>      k->xscom_base = 0x00603fc00000000ull;
>      dc->desc = "PowerNV Chip POWER9";
>  }
> diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
> index 402c4fefa886..d7721320a25b 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"
> @@ -535,16 +536,35 @@ 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, bool use_cpld, Error **errp)
>  {
> +    Error *local_err = NULL;
> +    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, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return NULL;
> +    }
> +
>      /* 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);
> +    if (use_cpld) {
> +        handler = pnv_lpc_isa_irq_handler_cpld;
>      } else {
> -        return qemu_allocate_irqs(pnv_lpc_isa_irq_handler_cpld, lpc, nirqs);
> +        handler = pnv_lpc_isa_irq_handler;
>      }
> +
> +    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 --]

[Qemu-devel] [PATCH v2 3/4] 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.

>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 |  23 +++-
 hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 231 insertions(+), 114 deletions(-)

diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 563279f3e00c..244856414580 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -57,12 +57,32 @@ typedef struct PnvChip {
     MemoryRegion xscom_mmio;
     MemoryRegion xscom;
     AddressSpace xscom_as;
+} 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 +95,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);
     ISABus *(*isa_create)(PnvChip *chip, Error **errp);
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index ac828d133173..b416a1a6ed63 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -531,12 +531,14 @@ static void pnv_reset(void)
 
 static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
 {
-    return pnv_lpc_isa_create(&chip->lpc, true, errp);
+    Pnv8Chip *chip8 = PNV8_CHIP(chip);
+    return pnv_lpc_isa_create(&chip8->lpc, true, errp);
 }
 
 static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
 {
-    return pnv_lpc_isa_create(&chip->lpc, false, errp);
+    Pnv8Chip *chip8 = PNV8_CHIP(chip);
+    return pnv_lpc_isa_create(&chip8->lpc, false, errp);
 }
 
 static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
@@ -725,6 +727,97 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
  */
 #define POWER9_CORE_MASK   (0xffffffffffffffull)
 
+static void pnv_chip_power8_instance_init(Object *obj)
+{
+    Pnv8Chip *chip8 = PNV8_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 PSI to generate interrupts */
+    object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
+                                   OBJECT(&chip8->psi), &error_abort);
+}
+
+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);
@@ -736,6 +829,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->isa_create = pnv_chip_power8_isa_create;
+    k->realize  = pnv_chip_power8_realize;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8E";
 }
@@ -751,6 +845,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->isa_create = pnv_chip_power8_isa_create;
+    k->realize  = pnv_chip_power8_realize;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8";
 }
@@ -766,10 +861,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
     k->core_pir = pnv_chip_core_pir_p8;
     k->intc_create = pnv_chip_power8_intc_create;
     k->isa_create = pnv_chip_power8nvl_isa_create;
+    k->realize  = pnv_chip_power8_realize;
     k->xscom_base = 0x003fc0000000000ull;
     dc->desc = "PowerNV Chip POWER8NVL";
 }
 
+static void pnv_chip_power9_instance_init(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);
@@ -781,6 +886,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
     k->core_pir = pnv_chip_core_pir_p9;
     k->intc_create = pnv_chip_power9_intc_create;
     k->isa_create = pnv_chip_power9_isa_create;
+    k->realize  = pnv_chip_power9_realize;
     k->xscom_base = 0x00603fc00000000ull;
     dc->desc = "PowerNV Chip POWER9";
 }
@@ -815,59 +921,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
     }
 }
 
-static void pnv_chip_init(Object *obj)
+static void pnv_chip_instance_init(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 PSI to generate interrupts */
-    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
-                                   OBJECT(&chip->psi), &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)
@@ -935,6 +991,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 */
@@ -952,36 +1009,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[] = {
@@ -1003,26 +1031,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
+
+        if (ics_valid_irq(&chip8->psi.ics, irq)) {
+            return &chip8->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 *chip8 = PNV8_CHIP(pnv->chips[i]);
+        ics_resend(&chip8->psi.ics);
     }
 }
 
@@ -1042,15 +1073,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;
@@ -1063,7 +1093,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
+        ics_pic_print_info(&chip8->psi.ics, mon);
     }
 }
 
@@ -1098,7 +1129,7 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
     pnv->num_chips = num_chips;
 }
 
-static void pnv_machine_initfn(Object *obj)
+static void pnv_machine_instance_init(Object *obj)
 {
     PnvMachineState *pnv = PNV_MACHINE(obj);
     pnv->num_chips = 1;
@@ -1117,8 +1148,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;
@@ -1130,48 +1159,115 @@ 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->desc = "IBM PowerNV (Non-Virtualized) Power8";
+    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)
+{
+   MachineClass *mc = MACHINE_CLASS(oc);
+
+   mc->desc = "IBM PowerNV (Non-Virtualized) Power9";
+   mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
+}
+
+#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,
+        .instance_init = pnv_machine_instance_init,
         .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_instance_init,
         .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_instance_init,
+        .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_instance_init,
+        .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)
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[David Gibson]

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

On Fri, Jun 15, 2018 at 05:25:35PM +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.
> 
> >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.

Yeah.. I'd leave this bit out for now.  It can go into a later patch,
with the machine type actually determining the chip type.

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>  include/hw/ppc/pnv.h |  23 +++-
>  hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
>  2 files changed, 231 insertions(+), 114 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 563279f3e00c..244856414580 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -57,12 +57,32 @@ typedef struct PnvChip {
>      MemoryRegion xscom_mmio;
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
> +} 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 +95,7 @@ typedef struct PnvChipClass {
>  
>      hwaddr       xscom_base;
>  
> +    void (*realize)(PnvChip *chip, Error **errp);

This looks the wrong way round from how things are usually done.
Rather than having the base class realize() call the subclass specific
realize hook, it's more usual for the subclass to set the
dc->realize() and have it call a k->parent_realize() to call up the
chain.  grep for device_class_set_parent_realize() for some more
examples.

As a general rule, giving the overall control flow to the most
specific subclass in the chain tends to work better.

Other than that, this looks good.

>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>      Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index ac828d133173..b416a1a6ed63 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -531,12 +531,14 @@ static void pnv_reset(void)
>  
>  static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
>  {
> -    return pnv_lpc_isa_create(&chip->lpc, true, errp);
> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +    return pnv_lpc_isa_create(&chip8->lpc, true, errp);
>  }
>  
>  static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
>  {
> -    return pnv_lpc_isa_create(&chip->lpc, false, errp);
> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
> +    return pnv_lpc_isa_create(&chip8->lpc, false, errp);
>  }
>  
>  static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
> @@ -725,6 +727,97 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
>   */
>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>  
> +static void pnv_chip_power8_instance_init(Object *obj)
> +{
> +    Pnv8Chip *chip8 = PNV8_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 PSI to generate interrupts */
> +    object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
> +                                   OBJECT(&chip8->psi), &error_abort);
> +}
> +
> +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);
> @@ -736,6 +829,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->isa_create = pnv_chip_power8_isa_create;
> +    k->realize  = pnv_chip_power8_realize;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
> @@ -751,6 +845,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->isa_create = pnv_chip_power8_isa_create;
> +    k->realize  = pnv_chip_power8_realize;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8";
>  }
> @@ -766,10 +861,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>      k->core_pir = pnv_chip_core_pir_p8;
>      k->intc_create = pnv_chip_power8_intc_create;
>      k->isa_create = pnv_chip_power8nvl_isa_create;
> +    k->realize  = pnv_chip_power8_realize;
>      k->xscom_base = 0x003fc0000000000ull;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
>  
> +static void pnv_chip_power9_instance_init(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);
> @@ -781,6 +886,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>      k->core_pir = pnv_chip_core_pir_p9;
>      k->intc_create = pnv_chip_power9_intc_create;
>      k->isa_create = pnv_chip_power9_isa_create;
> +    k->realize  = pnv_chip_power9_realize;
>      k->xscom_base = 0x00603fc00000000ull;
>      dc->desc = "PowerNV Chip POWER9";
>  }
> @@ -815,59 +921,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>      }
>  }
>  
> -static void pnv_chip_init(Object *obj)
> +static void pnv_chip_instance_init(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 PSI to generate interrupts */
> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
> -                                   OBJECT(&chip->psi), &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)
> @@ -935,6 +991,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 */
> @@ -952,36 +1009,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[] = {
> @@ -1003,26 +1031,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> +
> +        if (ics_valid_irq(&chip8->psi.ics, irq)) {
> +            return &chip8->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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> +        ics_resend(&chip8->psi.ics);
>      }
>  }
>  
> @@ -1042,15 +1073,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;
> @@ -1063,7 +1093,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> +        ics_pic_print_info(&chip8->psi.ics, mon);
>      }
>  }
>  
> @@ -1098,7 +1129,7 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
>      pnv->num_chips = num_chips;
>  }
>  
> -static void pnv_machine_initfn(Object *obj)
> +static void pnv_machine_instance_init(Object *obj)
>  {
>      PnvMachineState *pnv = PNV_MACHINE(obj);
>      pnv->num_chips = 1;
> @@ -1117,8 +1148,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;
> @@ -1130,48 +1159,115 @@ 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->desc = "IBM PowerNV (Non-Virtualized) Power8";
> +    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)
> +{
> +   MachineClass *mc = MACHINE_CLASS(oc);
> +
> +   mc->desc = "IBM PowerNV (Non-Virtualized) Power9";
> +   mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> +}
> +
> +#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,
> +        .instance_init = pnv_machine_instance_init,
>          .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_instance_init,
>          .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_instance_init,
> +        .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_instance_init,
> +        .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)

-- 
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 v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

On 06/18/2018 12:38 PM, David Gibson wrote:
> On Fri, Jun 15, 2018 at 05:25:35PM +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.
>>
>> >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.
> 
> Yeah.. I'd leave this bit out for now.  It can go into a later patch,
> with the machine type actually determining the chip type.

yes. There are other issues to fix around the machine type anyway. Like
making sure we cannot start a machine with : -M powernv9 -cpu POWER8
 
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  include/hw/ppc/pnv.h |  23 +++-
>>  hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
>>  2 files changed, 231 insertions(+), 114 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 563279f3e00c..244856414580 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -57,12 +57,32 @@ typedef struct PnvChip {
>>      MemoryRegion xscom_mmio;
>>      MemoryRegion xscom;
>>      AddressSpace xscom_as;
>> +} 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 +95,7 @@ typedef struct PnvChipClass {
>>  
>>      hwaddr       xscom_base;
>>  
>> +    void (*realize)(PnvChip *chip, Error **errp);
> 
> This looks the wrong way round from how things are usually done.
> Rather than having the base class realize() call the subclass specific
> realize hook, it's more usual for the subclass to set the
> dc->realize() and have it call a k->parent_realize() to call up the
> chain.  grep for device_class_set_parent_realize() for some more
> examples.

Ah. That is more to my liking. There are a couple of models following
the wrong object pattern, xics, vio. I will check them.
 
> As a general rule, giving the overall control flow to the most
> specific subclass in the chain tends to work better.

yes. 

> Other than that, this looks good.

OK. I will then reshuffle the patchset and move out of the physmap patch 
on which we don't seem to have the same ideas. It can come later.

After that round, the P8/phb3 and P9/Xive can be the next models.

phb3 would be a good extension to the existing machine. It did some
code rework, so it looks better.

and Xive is a must of have for P9. But to get a P9 machine running, 
we need a couple of more models :
 
	PSI, LPC, OCC, 

nothing complex, and I think they are ready, and some P9 CPU work: 

	32bit decrementer, stop_lite state, radix.

I worked around these for the moment.

Thanks,

C. 

>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>>      Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index ac828d133173..b416a1a6ed63 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -531,12 +531,14 @@ static void pnv_reset(void)
>>  
>>  static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
>>  {
>> -    return pnv_lpc_isa_create(&chip->lpc, true, errp);
>> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
>> +    return pnv_lpc_isa_create(&chip8->lpc, true, errp);
>>  }
>>  
>>  static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
>>  {
>> -    return pnv_lpc_isa_create(&chip->lpc, false, errp);
>> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
>> +    return pnv_lpc_isa_create(&chip8->lpc, false, errp);
>>  }
>>  
>>  static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
>> @@ -725,6 +727,97 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
>>   */
>>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>>  
>> +static void pnv_chip_power8_instance_init(Object *obj)
>> +{
>> +    Pnv8Chip *chip8 = PNV8_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 PSI to generate interrupts */
>> +    object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
>> +                                   OBJECT(&chip8->psi), &error_abort);
>> +}
>> +
>> +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);
>> @@ -736,6 +829,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>      k->core_pir = pnv_chip_core_pir_p8;
>>      k->intc_create = pnv_chip_power8_intc_create;
>>      k->isa_create = pnv_chip_power8_isa_create;
>> +    k->realize  = pnv_chip_power8_realize;
>>      k->xscom_base = 0x003fc0000000000ull;
>>      dc->desc = "PowerNV Chip POWER8E";
>>  }
>> @@ -751,6 +845,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
>>      k->core_pir = pnv_chip_core_pir_p8;
>>      k->intc_create = pnv_chip_power8_intc_create;
>>      k->isa_create = pnv_chip_power8_isa_create;
>> +    k->realize  = pnv_chip_power8_realize;
>>      k->xscom_base = 0x003fc0000000000ull;
>>      dc->desc = "PowerNV Chip POWER8";
>>  }
>> @@ -766,10 +861,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
>>      k->core_pir = pnv_chip_core_pir_p8;
>>      k->intc_create = pnv_chip_power8_intc_create;
>>      k->isa_create = pnv_chip_power8nvl_isa_create;
>> +    k->realize  = pnv_chip_power8_realize;
>>      k->xscom_base = 0x003fc0000000000ull;
>>      dc->desc = "PowerNV Chip POWER8NVL";
>>  }
>>  
>> +static void pnv_chip_power9_instance_init(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);
>> @@ -781,6 +886,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>>      k->core_pir = pnv_chip_core_pir_p9;
>>      k->intc_create = pnv_chip_power9_intc_create;
>>      k->isa_create = pnv_chip_power9_isa_create;
>> +    k->realize  = pnv_chip_power9_realize;
>>      k->xscom_base = 0x00603fc00000000ull;
>>      dc->desc = "PowerNV Chip POWER9";
>>  }
>> @@ -815,59 +921,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>>      }
>>  }
>>  
>> -static void pnv_chip_init(Object *obj)
>> +static void pnv_chip_instance_init(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 PSI to generate interrupts */
>> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
>> -                                   OBJECT(&chip->psi), &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)
>> @@ -935,6 +991,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 */
>> @@ -952,36 +1009,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[] = {
>> @@ -1003,26 +1031,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
>> +
>> +        if (ics_valid_irq(&chip8->psi.ics, irq)) {
>> +            return &chip8->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 *chip8 = PNV8_CHIP(pnv->chips[i]);
>> +        ics_resend(&chip8->psi.ics);
>>      }
>>  }
>>  
>> @@ -1042,15 +1073,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;
>> @@ -1063,7 +1093,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
>> +        ics_pic_print_info(&chip8->psi.ics, mon);
>>      }
>>  }
>>  
>> @@ -1098,7 +1129,7 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
>>      pnv->num_chips = num_chips;
>>  }
>>  
>> -static void pnv_machine_initfn(Object *obj)
>> +static void pnv_machine_instance_init(Object *obj)
>>  {
>>      PnvMachineState *pnv = PNV_MACHINE(obj);
>>      pnv->num_chips = 1;
>> @@ -1117,8 +1148,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;
>> @@ -1130,48 +1159,115 @@ 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->desc = "IBM PowerNV (Non-Virtualized) Power8";
>> +    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)
>> +{
>> +   MachineClass *mc = MACHINE_CLASS(oc);
>> +
>> +   mc->desc = "IBM PowerNV (Non-Virtualized) Power9";
>> +   mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
>> +}
>> +
>> +#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,
>> +        .instance_init = pnv_machine_instance_init,
>>          .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_instance_init,
>>          .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_instance_init,
>> +        .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_instance_init,
>> +        .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)
> 

Re: [Qemu-devel] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[David Gibson]

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

On Mon, Jun 18, 2018 at 01:30:13PM +0200, Cédric Le Goater wrote:
> On 06/18/2018 12:38 PM, David Gibson wrote:
> > On Fri, Jun 15, 2018 at 05:25:35PM +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.
> >>
> >> >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.
> > 
> > Yeah.. I'd leave this bit out for now.  It can go into a later patch,
> > with the machine type actually determining the chip type.
> 
> yes. There are other issues to fix around the machine type anyway. Like
> making sure we cannot start a machine with : -M powernv9 -cpu POWER8
>  
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> ---
> >>  include/hw/ppc/pnv.h |  23 +++-
> >>  hw/ppc/pnv.c         | 322 +++++++++++++++++++++++++++++++++------------------
> >>  2 files changed, 231 insertions(+), 114 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 563279f3e00c..244856414580 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -57,12 +57,32 @@ typedef struct PnvChip {
> >>      MemoryRegion xscom_mmio;
> >>      MemoryRegion xscom;
> >>      AddressSpace xscom_as;
> >> +} 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 +95,7 @@ typedef struct PnvChipClass {
> >>  
> >>      hwaddr       xscom_base;
> >>  
> >> +    void (*realize)(PnvChip *chip, Error **errp);
> > 
> > This looks the wrong way round from how things are usually done.
> > Rather than having the base class realize() call the subclass specific
> > realize hook, it's more usual for the subclass to set the
> > dc->realize() and have it call a k->parent_realize() to call up the
> > chain.  grep for device_class_set_parent_realize() for some more
> > examples.
> 
> Ah. That is more to my liking. There are a couple of models following
> the wrong object pattern, xics, vio. I will check them.

Ah, yeah, probably my ignorance at the time showing.

> > As a general rule, giving the overall control flow to the most
> > specific subclass in the chain tends to work better.
> 
> yes. 
> 
> > Other than that, this looks good.
> 
> OK. I will then reshuffle the patchset and move out of the physmap patch 
> on which we don't seem to have the same ideas. It can come later.
> 
> After that round, the P8/phb3 and P9/Xive can be the next models.
> 
> phb3 would be a good extension to the existing machine. It did some
> code rework, so it looks better.
> 
> and Xive is a must of have for P9. But to get a P9 machine running, 
> we need a couple of more models :
>  
> 	PSI, LPC, OCC, 
> 
> nothing complex, and I think they are ready, and some P9 CPU work: 
> 
> 	32bit decrementer, stop_lite state, radix.
> 
> I worked around these for the moment.
> 
> Thanks,
> 
> C. 
> 
> >>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> >>      Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
> >>      ISABus *(*isa_create)(PnvChip *chip, Error **errp);
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index ac828d133173..b416a1a6ed63 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -531,12 +531,14 @@ static void pnv_reset(void)
> >>  
> >>  static ISABus *pnv_chip_power8_isa_create(PnvChip *chip, Error **errp)
> >>  {
> >> -    return pnv_lpc_isa_create(&chip->lpc, true, errp);
> >> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
> >> +    return pnv_lpc_isa_create(&chip8->lpc, true, errp);
> >>  }
> >>  
> >>  static ISABus *pnv_chip_power8nvl_isa_create(PnvChip *chip, Error **errp)
> >>  {
> >> -    return pnv_lpc_isa_create(&chip->lpc, false, errp);
> >> +    Pnv8Chip *chip8 = PNV8_CHIP(chip);
> >> +    return pnv_lpc_isa_create(&chip8->lpc, false, errp);
> >>  }
> >>  
> >>  static ISABus *pnv_chip_power9_isa_create(PnvChip *chip, Error **errp)
> >> @@ -725,6 +727,97 @@ static Object *pnv_chip_power9_intc_create(PnvChip *chip, Object *child,
> >>   */
> >>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
> >>  
> >> +static void pnv_chip_power8_instance_init(Object *obj)
> >> +{
> >> +    Pnv8Chip *chip8 = PNV8_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 PSI to generate interrupts */
> >> +    object_property_add_const_link(OBJECT(&chip8->lpc), "psi",
> >> +                                   OBJECT(&chip8->psi), &error_abort);
> >> +}
> >> +
> >> +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);
> >> @@ -736,6 +829,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >>      k->core_pir = pnv_chip_core_pir_p8;
> >>      k->intc_create = pnv_chip_power8_intc_create;
> >>      k->isa_create = pnv_chip_power8_isa_create;
> >> +    k->realize  = pnv_chip_power8_realize;
> >>      k->xscom_base = 0x003fc0000000000ull;
> >>      dc->desc = "PowerNV Chip POWER8E";
> >>  }
> >> @@ -751,6 +845,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data)
> >>      k->core_pir = pnv_chip_core_pir_p8;
> >>      k->intc_create = pnv_chip_power8_intc_create;
> >>      k->isa_create = pnv_chip_power8_isa_create;
> >> +    k->realize  = pnv_chip_power8_realize;
> >>      k->xscom_base = 0x003fc0000000000ull;
> >>      dc->desc = "PowerNV Chip POWER8";
> >>  }
> >> @@ -766,10 +861,20 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
> >>      k->core_pir = pnv_chip_core_pir_p8;
> >>      k->intc_create = pnv_chip_power8_intc_create;
> >>      k->isa_create = pnv_chip_power8nvl_isa_create;
> >> +    k->realize  = pnv_chip_power8_realize;
> >>      k->xscom_base = 0x003fc0000000000ull;
> >>      dc->desc = "PowerNV Chip POWER8NVL";
> >>  }
> >>  
> >> +static void pnv_chip_power9_instance_init(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);
> >> @@ -781,6 +886,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >>      k->core_pir = pnv_chip_core_pir_p9;
> >>      k->intc_create = pnv_chip_power9_intc_create;
> >>      k->isa_create = pnv_chip_power9_isa_create;
> >> +    k->realize  = pnv_chip_power9_realize;
> >>      k->xscom_base = 0x00603fc00000000ull;
> >>      dc->desc = "PowerNV Chip POWER9";
> >>  }
> >> @@ -815,59 +921,9 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
> >>      }
> >>  }
> >>  
> >> -static void pnv_chip_init(Object *obj)
> >> +static void pnv_chip_instance_init(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 PSI to generate interrupts */
> >> -    object_property_add_const_link(OBJECT(&chip->lpc), "psi",
> >> -                                   OBJECT(&chip->psi), &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)
> >> @@ -935,6 +991,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 */
> >> @@ -952,36 +1009,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[] = {
> >> @@ -1003,26 +1031,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> >> +
> >> +        if (ics_valid_irq(&chip8->psi.ics, irq)) {
> >> +            return &chip8->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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> >> +        ics_resend(&chip8->psi.ics);
> >>      }
> >>  }
> >>  
> >> @@ -1042,15 +1073,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;
> >> @@ -1063,7 +1093,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 *chip8 = PNV8_CHIP(pnv->chips[i]);
> >> +        ics_pic_print_info(&chip8->psi.ics, mon);
> >>      }
> >>  }
> >>  
> >> @@ -1098,7 +1129,7 @@ static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name,
> >>      pnv->num_chips = num_chips;
> >>  }
> >>  
> >> -static void pnv_machine_initfn(Object *obj)
> >> +static void pnv_machine_instance_init(Object *obj)
> >>  {
> >>      PnvMachineState *pnv = PNV_MACHINE(obj);
> >>      pnv->num_chips = 1;
> >> @@ -1117,8 +1148,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;
> >> @@ -1130,48 +1159,115 @@ 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->desc = "IBM PowerNV (Non-Virtualized) Power8";
> >> +    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)
> >> +{
> >> +   MachineClass *mc = MACHINE_CLASS(oc);
> >> +
> >> +   mc->desc = "IBM PowerNV (Non-Virtualized) Power9";
> >> +   mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0");
> >> +}
> >> +
> >> +#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,
> >> +        .instance_init = pnv_machine_instance_init,
> >>          .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_instance_init,
> >>          .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_instance_init,
> >> +        .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_instance_init,
> >> +        .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)
> > 
> 

-- 
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 v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

>>>  typedef struct PnvChipClass {
>>>      /*< private >*/
>>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
>>>  
>>>      hwaddr       xscom_base;
>>>  
>>> +    void (*realize)(PnvChip *chip, Error **errp);
>>
>> This looks the wrong way round from how things are usually done.
>> Rather than having the base class realize() call the subclass specific
>> realize hook, it's more usual for the subclass to set the
>> dc->realize() and have it call a k->parent_realize() to call up the
>> chain.  grep for device_class_set_parent_realize() for some more
>> examples.
>
> Ah. That is more to my liking. There are a couple of models following
> the wrong object pattern, xics, vio. I will check them.

So XICS is causing some head-aches because the ics-kvm model inherits
from ics-simple which inherits from ics-base. so we have a grand-parent
class to handle.

if we could affiliate ics-kvm directly to ics-base it would make the 
family affair easier. we need to check migration though.  

C.

Re: [Qemu-devel] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[David Gibson]

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

On Tue, Jun 19, 2018 at 07:24:44AM +0200, Cédric Le Goater wrote:
> 
> >>>  typedef struct PnvChipClass {
> >>>      /*< private >*/
> >>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
> >>>  
> >>>      hwaddr       xscom_base;
> >>>  
> >>> +    void (*realize)(PnvChip *chip, Error **errp);
> >>
> >> This looks the wrong way round from how things are usually done.
> >> Rather than having the base class realize() call the subclass specific
> >> realize hook, it's more usual for the subclass to set the
> >> dc->realize() and have it call a k->parent_realize() to call up the
> >> chain.  grep for device_class_set_parent_realize() for some more
> >> examples.
> >
> > Ah. That is more to my liking. There are a couple of models following
> > the wrong object pattern, xics, vio. I will check them.
> 
> So XICS is causing some head-aches because the ics-kvm model inherits
> from ics-simple which inherits from ics-base. so we have a grand-parent
> class to handle.

Ok.  I mean, we should probably switch ics around to use the
parent_realize model, rather than the backwards one it does now.  But
it's not immediately obvious to me why having a grandparent class
breaks things.

> if we could affiliate ics-kvm directly to ics-base it would make the 
> family affair easier. we need to check migration though.

But that said, I've been thinking for a while that it might make sense
to fold ics-kvm into ics-base.  It seems very risky to have two
different object classes that are supposed to have guest-identical
behaviour.  Certainly adding any migratable state to one not the other
would be horribly wrong.

-- 
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 v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

On 06/20/2018 02:56 AM, David Gibson wrote:
> On Tue, Jun 19, 2018 at 07:24:44AM +0200, Cédric Le Goater wrote:
>>
>>>>>  typedef struct PnvChipClass {
>>>>>      /*< private >*/
>>>>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
>>>>>  
>>>>>      hwaddr       xscom_base;
>>>>>  
>>>>> +    void (*realize)(PnvChip *chip, Error **errp);
>>>>
>>>> This looks the wrong way round from how things are usually done.
>>>> Rather than having the base class realize() call the subclass specific
>>>> realize hook, it's more usual for the subclass to set the
>>>> dc->realize() and have it call a k->parent_realize() to call up the
>>>> chain.  grep for device_class_set_parent_realize() for some more
>>>> examples.
>>>
>>> Ah. That is more to my liking. There are a couple of models following
>>> the wrong object pattern, xics, vio. I will check them.
>>
>> So XICS is causing some head-aches because the ics-kvm model inherits
>> from ics-simple which inherits from ics-base. so we have a grand-parent
>> class to handle.
> 
> Ok.  I mean, we should probably switch ics around to use the
> parent_realize model, rather than the backwards one it does now.  But
> it's not immediately obvious to me why having a grandparent class
> breaks things.

If you follow the common realize pattern, you end up with a recursive 
loop with one of the realize routine. I didn't dig much the issue.

>> if we could affiliate ics-kvm directly to ics-base it would make the 
>> family affair easier. we need to check migration though.
> 
> But that said, I've been thinking for a while that it might make sense
> to fold ics-kvm into ics-base.  It seems very risky to have two
> different object classes that are supposed to have guest-identical
> behaviour.  Certainly adding any migratable state to one not the other
> would be horribly wrong.

yes. clearly. something like bellow would be better:

                   +---------------+
                   |     ICS       |
         +---------+  common/base  +--------+
         |         +---------------+        |
         |                                  |
         | spapr                    spapr   |
         |  pnv                             |
  +------v--------+                +--------v--------+
  |     ICS       |                |       ICS       |
  |  simple/QEMU  |                |       KVM       |
  +---------------+                +-----------------+

with only some reset and realize handling in the subclasses. The only
extra field we could add under the KVM class is the KVM XICS device fd.  


Thanks,

C.

Re: [Qemu-devel] [PATCH v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[David Gibson]

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

On Wed, Jun 20, 2018 at 07:29:37AM +0200, Cédric Le Goater wrote:
> On 06/20/2018 02:56 AM, David Gibson wrote:
> > On Tue, Jun 19, 2018 at 07:24:44AM +0200, Cédric Le Goater wrote:
> >>
> >>>>>  typedef struct PnvChipClass {
> >>>>>      /*< private >*/
> >>>>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
> >>>>>  
> >>>>>      hwaddr       xscom_base;
> >>>>>  
> >>>>> +    void (*realize)(PnvChip *chip, Error **errp);
> >>>>
> >>>> This looks the wrong way round from how things are usually done.
> >>>> Rather than having the base class realize() call the subclass specific
> >>>> realize hook, it's more usual for the subclass to set the
> >>>> dc->realize() and have it call a k->parent_realize() to call up the
> >>>> chain.  grep for device_class_set_parent_realize() for some more
> >>>> examples.
> >>>
> >>> Ah. That is more to my liking. There are a couple of models following
> >>> the wrong object pattern, xics, vio. I will check them.
> >>
> >> So XICS is causing some head-aches because the ics-kvm model inherits
> >> from ics-simple which inherits from ics-base. so we have a grand-parent
> >> class to handle.
> > 
> > Ok.  I mean, we should probably switch ics around to use the
> > parent_realize model, rather than the backwards one it does now.  But
> > it's not immediately obvious to me why having a grandparent class
> > breaks things.
> 
> If you follow the common realize pattern, you end up with a recursive 
> loop with one of the realize routine. I didn't dig much the issue.

Hmm.

> >> if we could affiliate ics-kvm directly to ics-base it would make the 
> >> family affair easier. we need to check migration though.
> > 
> > But that said, I've been thinking for a while that it might make sense
> > to fold ics-kvm into ics-base.  It seems very risky to have two
> > different object classes that are supposed to have guest-identical
> > behaviour.  Certainly adding any migratable state to one not the other
> > would be horribly wrong.
> 
> yes. clearly. something like bellow would be better:
> 
>                    +---------------+
>                    |     ICS       |
>          +---------+  common/base  +--------+
>          |         +---------------+        |
>          |                                  |
>          | spapr                    spapr   |
>          |  pnv                             |
>   +------v--------+                +--------v--------+
>   |     ICS       |                |       ICS       |
>   |  simple/QEMU  |                |       KVM       |
>   +---------------+                +-----------------+
> 
> with only some reset and realize handling in the subclasses. The only
> extra field we could add under the KVM class is the KVM XICS device fd.  

I think that would be an improvement over what we have, yes.  However,
it's not what I actually had in mind.  In fact I was planning on
getting rid of the KVM specific subclass entirely and merging it into
the base/simple classes with explicit if (kvm) logic.

The reason is that having different object classes for devices based
on accelerator is unusual and kind of dangerous.  We get away with it
because we don't have any migration information that gets tied to the
object class name - but that's a pretty non-obvious restriction that
would be easy to break in future.

-- 
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 v2 3/4] ppc/pnv: introduce Pnv8Chip and Pnv9Chip models

[Cédric Le Goater]

On 06/25/2018 08:36 AM, David Gibson wrote:
> On Wed, Jun 20, 2018 at 07:29:37AM +0200, Cédric Le Goater wrote:
>> On 06/20/2018 02:56 AM, David Gibson wrote:
>>> On Tue, Jun 19, 2018 at 07:24:44AM +0200, Cédric Le Goater wrote:
>>>>
>>>>>>>  typedef struct PnvChipClass {
>>>>>>>      /*< private >*/
>>>>>>> @@ -75,6 +95,7 @@ typedef struct PnvChipClass {
>>>>>>>  
>>>>>>>      hwaddr       xscom_base;
>>>>>>>  
>>>>>>> +    void (*realize)(PnvChip *chip, Error **errp);
>>>>>>
>>>>>> This looks the wrong way round from how things are usually done.
>>>>>> Rather than having the base class realize() call the subclass specific
>>>>>> realize hook, it's more usual for the subclass to set the
>>>>>> dc->realize() and have it call a k->parent_realize() to call up the
>>>>>> chain.  grep for device_class_set_parent_realize() for some more
>>>>>> examples.
>>>>>
>>>>> Ah. That is more to my liking. There are a couple of models following
>>>>> the wrong object pattern, xics, vio. I will check them.
>>>>
>>>> So XICS is causing some head-aches because the ics-kvm model inherits
>>>> from ics-simple which inherits from ics-base. so we have a grand-parent
>>>> class to handle.
>>>
>>> Ok.  I mean, we should probably switch ics around to use the
>>> parent_realize model, rather than the backwards one it does now.  But
>>> it's not immediately obvious to me why having a grandparent class
>>> breaks things.
>>
>> If you follow the common realize pattern, you end up with a recursive 
>> loop with one of the realize routine. I didn't dig much the issue.
> 
> Hmm.
> 
>>>> if we could affiliate ics-kvm directly to ics-base it would make the 
>>>> family affair easier. we need to check migration though.
>>>
>>> But that said, I've been thinking for a while that it might make sense
>>> to fold ics-kvm into ics-base.  It seems very risky to have two
>>> different object classes that are supposed to have guest-identical
>>> behaviour.  Certainly adding any migratable state to one not the other
>>> would be horribly wrong.
>>
>> yes. clearly. something like bellow would be better:
>>
>>                    +---------------+
>>                    |     ICS       |
>>          +---------+  common/base  +--------+
>>          |         +---------------+        |
>>          |                                  |
>>          | spapr                    spapr   |
>>          |  pnv                             |
>>   +------v--------+                +--------v--------+
>>   |     ICS       |                |       ICS       |
>>   |  simple/QEMU  |                |       KVM       |
>>   +---------------+                +-----------------+
>>
>> with only some reset and realize handling in the subclasses. The only
>> extra field we could add under the KVM class is the KVM XICS device fd.  
> 
> I think that would be an improvement over what we have, yes.  However,
> it's not what I actually had in mind.  In fact I was planning on
> getting rid of the KVM specific subclass entirely and merging it into
> the base/simple classes with explicit if (kvm) logic.

That is one way to do it yes, even if the common pattern used in other 
systems is more like the above. I don't have strong preferences.
 
We should see what is driving our needs. The most complex so far is to 
be able on P9 to switch from XICS to XIVE under KVM. 

> The reason is that having different object classes for devices based
> on accelerator is unusual and kind of dangerous. 

I agree.

> We get away with it
> because we don't have any migration information that gets tied to the
> object class name - but that's a pretty non-obvious restriction that
> would be easy to break in future.

Here is a summary of the KVM needs which applies to both XICS and XIVE,
when not activated both at the same time. 

The source and presenter objects need extra handlers to save/restore and 
synchronize KVM/QEMU states :

  - pre_save()
  - post_load()
  - synchronize_state()

The source object needs :

  - to create the KVM IRQ device (and surrounding support, VMAs for XIVE), 
  - to set up a different qemu_irq handler using the KVM device 

The presenter needs  

  - to attach the KVM vCPU to the KVM IRQ device 
  - a cpu hotplug/unplug tracking mechanism

C.

[Qemu-devel] [PATCH v2 4/4] ppc/pnv: consolidate the creation of the ISA bus device tree

[Cédric Le Goater]

The device tree node of the ISA bus was being partially done in
different places. Move all the nodes creation under the same routine.

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

diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
index b416a1a6ed63..8969ccdd7ba3 100644
--- a/hw/ppc/pnv.c
+++ b/hw/ppc/pnv.c
@@ -265,18 +265,6 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir,
     g_free(reg);
 }
 
-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;
-}
-
 static void pnv_dt_chip(PnvChip *chip, void *fdt)
 {
     const char *typename = pnv_chip_core_typename(chip);
@@ -285,16 +273,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);
 
@@ -418,16 +396,35 @@ static int pnv_dt_isa_device(DeviceState *dev, void *opaque)
     return 0;
 }
 
-static void pnv_dt_isa(ISABus *bus, void *fdt, int lpc_offset)
+static int pnv_chip_isa_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;
+}
+
+/* The default LPC bus of a multichip system is on chip 0. It's
+ * recognized by the firmware (skiboot) using a "primary" property.
+ */
+static void pnv_dt_isa(PnvMachineState *pnv, void *fdt)
 {
+    int isa_offset = pnv_chip_isa_offset(pnv->chips[0], fdt);
     ForeachPopulateArgs args = {
         .fdt = fdt,
-        .offset = lpc_offset,
+        .offset = isa_offset,
     };
 
+    _FDT((fdt_setprop(fdt, isa_offset, "primary", NULL, 0)));
+
     /* ISA devices are not necessarily parented to the ISA bus so we
      * can not use object_child_foreach() */
-    qbus_walk_children(BUS(bus), pnv_dt_isa_device, NULL, NULL, NULL, &args);
+    qbus_walk_children(BUS(pnv->isa_bus), pnv_dt_isa_device, NULL, NULL, NULL,
+                       &args);
 }
 
 static void *pnv_dt_create(MachineState *machine)
@@ -438,7 +435,6 @@ static void *pnv_dt_create(MachineState *machine)
     char *buf;
     int off;
     int i;
-    int lpc_offset;
 
     fdt = g_malloc0(FDT_MAX_SIZE);
     _FDT((fdt_create_empty_tree(fdt, FDT_MAX_SIZE)));
@@ -480,8 +476,7 @@ static void *pnv_dt_create(MachineState *machine)
     }
 
     /* Populate ISA devices on chip 0 */
-    lpc_offset = pnv_chip_lpc_offset(pnv->chips[0], fdt);
-    pnv_dt_isa(pnv->isa_bus, fdt, lpc_offset);
+    pnv_dt_isa(pnv, fdt);
 
     if (pnv->bmc) {
         pnv_dt_bmc_sensors(pnv->bmc, fdt);
-- 
2.13.6