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

[Cédric Le Goater]

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.

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