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: 3571 bytes --]

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.

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

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