From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YQQZW-0000pF-CA for qemu-devel@nongnu.org; Tue, 24 Feb 2015 20:15:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YQQZU-0006mV-Hw for qemu-devel@nongnu.org; Tue, 24 Feb 2015 20:15:30 -0500 Date: Wed, 25 Feb 2015 11:54:18 +1100 From: David Gibson Message-ID: <20150225005418.GB15695@voom.fritz.box> References: <1424096872-29868-1-git-send-email-mdroth@linux.vnet.ibm.com> <1424096872-29868-9-git-send-email-mdroth@linux.vnet.ibm.com> <20150224064942.GR4536@voom.redhat.com> <20150224200429.31752.33720@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="eAbsdosE1cNLO4uF" Content-Disposition: inline In-Reply-To: <20150224200429.31752.33720@loki> Subject: Re: [Qemu-devel] [PATCH v5 08/16] spapr_events: re-use EPOW event infrastructure for hotplug events List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: aik@ozlabs.ru, qemu-devel@nongnu.org, agraf@suse.de, ncmike@ncultra.org, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, bharata.rao@gmail.com, nfont@linux.vnet.ibm.com --eAbsdosE1cNLO4uF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 24, 2015 at 02:04:29PM -0600, Michael Roth wrote: > Quoting David Gibson (2015-02-24 00:49:42) > > On Mon, Feb 16, 2015 at 08:27:44AM -0600, Michael Roth wrote: > > > From: Nathan Fontenot [snip] > > > + hdr =3D &pending_epow->hdr; > > > + v6hdr =3D &pending_epow->v6hdr; > > > + maina =3D &pending_epow->maina; > > > + mainb =3D &pending_epow->mainb; > > > + epow =3D &pending_epow->epow; > > > + > > > + hdr->summary =3D cpu_to_be32(RTAS_LOG_VERSION_6 > > > + | RTAS_LOG_SEVERITY_EVENT > > > + | RTAS_LOG_DISPOSITION_NOT_RECOVERED > > > + | RTAS_LOG_OPTIONAL_PART_PRESENT > > > + | RTAS_LOG_TYPE_EPOW); > > > + hdr->extended_length =3D cpu_to_be32(sizeof(*pending_epow) > > > + - sizeof(pending_epow->hdr)); > > > + > > > + spapr_init_v6hdr(v6hdr); > > > + spapr_init_maina(maina, 3 /* Main-A, Main-B and EPOW */); > > > =20 > > > mainb->hdr.section_id =3D cpu_to_be16(RTAS_LOG_V6_SECTION_ID_MAI= NB); > > > mainb->hdr.section_length =3D cpu_to_be16(sizeof(*mainb)); > > > @@ -274,7 +356,78 @@ static void spapr_powerdown_req(Notifier *n, voi= d *opaque) > > > epow->event_modifier =3D RTAS_LOG_V6_EPOW_MODIFIER_NORMAL; > > > epow->extended_modifier =3D RTAS_LOG_V6_EPOW_XMODIFIER_PARTITION= _SPECIFIC; > > > =20 > > > - qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->epow_irq)); > > > + rtas_event_log_queue(RTAS_LOG_TYPE_EPOW, pending_epow); > > > + > > > + qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_= irq)); > >=20 > > Shouldn't the irq pulse go into rtas_event_log_queue()? >=20 > It seems maybe it should... >=20 > "If a platform chooses to report multiple event or error sources through a > single interrupt, it must ensure that the interrupt remains asserted > or is re-asserted until check-exception has been used to process all > out-standing errors or events for that interrupt." >=20 > But another alternative would be to have rtas-check-exception re-pulse if= there > are still events present for that particular IRQ. I think I'd prefer this > approach, since it makes it easier to share the queueing functionality wi= th > rtas-event-scan (which relies on polling instead of interrupts) Actually I wasn't thinking that deeply, just looking at the fact that both callerss to rtas_event_log_queue() pulse the irq immediately afterwards. Hrm. Now I'm a bit confused - the use of qemu_irq_pulse() suggests an edge/message interrupt, but the reference to "remains asserted" suggests level interrupt semantics. I no longer remember enough of PAPR to know why I used qemu_irq_pulse() in the first place. [snip] > > > - cpu_physical_memory_write(buf, pending_epow, len); > > > - g_free(pending_epow); > > > - pending_epow =3D NULL; > > > - rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > > - } else { > > > - rtas_st(rets, 0, RTAS_OUT_NO_ERRORS_FOUND); > > > + switch (event->log_type) { > > > + case RTAS_LOG_TYPE_EPOW: > > > + event_len =3D sizeof(struct epow_log_full); > > > + break; > > > + case RTAS_LOG_TYPE_HOTPLUG: > > > + event_len =3D sizeof(struct hp_log_full); > > > + break; > > > + default: > > > + goto out_no_events; > >=20 > > Doesn't one of the headers include size information you could use > > here, avoiding the switch? >=20 > Looks like hdr->extended_length + sizeof(hdr) should do the > trick. Nice! Excellent. A switch on the same selector at multiple levels of the same callstack always seems to be a warning sign of layering violations. --=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 --eAbsdosE1cNLO4uF Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU7R06AAoJEGw4ysog2bOSx9EQAKi01hV2i90nmcEtokRL4bFB MOqNp6zUiWsxR8BEM9ed2NYjiz3YhCLnq9Ue2X/7rnX4z6WXv6k1X+Sv5quPTLfO JgkvjYTUi9HkTc7l/SRyb5yd6Akz5pIyko9CkTjgTtNypO4/w72V0l7XKBMuKtIe Hz730MNQfo1vdyjji9ifQ6iy8Gn21ZL8d5CY4HHHyWozFZ13r5VNM39rjXRpmgUl ubjBIeKkdasvXLw88TmSDCX4DIf6jGsJaoBWshE+pkf52rExMtCcDpFD80CI0+xf +QW/BTh0ghi5vzOzX7kGnpLRBUj2gKgp3ojOZluSanUFj0t6jyzHDKulasxDkVpH aMMOhIlP65mWQ9kSHZDeGLq6jT2sJr293Vh9td2tKgg0Hgnfs/PoOWkcVIjyEj1w eR55s9ufxIykSV8qikyJvP/gEL94PwOZTKRPFsZeQK7BokLqcqa4aP418mVhmSB0 EQ0PlGXVeEwIi7isQHbI222YXAZ8P4ZKpvKLGO+MNpJbMtyOWKakc6saffLnKYz0 Ayb3uwfeRbg2J21q5CGkmM2hE8XAaaY+78eSsZPIDNNZnuVJDpr4alH6dCQIpCzy mdeRxt1e4SpcNf5nlwXlm1sBnN4beJiR6UzzjLfpPxYakJOUtOl7pXkJiwzGyUEn cTfjvptWmROWlaW3E3YV =dpLf -----END PGP SIGNATURE----- --eAbsdosE1cNLO4uF--