On Mon, Jul 08, 2013 at 01:39:26PM -0500, Anthony Liguori wrote:
> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
> 
> > From: David Gibson <david@gibson.dropbear.id.au>
> >
> > This patch adds the necessary VMStateDescription information to save the
> > state of PAPR TCE tables (that is, the PAPR specified IOMMU).
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> >  hw/ppc/spapr_iommu.c |   25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> > index 91bc8e4..ba1f7b6 100644
> > --- a/hw/ppc/spapr_iommu.c
> > +++ b/hw/ppc/spapr_iommu.c
> > @@ -112,6 +112,25 @@ static IOMMUTLBEntry spapr_tce_translate_iommu(MemoryRegion *iommu, hwaddr addr)
> >      };
> >  }
> >  
> > +static const VMStateDescription vmstate_spapr_tce_table = {
> > +    .name = "spapr_iommu",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .minimum_version_id_old = 1,
> > +    .fields      = (VMStateField []) {
> > +        /* Sanity check */
> > +        VMSTATE_UINT32_EQUAL(liobn, sPAPRTCETable),
> > +        VMSTATE_UINT32_EQUAL(window_size, sPAPRTCETable),
> > +
> > +        /* IOMMU state */
> > +        VMSTATE_BOOL(bypass, sPAPRTCETable),
> > +        VMSTATE_VBUFFER_DIVIDE(table, sPAPRTCETable, 0, NULL, 0, window_size,
> > +                               SPAPR_TCE_PAGE_SIZE /
> > sizeof(sPAPRTCE)),
> 
> Not endian safe.  I really don't get the divide bit at all either.

So, the actual bug is that we're currently storing the TCE table
native endian, whereas it should be stored big endan always.
 
> > +
> > +        VMSTATE_END_OF_LIST()
> > +    },
> > +};
> > +
> >  static MemoryRegionIOMMUOps spapr_iommu_ops = {
> >      .translate = spapr_tce_translate_iommu,
> >  };
> > @@ -156,6 +175,8 @@ sPAPRTCETable *spapr_tce_new_table(uint32_t liobn, size_t window_size)
> >  
> >      QLIST_INSERT_HEAD(&spapr_tce_tables, tcet, list);
> >  
> > +    vmstate_register(NULL, tcet->liobn, &vmstate_spapr_tce_table, tcet);
> > +
> 
> If you need to add these, then you need to do more QOM conversion.

Again, it's not clear how this should be QOMed.  Child of the device
constructing the TCE table?  But since that can often be a bus bridge,
wouldn't the TCE table instances get confused with the real bus
devices.

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