From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46115) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afiTy-0006m5-3u for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afiTw-0006Oe-P0 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 02:29:30 -0400 Date: Tue, 15 Mar 2016 16:30:48 +1100 From: David Gibson Message-ID: <20160315053048.GF15272@voom.fritz.box> References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-3-git-send-email-aik@ozlabs.ru> <20160303014019.GD1620@voom.redhat.com> <56E10A58.7020906@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Qz2CZ664xQdCRdPu" Content-Disposition: inline In-Reply-To: <56E10A58.7020906@ozlabs.ru> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] 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 --Qz2CZ664xQdCRdPu Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 10, 2016 at 04:47:04PM +1100, Alexey Kardashevskiy wrote: > On 03/03/2016 12:40 PM, David Gibson wrote: > >On Tue, Mar 01, 2016 at 08:10:27PM +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 > >>--- > >> hw/ppc/spapr_pci.c | 40 +++++++++++++++++++++++++++------------- > >> 1 file changed, 27 insertions(+), 13 deletions(-) > >> > >>diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > >>index 3d1145e..248f20a 100644 > >>--- a/hw/ppc/spapr_pci.c > >>+++ b/hw/ppc/spapr_pci.c > >>@@ -803,6 +803,29 @@ static char *spapr_phb_get_loc_code(sPAPRPHBState = *sphb, PCIDevice *pdev) > >> return buf; > >> } > >> > >>+static int spapr_phb_dma_window_enable(sPAPRPHBState *sphb, > >>+ uint32_t liobn, uint32_t page_s= hift, > >>+ uint64_t window_addr, > >>+ uint64_t window_size) > >>+{ > >>+ sPAPRTCETable *tcet; > >>+ uint32_t nb_table =3D window_size >> page_shift; > >>+ > >>+ if (!nb_table) { > >>+ return -1; > >>+ } > > > >The caller shouldn't do this, so this probably makes more sense as an > >assert() than an error return. >=20 >=20 > @dma_win_size is a PHB property so the cli can set it to zero - where is = it > supposed to fail? When DMA won't work? This will be not really obvious for > the user. Check dma_win_size=3D=3D0 where we check mem_win_addr/...? Ah.. good point. It could be checked in the caller, but it doesn't make a lot of sense to. > >>+ > >>+ tcet =3D spapr_tce_new_table(DEVICE(sphb), liobn, window_addr, > >>+ page_shift, nb_table, false); > >>+ if (!tcet) { > >>+ return -1; > >>+ } > > > >Since you're adding error reporting, you might as well make it via the > >error API instead of a return code. That way if we wasnt to add more > >detailed error API reporting to spapr_tce_new_table() in future, > >there's less to change. >=20 > Well, the table allocation is the only real thing which may fail there and > spapr_phb_realize() does not pass Error to the callers ?? I'm not sure what you mean by that. > so > spapr_phb_dma_window_enable() would be the first one to propagate an error > and it just seems a bit over engineered. Should I still do that? Yes. You can argue it's overengineered, but better we move towards overengineered in a consistent way, than continue to use a mishmash of error codes and the error api. >=20 >=20 > >>+ > >>+ memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, > >>+ spapr_tce_get_iommu(tcet)); > >>+ return 0; > >>+} > >>+ > >> /* 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 */ > >>@@ -1228,8 +1251,6 @@ 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; > >> > >> if (sphb->index !=3D (uint32_t)-1) { > >> hwaddr windows_base; > >>@@ -1381,18 +1402,11 @@ 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)); > >>+ if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_P= AGE_SHIFT, > >>+ sphb->dma_win_addr, sphb->dma_win_= size)) { > >>+ error_setg(errp, "Unable to create TCE table for %s", sphb->dt= busname); > >>+ } > >> > >> sphb->msi =3D g_hash_table_new_full(g_int_hash, g_int_equal, g_fr= ee, g_free); > >> } > > >=20 >=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 --Qz2CZ664xQdCRdPu Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW554IAAoJEGw4ysog2bOSJ5kP/0v5k22PXTIUPEKvyzBlMztl QuFyWf339jkjVzyYsyLLD5r3yrJm5YraP88iVzyxqTRSAB06f1UBKaCa0M9mk5jQ /WReUSO6uYCWAOwmwH/uY1jO8c25iujFLVUf9PpqFil9RXVfy5ayBFoGaLald1sT 6Qa2nLzcrWB9l9L7c4knLfjL9k8DhmaJmHtCCDIRnDsfIz3PwW8tV70byTNj0Vhq iMMSL1X+FLNwivuwD0vVhXb8JKoVQkycdOMAJAtms7XIwj0y5ccvkmSgzgBG1frW hRwAXzPJ2xbZ8xpv+xIQASDXTXLbMcab+vsu8uexfv/TyKsQlHUf7j3bvg44k3mQ 9eRAgP2z5/S2+HB2NukiHu1jRT7trFNVGqMePyJbZprX2+N9uwB1Pn0LKnvpfXsO h7E6eb1M7K+Xjl+TD4l3sOcQ9GLGsH9zuISqy7gf03UU+fEukGXW3AbBmNrUgHIY 9h77iU1wMYJ5eQMMwNmekLxQ3Q+FI598KI7FtXBFNTBFKCmLsOhN+jht7jiZKJl/ /UWRq3afevn9Q4TH10neWPVmKLja3Zp+buy9dtx2obqIk3ttHKaD9coN5vWlXk2E KZ1Lck55g4irTO2gX8ZbEd8NVmk1k8QJ4FeK2/Bn41aSdnhC+IHfzuykfKd5hUwF U9VBrCSOW2D4ZIj1YFOe =pzdn -----END PGP SIGNATURE----- --Qz2CZ664xQdCRdPu--