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

[David Gibson]

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.

> +        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)

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature