Re: [Qemu-devel] [PATCH 07/17] memory: add address_space_valid

Re: [Qemu-devel] [PATCH 07/17] memory: add address_space_valid

[David Gibson]

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

Hi Paolo,

I've been looking through your iommu branch, and spotted a few things,
so sending some comments about them.  Since I haven't see where
they've been posted before (if anywhere), I've kind of reconstructed a
mail to reply to from the patch in git.  I hope the result isn't too
cryptic.

On Wed, May 01, 2013 at 01:18:30PM +1000, David Gibson wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Checking whether an address space is possible in the old-style
> IOMMU implementation, but there is no equivalent in the memory API.
> Implement it with a lookup of the dispatch tree.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
[snip]
> ---
>  dma-helpers.c         |    5 +++++
>  exec.c                |   24 ++++++++++++++++++++++++
>  include/exec/memory.h |   12 ++++++++++++
>  include/sysemu/dma.h  |    3 ++-
>  4 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/dma-helpers.c b/dma-helpers.c
> index 272632f..2962b69 100644
> --- a/dma-helpers.c
> +++ b/dma-helpers.c
> @@ -298,6 +298,11 @@ bool iommu_dma_memory_valid(DMAContext *dma, dma_addr_t addr, dma_addr_t len,
>              plen = len;
>          }
>  
> +        if (!address_space_valid(dma->as, paddr, len,
> +                                 dir == DMA_DIRECTION_FROM_DEVICE)) {
> +            return false;
> +        }

I think the argument to address_space_valid() should be plen, not
len.  Otherwise we're checking that a range the size of the original
area is valid at the target address of just the first page.  Of
course, since the target address space will usually be falt and much
bigger than a page, it's unlikely to trigger.
-- 
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: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 07/17] memory: add address_space_valid

[Paolo Bonzini]

Il 01/05/2013 06:02, David Gibson ha scritto:
> Hi Paolo,
> 
> I've been looking through your iommu branch, and spotted a few
> things, so sending some comments about them.  Since I haven't see
> where they've been posted before (if anywhere), I've kind of
> reconstructed a mail to reply to from the patch in git.  I hope the
> result isn't too cryptic.

It's understandable, and very much appreciated! :)

Paolo

> On Wed, May 01, 2013 at 01:18:30PM +1000, David Gibson wrote:
>> From: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> Checking whether an address space is possible in the old-style 
>> IOMMU implementation, but there is no equivalent in the memory
>> API. Implement it with a lookup of the dispatch tree.
>> 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> [snip]
>> --- dma-helpers.c         |    5 +++++ exec.c                |
>> 24 ++++++++++++++++++++++++ include/exec/memory.h |   12
>> ++++++++++++ include/sysemu/dma.h  |    3 ++- 4 files changed, 43
>> insertions(+), 1 deletion(-)
>> 
>> diff --git a/dma-helpers.c b/dma-helpers.c index 272632f..2962b69
>> 100644 --- a/dma-helpers.c +++ b/dma-helpers.c @@ -298,6 +298,11
>> @@ bool iommu_dma_memory_valid(DMAContext *dma, dma_addr_t addr,
>> dma_addr_t len, plen = len; }
>> 
>> +        if (!address_space_valid(dma->as, paddr, len, +
>> dir == DMA_DIRECTION_FROM_DEVICE)) { +            return false; +
>> }
> 
> I think the argument to address_space_valid() should be plen, not 
> len.  Otherwise we're checking that a range the size of the
> original area is valid at the target address of just the first
> page.  Of course, since the target address space will usually be
> falt and much bigger than a page, it's unlikely to trigger.
> 

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

[David Gibson]

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

> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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 <pbonzini@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 13/17] spapr: use memory core for iommu support

[David Gibson]

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

On Wed, May 01, 2013 at 01:18:36PM +1000, David Gibson wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Now we can stop using a "translating" DMAContext, but we do not yet modify
> the sPAPRTCETable users to get an AddressSpace; they keep using the table
> via a DMAContext.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 14/17] dma: eliminate old-style IOMMU support

[David Gibson]

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

On Wed, May 01, 2013 at 01:18:37PM +1000, David Gibson wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> The translate function in the DMAContext is now always NULL.
> Remove every reference to it.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

-- 
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: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 15/17] pci: use memory core for iommu support

[David Gibson]

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

> From: Avi Kivity <avi.kivity@gmail.com>
> 
> Use the new iommu support in the memory core for iommu support.  The only
> user, spapr, is also converted, but it still provides a DMAContext
> interface until the non-PCI bits switch to AddressSpace.
> 
> Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c             |   58 ++++++++++++++++++++++++++++------------------
>  hw/ppc/spapr_pci.c       |   12 ++++++----
>  include/hw/pci/pci.h     |    7 ++++--
>  include/hw/pci/pci_bus.h |    5 ++--
>  4 files changed, 51 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 16ed118..799aa26 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -279,6 +279,21 @@ int pci_find_domain(const PCIBus *bus)
>      return -1;
>  }
>  
> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
> +{
> +    MemoryRegion *mr = g_new(MemoryRegion, 1);
> +
> +    /* FIXME: inherit memory region from bus creator */
> +    memory_region_init_alias(mr, "iommu-nop", get_system_memory(), 0, INT64_MAX);
> +    return mr;

I don't see the reason for creating a new alias for each PCI device.
Can't pci_dev->iommu just point directly to get_system_memory() in the
normal case?

In addition to creating additional objects, having these aliases makes
it much les obvious how to tell if two PCI devices share an IOMMU
address space.  We have to be able to determine that for VFIO, since
devices which share an address space in the host clearly can't be
assigned to different address spaces in the guest.

> +}
> +
> +static void pci_default_iommu_dtor(MemoryRegion *mr)
> +{
> +    memory_region_destroy(mr);
> +    g_free(mr);
> +}
> +
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                           const char *name,
>                           MemoryRegion *address_space_mem,
> @@ -289,6 +304,7 @@ static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>      bus->devfn_min = devfn_min;
>      bus->address_space_mem = address_space_mem;
>      bus->address_space_io = address_space_io;
> +    pci_setup_iommu(bus, pci_default_iommu, pci_default_iommu_dtor, NULL);
>  
>      /* host bridge */
>      QLIST_INIT(&bus->child);
> @@ -801,21 +817,15 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
>                       PCI_SLOT(devfn), PCI_FUNC(devfn), name, bus->devices[devfn]->name);
>          return NULL;
>      }
> +
>      pci_dev->bus = bus;
> -    if (bus->dma_context_fn) {
> -        pci_dev->dma = bus->dma_context_fn(bus, bus->dma_context_opaque, devfn);
> -    } else {
> -        /* FIXME: Make dma_context_fn use MemoryRegions instead, so this path is
> -         * taken unconditionally */
> -        /* FIXME: inherit memory region from bus creator */
> -        memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
> -                                 get_system_memory(), 0,
> -                                 memory_region_size(get_system_memory()));
> -        memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
> -        address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
> -        pci_dev->dma = g_new(DMAContext, 1);
> -        dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
> -    }
> +    pci_dev->iommu = bus->iommu_fn(bus, bus->iommu_opaque, devfn);
> +    memory_region_init_alias(&pci_dev->bus_master_enable_region, "bus master",
> +                             pci_dev->iommu, 0, memory_region_size(pci_dev->iommu));
> +    memory_region_set_enabled(&pci_dev->bus_master_enable_region, false);
> +    address_space_init(&pci_dev->bus_master_as, &pci_dev->bus_master_enable_region);
> +    pci_dev->dma = g_new(DMAContext, 1);
> +    dma_context_init(pci_dev->dma, &pci_dev->bus_master_as);
>  
>      pci_dev->devfn = devfn;
>      pstrcpy(pci_dev->name, sizeof(pci_dev->name), name);
> @@ -870,12 +880,12 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
>      pci_dev->bus->devices[pci_dev->devfn] = NULL;
>      pci_config_free(pci_dev);
>  
> -    if (!pci_dev->bus->dma_context_fn) {
> -        address_space_destroy(&pci_dev->bus_master_as);
> -        memory_region_destroy(&pci_dev->bus_master_enable_region);
> -        g_free(pci_dev->dma);
> -        pci_dev->dma = NULL;
> -    }
> +    address_space_destroy(&pci_dev->bus_master_as);
> +    memory_region_del_subregion(&pci_dev->bus_master_enable_region, pci_dev->iommu);
> +    pci_dev->bus->iommu_dtor_fn(pci_dev->iommu);
> +    memory_region_destroy(&pci_dev->bus_master_enable_region);

This looks like the wrong order: pci_dev->bus_master_enable_region is
an alias to the region in pci_dev->iommu, which has just been
destroyed by the iommu_dtor_fn() call.

> +    g_free(pci_dev->dma);
> +    pci_dev->dma = NULL;
>  }
>  
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
> @@ -2234,10 +2244,12 @@ static void pci_device_class_init(ObjectClass *klass, void *data)
>      k->props = pci_props;
>  }
>  
> -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque)
> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc dtor,
> +                     void *opaque)
>  {
> -    bus->dma_context_fn = fn;
> -    bus->dma_context_opaque = opaque;
> +    bus->iommu_fn = fn;
> +    bus->iommu_dtor_fn = dtor;
> +    bus->iommu_opaque = opaque;
>  }
>  
>  static const TypeInfo pci_device_type_info = {
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index eb64a8f..ffbb45e 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -506,12 +506,16 @@ static const MemoryRegionOps spapr_msi_ops = {
>  /*
>   * PHB PCI device
>   */
> -static DMAContext *spapr_pci_dma_context_fn(PCIBus *bus, void *opaque,
> -                                            int devfn)
> +static MemoryRegion *spapr_pci_dma_iommu_new(PCIBus *bus, void *opaque, int devfn)
>  {
>      sPAPRPHBState *phb = opaque;
>  
> -    return spapr_tce_get_dma(phb->tcet);
> +    return spapr_tce_get_iommu(phb->tcet);
> +}
> +
> +static void spapr_pci_dma_iommu_destroy(MemoryRegion *iommu)
> +{
> +    /* iommu is shared among devices, do nothing */
>  }
>  
>  static int spapr_phb_init(SysBusDevice *s)
> @@ -651,7 +655,7 @@ static int spapr_phb_init(SysBusDevice *s)
>          fprintf(stderr, "Unable to create TCE table for %s\n", sphb->dtbusname);
>          return -1;
>      }
> -    pci_setup_iommu(bus, spapr_pci_dma_context_fn, sphb);
> +    pci_setup_iommu(bus, spapr_pci_dma_iommu_new, spapr_pci_dma_iommu_destroy, sphb);
>  
>      QLIST_INSERT_HEAD(&spapr->phbs, sphb, list);
>  
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 8d075ab..7e7b0f4 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -242,6 +242,7 @@ struct PCIDevice {
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>      AddressSpace bus_master_as;
>      MemoryRegion bus_master_enable_region;
> +    MemoryRegion *iommu;
>      DMAContext *dma;
>  
>      /* do not access the following fields */
> @@ -401,9 +402,11 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp,
>  
>  void pci_device_deassert_intx(PCIDevice *dev);
>  
> -typedef DMAContext *(*PCIDMAContextFunc)(PCIBus *, void *, int);
> +typedef MemoryRegion *(*PCIIOMMUFunc)(PCIBus *, void *, int);
> +typedef void (*PCIIOMMUDestructorFunc)(MemoryRegion *mr);
>  
> -void pci_setup_iommu(PCIBus *bus, PCIDMAContextFunc fn, void *opaque);
> +void pci_setup_iommu(PCIBus *bus, PCIIOMMUFunc fn, PCIIOMMUDestructorFunc dtor,
> +                     void *opaque);
>  
>  static inline void
>  pci_set_byte(uint8_t *config, uint8_t val)
> diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> index 6ee443c..fada8f5 100644
> --- a/include/hw/pci/pci_bus.h
> +++ b/include/hw/pci/pci_bus.h
> @@ -10,8 +10,9 @@
>  
>  struct PCIBus {
>      BusState qbus;
> -    PCIDMAContextFunc dma_context_fn;
> -    void *dma_context_opaque;
> +    PCIIOMMUFunc iommu_fn;
> +    PCIIOMMUDestructorFunc iommu_dtor_fn;
> +    void *iommu_opaque;
>      uint8_t devfn_min;
>      pci_set_irq_fn set_irq;
>      pci_map_irq_fn map_irq;

-- 
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: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Qemu-devel] [PATCH 15/17] pci: use memory core for iommu support

[Paolo Bonzini]

Il 01/05/2013 07:06, David Gibson ha scritto:
>>> + +    /* FIXME: inherit memory region from bus creator */ +
>>> memory_region_init_alias(mr, "iommu-nop", get_system_memory(),
>>> 0, INT64_MAX); +    return mr;
> I don't see the reason for creating a new alias for each PCI
> device. Can't pci_dev->iommu just point directly to
> get_system_memory() in the normal case?
> 
> In addition to creating additional objects, having these aliases
> makes it much les obvious how to tell if two PCI devices share an
> IOMMU address space.  We have to be able to determine that for
> VFIO, since devices which share an address space in the host
> clearly can't be assigned to different address spaces in the
> guest.

Right, the alias that is needed to enable/disable bus-mastering is
created already below, so pci_dev->iommu can be shared (as it is in the
spapr case).

Paolo

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

[David Gibson]

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

On Wed, May 01, 2013 at 01:18:39PM +1000, David Gibson wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> 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 <pbonzini@redhat.com>
> ---
>  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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

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

[Paolo Bonzini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Il 01/05/2013 07:16, David Gibson ha scritto:
> Lack of atomicity makes me a little nervous there, although I
> guess its ok since qemu is single-threaded.

Yes.  The original plan was to add a boolean return value to
address_space_rw, but I left this for later since I wasn't sure of the
semantics for multipage writes.  What happens if the second half of
the destination buffer has an invalid translation?  Right now it's
atomic, but it sounds weird for real hardware.

Paolo

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


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRgT4xAAoJEBvWZb6bTYbyePoP/3BC7W/3LC/YfEJ16CbSsnhN
tJ2kLdjuI9+OAzAfb++TKoxkAAGtPSCFpBiS7s1T2r9gTSYMQX15enppU+lBFxwq
qfK7UF11carjnsDB3rXhUnZTGYPWYwA17mR4U2PBFy4EeZZ1p57m1QmJsrgI9UI7
u7QyC+NrNPM3fA0R/CCSn5ld3hh14GK3gEv+SK9IIsnwmoyQ6NKsDi3AAzdnSbMZ
pnMpFCb2Vu2MT/8M80r7J3aRUMGsVwlQgVyqxDpWJ4YJSDO6SaoIj7U+0gv867sa
gJwg94N6umurU49esGGjOzvTxlwdgQQDkG/sDUeMPsGS/cqb4RYeJKRK6ieFqdTY
SPUOz0ssUQhqzRX9zoUQE8aA0rt/CLL+E9+8MYmqAkKMLPpMmLsqrZxl11d19MTN
WdZnYmJvVLAVn+YUae4N0ehhgXSMv2MTabb39V7VG2QziEdcIn8wQIWGTTTw6o/p
GBFu0P05mXL8jvSQ5CQp3k36OqwKGlgjaHB4MOsbCKTahLBQ9VoarKB6Xh/4xWxj
3+kYjYdydnJMogOe4FE5mld6LJnlo66e/GJNdZyHzsdyB9cK9qMg+i3yXQElNSp9
H34sMKfpFgJ/6z/cwbrNrSWesgKDTqhhXKzocgvZkQUbvJCF5I2xigi8aKB3vC+N
wjEs5RUYBzb3ARKWCwy/
=Jr5p
-----END PGP SIGNATURE-----

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

[David Gibson]

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

On Wed, May 01, 2013 at 06:09:21PM +0200, Paolo Bonzini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Il 01/05/2013 07:16, David Gibson ha scritto:
> > Lack of atomicity makes me a little nervous there, although I
> > guess its ok since qemu is single-threaded.
> 
> Yes.  The original plan was to add a boolean return value to
> address_space_rw, but I left this for later since I wasn't sure of the
> semantics for multipage writes.  What happens if the second half of
> the destination buffer has an invalid translation?  Right now it's
> atomic, but it sounds weird for real hardware.

So, in this regard I don't think real hardware would be atomic.  It
would write a certain amount, then generate some sort of bus error
when it hits the bad translation.  So in general I expect the (guest)
OS would need to treat the target of in-flight device to host DMAs as
having undefined contents if there's a bus error like that.  It would
depend on bus and possibly individual device conventions what it could
assume about which DMAs are interrupted, and which might still be
in-flight.

That is, in this sense, we don't expect the hardware to behave
atomically at all.  The atomicity I was concerned about was atomicity
of checking permissions and returning an error based on that check.

So, case, I think an error return value from address_space_rw() is
appropriate.  Semantics would be that if an error is returned you
can't tell if the operation has not started, completed, or somewhere
in the middle.

-- 
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: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]