From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44396) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dIqKq-0004QL-4u for qemu-devel@nongnu.org; Thu, 08 Jun 2017 01:50:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dIqKl-0005ra-Pu for qemu-devel@nongnu.org; Thu, 08 Jun 2017 01:50:20 -0400 Received: from 15.mo4.mail-out.ovh.net ([91.121.62.11]:41883) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dIqKl-0005qy-Fy for qemu-devel@nongnu.org; Thu, 08 Jun 2017 01:50:15 -0400 Received: from player772.ha.ovh.net (b6.ovh.net [213.186.33.56]) by mo4.mail-out.ovh.net (Postfix) with ESMTP id AF4B470839 for ; Thu, 8 Jun 2017 07:50:12 +0200 (CEST) References: <149685579678.12025.9278446121024037161.stgit@bahia.lan> <149685582923.12025.13700165807436904935.stgit@bahia.lan> <20170607225507.5d2c6c33@bahia.ttt.fr.ibm.com> From: =?UTF-8?Q?C=c3=a9dric_Le_Goater?= Message-ID: <3a0cab6c-a913-e7e8-4a07-fbd7509731db@kaod.org> Date: Thu, 8 Jun 2017 07:50:08 +0200 MIME-Version: 1.0 In-Reply-To: <20170607225507.5d2c6c33@bahia.ttt.fr.ibm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 3/5] xics: setup cpu at realize time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, David Gibson On 06/07/2017 10:55 PM, Greg Kurz wrote: > On Wed, 7 Jun 2017 20:11:38 +0200 > C=C3=A9dric Le Goater wrote: >=20 >> On 06/07/2017 07:17 PM, Greg Kurz wrote: >>> Until recently, spapr used to allocate ICPState objects for the lifet= ime >>> of the machine. They would only be associated to vCPUs in xics_cpu_se= tup() >>> when plugging a CPU core. >>> >>> Now that ICPState objects have the same lifecycle as vCPUs, it is >>> possible to associate them during realization. >>> >>> This patch hence open-codes xics_cpu_setup() in icp_realize(). The vC= PU >>> is passed as a property. Note that vCPU now needs to be realized firs= t >>> for the IRQs to be allocated. It also needs to resetted before ICPSta= te >>> realization in order to synchronize with KVM. >>> >>> Since ICPState objects are freed when unrealized, xics_cpu_destroy() = isn't >>> needed anymore and can be safely dropped. =20 >> >> I like the idea but I think the assignment of ->cs attribute should be= =20 >> moved under icp_kvm_cpu_setup(), as it is only used under xics_kvm by=20 >> the kvm_vcpu_ioctl() calls.=20 >> >=20 > Well, cs->cpu_index is also used for traces and to implement the 'info = pic' > monitor command. yes. But, these are just printfs. >> Ideally, we should also introduce : >> >> struct KvmState { >> ICPState parent_obj; >> =09 >> CPUState *cs; >> }; >> >> That would be cleaner, but that might introduce some migration compat=20 >> issues again ... >> >=20 > No, as long as it doesn't change state, we're good. My concern is more > about how to pass cs to xics_kvm=20 That can be done in the cpu_setup handler. > and the cs->cpu_index to xics. The printfs are interesting to have but not vital.=20 David has a good point for keeping ->cs. So let's abandon the idea. =20 >> Some minor comments below, >> >> Thanks, >> >> C.=20 >> >>> Signed-off-by: Greg Kurz >>> --- >>> hw/intc/xics.c | 76 ++++++++++++++++++++-----------------= ---------- >>> hw/ppc/pnv_core.c | 16 ++++------ >>> hw/ppc/spapr_cpu_core.c | 21 ++++++------- >>> include/hw/ppc/xics.h | 2 - >>> 4 files changed, 48 insertions(+), 67 deletions(-) >>> >>> diff --git a/hw/intc/xics.c b/hw/intc/xics.c >>> index ec73f02144c9..330441ff7fe8 100644 >>> --- a/hw/intc/xics.c >>> +++ b/hw/intc/xics.c >>> @@ -38,50 +38,6 @@ >>> #include "monitor/monitor.h" >>> #include "hw/intc/intc.h" >>> =20 >>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu) >>> -{ >>> - CPUState *cs =3D CPU(cpu); >>> - ICPState *icp =3D ICP(cpu->intc); >>> - >>> - assert(icp); >>> - assert(cs =3D=3D icp->cs); >>> - >>> - icp->output =3D NULL; >>> - icp->cs =3D NULL; >>> -} >>> - >>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp) >>> -{ >>> - CPUState *cs =3D CPU(cpu); >>> - CPUPPCState *env =3D &cpu->env; >>> - ICPStateClass *icpc; >>> - >>> - assert(icp); >>> - >>> - cpu->intc =3D OBJECT(icp); >>> - icp->cs =3D cs; >>> - >>> - icpc =3D ICP_GET_CLASS(icp); >>> - if (icpc->cpu_setup) { >>> - icpc->cpu_setup(icp, cpu); >>> - } >>> - >>> - switch (PPC_INPUT(env)) { >>> - case PPC_FLAGS_INPUT_POWER7: >>> - icp->output =3D env->irq_inputs[POWER7_INPUT_INT]; >>> - break; >>> - >>> - case PPC_FLAGS_INPUT_970: >>> - icp->output =3D env->irq_inputs[PPC970_INPUT_INT]; >>> - break; >>> - >>> - default: >>> - error_report("XICS interrupt controller does not support thi= s CPU " >>> - "bus model"); >>> - abort(); >>> - } >>> -} >>> - >>> void icp_pic_print_info(ICPState *icp, Monitor *mon) >>> { >>> int cpu_index =3D icp->cs ? icp->cs->cpu_index : -1; >>> @@ -343,6 +299,8 @@ static void icp_realize(DeviceState *dev, Error *= *errp) >>> { >>> ICPState *icp =3D ICP(dev); >>> ICPStateClass *icpc =3D ICP_GET_CLASS(dev); >>> + PowerPCCPU *cpu; >>> + CPUPPCState *env; >>> Object *obj; >>> Error *err =3D NULL; >>> =20 >>> @@ -355,6 +313,36 @@ static void icp_realize(DeviceState *dev, Error = **errp) >>> =20 >>> icp->xics =3D XICS_FABRIC(obj); >>> =20 >>> + obj =3D object_property_get_link(OBJECT(dev), "cs", &err); >>> + if (!obj) { >>> + error_setg(errp, "%s: required link 'xics' not found: %s", >>> + __func__, error_get_pretty(err)); >>> + return; >>> + } >>> + >>> + cpu =3D POWERPC_CPU(obj); >>> + cpu->intc =3D OBJECT(icp); >>> + icp->cs =3D CPU(obj); =20 >> >> only xics_kvm needs ->cs.=20 >> >=20 > yeah, maybe the base class should only have a cpu_index field: >=20 > icp->cpu_index =3D CPU(obj)->cpu_index; arg, no. please, let's not add another cpu index :) >>> + >>> + if (icpc->cpu_setup) { >>> + icpc->cpu_setup(icp, cpu); >>> + } >>> + >>> + env =3D &cpu->env; >>> + switch (PPC_INPUT(env)) { >>> + case PPC_FLAGS_INPUT_POWER7: >>> + icp->output =3D env->irq_inputs[POWER7_INPUT_INT]; >>> + break; >>> + >>> + case PPC_FLAGS_INPUT_970: >>> + icp->output =3D env->irq_inputs[PPC970_INPUT_INT]; >>> + break; >>> + >>> + default: >>> + error_setg(errp, "XICS interrupt controller does not support= this CPU bus model"); >>> + return; >>> + } >>> + >>> if (icpc->realize) { >>> icpc->realize(dev, errp); >>> } >>> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c >>> index e8a9a94d5a24..1393005e76f3 100644 >>> --- a/hw/ppc/pnv_core.c >>> +++ b/hw/ppc/pnv_core.c >>> @@ -118,19 +118,19 @@ static void pnv_core_realize_child(Object *chil= d, XICSFabric *xi, Error **errp) >>> PowerPCCPU *cpu =3D POWERPC_CPU(cs); >>> Object *obj; >>> =20 >>> - obj =3D object_new(TYPE_PNV_ICP); >>> - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort)= ; >>> - object_unref(obj); >>> - object_property_add_const_link(obj, "xics", OBJECT(xi), &error_a= bort); >>> - object_property_set_bool(obj, true, "realized", &local_err); >>> + object_property_set_bool(child, true, "realized", &local_err); >>> if (local_err) { >>> error_propagate(errp, local_err); >>> return; >>> } >>> =20 >>> - object_property_set_bool(child, true, "realized", &local_err); >>> + obj =3D object_new(TYPE_PNV_ICP); >>> + object_property_add_child(child, "icp", obj, NULL); >>> + object_unref(obj); >>> + object_property_add_const_link(obj, "xics", OBJECT(xi), &error_a= bort); >>> + object_property_add_const_link(obj, "cs", child, &error_abort); = =20 >> >> may be link the cpu object instead ?=20 >> >=20 > I'm not sure to understand... isn't child supposed to be a CPU index he= re ? I meant linking the 'PowerPCCPU *cpu' object and not the CPUState. That's minor. >>> + object_property_set_bool(obj, true, "realized", &local_err); >>> if (local_err) { >>> - object_unparent(obj); >>> error_propagate(errp, local_err); >>> return; >>> } >>> @@ -141,8 +141,6 @@ static void pnv_core_realize_child(Object *child,= XICSFabric *xi, Error **errp) >>> error_propagate(errp, local_err); >>> return; >>> } >>> - >>> - xics_cpu_setup(xi, cpu, ICP(obj)); >>> } >>> =20 >>> static void pnv_core_realize(DeviceState *dev, Error **errp) >>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c >>> index 029a14120edd..9a6259525953 100644 >>> --- a/hw/ppc/spapr_cpu_core.c >>> +++ b/hw/ppc/spapr_cpu_core.c >>> @@ -53,9 +53,6 @@ static void spapr_cpu_reset(void *opaque) >>> =20 >>> static void spapr_cpu_destroy(PowerPCCPU *cpu) >>> { >>> - sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); >>> - >>> - xics_cpu_destroy(XICS_FABRIC(spapr), cpu); >>> qemu_unregister_reset(spapr_cpu_reset, cpu); >>> } >>> =20 >>> @@ -140,28 +137,28 @@ static void spapr_cpu_core_realize_child(Object= *child, Error **errp) >>> sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_machine()); >>> CPUState *cs =3D CPU(child); >>> PowerPCCPU *cpu =3D POWERPC_CPU(cs); >>> - Object *obj; >>> + Object *obj =3D NULL; >>> =20 >>> - obj =3D object_new(spapr->icp_type); >>> - object_property_add_child(OBJECT(cpu), "icp", obj, &error_abort)= ; >>> - object_unref(obj); >>> - object_property_add_const_link(obj, "xics", OBJECT(spapr), &erro= r_abort); >>> - object_property_set_bool(obj, true, "realized", &local_err); >>> + object_property_set_bool(child, true, "realized", &local_err); >>> if (local_err) { >>> goto error; >>> } >>> =20 >>> - object_property_set_bool(child, true, "realized", &local_err); >>> + spapr_cpu_init(spapr, cpu, &local_err); >>> if (local_err) { >>> goto error; >>> } >>> =20 >>> - spapr_cpu_init(spapr, cpu, &local_err); >>> + obj =3D object_new(spapr->icp_type); >>> + object_property_add_child(child, "icp", obj, &error_abort); >>> + object_unref(obj); >>> + object_property_add_const_link(obj, "xics", OBJECT(spapr), &erro= r_abort); >>> + object_property_add_const_link(obj, "cs", child, &error_abort); = =20 >> >> same here. >> >>> + object_property_set_bool(obj, true, "realized", &local_err); >>> if (local_err) { >>> goto error; >>> } >>> =20 >>> - xics_cpu_setup(XICS_FABRIC(spapr), cpu, ICP(obj)); >>> return; >>> =20 >>> error: >>> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h >>> index 40a506eacfb4..05767a15be9a 100644 >>> --- a/include/hw/ppc/xics.h >>> +++ b/include/hw/ppc/xics.h >>> @@ -183,8 +183,6 @@ void spapr_dt_xics(int nr_servers, void *fdt, uin= t32_t phandle); >>> =20 >>> qemu_irq xics_get_qirq(XICSFabric *xi, int irq); >>> ICPState *xics_icp_get(XICSFabric *xi, int server); >>> -void xics_cpu_setup(XICSFabric *xi, PowerPCCPU *cpu, ICPState *icp); >>> -void xics_cpu_destroy(XICSFabric *xi, PowerPCCPU *cpu); >>> =20 >>> /* Internal XICS interfaces */ >>> void icp_set_cppr(ICPState *icp, uint8_t cppr); >>> =20 >> >=20