From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aus4f-0003I5-W5 for qemu-devel@nongnu.org; Mon, 25 Apr 2016 21:46:03 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aus4c-0007fO-Ov for qemu-devel@nongnu.org; Mon, 25 Apr 2016 21:46:01 -0400 Date: Tue, 26 Apr 2016 11:18:25 +1000 From: David Gibson Message-ID: <20160426011825.GG15176@voom.fritz.box> References: <1461623065-6621-1-git-send-email-mdroth@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="lHGcFxmlz1yfXmOs" Content-Disposition: inline In-Reply-To: <1461623065-6621-1-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH for-2.6] spapr_drc: fix aborts during DRC-count based hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-devel@nongnu.org, Bharata B Rao , qemu-ppc@nongnu.org --lHGcFxmlz1yfXmOs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 25, 2016 at 05:24:25PM -0500, Michael Roth wrote: > CPU/memory resources can be signalled en-masse via > spapr_hotplug_req_add_by_count(), and when doing so, actually change > the meaning of the 'drc' parameter passed to > spapr_hotplug_req_event() to be a count rather than an index. >=20 > f40eb92 added a hook in spapr_hotplug_req_event() to record when a > device had been 'signalled' to the guest, but that code assumes that > drc is always an index. In cases where it's a count, such as memory > hotplug, the DRC lookup will fail, leading to an assert. >=20 > Fix this by only explicitly setting the signalled state for cases where > we are doing PCI hotplug. >=20 > For other resources types, since we cannot selectively track whether a > resource has been signalled in cases where we signal attach as a count, > set the 'signalled' state to true immediately upon making the > resource available via drck->attach(). >=20 > Reported-by: Bharata B Rao > Cc: Bharata B Rao > Cc: david@gibson.dropbear.id.au > Cc: qemu-ppc@nongnu.org > Signed-off-by: Michael Roth Applued to 2.6, I'll send a pull request today. > --- > Really sorry for the way last-minute fix, but without this memory hotplug > is totally broken :( Hoping to get this in for Wednesday's RC4, which > I think will be the final before release. > --- > hw/ppc/spapr_drc.c | 12 +++++++++++- > hw/ppc/spapr_events.c | 7 +++---- > 2 files changed, 14 insertions(+), 5 deletions(-) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 3173940..1f5f1d7 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -364,7 +364,17 @@ static void attach(sPAPRDRConnector *drc, DeviceStat= e *d, void *fdt, > drc->fdt =3D fdt; > drc->fdt_start_offset =3D fdt_start_offset; > drc->configured =3D coldplug; > - drc->signalled =3D coldplug; > + /* 'logical' DR resources such as memory/cpus are in some cases trea= ted > + * as a pool of resources from which the guest is free to choose from > + * based on only a count. for resources that can be assigned in this > + * fashion, we must assume the resource is signalled immediately > + * since a single hotplug request might make an arbitrary number of > + * such attached resources available to the guest, as opposed to > + * 'physical' DR resources such as PCI where each device/resource is > + * signalled individually. > + */ > + drc->signalled =3D (drc->type !=3D SPAPR_DR_CONNECTOR_TYPE_PCI) > + ? true : coldplug; > =20 > object_property_add_link(OBJECT(drc), "device", > object_get_typename(OBJECT(drc->dev)), > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 269ab7e..049fb1b 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -442,6 +442,9 @@ static void spapr_hotplug_req_event(uint8_t hp_id, ui= nt8_t hp_action, > switch (drc_type) { > case SPAPR_DR_CONNECTOR_TYPE_PCI: > hp->hotplug_type =3D RTAS_LOG_V6_HP_TYPE_PCI; > + if (hp->hotplug_action =3D=3D RTAS_LOG_V6_HP_ACTION_ADD) { > + spapr_hotplug_set_signalled(drc); > + } > break; > case SPAPR_DR_CONNECTOR_TYPE_LMB: > hp->hotplug_type =3D RTAS_LOG_V6_HP_TYPE_MEMORY; > @@ -462,10 +465,6 @@ static void spapr_hotplug_req_event(uint8_t hp_id, u= int8_t hp_action, > =20 > rtas_event_log_queue(RTAS_LOG_TYPE_HOTPLUG, new_hp, true); > =20 > - if (hp->hotplug_action =3D=3D RTAS_LOG_V6_HP_ACTION_ADD) { > - spapr_hotplug_set_signalled(drc); > - } > - > qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq)= ); > } > =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 --lHGcFxmlz1yfXmOs Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXHsHhAAoJEGw4ysog2bOSXZ8QAKHCd7lVyrgqFFjTzCQnvg0E xdgylf5gej1Z+koINcxYVQF+NHAg7ikKoc3B+8+0AuumES17+XEZ54Cc26BHZmKK RYB9q2+4tcmPScX2e7Re3+a4YO4Fp78xN6wbad0ax/MCDGh1SzsM0PO9hTMTFBL4 GniHD0Tphks+i0rNZFkYk24jIjF8vIvr30AXknAhzIMEXqYsSXCmR2EQ1M9r67d/ aqLaa6ZV1JXYvodNUFvUds634JUwZKUkIuMZSsQ4woIOEjdbbgglxYqldo3IItt0 pZbrFXI2h6dIy/xH7xh7DLvUWDxTW68k3XXWIHQiuN5AroW0cI2BJPX90lM//oXg dZnORC0G/wkVGRfS5n5Qnit13SBWZIxwu1eXIGy5dR0sS1YUzfgYse3NfHGDvZDc Vm3P9VliIPxx8jeL3ol/qtEuc+cnZovnw9cv2xTMDiGSVdulhsm/t1MKdBUnJEVa kkH48ODHjCmLUc2A+XN7DGkgnynDTXiLu4bJkUXMCbKaiJC24TZ8/w4nzg8g5bPr QrxqUQfIfJifv5tSmH09PEZvhkddpY5nWpk96x8dbV9SFu+9etV1P105FsI9+UNd uzkP0RVk5qUyHaZEl+vJDPTjQYbfxe97eSW4HBeQQb6f/PPAZgmAPnxKs7F17+Pv JAap0HPXqDFQ4+hP1aUv =XLjP -----END PGP SIGNATURE----- --lHGcFxmlz1yfXmOs--