From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:55257)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aliguori@us.ibm.com>) id 1UwJnE-0002Pl-S2
	for qemu-devel@nongnu.org; Mon, 08 Jul 2013 18:20:26 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <aliguori@us.ibm.com>) id 1UwJnD-0000lO-QO
	for qemu-devel@nongnu.org; Mon, 08 Jul 2013 18:20:24 -0400
Received: from e33.co.us.ibm.com ([32.97.110.151]:51126)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <aliguori@us.ibm.com>) id 1UwJnD-0000lK-K1
	for qemu-devel@nongnu.org; Mon, 08 Jul 2013 18:20:23 -0400
Received: from /spool/local
	by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only!
	Violators will be prosecuted
	for <qemu-devel@nongnu.org> from <aliguori@us.ibm.com>;
	Mon, 8 Jul 2013 22:20:01 -0000
From: Anthony Liguori <aliguori@us.ibm.com>
In-Reply-To: <1373319954.4446.52.camel@pasglop>
References: <1372315560-5478-1-git-send-email-aik@ozlabs.ru>
	<1372315560-5478-9-git-send-email-aik@ozlabs.ru>
	<87sizozyjl.fsf@codemonkey.ws> <1373319954.4446.52.camel@pasglop>
Date: Mon, 08 Jul 2013 17:15:56 -0500
Message-ID: <87sizosnoj.fsf@codemonkey.ws>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Subject: Re: [Qemu-devel] [PATCH 08/17] pseries: savevm support for PAPR TCE
	tables
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Alexey Kardashevskiy <aik@ozlabs.ru>, Alexander Graf <agraf@suse.de>, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>, Paul Mackerras <paulus@samba.org>, David Gibson <david@gibson.dropbear.id.au>

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Mon, 2013-07-08 at 13:39 -0500, Anthony Liguori wrote:
>> > +    .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.
>
> What do you mean by not endian safe ? The TCE table is a well defined format,
> it's always big endian regardless of the endianness of either host or
> guest.

VMSTATE_VBUFFER is essentially:

  write(fd, s->table, byte_size_of_table);

It treats whatever is given it as a sized data blob.

table is an array of sPAPRTCE which is just a struct wrapper around a
uint64_t value (the tce entry).

Those entries are set via the h_put_tce hcall through a simple
assignment:

> static target_ulong put_tce_emu(sPAPRTCETable *tcet, target_ulong ioba,
>                                target_ulong tce)
> {
>     ...
> 
>     tcep = tcet->table + (ioba >> SPAPR_TCE_PAGE_SHIFT);
>     tcep->tce = tce;
>  ...
>
> static target_ulong h_put_tce(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                               target_ulong opcode, target_ulong *args)
> {
>     ...
>     target_ulong tce = args[2];
>     sPAPRTCETable *tcet = spapr_tce_find_by_liobn(liobn);
> 
>     ...
> 
>     if (tcet) {
>         return put_tce_emu(tcet, ioba, tce);
>     }

Hypercall arguments are passed in CPU endianness so what's being stored
in the tce table is CPU endianness.

Since VBUFFER just does a blind write() of the full array of uint64s,
what goes on the wire will be CPU endianness.

So if you do a savevm on a little endian host and loadvm on a big endian
host, badness ensues.

The proper thing to do is use a VARRAY instead of a VBUFFER.  VARRAY
will handle endian because it treats the data as an array, not as an
opaque buffer.

Regards,

Anthony Liguori

>
> Cheers,
> Ben.