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

[David Gibson]

On Thu, Mar 30, 2017 at 10:15:11AM +0200, Cédric Le Goater wrote:
> On 03/30/2017 03:55 AM, David Gibson wrote:
> > 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?
> 
> OK, it would look better from a QOM perspective, I agree, as we would 
> not "realize" the PowerPCCPU object before creating the ICP object.
> I will send an update of this patch for you to review : 
> 
>       [PATCH v4 5/9] ppc/pnv: create the ICP object under PnvCore
> 
> 
> We could follow the exact same pattern for the sPAPR machine if we 
> knew which ICP type to use (KVM or not). May be a new XICSFabric 
> handler :
> 
>       const char *icp_type(XICSFabric *xi)
> 
> would help for that ? I don't think we can use a property because of 
> the hotplug mechanism.

Hmm, couldn't we just move our existing logic for deciding the ICP
type into the spapr core object?

> 
>  
> >>>> +        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.
> 
> Just to be very clear, what I meant is that we need to define all
> the handlers at once because the XICS layer does not check the 
> handler validity and we would end up calling NULL. In patchset v4,
> the handlers are empty routines. 
> 
> Thanks,
> 
> C. 
>  
> >>  
> >>>> +
> >>>> +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