| Message ID | 1488970371-8865-4-git-send-email-clg@kaod.org |
|---|---|
| State | New |
| Headers | show |
On Wed, Mar 08, 2017 at 11:52:46AM +0100, Cédric Le Goater wrote: > Like this is done for the sPAPR machine, we use a simple array under > the PowerNV machine to store the Interrupt Control Presenters (ICP) > objects, one for each vCPU. This array is indexed by 'cpu_index' of > the CPUState. > > We use a list to hold the different Interrupt Control Sources (ICS) > objects, as PowerNV needs to handle multiple sources: for PCI-E and > for the Processor Service Interface (PSI). > > Finally, to interface with the XICS layer which manipulates the ICP > and ICS objects, we extend the PowerNV machine with an XICSFabric > interface and its associated handlers. > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > hw/ppc/pnv.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++ > include/hw/ppc/pnv.h | 4 +++ > include/hw/ppc/xics.h | 1 + > 3 files changed, 94 insertions(+) > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 09f0d22defb8..461d3535e99c 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -32,6 +32,8 @@ > #include "exec/address-spaces.h" > #include "qemu/cutils.h" > #include "qapi/visitor.h" > +#include "monitor/monitor.h" > +#include "hw/intc/intc.h" > > #include "hw/ppc/pnv_xscom.h" > > @@ -416,6 +418,23 @@ static void ppc_powernv_init(MachineState *machine) > machine->cpu_model = "POWER8"; > } > > + /* Create the Interrupt Control Presenters before the vCPUs */ > + pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads; > + pnv->icps = g_new0(ICPState, pnv->nr_servers); > + for (i = 0; i < pnv->nr_servers; i++) { > + ICPState *icp = &pnv->icps[i]; > + object_initialize(icp, sizeof(*icp), TYPE_ICP); > + qdev_set_parent_bus(DEVICE(icp), sysbus_get_default()); Your fixup for the PAPR case suggests this is not the right approach to the device initialization. > + object_property_add_child(OBJECT(pnv), "icp[*]", OBJECT(icp), > + &error_fatal); This isn't an urgent problem, but I dislike using the icp[*] behaviour as a rule (it's fixed now, but it just to be you could easily get O(n^3) behaviour using it). In this case you already have a handle on a specific id for the object. As well as being a bit less expensive, using an explicit index means that the exposed QOM address will definitely match an otherwise meaningful id, rather than just lining up by accident if all the orders remain the same. > + object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv), > + &error_fatal); > + object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal); > + } > + > + /* and the list of Interrupt Control Sources */ > + QLIST_INIT(&pnv->ics); > + > /* Create the processor chips */ > chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); > if (!object_class_by_name(chip_typename)) { > @@ -742,6 +761,48 @@ static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, > visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp); > } > > +static ICSState *pnv_ics_get(XICSFabric *xi, int irq) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(xi); > + ICSState *ics; > + > + QLIST_FOREACH(ics, &pnv->ics, list) { > + if (ics_valid_irq(ics, irq)) { > + return ics; > + } > + } > + return NULL; > +} > + > +static void pnv_ics_resend(XICSFabric *xi) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(xi); > + ICSState *ics; > + > + QLIST_FOREACH(ics, &pnv->ics, list) { > + ics_resend(ics); > + } > +} > + > +static void pnv_ics_eoi(XICSFabric *xi, int irq) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(xi); > + ICSState *ics; > + > + QLIST_FOREACH(ics, &pnv->ics, list) { > + if (ics_valid_irq(ics, irq)) { Unless something is badly wrong, this should only return true for exactly one iteration through the loop. Which makes this equivalent to ics_get() followed by ics_eoi(). > + ics_eoi(ics, irq); > + } > + } > +} > + > +static ICPState *pnv_icp_get(XICSFabric *xi, int server) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(xi); > + > + return (server < pnv->nr_servers) ? &pnv->icps[server] : NULL; > +} > + > static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > @@ -783,9 +844,27 @@ static void powernv_machine_class_props_init(ObjectClass *oc) > NULL); > } > > +static void pnv_pic_print_info(InterruptStatsProvider *obj, > + Monitor *mon) > +{ > + PnvMachineState *pnv = POWERNV_MACHINE(obj); > + ICSState *ics; > + int i; > + > + for (i = 0; i < pnv->nr_servers; i++) { > + icp_pic_print_info(&pnv->icps[i], mon); > + } > + > + QLIST_FOREACH(ics, &pnv->ics, list) { > + ics_pic_print_info(ics, mon); > + } > +} > + > static void powernv_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > + XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); > + InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc); > > mc->desc = "IBM PowerNV (Non-Virtualized)"; > mc->init = ppc_powernv_init; > @@ -796,6 +875,11 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data) > mc->no_parallel = 1; > mc->default_boot_order = NULL; > mc->default_ram_size = 1 * G_BYTE; > + xic->icp_get = pnv_icp_get; > + xic->ics_get = pnv_ics_get; > + xic->ics_eoi = pnv_ics_eoi; > + xic->ics_resend = pnv_ics_resend; > + ispc->print_info = pnv_pic_print_info; > > powernv_machine_class_props_init(oc); > } > @@ -806,6 +890,11 @@ static const TypeInfo powernv_machine_info = { > .instance_size = sizeof(PnvMachineState), > .instance_init = powernv_machine_initfn, > .class_init = powernv_machine_class_init, > + .interfaces = (InterfaceInfo[]) { > + { TYPE_XICS_FABRIC }, > + { TYPE_INTERRUPT_STATS_PROVIDER }, > + { }, > + }, > }; > > static void powernv_machine_register_types(void) > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h > index df98a72006e4..6a0b004cea93 100644 > --- a/include/hw/ppc/pnv.h > +++ b/include/hw/ppc/pnv.h > @@ -22,6 +22,7 @@ > #include "hw/boards.h" > #include "hw/sysbus.h" > #include "hw/ppc/pnv_lpc.h" > +#include "hw/ppc/xics.h" > > #define TYPE_PNV_CHIP "powernv-chip" > #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP) > @@ -114,6 +115,9 @@ typedef struct PnvMachineState { > PnvChip **chips; > > ISABus *isa_bus; > + ICPState *icps; > + uint32_t nr_servers; > + QLIST_HEAD(, ICSState) ics; > } PnvMachineState; > > #define PNV_FDT_ADDR 0x01000000 > diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h > index 00b003b2392d..c2032cac55f6 100644 > --- a/include/hw/ppc/xics.h > +++ b/include/hw/ppc/xics.h > @@ -115,6 +115,7 @@ struct ICSState { > qemu_irq *qirqs; > ICSIRQState *irqs; > XICSFabric *xics; > + QLIST_ENTRY(ICSState) list; > }; > > static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
On 03/14/2017 06:45 AM, David Gibson wrote: > On Wed, Mar 08, 2017 at 11:52:46AM +0100, Cédric Le Goater wrote: >> Like this is done for the sPAPR machine, we use a simple array under >> the PowerNV machine to store the Interrupt Control Presenters (ICP) >> objects, one for each vCPU. This array is indexed by 'cpu_index' of >> the CPUState. >> >> We use a list to hold the different Interrupt Control Sources (ICS) >> objects, as PowerNV needs to handle multiple sources: for PCI-E and >> for the Processor Service Interface (PSI). >> >> Finally, to interface with the XICS layer which manipulates the ICP >> and ICS objects, we extend the PowerNV machine with an XICSFabric >> interface and its associated handlers. >> >> Signed-off-by: Cédric Le Goater <clg@kaod.org> >> --- >> hw/ppc/pnv.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> include/hw/ppc/pnv.h | 4 +++ >> include/hw/ppc/xics.h | 1 + >> 3 files changed, 94 insertions(+) >> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c >> index 09f0d22defb8..461d3535e99c 100644 >> --- a/hw/ppc/pnv.c >> +++ b/hw/ppc/pnv.c >> @@ -32,6 +32,8 @@ >> #include "exec/address-spaces.h" >> #include "qemu/cutils.h" >> #include "qapi/visitor.h" >> +#include "monitor/monitor.h" >> +#include "hw/intc/intc.h" >> >> #include "hw/ppc/pnv_xscom.h" >> >> @@ -416,6 +418,23 @@ static void ppc_powernv_init(MachineState *machine) >> machine->cpu_model = "POWER8"; >> } >> >> + /* Create the Interrupt Control Presenters before the vCPUs */ >> + pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads; >> + pnv->icps = g_new0(ICPState, pnv->nr_servers); >> + for (i = 0; i < pnv->nr_servers; i++) { >> + ICPState *icp = &pnv->icps[i]; >> + object_initialize(icp, sizeof(*icp), TYPE_ICP); >> + qdev_set_parent_bus(DEVICE(icp), sysbus_get_default()); > > Your fixup for the PAPR case suggests this is not the right approach > to the device initialization. yes. unless we introduce a new ICP type object with MMIOs. >> + object_property_add_child(OBJECT(pnv), "icp[*]", OBJECT(icp), >> + &error_fatal); > > This isn't an urgent problem, but I dislike using the icp[*] > behaviour as a rule (it's fixed now, but it just to be you could > easily get O(n^3) behaviour using it). In this case you already have > a handle on a specific id for the object. > > As well as being a bit less expensive, using an explicit index means > that the exposed QOM address will definitely match an otherwise > meaningful id, rather than just lining up by accident if all the > orders remain the same. I agree. The ICP should be indexed with the HW core ids and these are not necessarily contiguous. I will change that. >> + object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv), >> + &error_fatal); >> + object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal); >> + } >> + >> + /* and the list of Interrupt Control Sources */ >> + QLIST_INIT(&pnv->ics); >> + >> /* Create the processor chips */ >> chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); >> if (!object_class_by_name(chip_typename)) { >> @@ -742,6 +761,48 @@ static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, >> visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp); >> } >> >> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(xi); >> + ICSState *ics; >> + >> + QLIST_FOREACH(ics, &pnv->ics, list) { >> + if (ics_valid_irq(ics, irq)) { >> + return ics; >> + } >> + } >> + return NULL; >> +} >> + >> +static void pnv_ics_resend(XICSFabric *xi) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(xi); >> + ICSState *ics; >> + >> + QLIST_FOREACH(ics, &pnv->ics, list) { >> + ics_resend(ics); >> + } >> +} >> + >> +static void pnv_ics_eoi(XICSFabric *xi, int irq) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(xi); >> + ICSState *ics; >> + >> + QLIST_FOREACH(ics, &pnv->ics, list) { >> + if (ics_valid_irq(ics, irq)) { > > Unless something is badly wrong, this should only return true for > exactly one iteration through the loop. Which makes this equivalent > to ics_get() followed by ics_eoi(). yes. Thanks, C. >> + ics_eoi(ics, irq); >> + } >> + } >> +} >> + >> +static ICPState *pnv_icp_get(XICSFabric *xi, int server) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(xi); >> + >> + return (server < pnv->nr_servers) ? &pnv->icps[server] : NULL; >> +} >> + >> static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name, >> void *opaque, Error **errp) >> { >> @@ -783,9 +844,27 @@ static void powernv_machine_class_props_init(ObjectClass *oc) >> NULL); >> } >> >> +static void pnv_pic_print_info(InterruptStatsProvider *obj, >> + Monitor *mon) >> +{ >> + PnvMachineState *pnv = POWERNV_MACHINE(obj); >> + ICSState *ics; >> + int i; >> + >> + for (i = 0; i < pnv->nr_servers; i++) { >> + icp_pic_print_info(&pnv->icps[i], mon); >> + } >> + >> + QLIST_FOREACH(ics, &pnv->ics, list) { >> + ics_pic_print_info(ics, mon); >> + } >> +} >> + >> static void powernv_machine_class_init(ObjectClass *oc, void *data) >> { >> MachineClass *mc = MACHINE_CLASS(oc); >> + XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); >> + InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc); >> >> mc->desc = "IBM PowerNV (Non-Virtualized)"; >> mc->init = ppc_powernv_init; >> @@ -796,6 +875,11 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data) >> mc->no_parallel = 1; >> mc->default_boot_order = NULL; >> mc->default_ram_size = 1 * G_BYTE; >> + xic->icp_get = pnv_icp_get; >> + xic->ics_get = pnv_ics_get; >> + xic->ics_eoi = pnv_ics_eoi; >> + xic->ics_resend = pnv_ics_resend; >> + ispc->print_info = pnv_pic_print_info; >> >> powernv_machine_class_props_init(oc); >> } >> @@ -806,6 +890,11 @@ static const TypeInfo powernv_machine_info = { >> .instance_size = sizeof(PnvMachineState), >> .instance_init = powernv_machine_initfn, >> .class_init = powernv_machine_class_init, >> + .interfaces = (InterfaceInfo[]) { >> + { TYPE_XICS_FABRIC }, >> + { TYPE_INTERRUPT_STATS_PROVIDER }, >> + { }, >> + }, >> }; >> >> static void powernv_machine_register_types(void) >> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h >> index df98a72006e4..6a0b004cea93 100644 >> --- a/include/hw/ppc/pnv.h >> +++ b/include/hw/ppc/pnv.h >> @@ -22,6 +22,7 @@ >> #include "hw/boards.h" >> #include "hw/sysbus.h" >> #include "hw/ppc/pnv_lpc.h" >> +#include "hw/ppc/xics.h" >> >> #define TYPE_PNV_CHIP "powernv-chip" >> #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP) >> @@ -114,6 +115,9 @@ typedef struct PnvMachineState { >> PnvChip **chips; >> >> ISABus *isa_bus; >> + ICPState *icps; >> + uint32_t nr_servers; >> + QLIST_HEAD(, ICSState) ics; >> } PnvMachineState; >> >> #define PNV_FDT_ADDR 0x01000000 >> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >> index 00b003b2392d..c2032cac55f6 100644 >> --- a/include/hw/ppc/xics.h >> +++ b/include/hw/ppc/xics.h >> @@ -115,6 +115,7 @@ struct ICSState { >> qemu_irq *qirqs; >> ICSIRQState *irqs; >> XICSFabric *xics; >> + QLIST_ENTRY(ICSState) list; >> }; >> >> static inline bool ics_valid_irq(ICSState *ics, uint32_t nr) >
diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 09f0d22defb8..461d3535e99c 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -32,6 +32,8 @@ #include "exec/address-spaces.h" #include "qemu/cutils.h" #include "qapi/visitor.h" +#include "monitor/monitor.h" +#include "hw/intc/intc.h" #include "hw/ppc/pnv_xscom.h" @@ -416,6 +418,23 @@ static void ppc_powernv_init(MachineState *machine) machine->cpu_model = "POWER8"; } + /* Create the Interrupt Control Presenters before the vCPUs */ + pnv->nr_servers = pnv->num_chips * smp_cores * smp_threads; + pnv->icps = g_new0(ICPState, pnv->nr_servers); + for (i = 0; i < pnv->nr_servers; i++) { + ICPState *icp = &pnv->icps[i]; + object_initialize(icp, sizeof(*icp), TYPE_ICP); + qdev_set_parent_bus(DEVICE(icp), sysbus_get_default()); + object_property_add_child(OBJECT(pnv), "icp[*]", OBJECT(icp), + &error_fatal); + object_property_add_const_link(OBJECT(icp), "xics", OBJECT(pnv), + &error_fatal); + object_property_set_bool(OBJECT(icp), true, "realized", &error_fatal); + } + + /* and the list of Interrupt Control Sources */ + QLIST_INIT(&pnv->ics); + /* Create the processor chips */ chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", machine->cpu_model); if (!object_class_by_name(chip_typename)) { @@ -742,6 +761,48 @@ static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name, visit_type_uint32(v, name, &POWERNV_MACHINE(obj)->num_chips, errp); } +static ICSState *pnv_ics_get(XICSFabric *xi, int irq) +{ + PnvMachineState *pnv = POWERNV_MACHINE(xi); + ICSState *ics; + + QLIST_FOREACH(ics, &pnv->ics, list) { + if (ics_valid_irq(ics, irq)) { + return ics; + } + } + return NULL; +} + +static void pnv_ics_resend(XICSFabric *xi) +{ + PnvMachineState *pnv = POWERNV_MACHINE(xi); + ICSState *ics; + + QLIST_FOREACH(ics, &pnv->ics, list) { + ics_resend(ics); + } +} + +static void pnv_ics_eoi(XICSFabric *xi, int irq) +{ + PnvMachineState *pnv = POWERNV_MACHINE(xi); + ICSState *ics; + + QLIST_FOREACH(ics, &pnv->ics, list) { + if (ics_valid_irq(ics, irq)) { + ics_eoi(ics, irq); + } + } +} + +static ICPState *pnv_icp_get(XICSFabric *xi, int server) +{ + PnvMachineState *pnv = POWERNV_MACHINE(xi); + + return (server < pnv->nr_servers) ? &pnv->icps[server] : NULL; +} + static void pnv_set_num_chips(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { @@ -783,9 +844,27 @@ static void powernv_machine_class_props_init(ObjectClass *oc) NULL); } +static void pnv_pic_print_info(InterruptStatsProvider *obj, + Monitor *mon) +{ + PnvMachineState *pnv = POWERNV_MACHINE(obj); + ICSState *ics; + int i; + + for (i = 0; i < pnv->nr_servers; i++) { + icp_pic_print_info(&pnv->icps[i], mon); + } + + QLIST_FOREACH(ics, &pnv->ics, list) { + ics_pic_print_info(ics, mon); + } +} + static void powernv_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); + XICSFabricClass *xic = XICS_FABRIC_CLASS(oc); + InterruptStatsProviderClass *ispc = INTERRUPT_STATS_PROVIDER_CLASS(oc); mc->desc = "IBM PowerNV (Non-Virtualized)"; mc->init = ppc_powernv_init; @@ -796,6 +875,11 @@ static void powernv_machine_class_init(ObjectClass *oc, void *data) mc->no_parallel = 1; mc->default_boot_order = NULL; mc->default_ram_size = 1 * G_BYTE; + xic->icp_get = pnv_icp_get; + xic->ics_get = pnv_ics_get; + xic->ics_eoi = pnv_ics_eoi; + xic->ics_resend = pnv_ics_resend; + ispc->print_info = pnv_pic_print_info; powernv_machine_class_props_init(oc); } @@ -806,6 +890,11 @@ static const TypeInfo powernv_machine_info = { .instance_size = sizeof(PnvMachineState), .instance_init = powernv_machine_initfn, .class_init = powernv_machine_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_XICS_FABRIC }, + { TYPE_INTERRUPT_STATS_PROVIDER }, + { }, + }, }; static void powernv_machine_register_types(void) diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h index df98a72006e4..6a0b004cea93 100644 --- a/include/hw/ppc/pnv.h +++ b/include/hw/ppc/pnv.h @@ -22,6 +22,7 @@ #include "hw/boards.h" #include "hw/sysbus.h" #include "hw/ppc/pnv_lpc.h" +#include "hw/ppc/xics.h" #define TYPE_PNV_CHIP "powernv-chip" #define PNV_CHIP(obj) OBJECT_CHECK(PnvChip, (obj), TYPE_PNV_CHIP) @@ -114,6 +115,9 @@ typedef struct PnvMachineState { PnvChip **chips; ISABus *isa_bus; + ICPState *icps; + uint32_t nr_servers; + QLIST_HEAD(, ICSState) ics; } PnvMachineState; #define PNV_FDT_ADDR 0x01000000 diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h index 00b003b2392d..c2032cac55f6 100644 --- a/include/hw/ppc/xics.h +++ b/include/hw/ppc/xics.h @@ -115,6 +115,7 @@ struct ICSState { qemu_irq *qirqs; ICSIRQState *irqs; XICSFabric *xics; + QLIST_ENTRY(ICSState) list; }; static inline bool ics_valid_irq(ICSState *ics, uint32_t nr)
Like this is done for the sPAPR machine, we use a simple array under the PowerNV machine to store the Interrupt Control Presenters (ICP) objects, one for each vCPU. This array is indexed by 'cpu_index' of the CPUState. We use a list to hold the different Interrupt Control Sources (ICS) objects, as PowerNV needs to handle multiple sources: for PCI-E and for the Processor Service Interface (PSI). Finally, to interface with the XICS layer which manipulates the ICP and ICS objects, we extend the PowerNV machine with an XICSFabric interface and its associated handlers. Signed-off-by: Cédric Le Goater <clg@kaod.org> --- hw/ppc/pnv.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++ include/hw/ppc/pnv.h | 4 +++ include/hw/ppc/xics.h | 1 + 3 files changed, 94 insertions(+)