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 <c...@kaod.org> >>>> Reviewed-by: Philippe Mathieu-Daudé <f4...@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; >>> >> >