From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58180) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adtRL-0008SR-Mz for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:47:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adtRI-0008BX-He for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:47:15 -0500 Received: from mail-pa0-x244.google.com ([2607:f8b0:400e:c03::244]:33919) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adtRI-0008B6-73 for qemu-devel@nongnu.org; Thu, 10 Mar 2016 00:47:12 -0500 Received: by mail-pa0-x244.google.com with SMTP id hj7so4830251pac.1 for ; Wed, 09 Mar 2016 21:47:12 -0800 (PST) References: <1456823441-46757-1-git-send-email-aik@ozlabs.ru> <1456823441-46757-3-git-send-email-aik@ozlabs.ru> <20160303014019.GD1620@voom.redhat.com> From: Alexey Kardashevskiy Message-ID: <56E10A58.7020906@ozlabs.ru> Date: Thu, 10 Mar 2016 16:47:04 +1100 MIME-Version: 1.0 In-Reply-To: <20160303014019.GD1620@voom.redhat.com> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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: David Gibson Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org 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_shift, >> + uint64_t window_addr, >> + uint64_t window_size) >> +{ >> + sPAPRTCETable *tcet; >> + uint32_t nb_table = 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. @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==0 where we check mem_win_addr/...? > >> + >> + tcet = 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. Well, the table allocation is the only real thing which may fail there and spapr_phb_realize() does not pass Error to the callers 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? >> + >> + 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, Error **errp) >> int i; >> PCIBus *bus; >> uint64_t msi_window_size = 4096; >> - sPAPRTCETable *tcet; >> - uint32_t nb_table; >> >> if (sphb->index != (uint32_t)-1) { >> hwaddr windows_base; >> @@ -1381,18 +1402,11 @@ 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)); >> + if (spapr_phb_dma_window_enable(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT, >> + sphb->dma_win_addr, sphb->dma_win_size)) { >> + error_setg(errp, "Unable to create TCE table for %s", sphb->dtbusname); >> + } >> >> sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free); >> } > -- Alexey