From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33705) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YFyqZ-0001SO-1T for qemu-devel@nongnu.org; Tue, 27 Jan 2015 00:37:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YFyqU-0000PF-EJ for qemu-devel@nongnu.org; Tue, 27 Jan 2015 00:37:54 -0500 Date: Tue, 27 Jan 2015 16:27:22 +1100 From: David Gibson Message-ID: <20150127052722.GD28888@voom.redhat.com> References: <1419337831-16552-1-git-send-email-mdroth@linux.vnet.ibm.com> <1419337831-16552-9-git-send-email-mdroth@linux.vnet.ibm.com> <20150119043123.GR5297@voom.fritz.box> <20150126165651.23721.17168@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="J5MfuwkIyy7RmF4Q" Content-Disposition: inline In-Reply-To: <20150126165651.23721.17168@loki> Subject: Re: [Qemu-devel] [PATCH v4 08/17] 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 --J5MfuwkIyy7RmF4Q Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 26, 2015 at 10:56:51AM -0600, Michael Roth wrote: > Quoting David Gibson (2015-01-18 22:31:23) > > On Tue, Dec 23, 2014 at 06:30:22AM -0600, Michael Roth wrote: > > > From: Nathan Fontenot [snip] > > > +static void spapr_hotplug_req_event(sPAPRDRConnector *drc, uint8_t h= p_action) > > > +{ > > > + struct hp_log_full *new_hp; > > > + struct rtas_error_log *hdr; > > > + struct rtas_event_log_v6 *v6hdr; > > > + struct rtas_event_log_v6_maina *maina; > > > + struct rtas_event_log_v6_mainb *mainb; > > > + struct rtas_event_log_v6_hp *hp; > > > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc= ); > > > + sPAPRDRConnectorType drc_type =3D drck->get_type(drc); > > > + > > > + new_hp =3D g_malloc0(sizeof(struct hp_log_full)); > > > + hdr =3D &new_hp->hdr; > > > + v6hdr =3D &new_hp->v6hdr; > > > + maina =3D &new_hp->maina; > > > + mainb =3D &new_hp->mainb; > > > + hp =3D &new_hp->hp; > > > + > > > + 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_INITIATOR_HOTPLUG > > > + | RTAS_LOG_TYPE_HOTPLUG); > > > + hdr->extended_length =3D cpu_to_be32(sizeof(*new_hp) > > > + - sizeof(new_hp->hdr)); > > > + > > > + spapr_init_v6hdr(v6hdr); > > > + spapr_init_maina(maina, 3 /* Main-A, Main-B, HP */); > > > + > > > + 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)); > > > + mainb->subsystem_id =3D 0x80; /* External environment */ > > > + mainb->event_severity =3D 0x00; /* Informational / non-error */ > > > + mainb->event_subtype =3D 0x00; /* Normal shutdown */ > > > + > > > + hp->hdr.section_id =3D cpu_to_be16(RTAS_LOG_V6_SECTION_ID_HOTPLU= G); > > > + hp->hdr.section_length =3D cpu_to_be16(sizeof(*hp)); > > > + hp->hdr.section_version =3D 1; /* includes extended modifier */ > > > + hp->hotplug_action =3D hp_action; > > > + > > > + > > > + switch (drc_type) { > > > + case SPAPR_DR_CONNECTOR_TYPE_PCI: > > > + hp->drc.index =3D cpu_to_be32(drck->get_index(drc)); > > > + hp->hotplug_identifier =3D RTAS_LOG_V6_HP_ID_DRC_INDEX; > > > + hp->hotplug_type =3D RTAS_LOG_V6_HP_TYPE_PCI; > > > + break; > > > + default: > > > + /* skip notification for unknown connector types */ > > > + g_free(new_hp); > > > + return; > > > + } > > > + > > > + if (pending_hp) { > > > + /* Just toss any pending hotplug events for now, this will > > > + * need to be fixed later on. > > > + */ > >=20 > > So, we can get away with a 1-element queue for EPOW, because they're > > just triggering a shutdown - so once the first one's processed, any > > others aren't going to matter. For hotplug you really do need a > > proper queue. >=20 > Yah, this was discussed in the past, but until now I didn't notice how > easy it would be to trigger this when hotplugging multiple devices from > a script or management harness of some sort. Should be simple enough to > fix for v5 though. Ok, good. > > > + g_free(pending_hp); > > > + } > > > + pending_hp =3D new_hp; > > > + > > > + qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_= irq)); > > > +} > > > + > > > +void spapr_hotplug_req_add_event(sPAPRDRConnector *drc) > > > +{ > > > + spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_ADD); > > > +} > > > + > > > +void spapr_hotplug_req_remove_event(sPAPRDRConnector *drc) > > > +{ > > > + spapr_hotplug_req_event(drc, RTAS_LOG_V6_HP_ACTION_REMOVE); > > > } > > > =20 > > > static void check_exception(PowerPCCPU *cpu, sPAPREnvironment *spapr, > > > @@ -298,15 +420,26 @@ static void check_exception(PowerPCCPU *cpu, sP= APREnvironment *spapr, > > > xinfo |=3D (uint64_t)rtas_ld(args, 6) << 32; > > > } > > > =20 > > > - if ((mask & EVENT_MASK_EPOW) && pending_epow) { > > > - if (sizeof(*pending_epow) < len) { > > > - len =3D sizeof(*pending_epow); > > > - } > > > + if (mask & EVENT_MASK_EPOW) { > > > + if (pending_epow) { > > > + if (sizeof(*pending_epow) < len) { > > > + len =3D sizeof(*pending_epow); > > > + } > > > =20 > > > - cpu_physical_memory_write(buf, pending_epow, len); > > > - g_free(pending_epow); > > > - pending_epow =3D NULL; > > > - rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > > + cpu_physical_memory_write(buf, pending_epow, len); > > > + g_free(pending_epow); > > > + pending_epow =3D NULL; > > > + rtas_st(rets, 0, RTAS_OUT_SUCCESS); > > > + } else if (pending_hp) { > >=20 > > So.. the hotplug messages are a different type from EPOW, but are > > still selected by EVENT_MASK_EPOW? Seems a bit odd. >=20 > It's a little odd, but it's mainly just due to the way we surface the > hotplug event. Rather than requiring patched guest kernels, we opted > to re-use and generalize the EPOW IRQ to also surface hotplug-related > RTAS events, which is why we still expect the EVENT_MASK_EPOW when > returning an HP event via check-exception. >=20 > EPOW events have well-defined behavior in how they're exposed to > userspace via rtas_errd, which allows us to add hotplug support for > older guests via patched userspace tools. Ok. Kind of a hack, but for good reasons. I kind of think it needs a comment, but I'm not sure where it would belong. --=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 --J5MfuwkIyy7RmF4Q Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUxyG6AAoJEGw4ysog2bOSaD0QAIFE4LpthUAVjLFSUqzG0wt0 OQFbk/niC/2sc2bx8/aSGt30cjC7a4pTY8TFFBmDaiicRltPFNCfgMq4Ck16qgAU cG/UIdaSa3mnBA68O2wCc0/bM4lMYjO4CS3TubUeaiVfCm2CJmDPXSeMB/OUqQE4 wOc4zWSBmQq1VELoiq1M9uY1Q5J7ZQxkhjIHmMCGpIdlQJeR36pUx3aKb/Z9gNB8 YZYiI2xjCLyjYA7sY2dpWCOFGDlEBB7aRPrWi/0kXUPtDcgI+Azj0dvuD8rcyJU7 Oi+LLMHs2mOfMMv1f5ay9WUsjzHhJEluFheu3hXyfRZ3CeC0/QpfJS8LG/HK1dvh KeMLoHrauuLjDIrFTMzESmLK8JhfK3L4WuaVW2FS6lob80+7pef0HupNeWGXjksD EjST1sGtfKYXnAhzX9fpHtnQzHTM8tMcs1UqSs1mmJ3sUIUWvd7ryxzETaenUlsP rRwYErIZqBzVr7mVfP6v3XciiPN31OyU4G1e1NAGH4OzceWDbUovbqJTmT2BUxri opfjAs7KGYl34si9zU4OHnz4zDi7EgHX/5Ce/Lgau8RuERlkwC42rT+JgsLvrIBz PGckHWgBBLWH0yD3qjPnFWjt9IRSkHz77RD7PZoJUusjM/1ZDnFqUJmZaxpKoRZC tLQXO137s4Pj+9Co15eS =aV5U -----END PGP SIGNATURE----- --J5MfuwkIyy7RmF4Q--