From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49808) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmLvF-0003R5-Ta for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:21:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmLvB-0003fo-HN for qemu-devel@nongnu.org; Tue, 20 Sep 2016 10:21:20 -0400 Date: Tue, 20 Sep 2016 23:25:31 +1000 From: David Gibson Message-ID: <20160920132531.GH20488@umbus> References: <1474275250-26433-1-git-send-email-clg@kaod.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Nwn6wlxaqfRJxJFc" Content-Disposition: inline In-Reply-To: <1474275250-26433-1-git-send-email-clg@kaod.org> Subject: Re: [Qemu-devel] [RFC PATCH] ppc/xics: introduce helpers to find an ICP from some (CPU) index List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: qemu-ppc@nongnu.org, Bharata B Rao , Nikunj A Dadhania , qemu-devel@nongnu.org --Nwn6wlxaqfRJxJFc Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 19, 2016 at 10:54:10AM +0200, C=E9dric Le Goater wrote: > Today, the CPU index is used to index the icp array under xics. This > works correctly when the indexes are sync but that is an assumption > that could break future implementations. For instance, the PowerNV > platform CPUs use real HW ids and they are not contiguous. >=20 > Let's introduce some helpers to hide the underlying structure of the > ICP array. The criteria to look for an ICPstate is still the CPU index > but it is decorrelated from the array index. >=20 > This is an RFC to see what people think. It is used on the powernv > branch but it won't apply as it is on upstream. It should certainly be > optimised when a large number of CPUs are configured. >=20 > Signed-off-by: C=E9dric Le Goater Hm, so. First, I think the helpers to get the right icp state are a good idea, regardless of anything else. I'm happy to go ahead with a patch which introduces just that, because it will make future revisions easier. But, the rest of the changes seem to be predicated on allowing non-contiguous cpu_index values. Maybe we want to do that eventually, but we're a pretty long way off at present, doing so involves work in libvirt as well as qemu itself, and it's not clear we actually want it. I think for the time being you'd be better off keeping the simple array of icp states indexed by contiguous (barring cpu hotplug) cpu_index values, and dissociating the physical IDs (PIR =3D=3D interrupt server number =3D=3D DT id, IIUC) from the cpu_index. Obviously you'll need helpers to convert between cpu_index and physical ID at the machine level. > --- > hw/intc/xics.c | 59 ++++++++++++++++++++++++++++++++++++++++---= ------- > hw/intc/xics_kvm.c | 7 +---- > hw/intc/xics_spapr.c | 14 ++++++----- > include/hw/ppc/xics.h | 1=20 > 4 files changed, 59 insertions(+), 22 deletions(-) >=20 > Index: qemu-dgibson-for-2.8.git/hw/intc/xics.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics.c > @@ -50,12 +50,41 @@ int xics_get_cpu_index_by_dt_id(int cpu_ > return -1; > } > =20 > + > +static ICPState *xics_get_icp(XICSState *xics, CPUState *cs) > +{ > + int i; > + > + for (i =3D 0 ; i < xics->nr_servers; i++) { > + ICPState *ss =3D &xics->ss[i]; > + if (ss->cs =3D=3D NULL) { > + ss->cs =3D cs; > + return ss; > + } > + } > + > + return NULL; > +} > + > +ICPState *xics_find_icp(XICSState *xics, int cpu_index) > +{ > + int i; > + > + for (i =3D 0 ; i < xics->nr_servers; i++) { > + ICPState *ss =3D &xics->ss[i]; > + if (ss->cs && ss->cs->cpu_index =3D=3D cpu_index) > + return ss; > + } > + > + return NULL; > +} > + > void xics_cpu_destroy(XICSState *xics, PowerPCCPU *cpu) > { > CPUState *cs =3D CPU(cpu); > - ICPState *ss =3D &xics->ss[cs->cpu_index]; > + ICPState *ss =3D xics_find_icp(xics, cs->cpu_index); > =20 > - assert(cs->cpu_index < xics->nr_servers); > + assert(ss); > assert(cs =3D=3D ss->cs); > =20 > ss->output =3D NULL; > @@ -66,12 +95,10 @@ void xics_cpu_setup(XICSState *xics, Pow > { > CPUState *cs =3D CPU(cpu); > CPUPPCState *env =3D &cpu->env; > - ICPState *ss =3D &xics->ss[cs->cpu_index]; > + ICPState *ss =3D xics_get_icp(xics, cs); > XICSStateClass *info =3D XICS_COMMON_GET_CLASS(xics); > =20 > - assert(cs->cpu_index < xics->nr_servers); > - > - ss->cs =3D cs; > + assert(ss); > =20 > if (info->cpu_setup) { > info->cpu_setup(xics, cpu); > @@ -276,9 +303,11 @@ static void icp_check_ipi(ICPState *ss) > =20 > static void icp_resend(XICSState *xics, int server) > { > - ICPState *ss =3D xics->ss + server; > + ICPState *ss =3D xics_find_icp(xics, server); > ICSState *ics; > =20 > + assert(ss); > + > if (ss->mfrr < CPPR(ss)) { > icp_check_ipi(ss); > } > @@ -289,10 +318,12 @@ static void icp_resend(XICSState *xics, > =20 > void icp_set_cppr(XICSState *xics, int server, uint8_t cppr) > { > - ICPState *ss =3D xics->ss + server; > + ICPState *ss =3D xics_find_icp(xics, server); > uint8_t old_cppr; > uint32_t old_xisr; > =20 > + assert(ss); > + > old_cppr =3D CPPR(ss); > ss->xirr =3D (ss->xirr & ~CPPR_MASK) | (cppr << 24); > =20 > @@ -316,7 +347,9 @@ void icp_set_cppr(XICSState *xics, int s > =20 > void icp_set_mfrr(XICSState *xics, int server, uint8_t mfrr) > { > - ICPState *ss =3D xics->ss + server; > + ICPState *ss =3D xics_find_icp(xics, server); > + > + assert(ss); > =20 > ss->mfrr =3D mfrr; > if (mfrr < CPPR(ss)) { > @@ -348,10 +381,12 @@ uint32_t icp_ipoll(ICPState *ss, uint32_ > =20 > void icp_eoi(XICSState *xics, int server, uint32_t xirr) > { > - ICPState *ss =3D xics->ss + server; > + ICPState *ss =3D xics_find_icp(xics, server); > ICSState *ics; > uint32_t irq; > =20 > + assert(ss); > + > /* Send EOI -> ICS */ > ss->xirr =3D (ss->xirr & ~CPPR_MASK) | (xirr & CPPR_MASK); > trace_xics_icp_eoi(server, xirr, ss->xirr); > @@ -369,7 +404,9 @@ void icp_eoi(XICSState *xics, int server > static void icp_irq(ICSState *ics, int server, int nr, uint8_t priority) > { > XICSState *xics =3D ics->xics; > - ICPState *ss =3D xics->ss + server; > + ICPState *ss =3D xics_find_icp(xics, server); > + > + assert(ss); > =20 > trace_xics_icp_irq(server, nr, priority); > =20 > Index: qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- qemu-dgibson-for-2.8.git.orig/include/hw/ppc/xics.h > +++ qemu-dgibson-for-2.8.git/include/hw/ppc/xics.h > @@ -207,5 +207,6 @@ ICSState *xics_find_source(XICSState *ic > void xics_add_ics(XICSState *xics); > =20 > void xics_hmp_info_pic(Monitor *mon, const QDict *qdict); > +ICPState *xics_find_icp(XICSState *xics, int cpu_index); > =20 > #endif /* XICS_H */ > Index: qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_spapr.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics_spapr.c > @@ -67,9 +67,11 @@ static target_ulong h_xirr(PowerPCCPU *c > target_ulong opcode, target_ulong *args) > { > CPUState *cs =3D CPU(cpu); > - uint32_t xirr =3D icp_accept(spapr->xics->ss + cs->cpu_index); > + ICPState *ss =3D xics_find_icp(spapr->xics, cs->cpu_index); > =20 > - args[0] =3D xirr; > + assert(ss); > + > + args[0] =3D icp_accept(ss); > return H_SUCCESS; > } > =20 > @@ -77,10 +79,9 @@ static target_ulong h_xirr_x(PowerPCCPU > target_ulong opcode, target_ulong *args) > { > CPUState *cs =3D CPU(cpu); > - ICPState *ss =3D &spapr->xics->ss[cs->cpu_index]; > - uint32_t xirr =3D icp_accept(ss); > + ICPState *ss =3D xics_find_icp(spapr->xics, cs->cpu_index); > =20 > - args[0] =3D xirr; > + args[0] =3D icp_accept(ss); > args[1] =3D cpu_get_host_ticks(); > return H_SUCCESS; > } > @@ -99,8 +100,9 @@ static target_ulong h_ipoll(PowerPCCPU * > target_ulong opcode, target_ulong *args) > { > CPUState *cs =3D CPU(cpu); > + ICPState *ss =3D xics_find_icp(spapr->xics, cs->cpu_index); > uint32_t mfrr; > - uint32_t xirr =3D icp_ipoll(spapr->xics->ss + cs->cpu_index, &mfrr); > + uint32_t xirr =3D icp_ipoll(ss, &mfrr); > =20 > args[0] =3D xirr; > args[1] =3D mfrr; > Index: qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > --- qemu-dgibson-for-2.8.git.orig/hw/intc/xics_kvm.c > +++ qemu-dgibson-for-2.8.git/hw/intc/xics_kvm.c > @@ -326,14 +326,11 @@ static const TypeInfo ics_kvm_info =3D { > */ > static void xics_kvm_cpu_setup(XICSState *xics, PowerPCCPU *cpu) > { > - CPUState *cs; > - ICPState *ss; > + CPUState *cs =3D CPU(cpu); > + ICPState *ss =3D xics_find_icp(xics, cs->cpu_index); > KVMXICSState *xicskvm =3D XICS_SPAPR_KVM(xics); > int ret; > =20 > - cs =3D CPU(cpu); > - ss =3D &xics->ss[cs->cpu_index]; > - > assert(cs->cpu_index < xics->nr_servers); > if (xicskvm->kernel_xics_fd =3D=3D -1) { > abort(); >=20 --=20 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 --Nwn6wlxaqfRJxJFc Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX4TjIAAoJEGw4ysog2bOSIt0QAJb+CeSFv7NHz9n2417I+uNw Mq3WkEsZmG2Qxb6iuu7Ab2HlQYUU7euoUfM5Y3Viba4uAK3tvM9hetFnWW/NXs9D /ORSjK3VicgjGhfTyiOtniQF9wWlpVPK1Unr3nbqAVk+f0XnmmgQN2g/VEVDnd+w AqEJXz3knZSllfZqRoyeLiL9mlw/VzTDDtryxy7YNcOscfu+DlearAUs1TUpTg8n 0hLuQ+E9nPpz0HPd3ov84PUKVQiLpi5IZ7T7eo46QrsgIsB2atSCWqkLKm/X4ZH0 UgFRb9124u3VU1SyYoLfRKwJ0dZH92uCF3/VRhQfZXzPP2uSCAtzdaGslxtYtGiQ fjxXntOqOE42T07k9FUlOv3Pb4I9+zYsKUhLOb936BFhSA4bJfna1+voxNGu5z19 Dp27CWn+YqVTnso5PiCQoG3LDRMHMR35fNO9X/Suk5vzpkv4uZMTcrNLGGsnSo1r 0EWXM4BrQI+6xTrmHULbRx63M0RzAURmMPGua/bfFhSk6DDqumo9jPBvBHhPMDgE q4nsWKeCDPR+HyfF4g4+ammrvbxuV5vCky1b/mVjADeRRREvrUBR1XjiIqK1FGKs 4d8jfKlmUYZuLg8CwiUNewqvkqpdT4DD9BwboXb4WKxb8fq1R35ypK+8yjBU3pmk 74KZAGdbU5eioQJo2utp =K85W -----END PGP SIGNATURE----- --Nwn6wlxaqfRJxJFc--