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

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

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.