Alexey Kardashevskiy <a...@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 <da...@gibson.dropbear.id.au> >> [dwg: pseries: savevm support for PAPR TCE tables] >> Signed-off-by: Anthony Liguori <aligu...@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