[qemu,v5,07/12] spapr_iommu: Rework TCE table initialization

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 spapr_tce_set_props() to set the table size, start and
page size. It only assigns the properties. It will be called at the owner
object creation OR later from the "ibm,create-pe-dma-window" RTAS handler
so the table's parameters can change.

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() allocates the 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>
---
 hw/ppc/spapr_iommu.c    | 98 +++++++++++++++++++++++++++++++------------------
 hw/ppc/spapr_pci.c      |  8 ++--
 hw/ppc/spapr_pci_vfio.c | 11 ++++--
 hw/ppc/spapr_vio.c      | 10 ++---
 include/hw/ppc/spapr.h  | 12 +++---
 5 files changed, 87 insertions(+), 52 deletions(-)

On Tue, Mar 31, 2015 at 04:28:42PM +1100, 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 spapr_tce_set_props() to set the table size, start and
> page size. It only assigns the properties. It will be called at the owner
> object creation OR later from the "ibm,create-pe-dma-window" RTAS handler
> so the table's parameters can change.
> 
> 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() allocates the 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>
> ---
>  hw/ppc/spapr_iommu.c    | 98 +++++++++++++++++++++++++++++++------------------
>  hw/ppc/spapr_pci.c      |  8 ++--
>  hw/ppc/spapr_pci_vfio.c | 11 ++++--
>  hw/ppc/spapr_vio.c      | 10 ++---
>  include/hw/ppc/spapr.h  | 12 +++---
>  5 files changed, 87 insertions(+), 52 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index a14cdc4..a015357 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -126,25 +126,6 @@ static MemoryRegionIOMMUOps spapr_iommu_ops = {
>  static int spapr_tce_table_realize(DeviceState *dev)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> -    uint64_t window_size = (uint64_t)tcet->nb_table << tcet->page_shift;
> -
> -    if (kvm_enabled() && !(window_size >> 32)) {
> -        tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
> -                                              window_size,
> -                                              &tcet->fd,
> -                                              tcet->vfio_accel);
> -    }
> -
> -    if (!tcet->table) {
> -        size_t table_size = tcet->nb_table * sizeof(uint64_t);
> -        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,
> -                             "iommu-spapr",
> -                             (uint64_t)tcet->nb_table << tcet->page_shift);
>  
>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>  
> @@ -154,11 +135,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
>      return 0;
>  }
>  
> -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)
>  {
>      sPAPRTCETable *tcet;
>      char tmp[64];
> @@ -169,36 +146,87 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>          return NULL;
>      }
>  
> -    if (!nb_table) {
> -        return NULL;
> -    }
> -
>      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);
>  
> +    trace_spapr_iommu_new_table(tcet->liobn, tcet, tcet->table, tcet->fd);
> +
>      return tcet;
>  }
>  
> -static void spapr_tce_table_unrealize(DeviceState *dev, Error **errp)
> +void spapr_tce_set_props(sPAPRTCETable *tcet, uint64_t bus_offset,
> +                         uint32_t page_shift, uint32_t nb_table,
> +                         bool vfio_accel)
>  {
> -    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> +    if (tcet->enabled) {
> +        return;
> +    }

Since you can't change the properties while the table is enabled, why
not just make these parameters to spapr_tce_table_enable().


It seems to me what this is really about is making a distinction
between two objects: (1) is the TCE table as an abstract concept - it
knows its liobn and its owner, and that's about it  (2) the TCE table
as a specific instantiated table - it has a specific size and current
entries.

(2) can't be a QOM object or migration breaks, but you can still think
of it as a distinct entity at the C level.

> +    tcet->bus_offset = bus_offset;
> +    tcet->page_shift = page_shift;
> +    tcet->nb_table = nb_table;
> +    tcet->vfio_accel = vfio_accel;
> +}
>  
> -    QLIST_REMOVE(tcet, list);
> +void spapr_tce_table_enable(sPAPRTCETable *tcet)
> +{
> +    uint64_t window_size = (uint64_t)tcet->nb_table << tcet->page_shift;
> +
> +    if (tcet->enabled) {
> +        return;
> +    }
> +
> +    if (!tcet->nb_table) {
> +        return;
> +    }
> +
> +    if (kvm_enabled() && !(window_size >> 32)) {
> +        tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
> +                                              window_size,
> +                                              &tcet->fd,
> +                                              tcet->vfio_accel);
> +    }
> +
> +    if (!tcet->table) {
> +        size_t table_size = tcet->nb_table * sizeof(uint64_t);
> +        tcet->table = g_malloc0(table_size);
> +    }
> +
> +    memory_region_init_iommu(&tcet->iommu, OBJECT(tcet), &spapr_iommu_ops,
> +                             "iommu-spapr",
> +                             (uint64_t)tcet->nb_table << tcet->page_shift);
> +
> +    tcet->enabled = true;
> +}
> +
> +void spapr_tce_table_disable(sPAPRTCETable *tcet)
> +{
> +    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;
> +    spapr_tce_set_props(tcet, 0, 0, 0, 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 52c5c73..acfdbe5 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -895,15 +895,17 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
>      sPAPRTCETable *tcet;
>      uint32_t nb_table;
>  
> -    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
> -    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
> -                               0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
> +    tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn);
>      if (!tcet) {
>          error_setg(errp, "Unable to create TCE table for %s",
>                     sphb->dtbusname);
>          return ;
>      }
>  
> +    nb_table = SPAPR_PCI_DMA32_SIZE >> SPAPR_TCE_PAGE_SHIFT;
> +    spapr_tce_set_props(tcet, 0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
> +    spapr_tce_table_enable(tcet);
> +
>      /* Register default 32bit DMA window */
>      memory_region_add_subregion(&sphb->iommu_root, 0,
>                                  spapr_tce_get_iommu(tcet));
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index f8b503e..6c9adb5 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -34,6 +34,7 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>      int ret;
>      sPAPRTCETable *tcet;
>      uint32_t liobn = svphb->phb.dma_liobn;
> +    uint32_t nb_table;
>  
>      ret = vfio_container_ioctl(&svphb->phb.iommu_as,
>                                 VFIO_CHECK_EXTENSION,
> @@ -52,16 +53,18 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
>          return;
>      }
>  
> -    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, info.dma32_window_start,
> -                               SPAPR_TCE_PAGE_SHIFT,
> -                               info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT,
> -                               true);
> +    tcet = spapr_tce_new_table(DEVICE(sphb), liobn);
>      if (!tcet) {
>          error_setg(errp, "spapr-vfio: failed to create VFIO TCE table");
>          return;
>      }
>  
>      /* Register default 32bit DMA window */
> +    nb_table = info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT;
> +    spapr_tce_set_props(tcet, info.dma32_window_start, SPAPR_TCE_PAGE_SHIFT,
> +                        nb_table, true);
> +    spapr_tce_table_enable(tcet);
> +
>      memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
>                                  spapr_tce_get_iommu(tcet));
>  }
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index 174033d..6394527 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -479,11 +479,11 @@ 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_set_props(dev->tcet, 0, SPAPR_TCE_PAGE_SHIFT,
> +                            pc->rtce_window_size >> SPAPR_TCE_PAGE_SHIFT,
> +                            false);
> +        spapr_tce_table_enable(dev->tcet);
>          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..6e33b9b 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,12 @@ 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_set_props(sPAPRTCETable *tcet, uint64_t bus_offset,
> +                         uint32_t page_shift, uint32_t nb_table,
> +                         bool vfio_accel);
> +void spapr_tce_table_enable(sPAPRTCETable *tcet);
> +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);