From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46966) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abf7a-0003Qw-1k for qemu-devel@nongnu.org; Thu, 03 Mar 2016 21:05:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abf7W-00047y-Nm for qemu-devel@nongnu.org; Thu, 03 Mar 2016 21:05:37 -0500 Date: Fri, 4 Mar 2016 12:18:09 +1100 From: David Gibson Message-ID: <20160304011809.GQ1620@voom.redhat.com> References: <1457042136-4255-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="OxDG9cJJSSQMUzGF" Content-Disposition: inline In-Reply-To: <1457042136-4255-1-git-send-email-mdroth@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH for-2.6] spapr_pci: fix multifunction hotplug List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth Cc: shivaprasadbhat@gmail.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --OxDG9cJJSSQMUzGF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 03, 2016 at 03:55:36PM -0600, Michael Roth wrote: > Since 3f1e147, QEMU has adopted a convention of supporting function > hotplug by deferring hotplug events until func 0 is hotplugged. > This is likely how management tools like libvirt would expose > such support going forward. >=20 > Since sPAPR guests rely on per-func events rather than > slot-based, our protocol has been to hotplug func 0 *first* to > avoid cases where devices appear within guests without func 0 > present to avoid undefined behavior. Hmm.. I would have thought PAPR guests would be able to cope with a non-zero function device being plugged on its own. >=20 > To remain compatible with new convention, defer hotplug in a > similar manner, but then generate events in 0-first order as we > did in the past. Once func 0 present, fail any attempts to plug > additional functions (as we do with PCIe). >=20 > For unplug, defer unplug operations in a similar manner, but > generate unplug events such that function 0 is removed last in guest. >=20 > Signed-off-by: Michael Roth > --- > Note: I'm not super-certain this is 2.6 material/soft-freeze material, > as the current implementation does "work" if one orders device_adds > in the manner enforced by this patch. The main reason I'm tagging as > 2.6 is to avoid a future compatibility issue if/when libvirt adds support > for multifunction hotplug in the manner suggested by 3f1e147. This does > however guard a bit better against user error. On balance, I think it is, since it does improve behaviour, rather than add functionality. I've added it to my ppc-for-2.6 branch. >=20 > hw/ppc/spapr_pci.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++= ++---- > 1 file changed, 86 insertions(+), 7 deletions(-) >=20 > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index e8edad3..ab6dece 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1142,14 +1142,21 @@ static void spapr_phb_remove_pci_device(sPAPRDRCo= nnector *drc, > drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb,= errp); > } > =20 > -static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb, > - PCIDevice *pdev) > +static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, > + uint32_t busnr, > + int32_t devfn) > { > - uint32_t busnr =3D pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pd= ev)))); > return spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI, > (phb->index << 16) | > (busnr << 8) | > - pdev->devfn); > + devfn); > +} > + > +static sPAPRDRConnector *spapr_phb_get_pci_drc(sPAPRPHBState *phb, > + PCIDevice *pdev) > +{ > + uint32_t busnr =3D pci_bus_num(PCI_BUS(qdev_get_parent_bus(DEVICE(pd= ev)))); > + return spapr_phb_get_pci_func_drc(phb, busnr, pdev->devfn); > } > =20 > static uint32_t spapr_phb_get_pci_drc_index(sPAPRPHBState *phb, > @@ -1173,6 +1180,8 @@ static void spapr_phb_hot_plug_child(HotplugHandler= *plug_handler, > PCIDevice *pdev =3D PCI_DEVICE(plugged_dev); > sPAPRDRConnector *drc =3D spapr_phb_get_pci_drc(phb, pdev); > Error *local_err =3D NULL; > + PCIBus *bus =3D PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))); > + uint32_t slotnr =3D PCI_SLOT(pdev->devfn); > =20 > /* if DR is disabled we don't need to do anything in the case of > * hotplug or coldplug callbacks > @@ -1190,13 +1199,44 @@ static void spapr_phb_hot_plug_child(HotplugHandl= er *plug_handler, > =20 > g_assert(drc); > =20 > + /* Following the QEMU convention used for PCIe multifunction > + * hotplug, we do not allow functions to be hotplugged to a > + * slot that already has function 0 present > + */ > + if (plugged_dev->hotplugged && bus->devices[PCI_DEVFN(slotnr, 0)] && > + PCI_FUNC(pdev->devfn) !=3D 0) { > + error_setg(errp, "PCI: slot %d function 0 already ocuppied by %s= ," > + " additional functions can no longer be exposed to gu= est.", > + slotnr, bus->devices[PCI_DEVFN(slotnr, 0)]->name); > + return; > + } > + > spapr_phb_add_pci_device(drc, phb, pdev, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > - if (plugged_dev->hotplugged) { > - spapr_hotplug_req_add_by_index(drc); > + > + /* If this is function 0, signal hotplug for all the device function= s. > + * Otherwise defer sending the hotplug event. > + */ > + if (plugged_dev->hotplugged && PCI_FUNC(pdev->devfn) =3D=3D 0) { > + int i; > + > + for (i =3D 0; i < 8; i++) { > + sPAPRDRConnector *func_drc; > + sPAPRDRConnectorClass *func_drck; > + sPAPRDREntitySense state; > + > + func_drc =3D spapr_phb_get_pci_func_drc(phb, pci_bus_num(bus= ), > + PCI_DEVFN(slotnr, i)); > + func_drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); > + func_drck->entity_sense(func_drc, &state); > + > + if (state =3D=3D SPAPR_DR_ENTITY_SENSE_PRESENT) { > + spapr_hotplug_req_add_by_index(func_drc); > + } > + } > } > } > =20 > @@ -1219,12 +1259,51 @@ static void spapr_phb_hot_unplug_child(HotplugHan= dler *plug_handler, > =20 > drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc); > if (!drck->release_pending(drc)) { > + PCIBus *bus =3D PCI_BUS(qdev_get_parent_bus(DEVICE(pdev))); > + uint32_t slotnr =3D PCI_SLOT(pdev->devfn); > + sPAPRDRConnector *func_drc; > + sPAPRDRConnectorClass *func_drck; > + sPAPRDREntitySense state; > + int i; > + > + /* ensure any other present functions are pending unplug */ > + if (PCI_FUNC(pdev->devfn) =3D=3D 0) { > + for (i =3D 1; i < 8; i++) { > + func_drc =3D spapr_phb_get_pci_func_drc(phb, pci_bus_num= (bus), > + PCI_DEVFN(slotnr, = i)); > + func_drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); > + func_drck->entity_sense(func_drc, &state); > + if (state =3D=3D SPAPR_DR_ENTITY_SENSE_PRESENT > + && !func_drck->release_pending(func_drc)) { > + error_setg(errp, > + "PCI: slot %d, function %d still present.= " > + "Must unplug all non-0 functions first.", > + slotnr, i); > + return; > + } > + } > + } > + > spapr_phb_remove_pci_device(drc, phb, pdev, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > } > - spapr_hotplug_req_remove_by_index(drc); > + > + /* if this isn't func 0, defer unplug event. otherwise signal re= moval > + * for all present functions > + */ > + if (PCI_FUNC(pdev->devfn) =3D=3D 0) { > + for (i =3D 7; i >=3D 0; i--) { > + func_drc =3D spapr_phb_get_pci_func_drc(phb, pci_bus_num= (bus), > + PCI_DEVFN(slotnr, = i)); > + func_drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(func_drc); > + func_drck->entity_sense(func_drc, &state); > + if (state =3D=3D SPAPR_DR_ENTITY_SENSE_PRESENT) { > + spapr_hotplug_req_remove_by_index(func_drc); > + } > + } > + } > } > } > =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 --OxDG9cJJSSQMUzGF Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW2OJRAAoJEGw4ysog2bOSPfkP/1ZFvwhQdP1GjtQAtmFougAm uxh6oGu8/EwjjlqExHp9dOQh2CGmMDbfNWuhcNtIFeM4Acho+1iAK7F4V7Pe3OhE tvfv7nQdA1Hh6vr2vCqbtjrJB2HkX58rQQMcZPdb+9XMkDi8iuAaYjcPM2pE8GsB birOok9f0SIInqQxjKFHr7EYi8w+fj+VSAnjlW98rYZETkyE/9U05iIyaSwKeDbs yqVQrd7RVyt59awfzodz/Nux9PN5F8Wam/DXxDzsxyFJTUeOwpGNaLq8pfc3PKvc PhcAY3v7OeDUtF0c0viA5Ju1T02Kh/lacTTdbkydzI0OJi8SQMEaGv/MUif+5Gad LK22oqpPzrsB4sohzUeaadserGFDDqmMk33rTkmPt9KlhBlounk1vDhJQT+L0rrt y2cvbd48+DNyF+yfgt4i1mZlUJVG+nrq4xdSDfllQWKhQYD29pX/fre6nLoxBqIv L9j9vsdpoXJlvK97QtvFY14cSmixP6CpmNshyc6vFpoi42xdK8VS+D57Fo9BTuPw nIFh+LPuSVEIgSNF1c1u66lT974ebxLSTKwOcJ+2hSzZJ1hTdeAeyu8pGukIB+hg k0TNo3AnI38PlDGje1DZYrNYMOvHaxUBVytycP7yLZPpdtZegznVV6IBDzDK92Cv M3xy99HLt6nemEuQl3qS =Y9fV -----END PGP SIGNATURE----- --OxDG9cJJSSQMUzGF--