Re: [Qemu-devel] [PATCH for-2.10 3/8] ppc/pnv: create the ICP and ICS objects under the machine

[Cédric Le Goater]

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 <address@hidden>
>> ---
>>  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)
>