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