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

[David Gibson]

On Tue, Jul 09, 2013 at 10:22:39AM -0500, Anthony Liguori wrote:
> 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.
> 
> 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.

Hrm.  I suppose it could be fixed at either end.  The idea was that
the table array would contain exactly the same bytes as would be
present in physical memory on a real bare-metal system, which seems
like a generally nice property.

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

pgp_kwNrSTZlm.pgp
Description: PGP signature