Re: [Qemu-devel] [PATCH qemu v14 03/18] spapr_pci: Move DMA window enablement to a helper

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

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

On Tue, Mar 22, 2016 at 02:17:24PM +1100, Alexey Kardashevskiy wrote:
> 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 <aik@ozlabs.ru>
> >
> >Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> >
> >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).

Add the return for now, since the series will need a respin anyway.
If you have time it'd be great if you could do an unrealize() patch
that cleans up the existing failure paths, but that would be separate
from this series.

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