[v2] pnv: add a physical mapping array describing MMIO ranges in each chip

Based on previous work done in skiboot, the physical mapping array
helps in calculating the MMIO ranges of each controller depending on
the chip id and the chip type. This is will be particularly useful for
the P9 models which use less the XSCOM bus and rely more on MMIOs.

A link on the chip is now necessary to calculate MMIO BARs and
sizes. This is why such a link is introduced in the PSIHB model.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

 Changes since v1:

 - removed PNV_MAP_MAX which has unused
 - introduced a chip class handler to calculate the base address of a
   controller as suggested by Greg.
 - fix error reporting in pnv_psi_realize()

 include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
 hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
 hw/ppc/pnv_psi.c     | 15 ++++++++++++---
 hw/ppc/pnv_xscom.c   |  8 ++++----
 4 files changed, 96 insertions(+), 31 deletions(-)

On Wed, 30 May 2018 12:07:54 +0200
Cédric Le Goater <clg@kaod.org> wrote:

> Based on previous work done in skiboot, the physical mapping array
> helps in calculating the MMIO ranges of each controller depending on
> the chip id and the chip type. This is will be particularly useful for
> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> 
> A link on the chip is now necessary to calculate MMIO BARs and
> sizes. This is why such a link is introduced in the PSIHB model.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
>  Changes since v1:
> 
>  - removed PNV_MAP_MAX which has unused
>  - introduced a chip class handler to calculate the base address of a
>    controller as suggested by Greg.
>  - fix error reporting in pnv_psi_realize()
> 
>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
>  hw/ppc/pnv_xscom.c   |  8 ++++----
>  4 files changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7b1..ffa4a0899705 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>      uint64_t     cores_mask;
>      void         *cores;
>  
> -    hwaddr       xscom_base;
>      MemoryRegion xscom_mmio;
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
> @@ -64,6 +63,18 @@ typedef struct PnvChip {
>      PnvOCC       occ;
>  } PnvChip;
>  
> +typedef enum PnvPhysMapType {
> +    PNV_MAP_XSCOM,
> +    PNV_MAP_ICP,
> +    PNV_MAP_PSIHB,
> +    PNV_MAP_PSIHB_FSP,
> +} PnvPhysMapType;
> +
> +typedef struct PnvPhysMapEntry {
> +    uint64_t        base;
> +    uint64_t        size;
> +} PnvPhysMapEntry;
> +
>  typedef struct PnvChipClass {
>      /*< private >*/
>      SysBusDeviceClass parent_class;
> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
>      uint64_t     chip_cfam_id;
>      uint64_t     cores_mask;
>  
> -    hwaddr       xscom_base;
> +    const PnvPhysMapEntry *phys_map;
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  /*
>   * POWER8 MMIO base addresses
>   */
> -#define PNV_XSCOM_SIZE        0x800000000ull
> -#define PNV_XSCOM_BASE(chip)                                            \
> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +
> +    return map->size;
> +}
> +
> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> +}
> +
> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>  
>  /*
>   * XSCOM 0x20109CA defines the ICP BAR:
> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>   *      0xffffe02200000000 -> 0x0003ffff80800000
>   *      0xffffe02600000000 -> 0x0003ffff80900000
>   */
> -#define PNV_ICP_SIZE         0x0000000000100000ull
> -#define PNV_ICP_BASE(chip)                                              \
> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
> -
> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>  
> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
> -#define PNV_PSIHB_BASE(chip) \
> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>  
> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
> -#define PNV_PSIHB_FSP_BASE(chip) \
> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
> -     PNV_PSIHB_FSP_SIZE)
> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>  
>  #endif /* _PPC_PNV_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..77caaea64b2f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>   */
>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>  
> +/*
> + * POWER8 MMIOs
> + */
> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
> +};
> +
> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +
> +    return map->base + chip->chip_id * map->size;
> +}
> +
>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
>  
> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8";
>  }
>  
> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
>  
> +/*
> + * POWER9 MMIOs
> + */
> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
> +};
> +
> +/* Each chip has a 4TB range for its MMIOs */
> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +

These lines are indentical in the p8 version... maybe have the caller to pass
&pcc->phys_map[type] directly instead of type ?

> +    return map->base + ((uint64_t) chip->chip_id << 42);
> +}
> +
>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
> +    k->map_base = pnv_chip_map_base_p9;
> +    k->phys_map = pnv_chip_power9_phys_map;
>      dc->desc = "PowerNV Chip POWER9";
>  }
>  
> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>  static void pnv_chip_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), "chip", obj,
> +                                   &error_abort);
>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>                                     OBJECT(qdev_get_machine()), &error_abort);
>  
> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>      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);
> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>      g_free(name);
>  
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 5b969127c303..882b5d4b9f99 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>      Object *obj;
>      Error *err = NULL;
>      unsigned int i;
> +    PnvChip *chip;
> +
> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'chip' not found: ");
> +        return;
> +    }
> +    chip = PNV_CHIP(obj);
>  
>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>      if (!obj) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> -                   __func__, error_get_pretty(err));
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'xics' not found: ");
>          return;
>      }
>  
> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>  
>      /* Initialize MMIO region */
>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> -                          "psihb", PNV_PSIHB_SIZE);
> +                          "psihb", PNV_PSIHB_SIZE(chip));
>  
>      /* Default BAR for MMIO region */
>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index 46fae41f32b0..20ffc233174c 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  
>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>  {
> -    addr &= (PNV_XSCOM_SIZE - 1);
> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>  
>      if (pnv_chip_is_power9(chip)) {
>          return addr >> 3;
> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>  
>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
> -                          chip, name, PNV_XSCOM_SIZE);
> +                          chip, name, PNV_XSCOM_SIZE(chip));
>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>  
> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>      g_free(name);
>  }
> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>  {
>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>      int xscom_offset;
>      ForeachPopulateArgs args;
>      char *name;

On 05/30/2018 12:23 PM, Greg Kurz wrote:
> On Wed, 30 May 2018 12:07:54 +0200
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> Based on previous work done in skiboot, the physical mapping array
>> helps in calculating the MMIO ranges of each controller depending on
>> the chip id and the chip type. This is will be particularly useful for
>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>
>> A link on the chip is now necessary to calculate MMIO BARs and
>> sizes. This is why such a link is introduced in the PSIHB model.
>>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>
>>  Changes since v1:
>>
>>  - removed PNV_MAP_MAX which has unused
>>  - introduced a chip class handler to calculate the base address of a
>>    controller as suggested by Greg.
>>  - fix error reporting in pnv_psi_realize()
>>
>>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
>>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
>>  hw/ppc/pnv_xscom.c   |  8 ++++----
>>  4 files changed, 96 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 90759240a7b1..ffa4a0899705 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>>      uint64_t     cores_mask;
>>      void         *cores;
>>  
>> -    hwaddr       xscom_base;
>>      MemoryRegion xscom_mmio;
>>      MemoryRegion xscom;
>>      AddressSpace xscom_as;
>> @@ -64,6 +63,18 @@ typedef struct PnvChip {
>>      PnvOCC       occ;
>>  } PnvChip;
>>  
>> +typedef enum PnvPhysMapType {
>> +    PNV_MAP_XSCOM,
>> +    PNV_MAP_ICP,
>> +    PNV_MAP_PSIHB,
>> +    PNV_MAP_PSIHB_FSP,
>> +} PnvPhysMapType;
>> +
>> +typedef struct PnvPhysMapEntry {
>> +    uint64_t        base;
>> +    uint64_t        size;
>> +} PnvPhysMapEntry;
>> +
>>  typedef struct PnvChipClass {
>>      /*< private >*/
>>      SysBusDeviceClass parent_class;
>> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
>>      uint64_t     chip_cfam_id;
>>      uint64_t     cores_mask;
>>  
>> -    hwaddr       xscom_base;
>> +    const PnvPhysMapEntry *phys_map;
>>  
>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>>  } PnvChipClass;
>>  
>>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
>> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>  /*
>>   * POWER8 MMIO base addresses
>>   */
>> -#define PNV_XSCOM_SIZE        0x800000000ull
>> -#define PNV_XSCOM_BASE(chip)                                            \
>> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
>> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
>> +    return map->size;
>> +}
>> +
>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
>> +}
>> +
>> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
>> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>>  
>>  /*
>>   * XSCOM 0x20109CA defines the ICP BAR:
>> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>   *      0xffffe02200000000 -> 0x0003ffff80800000
>>   *      0xffffe02600000000 -> 0x0003ffff80900000
>>   */
>> -#define PNV_ICP_SIZE         0x0000000000100000ull
>> -#define PNV_ICP_BASE(chip)                                              \
>> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
>> -
>> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
>> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>>  
>> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
>> -#define PNV_PSIHB_BASE(chip) \
>> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
>> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
>> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>>  
>> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
>> -#define PNV_PSIHB_FSP_BASE(chip) \
>> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
>> -     PNV_PSIHB_FSP_SIZE)
>> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
>> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>>  
>>  #endif /* _PPC_PNV_H */
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 031488131629..77caaea64b2f 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>>   */
>>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>>  
>> +/*
>> + * POWER8 MMIOs
>> + */
>> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
>> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
>> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
>> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
>> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
>> +};
>> +
>> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
>> +    return map->base + chip->chip_id * map->size;
>> +}
>> +
>>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8E";
>>  }
>>  
>> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8";
>>  }
>>  
>> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8NVL";
>>  }
>>  
>> +/*
>> + * POWER9 MMIOs
>> + */
>> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
>> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
>> +};
>> +
>> +/* Each chip has a 4TB range for its MMIOs */
>> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
> 
> These lines are indentical in the p8 version... maybe have the caller to pass
> &pcc->phys_map[type] directly instead of type ?


yes. But I would like these to be consistent with pnv_map_size()

So I rather keep it that way.

Thanks,

C. 

>> +    return map->base + ((uint64_t) chip->chip_id << 42);
>> +}
>> +
>>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
>> +    k->map_base = pnv_chip_map_base_p9;
>> +    k->phys_map = pnv_chip_power9_phys_map;
>>      dc->desc = "PowerNV Chip POWER9";
>>  }
>>  
>> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>>  static void pnv_chip_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), "chip", obj,
>> +                                   &error_abort);
>>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>                                     OBJECT(qdev_get_machine()), &error_abort);
>>  
>> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>>      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);
>> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
>>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>>      g_free(name);
>>  
>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>> index 5b969127c303..882b5d4b9f99 100644
>> --- a/hw/ppc/pnv_psi.c
>> +++ b/hw/ppc/pnv_psi.c
>> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>      Object *obj;
>>      Error *err = NULL;
>>      unsigned int i;
>> +    PnvChip *chip;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
>> +    if (!obj) {
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'chip' not found: ");
>> +        return;
>> +    }
>> +    chip = PNV_CHIP(obj);
>>  
>>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>      if (!obj) {
>> -        error_setg(errp, "%s: required link 'xics' not found: %s",
>> -                   __func__, error_get_pretty(err));
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'xics' not found: ");
>>          return;
>>      }
>>  
>> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>  
>>      /* Initialize MMIO region */
>>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>> -                          "psihb", PNV_PSIHB_SIZE);
>> +                          "psihb", PNV_PSIHB_SIZE(chip));
>>  
>>      /* Default BAR for MMIO region */
>>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>> index 46fae41f32b0..20ffc233174c 100644
>> --- a/hw/ppc/pnv_xscom.c
>> +++ b/hw/ppc/pnv_xscom.c
>> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>>  
>>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>>  {
>> -    addr &= (PNV_XSCOM_SIZE - 1);
>> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>>  
>>      if (pnv_chip_is_power9(chip)) {
>>          return addr >> 3;
>> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>>  
>>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
>> -                          chip, name, PNV_XSCOM_SIZE);
>> +                          chip, name, PNV_XSCOM_SIZE(chip));
>>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>>  
>> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
>> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
>>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>>      g_free(name);
>>  }
>> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>>  {
>>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
>> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
>> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>>      int xscom_offset;
>>      ForeachPopulateArgs args;
>>      char *name;
>

On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
> Based on previous work done in skiboot, the physical mapping array
> helps in calculating the MMIO ranges of each controller depending on
> the chip id and the chip type. This is will be particularly useful for
> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> 
> A link on the chip is now necessary to calculate MMIO BARs and
> sizes. This is why such a link is introduced in the PSIHB model.

I think this message needs some work.  This says what it's for, but
what actually *is* this array, and how does it work?

The outside-core differences between POWER8 and POWER9 are substantial
enough that I'm wondering if pnvP8 and pnvP9 would be better off as
different machine types (sharing some routines, of course).

> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> 
>  Changes since v1:
> 
>  - removed PNV_MAP_MAX which has unused
>  - introduced a chip class handler to calculate the base address of a
>    controller as suggested by Greg.
>  - fix error reporting in pnv_psi_realize()
> 
>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
>  hw/ppc/pnv_xscom.c   |  8 ++++----
>  4 files changed, 96 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 90759240a7b1..ffa4a0899705 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>      uint64_t     cores_mask;
>      void         *cores;
>  
> -    hwaddr       xscom_base;
>      MemoryRegion xscom_mmio;
>      MemoryRegion xscom;
>      AddressSpace xscom_as;
> @@ -64,6 +63,18 @@ typedef struct PnvChip {
>      PnvOCC       occ;
>  } PnvChip;
>  
> +typedef enum PnvPhysMapType {
> +    PNV_MAP_XSCOM,
> +    PNV_MAP_ICP,
> +    PNV_MAP_PSIHB,
> +    PNV_MAP_PSIHB_FSP,
> +} PnvPhysMapType;
> +
> +typedef struct PnvPhysMapEntry {
> +    uint64_t        base;
> +    uint64_t        size;
> +} PnvPhysMapEntry;
> +
>  typedef struct PnvChipClass {
>      /*< private >*/
>      SysBusDeviceClass parent_class;
> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
>      uint64_t     chip_cfam_id;
>      uint64_t     cores_mask;
>  
> -    hwaddr       xscom_base;
> +    const PnvPhysMapEntry *phys_map;
>  
>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>  } PnvChipClass;
>  
>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  /*
>   * POWER8 MMIO base addresses
>   */
> -#define PNV_XSCOM_SIZE        0x800000000ull
> -#define PNV_XSCOM_BASE(chip)                                            \
> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +
> +    return map->size;
> +}
> +
> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> +}
> +
> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>  
>  /*
>   * XSCOM 0x20109CA defines the ICP BAR:
> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>   *      0xffffe02200000000 -> 0x0003ffff80800000
>   *      0xffffe02600000000 -> 0x0003ffff80900000
>   */
> -#define PNV_ICP_SIZE         0x0000000000100000ull
> -#define PNV_ICP_BASE(chip)                                              \
> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
> -
> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>  
> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
> -#define PNV_PSIHB_BASE(chip) \
> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>  
> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
> -#define PNV_PSIHB_FSP_BASE(chip) \
> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
> -     PNV_PSIHB_FSP_SIZE)
> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>  
>  #endif /* _PPC_PNV_H */
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 031488131629..77caaea64b2f 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>   */
>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>  
> +/*
> + * POWER8 MMIOs
> + */
> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
> +};
> +
> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +
> +    return map->base + chip->chip_id * map->size;
> +}
> +
>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8E";
>  }
>  
> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8";
>  }
>  
> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
> +    k->map_base = pnv_chip_map_base_p8;
> +    k->phys_map = pnv_chip_power8_phys_map;
>      dc->desc = "PowerNV Chip POWER8NVL";
>  }
>  
> +/*
> + * POWER9 MMIOs
> + */
> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
> +};
> +
> +/* Each chip has a 4TB range for its MMIOs */
> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
> +{
> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> +
> +    return map->base + ((uint64_t) chip->chip_id << 42);
> +}
> +
>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
> +    k->map_base = pnv_chip_map_base_p9;
> +    k->phys_map = pnv_chip_power9_phys_map;
>      dc->desc = "PowerNV Chip POWER9";
>  }
>  
> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>  static void pnv_chip_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), "chip", obj,
> +                                   &error_abort);
>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>                                     OBJECT(qdev_get_machine()), &error_abort);
>  
> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>      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);
> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>      g_free(name);
>  
> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> index 5b969127c303..882b5d4b9f99 100644
> --- a/hw/ppc/pnv_psi.c
> +++ b/hw/ppc/pnv_psi.c
> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>      Object *obj;
>      Error *err = NULL;
>      unsigned int i;
> +    PnvChip *chip;
> +
> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
> +    if (!obj) {
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'chip' not found: ");
> +        return;
> +    }
> +    chip = PNV_CHIP(obj);
>  
>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>      if (!obj) {
> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> -                   __func__, error_get_pretty(err));
> +        error_propagate(errp, err);
> +        error_prepend(errp, "required link 'xics' not found: ");
>          return;
>      }
>  
> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>  
>      /* Initialize MMIO region */
>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> -                          "psihb", PNV_PSIHB_SIZE);
> +                          "psihb", PNV_PSIHB_SIZE(chip));
>  
>      /* Default BAR for MMIO region */
>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index 46fae41f32b0..20ffc233174c 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  
>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>  {
> -    addr &= (PNV_XSCOM_SIZE - 1);
> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>  
>      if (pnv_chip_is_power9(chip)) {
>          return addr >> 3;
> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>  
>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
> -                          chip, name, PNV_XSCOM_SIZE);
> +                          chip, name, PNV_XSCOM_SIZE(chip));
>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>  
> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>      g_free(name);
>  }
> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>  {
>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>      int xscom_offset;
>      ForeachPopulateArgs args;
>      char *name;

On 06/06/2018 08:39 AM, David Gibson wrote:
> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
>> Based on previous work done in skiboot, the physical mapping array
>> helps in calculating the MMIO ranges of each controller depending on
>> the chip id and the chip type. This is will be particularly useful for
>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>
>> A link on the chip is now necessary to calculate MMIO BARs and
>> sizes. This is why such a link is introduced in the PSIHB model.
> 
> I think this message needs some work.  This says what it's for, but
> what actually *is* this array, and how does it work?

OK. It is relatively simple: each controller has an entry defining its 
MMIO range. 

> The outside-core differences between POWER8 and POWER9 are substantial
> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> different machine types (sharing some routines, of course).

yes and no. I have survived using a common PnvChip framework but
it is true that I had to add P9 classes for each: LPC, PSI, OCC 
They are very similar but not enough. P9 uses much more MMIOs than 
P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 

The interrupt controller is completely different of course.
  
C.

>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>
>>  Changes since v1:
>>
>>  - removed PNV_MAP_MAX which has unused
>>  - introduced a chip class handler to calculate the base address of a
>>    controller as suggested by Greg.
>>  - fix error reporting in pnv_psi_realize()
>>
>>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
>>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
>>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
>>  hw/ppc/pnv_xscom.c   |  8 ++++----
>>  4 files changed, 96 insertions(+), 31 deletions(-)
>>
>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>> index 90759240a7b1..ffa4a0899705 100644
>> --- a/include/hw/ppc/pnv.h
>> +++ b/include/hw/ppc/pnv.h
>> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>>      uint64_t     cores_mask;
>>      void         *cores;
>>  
>> -    hwaddr       xscom_base;
>>      MemoryRegion xscom_mmio;
>>      MemoryRegion xscom;
>>      AddressSpace xscom_as;
>> @@ -64,6 +63,18 @@ typedef struct PnvChip {
>>      PnvOCC       occ;
>>  } PnvChip;
>>  
>> +typedef enum PnvPhysMapType {
>> +    PNV_MAP_XSCOM,
>> +    PNV_MAP_ICP,
>> +    PNV_MAP_PSIHB,
>> +    PNV_MAP_PSIHB_FSP,
>> +} PnvPhysMapType;
>> +
>> +typedef struct PnvPhysMapEntry {
>> +    uint64_t        base;
>> +    uint64_t        size;
>> +} PnvPhysMapEntry;
>> +
>>  typedef struct PnvChipClass {
>>      /*< private >*/
>>      SysBusDeviceClass parent_class;
>> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
>>      uint64_t     chip_cfam_id;
>>      uint64_t     cores_mask;
>>  
>> -    hwaddr       xscom_base;
>> +    const PnvPhysMapEntry *phys_map;
>>  
>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>>  } PnvChipClass;
>>  
>>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
>> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>  /*
>>   * POWER8 MMIO base addresses
>>   */
>> -#define PNV_XSCOM_SIZE        0x800000000ull
>> -#define PNV_XSCOM_BASE(chip)                                            \
>> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
>> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
>> +    return map->size;
>> +}
>> +
>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
>> +}
>> +
>> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
>> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>>  
>>  /*
>>   * XSCOM 0x20109CA defines the ICP BAR:
>> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>   *      0xffffe02200000000 -> 0x0003ffff80800000
>>   *      0xffffe02600000000 -> 0x0003ffff80900000
>>   */
>> -#define PNV_ICP_SIZE         0x0000000000100000ull
>> -#define PNV_ICP_BASE(chip)                                              \
>> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
>> -
>> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
>> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>>  
>> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
>> -#define PNV_PSIHB_BASE(chip) \
>> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
>> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
>> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>>  
>> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
>> -#define PNV_PSIHB_FSP_BASE(chip) \
>> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
>> -     PNV_PSIHB_FSP_SIZE)
>> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
>> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>>  
>>  #endif /* _PPC_PNV_H */
>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>> index 031488131629..77caaea64b2f 100644
>> --- a/hw/ppc/pnv.c
>> +++ b/hw/ppc/pnv.c
>> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>>   */
>>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>>  
>> +/*
>> + * POWER8 MMIOs
>> + */
>> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
>> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
>> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
>> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
>> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
>> +};
>> +
>> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
>> +    return map->base + chip->chip_id * map->size;
>> +}
>> +
>>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8E";
>>  }
>>  
>> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8";
>>  }
>>  
>> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
>> +    k->map_base = pnv_chip_map_base_p8;
>> +    k->phys_map = pnv_chip_power8_phys_map;
>>      dc->desc = "PowerNV Chip POWER8NVL";
>>  }
>>  
>> +/*
>> + * POWER9 MMIOs
>> + */
>> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
>> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
>> +};
>> +
>> +/* Each chip has a 4TB range for its MMIOs */
>> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
>> +{
>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>> +
>> +    return map->base + ((uint64_t) chip->chip_id << 42);
>> +}
>> +
>>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
>> +    k->map_base = pnv_chip_map_base_p9;
>> +    k->phys_map = pnv_chip_power9_phys_map;
>>      dc->desc = "PowerNV Chip POWER9";
>>  }
>>  
>> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>>  static void pnv_chip_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), "chip", obj,
>> +                                   &error_abort);
>>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>                                     OBJECT(qdev_get_machine()), &error_abort);
>>  
>> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>>      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);
>> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
>>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>>      g_free(name);
>>  
>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>> index 5b969127c303..882b5d4b9f99 100644
>> --- a/hw/ppc/pnv_psi.c
>> +++ b/hw/ppc/pnv_psi.c
>> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>      Object *obj;
>>      Error *err = NULL;
>>      unsigned int i;
>> +    PnvChip *chip;
>> +
>> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
>> +    if (!obj) {
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'chip' not found: ");
>> +        return;
>> +    }
>> +    chip = PNV_CHIP(obj);
>>  
>>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>      if (!obj) {
>> -        error_setg(errp, "%s: required link 'xics' not found: %s",
>> -                   __func__, error_get_pretty(err));
>> +        error_propagate(errp, err);
>> +        error_prepend(errp, "required link 'xics' not found: ");
>>          return;
>>      }
>>  
>> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>  
>>      /* Initialize MMIO region */
>>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>> -                          "psihb", PNV_PSIHB_SIZE);
>> +                          "psihb", PNV_PSIHB_SIZE(chip));
>>  
>>      /* Default BAR for MMIO region */
>>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>> index 46fae41f32b0..20ffc233174c 100644
>> --- a/hw/ppc/pnv_xscom.c
>> +++ b/hw/ppc/pnv_xscom.c
>> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>>  
>>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>>  {
>> -    addr &= (PNV_XSCOM_SIZE - 1);
>> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>>  
>>      if (pnv_chip_is_power9(chip)) {
>>          return addr >> 3;
>> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>>  
>>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
>> -                          chip, name, PNV_XSCOM_SIZE);
>> +                          chip, name, PNV_XSCOM_SIZE(chip));
>>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>>  
>> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
>> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
>>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>>      g_free(name);
>>  }
>> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>>  {
>>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
>> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
>> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>>      int xscom_offset;
>>      ForeachPopulateArgs args;
>>      char *name;
>

On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
> On 06/06/2018 08:39 AM, David Gibson wrote:
> > On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
> >> Based on previous work done in skiboot, the physical mapping array
> >> helps in calculating the MMIO ranges of each controller depending on
> >> the chip id and the chip type. This is will be particularly useful for
> >> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> >>
> >> A link on the chip is now necessary to calculate MMIO BARs and
> >> sizes. This is why such a link is introduced in the PSIHB model.
> > 
> > I think this message needs some work.  This says what it's for, but
> > what actually *is* this array, and how does it work?
> 
> OK. It is relatively simple: each controller has an entry defining its 
> MMIO range. 
> 
> > The outside-core differences between POWER8 and POWER9 are substantial
> > enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> > different machine types (sharing some routines, of course).
> 
> yes and no. I have survived using a common PnvChip framework but
> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
> They are very similar but not enough. P9 uses much more MMIOs than 
> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 

Well, it's certainly *possible* to use the same machine type, I'm just
not convinced it's a good idea.  It seems kind of dodgy to me that so
many peripherals on the system change as a side-effect of setting the
cpu.  Compare to how x86 works where cpu really does change the CPU,
plugging it into the same virtual "chipset".  Different chipsets *are*
different machine types there (pc vs. q35).

> The interrupt controller is completely different of course.
>   
> C.
> 
> >> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >>
> >>  Changes since v1:
> >>
> >>  - removed PNV_MAP_MAX which has unused
> >>  - introduced a chip class handler to calculate the base address of a
> >>    controller as suggested by Greg.
> >>  - fix error reporting in pnv_psi_realize()
> >>
> >>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
> >>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
> >>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
> >>  hw/ppc/pnv_xscom.c   |  8 ++++----
> >>  4 files changed, 96 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 90759240a7b1..ffa4a0899705 100644
> >> --- a/include/hw/ppc/pnv.h
> >> +++ b/include/hw/ppc/pnv.h
> >> @@ -53,7 +53,6 @@ typedef struct PnvChip {
> >>      uint64_t     cores_mask;
> >>      void         *cores;
> >>  
> >> -    hwaddr       xscom_base;
> >>      MemoryRegion xscom_mmio;
> >>      MemoryRegion xscom;
> >>      AddressSpace xscom_as;
> >> @@ -64,6 +63,18 @@ typedef struct PnvChip {
> >>      PnvOCC       occ;
> >>  } PnvChip;
> >>  
> >> +typedef enum PnvPhysMapType {
> >> +    PNV_MAP_XSCOM,
> >> +    PNV_MAP_ICP,
> >> +    PNV_MAP_PSIHB,
> >> +    PNV_MAP_PSIHB_FSP,
> >> +} PnvPhysMapType;
> >> +
> >> +typedef struct PnvPhysMapEntry {
> >> +    uint64_t        base;
> >> +    uint64_t        size;
> >> +} PnvPhysMapEntry;
> >> +
> >>  typedef struct PnvChipClass {
> >>      /*< private >*/
> >>      SysBusDeviceClass parent_class;
> >> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
> >>      uint64_t     chip_cfam_id;
> >>      uint64_t     cores_mask;
> >>  
> >> -    hwaddr       xscom_base;
> >> +    const PnvPhysMapEntry *phys_map;
> >>  
> >>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
> >> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
> >>  } PnvChipClass;
> >>  
> >>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
> >> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >>  /*
> >>   * POWER8 MMIO base addresses
> >>   */
> >> -#define PNV_XSCOM_SIZE        0x800000000ull
> >> -#define PNV_XSCOM_BASE(chip)                                            \
> >> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
> >> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
> >> +{
> >> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> >> +
> >> +    return map->size;
> >> +}
> >> +
> >> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
> >> +{
> >> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
> >> +}
> >> +
> >> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
> >> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
> >>  
> >>  /*
> >>   * XSCOM 0x20109CA defines the ICP BAR:
> >> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >>   *      0xffffe02200000000 -> 0x0003ffff80800000
> >>   *      0xffffe02600000000 -> 0x0003ffff80900000
> >>   */
> >> -#define PNV_ICP_SIZE         0x0000000000100000ull
> >> -#define PNV_ICP_BASE(chip)                                              \
> >> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
> >> -
> >> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
> >> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
> >>  
> >> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
> >> -#define PNV_PSIHB_BASE(chip) \
> >> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
> >> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
> >> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
> >>  
> >> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
> >> -#define PNV_PSIHB_FSP_BASE(chip) \
> >> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
> >> -     PNV_PSIHB_FSP_SIZE)
> >> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
> >> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
> >>  
> >>  #endif /* _PPC_PNV_H */
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 031488131629..77caaea64b2f 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
> >>   */
> >>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
> >>  
> >> +/*
> >> + * POWER8 MMIOs
> >> + */
> >> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
> >> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
> >> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
> >> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
> >> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
> >> +};
> >> +
> >> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
> >> +{
> >> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> >> +
> >> +    return map->base + chip->chip_id * map->size;
> >> +}
> >> +
> >>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> >> +    k->map_base = pnv_chip_map_base_p8;
> >> +    k->phys_map = pnv_chip_power8_phys_map;
> >>      dc->desc = "PowerNV Chip POWER8E";
> >>  }
> >>  
> >> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
> >> +    k->map_base = pnv_chip_map_base_p8;
> >> +    k->phys_map = pnv_chip_power8_phys_map;
> >>      dc->desc = "PowerNV Chip POWER8";
> >>  }
> >>  
> >> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
> >> +    k->map_base = pnv_chip_map_base_p8;
> >> +    k->phys_map = pnv_chip_power8_phys_map;
> >>      dc->desc = "PowerNV Chip POWER8NVL";
> >>  }
> >>  
> >> +/*
> >> + * POWER9 MMIOs
> >> + */
> >> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
> >> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
> >> +};
> >> +
> >> +/* Each chip has a 4TB range for its MMIOs */
> >> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
> >> +{
> >> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
> >> +
> >> +    return map->base + ((uint64_t) chip->chip_id << 42);
> >> +}
> >> +
> >>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
> >> +    k->map_base = pnv_chip_map_base_p9;
> >> +    k->phys_map = pnv_chip_power9_phys_map;
> >>      dc->desc = "PowerNV Chip POWER9";
> >>  }
> >>  
> >> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
> >>  static void pnv_chip_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), "chip", obj,
> >> +                                   &error_abort);
> >>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
> >>                                     OBJECT(qdev_get_machine()), &error_abort);
> >>  
> >> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
> >>      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);
> >> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
> >>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
> >>      g_free(name);
> >>  
> >> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
> >> index 5b969127c303..882b5d4b9f99 100644
> >> --- a/hw/ppc/pnv_psi.c
> >> +++ b/hw/ppc/pnv_psi.c
> >> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
> >>      Object *obj;
> >>      Error *err = NULL;
> >>      unsigned int i;
> >> +    PnvChip *chip;
> >> +
> >> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
> >> +    if (!obj) {
> >> +        error_propagate(errp, err);
> >> +        error_prepend(errp, "required link 'chip' not found: ");
> >> +        return;
> >> +    }
> >> +    chip = PNV_CHIP(obj);
> >>  
> >>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
> >>      if (!obj) {
> >> -        error_setg(errp, "%s: required link 'xics' not found: %s",
> >> -                   __func__, error_get_pretty(err));
> >> +        error_propagate(errp, err);
> >> +        error_prepend(errp, "required link 'xics' not found: ");
> >>          return;
> >>      }
> >>  
> >> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
> >>  
> >>      /* Initialize MMIO region */
> >>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
> >> -                          "psihb", PNV_PSIHB_SIZE);
> >> +                          "psihb", PNV_PSIHB_SIZE(chip));
> >>  
> >>      /* Default BAR for MMIO region */
> >>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
> >> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> >> index 46fae41f32b0..20ffc233174c 100644
> >> --- a/hw/ppc/pnv_xscom.c
> >> +++ b/hw/ppc/pnv_xscom.c
> >> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
> >>  
> >>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
> >>  {
> >> -    addr &= (PNV_XSCOM_SIZE - 1);
> >> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
> >>  
> >>      if (pnv_chip_is_power9(chip)) {
> >>          return addr >> 3;
> >> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
> >>  
> >>      name = g_strdup_printf("xscom-%x", chip->chip_id);
> >>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
> >> -                          chip, name, PNV_XSCOM_SIZE);
> >> +                          chip, name, PNV_XSCOM_SIZE(chip));
> >>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
> >>  
> >> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
> >> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
> >>      address_space_init(&chip->xscom_as, &chip->xscom, name);
> >>      g_free(name);
> >>  }
> >> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
> >>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
> >>  {
> >>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
> >> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
> >> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
> >>      int xscom_offset;
> >>      ForeachPopulateArgs args;
> >>      char *name;
> > 
>

On 06/12/2018 07:58 AM, David Gibson wrote:
> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
>> On 06/06/2018 08:39 AM, David Gibson wrote:
>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
>>>> Based on previous work done in skiboot, the physical mapping array
>>>> helps in calculating the MMIO ranges of each controller depending on
>>>> the chip id and the chip type. This is will be particularly useful for
>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>>>
>>>> A link on the chip is now necessary to calculate MMIO BARs and
>>>> sizes. This is why such a link is introduced in the PSIHB model.
>>>
>>> I think this message needs some work.  This says what it's for, but
>>> what actually *is* this array, and how does it work?
>>
>> OK. It is relatively simple: each controller has an entry defining its 
>> MMIO range. 
>>
>>> The outside-core differences between POWER8 and POWER9 are substantial
>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
>>> different machine types (sharing some routines, of course).
>>
>> yes and no. I have survived using a common PnvChip framework but
>> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
>> They are very similar but not enough. P9 uses much more MMIOs than 
>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
> 
> Well, it's certainly *possible* to use the same machine type, I'm just
> not convinced it's a good idea.  It seems kind of dodgy to me that so
> many peripherals on the system change as a side-effect of setting the
> cpu.  Compare to how x86 works where cpu really does change the CPU,
> plugging it into the same virtual "chipset".  Different chipsets *are*
> different machine types there (pc vs. q35).

OK, I agree, and we can use a set of common routines to instantiate the 
different chipset models. 

So we would have a common pnv_init() routine to initialize the different 
'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
machine class attribute ? 

Nevertheless we would still need to introduce "a physical mapping array 
describing MMIO ranges" but we can start by differentiating the chipsets 
first.

C. 

>> The interrupt controller is completely different of course.
>>   
>> C.
>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> ---
>>>>
>>>>  Changes since v1:
>>>>
>>>>  - removed PNV_MAP_MAX which has unused
>>>>  - introduced a chip class handler to calculate the base address of a
>>>>    controller as suggested by Greg.
>>>>  - fix error reporting in pnv_psi_realize()
>>>>
>>>>  include/hw/ppc/pnv.h | 51 ++++++++++++++++++++++++++++++++++----------------
>>>>  hw/ppc/pnv.c         | 53 ++++++++++++++++++++++++++++++++++++++++++++--------
>>>>  hw/ppc/pnv_psi.c     | 15 ++++++++++++---
>>>>  hw/ppc/pnv_xscom.c   |  8 ++++----
>>>>  4 files changed, 96 insertions(+), 31 deletions(-)
>>>>
>>>> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
>>>> index 90759240a7b1..ffa4a0899705 100644
>>>> --- a/include/hw/ppc/pnv.h
>>>> +++ b/include/hw/ppc/pnv.h
>>>> @@ -53,7 +53,6 @@ typedef struct PnvChip {
>>>>      uint64_t     cores_mask;
>>>>      void         *cores;
>>>>  
>>>> -    hwaddr       xscom_base;
>>>>      MemoryRegion xscom_mmio;
>>>>      MemoryRegion xscom;
>>>>      AddressSpace xscom_as;
>>>> @@ -64,6 +63,18 @@ typedef struct PnvChip {
>>>>      PnvOCC       occ;
>>>>  } PnvChip;
>>>>  
>>>> +typedef enum PnvPhysMapType {
>>>> +    PNV_MAP_XSCOM,
>>>> +    PNV_MAP_ICP,
>>>> +    PNV_MAP_PSIHB,
>>>> +    PNV_MAP_PSIHB_FSP,
>>>> +} PnvPhysMapType;
>>>> +
>>>> +typedef struct PnvPhysMapEntry {
>>>> +    uint64_t        base;
>>>> +    uint64_t        size;
>>>> +} PnvPhysMapEntry;
>>>> +
>>>>  typedef struct PnvChipClass {
>>>>      /*< private >*/
>>>>      SysBusDeviceClass parent_class;
>>>> @@ -73,9 +84,10 @@ typedef struct PnvChipClass {
>>>>      uint64_t     chip_cfam_id;
>>>>      uint64_t     cores_mask;
>>>>  
>>>> -    hwaddr       xscom_base;
>>>> +    const PnvPhysMapEntry *phys_map;
>>>>  
>>>>      uint32_t (*core_pir)(PnvChip *chip, uint32_t core_id);
>>>> +    uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
>>>>  } PnvChipClass;
>>>>  
>>>>  #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP
>>>> @@ -159,9 +171,21 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>>>  /*
>>>>   * POWER8 MMIO base addresses
>>>>   */
>>>> -#define PNV_XSCOM_SIZE        0x800000000ull
>>>> -#define PNV_XSCOM_BASE(chip)                                            \
>>>> -    (chip->xscom_base + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
>>>> +static inline uint64_t pnv_map_size(const PnvChip *chip, PnvPhysMapType type)
>>>> +{
>>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>>>> +
>>>> +    return map->size;
>>>> +}
>>>> +
>>>> +static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
>>>> +{
>>>> +    return PNV_CHIP_GET_CLASS(chip)->map_base(chip, type);
>>>> +}
>>>> +
>>>> +#define PNV_XSCOM_SIZE(chip)         pnv_map_size(chip, PNV_MAP_XSCOM)
>>>> +#define PNV_XSCOM_BASE(chip)         pnv_map_base(chip, PNV_MAP_XSCOM)
>>>>  
>>>>  /*
>>>>   * XSCOM 0x20109CA defines the ICP BAR:
>>>> @@ -177,18 +201,13 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>>>>   *      0xffffe02200000000 -> 0x0003ffff80800000
>>>>   *      0xffffe02600000000 -> 0x0003ffff80900000
>>>>   */
>>>> -#define PNV_ICP_SIZE         0x0000000000100000ull
>>>> -#define PNV_ICP_BASE(chip)                                              \
>>>> -    (0x0003ffff80000000ull + (uint64_t) PNV_CHIP_INDEX(chip) * PNV_ICP_SIZE)
>>>> -
>>>> +#define PNV_ICP_SIZE(chip)           pnv_map_size(chip, PNV_MAP_ICP)
>>>> +#define PNV_ICP_BASE(chip)           pnv_map_base(chip, PNV_MAP_ICP)
>>>>  
>>>> -#define PNV_PSIHB_SIZE       0x0000000000100000ull
>>>> -#define PNV_PSIHB_BASE(chip) \
>>>> -    (0x0003fffe80000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * PNV_PSIHB_SIZE)
>>>> +#define PNV_PSIHB_SIZE(chip)         pnv_map_size(chip, PNV_MAP_PSIHB)
>>>> +#define PNV_PSIHB_BASE(chip)         pnv_map_base(chip, PNV_MAP_PSIHB)
>>>>  
>>>> -#define PNV_PSIHB_FSP_SIZE   0x0000000100000000ull
>>>> -#define PNV_PSIHB_FSP_BASE(chip) \
>>>> -    (0x0003ffe000000000ull + (uint64_t)PNV_CHIP_INDEX(chip) * \
>>>> -     PNV_PSIHB_FSP_SIZE)
>>>> +#define PNV_PSIHB_FSP_SIZE(chip)     pnv_map_size(chip, PNV_MAP_PSIHB_FSP)
>>>> +#define PNV_PSIHB_FSP_BASE(chip)     pnv_map_base(chip, PNV_MAP_PSIHB_FSP)
>>>>  
>>>>  #endif /* _PPC_PNV_H */
>>>> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
>>>> index 031488131629..77caaea64b2f 100644
>>>> --- a/hw/ppc/pnv.c
>>>> +++ b/hw/ppc/pnv.c
>>>> @@ -712,6 +712,24 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, uint32_t core_id)
>>>>   */
>>>>  #define POWER9_CORE_MASK   (0xffffffffffffffull)
>>>>  
>>>> +/*
>>>> + * POWER8 MMIOs
>>>> + */
>>>> +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
>>>> +    [PNV_MAP_XSCOM]     = { 0x0003fc0000000000ull, 0x0000000800000000ull },
>>>> +    [PNV_MAP_ICP]       = { 0x0003ffff80000000ull, 0x0000000000100000ull },
>>>> +    [PNV_MAP_PSIHB]     = { 0x0003fffe80000000ull, 0x0000000000100000ull },
>>>> +    [PNV_MAP_PSIHB_FSP] = { 0x0003ffe000000000ull, 0x0000000100000000ull },
>>>> +};
>>>> +
>>>> +static uint64_t pnv_chip_map_base_p8(const PnvChip *chip, PnvPhysMapType type)
>>>> +{
>>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>>>> +
>>>> +    return map->base + chip->chip_id * map->size;
>>>> +}
>>>> +
>>>>  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>> @@ -721,7 +739,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>>>> +    k->map_base = pnv_chip_map_base_p8;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8E";
>>>>  }
>>>>  
>>>> @@ -734,7 +753,8 @@ 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->xscom_base = 0x003fc0000000000ull;
>>>> +    k->map_base = pnv_chip_map_base_p8;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8";
>>>>  }
>>>>  
>>>> @@ -747,10 +767,27 @@ 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->xscom_base = 0x003fc0000000000ull;
>>>> +    k->map_base = pnv_chip_map_base_p8;
>>>> +    k->phys_map = pnv_chip_power8_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER8NVL";
>>>>  }
>>>>  
>>>> +/*
>>>> + * POWER9 MMIOs
>>>> + */
>>>> +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
>>>> +    [PNV_MAP_XSCOM]     = { 0x000603fc00000000ull, 0x0000000400000000ull },
>>>> +};
>>>> +
>>>> +/* Each chip has a 4TB range for its MMIOs */
>>>> +static uint64_t pnv_chip_map_base_p9(const PnvChip *chip, PnvPhysMapType type)
>>>> +{
>>>> +    PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
>>>> +    const PnvPhysMapEntry *map = &pcc->phys_map[type];
>>>> +
>>>> +    return map->base + ((uint64_t) chip->chip_id << 42);
>>>> +}
>>>> +
>>>>  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
>>>>  {
>>>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>>> @@ -760,7 +797,8 @@ 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->xscom_base = 0x00603fc00000000ull;
>>>> +    k->map_base = pnv_chip_map_base_p9;
>>>> +    k->phys_map = pnv_chip_power9_phys_map;
>>>>      dc->desc = "PowerNV Chip POWER9";
>>>>  }
>>>>  
>>>> @@ -797,15 +835,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, Error **errp)
>>>>  static void pnv_chip_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), "chip", obj,
>>>> +                                   &error_abort);
>>>>      object_property_add_const_link(OBJECT(&chip->psi), "xics",
>>>>                                     OBJECT(qdev_get_machine()), &error_abort);
>>>>  
>>>> @@ -829,7 +866,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error **errp)
>>>>      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);
>>>> +    memory_region_init(&chip->icp_mmio, OBJECT(chip), name, PNV_ICP_SIZE(chip));
>>>>      sysbus_init_mmio(SYS_BUS_DEVICE(chip), &chip->icp_mmio);
>>>>      g_free(name);
>>>>  
>>>> diff --git a/hw/ppc/pnv_psi.c b/hw/ppc/pnv_psi.c
>>>> index 5b969127c303..882b5d4b9f99 100644
>>>> --- a/hw/ppc/pnv_psi.c
>>>> +++ b/hw/ppc/pnv_psi.c
>>>> @@ -465,11 +465,20 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>>>      Object *obj;
>>>>      Error *err = NULL;
>>>>      unsigned int i;
>>>> +    PnvChip *chip;
>>>> +
>>>> +    obj = object_property_get_link(OBJECT(dev), "chip", &err);
>>>> +    if (!obj) {
>>>> +        error_propagate(errp, err);
>>>> +        error_prepend(errp, "required link 'chip' not found: ");
>>>> +        return;
>>>> +    }
>>>> +    chip = PNV_CHIP(obj);
>>>>  
>>>>      obj = object_property_get_link(OBJECT(dev), "xics", &err);
>>>>      if (!obj) {
>>>> -        error_setg(errp, "%s: required link 'xics' not found: %s",
>>>> -                   __func__, error_get_pretty(err));
>>>> +        error_propagate(errp, err);
>>>> +        error_prepend(errp, "required link 'xics' not found: ");
>>>>          return;
>>>>      }
>>>>  
>>>> @@ -497,7 +506,7 @@ static void pnv_psi_realize(DeviceState *dev, Error **errp)
>>>>  
>>>>      /* Initialize MMIO region */
>>>>      memory_region_init_io(&psi->regs_mr, OBJECT(dev), &psi_mmio_ops, psi,
>>>> -                          "psihb", PNV_PSIHB_SIZE);
>>>> +                          "psihb", PNV_PSIHB_SIZE(chip));
>>>>  
>>>>      /* Default BAR for MMIO region */
>>>>      pnv_psi_set_bar(psi, psi->bar | PSIHB_BAR_EN);
>>>> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
>>>> index 46fae41f32b0..20ffc233174c 100644
>>>> --- a/hw/ppc/pnv_xscom.c
>>>> +++ b/hw/ppc/pnv_xscom.c
>>>> @@ -50,7 +50,7 @@ static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>>>>  
>>>>  static uint32_t pnv_xscom_pcba(PnvChip *chip, uint64_t addr)
>>>>  {
>>>> -    addr &= (PNV_XSCOM_SIZE - 1);
>>>> +    addr &= (PNV_XSCOM_SIZE(chip) - 1);
>>>>  
>>>>      if (pnv_chip_is_power9(chip)) {
>>>>          return addr >> 3;
>>>> @@ -179,10 +179,10 @@ void pnv_xscom_realize(PnvChip *chip, Error **errp)
>>>>  
>>>>      name = g_strdup_printf("xscom-%x", chip->chip_id);
>>>>      memory_region_init_io(&chip->xscom_mmio, OBJECT(chip), &pnv_xscom_ops,
>>>> -                          chip, name, PNV_XSCOM_SIZE);
>>>> +                          chip, name, PNV_XSCOM_SIZE(chip));
>>>>      sysbus_init_mmio(sbd, &chip->xscom_mmio);
>>>>  
>>>> -    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE);
>>>> +    memory_region_init(&chip->xscom, OBJECT(chip), name, PNV_XSCOM_SIZE(chip));
>>>>      address_space_init(&chip->xscom_as, &chip->xscom, name);
>>>>      g_free(name);
>>>>  }
>>>> @@ -225,7 +225,7 @@ static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom";
>>>>  int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset)
>>>>  {
>>>>      uint64_t reg[] = { cpu_to_be64(PNV_XSCOM_BASE(chip)),
>>>> -                       cpu_to_be64(PNV_XSCOM_SIZE) };
>>>> +                       cpu_to_be64(PNV_XSCOM_SIZE(chip)) };
>>>>      int xscom_offset;
>>>>      ForeachPopulateArgs args;
>>>>      char *name;
>>>
>>
>

On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote:
> On 06/12/2018 07:58 AM, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
> >> On 06/06/2018 08:39 AM, David Gibson wrote:
> >>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
> >>>> Based on previous work done in skiboot, the physical mapping array
> >>>> helps in calculating the MMIO ranges of each controller depending on
> >>>> the chip id and the chip type. This is will be particularly useful for
> >>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> >>>>
> >>>> A link on the chip is now necessary to calculate MMIO BARs and
> >>>> sizes. This is why such a link is introduced in the PSIHB model.
> >>>
> >>> I think this message needs some work.  This says what it's for, but
> >>> what actually *is* this array, and how does it work?
> >>
> >> OK. It is relatively simple: each controller has an entry defining its 
> >> MMIO range. 
> >>
> >>> The outside-core differences between POWER8 and POWER9 are substantial
> >>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> >>> different machine types (sharing some routines, of course).
> >>
> >> yes and no. I have survived using a common PnvChip framework but
> >> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
> >> They are very similar but not enough. P9 uses much more MMIOs than 
> >> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
> > 
> > Well, it's certainly *possible* to use the same machine type, I'm just
> > not convinced it's a good idea.  It seems kind of dodgy to me that so
> > many peripherals on the system change as a side-effect of setting the
> > cpu.  Compare to how x86 works where cpu really does change the CPU,
> > plugging it into the same virtual "chipset".  Different chipsets *are*
> > different machine types there (pc vs. q35).
> 
> OK, I agree, and we can use a set of common routines to instantiate the 
> different chipset models. 
> 
> So we would have a common pnv_init() routine to initialize the different 
> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
> machine class attribute ?

Well.. that's one option.  Usually for these things, it works out
better to instead of parameterizing big high-level routines like
pnv_init(), you have separate versions of those calling a combination
of case-specific and common routines as necessary.

Mostly it just comes down to what is simplest to implement for you, though.

> Nevertheless we would still need to introduce "a physical mapping array 
> describing MMIO ranges" but we can start by differentiating the chipsets 
> first.

Well, maybe.  I'm wondering if you can more easily encapsulate the
information in that array in a top-level init routine, that calls
common helpers with different addresses / device types / whatever.

On 06/13/2018 02:47 AM, David Gibson wrote:
> On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote:
>> On 06/12/2018 07:58 AM, David Gibson wrote:
>>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
>>>> On 06/06/2018 08:39 AM, David Gibson wrote:
>>>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
>>>>>> Based on previous work done in skiboot, the physical mapping array
>>>>>> helps in calculating the MMIO ranges of each controller depending on
>>>>>> the chip id and the chip type. This is will be particularly useful for
>>>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>>>>>
>>>>>> A link on the chip is now necessary to calculate MMIO BARs and
>>>>>> sizes. This is why such a link is introduced in the PSIHB model.
>>>>>
>>>>> I think this message needs some work.  This says what it's for, but
>>>>> what actually *is* this array, and how does it work?
>>>>
>>>> OK. It is relatively simple: each controller has an entry defining its 
>>>> MMIO range. 
>>>>
>>>>> The outside-core differences between POWER8 and POWER9 are substantial
>>>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
>>>>> different machine types (sharing some routines, of course).
>>>>
>>>> yes and no. I have survived using a common PnvChip framework but
>>>> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
>>>> They are very similar but not enough. P9 uses much more MMIOs than 
>>>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
>>>
>>> Well, it's certainly *possible* to use the same machine type, I'm just
>>> not convinced it's a good idea.  It seems kind of dodgy to me that so
>>> many peripherals on the system change as a side-effect of setting the
>>> cpu.  Compare to how x86 works where cpu really does change the CPU,
>>> plugging it into the same virtual "chipset".  Different chipsets *are*
>>> different machine types there (pc vs. q35).
>>
>> OK, I agree, and we can use a set of common routines to instantiate the 
>> different chipset models. 
>>
>> So we would have a common pnv_init() routine to initialize the different 
>> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
>> machine class attribute ?
> 
> Well.. that's one option.  Usually for these things, it works out
> better to instead of parameterizing big high-level routines like
> pnv_init(), you have separate versions of those calling a combination
> of case-specific and common routines as necessary.
> 
> Mostly it just comes down to what is simplest to implement for you, though.

I am introducing a powernv8 machine, the machine init routine is still
generic and did not change much. But I have deepen the PnvChip class
inheritance hierarchy with an intermediate class taking care of the
Chip sub controllers, which gives us something like :

  pnv_init()
    . skiboot
    . kernel
    . initrd
    . chips creation
    . platform bus/device :
       isa bus
       pci layout
       bmc handling.

  p8 chip hierarchy:
 
   power8_v2.0-pnv-chip (gives the cpu type)
   pnv8-chip   : creates the devices only       
   pnv-chip    : creates xscom and the cores 

The powervn9 machine has this hierarchy :

   power9_v2.0-pnv-chip
   pnv9-chip
   pnv-chip

I had to introduce these new PnvChipClass ops : 

    void (*realize)(PnvChip *chip, Error **errp);
    Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
    ISABus *(*isa_create)(PnvChip *chip);

Overall, it's looking fine and it should remove most of these tests :

	 pnv_chip_is_power9(chip)

If not, it means we are missing a PnvChipClass ops anyway.

I will send a patchset this week, the final one will shuffle quite a
bit of code and the resulting diff will be a bit fuzy. You will have
to trust me on this one.

 
>> Nevertheless we would still need to introduce "a physical mapping array 
>> describing MMIO ranges" but we can start by differentiating the chipsets 
>> first.
> 
> Well, maybe.  I'm wondering if you can more easily encapsulate the
> information in that array in a top-level init routine, that calls
> common helpers with different addresses / device types / whatever.

Hmm, I think I understand but could you give me a prototype example. 
Please. To make sure.

I would like to keep the array somewhere because, in a quick look, 
it gives you an overview of the POWER Processor address space. 

Thanks,

C.

On Wed, Jun 13, 2018 at 09:03:22AM +0200, Cédric Le Goater wrote:
> On 06/13/2018 02:47 AM, David Gibson wrote:
> > On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote:
> >> On 06/12/2018 07:58 AM, David Gibson wrote:
> >>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
> >>>> On 06/06/2018 08:39 AM, David Gibson wrote:
> >>>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
> >>>>>> Based on previous work done in skiboot, the physical mapping array
> >>>>>> helps in calculating the MMIO ranges of each controller depending on
> >>>>>> the chip id and the chip type. This is will be particularly useful for
> >>>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
> >>>>>>
> >>>>>> A link on the chip is now necessary to calculate MMIO BARs and
> >>>>>> sizes. This is why such a link is introduced in the PSIHB model.
> >>>>>
> >>>>> I think this message needs some work.  This says what it's for, but
> >>>>> what actually *is* this array, and how does it work?
> >>>>
> >>>> OK. It is relatively simple: each controller has an entry defining its 
> >>>> MMIO range. 
> >>>>
> >>>>> The outside-core differences between POWER8 and POWER9 are substantial
> >>>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
> >>>>> different machine types (sharing some routines, of course).
> >>>>
> >>>> yes and no. I have survived using a common PnvChip framework but
> >>>> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
> >>>> They are very similar but not enough. P9 uses much more MMIOs than 
> >>>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
> >>>
> >>> Well, it's certainly *possible* to use the same machine type, I'm just
> >>> not convinced it's a good idea.  It seems kind of dodgy to me that so
> >>> many peripherals on the system change as a side-effect of setting the
> >>> cpu.  Compare to how x86 works where cpu really does change the CPU,
> >>> plugging it into the same virtual "chipset".  Different chipsets *are*
> >>> different machine types there (pc vs. q35).
> >>
> >> OK, I agree, and we can use a set of common routines to instantiate the 
> >> different chipset models. 
> >>
> >> So we would have a common pnv_init() routine to initialize the different 
> >> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
> >> machine class attribute ?
> > 
> > Well.. that's one option.  Usually for these things, it works out
> > better to instead of parameterizing big high-level routines like
> > pnv_init(), you have separate versions of those calling a combination
> > of case-specific and common routines as necessary.
> > 
> > Mostly it just comes down to what is simplest to implement for you, though.
> 
> I am introducing a powernv8 machine, the machine init routine is still
> generic and did not change much. But I have deepen the PnvChip class
> inheritance hierarchy with an intermediate class taking care of the
> Chip sub controllers, which gives us something like :
> 
>   pnv_init()
>     . skiboot
>     . kernel
>     . initrd
>     . chips creation
>     . platform bus/device :
>        isa bus
>        pci layout
>        bmc handling.
> 
>   p8 chip hierarchy:
>  
>    power8_v2.0-pnv-chip (gives the cpu type)
>    pnv8-chip   : creates the devices only       
>    pnv-chip    : creates xscom and the cores 
> 
> The powervn9 machine has this hierarchy :
> 
>    power9_v2.0-pnv-chip
>    pnv9-chip
>    pnv-chip
> 
> I had to introduce these new PnvChipClass ops : 
> 
>     void (*realize)(PnvChip *chip, Error **errp);

Looks sensible up to this point.

>     Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>     ISABus *(*isa_create)(PnvChip *chip);

I'm a little more dubious about this - I would have thought your
realize() hook could construct the right intc and isa, rather than
going into generic code, then back out to the callback.

But it's not a big deal.

> Overall, it's looking fine and it should remove most of these tests :
> 
> 	 pnv_chip_is_power9(chip)
> 
> If not, it means we are missing a PnvChipClass ops anyway.

Nice.

> I will send a patchset this week, the final one will shuffle quite a
> bit of code and the resulting diff will be a bit fuzy. You will have
> to trust me on this one.
> 
>  
> >> Nevertheless we would still need to introduce "a physical mapping array 
> >> describing MMIO ranges" but we can start by differentiating the chipsets 
> >> first.
> > 
> > Well, maybe.  I'm wondering if you can more easily encapsulate the
> > information in that array in a top-level init routine, that calls
> > common helpers with different addresses / device types / whatever.
> 
> Hmm, I think I understand but could you give me a prototype example. 
> Please. To make sure.
> 
> I would like to keep the array somewhere because, in a quick look, 
> it gives you an overview of the POWER Processor address space.

Ok.  Going to a data-driven approach for constructing things sounds
like it might be a reasonable plan in its own right.  But I'd prefer
not to conflate with other structural questions.

On 06/14/2018 08:44 AM, David Gibson wrote:
> On Wed, Jun 13, 2018 at 09:03:22AM +0200, Cédric Le Goater wrote:
>> On 06/13/2018 02:47 AM, David Gibson wrote:
>>> On Tue, Jun 12, 2018 at 08:13:49AM +0200, Cédric Le Goater wrote:
>>>> On 06/12/2018 07:58 AM, David Gibson wrote:
>>>>> On Wed, Jun 06, 2018 at 09:04:10AM +0200, Cédric Le Goater wrote:
>>>>>> On 06/06/2018 08:39 AM, David Gibson wrote:
>>>>>>> On Wed, May 30, 2018 at 12:07:54PM +0200, Cédric Le Goater wrote:
>>>>>>>> Based on previous work done in skiboot, the physical mapping array
>>>>>>>> helps in calculating the MMIO ranges of each controller depending on
>>>>>>>> the chip id and the chip type. This is will be particularly useful for
>>>>>>>> the P9 models which use less the XSCOM bus and rely more on MMIOs.
>>>>>>>>
>>>>>>>> A link on the chip is now necessary to calculate MMIO BARs and
>>>>>>>> sizes. This is why such a link is introduced in the PSIHB model.
>>>>>>>
>>>>>>> I think this message needs some work.  This says what it's for, but
>>>>>>> what actually *is* this array, and how does it work?
>>>>>>
>>>>>> OK. It is relatively simple: each controller has an entry defining its 
>>>>>> MMIO range. 
>>>>>>
>>>>>>> The outside-core differences between POWER8 and POWER9 are substantial
>>>>>>> enough that I'm wondering if pnvP8 and pnvP9 would be better off as
>>>>>>> different machine types (sharing some routines, of course).
>>>>>>
>>>>>> yes and no. I have survived using a common PnvChip framework but
>>>>>> it is true that I had to add P9 classes for each: LPC, PSI, OCC 
>>>>>> They are very similar but not enough. P9 uses much more MMIOs than 
>>>>>> P8 which still uses a lot of XSCOM. I haven't looked at PHB4. 
>>>>>
>>>>> Well, it's certainly *possible* to use the same machine type, I'm just
>>>>> not convinced it's a good idea.  It seems kind of dodgy to me that so
>>>>> many peripherals on the system change as a side-effect of setting the
>>>>> cpu.  Compare to how x86 works where cpu really does change the CPU,
>>>>> plugging it into the same virtual "chipset".  Different chipsets *are*
>>>>> different machine types there (pc vs. q35).
>>>>
>>>> OK, I agree, and we can use a set of common routines to instantiate the 
>>>> different chipset models. 
>>>>
>>>> So we would have a common pnv_init() routine to initialize the different 
>>>> 'powernv8' and 'powernv9' machines and the PnvChip typename would be a 
>>>> machine class attribute ?
>>>
>>> Well.. that's one option.  Usually for these things, it works out
>>> better to instead of parameterizing big high-level routines like
>>> pnv_init(), you have separate versions of those calling a combination
>>> of case-specific and common routines as necessary.
>>>
>>> Mostly it just comes down to what is simplest to implement for you, though.
>>
>> I am introducing a powernv8 machine, the machine init routine is still
>> generic and did not change much. But I have deepen the PnvChip class
>> inheritance hierarchy with an intermediate class taking care of the
>> Chip sub controllers, which gives us something like :
>>
>>   pnv_init()
>>     . skiboot
>>     . kernel
>>     . initrd
>>     . chips creation
>>     . platform bus/device :
>>        isa bus
>>        pci layout
>>        bmc handling.
>>
>>   p8 chip hierarchy:
>>  
>>    power8_v2.0-pnv-chip (gives the cpu type)
>>    pnv8-chip   : creates the devices only       
>>    pnv-chip    : creates xscom and the cores 
>>
>> The powervn9 machine has this hierarchy :
>>
>>    power9_v2.0-pnv-chip
>>    pnv9-chip
>>    pnv-chip
>>
>> I had to introduce these new PnvChipClass ops : 
>>
>>     void (*realize)(PnvChip *chip, Error **errp);
> 
> Looks sensible up to this point.

yes. 

The common part only initializes the xscom bus. I wished we could create 
also the cores but, on the P9, we need the XIVE interrupt controller
to be realized before :/ 

> 
>>     Object *(*intc_create)(PnvChip *chip, Object *child, Error **errp);
>>     ISABus *(*isa_create)(PnvChip *chip);
> 
> I'm a little more dubious about this - I would have thought your
> realize() hook could construct the right intc and isa, rather than
> going into generic code, then back out to the callback.

The 'intc_create' op creates the interrupt presenter. it is called from 
the powernv core realize routine and each chip has a different interrupt 
controller, and so a different presenter type. We cannot create the
presenter before and 'pass' it to the core object at realize time 
because the presenter links in the CPU state. It's a bit of a chicken
and egg problem, but I think it is good to have a presenter object
all well initialized before we use it. I am open to any suggestions
to get rid of this op. sPAPR has exactly the same need.

As for the isa_create op, this is because there is only one isa_bus 
per machine, but you might be right. We can probably create the isa
bus on all chips and link the machine to the one on chip0. It should 
save the op.


> But it's not a big deal.
> 
>> Overall, it's looking fine and it should remove most of these tests :
>>
>> 	 pnv_chip_is_power9(chip)
>>
>> If not, it means we are missing a PnvChipClass ops anyway.
> 
> Nice.
> 
>> I will send a patchset this week, the final one will shuffle quite a
>> bit of code and the resulting diff will be a bit fuzy. You will have
>> to trust me on this one.
>>
>>  
>>>> Nevertheless we would still need to introduce "a physical mapping array 
>>>> describing MMIO ranges" but we can start by differentiating the chipsets 
>>>> first.
>>>
>>> Well, maybe.  I'm wondering if you can more easily encapsulate the
>>> information in that array in a top-level init routine, that calls
>>> common helpers with different addresses / device types / whatever.
>>
>> Hmm, I think I understand but could you give me a prototype example. 
>> Please. To make sure.
>>
>> I would like to keep the array somewhere because, in a quick look, 
>> it gives you an overview of the POWER Processor address space.
> 
> Ok.  Going to a data-driven approach for constructing things sounds
> like it might be a reasonable plan in its own right.  But I'd prefer
> not to conflate with other structural questions.
> 

OK. It can come later with XIVE, which needs 4 extra MMIO regions.

Thanks,

C.