Re: [Qemu-devel] [Qemu-ppc] [PATCH qemu v13 02/16] spapr_pci: Move DMA window enablement to a helper

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4746 bytes --]

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 <aik@ozlabs.ru>
> >>---
> >>  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/...?

Ah.. good point.  It could be checked in the caller, but it doesn't
make a lot of sense to.

> >>+
> >>+    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

?? 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.


> 
> 
> >>+
> >>+    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);
> >>  }
> >
> 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]