From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55233) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alp2q-0003Kc-6Y for qemu-devel@nongnu.org; Thu, 31 Mar 2016 22:42:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alp2o-0005D9-FN for qemu-devel@nongnu.org; Thu, 31 Mar 2016 22:42:44 -0400 Date: Fri, 1 Apr 2016 13:33:25 +1100 From: David Gibson Message-ID: <20160401023325.GK416@voom.redhat.com> References: <1459463257-3137-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="rwbb4r/vLufKlfJs" Content-Disposition: inline In-Reply-To: <1459463257-3137-1-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH for-2.6] spapr_drc: enable immediate detach for unsignalled devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: qemu-ppc@nongnu.org, sbhat@linux.vnet.ibm.com, qemu-devel@nongnu.org, bharata@linux.vnet.ibm.com --rwbb4r/vLufKlfJs Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 31, 2016 at 05:27:37PM -0500, Michael Roth wrote: > Currently spapr doesn't support "aborting" hotplug of PCI > devices by allowing device_del to immediately remove the > device if we haven't signalled the presence of the device > to the guest. >=20 > In the past this wasn't an issue, since we always immediately > signalled device attach and simply relied on full guest-aware > add->remove path for device removal. However, as of 788d259, > we now defer signalling for PCI functions until function 0 > is attached, so now we need to deal with these "abort" operations > for cases where a user hotplugs a non-0 function, then opts to > remove it prior hotplugging function 0. Currently they'd have to > reboot before the unplug completed. PCIe multifunction hotplug > does not have this requirement however, so from a management > implementation perspective it would be good to address this within > the same release as 788d259. >=20 > We accomplish this by simply adding a 'signalled' flag to track > whether a device hotplug event has been sent to the guest. If it > hasn't, we allow immediate removal under the assumption that the > guest will not be using the device. Devices present at boot/reset > time are also assumed to be 'signalled'. >=20 > For CPU/memory/etc, signalling will still happen immediately > as part of device_add, so only PCI functions should be affected. >=20 > Cc: bharata@linux.vnet.ibm.com > Cc: david@gibson.dropbear.id.au > Cc: sbhat@linux.vnet.ibm.com > Cc: qemu-ppc@nongnu.org > Signed-off-by: Michael Roth So, I'm disinclined to take this during the hard freeze, without some evidence that it fixes a problem case that's really being hit in practice. > --- > hw/ppc/spapr_drc.c | 34 ++++++++++++++++++++++++++++++++++ > hw/ppc/spapr_events.c | 11 +++++++++++ > include/hw/ppc/spapr_drc.h | 2 ++ > 3 files changed, 47 insertions(+) >=20 > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index ef063c0..5568e44 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -173,6 +173,12 @@ static void set_configured(sPAPRDRConnector *drc) > drc->configured =3D true; > } > =20 > +/* has the guest been notified of device attachment? */ > +static void set_signalled(sPAPRDRConnector *drc) > +{ > + drc->signalled =3D true; > +} > + > /* > * dr-entity-sense sensor value > * returned via get-sensor-state RTAS calls > @@ -355,6 +361,7 @@ static void attach(sPAPRDRConnector *drc, DeviceState= *d, void *fdt, > drc->fdt =3D fdt; > drc->fdt_start_offset =3D fdt_start_offset; > drc->configured =3D coldplug; > + drc->signalled =3D coldplug; > =20 > object_property_add_link(OBJECT(drc), "device", > object_get_typename(OBJECT(drc->dev)), > @@ -371,6 +378,26 @@ static void detach(sPAPRDRConnector *drc, DeviceStat= e *d, > drc->detach_cb =3D detach_cb; > drc->detach_cb_opaque =3D detach_cb_opaque; > =20 > + /* if we've signalled device presence to the guest, or if the guest > + * has gone ahead and configured the device (via manually-executed > + * device add via drmgr in guest, namely), we need to wait > + * for the guest to quiesce the device before completing detach. > + * Otherwise, we can assume the guest hasn't seen it and complete the > + * detach immediately. Note that there is a small race window > + * just before, or during, configuration, which is this context > + * refers mainly to fetching the device tree via RTAS. > + * During this window the device access will be arbitrated by > + * associated DRC, which will simply fail the RTAS calls as invalid. > + * This is recoverable within guest and current implementations of > + * drmgr should be able to cope. Sorry, I'm really not following this description of the race, or seeing how it relates to allowing removal of "half plugged" devices. > + */ > + if (!drc->signalled && !drc->configured) { > + /* if the guest hasn't seen the device we can't rely on it to > + * set it back to an isolated state via RTAS, so do it here manu= ally > + */ > + drc->isolation_state =3D SPAPR_DR_ISOLATION_STATE_ISOLATED; > + } > + > if (drc->isolation_state !=3D SPAPR_DR_ISOLATION_STATE_ISOLATED) { > DPRINTFN("awaiting transition to isolated state before removal"); > drc->awaiting_release =3D true; > @@ -409,6 +436,7 @@ static void reset(DeviceState *d) > { > sPAPRDRConnector *drc =3D SPAPR_DR_CONNECTOR(d); > sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + sPAPRDREntitySense state; > =20 > DPRINTFN("drc reset: %x", drck->get_index(drc)); > /* immediately upon reset we can safely assume DRCs whose devices > @@ -436,6 +464,11 @@ static void reset(DeviceState *d) > drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_UN= USABLE); > } > } > + > + drck->entity_sense(drc, &state); > + if (state =3D=3D SPAPR_DR_ENTITY_SENSE_PRESENT) { > + drck->set_signalled(drc); > + } > } > =20 > static void realize(DeviceState *d, Error **errp) > @@ -594,6 +627,7 @@ static void spapr_dr_connector_class_init(ObjectClass= *k, void *data) > drck->attach =3D attach; > drck->detach =3D detach; > drck->release_pending =3D release_pending; > + drck->set_signalled =3D set_signalled; > /* > * Reason: it crashes FIXME find and document the real reason > */ > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 39f4682..be8de63 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -387,6 +387,13 @@ static void spapr_powerdown_req(Notifier *n, void *o= paque) > qemu_irq_pulse(xics_get_qirq(spapr->icp, spapr->check_exception_irq)= ); > } > =20 > +static void spapr_hotplug_set_signalled(uint32_t drc_index) > +{ > + sPAPRDRConnector *drc =3D spapr_dr_connector_by_index(drc_index); > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > + drck->set_signalled(drc); > +} > + > static void spapr_hotplug_req_event(uint8_t hp_id, uint8_t hp_action, > sPAPRDRConnectorType drc_type, > uint32_t drc) > @@ -453,6 +460,10 @@ 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 > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h > index 7e56347..fa21ba0 100644 > --- a/include/hw/ppc/spapr_drc.h > +++ b/include/hw/ppc/spapr_drc.h > @@ -151,6 +151,7 @@ typedef struct sPAPRDRConnector { > bool configured; > =20 > bool awaiting_release; > + bool signalled; > =20 > /* device pointer, via link property */ > DeviceState *dev; > @@ -188,6 +189,7 @@ typedef struct sPAPRDRConnectorClass { > spapr_drc_detach_cb *detach_cb, > void *detach_cb_opaque, Error **errp); > bool (*release_pending)(sPAPRDRConnector *drc); > + void (*set_signalled)(sPAPRDRConnector *drc); > } sPAPRDRConnectorClass; > =20 > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, --=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 --rwbb4r/vLufKlfJs Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW/d30AAoJEGw4ysog2bOS6QcP/RkP+swTDEew/oVsmmIgQ4fP /JJOqgCQhO33v2xDNc5bgPgtzZSWOyS8XrKS/AqnG6XOIUDvk6Ru24O3W88Ep99W v21yMx5okw0jEmip1HHZ5cFQozsdQHrHiYGuOFSG2ghuAsmVcZiP5tJEEaPgIj+l W7+T0DMu4c0em4b7NwZKOOZegi8RiN6g4U8zvGVKuu5BIwI1PkjDlo9+wdSLg/Wi uGu08oKs9CoBwBHwyaZtkwCgiVshKNAXDQZwWDHx1dnis7lQv3sWvkuL7ZwwEWB2 lFyDMmj3XjTESWlvTAGeKDyp6hsFyq6ayKqGLrOQ7WgWSwP/XEzZT4K1USxqhHUH m0+XUR2r0JW2dpYg5eVb9HnzrchkBnPhhv0BChzZJX1hWE8ggLv4ETfdUWfQRHCQ IQRn4+5LjF4lOpSY2apVPELsKrTQ6zhcEdn//tqNcRyeGdoi6QHLVH8LuJr6B5kj YHWAP7S5kgZk0mvx8WLLMiO3Ab7lH0mO29EkbJ/RJBhdJFSLujrpCPyNbcl/bp7f HHfn/DK9fd9FSPJE81AyXc8xT/KrbxBoZl2ezP1XUd8dxa4tTY08gtaxxBpbR83Y QSswEGj4yx1hQj6hHefdjRpW2e0pmUVBaNlKHyOJd4I1HHCDAWqQks4/33DXfTLQ d1iZPyPSKPceMjOSVUD7 =+EGP -----END PGP SIGNATURE----- --rwbb4r/vLufKlfJs--