Re: [Qemu-ppc] [PATCH qemu v13 04/16] spapr_iommu: Introduce "enabled" state for TCE table

[David Gibson]

On Thu, Mar 10, 2016 at 06:39:39PM +1100, Alexey Kardashevskiy wrote:
> On 03/03/2016 02:00 PM, David Gibson wrote:
> >On Tue, Mar 01, 2016 at 08:10:29PM +1100, Alexey Kardashevskiy wrote:
> >>Currently TCE tables are created once at start and their sizes never
> >>change. 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 zero-size IOMMU
> >>memory region (IOMMU MR). 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, allocates
> >>a guest view of the TCE table (in the user space or KVM) and
> >>sets the correct size on the IOMMU MR.
> >>- spapr_tce_table_disable() disposes the table and resets the IOMMU MR
> >>size.
> >>
> >>This changes the PHB reset handler to do the default DMA initialization
> >>instead of spapr_phb_realize(). This does not make differenct now but
> >>later with more than just one DMA window, we will have to remove them all
> >>and create the default one on a system 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 the migration code expects all QOM objects to exist at the receiver
> >>so we have to have TCE table objects created when migration begins.
> >>
> >>spapr_tce_table_do_enable() is separated from from spapr_tce_table_enable()
> >>as later it will be called at the sPAPRTCETable post-migration stage when
> >>it already has all the properties set after the migration.
> >>
> >>Signed-off-by: Alexey Kardashevskiy <address@hidden>
> >
> >Reviewed-by: David Gibson <address@hidden>
> >
> >Although there's one nit that could be improved:
> >
> >
> >>---
> >>  hw/ppc/spapr_iommu.c   | 80 
> >> +++++++++++++++++++++++++++++++++++---------------
> >>  hw/ppc/spapr_pci.c     | 21 +++++++++----
> >>  hw/ppc/spapr_vio.c     |  9 +++---
> >>  include/hw/ppc/spapr.h | 10 +++----
> >>  4 files changed, 80 insertions(+), 40 deletions(-)
> >>
> >>diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> >>index 8132f64..e66e128 100644
> >>--- a/hw/ppc/spapr_iommu.c
> >>+++ b/hw/ppc/spapr_iommu.c
> >>@@ -174,15 +174,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
> >>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
> >>
> >>      tcet->fd = -1;
> >>-    tcet->table = spapr_tce_alloc_table(tcet->liobn,
> >>-                                        tcet->page_shift,
> >>-                                        tcet->nb_table,
> >>-                                        &tcet->fd,
> >>-                                        tcet->need_vfio);
> >>-
> >>      memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
> >>-                             "iommu-spapr",
> >>-                             (uint64_t)tcet->nb_table << tcet->page_shift);
> >>+                             "iommu-spapr", 0);
> >>
> >>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> >>
> >>@@ -224,14 +217,10 @@ void spapr_tce_set_need_vfio(sPAPRTCETable *tcet, 
> >>bool need_vfio)
> >>      tcet->table = newtable;
> >>  }
> >>
> >>-sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
> >>-                                   uint64_t bus_offset,
> >>-                                   uint32_t page_shift,
> >>-                                   uint32_t nb_table,
> >>-                                   bool need_vfio)
> >>+sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn)
> >>  {
> >>      sPAPRTCETable *tcet;
> >>-    char tmp[64];
> >>+    char tmp[32];
> >>
> >>      if (spapr_tce_find_by_liobn(liobn)) {
> >>          fprintf(stderr, "Attempted to create TCE table with duplicate"
> >>@@ -239,16 +228,8 @@ 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->need_vfio = need_vfio;
> >>
> >>      snprintf(tmp, sizeof(tmp), "tce-table-%x", liobn);
> >>      object_property_add_child(OBJECT(owner), tmp, OBJECT(tcet), NULL);
> >>@@ -258,14 +239,65 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState 
> >>*owner, uint32_t liobn,
> >>      return tcet;
> >>  }
> >>
> >>+static void spapr_tce_table_do_enable(sPAPRTCETable *tcet)
> >>+{
> >>+    if (!tcet->nb_table) {
> >>+        return;
> >>+    }
> >>+
> >>+    tcet->table = spapr_tce_alloc_table(tcet->liobn,
> >>+                                        tcet->page_shift,
> >>+                                        tcet->nb_table,
> >>+                                        &tcet->fd,
> >>+                                        tcet->need_vfio);
> >>+
> >>+    memory_region_set_size(&tcet->iommu,
> >>+                           (uint64_t)tcet->nb_table << tcet->page_shift);
> >>+
> >>+    tcet->enabled = true;
> >>+}
> >>+
> >>+void spapr_tce_table_enable(sPAPRTCETable *tcet,
> >>+                            uint32_t page_shift, uint64_t bus_offset,
> >>+                            uint32_t nb_table, bool need_vfio)
> >>+{
> >>+    if (tcet->enabled) {
> >>+        return;
> >
> >If the given parameters are different from the current ones, treating
> >this as a no-op is rather misleading.  I gather that to resize the
> >window you're expected to disable, then re-enable.  In which case I
> >think it would be safer to actually throw some kind of error on a
> >double enable.
> 
> I'll add here
> 
>  error_report("Warning: trying to enable already enabled TCE table");
> 
> ...
> 
> 
> 
> 
> >
> >
> >>+    }
> >>+
> >>+    tcet->bus_offset = bus_offset;
> >>+    tcet->page_shift = page_shift;
> >>+    tcet->nb_table = nb_table;
> >>+    tcet->need_vfio = need_vfio;
> >>+
> >>+    spapr_tce_table_do_enable(tcet);
> >>+}
> >>+
> >>+static void spapr_tce_table_disable(sPAPRTCETable *tcet)
> >>+{
> >>+    if (!tcet->enabled) {
> 
> ...and
> 
> error_report("Warning: trying to disable already disabled TCE table");

That sounds good.

> or g_assert()?

Erm.. only if this can't be triggered by user actions.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature