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 <c...@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)
>
