Re: [Qemu-devel] [PATCH qemu v15 15/17] spapr_pci: Get rid of dma_loibn

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

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

s/dma_loibn/dma_liobn/ in subject line.

On Mon, Apr 04, 2016 at 07:33:44PM +1000, Alexey Kardashevskiy wrote:
> We are going to have 2 DMA windows which LIOBNs are calculated from
> the PHB index and the window number using the SPAPR_PCI_LIOBN macro
> so there is no actual use for dma_liobn.
> 
> This replaces dma_liobn with SPAPR_PCI_LIOBN. This marks it as unused
> in the migration stream. This renames dma_liobn to _dma_liobn as we have
> to keep the property for the CLI compatibility and we need a storage
> for it, although it has never really been used.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

This doesn't quite make sense.  We can't really do that without
entirely removing support for PHBs without an 'index' value.
Basically the idea of the PHB config parameters what that you either
specified just "index" or you specified *all* the relevant addresses.
Removing option 2 might be a reasonable idea, but it shouldn't just be
done as a side effect of this other change.  With this patch the
"specify everything" approach still has code, but can't work, because
such a device will never get a reasonable liobn (or worse, it might
get a duplicate liobn, because the index isn't verified in this mode).

Then again.. the "index" approach has also bitten us with the problem
of the not-quite-big-enough MMIO space per-PHB, so I'm not entirely
sure that making it the only choice is the right way to go either.

The short term approach to handle DDW might be to instead add a
dma64_liobn property.

> ---
> Changes:
> v15:
> * new to the series
> ---
>  hw/ppc/spapr_pci.c          | 17 ++++++-----------
>  include/hw/pci-host/spapr.h |  2 +-
>  2 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index f864fde..d4bdb27 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1306,7 +1306,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      if (sphb->index != (uint32_t)-1) {
>          hwaddr windows_base;
>  
> -        if ((sphb->buid != (uint64_t)-1) || (sphb->dma_liobn != (uint32_t)-1)
> +        if ((sphb->buid != (uint64_t)-1)
>              || (sphb->mem_win_addr != (hwaddr)-1)
>              || (sphb->io_win_addr != (hwaddr)-1)) {
>              error_setg(errp, "Either \"index\" or other parameters must"
> @@ -1321,7 +1321,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          }
>  
>          sphb->buid = SPAPR_PCI_BASE_BUID + sphb->index;
> -        sphb->dma_liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
>  
>          windows_base = SPAPR_PCI_WINDOW_BASE
>              + sphb->index * SPAPR_PCI_WINDOW_SPACING;
> @@ -1334,11 +1333,6 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    if (sphb->dma_liobn == (uint32_t)-1) {
> -        error_setg(errp, "LIOBN not specified for PHB");
> -        return;
> -    }
> -
>      if (sphb->mem_win_addr == (hwaddr)-1) {
>          error_setg(errp, "Memory window address not specified for PHB");
>          return;
> @@ -1453,7 +1447,7 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          }
>      }
>  
> -    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
> +    tcet = spapr_tce_new_table(DEVICE(sphb), SPAPR_PCI_LIOBN(sphb->index, 0));
>      if (!tcet) {
>          error_setg(errp, "Unable to create TCE table for %s",
>                     sphb->dtbusname);
> @@ -1479,7 +1473,8 @@ static int spapr_phb_children_reset(Object *child, void *opaque)
>  
>  void spapr_phb_dma_reset(sPAPRPHBState *sphb)
>  {
> -    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(sphb->dma_liobn);
> +    uint32_t liobn = SPAPR_PCI_LIOBN(sphb->index, 0);
> +    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>  
>      if (tcet && tcet->enabled) {
>          spapr_tce_table_disable(tcet);
> @@ -1507,7 +1502,7 @@ static void spapr_phb_reset(DeviceState *qdev)
>  static Property spapr_phb_properties[] = {
>      DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
> -    DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
> +    DEFINE_PROP_UINT32("liobn", sPAPRPHBState, _dma_liobn, -1),
>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
>      DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
>                         SPAPR_PCI_MMIO_WIN_SIZE),
> @@ -1595,7 +1590,7 @@ static const VMStateDescription vmstate_spapr_pci = {
>      .post_load = spapr_pci_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
> -        VMSTATE_UINT32_EQUAL(dma_liobn, sPAPRPHBState),
> +        VMSTATE_UNUSED(4), /* former dma_liobn */
>          VMSTATE_UINT64_EQUAL(mem_win_addr, sPAPRPHBState),
>          VMSTATE_UINT64_EQUAL(mem_win_size, sPAPRPHBState),
>          VMSTATE_UINT64_EQUAL(io_win_addr, sPAPRPHBState),
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index 7848366..3fca1c3 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -56,7 +56,7 @@ struct sPAPRPHBState {
>      hwaddr mem_win_addr, mem_win_size, io_win_addr, io_win_size;
>      MemoryRegion memwindow, iowindow, msiwindow;
>  
> -    uint32_t dma_liobn;
> +    uint32_t _dma_liobn;
>      hwaddr dma_win_addr, dma_win_size;
>      AddressSpace iommu_as;
>      MemoryRegion iommu_root;

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