From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiDRt-0002vp-PF for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:57:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiDRp-0001VT-Ow for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:57:41 -0400 Date: Tue, 22 Mar 2016 14:28:20 +1100 From: David Gibson Message-ID: <20160322032820.GB23586@voom.redhat.com> References: <1458546426-26222-1-git-send-email-aik@ozlabs.ru> <1458546426-26222-4-git-send-email-aik@ozlabs.ru> <20160322010248.GT23586@voom.redhat.com> <56F0B944.1090908@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="YV7D9JCVdz8QpFLW" Content-Disposition: inline In-Reply-To: <56F0B944.1090908@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH qemu v14 03/18] spapr_pci: Move DMA window enablement to a helper List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --YV7D9JCVdz8QpFLW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 22, 2016 at 02:17:24PM +1100, Alexey Kardashevskiy wrote: > On 03/22/2016 12:02 PM, David Gibson wrote: > >On Mon, Mar 21, 2016 at 06:46:51PM +1100, Alexey Kardashevskiy wrote: > >>We are going to have multiple DMA windows soon so let's start preparing. > >> > >>This adds a new helper to create a DMA window and makes use of it in > >>sPAPRPHBState::realize(). > >> > >>Signed-off-by: Alexey Kardashevskiy > > > >Reviewed-by: David Gibson > > > >With one tweak.. > > > >>--- > >>Changes: > >>v14: > >>* replaced "int" return to Error* in spapr_phb_dma_window_enable() > >>--- > >> hw/ppc/spapr_pci.c | 47 ++++++++++++++++++++++++++++++++++-----------= -- > >> 1 file changed, 34 insertions(+), 13 deletions(-) > >> > >>diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > >>index 79baa7b..18332bf 100644 > >>--- a/hw/ppc/spapr_pci.c > >>+++ b/hw/ppc/spapr_pci.c > >>@@ -803,6 +803,33 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState = *sphb, PCIDevice *pdev) > >> return buf; > >> } > >> > >>+static void spapr_phb_dma_window_enable(sPAPRPHBState *sphb, > >>+ uint32_t liobn, > >>+ uint32_t page_shift, > >>+ uint64_t window_addr, > >>+ uint64_t window_size, > >>+ Error **errp) > >>+{ > >>+ sPAPRTCETable *tcet; > >>+ uint32_t nb_table =3D window_size >> page_shift; > >>+ > >>+ if (!nb_table) { > >>+ error_setg(errp, "Zero size table"); > >>+ return; > >>+ } > >>+ > >>+ tcet =3D spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, > >>+ page_shift, nb_table, false); > >>+ if (!tcet) { > >>+ error_setg(errp, "Unable to create TCE table liobn %x for %s", > >>+ liobn, sphb->dtbusname); > >>+ return; > >>+ } > >>+ > >>+ memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > >>+ spapr_tce_get_iommu(tcet)); > >>+} > >>+ > >> /* Macros to operate with address in OF binding to PCI */ > >> #define b_x(x, p, l) (((x) & ((1<<(l))-1)) << (p)) > >> #define b_n(x) b_x((x), 31, 1) /* 0 if relocatable */ > >>@@ -1307,8 +1334,7 @@ static void spapr_phb_realize(DeviceState *dev, E= rror **errp) > >> int i; > >> PCIBus *bus; > >> uint64_t msi_window_size =3D 4096; > >>- sPAPRTCETable *tcet; > >>- uint32_t nb_table; > >>+ Error *local_err =3D NULL; > >> > >> if (sphb->index !=3D (uint32_t)-1) { > >> hwaddr windows_base; > >>@@ -1460,18 +1486,13 @@ static void spapr_phb_realize(DeviceState *dev,= Error **errp) > >> } > >> } > >> > >>- nb_table =3D sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; > >>- tcet =3D spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn, > >>- 0, SPAPR_TCE_PAGE_SHIFT, nb_table, fals= e); > >>- if (!tcet) { > >>- error_setg(errp, "Unable to create TCE table for %s", > >>- sphb->dtbusname); > >>- return; > >>- } > >>- > >> /* Register default 32bit DMA window */ > >>- memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr, > >>- spapr_tce_get_iommu(tcet)); > >>+ spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_= SHIFT, > >>+ sphb->dma_win_addr, sphb->dma_win_size, > >>+ &local_err); > >>+ if (local_err) { > >>+ error_propagate(errp, local_err); > > > >Should be a return; here so we don't continue if there's an error. > > > >Actually.. that's not really right, we should be cleaning up all setup > >we've done already on the failure path. Without that I think we'll > >leak some objects on a failed device_add. > > > >But.. there are already a bunch of cases here that will do that, so we > >can clean that up separately. Probably the sanest way would be to add > >an unrealize function() that can handle a partially realized object > >and make sure it's called on all the error paths. >=20 >=20 > So what do I do right now with this patch? Leave it as is, add "return", > implement unrealize(), ...? In practice, being unable to create a PHB is a > fatal error today (as we do not have PHB hotplug yet and this is what > unrealize() is for). Add the return for now, since the series will need a respin anyway. If you have time it'd be great if you could do an unrealize() patch that cleans up the existing failure paths, but that would be separate =66rom this series. --=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 --YV7D9JCVdz8QpFLW Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8LvUAAoJEGw4ysog2bOSoC8QAMVjxbmN3HYjMK3exfbUGW5i OqIAKyRu38YnRmZOKnmrD/n6FgAQnp0z7MUvmVimUWaVFTmhThLmftk4TWm8ptN9 zWb/MnuEh4vofB0/O3usmGCLumHoqSypOGPQHdpkyPeePJZrZGMmLEBPWIPZ6noG o+D+4N+99i2W4F3UFiFPILiDKKdwg8XpQw5+SOX9SY2j/+e46DQrs66fFHQXmyBA CUpIQwjQFjaMwkjjM6Maecj74IsHTtYskkebUNioTWrV4VuqSPN+wNtEqFdWoXV2 stDhyfWdQhZ/+LWQ6JsZLeZ5xbqd6ZMWMk3iUztrzeGDNyf8ok9TQSQhiVI04Yet 3slQre7i/mdpxONyE3cSt0K19ynUNCgqEQi1McNUyV+q5OhL63OitHjIFoZeC1cs 6CiDkCrd6yl2TlNuA3CF6H/mIkstm5ES1uP1Q47GFQbxlcZoC9snOHn/iqgOBhxr T+D04hb+FcmhkFHKx4yzA35v/p6UPTD1BS8gUEX/beZN2TuLno+26glO5akSXnox /KuUVbmbwNw/bwao+Wz6ziYEclvcQoCF1rLkgs1A2f6EUbbSnHZlw4SdsOWq5Kz5 tPqXHSYYh0exoxChiywlDKvUTTFeQVgwzp8MrdfS9ZqgZu2c7G8S0WikdcoCCn7f R1G5kfJifveI7mrAIY37 =M+8r -----END PGP SIGNATURE----- --YV7D9JCVdz8QpFLW--