From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59047) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiCp4-0003gk-7w for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:17:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aiCp0-0001f1-6P for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:17:34 -0400 Received: from mail-pf0-x241.google.com ([2607:f8b0:400e:c00::241]:36665) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aiCoz-0001ef-Qw for qemu-devel@nongnu.org; Mon, 21 Mar 2016 23:17:30 -0400 Received: by mail-pf0-x241.google.com with SMTP id q129so33860551pfb.3 for ; Mon, 21 Mar 2016 20:17:29 -0700 (PDT) References: <1458546426-26222-1-git-send-email-aik@ozlabs.ru> <1458546426-26222-4-git-send-email-aik@ozlabs.ru> <20160322010248.GT23586@voom.redhat.com> From: Alexey Kardashevskiy Message-ID: <56F0B944.1090908@ozlabs.ru> Date: Tue, 22 Mar 2016 14:17:24 +1100 MIME-Version: 1.0 In-Reply-To: <20160322010248.GT23586@voom.redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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: David Gibson Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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 = window_size >> page_shift; >> + >> + if (!nb_table) { >> + error_setg(errp, "Zero size table"); >> + return; >> + } >> + >> + tcet = 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, Error **errp) >> int i; >> PCIBus *bus; >> uint64_t msi_window_size = 4096; >> - sPAPRTCETable *tcet; >> - uint32_t nb_table; >> + Error *local_err = NULL; >> >> if (sphb->index != (uint32_t)-1) { >> hwaddr windows_base; >> @@ -1460,18 +1486,13 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> } >> } >> >> - nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT; >> - tcet = 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_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. 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). > >> + } >> >> sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> } > -- Alexey