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

[Alexey Kardashevskiy]

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 <address@hidden>
>
> Reviewed-by: David Gibson <address@hidden>
>
> 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