Re: [Qemu-devel] [PATCH 16/17] spapr_vio: take care of creating our own AddressSpace/DMAContext

[David Gibson]

On Wed, May 01, 2013 at 01:18:39PM +1000, David Gibson wrote:
> From: Paolo Bonzini <address@hidden>
> 
> Fetch the root region from the sPAPRTCETable, and use it to build
> an AddressSpace and DMAContext.
> 
> Now, everywhere we have a DMAContext we also have access to the
> corresponding AddressSpace (either because we create it just before
> the DMAContext, or because dma_context_memory's AddressSpace is
> trivially address_space_memory).
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/ppc/spapr_iommu.c       |   11 -----------
>  hw/ppc/spapr_vio.c         |    3 ++-
>  include/hw/ppc/spapr.h     |    1 -
>  include/hw/ppc/spapr_vio.h |   36 +++++++++++++++++++++++-------------
>  4 files changed, 25 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index e26f957..2bc45d8 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -37,10 +37,6 @@ enum sPAPRTCEAccess {
>  };
>  
>  struct sPAPRTCETable {
> -    /* temporary until everyone has its own AddressSpace */
> -    DMAContext dma;
> -    AddressSpace as;
> -
>      uint32_t liobn;
>      uint32_t window_size;
>      sPAPRTCE *table;
> @@ -148,8 +144,6 @@ sPAPRTCETable *spapr_tce_new_table(uint32_t liobn, size_t 
> window_size)
>      memory_region_init_iommu(&tcet->iommu, &spapr_iommu_ops,
>                               get_system_memory(),
>                               "iommu-spapr", INT64_MAX);
> -    address_space_init(&tcet->as, &tcet->iommu);
> -    dma_context_init(&tcet->dma, &tcet->as);
>  
>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>  
> @@ -169,11 +163,6 @@ void spapr_tce_free(sPAPRTCETable *tcet)
>      g_free(tcet);
>  }
>  
> -DMAContext *spapr_tce_get_dma(sPAPRTCETable *tcet)
> -{
> -    return &tcet->dma;
> -}
> -
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
>  {
>      return &tcet->iommu;
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 7544def..2bd86ad 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -449,7 +449,8 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>      if (pc->rtce_window_size) {
>          uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg;
>          dev->tcet = spapr_tce_new_table(liobn, pc->rtce_window_size);
> -        dev->dma = spapr_tce_get_dma(dev->tcet);
> +        address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet));
> +        dma_context_init(&dev->dma, &dev->as);
>      }
>  
>      return pc->init(dev);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 142abb7..a83720e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -348,7 +348,6 @@ void spapr_iommu_init(void);
>  void spapr_events_init(sPAPREnvironment *spapr);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>  sPAPRTCETable *spapr_tce_new_table(uint32_t liobn, size_t window_size);
> -DMAContext *spapr_tce_get_dma(sPAPRTCETable *tcet);
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>  void spapr_tce_free(sPAPRTCETable *tcet);
>  void spapr_tce_reset(sPAPRTCETable *tcet);
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 56f2821..b757f32 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -63,8 +63,9 @@ struct VIOsPAPRDevice {
>      uint32_t irq;
>      target_ulong signal_state;
>      VIOsPAPR_CRQ crq;
> +    AddressSpace as;
> +    DMAContext dma;
>      sPAPRTCETable *tcet;
> -    DMAContext *dma;
>  };
>  
>  #define DEFINE_SPAPR_PROPERTIES(type, field)           \
> @@ -92,35 +93,44 @@ static inline qemu_irq spapr_vio_qirq(VIOsPAPRDevice *dev)
>  static inline bool spapr_vio_dma_valid(VIOsPAPRDevice *dev, uint64_t taddr,
>                                         uint32_t size, DMADirection dir)
>  {
> -    return dma_memory_valid(dev->dma, taddr, size, dir);
> +    return dma_memory_valid(&dev->dma, taddr, size, dir);
>  }
>  
>  static inline int spapr_vio_dma_read(VIOsPAPRDevice *dev, uint64_t taddr,
>                                       void *buf, uint32_t size)
>  {
> -    return (dma_memory_read(dev->dma, taddr, buf, size) != 0) ?
> -        H_DEST_PARM : H_SUCCESS;
> +    if (!dma_memory_valid(&dev->dma, taddr, size, DMA_DIRECTION_TO_DEVICE)) {
> +        return H_DEST_PARM;
> +    }
> +    dma_memory_read(&dev->dma, taddr, buf, size);

Lack of atomicity makes me a little nervous there, although I guess
its ok since qemu is single-threaded.  It does mean we do the
translations twice, effectively, which isn't ideal, although emulated
devices are already so slow it probably doesn't matter.

> +    return H_SUCCESS;
>  }
>  
>  static inline int spapr_vio_dma_write(VIOsPAPRDevice *dev, uint64_t taddr,
>                                        const void *buf, uint32_t size)
>  {
> -    return (dma_memory_write(dev->dma, taddr, buf, size) != 0) ?
> -        H_DEST_PARM : H_SUCCESS;
> +    if (!dma_memory_valid(&dev->dma, taddr, size, 
> DMA_DIRECTION_FROM_DEVICE)) {
> +        return H_DEST_PARM;
> +    }
> +    dma_memory_write(&dev->dma, taddr, buf, size);
> +    return H_SUCCESS;
>  }
>  
>  static inline int spapr_vio_dma_set(VIOsPAPRDevice *dev, uint64_t taddr,
>                                      uint8_t c, uint32_t size)
>  {
> -    return (dma_memory_set(dev->dma, taddr, c, size) != 0) ?
> -        H_DEST_PARM : H_SUCCESS;
> +    if (!dma_memory_valid(&dev->dma, taddr, size, 
> DMA_DIRECTION_FROM_DEVICE)) {
> +        return H_DEST_PARM;
> +    }
> +    dma_memory_set(&dev->dma, taddr, c, size);
> +    return H_SUCCESS;
>  }
>  
> -#define vio_stb(_dev, _addr, _val) (stb_dma((_dev)->dma, (_addr), (_val)))
> -#define vio_sth(_dev, _addr, _val) (stw_be_dma((_dev)->dma, (_addr), (_val)))
> -#define vio_stl(_dev, _addr, _val) (stl_be_dma((_dev)->dma, (_addr), (_val)))
> -#define vio_stq(_dev, _addr, _val) (stq_be_dma((_dev)->dma, (_addr), (_val)))
> -#define vio_ldq(_dev, _addr) (ldq_be_dma((_dev)->dma, (_addr)))
> +#define vio_stb(_dev, _addr, _val) (stb_dma(&(_dev)->dma, (_addr), (_val)))
> +#define vio_sth(_dev, _addr, _val) (stw_be_dma(&(_dev)->dma, (_addr), 
> (_val)))
> +#define vio_stl(_dev, _addr, _val) (stl_be_dma(&(_dev)->dma, (_addr), 
> (_val)))
> +#define vio_stq(_dev, _addr, _val) (stq_be_dma(&(_dev)->dma, (_addr), 
> (_val)))
> +#define vio_ldq(_dev, _addr) (ldq_be_dma(&(_dev)->dma, (_addr)))
>  
>  int spapr_vio_send_crq(VIOsPAPRDevice *dev, uint8_t *crq);
>  

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