Re: [Qemu-ppc] [PATCH v2] pnv: add a physical mapping array describing MMIO ranges in each chip

[David Gibson]

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 <address@hidden>
> >> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> >> ---
> >>
> >>  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;
> > 
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature