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 <c...@kaod.org>
> >> ---
> >>
> >> 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
