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 <a...@ozlabs.ru>
Reviewed-by: David Gibson <da...@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. > + } > > 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
signature.asc
Description: PGP signature