| Message ID | 20140328105717.21018.17649.stgit@bahia.local |
|---|---|
| State | New |
| Headers | show |
On Fri, 28 Mar 2014 11:57:17 +0100 Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > 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. > > 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> > --- > hw/virtio/virtio.c | 5 + > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++ > include/hw/virtio/virtio.h | 3 + > stubs/Makefile.objs | 1 > stubs/virtio_get_byteswap.c | 6 ++ > 5 files changed, 154 insertions(+) > create mode 100644 include/hw/virtio/virtio-access.h > create mode 100644 stubs/virtio_get_byteswap.c > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index aeabf3a..24b565f 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -19,6 +19,7 @@ > #include "hw/virtio/virtio.h" > #include "qemu/atomic.h" > #include "hw/virtio/virtio-bus.h" > +#include "hw/virtio/virtio-access.h" > > /* > * The alignment to use between consumer and producer parts of vring. > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque) > > virtio_set_status(vdev, 0); > > + vdev->needs_byteswap = virtio_legacy_get_byteswap(); > + > if (k->reset) { > k->reset(vdev); > } > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > qemu_put_8s(f, &vdev->status); > qemu_put_8s(f, &vdev->isr); > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap); > qemu_put_be16s(f, &vdev->queue_sel); > qemu_put_be32s(f, &vdev->guest_features); > qemu_put_be32(f, vdev->config_len); > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > > qemu_get_8s(f, &vdev->status); > qemu_get_8s(f, &vdev->isr); > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap); > qemu_get_be16s(f, &vdev->queue_sel); > qemu_get_be32s(f, &features); Two remarks here: 1) You've declared needs_byteswap as "bool", but in above code you assume that the "bool" type is implemented as a single byte. That's most likely true on most system, but AFAIK it's specific to the compiler. So for portable code, I think you should either change the type of needs_byteswap or you should use a temporary uint8_t variable here instead. 2) You're changing the layout of the saved data ... don't you also have to increase the version numbers in that case, too? (e.g. change the version id for the register_savevm call in virtio-blk.c, virtio-net.c, etc.)? Thomas
On Fri, 28 Mar 2014 15:15:46 +0100 Thomas Huth <thuth@linux.vnet.ibm.com> wrote: > On Fri, 28 Mar 2014 11:57:17 +0100 > Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > > > 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. > > > > 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> > > --- > > hw/virtio/virtio.c | 5 + > > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++ > > include/hw/virtio/virtio.h | 3 + > > stubs/Makefile.objs | 1 > > stubs/virtio_get_byteswap.c | 6 ++ > > 5 files changed, 154 insertions(+) > > create mode 100644 include/hw/virtio/virtio-access.h > > create mode 100644 stubs/virtio_get_byteswap.c > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index aeabf3a..24b565f 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -19,6 +19,7 @@ > > #include "hw/virtio/virtio.h" > > #include "qemu/atomic.h" > > #include "hw/virtio/virtio-bus.h" > > +#include "hw/virtio/virtio-access.h" > > > > /* > > * The alignment to use between consumer and producer parts of vring. > > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque) > > > > virtio_set_status(vdev, 0); > > > > + vdev->needs_byteswap = virtio_legacy_get_byteswap(); > > + > > if (k->reset) { > > k->reset(vdev); > > } > > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > > > qemu_put_8s(f, &vdev->status); > > qemu_put_8s(f, &vdev->isr); > > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap); > > qemu_put_be16s(f, &vdev->queue_sel); > > qemu_put_be32s(f, &vdev->guest_features); > > qemu_put_be32(f, vdev->config_len); > > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > > > > qemu_get_8s(f, &vdev->status); > > qemu_get_8s(f, &vdev->isr); > > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap); > > qemu_get_be16s(f, &vdev->queue_sel); > > qemu_get_be32s(f, &features); > > Two remarks here: > > 1) You've declared needs_byteswap as "bool", but in above code you > assume that the "bool" type is implemented as a single byte. That's > most likely true on most system, but AFAIK it's specific to the > compiler. So for portable code, I think you should either change the > type of needs_byteswap or you should use a temporary uint8_t variable > here instead. > Thomas, I guess turning needs_byteswap into a uint8_t is ok and less intrusive for the code. > 2) You're changing the layout of the saved data ... don't you also have > to increase the version numbers in that case, too? (e.g. change the > version id for the register_savevm call in virtio-blk.c, virtio-net.c, > etc.)? > Oops, my bad ! :-\ > Thomas Thanks for your time. Cheers.
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? 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 <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>
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.
On 03/28/2014 11:57 AM, Greg Kurz wrote: > 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. > > 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> > --- > hw/virtio/virtio.c | 5 + > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++ > include/hw/virtio/virtio.h | 3 + > stubs/Makefile.objs | 1 > stubs/virtio_get_byteswap.c | 6 ++ > 5 files changed, 154 insertions(+) > create mode 100644 include/hw/virtio/virtio-access.h > create mode 100644 stubs/virtio_get_byteswap.c > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index aeabf3a..24b565f 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -19,6 +19,7 @@ > #include "hw/virtio/virtio.h" > #include "qemu/atomic.h" > #include "hw/virtio/virtio-bus.h" > +#include "hw/virtio/virtio-access.h" > > /* > * The alignment to use between consumer and producer parts of vring. > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque) > > virtio_set_status(vdev, 0); > > + vdev->needs_byteswap = virtio_legacy_get_byteswap(); > + > if (k->reset) { > k->reset(vdev); > } > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > qemu_put_8s(f, &vdev->status); > qemu_put_8s(f, &vdev->isr); > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap); > qemu_put_be16s(f, &vdev->queue_sel); > qemu_put_be32s(f, &vdev->guest_features); > qemu_put_be32(f, vdev->config_len); > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > > qemu_get_8s(f, &vdev->status); > qemu_get_8s(f, &vdev->isr); > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap); > qemu_get_be16s(f, &vdev->queue_sel); > qemu_get_be32s(f, &features); > > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h > new file mode 100644 > index 0000000..70dd1e2 > --- /dev/null > +++ b/include/hw/virtio/virtio-access.h > @@ -0,0 +1,139 @@ > +/* > + * Virtio Accessor Support: In case your target can change endian. > + * > + * Copyright IBM, Corp. 2013 > + * > + * Authors: > + * Rusty Russell <rusty@au.ibm.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation, either version 2 of the License, or > + * (at your option) any later version. > + * > + */ > +#ifndef _QEMU_VIRTIO_ACCESS_H > +#define _QEMU_VIRTIO_ACCESS_H > +#include "hw/virtio/virtio.h" > + > +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap16(lduw_phys(as, pa)); > + } > + return lduw_phys(as, pa); > +} > + > +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap32(ldl_phys(as, pa)); > + } > + return ldl_phys(as, pa); > +} > + > +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap64(ldq_phys(as, pa)); > + } > + return ldq_phys(as, pa); > +} > + > +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + stw_phys(as, pa, bswap16(value)); > + } else { > + stw_phys(as, pa, value); > + } > +} > + > +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + stl_phys(as, pa, bswap32(value)); > + } else { > + stl_phys(as, pa, value); > + } > +} > + > +static inline void virtio_stw_p(void *ptr, uint16_t v, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + stw_p(ptr, bswap16(v)); > + } else { > + stw_p(ptr, v); > + } > +} > + > +static inline void virtio_stl_p(void *ptr, uint32_t v, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + stl_p(ptr, bswap32(v)); > + } else { > + stl_p(ptr, v); > + } > +} > + > +static inline void virtio_stq_p(void *ptr, uint64_t v, > + struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + stq_p(ptr, bswap64(v)); > + } else { > + stq_p(ptr, v); > + } > +} > + > +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap16(lduw_p(ptr)); > + } else { > + return lduw_p(ptr); > + } > +} > + > +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap32(ldl_p(ptr)); > + } else { > + return ldl_p(ptr); > + } > +} > + > +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap64(ldq_p(ptr)); > + } else { > + return ldq_p(ptr); > + } > +} > + > +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev) > +{ > + if (vdev->needs_byteswap) { > + return bswap32(tswap32(s)); > + } else { > + return tswap32(s); > + } > +} > + > +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev) > +{ > + tswap32s(s); > + if (vdev->needs_byteswap) { > + *s = bswap32(*s); > + } Couldn't you just reuse virtio_tswap32() here? Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't have any access to CPU endian specific RAM accessors. How about we introduce a flag like "is_bigendian" that we set depending on the CPU default endianness for legacy virtio. We can then change that flag accordingly. For the wrappers here, we could just use accessors like ldl_be_phys() or ldl_le_phys(). That should make things a lot easier to understand - and for virtio 1.0 we only need to set is_bigendian = false :). Alex
On Mon, 31 Mar 2014 16:50:55 +0200 Alexander Graf <agraf@suse.de> wrote: > On 03/28/2014 11:57 AM, Greg Kurz wrote: > > 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. > > > > 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> > > --- > > hw/virtio/virtio.c | 5 + > > include/hw/virtio/virtio-access.h | 139 +++++++++++++++++++++++++++++++++++++ > > include/hw/virtio/virtio.h | 3 + > > stubs/Makefile.objs | 1 > > stubs/virtio_get_byteswap.c | 6 ++ > > 5 files changed, 154 insertions(+) > > create mode 100644 include/hw/virtio/virtio-access.h > > create mode 100644 stubs/virtio_get_byteswap.c > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index aeabf3a..24b565f 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -19,6 +19,7 @@ > > #include "hw/virtio/virtio.h" > > #include "qemu/atomic.h" > > #include "hw/virtio/virtio-bus.h" > > +#include "hw/virtio/virtio-access.h" > > > > /* > > * The alignment to use between consumer and producer parts of vring. > > @@ -546,6 +547,8 @@ void virtio_reset(void *opaque) > > > > virtio_set_status(vdev, 0); > > > > + vdev->needs_byteswap = virtio_legacy_get_byteswap(); > > + > > if (k->reset) { > > k->reset(vdev); > > } > > @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) > > > > qemu_put_8s(f, &vdev->status); > > qemu_put_8s(f, &vdev->isr); > > + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap); > > qemu_put_be16s(f, &vdev->queue_sel); > > qemu_put_be32s(f, &vdev->guest_features); > > qemu_put_be32(f, vdev->config_len); > > @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) > > > > qemu_get_8s(f, &vdev->status); > > qemu_get_8s(f, &vdev->isr); > > + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap); > > qemu_get_be16s(f, &vdev->queue_sel); > > qemu_get_be32s(f, &features); > > > > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h > > new file mode 100644 > > index 0000000..70dd1e2 > > --- /dev/null > > +++ b/include/hw/virtio/virtio-access.h > > @@ -0,0 +1,139 @@ > > +/* > > + * Virtio Accessor Support: In case your target can change endian. > > + * > > + * Copyright IBM, Corp. 2013 > > + * > > + * Authors: > > + * Rusty Russell <rusty@au.ibm.com> > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation, either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + */ > > +#ifndef _QEMU_VIRTIO_ACCESS_H > > +#define _QEMU_VIRTIO_ACCESS_H > > +#include "hw/virtio/virtio.h" > > + > > +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap16(lduw_phys(as, pa)); > > + } > > + return lduw_phys(as, pa); > > +} > > + > > +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(ldl_phys(as, pa)); > > + } > > + return ldl_phys(as, pa); > > +} > > + > > +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap64(ldq_phys(as, pa)); > > + } > > + return ldq_phys(as, pa); > > +} > > + > > +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stw_phys(as, pa, bswap16(value)); > > + } else { > > + stw_phys(as, pa, value); > > + } > > +} > > + > > +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stl_phys(as, pa, bswap32(value)); > > + } else { > > + stl_phys(as, pa, value); > > + } > > +} > > + > > +static inline void virtio_stw_p(void *ptr, uint16_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stw_p(ptr, bswap16(v)); > > + } else { > > + stw_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stl_p(void *ptr, uint32_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stl_p(ptr, bswap32(v)); > > + } else { > > + stl_p(ptr, v); > > + } > > +} > > + > > +static inline void virtio_stq_p(void *ptr, uint64_t v, > > + struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + stq_p(ptr, bswap64(v)); > > + } else { > > + stq_p(ptr, v); > > + } > > +} > > + > > +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap16(lduw_p(ptr)); > > + } else { > > + return lduw_p(ptr); > > + } > > +} > > + > > +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(ldl_p(ptr)); > > + } else { > > + return ldl_p(ptr); > > + } > > +} > > + > > +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap64(ldq_p(ptr)); > > + } else { > > + return ldq_p(ptr); > > + } > > +} > > + > > +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev) > > +{ > > + if (vdev->needs_byteswap) { > > + return bswap32(tswap32(s)); > > + } else { > > + return tswap32(s); > > + } > > +} > > + > > +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev) > > +{ > > + tswap32s(s); > > + if (vdev->needs_byteswap) { > > + *s = bswap32(*s); > > + } > > Couldn't you just reuse virtio_tswap32() here? > Yes. Good catch ! > Also, I think a "needs_byteswap" flag gets confusing. Devices shouldn't > have any access to CPU endian specific RAM accessors. > > How about we introduce a flag like "is_bigendian" that we set depending > on the CPU default endianness for legacy virtio. We can then change that > flag accordingly. For the wrappers here, we could just use accessors > like ldl_be_phys() or ldl_le_phys(). > > That should make things a lot easier to understand - and for virtio 1.0 > we only need to set is_bigendian = false :). > Yeah definitely ! > > Alex > > Thanks.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index aeabf3a..24b565f 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -19,6 +19,7 @@ #include "hw/virtio/virtio.h" #include "qemu/atomic.h" #include "hw/virtio/virtio-bus.h" +#include "hw/virtio/virtio-access.h" /* * The alignment to use between consumer and producer parts of vring. @@ -546,6 +547,8 @@ void virtio_reset(void *opaque) virtio_set_status(vdev, 0); + vdev->needs_byteswap = virtio_legacy_get_byteswap(); + if (k->reset) { k->reset(vdev); } @@ -845,6 +848,7 @@ void virtio_save(VirtIODevice *vdev, QEMUFile *f) qemu_put_8s(f, &vdev->status); qemu_put_8s(f, &vdev->isr); + qemu_put_8s(f, (uint8_t *) &vdev->needs_byteswap); qemu_put_be16s(f, &vdev->queue_sel); qemu_put_be32s(f, &vdev->guest_features); qemu_put_be32(f, vdev->config_len); @@ -905,6 +909,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f) qemu_get_8s(f, &vdev->status); qemu_get_8s(f, &vdev->isr); + qemu_get_8s(f, (uint8_t *) &vdev->needs_byteswap); qemu_get_be16s(f, &vdev->queue_sel); qemu_get_be32s(f, &features); diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h new file mode 100644 index 0000000..70dd1e2 --- /dev/null +++ b/include/hw/virtio/virtio-access.h @@ -0,0 +1,139 @@ +/* + * Virtio Accessor Support: In case your target can change endian. + * + * Copyright IBM, Corp. 2013 + * + * Authors: + * Rusty Russell <rusty@au.ibm.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 2 of the License, or + * (at your option) any later version. + * + */ +#ifndef _QEMU_VIRTIO_ACCESS_H +#define _QEMU_VIRTIO_ACCESS_H +#include "hw/virtio/virtio.h" + +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap16(lduw_phys(as, pa)); + } + return lduw_phys(as, pa); +} + +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap32(ldl_phys(as, pa)); + } + return ldl_phys(as, pa); +} + +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap64(ldq_phys(as, pa)); + } + return ldq_phys(as, pa); +} + +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + stw_phys(as, pa, bswap16(value)); + } else { + stw_phys(as, pa, value); + } +} + +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + stl_phys(as, pa, bswap32(value)); + } else { + stl_phys(as, pa, value); + } +} + +static inline void virtio_stw_p(void *ptr, uint16_t v, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + stw_p(ptr, bswap16(v)); + } else { + stw_p(ptr, v); + } +} + +static inline void virtio_stl_p(void *ptr, uint32_t v, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + stl_p(ptr, bswap32(v)); + } else { + stl_p(ptr, v); + } +} + +static inline void virtio_stq_p(void *ptr, uint64_t v, + struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + stq_p(ptr, bswap64(v)); + } else { + stq_p(ptr, v); + } +} + +static inline int virtio_lduw_p(const void *ptr, struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap16(lduw_p(ptr)); + } else { + return lduw_p(ptr); + } +} + +static inline int virtio_ldl_p(const void *ptr, struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap32(ldl_p(ptr)); + } else { + return ldl_p(ptr); + } +} + +static inline uint64_t virtio_ldq_p(const void *ptr, struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap64(ldq_p(ptr)); + } else { + return ldq_p(ptr); + } +} + +static inline uint32_t virtio_tswap32(uint32_t s, struct VirtIODevice *vdev) +{ + if (vdev->needs_byteswap) { + return bswap32(tswap32(s)); + } else { + return tswap32(s); + } +} + +static inline void virtio_tswap32s(uint32_t *s, struct VirtIODevice *vdev) +{ + tswap32s(s); + if (vdev->needs_byteswap) { + *s = bswap32(*s); + } +} +#endif /* _QEMU_VIRTIO_ACCESS_H */ diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 3e54e90..3595da5 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -121,6 +121,7 @@ struct VirtIODevice bool vm_running; VMChangeStateEntry *vmstate; char *bus_name; + bool needs_byteswap; }; typedef struct VirtioDeviceClass { @@ -253,4 +254,6 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler); void virtio_queue_notify_vq(VirtQueue *vq); void virtio_irq(VirtQueue *vq); + +extern bool virtio_legacy_get_byteswap(void); #endif diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index 5ed1d38..a13381a 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -30,3 +30,4 @@ stub-obj-y += vmstate.o stub-obj-$(CONFIG_WIN32) += fd-register.o stub-obj-y += cpus.o stub-obj-y += kvm.o +stub-obj-y += virtio_get_byteswap.o diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c new file mode 100644 index 0000000..28af5e3 --- /dev/null +++ b/stubs/virtio_get_byteswap.c @@ -0,0 +1,6 @@ +#include "hw/virtio/virtio.h" + +bool virtio_legacy_get_byteswap(void) +{ + return false; +}