diff mbox

[20/40] pci: use memory core for iommu support

Message ID 1367936238-12196-21-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini May 7, 2013, 2:16 p.m. UTC
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.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci/pci.c             |   53 ++++++++++++++++++++++++++--------------------
 hw/ppc/spapr_pci.c       |   12 +++++++---
 include/hw/pci/pci.h     |    7 ++++-
 include/hw/pci/pci_bus.h |    5 ++-
 4 files changed, 46 insertions(+), 31 deletions(-)

Comments

Peter Maydell May 7, 2013, 6:30 p.m. UTC | #1
On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 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.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci/pci.c             |   53 ++++++++++++++++++++++++++--------------------
>  hw/ppc/spapr_pci.c       |   12 +++++++---
>  include/hw/pci/pci.h     |    7 ++++-
>  include/hw/pci/pci_bus.h |    5 ++-
>  4 files changed, 46 insertions(+), 31 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 16ed118..3eb397c 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -279,6 +279,16 @@ int pci_find_domain(const PCIBus *bus)
>      return -1;
>  }
>
> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
> +{
> +    /* FIXME: inherit memory region from bus creator */
> +    return get_system_memory();
> +}

This seems a bit misnamed since it doesn't actually need to
return an iommu MemoryRegion (and in fact in most cases it
won't). What it's actually returning is "this is the memory
region representing the view for bus master DMA devices to
DMA into", I think.

Also, technically speaking get_system_memory() is never the
right answer, though in practice it's good enough for our
purposes. (returning get_system_memory() would allow a bus
master DMA device to access back into the PCI bus by
DMAing to the address in system memory where the PCI host
controller is mapped, which I'm guessing is not possible
on real hardware.)

As you kind of imply with the FIXME comment here, I think
what we probably actually eventually want is for pci_bus_init()
and friends to have a parameter for the DMA MemoryRegion
(but what this patch does is fine for now).

Once these patches go in I could use this to do the
versatile_pci SMAP registers, though that's more for
completeness than anything else.

> +
> +static void pci_default_iommu_dtor(MemoryRegion *mr)
> +{
> +}
> +
>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>                           const char *name,
>                           MemoryRegion *address_space_mem,
> @@ -289,6 +299,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, NULL, NULL);
>
>      /* host bridge */
>      QLIST_INIT(&bus->child);
> @@ -801,21 +812,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 +875,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);
> +    g_free(pci_dev->dma);
> +    pci_dev->dma = NULL;
>  }
>
>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
> @@ -2234,10 +2239,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 ? dtor : pci_default_iommu_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,11 @@ 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(PCIBus *bus, void *opaque, int devfn)
>  {
>      sPAPRPHBState *phb = opaque;
>
> -    return spapr_tce_get_dma(phb->tcet);
> +    return spapr_tce_get_iommu(phb->tcet);
>  }
>
>  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, NULL, 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);

I'm not entirely convinced by this API. Some doc comments
might help...

thanks
-- PMM
pingfan liu May 11, 2013, 5:09 a.m. UTC | #2
On Wed, May 8, 2013 at 2:30 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 7 May 2013 15:16, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> 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.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Avi Kivity <avi.kivity@gmail.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/pci/pci.c             |   53 ++++++++++++++++++++++++++--------------------
>>  hw/ppc/spapr_pci.c       |   12 +++++++---
>>  include/hw/pci/pci.h     |    7 ++++-
>>  include/hw/pci/pci_bus.h |    5 ++-
>>  4 files changed, 46 insertions(+), 31 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 16ed118..3eb397c 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -279,6 +279,16 @@ int pci_find_domain(const PCIBus *bus)
>>      return -1;
>>  }
>>
>> +static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
>> +{
>> +    /* FIXME: inherit memory region from bus creator */
>> +    return get_system_memory();
>> +}
>
> This seems a bit misnamed since it doesn't actually need to
> return an iommu MemoryRegion (and in fact in most cases it
> won't). What it's actually returning is "this is the memory
> region representing the view for bus master DMA devices to
> DMA into", I think.
>
Maybe pci_fake_iommu ?

> Also, technically speaking get_system_memory() is never the
> right answer, though in practice it's good enough for our
> purposes. (returning get_system_memory() would allow a bus
> master DMA device to access back into the PCI bus by
> DMAing to the address in system memory where the PCI host
> controller is mapped, which I'm guessing is not possible
> on real hardware.)
>
I think although it is rare case, but PCI device can fetch another's
data on its pci-ram, expecially pci-spec does not forbid it.

Regards,
Pingfan

> As you kind of imply with the FIXME comment here, I think
> what we probably actually eventually want is for pci_bus_init()
> and friends to have a parameter for the DMA MemoryRegion
> (but what this patch does is fine for now).
>
> Once these patches go in I could use this to do the
> versatile_pci SMAP registers, though that's more for
> completeness than anything else.
>
>> +
>> +static void pci_default_iommu_dtor(MemoryRegion *mr)
>> +{
>> +}
>> +
>>  static void pci_bus_init(PCIBus *bus, DeviceState *parent,
>>                           const char *name,
>>                           MemoryRegion *address_space_mem,
>> @@ -289,6 +299,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, NULL, NULL);
>>
>>      /* host bridge */
>>      QLIST_INIT(&bus->child);
>> @@ -801,21 +812,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 +875,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);
>> +    g_free(pci_dev->dma);
>> +    pci_dev->dma = NULL;
>>  }
>>
>>  static void pci_unregister_io_regions(PCIDevice *pci_dev)
>> @@ -2234,10 +2239,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 ? dtor : pci_default_iommu_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,11 @@ 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(PCIBus *bus, void *opaque, int devfn)
>>  {
>>      sPAPRPHBState *phb = opaque;
>>
>> -    return spapr_tce_get_dma(phb->tcet);
>> +    return spapr_tce_get_iommu(phb->tcet);
>>  }
>>
>>  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, NULL, 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);
>
> I'm not entirely convinced by this API. Some doc comments
> might help...
>
> thanks
> -- PMM
Peter Maydell May 11, 2013, 8:07 a.m. UTC | #3
On 11 May 2013 06:09, liu ping fan <qemulist@gmail.com> wrote:
> On Wed, May 8, 2013 at 2:30 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Also, technically speaking get_system_memory() is never the
>> right answer, though in practice it's good enough for our
>> purposes. (returning get_system_memory() would allow a bus
>> master DMA device to access back into the PCI bus by
>> DMAing to the address in system memory where the PCI host
>> controller is mapped, which I'm guessing is not possible
>> on real hardware.)
>>
> I think although it is rare case, but PCI device can fetch another's
> data on its pci-ram, expecially pci-spec does not forbid it.

Yes, certainly, but this happens because the other pci device
is present in the PCI address space, not because PCI device
A talks to the host controller's CPU-facing interface and
accesses PCI device B that way...

-- PMM
diff mbox

Patch

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 16ed118..3eb397c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -279,6 +279,16 @@  int pci_find_domain(const PCIBus *bus)
     return -1;
 }
 
+static MemoryRegion *pci_default_iommu(PCIBus *bus, void *opaque, int devfn)
+{
+    /* FIXME: inherit memory region from bus creator */
+    return get_system_memory();
+}
+
+static void pci_default_iommu_dtor(MemoryRegion *mr)
+{
+}
+
 static void pci_bus_init(PCIBus *bus, DeviceState *parent,
                          const char *name,
                          MemoryRegion *address_space_mem,
@@ -289,6 +299,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, NULL, NULL);
 
     /* host bridge */
     QLIST_INIT(&bus->child);
@@ -801,21 +812,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 +875,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);
+    g_free(pci_dev->dma);
+    pci_dev->dma = NULL;
 }
 
 static void pci_unregister_io_regions(PCIDevice *pci_dev)
@@ -2234,10 +2239,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 ? dtor : pci_default_iommu_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,11 @@  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(PCIBus *bus, void *opaque, int devfn)
 {
     sPAPRPHBState *phb = opaque;
 
-    return spapr_tce_get_dma(phb->tcet);
+    return spapr_tce_get_iommu(phb->tcet);
 }
 
 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, NULL, 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;