From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiAhe-0005Vm-Jo for qemu-devel@nongnu.org; Mon, 21 Mar 2016 21:01:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiAhb-0008Cl-Co for qemu-devel@nongnu.org; Mon, 21 Mar 2016 21:01:46 -0400 Date: Tue, 22 Mar 2016 12:02:48 +1100 From: David Gibson Message-ID: <20160322010248.GT23586@voom.redhat.com> References: <1458546426-26222-1-git-send-email-aik@ozlabs.ru> <1458546426-26222-4-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="9hshNW4m6zn79FF/" Content-Disposition: inline In-Reply-To: <1458546426-26222-4-git-send-email-aik@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 --9hshNW4m6zn79FF/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > This adds a new helper to create a DMA window and makes use of it in > sPAPRPHBState::realize(). >=20 > 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(-) >=20 > 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 *s= phb, PCIDevice *pdev) > return buf; > } > =20 > +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, Err= or **errp) > int i; > PCIBus *bus; > uint64_t msi_window_size =3D 4096; > - sPAPRTCETable *tcet; > - uint32_t nb_table; > + Error *local_err =3D NULL; > =20 > if (sphb->index !=3D (uint32_t)-1) { > hwaddr windows_base; > @@ -1460,18 +1486,13 @@ static void spapr_phb_realize(DeviceState *dev, E= rror **errp) > } > } > =20 > - 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, false); > - 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_SH= IFT, > + 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 > sphb->msi =3D g_hash_table_new_full(g_int_hash, g_int_equal, g_free,= g_free); > } --=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 --9hshNW4m6zn79FF/ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW8Jm4AAoJEGw4ysog2bOSLMwQAIw4Z8wes4dgZFpOyFjmzod3 ZT2s3Cu2Oe3evKletn7FC6AgLXl3gXz02qZmF9LDPGOjZ8SAEjfeJHBAWZ2TDUeW FD7j/s9aXBioF3/7eamyO8YQgnDhELDOYcrvQjUSkV+DKzwdRASYJP75aliDub0/ gVJYWVz1Qg5S8rYZf7o2sEM0xeDBRzKuEpj5RJM9dBXExyrclSgS2RxRy5YYpsw7 D/J+BPMEpuqcYS0Ww9mPSmlNAWiMZcGPkELecDJIbLiakJ55ReoIN4Q+VfAb7FyX uA/exDzxXvt0yVaZV77tWXblQgEZY+TybZCLMWMC2MtSdKU3R1aLmiSMUkbKhS+W aD2m6e8tKxX5cAXTDrVuA7w9971aIy1IL0LVaWI0kAC5vbQWwg7yDSBJpkTKKUS0 oeCfD7MS51J0661nYE8vtZ6qBEPKKy7Q/b/xaJY8vgEJtMk29r4RibeHW3T2uzd0 cvXNVOtdOtB6ilsRkk+jRByUjRYD5v/K664JiurEXGI0kwEYTV+93UxVf6sD4nEO AX5I3Sn24HSXoXDR7AqEEdRTB9jlFjuCr/BtedeFQyQyYUIBo2SVA+f2T8J6KjMI ng8sHCEh1dzsPx+/dA9DadQEUM1B/Q7aEuHsXur5kEnAfYl0NV9FUpG+UbHuX37p nbh/gNQ4L9BePHL6UP/q =vR6j -----END PGP SIGNATURE----- --9hshNW4m6zn79FF/--