Re: [Qemu-devel] [PATCH 11/17] spapr: convert TCE API to use an opaque type

[David Gibson]

> From: Paolo Bonzini <address@hidden>
> 
> The TCE table is currently returned as a DMAContext, and non-type-safe
> APIs are called later passing back the DMAContext.  Since we want to move
> away from DMAContext, use an opaque type instead, and add an accessor
> to retrieve the DMAContext from it.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>

Acked-by: David Gibson <address@hidden>

Using DMAContext * as the handle throughout these functions seemed
like a good idea at the time, but it was a mistake in hindsight.  I
might look at merging this in through our tree, since it doesn't
really depend on the rest of the iommu stuff.

> ---
>  hw/ppc/spapr_iommu.c        |   54 
> ++++++++++++++++++-------------------------
>  hw/ppc/spapr_pci.c          |    8 +++----
>  hw/ppc/spapr_vio.c          |   13 ++++++-----
>  include/hw/pci-host/spapr.h |    2 +-
>  include/hw/ppc/spapr.h      |   12 ++++++----
>  include/hw/ppc/spapr_vio.h  |    1 +
>  6 files changed, 42 insertions(+), 48 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index d2782cf..ab34a88 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -36,8 +36,6 @@ enum sPAPRTCEAccess {
>      SPAPR_TCE_RW = 3,
>  };
>  
> -typedef struct sPAPRTCETable sPAPRTCETable;
> -
>  struct sPAPRTCETable {
>      DMAContext dma;
>      uint32_t liobn;
> @@ -116,7 +114,7 @@ static int spapr_tce_translate(DMAContext *dma,
>      return 0;
>  }
>  
> -DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size)
> +sPAPRTCETable *spapr_tce_new_table(uint32_t liobn, size_t window_size)
>  {
>      sPAPRTCETable *tcet;
>  
> @@ -149,43 +147,40 @@ DMAContext *spapr_tce_new_dma_context(uint32_t liobn, 
> size_t window_size)
>      }
>  
>  #ifdef DEBUG_TCE
> -    fprintf(stderr, "spapr_iommu: New TCE table, liobn=0x%x, context @ %p, "
> -            "table @ %p, fd=%d\n", liobn, &tcet->dma, tcet->table, tcet->fd);
> +    fprintf(stderr, "spapr_iommu: New TCE table @ %p, liobn=0x%x, "
> +            "table @ %p, fd=%d\n", tcet, liobn, tcet->table, tcet->fd);
>  #endif
>  
>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>  
> -    return &tcet->dma;
> +    return tcet;
>  }
>  
> -void spapr_tce_free(DMAContext *dma)
> +void spapr_tce_free(sPAPRTCETable *tcet)
>  {
> +    QLIST_REMOVE(tcet, list);
>  
> -    if (dma) {

Strictly speaking, removing this test is an unrelated change, but it
is correct anyway.

> -        sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, dma);
> -
> -        QLIST_REMOVE(tcet, list);
> -
> -        if (!kvm_enabled() ||
> -            (kvmppc_remove_spapr_tce(tcet->table, tcet->fd,
> -                                     tcet->window_size) != 0)) {
> -            g_free(tcet->table);
> -        }
> -
> -        g_free(tcet);
> +    if (!kvm_enabled() ||
> +        (kvmppc_remove_spapr_tce(tcet->table, tcet->fd,
> +                                 tcet->window_size) != 0)) {
> +        g_free(tcet->table);
>      }
> +
> +    g_free(tcet);
>  }
>  
> -void spapr_tce_set_bypass(DMAContext *dma, bool bypass)
> +DMAContext *spapr_tce_get_dma(sPAPRTCETable *tcet)
>  {
> -    sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, dma);
> +    return &tcet->dma;
> +}
>  
> +void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass)
> +{
>      tcet->bypass = bypass;
>  }
>  
> -void spapr_tce_reset(DMAContext *dma)
> +void spapr_tce_reset(sPAPRTCETable *tcet)
>  {
> -    sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, dma);
>      size_t table_size = (tcet->window_size >> SPAPR_TCE_PAGE_SHIFT)
>          * sizeof(sPAPRTCE);
>  
> @@ -277,17 +272,12 @@ int spapr_dma_dt(void *fdt, int node_off, const char 
> *propname,
>  }
>  
>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
> -                      DMAContext *iommu)
> +                      sPAPRTCETable *tcet)
>  {
> -    if (!iommu) {
> +    if (!tcet) {
>          return 0;
>      }
>  
> -    if (iommu->translate == spapr_tce_translate) {
> -        sPAPRTCETable *tcet = DO_UPCAST(sPAPRTCETable, dma, iommu);
> -        return spapr_dma_dt(fdt, node_off, propname,
> -                tcet->liobn, 0, tcet->window_size);
> -    }
> -
> -    return -1;
> +    return spapr_dma_dt(fdt, node_off, propname,
> +                        tcet->liobn, 0, tcet->window_size);
>  }
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 62ff323..eb64a8f 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -511,7 +511,7 @@ static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, 
> void *opaque,
>  {
>      sPAPRPHBState *phb = opaque;
>  
> -    return phb->dma;
> +    return spapr_tce_get_dma(phb->tcet);
>  }
>  
>  static int spapr_phb_init(SysBusDevice *s)
> @@ -646,8 +646,8 @@ static int spapr_phb_init(SysBusDevice *s)
>  
>      sphb->dma_window_start = 0;
>      sphb->dma_window_size = 0x40000000;
> -    sphb->dma = spapr_tce_new_dma_context(sphb->dma_liobn, 
> sphb->dma_window_size);
> -    if (!sphb->dma) {
> +    sphb->tcet = spapr_tce_new_table(sphb->dma_liobn, sphb->dma_window_size);
> +    if (!sphb->tcet) {
>          fprintf(stderr, "Unable to create TCE table for %s\n", 
> sphb->dtbusname);
>          return -1;
>      }
> @@ -676,7 +676,7 @@ static void spapr_phb_reset(DeviceState *qdev)
>      sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
>  
>      /* Reset the IOMMU state */
> -    spapr_tce_reset(sphb->dma);
> +    spapr_tce_reset(sphb->tcet);
>  }
>  
>  static Property spapr_phb_properties[] = {
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 4dbc315..7544def 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -136,7 +136,7 @@ static int vio_make_devnode(VIOsPAPRDevice *dev,
>          }
>      }
>  
> -    ret = spapr_tcet_dma_dt(fdt, node_off, "ibm,my-dma-window", dev->dma);
> +    ret = spapr_tcet_dma_dt(fdt, node_off, "ibm,my-dma-window", dev->tcet);
>      if (ret < 0) {
>          return ret;
>      }
> @@ -310,8 +310,8 @@ int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq)
>  
>  static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev)
>  {
> -    if (dev->dma) {
> -        spapr_tce_reset(dev->dma);
> +    if (dev->tcet) {
> +        spapr_tce_reset(dev->tcet);
>      }
>      free_crq(dev);
>  }
> @@ -336,12 +336,12 @@ static void rtas_set_tce_bypass(sPAPREnvironment 
> *spapr, uint32_t token,
>          return;
>      }
>  
> -    if (!dev->dma) {
> +    if (!dev->tcet) {
>          rtas_st(rets, 0, -3);
>          return;
>      }
>  
> -    spapr_tce_set_bypass(dev->dma, !!enable);
> +    spapr_tce_set_bypass(dev->tcet, !!enable);
>  
>      rtas_st(rets, 0, 0);
>  }
> @@ -448,7 +448,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>  
>      if (pc->rtce_window_size) {
>          uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg;
> -        dev->dma = spapr_tce_new_dma_context(liobn, pc->rtce_window_size);
> +        dev->tcet = spapr_tce_new_table(liobn, pc->rtce_window_size);
> +        dev->dma = spapr_tce_get_dma(dev->tcet);
>      }
>  
>      return pc->init(dev);
> diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
> index b21080c..653dd40 100644
> --- a/include/hw/pci-host/spapr.h
> +++ b/include/hw/pci-host/spapr.h
> @@ -49,7 +49,7 @@ typedef struct sPAPRPHBState {
>      uint32_t dma_liobn;
>      uint64_t dma_window_start;
>      uint64_t dma_window_size;
> -    DMAContext *dma;
> +    sPAPRTCETable *tcet;
>  
>      struct {
>          uint32_t irq;
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 864bee9..e8d617b 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -342,17 +342,19 @@ typedef struct sPAPRTCE {
>  
>  #define RTAS_ERROR_LOG_MAX      2048
>  
> +typedef struct sPAPRTCETable sPAPRTCETable;
>  
>  void spapr_iommu_init(void);
>  void spapr_events_init(sPAPREnvironment *spapr);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
> -DMAContext *spapr_tce_new_dma_context(uint32_t liobn, size_t window_size);
> -void spapr_tce_free(DMAContext *dma);
> -void spapr_tce_reset(DMAContext *dma);
> -void spapr_tce_set_bypass(DMAContext *dma, bool bypass);
> +sPAPRTCETable *spapr_tce_new_table(uint32_t liobn, size_t window_size);
> +DMAContext *spapr_tce_get_dma(sPAPRTCETable *tcet);
> +void spapr_tce_free(sPAPRTCETable *tcet);
> +void spapr_tce_reset(sPAPRTCETable *tcet);
> +void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass);
>  int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>                   uint32_t liobn, uint64_t window, uint32_t size);
>  int spapr_tcet_dma_dt(void *fdt, int node_off, const char *propname,
> -                      DMAContext *dma);
> +                      sPAPRTCETable *tcet);
>  
>  #endif /* !defined (__HW_SPAPR_H__) */
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index f98ec0a..56f2821 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -63,6 +63,7 @@ struct VIOsPAPRDevice {
>      uint32_t irq;
>      target_ulong signal_state;
>      VIOsPAPR_CRQ crq;
> +    sPAPRTCETable *tcet;
>      DMAContext *dma;

It would be nice to remove this DMAContext field as well.

>  };
>  

-- 
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: Digital signature