Re: [Qemu-devel] [PATCH 08/17] pseries: savevm support for PAPR TCE tables

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

David Gibson <david@gibson.dropbear.id.au> writes:

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

Why?  There are no guest visible byte accesses done to the table
AFAICT.  Everything is done as words and there's quite a lot of math
done to the entries.

It seems like native endian is the right internal representation.

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

I can't apply this series (I'm not sure what tree it's against), but if
Alexey pushes a branch somewhere I can do the QOM conversions to
demonstrate.

Regards,

Anthony Liguori

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