Re: [Qemu-devel] [PATCH 05/11] spapr-tce: make sPAPRTCETable a proper device

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 07/16/2013 01:11 AM, Anthony Liguori wrote:
> Model TCE tables as a device that's hooked up as a child object to
> the owner.  Besides the code cleanup, we get a few nice benefits:
> 
> 1) free actually works now (it was dead code before)
> 
> 2) the TCE information is visible in the device tree
> 
> 3) we can expose table information as properties such that if we
>    change the window_size, we can use globals to keep migration
>    working.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> [dwg: pseries: savevm support for PAPR TCE tables]
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/ppc/spapr.c         |   3 -
>  hw/ppc/spapr_iommu.c   | 145 ++++++++++++++++++++++++++++++++-----------------
>  hw/ppc/spapr_pci.c     |   2 +-
>  hw/ppc/spapr_vio.c     |   2 +-
>  include/hw/ppc/spapr.h |  23 +++++---
>  5 files changed, 114 insertions(+), 61 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 48ae092..e340708 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -848,9 +848,6 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)
>      /* Set up EPOW events infrastructure */
>      spapr_events_init(spapr);
>  
> -    /* Set up IOMMU */
> -    spapr_iommu_init();
> -
>      /* Set up VIO bus */
>      spapr->vio_bus = spapr_vio_bus_init();
>  
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 89b33a5..709cc34 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -36,17 +36,6 @@ enum sPAPRTCEAccess {
>      SPAPR_TCE_RW = 3,
>  };
>  
> -struct sPAPRTCETable {
> -    uint32_t liobn;
> -    uint32_t window_size;
> -    sPAPRTCE *table;
> -    bool bypass;
> -    int fd;
> -    MemoryRegion iommu;
> -    QLIST_ENTRY(sPAPRTCETable) list;
> -};
> -
> -
>  QLIST_HEAD(spapr_tce_tables, sPAPRTCETable) spapr_tce_tables;
>  
>  static sPAPRTCETable *spapr_tce_find_by_liobn(uint32_t liobn)
> @@ -96,7 +85,7 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr)
>          return (IOMMUTLBEntry) { .perm = IOMMU_NONE };
>      }
>  
> -    tce = tcet->table[addr >> SPAPR_TCE_PAGE_SHIFT].tce;
> +    tce = tcet->table[addr >> SPAPR_TCE_PAGE_SHIFT];
>  
>  #ifdef DEBUG_TCE
>      fprintf(stderr, " ->  *paddr=0x%llx, *len=0x%llx\n",
> @@ -112,37 +101,51 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr)
>      };
>  }
>  
> +static int spapr_tce_table_pre_load(void *opaque)
> +{
> +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque);
> +
> +    tcet->nb_table = tcet->window_size >> SPAPR_TCE_PAGE_SHIFT;
> +
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_spapr_tce_table = {
> +    .name = "spapr_iommu",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .pre_load = spapr_tce_table_pre_load,
> +    .fields      = (VMStateField []) {
> +        /* Sanity check */
> +        VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),
> +        VMSTATE_UINT32_EQUAL(window_size, sPAPRTCETable),
> +
> +        /* IOMMU state */
> +        VMSTATE_BOOL(bypass, sPAPRTCETable),
> +        VMSTATE_VARRAY_UINT32(table, sPAPRTCETable, nb_table, 0, vmstate_info_uint64, uint64_t),
> +
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  static MemoryRegionIOMMUOps spapr_iommu_ops = {
>      .translate = spapr_tce_translate_iommu,
>  };
>  
> -sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, size_t window_size)
> +static int spapr_tce_table_realize(DeviceState *dev)
>  {
> -    sPAPRTCETable *tcet;
> -
> -    if (spapr_tce_find_by_liobn(liobn)) {
> -        fprintf(stderr, "Attempted to create TCE table with duplicate"
> -                " LIOBN 0x%x\n", liobn);
> -        return NULL;
> -    }
> -
> -    if (!window_size) {
> -        return NULL;
> -    }
> -
> -    tcet = g_malloc0(sizeof(*tcet));
> -    tcet->liobn = liobn;
> -    tcet->window_size = window_size;
> +    sPAPRTCETable *tcet = SPAPR_TCE_TABLE(dev);
>  
>      if (kvm_enabled()) {
> -        tcet->table = kvmppc_create_spapr_tce(liobn,
> -                                              window_size,
> +        tcet->table = kvmppc_create_spapr_tce(tcet->liobn,
> +                                              tcet->window_size,
>                                                &tcet->fd);
>      }
>  
>      if (!tcet->table) {
> -        size_t table_size = (window_size >> SPAPR_TCE_PAGE_SHIFT)
> -            * sizeof(sPAPRTCE);
> +        size_t table_size = (tcet->window_size >> SPAPR_TCE_PAGE_SHIFT)
> +            * sizeof(uint64_t);
>          tcet->table = g_malloc0(table_size);
>      }
>  
> @@ -151,16 +154,43 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, size_t wi
>              "table @ %p, fd=%d\n", tcet, liobn, tcet->table, tcet->fd);
>  #endif
>  
> -    memory_region_init_iommu(&tcet->iommu, OBJECT(owner), &spapr_iommu_ops,
> +    memory_region_init_iommu(&tcet->iommu, OBJECT(dev), &spapr_iommu_ops,
>                               "iommu-spapr", UINT64_MAX);
>  
>      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
>  
> +    return 0;
> +}
> +
> +sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, size_t window_size)
> +{
> +    sPAPRTCETable *tcet;
> +
> +    if (spapr_tce_find_by_liobn(liobn)) {
> +        fprintf(stderr, "Attempted to create TCE table with duplicate"
> +                " LIOBN 0x%x\n", liobn);
> +        return NULL;
> +    }
> +
> +    if (!window_size) {
> +        return NULL;
> +    }
> +
> +    tcet = SPAPR_TCE_TABLE(object_new(TYPE_SPAPR_TCE_TABLE));
> +    tcet->liobn = liobn;
> +    tcet->window_size = window_size;



I am trying to understand the QOM. How do you understand what
initialization should go to .realize and what should stay in
spapr_tce_new_table?

In this particular case having the .realize implementation does not make
much sense to me. If you made .liobn and .window_size members of
sPAPRTCETable then it would be ok but you did not. What do I miss? Thanks.



-- 
Alexey