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