Re: [Qemu-ppc] [PATCH v3 5/8] ppc/pnv: create the ICP and ICS objects under the machine

[David Gibson]

On Wed, Mar 29, 2017 at 10:13:59AM +0200, Cédric Le Goater wrote:
> On 03/29/2017 07:18 AM, David Gibson wrote:
> > On Tue, Mar 28, 2017 at 09:32:29AM +0200, 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 but the users will provide a core PIR number. The mapping
> >> is done in the icp_get() handler of the machine and is transparent to
> >> XICS.
> >>
> >> The Interrupt Control Sources (ICS), Processor Service Interface and
> >> PCI-E interface models, will be introduced in subsequent patches. For
> >> now, we have none, so we just prepare ground with place holders.
> >>
> >> 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>
> >> ---
> >>
> >>  Changes since v2:
> >>
> >>  - removed the list of ICS. The handlers will iterate on the chips to
> >>    use the available ICS.
> >>
> >>  Changes since v1:
> >>
> >>  - handled pir-to-cpu_index mapping under icp_get 
> >>  - removed ics_eio handler
> >>  - changed ICP name indexing
> >>  - removed sysbus parenting of the ICP object
> >>
> >>  hw/ppc/pnv.c         | 96 
> >> ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/ppc/pnv.h |  3 ++
> >>  2 files changed, 99 insertions(+)
> >>
> >> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> >> index 3fa722af82e6..e441b8ac1cad 100644
> >> --- a/hw/ppc/pnv.c
> >> +++ b/hw/ppc/pnv.c
> >> @@ -33,7 +33,10 @@
> >>  #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/xics.h"
> >>  #include "hw/ppc/pnv_xscom.h"
> >>  
> >>  #include "hw/isa/isa.h"
> >> @@ -417,6 +420,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(PnvICPState, pnv->nr_servers);
> >> +    for (i = 0; i < pnv->nr_servers; i++) {
> >> +        PnvICPState *icp = &pnv->icps[i];
> >> +        char name[32];
> >> +
> >> +        /* TODO: fix ICP object name to be in sync with the core name */
> >> +        snprintf(name, sizeof(name), "icp[%d]", i);
> > 
> > It may end up being the same value, but since the qom name is exposed
> > to the outside, it would be better to have it be the PIR, rather than
> > the cpu_index.
> 
> The problem is that the ICPState objects are created before the PnvChip
> objects. The PnvChip sanitizes the core layout in terms HW id and then 
> creates the PnvCore objects. The core creates a PowerPCCPU object per 
> thread and, in the realize function, uses the XICSFabric to look for
> a matching ICP to link the CPU with. 
> 
> So it is a little complex to do what you ask for ...
> 
> What would really simplify the code, would be to allocate a TYPE_PNV_ICP
> object when the PowerPCCPU object is realized and store it under the 
> 'icp/intc' backlink. The XICSFabric handler icp_get() would not need 
> the 'icps' array anymore. 
> 
> How's that proposal ?

Sounds workable.  You could do that from the core realize function,
couldn't you?

> 
> >> +        object_initialize(icp, sizeof(*icp), TYPE_PNV_ICP);
> >> +        object_property_add_child(OBJECT(pnv), name, 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);
> >> +    }
> >> +
> >>      /* Create the processor chips */
> >>      chip_typename = g_strdup_printf(TYPE_PNV_CHIP "-%s", 
> >> machine->cpu_model);
> >>      if (!object_class_by_name(chip_typename)) {
> >> @@ -737,6 +757,71 @@ static const TypeInfo pnv_chip_info = {
> >>      .abstract      = true,
> >>  };
> >>  
> >> +static ICSState *pnv_ics_get(XICSFabric *xi, int irq)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +    return NULL;
> >> +}
> >> +
> >> +static void pnv_ics_resend(XICSFabric *xi)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +}
> > 
> > Seems like the above two functions belong in a later patch.
> 
> OK. I guess we can add these handlers in the next patchset introducing PSI. 
> I will check that the tests still run because the XICS layer uses the 
> XICSFabric handlers blindly without any checks.

Well, sure, but the whole XICS isn't going to work without real
implementations of the ICS callbacks, so I don't see that it matters.

>  
> >> +
> >> +static PowerPCCPU *ppc_get_vcpu_by_pir(int pir)
> >> +{
> >> +    CPUState *cs;
> >> +
> >> +    CPU_FOREACH(cs) {
> >> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> >> +        CPUPPCState *env = &cpu->env;
> >> +
> >> +        if (env->spr_cb[SPR_PIR].default_value == pir) {
> >> +            return cpu;
> >> +        }
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >> +static ICPState *pnv_icp_get(XICSFabric *xi, int pir)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(xi);
> >> +    PowerPCCPU *cpu = ppc_get_vcpu_by_pir(pir);
> >> +
> >> +    if (!cpu) {
> >> +        return NULL;
> >> +    }
> >> +
> >> +    assert(cpu->parent_obj.cpu_index < pnv->nr_servers);
> >> +    return ICP(&pnv->icps[cpu->parent_obj.cpu_index]);
> > 
> > Should use CPU() instead of parent_obj here.
> 
> OK. I might just remove the array though.
> 
> Thanks,
> 
> C.
> 
> 
> >> +}
> >> +
> >> +static void pnv_pic_print_info(InterruptStatsProvider *obj,
> >> +                               Monitor *mon)
> >> +{
> >> +    PnvMachineState *pnv = POWERNV_MACHINE(obj);
> >> +    int i;
> >> +
> >> +    for (i = 0; i < pnv->nr_servers; i++) {
> >> +        icp_pic_print_info(ICP(&pnv->icps[i]), mon);
> >> +    }
> >> +
> >> +    for (i = 0; i < pnv->num_chips; i++) {
> >> +        /* place holder */
> >> +    }
> >> +}
> >> +
> >>  static void pnv_get_num_chips(Object *obj, Visitor *v, const char *name,
> >>                                void *opaque, Error **errp)
> >>  {
> >> @@ -787,6 +872,8 @@ static void 
> >> powernv_machine_class_props_init(ObjectClass *oc)
> >>  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;
> >> @@ -797,6 +884,10 @@ 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_resend = pnv_ics_resend;
> >> +    ispc->print_info = pnv_pic_print_info;
> >>  
> >>      powernv_machine_class_props_init(oc);
> >>  }
> >> @@ -807,6 +898,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..1ca197d2ec83 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,8 @@ typedef struct PnvMachineState {
> >>      PnvChip      **chips;
> >>  
> >>      ISABus       *isa_bus;
> >> +    PnvICPState  *icps;
> >> +    uint32_t     nr_servers;
> >>  } PnvMachineState;
> >>  
> >>  #define PNV_FDT_ADDR          0x01000000
> > 
> 

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