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

[Anthony Liguori <aliguori@us.ibm.com>]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> 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?

Really nothing should be in spapr_tce_new_table().   It should strictly
be an helper function to create a device and set properties (via
qdev_prop_set..).  We really should make liobn and window_size proper
properties.

Regards,

Anthony Liguori

>
> 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