Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio

[Andreas Färber]

Am 28.03.2014 11:57, schrieb Greg Kurz:
> From: Rusty Russell <address@hidden>
> 
> virtio data structures are defined as "target endian", which assumes
> that's a fixed value.  In fact, that actually means it's platform-specific.
> The OASIS virtio 1.0 spec will fix this, by making all little endian.
> 
> We need to support both implementations and we want to share as much code
> as possible.
> 
> A good way to do it is to introduce a per-device boolean property to tell
> memory accessors whether they should swap bytes or not. This flag should
> be set at device reset time, because:
> - endianness won't change while the device is in use, and if we reboot
>   into a different endianness, a new device reset will occur
> - as suggested by Alexander Graf, we can keep all the logic to set the
>   property in a single place and share all the virtio memory accessors
>   between the two implementations
> 
> For legacy devices, we rely on a per-platform hook to set the flag. The
> virtio 1.0 implementation will just have to add some more logic in
> virtio_reset() instead of calling the hook:
> 
> if (vdev->legacy) {
>    vdev->needs_byteswap = virtio_legacy_get_byteswap();
> } else {
> #ifdef HOST_WORDS_BIGENDIAN
>    vdev->needs_byteswap = true;
> #else
>    vdev->needs_byteswap = false;
> #endif
> }
> 
> The needs_byteswap flag is preserved accross migrations.

"across"

Why? :) For all targets except ppc this field does not change during
runtime. Do you intend to support ppc64 <-> ppc64le migration, i.e.
protection against changing HOST_WORDS_BIGENDIAN?

Since you're setting the field on reset rather than in instance_init or
realize, resetting the device on one host with changing ILE may lead to
weird endianness settings: Devices are initially reset before the VM
starts. ILE will always be Big Endian then IIUC, since all PowerPCCPU
models default to Big Endian and SLOF runs Big Endian. SLOF might use a
virtio-blk device to boot from. We then boot into SLES12 ppc64le - ILE
indicates Little Endian now. As soon as the guest triggers a reset of
the device, such as by resetting the whole PCI bus, endianness changes
to Little Endian. Now you indeed have an endianness on the source that
is different from that of the newly Big Endian reset device on the
destination. Is this desired, or did you accidentally initialize the
flag in the wrong place?

If we do need it, maybe you can place the field into a subsection to
avoid imposing it on x86?

Regards,
Andreas

> 
> Signed-off-by: Rusty Russell <address@hidden>
> [ fixed checkpatch.pl error with the virtio_byteswap initialisation,
>   ldq_phys() API change,
>   relicensed virtio-access.h to GPLv2+ on Rusty's request,
>   introduce a per-device needs_byteswap flag,
>   add VirtIODevice * arg to virtio helpers,
>   rename virtio_get_byteswap to virtio_legacy_get_byteswap,
>   Greg Kurz <address@hidden> ]
> Signed-off-by: Greg Kurz <address@hidden>

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg