On Wed, 23 May 2018 14:37:30 +0200
Cédric Le Goater <c...@kaod.org> wrote:
> On 05/18/2018 11:00 AM, Greg Kurz wrote:
> > On Thu, 17 May 2018 15:18:14 +0200
> > Cédric Le Goater <c...@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 <c...@kaod.org>
> >> ---
> >> hw/ppc/pnv.c | 32 +++++++++++++++++++++--------
> >> hw/ppc/pnv_psi.c | 11 +++++++++-
> >> hw/ppc/pnv_xscom.c | 8 ++++----
> >> include/hw/ppc/pnv.h | 58
> >> +++++++++++++++++++++++++++++++++++++---------------
> >> 4 files changed, 80 insertions(+), 29 deletions(-)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 031488131629..330bf6f74810 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -712,6 +712,16 @@ 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 void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -721,7 +731,7 @@ static void pnv_chip_power8e_class_init(ObjectClass
> >> *klass, void *data)
> >> k->chip_cfam_id = 0x221ef04980000000ull; /* P8 Murano DD2.1 */
> >> k->cores_mask = POWER8E_CORE_MASK;
> >> k->core_pir = pnv_chip_core_pir_p8;
> >> - k->xscom_base = 0x003fc0000000000ull;
> >> + k->phys_map = pnv_chip_power8_phys_map;
> >> dc->desc = "PowerNV Chip POWER8E";
> >> }
> >>
> >> @@ -734,7 +744,7 @@ static void pnv_chip_power8_class_init(ObjectClass
> >> *klass, void *data)
> >> k->chip_cfam_id = 0x220ea04980000000ull; /* P8 Venice DD2.0 */
> >> k->cores_mask = POWER8_CORE_MASK;
> >> k->core_pir = pnv_chip_core_pir_p8;
> >> - k->xscom_base = 0x003fc0000000000ull;
> >> + k->phys_map = pnv_chip_power8_phys_map;
> >> dc->desc = "PowerNV Chip POWER8";
> >> }
> >>
> >> @@ -747,10 +757,17 @@ 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->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, 0x0000040000000000ull
> >> },
> >> +};
> >> +
> >> static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
> >> {
> >> DeviceClass *dc = DEVICE_CLASS(klass);
> >> @@ -760,7 +777,7 @@ static void pnv_chip_power9_class_init(ObjectClass
> >> *klass, void *data)
> >> k->chip_cfam_id = 0x220d104900008000ull; /* P9 Nimbus DD2.0 */
> >> k->cores_mask = POWER9_CORE_MASK;
> >> k->core_pir = pnv_chip_core_pir_p9;
> >> - k->xscom_base = 0x00603fc00000000ull;
> >> + k->phys_map = pnv_chip_power9_phys_map;
> >> dc->desc = "PowerNV Chip POWER9";
> >> }
> >>
> >> @@ -797,15 +814,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 +845,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..dd7707b971c9 100644
> >> --- a/hw/ppc/pnv_psi.c
> >> +++ b/hw/ppc/pnv_psi.c
> >> @@ -465,6 +465,15 @@ 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_setg(errp, "%s: required link 'chip' not found: %s",
> >> + __func__, error_get_pretty(err));
> >> + return;
> >
> > err was dynamically allocated and must be freed with error_free().
>
> ok. There are quite a few places not doing it right then.
>
Yeah, error_get_pretty users are the usual suspects here.
> > This being said, the link is supposed to have been set in pnv_chip_init()
> > already, so if we can't get it here, it's likely a bug in QEMU. I'd rather
> > pass &error_abort.
>
> The followed pattern in pnv is most of the time :
>
> object_property_add_const_link(OBJECT(&chip->bar), "foo", obj,
> &error_abort);
>
> and in the realize function :
>
> Error *local_err = NULL;
>
> obj = object_property_get_link(OBJECT(dev), "foo", &local_err);
> if (!obj) {
> error_setg(errp, "%s: required link 'foo' not found: %s",
> __func__, error_get_pretty(local_err));
> return;
> }
>
> but you propose that we should rather simply do :
>
> obj = object_property_get_link(OBJECT(dev), "foo", &error_abort);
>
> Correct ?
>
Yes, but this is questionable :)
The realize function has an Error ** arg, ie, it is expected to propagate
errors and let the caller decide... so it is a matter of taste here.
Another option to using error_get_pretty() is to use error_propagate() and
error_prepend(), as suggested in include/qapi/error.h .
See commit a1a6bbde4f6a29368f8f605cea2e73630ec1bc7c for an example.
> >> + }
> >> + chip = PNV_CHIP(obj);
> >>
> >> obj = object_property_get_link(OBJECT(dev), "xics", &err);
> >> if (!obj) {
> >> @@ -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;
> >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> >> index 90759240a7b1..b810e7b84ec0 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,19 @@ typedef struct PnvChip {
> >> PnvOCC occ;
> >> } PnvChip;
> >>
> >> +typedef enum PnvPhysMapType {
> >> + PNV_MAP_XSCOM,
> >> + PNV_MAP_ICP,
> >> + PNV_MAP_PSIHB,
> >> + PNV_MAP_PSIHB_FSP,
> >> + PNV_MAP_MAX,
> >> +} PnvPhysMapType;
> >> +
> >> +typedef struct PnvPhysMapEntry {
> >> + uint64_t base;
> >> + uint64_t size;
> >> +} PnvPhysMapEntry;
> >> +
> >> typedef struct PnvChipClass {
> >> /*< private >*/
> >> SysBusDeviceClass parent_class;
> >> @@ -73,7 +85,7 @@ 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);
> >> } PnvChipClass;
> >> @@ -159,9 +171,28 @@ 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)
> >> +{
> >> + PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
> >> + const PnvPhysMapEntry *map = &pcc->phys_map[type];
> >> +
> >> + if (pnv_chip_is_power9(chip)) {
> >> + return map->base + ((uint64_t) chip->chip_id << 42);
> >
> > So this patch also changes the way the base address is computed
> > with POWER9. Shouldn't this go to a separate patch or at least
> > mention the functional change in the changelog ?
>
> P9 is not really supported, multi P9 chips even less. But yes, I can
> mention that the P9 MMIO base address is being fixed.
>
> > Also, shouldn't this be a PnvChipClass level function rather than
> > doing a runtime check ?
>
> Hmm. pnv_map_base() needs the 'chip_id' to calculate the base
> address which is not a class level attribute. The P9 check only
> needs the chip class but it is more convenient to use a PnvChip.
> Only class_init routines use class variables.
Not sure to understand... My suggestion was just to add a new function
attribute to PnvChipClass:
uint64_t (*map_base)(const PnvChip *chip, PnvPhysMapType type);
And have two separate implementations for P8 and P9:
static uint64_t pnv_chip_map_base_p8(const PnvChip *chip,
const PnvPhysMapEntry *map)
{
return map->base + chip->chip_id * map->size;
}
static uint64_t pnv_chip_map_base_p9(const PnvChip *chip,
const PnvPhysMapEntry *map)
{
return map->base + ((uint64_t) chip->chip_id << 42);
}
And pnv_map_base() would be:
static inline uint64_t pnv_map_base(const PnvChip *chip, PnvPhysMapType type)
{
PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
return pcc->map_base(chip, &pcc->phys_map[type]);
}
> > The rest of the patch looks good.
>
> Thanks,
>
> C.
>
> >> + } else {
> >> + return map->base + chip->chip_id * map->size;
> >> + }
> >> +}
> >> +
> >> +#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 +208,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 */
> >
>
