From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YFyqa-0001Tx-Bj for qemu-devel@nongnu.org; Tue, 27 Jan 2015 00:37:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YFyqU-0000PL-Fl for qemu-devel@nongnu.org; Tue, 27 Jan 2015 00:37:56 -0500 Date: Tue, 27 Jan 2015 16:37:31 +1100 From: David Gibson Message-ID: <20150127053731.GF28888@voom.redhat.com> References: <1419337831-16552-1-git-send-email-mdroth@linux.vnet.ibm.com> <1419337831-16552-17-git-send-email-mdroth@linux.vnet.ibm.com> <20150119055828.GZ5297@voom.fritz.box> <20150126211731.23721.52817@loki> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="7cm2iqirTL37Ot+N" Content-Disposition: inline In-Reply-To: <20150126211731.23721.52817@loki> Subject: Re: [Qemu-devel] [PATCH v4 16/17] spapr_pci: enable basic hotplug operations 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 --7cm2iqirTL37Ot+N Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 26, 2015 at 03:17:31PM -0600, Michael Roth wrote: > Quoting David Gibson (2015-01-18 23:58:28) > > On Tue, Dec 23, 2014 at 06:30:30AM -0600, Michael Roth wrote: [snip] > > > +/* create OF node for pci device and required OF DT properties */ > > > +static void *spapr_create_pci_child_dt(sPAPRPHBState *phb, PCIDevice= *dev, > > > + int drc_index, int *dt_offset) > > > +{ > > > + void *fdt_orig, *fdt; > > > + int offset, ret; > > > + int slot =3D PCI_SLOT(dev->devfn); > > > + char nodename[512]; > > > + > > > + fdt_orig =3D g_malloc0(FDT_MAX_SIZE); > > > + offset =3D fdt_create(fdt_orig, FDT_MAX_SIZE); > > > + fdt_begin_node(fdt_orig, ""); > > > + fdt_end_node(fdt_orig); > > > + fdt_finish(fdt_orig); > >=20 > > Recent versions of libfdt have an fdt_create_empty_tree() function to > > simplify that standard idiom. >=20 > Hmm, it doesn't seem to be in the source that qemu.git/dtc points to, so = I'm > hesitant to rely on it. Would it be viable to get the QEMU submodule > updated to v1.4.0? Ah, right. Yes, we should probably update the qemu submodule, but I don't think your patches should have to wait on that. > > > + fdt =3D g_malloc0(FDT_MAX_SIZE); > > > + fdt_open_into(fdt_orig, fdt, FDT_MAX_SIZE); > >=20 > > There's no need for a second malloc here - fdt_open_into() may be used > > in place. > >=20 > > > + sprintf(nodename, "pci@%d", slot); > > > + offset =3D fdt_add_subnode(fdt, 0, nodename); > > > + ret =3D spapr_populate_pci_child_dt(dev, fdt, offset, phb->index= , drc_index); > > > + g_assert(!ret); > > > + g_free(fdt_orig); > > > + > > > + *dt_offset =3D offset; > > > + return fdt; > > > +} > > > + > > > +static void spapr_device_hotplug_add(sPAPRDRConnector *drc, > > > + sPAPRPHBState *phb, > > > + PCIDevice *pdev) > > > +{ > > > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc= ); > > > + DeviceState *dev =3D DEVICE(pdev); > > > + int drc_index =3D drck->get_index(drc); > > > + void *fdt =3D NULL; > > > + int fdt_start_offset =3D 0; > > > + > > > + /* boot-time devices get their device tree node created by SLOF,= but for > > > + * hotplugged devices we need QEMU to generate it so the guest c= an fetch > > > + * it via RTAS > >=20 > > Now that we have to have this code in qemu for the hotplug case we may > > want to consider using it for boot-time devices as well, and removing > > the corresponding code from SLOF, but that's a problem for another day. >=20 > Makes sense, since we do this for PHBs already. Can look into it as > a follow-up. Ok, great. > > > + */ > > > + if (dev->hotplugged) { > > > + fdt =3D spapr_create_pci_child_dt(phb, pdev, drc_index, > > > + &fdt_start_offset); > > > + } > > > + drck->attach(drc, DEVICE(pdev), fdt, fdt_start_offset, !dev->hot= plugged); > > > +} > > > + > > > +static void spapr_device_hotplug_remove_cb(DeviceState *dev, void *o= paque) > > > +{ > > > + object_unparent(OBJECT(dev)); > > > +} > > > + > > > +static void spapr_device_hotplug_remove(sPAPRDRConnector *drc, > > > + sPAPRPHBState *phb, > > > + PCIDevice *pdev) > > > +{ > > > + sPAPRDRConnectorClass *drck =3D SPAPR_DR_CONNECTOR_GET_CLASS(drc= ); > > > + > > > + drck->detach(drc, DEVICE(pdev), spapr_device_hotplug_remove_cb, = phb); > > > +} > > > + > > > +static void spapr_phb_hot_plug(HotplugHandler *plug_handler, > > > + DeviceState *plugged_dev, Error **err= p) > >=20 > > So, this function is hotplugging a PCI device into an existing PHB, > > rather than hotplugging a PHB itself. Since the DR protocol does > > support both operations, I could see this name becoming confusing. > >=20 > > > +{ > > > + sPAPRPHBState *phb =3D SPAPR_PCI_HOST_BRIDGE(DEVICE(plug_handler= )); > > > + PCIDevice *pdev =3D PCI_DEVICE(plugged_dev); > > > + sPAPRDRConnector *drc =3D > > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_PCI, pdev->= devfn); > >=20 > > Is it safe to call this before checking phb->dr_enabled? >=20 > It will be NULL if the DRC wasn't created, so the assertion below the che= ck > should catch any misuse before it happens. Ok. --=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 --7cm2iqirTL37Ot+N Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUxyQbAAoJEGw4ysog2bOSzwMQAOO49Ns+Q9nyoawj5sgNZO23 ga5FVG0VgHQee7fzjUYX2o4QxIJfJyUSf+xVFNIHGWPu77SIaomzcizTG7E+R+OR MlBstqmISNzOY1sli3oPkiZ8DmkZcPO3LF2K4QRb7oeQQ30EvPxgyi3PzUB0CRU5 XPECnElxaQ8REAtgXdmms9+nKuZKNBDYtXgutGKK84YwIVRMdBLUz97N4mfHG1WF YXzvxKtTl5Sa95f3mQ3geibogE1UYYw+d7S1sDHUxJDUv0ydxXGz64S3v2N8s8ET vh2p8/qIGxu4/3NqVRCrM2QlmnIib+uZEyc9ookw3aLFuPHAtOSfm4ZKjomxD+WR 0ZRbfjXDXV9bH/re+Sqtvj3wkyYi5uIe5mxguUlr1xb0PRYnql5zjwdjmr67vWMm ojpA4OGZhIsGHwIsdjpN24xkR/D/BSaPBNBoKrIgg36rksJGD9v0vIc7yQFjouYx vxK2z839rkl3zUhg9UbYfmopeQccZKmdb4X2vMA+l4krBplq3DjXAdO9iG8zVYQa l54+mpjiH23/X9FWqmAtoDVXfwUSk/BsW1OmhYW29gwO10+0/uKGOOsnyQeM6T3P NHmdJP0hzfIBcWpkJgH0VhHvvjoEdKHusEgfVEeFR22CDjOYWx/3/qzE0Lpl9oFq 3TePnc9pkZnOJJlZM4U3 =LXP1 -----END PGP SIGNATURE----- --7cm2iqirTL37Ot+N--