| Message ID | 157623836852.360005.1112241220707384093.stgit@bahia.lan (mailing list archive) |
|---|---|
| Headers | show |
| Series | ppc/pnv: Get rid of chip_type attributes | expand |
On 13/12/2019 12:59, Greg Kurz wrote: > The pnv_dt_create() function generates different contents for the > "compatible" property of the root node in the DT, depending on the > CPU type. This is open coded with multiple ifs using pnv_is_powerXX() > helpers. > > It seems cleaner to achieve with QOM. Introduce a base class for the > powernv machine and a compat attribute that each child class can use > to provide the value for the "compatible" property. > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv.c | 33 +++++++++++++++++++-------------- > include/hw/ppc/pnv.h | 13 +++++++++++++ > 2 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 0be0b6b411c3..5ac149b149d8 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt) > > static void *pnv_dt_create(MachineState *machine) > { > - const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > - const char plat_compat9[] = "qemu,powernv9\0ibm,powernv"; > - const char plat_compat10[] = "qemu,powernv10\0ibm,powernv"; > + PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine); > PnvMachineState *pnv = PNV_MACHINE(machine); > void *fdt; > char *buf; > @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine) > _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2))); > _FDT((fdt_setprop_string(fdt, 0, "model", > "IBM PowerNV (emulated by qemu)"))); > - if (pnv_is_power10(pnv)) { > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10, > - sizeof(plat_compat10)))); > - } else if (pnv_is_power9(pnv)) { > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9, > - sizeof(plat_compat9)))); > - } else { > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8, > - sizeof(plat_compat8)))); > - } > - > + _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat, > + sizeof(pmc->compat)))); > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf))); > @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > + static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); > @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > xic->icp_get = pnv_icp_get; > xic->ics_get = pnv_ics_get; > xic->ics_resend = pnv_ics_resend; > + > + pmc->compat = compat; > + pmc->compat_size = sizeof(compat); > } > > static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc); > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > + static const char compat[] = "qemu,powernv9\0ibm,powernv"; > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER9"; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0"); > xfc->match_nvt = pnv_match_nvt; > > mc->alias = "powernv"; > + > + pmc->compat = compat; > + pmc->compat_size = sizeof(compat); > } > > static void pnv_machine_power10_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > + static const char compat[] = "qemu,powernv10\0ibm,powernv"; > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER10"; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0"); > + > + pmc->compat = compat; > + pmc->compat_size = sizeof(compat); > } > > static void pnv_machine_class_init(ObjectClass *oc, void *data) > @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = { > .instance_size = sizeof(PnvMachineState), > .instance_init = pnv_machine_instance_init, > .class_init = pnv_machine_class_init, > + .class_size = sizeof(PnvMachineClass), > .interfaces = (InterfaceInfo[]) { > { TYPE_INTERRUPT_STATS_PROVIDER }, > { }, > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 92f80b1ccead..d534746bd493 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir); > #define TYPE_PNV_MACHINE MACHINE_TYPE_NAME("powernv") > #define PNV_MACHINE(obj) \ > OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE) > +#define PNV_MACHINE_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE) > +#define PNV_MACHINE_CLASS(klass) \ > + OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE) > + > +typedef struct PnvMachineClass { > + /*< private >*/ > + MachineClass parent_class; > + > + /*< public >*/ > + const char *compat; > + int compat_size; > +} PnvMachineClass; > > typedef struct PnvMachineState { > /*< private >*/ >
On 13/12/2019 13:00, Greg Kurz wrote: > The pnv_pic_print_info() callback checks the type of the chip in order > to forward to the request appropriate interrupt controller. This can > be achieved with QOM. Introduce a method for this in the base chip class > and implement it in child classes. > > This also prepares ground for the upcoming interrupt controller of POWER10 > chips. > > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> One comment below. > --- > hw/ppc/pnv.c | 30 +++++++++++++++++++++++++----- > include/hw/ppc/pnv.h | 1 + > 2 files changed, 26 insertions(+), 5 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index efc00f4cb67a..2a53e99bda2e 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > pnv_cpu->intc = NULL; > } > > +static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > + Monitor *mon) > +{ > + icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon); > +} > + > /* > * 0:48 Reserved - Read as zeroes > * 49:52 Node ID > @@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > pnv_cpu->intc = NULL; > } > > +static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > + Monitor *mon) > +{ > + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > +} > + > static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu, > Error **errp) > { > @@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > pnv_cpu->intc = NULL; > } > > +static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > + Monitor *mon) > +{ > +} > + > /* > * Allowed core identifiers on a POWER8 Processor Chip : > * > @@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > k->intc_destroy = pnv_chip_power8_intc_destroy; > + k->intc_print_info = pnv_chip_power8_intc_print_info; > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > k->intc_destroy = pnv_chip_power8_intc_destroy; > + k->intc_print_info = pnv_chip_power8_intc_print_info; > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > k->intc_create = pnv_chip_power8_intc_create; > k->intc_reset = pnv_chip_power8_intc_reset; > k->intc_destroy = pnv_chip_power8_intc_destroy; > + k->intc_print_info = pnv_chip_power8_intc_print_info; > k->isa_create = pnv_chip_power8nvl_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > @@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > k->intc_create = pnv_chip_power9_intc_create; > k->intc_reset = pnv_chip_power9_intc_reset; > k->intc_destroy = pnv_chip_power9_intc_destroy; > + k->intc_print_info = pnv_chip_power9_intc_print_info; > k->isa_create = pnv_chip_power9_isa_create; > k->dt_populate = pnv_chip_power9_dt_populate; > k->pic_print_info = pnv_chip_power9_pic_print_info; > @@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > k->intc_create = pnv_chip_power10_intc_create; > k->intc_reset = pnv_chip_power10_intc_reset; > k->intc_destroy = pnv_chip_power10_intc_destroy; > + k->intc_print_info = pnv_chip_power10_intc_print_info; > k->isa_create = pnv_chip_power10_isa_create; > k->dt_populate = pnv_chip_power10_dt_populate; > k->pic_print_info = pnv_chip_power10_pic_print_info; > @@ -1575,11 +1597,9 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj, > CPU_FOREACH(cs) { > PowerPCCPU *cpu = POWERPC_CPU(cs); > > - if (pnv_chip_is_power9(pnv->chips[0])) { > - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > - } else { > - icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon); > - } > + /* XXX: loop on each chip/core/thread instead of CPU_FOREACH() */ May be we should introduce a helper such as : int pnv_chip_cpu_foreach(PnvChip *chip, int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque) { int i, j; int ret = 0; for (i = 0; i < chip->nr_cores; i++) { PnvCore *pc = chip->cores[i]; CPUCore *cc = CPU_CORE(pc); for (j = 0; j < cc->nr_threads; j++) { PowerPCCPU *cpu = pc->threads[j]; ret = doit(chip, cpu, opaque); if (ret) { break; } } } return ret; } > + PNV_CHIP_GET_CLASS(pnv->chips[0])->intc_print_info(pnv->chips[0], cpu, > + mon); > } > > for (i = 0; i < pnv->num_chips; i++) { > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index c213bdd5ecd3..7d2402784d4b 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -133,6 +133,7 @@ typedef struct PnvChipClass { > void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp); > void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu); > void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu); > + void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon); > ISABus *(*isa_create)(PnvChip *chip, Error **errp); > void (*dt_populate)(PnvChip *chip, void *fdt); > void (*pic_print_info)(PnvChip *chip, Monitor *mon); >
On 13/12/2019 13:00, Greg Kurz wrote: > The pnv_chip_core_realize() function configures the XSCOM MMIO subregion > for each core of a single chip. The base address of the subregion depends > on the CPU type. Its computation is currently open-code using the > pnv_chip_is_powerXX() helpers. This can be achieved with QOM. Introduce > a method for this in the base chip class and implement it in child classes. OK. We might need to introduce a PnvXscom model one day but this is fine for now. > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv.c | 31 ++++++++++++++++++++++++------- > include/hw/ppc/pnv.h | 1 + > 2 files changed, 25 insertions(+), 7 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 2a53e99bda2e..88efa755e611 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -616,6 +616,24 @@ static void pnv_chip_power9_pic_print_info(PnvChip *chip, Monitor *mon) > pnv_psi_pic_print_info(&chip9->psi, mon); > } > > +static uint64_t pnv_chip_power8_xscom_core_base(PnvChip *chip, > + uint32_t core_id) > +{ > + return PNV_XSCOM_EX_BASE(core_id); > +} > + > +static uint64_t pnv_chip_power9_xscom_core_base(PnvChip *chip, > + uint32_t core_id) > +{ > + return PNV9_XSCOM_EC_BASE(core_id); > +} > + > +static uint64_t pnv_chip_power10_xscom_core_base(PnvChip *chip, > + uint32_t core_id) > +{ > + return PNV10_XSCOM_EC_BASE(core_id); > +} > + > static bool pnv_match_cpu(const char *default_type, const char *cpu_type) > { > PowerPCCPUClass *ppc_default = > @@ -1107,6 +1125,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > + k->xscom_core_base = pnv_chip_power8_xscom_core_base; > dc->desc = "PowerNV Chip POWER8E"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1129,6 +1148,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > k->isa_create = pnv_chip_power8_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > + k->xscom_core_base = pnv_chip_power8_xscom_core_base; > dc->desc = "PowerNV Chip POWER8"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1151,6 +1171,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > k->isa_create = pnv_chip_power8nvl_isa_create; > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > + k->xscom_core_base = pnv_chip_power8_xscom_core_base; > dc->desc = "PowerNV Chip POWER8NVL"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1323,6 +1344,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > k->isa_create = pnv_chip_power9_isa_create; > k->dt_populate = pnv_chip_power9_dt_populate; > k->pic_print_info = pnv_chip_power9_pic_print_info; > + k->xscom_core_base = pnv_chip_power9_xscom_core_base; > dc->desc = "PowerNV Chip POWER9"; > > device_class_set_parent_realize(dc, pnv_chip_power9_realize, > @@ -1404,6 +1426,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > k->isa_create = pnv_chip_power10_isa_create; > k->dt_populate = pnv_chip_power10_dt_populate; > k->pic_print_info = pnv_chip_power10_pic_print_info; > + k->xscom_core_base = pnv_chip_power10_xscom_core_base; > dc->desc = "PowerNV Chip POWER10"; > > device_class_set_parent_realize(dc, pnv_chip_power10_realize, > @@ -1491,13 +1514,7 @@ static void pnv_chip_core_realize(PnvChip *chip, Error **errp) > &error_fatal); > > /* Each core has an XSCOM MMIO region */ > - if (pnv_chip_is_power10(chip)) { > - xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid); > - } else if (pnv_chip_is_power9(chip)) { > - xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid); > - } else { > - xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid); > - } > + xscom_core_base = pcc->xscom_core_base(chip, core_hwid); > > pnv_xscom_add_subregion(chip, xscom_core_base, > &pnv_core->xscom_regs); > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 7d2402784d4b..17ca9a14ac8f 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -137,6 +137,7 @@ typedef struct PnvChipClass { > ISABus *(*isa_create)(PnvChip *chip, Error **errp); > void (*dt_populate)(PnvChip *chip, void *fdt); > void (*pic_print_info)(PnvChip *chip, Monitor *mon); > + uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id); > } PnvChipClass; > > #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP >
On 13/12/2019 13:00, Greg Kurz wrote: > Since pnv_dt_xscom() is called from chip specific dt_populate() hooks, > it shouldn't have to guess the chip type in order to populate the > "compatible" property. Just pass the compat string and its size as > arguments. Yeah. That is where I think a PnXscom model and class could be a little cleaner. This is minor. > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv.c | 12 +++++++++--- > hw/ppc/pnv_xscom.c | 20 +++----------------- > include/hw/ppc/pnv_xscom.h | 3 ++- > 3 files changed, 14 insertions(+), 21 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index c532e98e752a..0447b534b8c5 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -280,11 +280,13 @@ static void pnv_dt_icp(PnvChip *chip, void *fdt, uint32_t pir, > > static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt) > { > + static const char compat[] = "ibm,power8-xscom\0ibm,xscom"; > int i; > > pnv_dt_xscom(chip, fdt, 0, > cpu_to_be64(PNV_XSCOM_BASE(chip)), > - cpu_to_be64(PNV_XSCOM_SIZE)); > + cpu_to_be64(PNV_XSCOM_SIZE), > + compat, sizeof(compat)); > > for (i = 0; i < chip->nr_cores; i++) { > PnvCore *pnv_core = chip->cores[i]; > @@ -302,11 +304,13 @@ static void pnv_chip_power8_dt_populate(PnvChip *chip, void *fdt) > > static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt) > { > + static const char compat[] = "ibm,power9-xscom\0ibm,xscom"; > int i; > > pnv_dt_xscom(chip, fdt, 0, > cpu_to_be64(PNV9_XSCOM_BASE(chip)), > - cpu_to_be64(PNV9_XSCOM_SIZE)); > + cpu_to_be64(PNV9_XSCOM_SIZE), > + compat, sizeof(compat)); > > for (i = 0; i < chip->nr_cores; i++) { > PnvCore *pnv_core = chip->cores[i]; > @@ -323,11 +327,13 @@ static void pnv_chip_power9_dt_populate(PnvChip *chip, void *fdt) > > static void pnv_chip_power10_dt_populate(PnvChip *chip, void *fdt) > { > + static const char compat[] = "ibm,power10-xscom\0ibm,xscom"; > int i; > > pnv_dt_xscom(chip, fdt, 0, > cpu_to_be64(PNV10_XSCOM_BASE(chip)), > - cpu_to_be64(PNV10_XSCOM_SIZE)); > + cpu_to_be64(PNV10_XSCOM_SIZE), > + compat, sizeof(compat)); > > for (i = 0; i < chip->nr_cores; i++) { > PnvCore *pnv_core = chip->cores[i]; > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c > index 8189767eb0bb..5ae9dfbb88ad 100644 > --- a/hw/ppc/pnv_xscom.c > +++ b/hw/ppc/pnv_xscom.c > @@ -282,12 +282,9 @@ static int xscom_dt_child(Object *child, void *opaque) > return 0; > } > > -static const char compat_p8[] = "ibm,power8-xscom\0ibm,xscom"; > -static const char compat_p9[] = "ibm,power9-xscom\0ibm,xscom"; > -static const char compat_p10[] = "ibm,power10-xscom\0ibm,xscom"; > - > int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset, > - uint64_t xscom_base, uint64_t xscom_size) > + uint64_t xscom_base, uint64_t xscom_size, > + const char *compat, int compat_size) > { > uint64_t reg[] = { xscom_base, xscom_size }; > int xscom_offset; > @@ -302,18 +299,7 @@ int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset, > _FDT((fdt_setprop_cell(fdt, xscom_offset, "#address-cells", 1))); > _FDT((fdt_setprop_cell(fdt, xscom_offset, "#size-cells", 1))); > _FDT((fdt_setprop(fdt, xscom_offset, "reg", reg, sizeof(reg)))); > - > - if (pnv_chip_is_power10(chip)) { > - _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p10, > - sizeof(compat_p10)))); > - } else if (pnv_chip_is_power9(chip)) { > - _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p9, > - sizeof(compat_p9)))); > - } else { > - _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat_p8, > - sizeof(compat_p8)))); > - } > - > + _FDT((fdt_setprop(fdt, xscom_offset, "compatible", compat, compat_size))); > _FDT((fdt_setprop(fdt, xscom_offset, "scom-controller", NULL, 0))); > > args.fdt = fdt; > diff --git a/include/hw/ppc/pnv_xscom.h b/include/hw/ppc/pnv_xscom.h > index ad53f788b44c..f74c81a980f3 100644 > --- a/include/hw/ppc/pnv_xscom.h > +++ b/include/hw/ppc/pnv_xscom.h > @@ -115,7 +115,8 @@ typedef struct PnvXScomInterfaceClass { > > void pnv_xscom_realize(PnvChip *chip, uint64_t size, Error **errp); > int pnv_dt_xscom(PnvChip *chip, void *fdt, int root_offset, > - uint64_t xscom_base, uint64_t xscom_size); > + uint64_t xscom_base, uint64_t xscom_size, > + const char *compat, int compat_size); > > void pnv_xscom_add_subregion(PnvChip *chip, hwaddr offset, > MemoryRegion *mr); >
On 13/12/2019 13:00, Greg Kurz wrote: > They aren't used anymore. Good ! > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > include/hw/ppc/pnv.h | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 17ca9a14ac8f..7a134a15d3b5 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -224,21 +224,11 @@ struct PnvMachineState { > PnvPnor *pnor; > }; > > -static inline bool pnv_chip_is_power9(const PnvChip *chip) > -{ > - return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER9; > -} > - > PnvChip *pnv_get_chip(uint32_t chip_id); > > #define PNV_FDT_ADDR 0x01000000 > #define PNV_TIMEBASE_FREQ 512000000ULL > > -static inline bool pnv_chip_is_power10(const PnvChip *chip) > -{ > - return PNV_CHIP_GET_CLASS(chip)->chip_type == PNV_CHIP_POWER10; > -} > - > /* > * BMC helpers > */ >
On 13/12/2019 13:00, Greg Kurz wrote: > The XSCOM bus is implemented with a QOM interface, which is mostly > generic from a CPU type standpoint, except for the computation of > addresses on the Pervasize Connect Bus (PCB) network. This is handled Pervasive > by the pnv_xscom_pcba() function with a switch statement based on > the chip_type class level attribute of the CPU chip. > > This can be achieved using QOM. Also the address argument is masked with > PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different > sizes with other CPU types. Have each CPU chip type handle the appropriate > computation with a QOM xscom_pcba() method. PnvXscom model ? :) > Signed-off-by: Greg Kurz <groug@kaod.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv.c | 23 +++++++++++++++++++++++ > hw/ppc/pnv_xscom.c | 14 +------------- > include/hw/ppc/pnv.h | 1 + > 3 files changed, 25 insertions(+), 13 deletions(-) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 0447b534b8c5..cc40b90e9cd2 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp) > &chip8->homer.regs); > } > > +static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr) > +{ > + addr &= (PNV_XSCOM_SIZE - 1); > + return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); > +} > + > static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > dc->desc = "PowerNV Chip POWER8E"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > dc->desc = "PowerNV Chip POWER8"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > k->dt_populate = pnv_chip_power8_dt_populate; > k->pic_print_info = pnv_chip_power8_pic_print_info; > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > dc->desc = "PowerNV Chip POWER8NVL"; > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > @@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp) > &chip9->homer.regs); > } > > +static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr) > +{ > + addr &= (PNV9_XSCOM_SIZE - 1); > + return addr >> 3; > +} > + > static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > k->dt_populate = pnv_chip_power9_dt_populate; > k->pic_print_info = pnv_chip_power9_pic_print_info; > k->xscom_core_base = pnv_chip_power9_xscom_core_base; > + k->xscom_pcba = pnv_chip_power9_xscom_pcba; > dc->desc = "PowerNV Chip POWER9"; > > device_class_set_parent_realize(dc, pnv_chip_power9_realize, > @@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp) > (uint64_t) PNV10_LPCM_BASE(chip)); > } > > +static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr) > +{ > + addr &= (PNV10_XSCOM_SIZE - 1); > + return addr >> 3; > +} > + > static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > k->dt_populate = pnv_chip_power10_dt_populate; > k->pic_print_info = pnv_chip_power10_pic_print_info; > k->xscom_core_base = pnv_chip_power10_xscom_core_base; > + k->xscom_pcba = pnv_chip_power10_xscom_pcba; > dc->desc = "PowerNV Chip POWER10"; > > device_class_set_parent_realize(dc, pnv_chip_power10_realize, > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c > index 5ae9dfbb88ad..b681c72575b2 100644 > --- a/hw/ppc/pnv_xscom.c > +++ b/hw/ppc/pnv_xscom.c > @@ -57,19 +57,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); > - > - switch (PNV_CHIP_GET_CLASS(chip)->chip_type) { > - case PNV_CHIP_POWER8E: > - case PNV_CHIP_POWER8: > - case PNV_CHIP_POWER8NVL: > - return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); > - case PNV_CHIP_POWER9: > - case PNV_CHIP_POWER10: > - return addr >> 3; > - default: > - g_assert_not_reached(); > - } > + return PNV_CHIP_GET_CLASS(chip)->xscom_pcba(chip, addr); > } > > static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba) > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index 7a134a15d3b5..4972e93c2619 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -138,6 +138,7 @@ typedef struct PnvChipClass { > void (*dt_populate)(PnvChip *chip, void *fdt); > void (*pic_print_info)(PnvChip *chip, Monitor *mon); > uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id); > + uint32_t (*xscom_pcba)(PnvChip *chip, uint64_t addr); > } PnvChipClass; > > #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP >
On Fri, Dec 13, 2019 at 02:00:53PM +0100, Cédric Le Goater wrote: > On 13/12/2019 13:00, Greg Kurz wrote: > > The pnv_pic_print_info() callback checks the type of the chip in order > > to forward to the request appropriate interrupt controller. This can > > be achieved with QOM. Introduce a method for this in the base chip class > > and implement it in child classes. > > > > This also prepares ground for the upcoming interrupt controller of POWER10 > > chips. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > One comment below. > > > --- > > hw/ppc/pnv.c | 30 +++++++++++++++++++++++++----- > > include/hw/ppc/pnv.h | 1 + > > 2 files changed, 26 insertions(+), 5 deletions(-) > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > index efc00f4cb67a..2a53e99bda2e 100644 > > --- a/hw/ppc/pnv.c > > +++ b/hw/ppc/pnv.c > > @@ -832,6 +832,12 @@ static void pnv_chip_power8_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > > pnv_cpu->intc = NULL; > > } > > > > +static void pnv_chip_power8_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > > + Monitor *mon) > > +{ > > + icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon); > > +} > > + > > /* > > * 0:48 Reserved - Read as zeroes > > * 49:52 Node ID > > @@ -889,6 +895,12 @@ static void pnv_chip_power9_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > > pnv_cpu->intc = NULL; > > } > > > > +static void pnv_chip_power9_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > > + Monitor *mon) > > +{ > > + xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > > +} > > + > > static void pnv_chip_power10_intc_create(PnvChip *chip, PowerPCCPU *cpu, > > Error **errp) > > { > > @@ -910,6 +922,11 @@ static void pnv_chip_power10_intc_destroy(PnvChip *chip, PowerPCCPU *cpu) > > pnv_cpu->intc = NULL; > > } > > > > +static void pnv_chip_power10_intc_print_info(PnvChip *chip, PowerPCCPU *cpu, > > + Monitor *mon) > > +{ > > +} > > + > > /* > > * Allowed core identifiers on a POWER8 Processor Chip : > > * > > @@ -1086,6 +1103,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > > k->intc_create = pnv_chip_power8_intc_create; > > k->intc_reset = pnv_chip_power8_intc_reset; > > k->intc_destroy = pnv_chip_power8_intc_destroy; > > + k->intc_print_info = pnv_chip_power8_intc_print_info; > > k->isa_create = pnv_chip_power8_isa_create; > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > @@ -1107,6 +1125,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > > k->intc_create = pnv_chip_power8_intc_create; > > k->intc_reset = pnv_chip_power8_intc_reset; > > k->intc_destroy = pnv_chip_power8_intc_destroy; > > + k->intc_print_info = pnv_chip_power8_intc_print_info; > > k->isa_create = pnv_chip_power8_isa_create; > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > @@ -1128,6 +1147,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > > k->intc_create = pnv_chip_power8_intc_create; > > k->intc_reset = pnv_chip_power8_intc_reset; > > k->intc_destroy = pnv_chip_power8_intc_destroy; > > + k->intc_print_info = pnv_chip_power8_intc_print_info; > > k->isa_create = pnv_chip_power8nvl_isa_create; > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > @@ -1299,6 +1319,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > > k->intc_create = pnv_chip_power9_intc_create; > > k->intc_reset = pnv_chip_power9_intc_reset; > > k->intc_destroy = pnv_chip_power9_intc_destroy; > > + k->intc_print_info = pnv_chip_power9_intc_print_info; > > k->isa_create = pnv_chip_power9_isa_create; > > k->dt_populate = pnv_chip_power9_dt_populate; > > k->pic_print_info = pnv_chip_power9_pic_print_info; > > @@ -1379,6 +1400,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > > k->intc_create = pnv_chip_power10_intc_create; > > k->intc_reset = pnv_chip_power10_intc_reset; > > k->intc_destroy = pnv_chip_power10_intc_destroy; > > + k->intc_print_info = pnv_chip_power10_intc_print_info; > > k->isa_create = pnv_chip_power10_isa_create; > > k->dt_populate = pnv_chip_power10_dt_populate; > > k->pic_print_info = pnv_chip_power10_pic_print_info; > > @@ -1575,11 +1597,9 @@ static void pnv_pic_print_info(InterruptStatsProvider *obj, > > CPU_FOREACH(cs) { > > PowerPCCPU *cpu = POWERPC_CPU(cs); > > > > - if (pnv_chip_is_power9(pnv->chips[0])) { > > - xive_tctx_pic_print_info(XIVE_TCTX(pnv_cpu_state(cpu)->intc), mon); > > - } else { > > - icp_pic_print_info(ICP(pnv_cpu_state(cpu)->intc), mon); > > - } > > + /* XXX: loop on each chip/core/thread instead of CPU_FOREACH() */ > > May be we should introduce a helper such as : > > int pnv_chip_cpu_foreach(PnvChip *chip, > int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque) > { > int i, j; > int ret = 0; > > for (i = 0; i < chip->nr_cores; i++) { > PnvCore *pc = chip->cores[i]; > CPUCore *cc = CPU_CORE(pc); > > for (j = 0; j < cc->nr_threads; j++) { > PowerPCCPU *cpu = pc->threads[j]; > ret = doit(chip, cpu, opaque); > if (ret) { > break; > } > } > } > return ret; > } What I'd actually like to work towards is just having the interrupt controllers themselves advertize TYPE_INTERRUPT_STATS_PROVIDER and not needing anything specific at the machine level to locate them, just let the generic code in hmp_info_pic handle it. But this patch is fine in the meantime. > > > + PNV_CHIP_GET_CLASS(pnv->chips[0])->intc_print_info(pnv->chips[0], cpu, > > + mon); > > } > > > > for (i = 0; i < pnv->num_chips; i++) { > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > > index c213bdd5ecd3..7d2402784d4b 100644 > > --- a/include/hw/ppc/pnv.h > > +++ b/include/hw/ppc/pnv.h > > @@ -133,6 +133,7 @@ typedef struct PnvChipClass { > > void (*intc_create)(PnvChip *chip, PowerPCCPU *cpu, Error **errp); > > void (*intc_reset)(PnvChip *chip, PowerPCCPU *cpu); > > void (*intc_destroy)(PnvChip *chip, PowerPCCPU *cpu); > > + void (*intc_print_info)(PnvChip *chip, PowerPCCPU *cpu, Monitor *mon); > > ISABus *(*isa_create)(PnvChip *chip, Error **errp); > > void (*dt_populate)(PnvChip *chip, void *fdt); > > void (*pic_print_info)(PnvChip *chip, Monitor *mon); > > >
On Fri, Dec 13, 2019 at 02:06:31PM +0100, Cédric Le Goater wrote: > On 13/12/2019 13:00, Greg Kurz wrote: > > The XSCOM bus is implemented with a QOM interface, which is mostly > > generic from a CPU type standpoint, except for the computation of > > addresses on the Pervasize Connect Bus (PCB) network. This is handled > > Pervasive I've fixed this typo in transit. > > > by the pnv_xscom_pcba() function with a switch statement based on > > the chip_type class level attribute of the CPU chip. > > > > This can be achieved using QOM. Also the address argument is masked with > > PNV_XSCOM_SIZE - 1, which is for POWER8 only. Addresses may have different > > sizes with other CPU types. Have each CPU chip type handle the appropriate > > computation with a QOM xscom_pcba() method. > > PnvXscom model ? :) > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > > --- > > hw/ppc/pnv.c | 23 +++++++++++++++++++++++ > > hw/ppc/pnv_xscom.c | 14 +------------- > > include/hw/ppc/pnv.h | 1 + > > 3 files changed, 25 insertions(+), 13 deletions(-) > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > index 0447b534b8c5..cc40b90e9cd2 100644 > > --- a/hw/ppc/pnv.c > > +++ b/hw/ppc/pnv.c > > @@ -1121,6 +1121,12 @@ static void pnv_chip_power8_realize(DeviceState *dev, Error **errp) > > &chip8->homer.regs); > > } > > > > +static uint32_t pnv_chip_power8_xscom_pcba(PnvChip *chip, uint64_t addr) > > +{ > > + addr &= (PNV_XSCOM_SIZE - 1); > > + return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); > > +} > > + > > static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -1138,6 +1144,7 @@ static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data) > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > > dc->desc = "PowerNV Chip POWER8E"; > > > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > > @@ -1161,6 +1168,7 @@ static void pnv_chip_power8_class_init(ObjectClass *klass, void *data) > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > > dc->desc = "PowerNV Chip POWER8"; > > > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > > @@ -1184,6 +1192,7 @@ static void pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data) > > k->dt_populate = pnv_chip_power8_dt_populate; > > k->pic_print_info = pnv_chip_power8_pic_print_info; > > k->xscom_core_base = pnv_chip_power8_xscom_core_base; > > + k->xscom_pcba = pnv_chip_power8_xscom_pcba; > > dc->desc = "PowerNV Chip POWER8NVL"; > > > > device_class_set_parent_realize(dc, pnv_chip_power8_realize, > > @@ -1340,6 +1349,12 @@ static void pnv_chip_power9_realize(DeviceState *dev, Error **errp) > > &chip9->homer.regs); > > } > > > > +static uint32_t pnv_chip_power9_xscom_pcba(PnvChip *chip, uint64_t addr) > > +{ > > + addr &= (PNV9_XSCOM_SIZE - 1); > > + return addr >> 3; > > +} > > + > > static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -1357,6 +1372,7 @@ static void pnv_chip_power9_class_init(ObjectClass *klass, void *data) > > k->dt_populate = pnv_chip_power9_dt_populate; > > k->pic_print_info = pnv_chip_power9_pic_print_info; > > k->xscom_core_base = pnv_chip_power9_xscom_core_base; > > + k->xscom_pcba = pnv_chip_power9_xscom_pcba; > > dc->desc = "PowerNV Chip POWER9"; > > > > device_class_set_parent_realize(dc, pnv_chip_power9_realize, > > @@ -1422,6 +1438,12 @@ static void pnv_chip_power10_realize(DeviceState *dev, Error **errp) > > (uint64_t) PNV10_LPCM_BASE(chip)); > > } > > > > +static uint32_t pnv_chip_power10_xscom_pcba(PnvChip *chip, uint64_t addr) > > +{ > > + addr &= (PNV10_XSCOM_SIZE - 1); > > + return addr >> 3; > > +} > > + > > static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > @@ -1439,6 +1461,7 @@ static void pnv_chip_power10_class_init(ObjectClass *klass, void *data) > > k->dt_populate = pnv_chip_power10_dt_populate; > > k->pic_print_info = pnv_chip_power10_pic_print_info; > > k->xscom_core_base = pnv_chip_power10_xscom_core_base; > > + k->xscom_pcba = pnv_chip_power10_xscom_pcba; > > dc->desc = "PowerNV Chip POWER10"; > > > > device_class_set_parent_realize(dc, pnv_chip_power10_realize, > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c > > index 5ae9dfbb88ad..b681c72575b2 100644 > > --- a/hw/ppc/pnv_xscom.c > > +++ b/hw/ppc/pnv_xscom.c > > @@ -57,19 +57,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); > > - > > - switch (PNV_CHIP_GET_CLASS(chip)->chip_type) { > > - case PNV_CHIP_POWER8E: > > - case PNV_CHIP_POWER8: > > - case PNV_CHIP_POWER8NVL: > > - return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); > > - case PNV_CHIP_POWER9: > > - case PNV_CHIP_POWER10: > > - return addr >> 3; > > - default: > > - g_assert_not_reached(); > > - } > > + return PNV_CHIP_GET_CLASS(chip)->xscom_pcba(chip, addr); > > } > > > > static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba) > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > > index 7a134a15d3b5..4972e93c2619 100644 > > --- a/include/hw/ppc/pnv.h > > +++ b/include/hw/ppc/pnv.h > > @@ -138,6 +138,7 @@ typedef struct PnvChipClass { > > void (*dt_populate)(PnvChip *chip, void *fdt); > > void (*pic_print_info)(PnvChip *chip, Monitor *mon); > > uint64_t (*xscom_core_base)(PnvChip *chip, uint32_t core_id); > > + uint32_t (*xscom_pcba)(PnvChip *chip, uint64_t addr); > > } PnvChipClass; > > > > #define PNV_CHIP_TYPE_SUFFIX "-" TYPE_PNV_CHIP > > >
On Fri, Dec 13, 2019 at 12:59:28PM +0100, Greg Kurz wrote: > The PnvChipClass type has a chip_type attribute which identifies various > POWER CPU chip types that can be used in a powernv machine. > > typedef enum PnvChipType { > PNV_CHIP_POWER8E, /* AKA Murano (default) */ > PNV_CHIP_POWER8, /* AKA Venice */ > PNV_CHIP_POWER8NVL, /* AKA Naples */ > PNV_CHIP_POWER9, /* AKA Nimbus */ > PNV_CHIP_POWER10, /* AKA TBD */ > } PnvChipType; > > This attribute is used in many places where we want a different behaviour > depending on the CPU type, either directly like: > > switch (PNV_CHIP_GET_CLASS(chip)->chip_type) { > case PNV_CHIP_POWER8E: > case PNV_CHIP_POWER8: > case PNV_CHIP_POWER8NVL: > return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); > case PNV_CHIP_POWER9: > case PNV_CHIP_POWER10: > return addr >> 3; > default: > g_assert_not_reached(); > } > > or through various helpers that rely on it: > > /* Each core has an XSCOM MMIO region */ > if (pnv_chip_is_power10(chip)) { > xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid); > } else if (pnv_chip_is_power9(chip)) { > xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid); > } else { > xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid); > } > > The chip_type is also duplicated in the PnvPsiClass type. > > It looks a bit unfortunate to implement manually something that falls into > the scope of QOM. Especially since we don't seem to need a finer grain than > the CPU familly, ie. POWER8, POWER9, POWER10, ..., and we already have > specialized versions of PnvChipClass and PnvPsiClass for these. > > This series basically QOM-ifies all the places where we check on the chip > type, and gets rid of the chip_type attributes and the is_powerXX() helpers. > > Patch 1 was recently posted to the list but it isn't available in David's > ppc-for-5.0 tree yet, so I include it in this series for convenience. Applied to ppc-for-5.0. I had been anticipating just replacing a bunch of tests on ->chip_type with object_dynamic_cast() checks, but this is better still.
>> May be we should introduce a helper such as : >> >> int pnv_chip_cpu_foreach(PnvChip *chip, >> int (*doit)(PnvChip *chip, PowerPCCPU *cpu, void *opaque), void *opaque) >> { >> int i, j; >> int ret = 0; >> >> for (i = 0; i < chip->nr_cores; i++) { >> PnvCore *pc = chip->cores[i]; >> CPUCore *cc = CPU_CORE(pc); >> >> for (j = 0; j < cc->nr_threads; j++) { >> PowerPCCPU *cpu = pc->threads[j]; >> ret = doit(chip, cpu, opaque); >> if (ret) { >> break; >> } >> } >> } >> return ret; >> } > > What I'd actually like to work towards is just having the interrupt > controllers themselves advertize TYPE_INTERRUPT_STATS_PROVIDER and not > needing anything specific at the machine level to locate them, just > let the generic code in hmp_info_pic handle it. OK. It would good to at least loop on the chips, so that the output of the possible TYPE_INTERRUPT_STATS_PROVIDER (IC, PSIHB, PHB, NPU) are ordered. C.
On Fri, 13 Dec 2019 13:44:48 +0100 Cédric Le Goater <clg@kaod.org> wrote: > On 13/12/2019 12:59, Greg Kurz wrote: > > The pnv_dt_create() function generates different contents for the > > "compatible" property of the root node in the DT, depending on the > > CPU type. This is open coded with multiple ifs using pnv_is_powerXX() > > helpers. > > > > It seems cleaner to achieve with QOM. Introduce a base class for the > > powernv machine and a compat attribute that each child class can use > > to provide the value for the "compatible" property. > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > > > --- > > hw/ppc/pnv.c | 33 +++++++++++++++++++-------------- > > include/hw/ppc/pnv.h | 13 +++++++++++++ > > 2 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > index 0be0b6b411c3..5ac149b149d8 100644 > > --- a/hw/ppc/pnv.c > > +++ b/hw/ppc/pnv.c > > @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt) > > > > static void *pnv_dt_create(MachineState *machine) > > { > > - const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > - const char plat_compat9[] = "qemu,powernv9\0ibm,powernv"; > > - const char plat_compat10[] = "qemu,powernv10\0ibm,powernv"; > > + PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine); > > PnvMachineState *pnv = PNV_MACHINE(machine); > > void *fdt; > > char *buf; > > @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine) > > _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2))); > > _FDT((fdt_setprop_string(fdt, 0, "model", > > "IBM PowerNV (emulated by qemu)"))); > > - if (pnv_is_power10(pnv)) { > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10, > > - sizeof(plat_compat10)))); > > - } else if (pnv_is_power9(pnv)) { > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9, > > - sizeof(plat_compat9)))); > > - } else { > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8, > > - sizeof(plat_compat8)))); > > - } > > - > > + _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat, > > + sizeof(pmc->compat)))); Of course the size should be pmc->compat_size ... David, can you fix this in your tree or should I post a v2 ? > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf))); > > @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc = MACHINE_CLASS(oc); > > XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > + static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); > > @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > > xic->icp_get = pnv_icp_get; > > xic->ics_get = pnv_ics_get; > > xic->ics_resend = pnv_ics_resend; > > + > > + pmc->compat = compat; > > + pmc->compat_size = sizeof(compat); > > } > > > > static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc = MACHINE_CLASS(oc); > > XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc); > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > + static const char compat[] = "qemu,powernv9\0ibm,powernv"; > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER9"; > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0"); > > xfc->match_nvt = pnv_match_nvt; > > > > mc->alias = "powernv"; > > + > > + pmc->compat = compat; > > + pmc->compat_size = sizeof(compat); > > } > > > > static void pnv_machine_power10_class_init(ObjectClass *oc, void *data) > > { > > MachineClass *mc = MACHINE_CLASS(oc); > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > + static const char compat[] = "qemu,powernv10\0ibm,powernv"; > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER10"; > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0"); > > + > > + pmc->compat = compat; > > + pmc->compat_size = sizeof(compat); > > } > > > > static void pnv_machine_class_init(ObjectClass *oc, void *data) > > @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = { > > .instance_size = sizeof(PnvMachineState), > > .instance_init = pnv_machine_instance_init, > > .class_init = pnv_machine_class_init, > > + .class_size = sizeof(PnvMachineClass), > > .interfaces = (InterfaceInfo[]) { > > { TYPE_INTERRUPT_STATS_PROVIDER }, > > { }, > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > > index 92f80b1ccead..d534746bd493 100644 > > --- a/include/hw/ppc/pnv.h > > +++ b/include/hw/ppc/pnv.h > > @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir); > > #define TYPE_PNV_MACHINE MACHINE_TYPE_NAME("powernv") > > #define PNV_MACHINE(obj) \ > > OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE) > > +#define PNV_MACHINE_GET_CLASS(obj) \ > > + OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE) > > +#define PNV_MACHINE_CLASS(klass) \ > > + OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE) > > + > > +typedef struct PnvMachineClass { > > + /*< private >*/ > > + MachineClass parent_class; > > + > > + /*< public >*/ > > + const char *compat; > > + int compat_size; > > +} PnvMachineClass; > > > > typedef struct PnvMachineState { > > /*< private >*/ > > > >
On Mon, Dec 16, 2019 at 07:07:43PM +0100, Greg Kurz wrote: > On Fri, 13 Dec 2019 13:44:48 +0100 > Cédric Le Goater <clg@kaod.org> wrote: > > > On 13/12/2019 12:59, Greg Kurz wrote: > > > The pnv_dt_create() function generates different contents for the > > > "compatible" property of the root node in the DT, depending on the > > > CPU type. This is open coded with multiple ifs using pnv_is_powerXX() > > > helpers. > > > > > > It seems cleaner to achieve with QOM. Introduce a base class for the > > > powernv machine and a compat attribute that each child class can use > > > to provide the value for the "compatible" property. > > > > > > Signed-off-by: Greg Kurz <groug@kaod.org> > > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > > > > > > --- > > > hw/ppc/pnv.c | 33 +++++++++++++++++++-------------- > > > include/hw/ppc/pnv.h | 13 +++++++++++++ > > > 2 files changed, 32 insertions(+), 14 deletions(-) > > > > > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > > > index 0be0b6b411c3..5ac149b149d8 100644 > > > --- a/hw/ppc/pnv.c > > > +++ b/hw/ppc/pnv.c > > > @@ -484,9 +484,7 @@ static void pnv_dt_power_mgt(void *fdt) > > > > > > static void *pnv_dt_create(MachineState *machine) > > > { > > > - const char plat_compat8[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > > - const char plat_compat9[] = "qemu,powernv9\0ibm,powernv"; > > > - const char plat_compat10[] = "qemu,powernv10\0ibm,powernv"; > > > + PnvMachineClass *pmc = PNV_MACHINE_GET_CLASS(machine); > > > PnvMachineState *pnv = PNV_MACHINE(machine); > > > void *fdt; > > > char *buf; > > > @@ -504,17 +502,8 @@ static void *pnv_dt_create(MachineState *machine) > > > _FDT((fdt_setprop_cell(fdt, 0, "#size-cells", 0x2))); > > > _FDT((fdt_setprop_string(fdt, 0, "model", > > > "IBM PowerNV (emulated by qemu)"))); > > > - if (pnv_is_power10(pnv)) { > > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat10, > > > - sizeof(plat_compat10)))); > > > - } else if (pnv_is_power9(pnv)) { > > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat9, > > > - sizeof(plat_compat9)))); > > > - } else { > > > - _FDT((fdt_setprop(fdt, 0, "compatible", plat_compat8, > > > - sizeof(plat_compat8)))); > > > - } > > > - > > > + _FDT((fdt_setprop(fdt, 0, "compatible", pmc->compat, > > > + sizeof(pmc->compat)))); > > Of course the size should be pmc->compat_size ... David, can you fix this > in your tree or should I post a v2 ? Ah, yes. This message came just barely in time - I've folded in the fix as I've been setting up to to a pull request including it. > > > > > > > buf = qemu_uuid_unparse_strdup(&qemu_uuid); > > > _FDT((fdt_setprop_string(fdt, 0, "vm,uuid", buf))); > > > @@ -1692,6 +1681,8 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > > > { > > > MachineClass *mc = MACHINE_CLASS(oc); > > > XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); > > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > > + static const char compat[] = "qemu,powernv8\0qemu,powernv\0ibm,powernv"; > > > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER8"; > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0"); > > > @@ -1699,26 +1690,39 @@ static void pnv_machine_power8_class_init(ObjectClass *oc, void *data) > > > xic->icp_get = pnv_icp_get; > > > xic->ics_get = pnv_ics_get; > > > xic->ics_resend = pnv_ics_resend; > > > + > > > + pmc->compat = compat; > > > + pmc->compat_size = sizeof(compat); > > > } > > > > > > static void pnv_machine_power9_class_init(ObjectClass *oc, void *data) > > > { > > > MachineClass *mc = MACHINE_CLASS(oc); > > > XiveFabricClass *xfc = XIVE_FABRIC_CLASS(oc); > > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > > + static const char compat[] = "qemu,powernv9\0ibm,powernv"; > > > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER9"; > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power9_v2.0"); > > > xfc->match_nvt = pnv_match_nvt; > > > > > > mc->alias = "powernv"; > > > + > > > + pmc->compat = compat; > > > + pmc->compat_size = sizeof(compat); > > > } > > > > > > static void pnv_machine_power10_class_init(ObjectClass *oc, void *data) > > > { > > > MachineClass *mc = MACHINE_CLASS(oc); > > > + PnvMachineClass *pmc = PNV_MACHINE_CLASS(oc); > > > + static const char compat[] = "qemu,powernv10\0ibm,powernv"; > > > > > > mc->desc = "IBM PowerNV (Non-Virtualized) POWER10"; > > > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power10_v1.0"); > > > + > > > + pmc->compat = compat; > > > + pmc->compat_size = sizeof(compat); > > > } > > > > > > static void pnv_machine_class_init(ObjectClass *oc, void *data) > > > @@ -1796,6 +1800,7 @@ static const TypeInfo types[] = { > > > .instance_size = sizeof(PnvMachineState), > > > .instance_init = pnv_machine_instance_init, > > > .class_init = pnv_machine_class_init, > > > + .class_size = sizeof(PnvMachineClass), > > > .interfaces = (InterfaceInfo[]) { > > > { TYPE_INTERRUPT_STATS_PROVIDER }, > > > { }, > > > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > > > index 92f80b1ccead..d534746bd493 100644 > > > --- a/include/hw/ppc/pnv.h > > > +++ b/include/hw/ppc/pnv.h > > > @@ -185,6 +185,19 @@ PowerPCCPU *pnv_chip_find_cpu(PnvChip *chip, uint32_t pir); > > > #define TYPE_PNV_MACHINE MACHINE_TYPE_NAME("powernv") > > > #define PNV_MACHINE(obj) \ > > > OBJECT_CHECK(PnvMachineState, (obj), TYPE_PNV_MACHINE) > > > +#define PNV_MACHINE_GET_CLASS(obj) \ > > > + OBJECT_GET_CLASS(PnvMachineClass, obj, TYPE_PNV_MACHINE) > > > +#define PNV_MACHINE_CLASS(klass) \ > > > + OBJECT_CLASS_CHECK(PnvMachineClass, klass, TYPE_PNV_MACHINE) > > > + > > > +typedef struct PnvMachineClass { > > > + /*< private >*/ > > > + MachineClass parent_class; > > > + > > > + /*< public >*/ > > > + const char *compat; > > > + int compat_size; > > > +} PnvMachineClass; > > > > > > typedef struct PnvMachineState { > > > /*< private >*/ > > > > > > > >
The PnvChipClass type has a chip_type attribute which identifies various POWER CPU chip types that can be used in a powernv machine. typedef enum PnvChipType { PNV_CHIP_POWER8E, /* AKA Murano (default) */ PNV_CHIP_POWER8, /* AKA Venice */ PNV_CHIP_POWER8NVL, /* AKA Naples */ PNV_CHIP_POWER9, /* AKA Nimbus */ PNV_CHIP_POWER10, /* AKA TBD */ } PnvChipType; This attribute is used in many places where we want a different behaviour depending on the CPU type, either directly like: switch (PNV_CHIP_GET_CLASS(chip)->chip_type) { case PNV_CHIP_POWER8E: case PNV_CHIP_POWER8: case PNV_CHIP_POWER8NVL: return ((addr >> 4) & ~0xfull) | ((addr >> 3) & 0xf); case PNV_CHIP_POWER9: case PNV_CHIP_POWER10: return addr >> 3; default: g_assert_not_reached(); } or through various helpers that rely on it: /* Each core has an XSCOM MMIO region */ if (pnv_chip_is_power10(chip)) { xscom_core_base = PNV10_XSCOM_EC_BASE(core_hwid); } else if (pnv_chip_is_power9(chip)) { xscom_core_base = PNV9_XSCOM_EC_BASE(core_hwid); } else { xscom_core_base = PNV_XSCOM_EX_BASE(core_hwid); } The chip_type is also duplicated in the PnvPsiClass type. It looks a bit unfortunate to implement manually something that falls into the scope of QOM. Especially since we don't seem to need a finer grain than the CPU familly, ie. POWER8, POWER9, POWER10, ..., and we already have specialized versions of PnvChipClass and PnvPsiClass for these. This series basically QOM-ifies all the places where we check on the chip type, and gets rid of the chip_type attributes and the is_powerXX() helpers. Patch 1 was recently posted to the list but it isn't available in David's ppc-for-5.0 tree yet, so I include it in this series for convenience. -- Greg --- Greg Kurz (13): ppc: Drop useless extern annotation for functions ppc/pnv: Introduce PnvPsiClass::compat ppc/pnv: Drop PnvPsiClass::chip_type ppc/pnv: Introduce PnvMachineClass and PnvMachineClass::compat ppc/pnv: Introduce PnvMachineClass::dt_power_mgt() ppc/pnv: Drop pnv_is_power9() and pnv_is_power10() helpers ppc/pnv: Introduce PnvChipClass::intc_print_info() method ppc/pnv: Introduce PnvChipClass::xscom_core_base() method ppc/pnv: Pass XSCOM base address and address size to pnv_dt_xscom() ppc/pnv: Pass content of the "compatible" property to pnv_dt_xscom() ppc/pnv: Drop pnv_chip_is_power9() and pnv_chip_is_power10() helpers ppc/pnv: Introduce PnvChipClass::xscom_pcba() method ppc/pnv: Drop PnvChipClass::type hw/ppc/pnv.c | 150 +++++++++++++++++++++++++++++++++----------- hw/ppc/pnv_psi.c | 28 +++----- hw/ppc/pnv_xscom.c | 48 ++------------ include/hw/ppc/pnv.h | 53 ++++++---------- include/hw/ppc/pnv_psi.h | 3 + include/hw/ppc/pnv_xscom.h | 24 ++++--- include/hw/ppc/spapr_vio.h | 6 +- 7 files changed, 169 insertions(+), 143 deletions(-)