diff mbox

[qemu,v6,08/15] spapr_iommu: Introduce "enabled" state for TCE table

Message ID 1428679484-15451-9-git-send-email-aik@ozlabs.ru
State New
Headers show

Commit Message

Alexey Kardashevskiy April 10, 2015, 3:24 p.m. UTC
Currently TCE tables are created once at start and their size never
changes. We are going to change that by introducing a Dynamic DMA windows
support where DMA configuration may change during the guest execution.

This changes spapr_tce_new_table() to create an empty stub object. Only
LIOBN is assigned by the time of creation. It still will be called once
at the owner object (VIO or PHB) creation.

This introduces an "enabled" state for TCE table objects with two
helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
spapr_tce_table_enable() receives TCE table parameters and allocates
a guest view of the TCE table (in the user space or KVM).
spapr_tce_table_disable() disposes the table.

Follow up patches will disable+enable tables on reset (system reset
or DDW reset).

No visible change in behaviour is expected except the actual table
will be reallocated every reset. We might optimize this later.

The other way to implement this would be dynamically create/remove
the TCE table QOM objects but this would make migration impossible
as migration expects all QOM objects to exist at the receiver
so we have to have TCE table objects created when migration begins.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v6:
* got rid of set_props()
---
 hw/ppc/spapr_iommu.c    | 104 +++++++++++++++++++++++++++++++-----------------
 hw/ppc/spapr_pci.c      |  16 +++++---
 hw/ppc/spapr_pci_vfio.c |  10 ++---
 hw/ppc/spapr_vio.c      |   9 ++---
 include/hw/ppc/spapr.h  |  11 ++---
 5 files changed, 93 insertions(+), 57 deletions(-)

Comments

Thomas Huth April 16, 2015, 10:59 a.m. UTC | #1
Am Sat, 11 Apr 2015 01:24:37 +1000
schrieb Alexey Kardashevskiy <aik@ozlabs.ru>:

> Currently TCE tables are created once at start and their size never
> changes. We are going to change that by introducing a Dynamic DMA windows
> support where DMA configuration may change during the guest execution.
> 
> This changes spapr_tce_new_table() to create an empty stub object. Only
> LIOBN is assigned by the time of creation. It still will be called once
> at the owner object (VIO or PHB) creation.
> 
> This introduces an "enabled" state for TCE table objects with two
> helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
> spapr_tce_table_enable() receives TCE table parameters and allocates
> a guest view of the TCE table (in the user space or KVM).
> spapr_tce_table_disable() disposes the table.
> 
> Follow up patches will disable+enable tables on reset (system reset
> or DDW reset).
> 
> No visible change in behaviour is expected except the actual table
> will be reallocated every reset. We might optimize this later.
> 
> The other way to implement this would be dynamically create/remove
> the TCE table QOM objects but this would make migration impossible
> as migration expects all QOM objects to exist at the receiver
> so we have to have TCE table objects created when migration begins.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v6:
> * got rid of set_props()
> ---
>  hw/ppc/spapr_iommu.c    | 104 +++++++++++++++++++++++++++++++-----------------
>  hw/ppc/spapr_pci.c      |  16 +++++---
>  hw/ppc/spapr_pci_vfio.c |  10 ++---
>  hw/ppc/spapr_vio.c      |   9 ++---
>  include/hw/ppc/spapr.h  |  11 ++---
>  5 files changed, 93 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index a14cdc4..64f20f2 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -126,8 +126,47 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = {
>  static int spapr_tce_table_realize(DeviceState *dev)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> +
> +    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> +
> +    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
> +                     tcet);
> +
> +    return 0;
> +}
> +
> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
> +{
> +    sPAPRTCETable *tcet;
> +    char tmp[64];

Hm, any reason for that value "64"? 64 bytes seem much more than enough
here, if I count correctly, you only need 19 bytes max.

> +    if (spapr_tce_find_by_liobn(liobn)) {
> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
> +                " LIOBN 0x%x\n", liobn);
> +        return NULL;
> +    }
> +
> +    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
> +    tcet->liobn = liobn;
> +
> +    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
> +    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> +
> +    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
> +
> +    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
> +
> +    return tcet;
> +}

 Thomas
David Gibson April 22, 2015, 6:14 a.m. UTC | #2
On Sat, Apr 11, 2015 at 01:24:37AM +1000, Alexey Kardashevskiy wrote:
> Currently TCE tables are created once at start and their size never
> changes. We are going to change that by introducing a Dynamic DMA windows
> support where DMA configuration may change during the guest execution.
> 
> This changes spapr_tce_new_table() to create an empty stub object. Only
> LIOBN is assigned by the time of creation. It still will be called once
> at the owner object (VIO or PHB) creation.
> 
> This introduces an "enabled" state for TCE table objects with two
> helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
> spapr_tce_table_enable() receives TCE table parameters and allocates
> a guest view of the TCE table (in the user space or KVM).
> spapr_tce_table_disable() disposes the table.
> 
> Follow up patches will disable+enable tables on reset (system reset
> or DDW reset).
> 
> No visible change in behaviour is expected except the actual table
> will be reallocated every reset. We might optimize this later.
> 
> The other way to implement this would be dynamically create/remove
> the TCE table QOM objects but this would make migration impossible
> as migration expects all QOM objects to exist at the receiver
> so we have to have TCE table objects created when migration begins.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v6:
> * got rid of set_props()
> ---
>  hw/ppc/spapr_iommu.c    | 104 +++++++++++++++++++++++++++++++-----------------
>  hw/ppc/spapr_pci.c      |  16 +++++---
>  hw/ppc/spapr_pci_vfio.c |  10 ++---
>  hw/ppc/spapr_vio.c      |   9 ++---
>  include/hw/ppc/spapr.h  |  11 ++---
>  5 files changed, 93 insertions(+), 57 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index a14cdc4..64f20f2 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -126,8 +126,47 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = {
>  static int spapr_tce_table_realize(DeviceState *dev)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> +
> +    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> +
> +    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
> +                     tcet);
> +
> +    return 0;
> +}
> +
> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
> +{
> +    sPAPRTCETable *tcet;
> +    char tmp[64];
> +
> +    if (spapr_tce_find_by_liobn(liobn)) {
> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
> +                " LIOBN 0x%x\n", liobn);
> +        return NULL;
> +    }
> +
> +    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
> +    tcet->liobn = liobn;
> +
> +    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
> +    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> +
> +    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
> +
> +    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
> +
> +    return tcet;
> +}
> +
> +static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)

AFAICT there's only one caller of this, so it's not clear why this
isn't just open-coded in spapr_tce_table_enable().

> +{
>      uint64_t window_size = (uint64_t)tcet->nb_table << tcet->page_shift;
>  
> +    if (!tcet->nb_table) {
> +        return;
> +    }
> +
>      if (kvm_enabled() && !(window_size >> 32)) {
>          tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
>                                                window_size,
> @@ -140,65 +179,56 @@ static int spapr_tce_table_realize(DeviceState *dev)
>          tcet->table = g_malloc0(table_size);
>      }
>  
> -    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
> -
> -    memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
> +    memory_region_init_iommu(&tcet->iommu, OBJECT(tcet), &spapr_iommu_ops,
>                               "iommu-spapr",
>                               (uint64_t)tcet->nb_table << tcet->page_shift);
>  
> -    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> -
> -    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
> -                     tcet);
> -
> -    return 0;
> +    tcet->enabled = true;
>  }
>  
> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> -                                   uint64_t bus_offset,
> -                                   uint32_t page_shift,
> -                                   uint32_t nb_table,
> -                                   bool vfio_accel)
> +void spapr_tce_table_enable(sPAPRTCETable *tcet,
> +                            uint64_t bus_offset, uint32_t page_shift,
> +                            uint32_t nb_table, bool vfio_accel)
>  {
> -    sPAPRTCETable *tcet;
> -    char tmp[64];
> -
> -    if (spapr_tce_find_by_liobn(liobn)) {
> -        fprintf(stderr, "Attempted to create TCE table with duplicate"
> -                " LIOBN 0x%x\n", liobn);
> -        return NULL;
> -    }
> -
> -    if (!nb_table) {
> -        return NULL;
> +    if (tcet->enabled) {
> +        return;
>      }
>  
> -    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
> -    tcet->liobn = liobn;
>      tcet->bus_offset = bus_offset;
>      tcet->page_shift = page_shift;
>      tcet->nb_table = nb_table;
>      tcet->vfio_accel = vfio_accel;
>  
> -    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
> -    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> -
> -    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
> -
> -    return tcet;
> +    spapr_tce_table_do_enable(tcet);
>  }
>  
> -static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
> +void spapr_tce_table_disable(sPAPRTCETable *tcet)
>  {
> -    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> -
> -    QLIST_REMOVE(tcet, list);
> +    if (!tcet->enabled) {
> +        return;
> +    }
>  
>      if (!kvm_enabled() ||
>          (kvmppc_remove_spapr_tce(tcet->table, tcet->fd,
>                                   tcet->nb_table) != 0)) {
> +        tcet->fd = -1;
>          g_free(tcet->table);
>      }
> +    tcet->table = NULL;
> +    tcet->enabled = false;
> +    tcet->bus_offset = 0;
> +    tcet->page_shift = 0;
> +    tcet->nb_table = 0;
> +    tcet->vfio_accel = false;
> +}
> +
> +static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
> +{
> +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> +
> +    QLIST_REMOVE(tcet, list);
> +
> +    spapr_tce_table_disable(tcet);
>  }
>  
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 8c0d2eb..c3410b8 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -881,6 +881,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>          sphb->lsi_table[i].irq = irq;
>      }
>  
> +    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
> +    if (!tcet) {
> +            error_setg(errp, "failed to create TCE table");
> +            return;
> +    }
> +
>      info->dma_capabilities_update(sphb);
>      info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT,
>                            sphb->dma32_window_size);
> @@ -908,13 +914,13 @@ static int spapr_phb_dma_init_window(sPAPRPHBState *sphb,
>                                       uint64_t window_size)
>  {
>      uint64_t bus_offset = sphb->dma32_window_start;
> -    sPAPRTCETable *tcet;
> +    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>  
> -    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
> -                               window_size >> page_shift,
> -                               false);
> +    spapr_tce_table_enable(tcet, bus_offset, page_shift,
> +                           window_size >> page_shift,
> +                           false);
>  
> -    return tcet ? 0 : -1;
> +    return 0;
>  }
>  
>  static int spapr_phb_children_reset(Object *child, void *opaque)
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index 0ce8e61..a428166 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -49,13 +49,13 @@ static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb,
>                                            uint64_t window_size)
>  {
>      uint64_t bus_offset = sphb->dma32_window_start;
> -    sPAPRTCETable *tcet;
> +    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
>  
> -    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
> -                               window_size >> page_shift,
> -                               true);
> +    spapr_tce_table_enable(tcet, bus_offset, page_shift,
> +                           window_size >> page_shift,
> +                           true);
>  
> -    return tcet ? 0 : -1;
> +    return 0;
>  }
>  
>  static void spapr_phb_vfio_reset(DeviceState *qdev)
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 174033d..3e28835 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -479,11 +479,10 @@ static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
>          memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1);
>          address_space_init(&dev->as, &dev->mrroot, qdev->id);
>  
> -        dev->tcet = spapr_tce_new_table(qdev, liobn,
> -                                        0,
> -                                        SPAPR_TCE_PAGE_SHIFT,
> -                                        pc->rtce_window_size >>
> -                                        SPAPR_TCE_PAGE_SHIFT, false);
> +        dev->tcet = spapr_tce_new_table(qdev, liobn);
> +        spapr_tce_table_enable(dev->tcet, 0, SPAPR_TCE_PAGE_SHIFT,
> +                               pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT,
> +                               false);
>          dev->tcet->vdev = dev;
>          memory_region_add_subregion_overlap(&dev->mrroot, 0,
>                                              spapr_tce_get_iommu(dev->tcet), 2);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 7d9ab9d..074d837 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -498,6 +498,7 @@ typedef struct sPAPRTCETable sPAPRTCETable;
>  
>  struct sPAPRTCETable {
>      DeviceState parent;
> +    bool enabled;
>      uint32_t liobn;
>      uint32_t nb_table;
>      uint64_t bus_offset;
> @@ -515,11 +516,11 @@ sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t liobn);
>  void spapr_events_init(sPAPREnvironment *spapr);
>  void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
>  int spapr_h_cas_compose_response(target_ulong addr, target_ulong size);
> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> -                                   uint64_t bus_offset,
> -                                   uint32_t page_shift,
> -                                   uint32_t nb_table,
> -                                   bool vfio_accel);
> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
> +void spapr_tce_table_enable(sPAPRTCETable *tcet,
> +                            uint64_t bus_offset, uint32_t page_shift,
> +                            uint32_t nb_table, bool vfio_accel);
> +void spapr_tce_table_disable(sPAPRTCETable *tcet);
>  MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
>  int spapr_dma_dt(void *fdt, int node_off, const char *propname,
>                   uint32_t liobn, uint64_t window, uint32_t size);
Alexey Kardashevskiy April 22, 2015, 9:10 a.m. UTC | #3
On 04/22/2015 04:14 PM, David Gibson wrote:
> On Sat, Apr 11, 2015 at 01:24:37AM +1000, Alexey Kardashevskiy wrote:
>> Currently TCE tables are created once at start and their size never
>> changes. We are going to change that by introducing a Dynamic DMA windows
>> support where DMA configuration may change during the guest execution.
>>
>> This changes spapr_tce_new_table() to create an empty stub object. Only
>> LIOBN is assigned by the time of creation. It still will be called once
>> at the owner object (VIO or PHB) creation.
>>
>> This introduces an "enabled" state for TCE table objects with two
>> helper functions - spapr_tce_table_enable()/spapr_tce_table_disable().
>> spapr_tce_table_enable() receives TCE table parameters and allocates
>> a guest view of the TCE table (in the user space or KVM).
>> spapr_tce_table_disable() disposes the table.
>>
>> Follow up patches will disable+enable tables on reset (system reset
>> or DDW reset).
>>
>> No visible change in behaviour is expected except the actual table
>> will be reallocated every reset. We might optimize this later.
>>
>> The other way to implement this would be dynamically create/remove
>> the TCE table QOM objects but this would make migration impossible
>> as migration expects all QOM objects to exist at the receiver
>> so we have to have TCE table objects created when migration begins.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v6:
>> * got rid of set_props()
>> ---
>>   hw/ppc/spapr_iommu.c    | 104 +++++++++++++++++++++++++++++++-----------------
>>   hw/ppc/spapr_pci.c      |  16 +++++---
>>   hw/ppc/spapr_pci_vfio.c |  10 ++---
>>   hw/ppc/spapr_vio.c      |   9 ++---
>>   include/hw/ppc/spapr.h  |  11 ++---
>>   5 files changed, 93 insertions(+), 57 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index a14cdc4..64f20f2 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -126,8 +126,47 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = {
>>   static int spapr_tce_table_realize(DeviceState *dev)
>>   {
>>       sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>> +
>> +    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>> +
>> +    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
>> +                     tcet);
>> +
>> +    return 0;
>> +}
>> +
>> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
>> +{
>> +    sPAPRTCETable *tcet;
>> +    char tmp[64];
>> +
>> +    if (spapr_tce_find_by_liobn(liobn)) {
>> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
>> +                " LIOBN 0x%x\n", liobn);
>> +        return NULL;
>> +    }
>> +
>> +    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
>> +    tcet->liobn = liobn;
>> +
>> +    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
>> +    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
>> +
>> +    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
>> +
>> +    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
>> +
>> +    return tcet;
>> +}
>> +
>> +static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
>
> AFAICT there's only one caller of this, so it's not clear why this
> isn't just open-coded in spapr_tce_table_enable().


There is another call in "[PATCH qemu v6 14/15] spapr_pci/spapr_pci_vfio: 
Support Dynamic DMA Windows (DDW)", in spapr_tce_table_post_load().

Should I put a note in the commit log?
diff mbox

Patch

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index a14cdc4..64f20f2 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -126,8 +126,47 @@  static MemoryRegionIOMMUOps spapr_iommu_ops = {
 static int spapr_tce_table_realize(DeviceState *dev)
 {
     sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
+
+    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
+
+    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
+                     tcet);
+
+    return 0;
+}
+
+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
+{
+    sPAPRTCETable *tcet;
+    char tmp[64];
+
+    if (spapr_tce_find_by_liobn(liobn)) {
+        fprintf(stderr, "Attempted to create TCE table with duplicate"
+                " LIOBN 0x%x\n", liobn);
+        return NULL;
+    }
+
+    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
+    tcet->liobn = liobn;
+
+    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
+    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
+
+    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
+
+    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
+
+    return tcet;
+}
+
+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
+{
     uint64_t window_size = (uint64_t)tcet->nb_table << tcet->page_shift;
 
+    if (!tcet->nb_table) {
+        return;
+    }
+
     if (kvm_enabled() && !(window_size >> 32)) {
         tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
                                               window_size,
@@ -140,65 +179,56 @@  static int spapr_tce_table_realize(DeviceState *dev)
         tcet->table = g_malloc0(table_size);
     }
 
-    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
-
-    memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
+    memory_region_init_iommu(&tcet->iommu, OBJECT(tcet), &spapr_iommu_ops,
                              "iommu-spapr",
                              (uint64_t)tcet->nb_table << tcet->page_shift);
 
-    QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
-
-    vmstate_register(DEVICE(tcet), tcet->liobn, &vmstate_spapr_tce_table,
-                     tcet);
-
-    return 0;
+    tcet->enabled = true;
 }
 
-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
-                                   uint64_t bus_offset,
-                                   uint32_t page_shift,
-                                   uint32_t nb_table,
-                                   bool vfio_accel)
+void spapr_tce_table_enable(sPAPRTCETable *tcet,
+                            uint64_t bus_offset, uint32_t page_shift,
+                            uint32_t nb_table, bool vfio_accel)
 {
-    sPAPRTCETable *tcet;
-    char tmp[64];
-
-    if (spapr_tce_find_by_liobn(liobn)) {
-        fprintf(stderr, "Attempted to create TCE table with duplicate"
-                " LIOBN 0x%x\n", liobn);
-        return NULL;
-    }
-
-    if (!nb_table) {
-        return NULL;
+    if (tcet->enabled) {
+        return;
     }
 
-    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
-    tcet->liobn = liobn;
     tcet->bus_offset = bus_offset;
     tcet->page_shift = page_shift;
     tcet->nb_table = nb_table;
     tcet->vfio_accel = vfio_accel;
 
-    snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
-    object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
-
-    object_property_set_bool(OBJECT(tcet), true, "realized", NULL);
-
-    return tcet;
+    spapr_tce_table_do_enable(tcet);
 }
 
-static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
+void spapr_tce_table_disable(sPAPRTCETable *tcet)
 {
-    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
-
-    QLIST_REMOVE(tcet, list);
+    if (!tcet->enabled) {
+        return;
+    }
 
     if (!kvm_enabled() ||
         (kvmppc_remove_spapr_tce(tcet->table, tcet->fd,
                                  tcet->nb_table) != 0)) {
+        tcet->fd = -1;
         g_free(tcet->table);
     }
+    tcet->table = NULL;
+    tcet->enabled = false;
+    tcet->bus_offset = 0;
+    tcet->page_shift = 0;
+    tcet->nb_table = 0;
+    tcet->vfio_accel = false;
+}
+
+static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
+{
+    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
+
+    QLIST_REMOVE(tcet, list);
+
+    spapr_tce_table_disable(tcet);
 }
 
 MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 8c0d2eb..c3410b8 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -881,6 +881,12 @@  static void spapr_phb_realize(DeviceState *dev, Error **errp)
         sphb->lsi_table[i].irq = irq;
     }
 
+    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
+    if (!tcet) {
+            error_setg(errp, "failed to create TCE table");
+            return;
+    }
+
     info->dma_capabilities_update(sphb);
     info->dma_init_window(sphb, sphb->dma_liobn, SPAPR_TCE_PAGE_SHIFT,
                           sphb->dma32_window_size);
@@ -908,13 +914,13 @@  static int spapr_phb_dma_init_window(sPAPRPHBState *sphb,
                                      uint64_t window_size)
 {
     uint64_t bus_offset = sphb->dma32_window_start;
-    sPAPRTCETable *tcet;
+    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
 
-    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
-                               window_size >> page_shift,
-                               false);
+    spapr_tce_table_enable(tcet, bus_offset, page_shift,
+                           window_size >> page_shift,
+                           false);
 
-    return tcet ? 0 : -1;
+    return 0;
 }
 
 static int spapr_phb_children_reset(Object *child, void *opaque)
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 0ce8e61..a428166 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -49,13 +49,13 @@  static int spapr_phb_vfio_dma_init_window(sPAPRPHBState *sphb,
                                           uint64_t window_size)
 {
     uint64_t bus_offset = sphb->dma32_window_start;
-    sPAPRTCETable *tcet;
+    sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
 
-    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, bus_offset, page_shift,
-                               window_size >> page_shift,
-                               true);
+    spapr_tce_table_enable(tcet, bus_offset, page_shift,
+                           window_size >> page_shift,
+                           true);
 
-    return tcet ? 0 : -1;
+    return 0;
 }
 
 static void spapr_phb_vfio_reset(DeviceState *qdev)
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index 174033d..3e28835 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -479,11 +479,10 @@  static void spapr_vio_busdev_realize(DeviceState *qdev, Error **errp)
         memory_region_add_subregion_overlap(&dev->mrroot, 0, &dev->mrbypass, 1);
         address_space_init(&dev->as, &dev->mrroot, qdev->id);
 
-        dev->tcet = spapr_tce_new_table(qdev, liobn,
-                                        0,
-                                        SPAPR_TCE_PAGE_SHIFT,
-                                        pc->rtce_window_size >>
-                                        SPAPR_TCE_PAGE_SHIFT, false);
+        dev->tcet = spapr_tce_new_table(qdev, liobn);
+        spapr_tce_table_enable(dev->tcet, 0, SPAPR_TCE_PAGE_SHIFT,
+                               pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT,
+                               false);
         dev->tcet->vdev = dev;
         memory_region_add_subregion_overlap(&dev->mrroot, 0,
                                             spapr_tce_get_iommu(dev->tcet), 2);
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 7d9ab9d..074d837 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -498,6 +498,7 @@  typedef struct sPAPRTCETable sPAPRTCETable;
 
 struct sPAPRTCETable {
     DeviceState parent;
+    bool enabled;
     uint32_t liobn;
     uint32_t nb_table;
     uint64_t bus_offset;
@@ -515,11 +516,11 @@  sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t liobn);
 void spapr_events_init(sPAPREnvironment *spapr);
 void spapr_events_fdt_skel(void *fdt, uint32_t epow_irq);
 int spapr_h_cas_compose_response(target_ulong addr, target_ulong size);
-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
-                                   uint64_t bus_offset,
-                                   uint32_t page_shift,
-                                   uint32_t nb_table,
-                                   bool vfio_accel);
+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
+void spapr_tce_table_enable(sPAPRTCETable *tcet,
+                            uint64_t bus_offset, uint32_t page_shift,
+                            uint32_t nb_table, bool vfio_accel);
+void spapr_tce_table_disable(sPAPRTCETable *tcet);
 MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet);
 int spapr_dma_dt(void *fdt, int node_off, const char *propname,
                  uint32_t liobn, uint64_t window, uint32_t size);