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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Fri, 28 Mar 2014 18:59:22 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 28.03.2014 11:57, schrieb Greg Kurz:
> > From: Rusty Russell <rusty@rustcorp.com.au>
> > 
> > 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?
> 

Because we only set this property at virtio_reset() time... how can
we ensure it still has the correct value after a migration then ?

And no, I don't intend to support cross-endian migration... The
only endianness change that we can support is rebooting into a
different endianness.

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

Hmmm... the assumption is that ALL virtio devices get reset after the guest
kernel switches to LE. Are you saying this is not the case if SLOF uses
a virtio-blk device to boot from ? This device would be handed over to
the guest kernel to be used as is ? If yes, then I don't see how we can
cope with that... The way legacy virtio is implemented, we cannot
reasonably support an endianness change without fully reseting the device.

I guess this is the main motivation behind virtio 1.0 :)

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

I think it is needed anyway as long as we want to support a ppc64 guest
that can change endianness and uses legacy virtio devices, even with
a x86 host.

> Regards,
> Andreas
> 
> > 
> > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> > [ 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 <gkurz@linux.vnet.ibm.com> ]
> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> 

Cheers.


-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.