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

[Anthony Liguori]

David Gibson <address@hidden> writes:

> On Mon, Jul 08, 2013 at 01:39:26PM -0500, Anthony Liguori wrote:
>> Alexey Kardashevskiy <address@hidden> writes:
>> 
>> > From: David Gibson <address@hidden>
>> >
>> > 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 <address@hidden>
>> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> > ---
>> >  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.

Only build tested.

https://github.com/aliguori/qemu/commit/a47a391c875a69f203110811c730877da12f5b14

I'll put together a patch series once I have a chance to test properly.

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