[Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[Alexey Kardashevskiy]

Instead of tweaking a TCE table device by adding there a bypass flag,
let's add an alias to RAM and IOMMU memory region, and enable/disable
those according to the selected bypass mode.
This way IOMMU memory region can have size of the actual window rather
than ram_size which is essential for upcoming DDW support.

This moves bypass logic to VIO layer and keeps @bypass flag in TCE table
for migration compatibility only. This replaces spapr_tce_set_bypass()
calls with explicit assignment to avoid confusion as the function could
do something more that just syncing the @bypass flag.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* kept @bypass in sPAPRTCETable not to break migration
---
 hw/ppc/spapr_iommu.c       | 13 +++----------
 hw/ppc/spapr_vio.c         | 39 +++++++++++++++++++++++++++++++++++++--
 include/hw/ppc/spapr.h     |  1 -
 include/hw/ppc/spapr_vio.h |  2 ++
 4 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
index da47474..a7b027a 100644
--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -72,9 +72,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
         .perm = IOMMU_NONE,
     };
 
-    if (tcet->bypass) {
-        ret.perm = IOMMU_RW;
-    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
+    if ((addr >> tcet->page_shift) < tcet->nb_table) {
         /* Check if we are in bound */
         hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);
 
@@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
     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", ram_size);
+                             "iommu-spapr",
+                             (uint64_t)tcet->nb_table << tcet->page_shift);
 
     QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
 
@@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
     return &tcet->iommu;
 }
 
-void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass)
-{
-    tcet->bypass = bypass;
-}
-
 static void spapr_tce_reset(DeviceState *dev)
 {
     sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
     size_t table_size = tcet->nb_table * sizeof(uint64_t);
 
-    tcet->bypass = false;
     memset(tcet->table, 0, table_size);
 }
 
diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
index dc9e46a..b65eb11 100644
--- a/hw/ppc/spapr_vio.c
+++ b/hw/ppc/spapr_vio.c
@@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev)
     free_crq(dev);
 }
 
+static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass)
+{
+    if (!dev->tcet) {
+        return;
+    }
+
+    memory_region_set_enabled(&dev->mrbypass, bypass);
+    memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass);
+
+    dev->tcet->bypass = bypass;
+}
+
 static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
                                 uint32_t token,
                                 uint32_t nargs, target_ulong args,
@@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
         return;
     }
 
-    spapr_tce_set_bypass(dev->tcet, !!enable);
+    spapr_vio_set_bypass(dev, !!enable);
 
     rtas_st(rets, 0, RTAS_OUT_SUCCESS);
 }
@@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
 
     dev->signal_state = 0;
 
+    spapr_vio_set_bypass(dev, false);
     if (pc->reset) {
         pc->reset(dev);
     }
@@ -456,12 +469,22 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
 
     if (pc->rtce_window_size) {
         uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg;
+
+        memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root",
+                           ram_size);
+        memory_region_init_alias(&dev->mrbypass, OBJECT(dev),
+                                 "iommu-spapr-bypass", get_system_memory(),
+                                 0, ram_size);
+        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);
-        address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id);
+        memory_region_add_subregion_overlap(&dev->mrroot, 0,
+                                            spapr_tce_get_iommu(dev->tcet), 2);
     }
 
     return pc->init(dev);
@@ -541,6 +564,17 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
     k->init = spapr_vio_bridge_init;
 }
 
+static int spapr_vio_post_load(void *opaque, int version_id)
+{
+    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque);
+
+    if (dev->tcet) {
+        spapr_vio_set_bypass(dev, dev->tcet->bypass);
+    }
+
+    return 0;
+}
+
 static const TypeInfo spapr_vio_bridge_info = {
     .name          = "spapr-vio-bridge",
     .parent        = TYPE_SYS_BUS_DEVICE,
@@ -552,6 +586,7 @@ const VMStateDescription vmstate_spapr_vio = {
     .name = "spapr_vio",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = spapr_vio_post_load,
     .fields = (VMStateField[]) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice),
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 716bff4..ebc7e51 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -475,7 +475,6 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
                                    uint32_t nb_table,
                                    bool vfio_accel);
 MemoryRegion *spapr_tce_get_iommu(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,
diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
index 46edc2a..6ad55d1 100644
--- a/include/hw/ppc/spapr_vio.h
+++ b/include/hw/ppc/spapr_vio.h
@@ -64,6 +64,8 @@ struct VIOsPAPRDevice {
     target_ulong signal_state;
     VIOsPAPR_CRQ crq;
     AddressSpace as;
+    MemoryRegion mrroot;
+    MemoryRegion mrbypass;
     sPAPRTCETable *tcet;
 };
 
-- 
2.0.0

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[Alexey Kardashevskiy]

On 01/27/2015 05:13 PM, Alexey Kardashevskiy wrote:
> Instead of tweaking a TCE table device by adding there a bypass flag,
> let's add an alias to RAM and IOMMU memory region, and enable/disable
> those according to the selected bypass mode.
> This way IOMMU memory region can have size of the actual window rather
> than ram_size which is essential for upcoming DDW support.
> 
> This moves bypass logic to VIO layer and keeps @bypass flag in TCE table
> for migration compatibility only. This replaces spapr_tce_set_bypass()
> calls with explicit assignment to avoid confusion as the function could
> do something more that just syncing the @bypass flag.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * kept @bypass in sPAPRTCETable not to break migration
> ---
>  hw/ppc/spapr_iommu.c       | 13 +++----------
>  hw/ppc/spapr_vio.c         | 39 +++++++++++++++++++++++++++++++++++++--
>  include/hw/ppc/spapr.h     |  1 -
>  include/hw/ppc/spapr_vio.h |  2 ++
>  4 files changed, 42 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index da47474..a7b027a 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -72,9 +72,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
>          .perm = IOMMU_NONE,
>      };
>  
> -    if (tcet->bypass) {
> -        ret.perm = IOMMU_RW;
> -    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
> +    if ((addr >> tcet->page_shift) < tcet->nb_table) {
>          /* Check if we are in bound */
>          hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);
>  
> @@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
>      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", ram_size);
> +                             "iommu-spapr",
> +                             (uint64_t)tcet->nb_table << tcet->page_shift);
>  
>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>  
> @@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
>      return &tcet->iommu;
>  }
>  
> -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass)
> -{
> -    tcet->bypass = bypass;
> -}
> -
>  static void spapr_tce_reset(DeviceState *dev)
>  {
>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>      size_t table_size = tcet->nb_table * sizeof(uint64_t);
>  
> -    tcet->bypass = false;
>      memset(tcet->table, 0, table_size);
>  }
>  
> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
> index dc9e46a..b65eb11 100644
> --- a/hw/ppc/spapr_vio.c
> +++ b/hw/ppc/spapr_vio.c
> @@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev)
>      free_crq(dev);
>  }
>  
> +static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass)
> +{
> +    if (!dev->tcet) {
> +        return;
> +    }
> +
> +    memory_region_set_enabled(&dev->mrbypass, bypass);
> +    memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass);
> +
> +    dev->tcet->bypass = bypass;
> +}
> +
>  static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                  uint32_t token,
>                                  uint32_t nargs, target_ulong args,
> @@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>          return;
>      }
>  
> -    spapr_tce_set_bypass(dev->tcet, !!enable);
> +    spapr_vio_set_bypass(dev, !!enable);
>  
>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>  }
> @@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>  
>      dev->signal_state = 0;
>  
> +    spapr_vio_set_bypass(dev, false);
>      if (pc->reset) {
>          pc->reset(dev);
>      }
> @@ -456,12 +469,22 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>  
>      if (pc->rtce_window_size) {
>          uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg;
> +
> +        memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root",
> +                           ram_size);
> +        memory_region_init_alias(&dev->mrbypass, OBJECT(dev),
> +                                 "iommu-spapr-bypass", get_system_memory(),
> +                                 0, ram_size);
> +        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);
> -        address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id);
> +        memory_region_add_subregion_overlap(&dev->mrroot, 0,
> +                                            spapr_tce_get_iommu(dev->tcet), 2);
>      }
>  
>      return pc->init(dev);
> @@ -541,6 +564,17 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
>      k->init = spapr_vio_bridge_init;
>  }
>  
> +static int spapr_vio_post_load(void *opaque, int version_id)
> +{
> +    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque);
> +
> +    if (dev->tcet) {
> +        spapr_vio_set_bypass(dev, dev->tcet->bypass);


Oh. Just realized that if VIOsPAPRDevice migrated before sPAPRTCETable,
@bypass will always be false. So please ignore this patch for now :(




> +    }
> +
> +    return 0;
> +}
> +
>  static const TypeInfo spapr_vio_bridge_info = {
>      .name          = "spapr-vio-bridge",
>      .parent        = TYPE_SYS_BUS_DEVICE,
> @@ -552,6 +586,7 @@ const VMStateDescription vmstate_spapr_vio = {
>      .name = "spapr_vio",
>      .version_id = 1,
>      .minimum_version_id = 1,
> +    .post_load = spapr_vio_post_load,
>      .fields = (VMStateField[]) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice),
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 716bff4..ebc7e51 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -475,7 +475,6 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>                                     uint32_t nb_table,
>                                     bool vfio_accel);
>  MemoryRegion *spapr_tce_get_iommu(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,
> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> index 46edc2a..6ad55d1 100644
> --- a/include/hw/ppc/spapr_vio.h
> +++ b/include/hw/ppc/spapr_vio.h
> @@ -64,6 +64,8 @@ struct VIOsPAPRDevice {
>      target_ulong signal_state;
>      VIOsPAPR_CRQ crq;
>      AddressSpace as;
> +    MemoryRegion mrroot;
> +    MemoryRegion mrbypass;
>      sPAPRTCETable *tcet;
>  };
>  
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[Alexey Kardashevskiy]

On 01/28/2015 08:21 AM, Alexey Kardashevskiy wrote:
> On 01/27/2015 05:13 PM, Alexey Kardashevskiy wrote:
>> Instead of tweaking a TCE table device by adding there a bypass flag,
>> let's add an alias to RAM and IOMMU memory region, and enable/disable
>> those according to the selected bypass mode.
>> This way IOMMU memory region can have size of the actual window rather
>> than ram_size which is essential for upcoming DDW support.
>>
>> This moves bypass logic to VIO layer and keeps @bypass flag in TCE table
>> for migration compatibility only. This replaces spapr_tce_set_bypass()
>> calls with explicit assignment to avoid confusion as the function could
>> do something more that just syncing the @bypass flag.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v2:
>> * kept @bypass in sPAPRTCETable not to break migration
>> ---
>>  hw/ppc/spapr_iommu.c       | 13 +++----------
>>  hw/ppc/spapr_vio.c         | 39 +++++++++++++++++++++++++++++++++++++--
>>  include/hw/ppc/spapr.h     |  1 -
>>  include/hw/ppc/spapr_vio.h |  2 ++
>>  4 files changed, 42 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index da47474..a7b027a 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -72,9 +72,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
>>          .perm = IOMMU_NONE,
>>      };
>>  
>> -    if (tcet->bypass) {
>> -        ret.perm = IOMMU_RW;
>> -    } else if ((addr >> tcet->page_shift) < tcet->nb_table) {
>> +    if ((addr >> tcet->page_shift) < tcet->nb_table) {
>>          /* Check if we are in bound */
>>          hwaddr page_mask = IOMMU_PAGE_MASK(tcet->page_shift);
>>  
>> @@ -131,7 +129,8 @@ static int spapr_tce_table_realize(DeviceState *dev)
>>      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", ram_size);
>> +                             "iommu-spapr",
>> +                             (uint64_t)tcet->nb_table << tcet->page_shift);
>>  
>>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>>  
>> @@ -191,17 +190,11 @@ MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet)
>>      return &tcet->iommu;
>>  }
>>  
>> -void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass)
>> -{
>> -    tcet->bypass = bypass;
>> -}
>> -
>>  static void spapr_tce_reset(DeviceState *dev)
>>  {
>>      sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>>      size_t table_size = tcet->nb_table * sizeof(uint64_t);
>>  
>> -    tcet->bypass = false;
>>      memset(tcet->table, 0, table_size);
>>  }
>>  
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index dc9e46a..b65eb11 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -322,6 +322,18 @@ static void spapr_vio_quiesce_one(VIOsPAPRDevice *dev)
>>      free_crq(dev);
>>  }
>>  
>> +static void spapr_vio_set_bypass(VIOsPAPRDevice *dev, bool bypass)
>> +{
>> +    if (!dev->tcet) {
>> +        return;
>> +    }
>> +
>> +    memory_region_set_enabled(&dev->mrbypass, bypass);
>> +    memory_region_set_enabled(spapr_tce_get_iommu(dev->tcet), !bypass);
>> +
>> +    dev->tcet->bypass = bypass;
>> +}
>> +
>>  static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>                                  uint32_t token,
>>                                  uint32_t nargs, target_ulong args,
>> @@ -348,7 +360,7 @@ static void rtas_set_tce_bypass(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>>          return;
>>      }
>>  
>> -    spapr_tce_set_bypass(dev->tcet, !!enable);
>> +    spapr_vio_set_bypass(dev, !!enable);
>>  
>>      rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>  }
>> @@ -407,6 +419,7 @@ static void spapr_vio_busdev_reset(DeviceState *qdev)
>>  
>>      dev->signal_state = 0;
>>  
>> +    spapr_vio_set_bypass(dev, false);
>>      if (pc->reset) {
>>          pc->reset(dev);
>>      }
>> @@ -456,12 +469,22 @@ static int spapr_vio_busdev_init(DeviceState *qdev)
>>  
>>      if (pc->rtce_window_size) {
>>          uint32_t liobn = SPAPR_VIO_BASE_LIOBN | dev->reg;
>> +
>> +        memory_region_init(&dev->mrroot, OBJECT(dev), "iommu-spapr-root",
>> +                           ram_size);
>> +        memory_region_init_alias(&dev->mrbypass, OBJECT(dev),
>> +                                 "iommu-spapr-bypass", get_system_memory(),
>> +                                 0, ram_size);
>> +        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);
>> -        address_space_init(&dev->as, spapr_tce_get_iommu(dev->tcet), qdev->id);
>> +        memory_region_add_subregion_overlap(&dev->mrroot, 0,
>> +                                            spapr_tce_get_iommu(dev->tcet), 2);
>>      }
>>  
>>      return pc->init(dev);
>> @@ -541,6 +564,17 @@ static void spapr_vio_bridge_class_init(ObjectClass *klass, void *data)
>>      k->init = spapr_vio_bridge_init;
>>  }
>>  
>> +static int spapr_vio_post_load(void *opaque, int version_id)
>> +{
>> +    VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(opaque);
>> +
>> +    if (dev->tcet) {
>> +        spapr_vio_set_bypass(dev, dev->tcet->bypass);
> 
> 
> Oh. Just realized that if VIOsPAPRDevice migrated before sPAPRTCETable,
> @bypass will always be false. So please ignore this patch for now :(
> 
> 
> 
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static const TypeInfo spapr_vio_bridge_info = {
>>      .name          = "spapr-vio-bridge",
>>      .parent        = TYPE_SYS_BUS_DEVICE,
>> @@ -552,6 +586,7 @@ const VMStateDescription vmstate_spapr_vio = {
>>      .name = "spapr_vio",
>>      .version_id = 1,
>>      .minimum_version_id = 1,
>> +    .post_load = spapr_vio_post_load,
>>      .fields = (VMStateField[]) {
>>          /* Sanity check */
>>          VMSTATE_UINT32_EQUAL(reg, VIOsPAPRDevice),
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 716bff4..ebc7e51 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -475,7 +475,6 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn,
>>                                     uint32_t nb_table,
>>                                     bool vfio_accel);
>>  MemoryRegion *spapr_tce_get_iommu(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,
>> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
>> index 46edc2a..6ad55d1 100644
>> --- a/include/hw/ppc/spapr_vio.h
>> +++ b/include/hw/ppc/spapr_vio.h
>> @@ -64,6 +64,8 @@ struct VIOsPAPRDevice {
>>      target_ulong signal_state;
>>      VIOsPAPR_CRQ crq;
>>      AddressSpace as;
>> +    MemoryRegion mrroot;
>> +    MemoryRegion mrbypass;
>>      sPAPRTCETable *tcet;
>>  };
>>  
>>
> 
> 


I believe doing something like this is way too disguising because of
tobj->parent?



--- a/hw/ppc/spapr_iommu.c
+++ b/hw/ppc/spapr_iommu.c
@@ -25,6 +25,7 @@
 #include "trace.h"

 #include "hw/ppc/spapr.h"
+#include "hw/ppc/spapr_vio.h"

 #include <libfdt.h>

@@ -88,10 +89,26 @@ static IOMMUTLBEntry
spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
     return ret;
 }

+static int spapr_tce_table_post_load(void *opaque, int version_id)
+{
+    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
+    Object *tobj = OBJECT(tcet);
+    Object *vobj = object_dynamic_cast(tobj->parent, TYPE_VIO_SPAPR_DEVICE);
+
+    if (!vobj) {
+        return 0;
+    }
+
+    spapr_vio_set_bypass((VIOsPAPRDevice *) vobj, tcet->bypass);
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_spapr_tce_table = {
     .name = "spapr_iommu",
     .version_id = 2,
     .minimum_version_id = 2,
+    .post_load = spapr_tce_table_post_load,
     .fields      = (VMStateField []) {
         /* Sanity check */
         VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),



-- 
Alexey

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[David Gibson]

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

On Tue, Jan 27, 2015 at 05:13:32PM +1100, Alexey Kardashevskiy wrote:
> Instead of tweaking a TCE table device by adding there a bypass flag,
> let's add an alias to RAM and IOMMU memory region, and enable/disable
> those according to the selected bypass mode.
> This way IOMMU memory region can have size of the actual window rather
> than ram_size which is essential for upcoming DDW support.
> 
> This moves bypass logic to VIO layer and keeps @bypass flag in TCE table
> for migration compatibility only. This replaces spapr_tce_set_bypass()
> calls with explicit assignment to avoid confusion as the function could
> do something more that just syncing the @bypass flag.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v2:
> * kept @bypass in sPAPRTCETable not to break migration

So, the bypass field definition in the struct should probably get a
comment explaining how it's only used for migration compatibility now.

Apart from that:

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

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[David Gibson]

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

On Wed, Jan 28, 2015 at 07:49:48PM +1100, Alexey Kardashevskiy wrote:
> On 01/28/2015 08:21 AM, Alexey Kardashevskiy wrote:
> > On 01/27/2015 05:13 PM, Alexey Kardashevskiy wrote:
[snip]
> >> diff --git a/include/hw/ppc/spapr_vio.h b/include/hw/ppc/spapr_vio.h
> >> index 46edc2a..6ad55d1 100644
> >> --- a/include/hw/ppc/spapr_vio.h
> >> +++ b/include/hw/ppc/spapr_vio.h
> >> @@ -64,6 +64,8 @@ struct VIOsPAPRDevice {
> >>      target_ulong signal_state;
> >>      VIOsPAPR_CRQ crq;
> >>      AddressSpace as;
> >> +    MemoryRegion mrroot;
> >> +    MemoryRegion mrbypass;
> >>      sPAPRTCETable *tcet;
> >>  };
> >>  
> >>
> > 
> > 
> 
> 
> I believe doing something like this is way too disguising because of
> tobj->parent?

It's kinda ugly, but it's the best way I can think of.

So, I think this is a reasonable approach, but it does need a comment
saying why the hack is necessary.

> 
> 
> 
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -25,6 +25,7 @@
>  #include "trace.h"
> 
>  #include "hw/ppc/spapr.h"
> +#include "hw/ppc/spapr_vio.h"
> 
>  #include <libfdt.h>
> 
> @@ -88,10 +89,26 @@ static IOMMUTLBEntry
> spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr,
>      return ret;
>  }
> 
> +static int spapr_tce_table_post_load(void *opaque, int version_id)
> +{
> +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> +    Object *tobj = OBJECT(tcet);
> +    Object *vobj = object_dynamic_cast(tobj->parent, TYPE_VIO_SPAPR_DEVICE);
> +
> +    if (!vobj) {
> +        return 0;
> +    }
> +
> +    spapr_vio_set_bypass((VIOsPAPRDevice *) vobj, tcet->bypass);
> +
> +    return 0;
> +}
> +
>  static const VMStateDescription vmstate_spapr_tce_table = {
>      .name = "spapr_iommu",
>      .version_id = 2,
>      .minimum_version_id = 2,
> +    .post_load = spapr_tce_table_post_load,
>      .fields      = (VMStateField []) {
>          /* Sanity check */
>          VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),
> 
> 
> 

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

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[Alexey Kardashevskiy]

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

On 01/29/2015 11:31 AM, David Gibson wrote:
> On Wed, Jan 28, 2015 at 07:49:48PM +1100, Alexey Kardashevskiy wrote:
>> On 01/28/2015 08:21 AM, Alexey Kardashevskiy wrote:
>>> On 01/27/2015 05:13 PM, Alexey Kardashevskiy wrote:
> [snip]
>>>> diff --git a/include/hw/ppc/spapr_vio.h
>>>> b/include/hw/ppc/spapr_vio.h index 46edc2a..6ad55d1 100644 ---
>>>> a/include/hw/ppc/spapr_vio.h +++ b/include/hw/ppc/spapr_vio.h @@
>>>> -64,6 +64,8 @@ struct VIOsPAPRDevice { target_ulong
>>>> signal_state; VIOsPAPR_CRQ crq; AddressSpace as; +
>>>> MemoryRegion mrroot; +    MemoryRegion mrbypass; sPAPRTCETable
>>>> *tcet; };
>>>> 
>>>> 
>>> 
>>> 
>> 
>> 
>> I believe doing something like this is way too disguising because
>> of tobj->parent?
> 
> It's kinda ugly, but it's the best way I can think of.



Better than having VIOsPAPRDevice pointer in sPAPRTCETable? I would
expect a object_get_parent() helper to exist but it is not there and I
believe this is for a reason (may be it will be get rid of some time
later)...


> So, I think this is a reasonable approach, but it does need a comment 
> saying why the hack is necessary.
> 
>> 
>> 
>> 
>> --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -25,6 +25,7
>> @@ #include "trace.h"
>> 
>> #include "hw/ppc/spapr.h" +#include "hw/ppc/spapr_vio.h"
>> 
>> #include <libfdt.h>
>> 
>> @@ -88,10 +89,26 @@ static IOMMUTLBEntry 
>> spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr, return
>> ret; }
>> 
>> +static int spapr_tce_table_post_load(void *opaque, int version_id) 
>> +{ +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); +    Object
>> *tobj = OBJECT(tcet); +    Object *vobj =
>> object_dynamic_cast(tobj->parent, TYPE_VIO_SPAPR_DEVICE); + +    if
>> (!vobj) { +        return 0; +    } + +
>> spapr_vio_set_bypass((VIOsPAPRDevice *) vobj, tcet->bypass); + +
>> return 0; +} + static const VMStateDescription
>> vmstate_spapr_tce_table = { .name = "spapr_iommu", .version_id = 2, 
>> .minimum_version_id = 2, +    .post_load =
>> spapr_tce_table_post_load, .fields      = (VMStateField []) { /*
>> Sanity check */ VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),
>> 
>> 
>> 
> 


- -- 
Alexey
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJUyYNtAAoJEIYTPdgrwSC54t0P/0SGpr+UX7qlwKKs89bwVc//
dzUS4wPXeH7zYkLughLprEh3uINY9x7z8euqSJg1wPoh54F6QX9PDb8ipKavie+a
NlxDeqVNh+eWLR/rI1OxMbg/PcQSnqCN13EHJLiz69xkdEZsNReLKUFueMrk4Grl
H4l3+GDVx6HbBSTWawOmcpN4Bcz0/nRgsYf6RytIk9ZP60zplbNkrCt3jwPV87O3
/+g73eBvpusiY+8NIEPbzAEssMcTj+JDYB5Dy5JlsOYewQFyXiiXaVSdRnNIPB4o
tSaD93LYvkNEYXACJ7awsfWWx45tvWjt0GtO0YUU2vGD/cAWjsW8Q76f1DjNZdvD
wZwhJF0cieaRnlrNMlCE199DyxcKDqDsENEnRUsySE8qq1uHGXo1dlyn+rA3Spc7
jEfwxkmtpHf4l9JAB01hGmq37eYPPKS9uyjXwqtKZuDea0ObVEtsmXiSZnOho82D
+jJzr1KUDUMFcWw7ZWxXJ4I1wCt0u3krRWtO4vD0RXnjWgKUM8MbuNGFBFUt1qtF
8M1KTrm06jW4w0h19nBSP2ed7j2ObE2KJebE7I/tl//gxgQo4PmR/cwZv8EXpHCC
KNz6SDvUSboJ9i02Gxkk7GWKYPbGthQkrICxTfVwKGyms4O58/KiH8T/Koey/BLJ
92imwmKo3VtddbQfe5Ay
=dlFh
-----END PGP SIGNATURE-----

Re: [Qemu-devel] [PATCH v2] spapr_vio/spapr_iommu: Move VIO bypass where it belongs

[David Gibson]

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

On Thu, Jan 29, 2015 at 11:48:46AM +1100, Alexey Kardashevskiy wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 01/29/2015 11:31 AM, David Gibson wrote:
> > On Wed, Jan 28, 2015 at 07:49:48PM +1100, Alexey Kardashevskiy wrote:
> >> On 01/28/2015 08:21 AM, Alexey Kardashevskiy wrote:
> >>> On 01/27/2015 05:13 PM, Alexey Kardashevskiy wrote:
> > [snip]
> >>>> diff --git a/include/hw/ppc/spapr_vio.h
> >>>> b/include/hw/ppc/spapr_vio.h index 46edc2a..6ad55d1 100644 ---
> >>>> a/include/hw/ppc/spapr_vio.h +++ b/include/hw/ppc/spapr_vio.h @@
> >>>> -64,6 +64,8 @@ struct VIOsPAPRDevice { target_ulong
> >>>> signal_state; VIOsPAPR_CRQ crq; AddressSpace as; +
> >>>> MemoryRegion mrroot; +    MemoryRegion mrbypass; sPAPRTCETable
> >>>> *tcet; };
> >>>> 
> >>>> 
> >>> 
> >>> 
> >> 
> >> 
> >> I believe doing something like this is way too disguising because
> >> of tobj->parent?
> > 
> > It's kinda ugly, but it's the best way I can think of.
> 
> 
> 
> Better than having VIOsPAPRDevice pointer in sPAPRTCETable? I would
> expect a object_get_parent() helper to exist but it is not there and I
> believe this is for a reason (may be it will be get rid of some time
> later)...


Uh.. I think someone who knows qom better than me will have to answer
that..

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