[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambiv
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/8] virtio_legacy_get_byteswap: endian-ambivalent targets using legacy virtio |
Date: |
Fri, 28 Mar 2014 20:00:27 +0100 |
On Fri, 28 Mar 2014 18:59:22 +0100
Andreas Färber <address@hidden> wrote:
> 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?
>
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 <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>
>
Cheers.
--
Gregory Kurz address@hidden
address@hidden
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.