Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
Il 17/04/2014 15:44, Michael S. Tsirkin ha scritto: I'm fine with this. And I'm not against a runtime switch to get rid of per-target build of virtio. I am merely asking for separatable patchsets, e.g. structured like this: 1. implement bi-endian support with no data path overhead for fixed endian targets 2. cleanup getting rid of per-target build adding minor data path overhead This way down the line if we see performance regressions it's easy to revert and verify what causes it. An alternative is accompanying patchset with performance benchmarking results. There is no such thing as complete benchmarks, and splitting the series in two as you suggest is cleaner (in addition to more clearly bisectable), so I'd really go that way. Paolo
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Thu, Apr 17, 2014 at 02:29:13PM +0200, Greg Kurz wrote:
> On Thu, 17 Apr 2014 11:00:26 +0300
> "Michael S. Tsirkin" wrote:
>
> > On Thu, Apr 17, 2014 at 08:54:12AM +0200, Greg Kurz wrote:
> > > On Wed, 16 Apr 2014 20:32:07 +0300
> > > "Michael S. Tsirkin" wrote:
> > >
> > > > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote:
> > > > > On 16 April 2014 17:34, Michael S. Tsirkin wrote:
> > > > > > so it looks like virtio is currently compiled per-target.
> > > > > > So why isn't it reasonable to keep it per-target for
> > > > > > purpose of this enhancement?
> > > > > > What am I missing?
> > > > >
> > > > > "virtio" is more than one C file. Currently per-target:
> > > > > hw/virtio/virtio.c
> > > > > hw/virtio/virtio-balloon.c
> > > > > hw/scsi/virtio-scsi.c
> > > > > hw/block/virtio-blk.c
> > > > > hw/char/virtio-serial-bus.c
> > > > > hw/net/virtio-net.c
> > > > >
> > > > > Currently built once:
> > > > > hw/char/virtio-console.c
> > > > > hw/virtio/virtio-bus.c
> > > > > hw/virtio/virtio-mmio.c
> > > > > hw/virtio/virtio-pci.c
> > > > > hw/virtio/virtio-rng.c
> > > > >
> > > > > If we can move files from the first group to the second
> > > > > that's cool.
> > > >
> > > > But doesn't have to be part of this series, yes?
> > > > I would say let's at least have virtio 1.0 out
> > > > and implemented in qemu and linux guests, then
> > > > we can start deprecate virtio 0.X in favor of it.
> > > >
> > > > > Moving files from the second to the first is
> > > > > what I'd like to avoid...
> > > > >
> > > > > thanks
> > > > > -- PMM
> > > >
> > > > Absolutely.
> > > >
> > > > Looks like we are in agreement after all.
> > > >
> > > > So as far as I could see the only issue is with config access
> > > > e.g. in virtio-pci?
> > > > That one already has a branch around virtio_is_big_endian,
> > > > so it's not a problem, there's no need to optimize that.
> > > >
> > >
> > > It is because you haven't looked at the other patches... When
> > > the whole serie is applied, all virtio files use inlined helpers
> > > from virtio-access.h to access memory and fix endianness. That is
> > > why we'd rather not add target dependency here...
> >
> > Right, so what I'm saying is very simple: there are two types of helpers:
> > - per target ones for guest memory access
> > - generic ones for config access
> >
> >
> > One way to implement generic ones is as out of line wrappers
> > for inline per target ones, another way is a separate implementation.
> >
>
> Then you want something like this ?
>
> static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
> {
> #ifdef TARGET_BIENDIAN
> if (virtio_is_big_endian(vdev)) {
> stl_be_p(ptr, v);
> } else {
> stl_le_p(ptr, v);
> }
> #else
> stl_p(ptr, v);
> #endif
> }
I'm fine with this.
And I'm not against a runtime switch to get rid of per-target build of virtio.
I am merely asking for separatable patchsets, e.g. structured like this:
1. implement bi-endian support with no data path overhead for fixed endian
targets
2. cleanup getting rid of per-target build adding minor data path overhead
This way down the line if we see performance regressions it's easy to
revert and verify what causes it.
An alternative is accompanying patchset with performance benchmarking
results.
> >
> > > This being said, if you have an insight to have branch-less code
> > > for fixed endian targets, and dedicated code for ambivalent endian
> > > targets, please send something.
> > >
> > > Cheers.
> > >
> > > --
> > > Greg
> >
> > Sure but I'm a bit busy otherwise - if above hints are insufficient,
> > expect something by Tuesday.
> >
> >
>
> --
> Gregory Kurz kurzg...@fr.ibm.com
> gk...@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.
>
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Thu, 17 Apr 2014 11:00:26 +0300
"Michael S. Tsirkin" wrote:
> On Thu, Apr 17, 2014 at 08:54:12AM +0200, Greg Kurz wrote:
> > On Wed, 16 Apr 2014 20:32:07 +0300
> > "Michael S. Tsirkin" wrote:
> >
> > > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote:
> > > > On 16 April 2014 17:34, Michael S. Tsirkin wrote:
> > > > > so it looks like virtio is currently compiled per-target.
> > > > > So why isn't it reasonable to keep it per-target for
> > > > > purpose of this enhancement?
> > > > > What am I missing?
> > > >
> > > > "virtio" is more than one C file. Currently per-target:
> > > > hw/virtio/virtio.c
> > > > hw/virtio/virtio-balloon.c
> > > > hw/scsi/virtio-scsi.c
> > > > hw/block/virtio-blk.c
> > > > hw/char/virtio-serial-bus.c
> > > > hw/net/virtio-net.c
> > > >
> > > > Currently built once:
> > > > hw/char/virtio-console.c
> > > > hw/virtio/virtio-bus.c
> > > > hw/virtio/virtio-mmio.c
> > > > hw/virtio/virtio-pci.c
> > > > hw/virtio/virtio-rng.c
> > > >
> > > > If we can move files from the first group to the second
> > > > that's cool.
> > >
> > > But doesn't have to be part of this series, yes?
> > > I would say let's at least have virtio 1.0 out
> > > and implemented in qemu and linux guests, then
> > > we can start deprecate virtio 0.X in favor of it.
> > >
> > > > Moving files from the second to the first is
> > > > what I'd like to avoid...
> > > >
> > > > thanks
> > > > -- PMM
> > >
> > > Absolutely.
> > >
> > > Looks like we are in agreement after all.
> > >
> > > So as far as I could see the only issue is with config access
> > > e.g. in virtio-pci?
> > > That one already has a branch around virtio_is_big_endian,
> > > so it's not a problem, there's no need to optimize that.
> > >
> >
> > It is because you haven't looked at the other patches... When
> > the whole serie is applied, all virtio files use inlined helpers
> > from virtio-access.h to access memory and fix endianness. That is
> > why we'd rather not add target dependency here...
>
> Right, so what I'm saying is very simple: there are two types of helpers:
> - per target ones for guest memory access
> - generic ones for config access
>
>
> One way to implement generic ones is as out of line wrappers
> for inline per target ones, another way is a separate implementation.
>
Then you want something like this ?
static inline void virtio_stl_p(VirtIODevice *vdev, void *ptr, uint32_t v)
{
#ifdef TARGET_BIENDIAN
if (virtio_is_big_endian(vdev)) {
stl_be_p(ptr, v);
} else {
stl_le_p(ptr, v);
}
#else
stl_p(ptr, v);
#endif
}
>
> > This being said, if you have an insight to have branch-less code
> > for fixed endian targets, and dedicated code for ambivalent endian
> > targets, please send something.
> >
> > Cheers.
> >
> > --
> > Greg
>
> Sure but I'm a bit busy otherwise - if above hints are insufficient,
> expect something by Tuesday.
>
>
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@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.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Wed, 16 Apr 2014 17:42:22 +0100 Peter Maydell wrote: > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > so it looks like virtio is currently compiled per-target. > > So why isn't it reasonable to keep it per-target for > > purpose of this enhancement? > > What am I missing? > > "virtio" is more than one C file. Currently per-target: > hw/virtio/virtio.c > hw/virtio/virtio-balloon.c > hw/scsi/virtio-scsi.c > hw/block/virtio-blk.c > hw/char/virtio-serial-bus.c > hw/net/virtio-net.c > > Currently built once: > hw/char/virtio-console.c > hw/virtio/virtio-bus.c > hw/virtio/virtio-mmio.c > hw/virtio/virtio-pci.c > hw/virtio/virtio-rng.c > > If we can move files from the first group to the second > that's cool. Moving files from the second to the first is > what I'd like to avoid... > > thanks > -- PMM > I had got at a point where I could move nearly all the code to common-obj, and the cool benefits distracted me from that unwanted branch for fixed endian targets... Sorry for the confusion and noise. :) -- Gregory Kurz kurzg...@fr.ibm.com gk...@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.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Thu, Apr 17, 2014 at 08:54:12AM +0200, Greg Kurz wrote: > On Wed, 16 Apr 2014 20:32:07 +0300 > "Michael S. Tsirkin" wrote: > > > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote: > > > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > > > so it looks like virtio is currently compiled per-target. > > > > So why isn't it reasonable to keep it per-target for > > > > purpose of this enhancement? > > > > What am I missing? > > > > > > "virtio" is more than one C file. Currently per-target: > > > hw/virtio/virtio.c > > > hw/virtio/virtio-balloon.c > > > hw/scsi/virtio-scsi.c > > > hw/block/virtio-blk.c > > > hw/char/virtio-serial-bus.c > > > hw/net/virtio-net.c > > > > > > Currently built once: > > > hw/char/virtio-console.c > > > hw/virtio/virtio-bus.c > > > hw/virtio/virtio-mmio.c > > > hw/virtio/virtio-pci.c > > > hw/virtio/virtio-rng.c > > > > > > If we can move files from the first group to the second > > > that's cool. > > > > But doesn't have to be part of this series, yes? > > I would say let's at least have virtio 1.0 out > > and implemented in qemu and linux guests, then > > we can start deprecate virtio 0.X in favor of it. > > > > > Moving files from the second to the first is > > > what I'd like to avoid... > > > > > > thanks > > > -- PMM > > > > Absolutely. > > > > Looks like we are in agreement after all. > > > > So as far as I could see the only issue is with config access > > e.g. in virtio-pci? > > That one already has a branch around virtio_is_big_endian, > > so it's not a problem, there's no need to optimize that. > > > > It is because you haven't looked at the other patches... When > the whole serie is applied, all virtio files use inlined helpers > from virtio-access.h to access memory and fix endianness. That is > why we'd rather not add target dependency here... Right, so what I'm saying is very simple: there are two types of helpers: - per target ones for guest memory access - generic ones for config access One way to implement generic ones is as out of line wrappers for inline per target ones, another way is a separate implementation. > This being said, if you have an insight to have branch-less code > for fixed endian targets, and dedicated code for ambivalent endian > targets, please send something. > > Cheers. > > -- > Greg Sure but I'm a bit busy otherwise - if above hints are insufficient, expect something by Tuesday.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Wed, 16 Apr 2014 20:32:07 +0300 "Michael S. Tsirkin" wrote: > On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote: > > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > > so it looks like virtio is currently compiled per-target. > > > So why isn't it reasonable to keep it per-target for > > > purpose of this enhancement? > > > What am I missing? > > > > "virtio" is more than one C file. Currently per-target: > > hw/virtio/virtio.c > > hw/virtio/virtio-balloon.c > > hw/scsi/virtio-scsi.c > > hw/block/virtio-blk.c > > hw/char/virtio-serial-bus.c > > hw/net/virtio-net.c > > > > Currently built once: > > hw/char/virtio-console.c > > hw/virtio/virtio-bus.c > > hw/virtio/virtio-mmio.c > > hw/virtio/virtio-pci.c > > hw/virtio/virtio-rng.c > > > > If we can move files from the first group to the second > > that's cool. > > But doesn't have to be part of this series, yes? > I would say let's at least have virtio 1.0 out > and implemented in qemu and linux guests, then > we can start deprecate virtio 0.X in favor of it. > > > Moving files from the second to the first is > > what I'd like to avoid... > > > > thanks > > -- PMM > > Absolutely. > > Looks like we are in agreement after all. > > So as far as I could see the only issue is with config access > e.g. in virtio-pci? > That one already has a branch around virtio_is_big_endian, > so it's not a problem, there's no need to optimize that. > It is because you haven't looked at the other patches... When the whole serie is applied, all virtio files use inlined helpers from virtio-access.h to access memory and fix endianness. That is why we'd rather not add target dependency here... This being said, if you have an insight to have branch-less code for fixed endian targets, and dedicated code for ambivalent endian targets, please send something. Cheers. -- Greg
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Wed, Apr 16, 2014 at 05:42:22PM +0100, Peter Maydell wrote: > On 16 April 2014 17:34, Michael S. Tsirkin wrote: > > so it looks like virtio is currently compiled per-target. > > So why isn't it reasonable to keep it per-target for > > purpose of this enhancement? > > What am I missing? > > "virtio" is more than one C file. Currently per-target: > hw/virtio/virtio.c > hw/virtio/virtio-balloon.c > hw/scsi/virtio-scsi.c > hw/block/virtio-blk.c > hw/char/virtio-serial-bus.c > hw/net/virtio-net.c > > Currently built once: > hw/char/virtio-console.c > hw/virtio/virtio-bus.c > hw/virtio/virtio-mmio.c > hw/virtio/virtio-pci.c > hw/virtio/virtio-rng.c > > If we can move files from the first group to the second > that's cool. But doesn't have to be part of this series, yes? I would say let's at least have virtio 1.0 out and implemented in qemu and linux guests, then we can start deprecate virtio 0.X in favor of it. > Moving files from the second to the first is > what I'd like to avoid... > > thanks > -- PMM Absolutely. Looks like we are in agreement after all. So as far as I could see the only issue is with config access e.g. in virtio-pci? That one already has a branch around virtio_is_big_endian, so it's not a problem, there's no need to optimize that. -- MST
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 16 April 2014 17:34, Michael S. Tsirkin wrote: > so it looks like virtio is currently compiled per-target. > So why isn't it reasonable to keep it per-target for > purpose of this enhancement? > What am I missing? "virtio" is more than one C file. Currently per-target: hw/virtio/virtio.c hw/virtio/virtio-balloon.c hw/scsi/virtio-scsi.c hw/block/virtio-blk.c hw/char/virtio-serial-bus.c hw/net/virtio-net.c Currently built once: hw/char/virtio-console.c hw/virtio/virtio-bus.c hw/virtio/virtio-mmio.c hw/virtio/virtio-pci.c hw/virtio/virtio-rng.c If we can move files from the first group to the second that's cool. Moving files from the second to the first is what I'd like to avoid... thanks -- PMM
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Tue, Apr 15, 2014 at 03:22:56PM +0200, Greg Kurz wrote: > On Tue, 15 Apr 2014 13:35:03 +0200 > Alexander Graf wrote: > > > On 04/15/2014 10:40 AM, Greg Kurz wrote: > > > On Mon, 14 Apr 2014 15:08:23 +0200 > > > Alexander Graf wrote: > > > > > >> On 14.04.14 14:55, Michael S. Tsirkin wrote: > > >>> On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > > On 14.04.14 14:37, Michael S. Tsirkin wrote: > > > On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > > >> On 14.04.14 14:24, Michael S. Tsirkin wrote: > > >>> On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > > On 14.04.14 13:58, Greg Kurz wrote: > > > From: Rusty Russell > > > > > > 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 it all little > > > endian. > > > > > > We introduce memory accessors to be used accross the virtio code > > > where > > > needed. These accessors should support both legacy and 1.0 > > > devices. > > > A good way to do it is to introduce a per-device property to > > > store the > > > endianness. We choose to set this flag at device reset time > > > because it > > > is reasonnable to assume the endianness won't change unless we > > > reboot or > > > kexec another kernel. And it is also reasonnable to assume the > > > new kernel > > > will reset the devices before using them (otherwise it will > > > break). > > > > > > We reuse the virtio_is_big_endian() helper since it provides the > > > right > > > value for legacy devices with most of the targets, that have fixed > > > endianness. It can then be overriden to support endian-ambivalent > > > targets. > > > > > > To support migration, we need to set the flag in virtio_load() as > > > well. > > > > > > (a) One solution would be to add it to the stream, but it have > > > some > > > drawbacks: > > > - since this only affects a few targets, the field should be put > > > into a > > > subsection > > > - virtio migration code should be ported to vmstate to be able to > > > introduce > > > such a subsection > > > > > > (b) If we assume the following to be true: > > > - target endianness falls under some cpu state > > > - cpu state is always restored before virtio devices state > > > because they > > > get initialized in this order in main(). > > > Then an alternative is to rely on virtio_is_big_endian() again at > > > load time. No need to mess around with the migration stream in > > > this case. > > > > > > This patch implements (b). > > > > > > Note that the tswap helpers are implemented in virtio.c so that > > > virtio-access.h stays platform independant. Most of the virtio > > > code > > > will be buildable under common-obj instead of obj then, and spare > > > some cycles when building for multiple targets. > > > > > > Signed-off-by: Rusty Russell > > > [ ldq_phys() API change, > > > relicensed virtio-access.h to GPLv2+ on Rusty's request, > > > introduce a per-device is_big_endian flag (supersedes > > > needs_byteswap global) > > > add VirtIODevice * arg to virtio helpers, > > > use the existing virtio_is_big_endian() helper, > > > virtio-pci: use the device is_big_endian flag, > > > introduce virtio tswap16 and tswap64 helpers, > > > move calls to tswap* out of virtio-access.h to make it > > > platform independant, > > > migration support, > > > Greg Kurz ] > > > Cc: Cédric Le Goater > > > Signed-off-by: Greg Kurz > > > --- > > > > > > Changes since v6: > > > - merge the virtio_needs_byteswap() helper from v6 and existing > > > virtio_is_big_endian() > > > - virtio-pci: now supports endianness changes > > > - virtio-access.h fixes (target independant) > > > > > >exec.c|2 - > > >hw/virtio/Makefile.objs |2 - > > >hw/virtio/virtio-pci.c| 11 +-- > > >hw/virtio/virtio.c| 35 + > > >include/hw/virtio/virtio-access.h | 138 > > > + > > >include/hw/virtio/virtio.h|2 + > > >vl.c
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 15 April 2014 14:22, Greg Kurz wrote:
> On Tue, 15 Apr 2014 13:35:03 +0200
> Alexander Graf wrote:
>> That would defeat the purpose - the reason to have the helper inlined is
>> to remove the conditional branch for x86.
>>
>
> Sure but on the other hand, Peter does not like the idea of moving virtio
> to compiled-per-target... these look like contradictory requests to me... :-\
> Unless I have missed something, we have to settle whether we favor
> building/testing
> time or performance of non-{powerpc,arm} targets to have legacy virtio
> supporting
> LE powerpc and BE arm...
To repeat my remarks from IRC: this weirdo endianness behaviour
was a dumb spec decision, and dumb spec decisions have to be
paid for. virtio 1.0 should fix this, so we can just suggest
that anybody who really cares about performance down to avoiding
conditional branches should switch to that, right?
My suggestion was that:
* virtio_is_big_endian should be a CPU object method
* PPC (and eventually ARM) implement this by checking whatever
the state flag is
* base class implementation returns true/false based on
TARGET_WORDS_BIGENDIAN
* virtio devices call this method on current_cpu when the
guest first touches/resets the virtio device, and cache
the result
thanks
-- PMM
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Tue, 15 Apr 2014 13:35:03 +0200 Alexander Graf wrote: > On 04/15/2014 10:40 AM, Greg Kurz wrote: > > On Mon, 14 Apr 2014 15:08:23 +0200 > > Alexander Graf wrote: > > > >> On 14.04.14 14:55, Michael S. Tsirkin wrote: > >>> On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > On 14.04.14 14:37, Michael S. Tsirkin wrote: > > On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > >> On 14.04.14 14:24, Michael S. Tsirkin wrote: > >>> On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > On 14.04.14 13:58, Greg Kurz wrote: > > From: Rusty Russell > > > > 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 it all little > > endian. > > > > We introduce memory accessors to be used accross the virtio code > > where > > needed. These accessors should support both legacy and 1.0 devices. > > A good way to do it is to introduce a per-device property to store > > the > > endianness. We choose to set this flag at device reset time because > > it > > is reasonnable to assume the endianness won't change unless we > > reboot or > > kexec another kernel. And it is also reasonnable to assume the new > > kernel > > will reset the devices before using them (otherwise it will break). > > > > We reuse the virtio_is_big_endian() helper since it provides the > > right > > value for legacy devices with most of the targets, that have fixed > > endianness. It can then be overriden to support endian-ambivalent > > targets. > > > > To support migration, we need to set the flag in virtio_load() as > > well. > > > > (a) One solution would be to add it to the stream, but it have some > > drawbacks: > > - since this only affects a few targets, the field should be put > > into a > > subsection > > - virtio migration code should be ported to vmstate to be able to > > introduce > > such a subsection > > > > (b) If we assume the following to be true: > > - target endianness falls under some cpu state > > - cpu state is always restored before virtio devices state because > > they > > get initialized in this order in main(). > > Then an alternative is to rely on virtio_is_big_endian() again at > > load time. No need to mess around with the migration stream in this > > case. > > > > This patch implements (b). > > > > Note that the tswap helpers are implemented in virtio.c so that > > virtio-access.h stays platform independant. Most of the virtio code > > will be buildable under common-obj instead of obj then, and spare > > some cycles when building for multiple targets. > > > > Signed-off-by: Rusty Russell > > [ ldq_phys() API change, > > relicensed virtio-access.h to GPLv2+ on Rusty's request, > > introduce a per-device is_big_endian flag (supersedes > > needs_byteswap global) > > add VirtIODevice * arg to virtio helpers, > > use the existing virtio_is_big_endian() helper, > > virtio-pci: use the device is_big_endian flag, > > introduce virtio tswap16 and tswap64 helpers, > > move calls to tswap* out of virtio-access.h to make it platform > > independant, > > migration support, > > Greg Kurz ] > > Cc: Cédric Le Goater > > Signed-off-by: Greg Kurz > > --- > > > > Changes since v6: > > - merge the virtio_needs_byteswap() helper from v6 and existing > > virtio_is_big_endian() > > - virtio-pci: now supports endianness changes > > - virtio-access.h fixes (target independant) > > > >exec.c|2 - > >hw/virtio/Makefile.objs |2 - > >hw/virtio/virtio-pci.c| 11 +-- > >hw/virtio/virtio.c| 35 + > >include/hw/virtio/virtio-access.h | 138 > > + > >include/hw/virtio/virtio.h|2 + > >vl.c |4 + > >7 files changed, 185 insertions(+), 9 deletions(-) > >create mode 100644 include/hw/virtio/virtio-access.h > > > > diff --git a/exec.c b/exec.c > > index 91513c6..e6777d0 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -42,6 +42,7 @@ > >
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 04/15/2014 10:40 AM, Greg Kurz wrote:
On Mon, 14 Apr 2014 15:08:23 +0200
Alexander Graf wrote:
On 14.04.14 14:55, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote:
On 14.04.14 14:37, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
On 14.04.14 14:24, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138
+
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find
out if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 15:08:23 +0200 Alexander Graf wrote: > > On 14.04.14 14:55, Michael S. Tsirkin wrote: > > On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > >> On 14.04.14 14:37, Michael S. Tsirkin wrote: > >>> On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > On 14.04.14 14:24, Michael S. Tsirkin wrote: > > On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > >> On 14.04.14 13:58, Greg Kurz wrote: > >>> From: Rusty Russell > >>> > >>> 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 it all little > >>> endian. > >>> > >>> We introduce memory accessors to be used accross the virtio code where > >>> needed. These accessors should support both legacy and 1.0 devices. > >>> A good way to do it is to introduce a per-device property to store the > >>> endianness. We choose to set this flag at device reset time because it > >>> is reasonnable to assume the endianness won't change unless we reboot > >>> or > >>> kexec another kernel. And it is also reasonnable to assume the new > >>> kernel > >>> will reset the devices before using them (otherwise it will break). > >>> > >>> We reuse the virtio_is_big_endian() helper since it provides the right > >>> value for legacy devices with most of the targets, that have fixed > >>> endianness. It can then be overriden to support endian-ambivalent > >>> targets. > >>> > >>> To support migration, we need to set the flag in virtio_load() as > >>> well. > >>> > >>> (a) One solution would be to add it to the stream, but it have some > >>> drawbacks: > >>> - since this only affects a few targets, the field should be put into > >>> a > >>>subsection > >>> - virtio migration code should be ported to vmstate to be able to > >>> introduce > >>>such a subsection > >>> > >>> (b) If we assume the following to be true: > >>> - target endianness falls under some cpu state > >>> - cpu state is always restored before virtio devices state because > >>> they > >>>get initialized in this order in main(). > >>> Then an alternative is to rely on virtio_is_big_endian() again at > >>> load time. No need to mess around with the migration stream in this > >>> case. > >>> > >>> This patch implements (b). > >>> > >>> Note that the tswap helpers are implemented in virtio.c so that > >>> virtio-access.h stays platform independant. Most of the virtio code > >>> will be buildable under common-obj instead of obj then, and spare > >>> some cycles when building for multiple targets. > >>> > >>> Signed-off-by: Rusty Russell > >>> [ ldq_phys() API change, > >>>relicensed virtio-access.h to GPLv2+ on Rusty's request, > >>>introduce a per-device is_big_endian flag (supersedes > >>> needs_byteswap global) > >>>add VirtIODevice * arg to virtio helpers, > >>>use the existing virtio_is_big_endian() helper, > >>>virtio-pci: use the device is_big_endian flag, > >>>introduce virtio tswap16 and tswap64 helpers, > >>>move calls to tswap* out of virtio-access.h to make it platform > >>> independant, > >>>migration support, > >>>Greg Kurz ] > >>> Cc: Cédric Le Goater > >>> Signed-off-by: Greg Kurz > >>> --- > >>> > >>> Changes since v6: > >>> - merge the virtio_needs_byteswap() helper from v6 and existing > >>>virtio_is_big_endian() > >>> - virtio-pci: now supports endianness changes > >>> - virtio-access.h fixes (target independant) > >>> > >>> exec.c|2 - > >>> hw/virtio/Makefile.objs |2 - > >>> hw/virtio/virtio-pci.c| 11 +-- > >>> hw/virtio/virtio.c| 35 + > >>> include/hw/virtio/virtio-access.h | 138 > >>> + > >>> include/hw/virtio/virtio.h|2 + > >>> vl.c |4 + > >>> 7 files changed, 185 insertions(+), 9 deletions(-) > >>> create mode 100644 include/hw/virtio/virtio-access.h > >>> > >>> diff --git a/exec.c b/exec.c > >>> index 91513c6..e6777d0 100644 > >>> --- a/exec.c > >>> +++ b/exec.c > >>> @@ -42,6 +42,7 @@ > >>> #else /* !CONFIG_USER_ONLY */ > >>> #include "sysemu/xen-mapcache.h" > >>> #include "trace.h" > >>> +#include "hw/virtio/virtio.h" > >>> #endif > >>> #include "exec/cpu-all.h" > >>> @@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, > >>> target_ulong addr, > >>>* A helper function for the _utterly broken_ virtio d
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 14:40:04 +0200
Alexander Graf wrote:
>
> On 14.04.14 14:37, Michael S. Tsirkin wrote:
> > On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
> >> On 14.04.14 14:24, Michael S. Tsirkin wrote:
> >>> On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
> On 14.04.14 13:58, Greg Kurz wrote:
> > From: Rusty Russell
> >
> > 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 it all little endian.
> >
> > We introduce memory accessors to be used accross the virtio code where
> > needed. These accessors should support both legacy and 1.0 devices.
> > A good way to do it is to introduce a per-device property to store the
> > endianness. We choose to set this flag at device reset time because it
> > is reasonnable to assume the endianness won't change unless we reboot or
> > kexec another kernel. And it is also reasonnable to assume the new
> > kernel
> > will reset the devices before using them (otherwise it will break).
> >
> > We reuse the virtio_is_big_endian() helper since it provides the right
> > value for legacy devices with most of the targets, that have fixed
> > endianness. It can then be overriden to support endian-ambivalent
> > targets.
> >
> > To support migration, we need to set the flag in virtio_load() as well.
> >
> > (a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> > - since this only affects a few targets, the field should be put into a
> >subsection
> > - virtio migration code should be ported to vmstate to be able to
> > introduce
> >such a subsection
> >
> > (b) If we assume the following to be true:
> > - target endianness falls under some cpu state
> > - cpu state is always restored before virtio devices state because they
> >get initialized in this order in main().
> > Then an alternative is to rely on virtio_is_big_endian() again at
> > load time. No need to mess around with the migration stream in this
> > case.
> >
> > This patch implements (b).
> >
> > Note that the tswap helpers are implemented in virtio.c so that
> > virtio-access.h stays platform independant. Most of the virtio code
> > will be buildable under common-obj instead of obj then, and spare
> > some cycles when building for multiple targets.
> >
> > Signed-off-by: Rusty Russell
> > [ ldq_phys() API change,
> >relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> >add VirtIODevice * arg to virtio helpers,
> >use the existing virtio_is_big_endian() helper,
> >virtio-pci: use the device is_big_endian flag,
> >introduce virtio tswap16 and tswap64 helpers,
> >move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> >migration support,
> >Greg Kurz ]
> > Cc: Cédric Le Goater
> > Signed-off-by: Greg Kurz
> > ---
> >
> > Changes since v6:
> > - merge the virtio_needs_byteswap() helper from v6 and existing
> >virtio_is_big_endian()
> > - virtio-pci: now supports endianness changes
> > - virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> > diff --git a/exec.c b/exec.c
> > index 91513c6..e6777d0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> > +#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> > @@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu,
> > target_ulong addr,
> >* A helper function for the _utterly broken_ virtio device model to
> > find out if
> >* it's running on a big endian machine. Don't do this at home kids!
> >*/
> > -bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> > index 1ba53d9..68c30
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 15:12, Greg Kurz wrote:
On Mon, 14 Apr 2014 14:53:03 +0200
Alexander Graf wrote:
On 14.04.14 14:50, Greg Kurz wrote:
On Mon, 14 Apr 2014 14:22:36 +0200
Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap
global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform
independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
[...]
+
+uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s);
+
+static inline void virtio_tswap64s(VirtIODevice *vdev, uint64_t *s)
+{
+*s = virtio_tswap64(vdev, *s);
+}
Could we try to poison any non-virtio, non-endian-specific memory
accessors when this file is included? That way we don't fall into
pitfalls where we end up running stl_p in a tiny corner case somewhere
still.
Alex
Hmmm... there are still some stl_p users in virtio.c that I could not
port to the new virtio memory accessors... These are the virtio_config_*
helpers that gets called from virtio-pci and virtio-mmio.
Why couldn't you port them to the new virtio memory accessors? Are you
missing a vdev parameter? Could you add one?
IIRC config space is a weird mix of target endianness and little endian.
The helpers in virtio do the stl_p, but it is the caller that does the
byteswap (virtio-pci) or not (virtio-mmio)... That's where I got a little
confused and did not dare to touch this code :-\
Of course, I can try to focus harder see what can/should be done here. :)
Let's make Michael happy with target specific virtio drivers first and
then look at this mess ;).
Alex
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 14:53:03 +0200
Alexander Graf wrote:
>
> On 14.04.14 14:50, Greg Kurz wrote:
> > On Mon, 14 Apr 2014 14:22:36 +0200
> > Alexander Graf wrote:
> >
> >> On 14.04.14 13:58, Greg Kurz wrote:
> >>> From: Rusty Russell
> >>>
> >>> 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 it all little endian.
> >>>
> >>> We introduce memory accessors to be used accross the virtio code where
> >>> needed. These accessors should support both legacy and 1.0 devices.
> >>> A good way to do it is to introduce a per-device property to store the
> >>> endianness. We choose to set this flag at device reset time because it
> >>> is reasonnable to assume the endianness won't change unless we reboot or
> >>> kexec another kernel. And it is also reasonnable to assume the new kernel
> >>> will reset the devices before using them (otherwise it will break).
> >>>
> >>> We reuse the virtio_is_big_endian() helper since it provides the right
> >>> value for legacy devices with most of the targets, that have fixed
> >>> endianness. It can then be overriden to support endian-ambivalent targets.
> >>>
> >>> To support migration, we need to set the flag in virtio_load() as well.
> >>>
> >>> (a) One solution would be to add it to the stream, but it have some
> >>> drawbacks:
> >>> - since this only affects a few targets, the field should be put into a
> >>> subsection
> >>> - virtio migration code should be ported to vmstate to be able to
> >>> introduce
> >>> such a subsection
> >>>
> >>> (b) If we assume the following to be true:
> >>> - target endianness falls under some cpu state
> >>> - cpu state is always restored before virtio devices state because they
> >>> get initialized in this order in main().
> >>> Then an alternative is to rely on virtio_is_big_endian() again at
> >>> load time. No need to mess around with the migration stream in this case.
> >>>
> >>> This patch implements (b).
> >>>
> >>> Note that the tswap helpers are implemented in virtio.c so that
> >>> virtio-access.h stays platform independant. Most of the virtio code
> >>> will be buildable under common-obj instead of obj then, and spare
> >>> some cycles when building for multiple targets.
> >>>
> >>> Signed-off-by: Rusty Russell
> >>> [ ldq_phys() API change,
> >>> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >>> introduce a per-device is_big_endian flag (supersedes needs_byteswap
> >>> global)
> >>> add VirtIODevice * arg to virtio helpers,
> >>> use the existing virtio_is_big_endian() helper,
> >>> virtio-pci: use the device is_big_endian flag,
> >>> introduce virtio tswap16 and tswap64 helpers,
> >>> move calls to tswap* out of virtio-access.h to make it platform
> >>> independant,
> >>> migration support,
> >>> Greg Kurz ]
> >>> Cc: Cédric Le Goater
> >>> Signed-off-by: Greg Kurz
> >>> ---
>
> [...]
>
> >>> +
> >>> +uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s);
> >>> +
> >>> +static inline void virtio_tswap64s(VirtIODevice *vdev, uint64_t *s)
> >>> +{
> >>> +*s = virtio_tswap64(vdev, *s);
> >>> +}
> >> Could we try to poison any non-virtio, non-endian-specific memory
> >> accessors when this file is included? That way we don't fall into
> >> pitfalls where we end up running stl_p in a tiny corner case somewhere
> >> still.
> >>
> >>
> >> Alex
> >>
> > Hmmm... there are still some stl_p users in virtio.c that I could not
> > port to the new virtio memory accessors... These are the virtio_config_*
> > helpers that gets called from virtio-pci and virtio-mmio.
>
> Why couldn't you port them to the new virtio memory accessors? Are you
> missing a vdev parameter? Could you add one?
>
> IIRC config space is a weird mix of target endianness and little endian.
>
The helpers in virtio do the stl_p, but it is the caller that does the
byteswap (virtio-pci) or not (virtio-mmio)... That's where I got a little
confused and did not dare to touch this code :-\
Of course, I can try to focus harder see what can/should be done here. :)
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@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.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 03:08:23PM +0200, Alexander Graf wrote: > > On 14.04.14 14:55, Michael S. Tsirkin wrote: > >On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > >>On 14.04.14 14:37, Michael S. Tsirkin wrote: > >>>On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > On 14.04.14 14:24, Michael S. Tsirkin wrote: > >On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > >>On 14.04.14 13:58, Greg Kurz wrote: > >>>From: Rusty Russell > >>> > >>>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 it all little > >>>endian. > >>> > >>>We introduce memory accessors to be used accross the virtio code where > >>>needed. These accessors should support both legacy and 1.0 devices. > >>>A good way to do it is to introduce a per-device property to store the > >>>endianness. We choose to set this flag at device reset time because it > >>>is reasonnable to assume the endianness won't change unless we reboot > >>>or > >>>kexec another kernel. And it is also reasonnable to assume the new > >>>kernel > >>>will reset the devices before using them (otherwise it will break). > >>> > >>>We reuse the virtio_is_big_endian() helper since it provides the right > >>>value for legacy devices with most of the targets, that have fixed > >>>endianness. It can then be overriden to support endian-ambivalent > >>>targets. > >>> > >>>To support migration, we need to set the flag in virtio_load() as well. > >>> > >>>(a) One solution would be to add it to the stream, but it have some > >>> drawbacks: > >>>- since this only affects a few targets, the field should be put into a > >>> subsection > >>>- virtio migration code should be ported to vmstate to be able to > >>>introduce > >>> such a subsection > >>> > >>>(b) If we assume the following to be true: > >>>- target endianness falls under some cpu state > >>>- cpu state is always restored before virtio devices state because they > >>> get initialized in this order in main(). > >>>Then an alternative is to rely on virtio_is_big_endian() again at > >>>load time. No need to mess around with the migration stream in this > >>>case. > >>> > >>>This patch implements (b). > >>> > >>>Note that the tswap helpers are implemented in virtio.c so that > >>>virtio-access.h stays platform independant. Most of the virtio code > >>>will be buildable under common-obj instead of obj then, and spare > >>>some cycles when building for multiple targets. > >>> > >>>Signed-off-by: Rusty Russell > >>>[ ldq_phys() API change, > >>> relicensed virtio-access.h to GPLv2+ on Rusty's request, > >>> introduce a per-device is_big_endian flag (supersedes > >>> needs_byteswap global) > >>> add VirtIODevice * arg to virtio helpers, > >>> use the existing virtio_is_big_endian() helper, > >>> virtio-pci: use the device is_big_endian flag, > >>> introduce virtio tswap16 and tswap64 helpers, > >>> move calls to tswap* out of virtio-access.h to make it platform > >>> independant, > >>> migration support, > >>> Greg Kurz ] > >>>Cc: Cédric Le Goater > >>>Signed-off-by: Greg Kurz > >>>--- > >>> > >>>Changes since v6: > >>>- merge the virtio_needs_byteswap() helper from v6 and existing > >>> virtio_is_big_endian() > >>>- virtio-pci: now supports endianness changes > >>>- virtio-access.h fixes (target independant) > >>> > >>> exec.c|2 - > >>> hw/virtio/Makefile.objs |2 - > >>> hw/virtio/virtio-pci.c| 11 +-- > >>> hw/virtio/virtio.c| 35 + > >>> include/hw/virtio/virtio-access.h | 138 > >>> + > >>> include/hw/virtio/virtio.h|2 + > >>> vl.c |4 + > >>> 7 files changed, 185 insertions(+), 9 deletions(-) > >>> create mode 100644 include/hw/virtio/virtio-access.h > >>> > >>>diff --git a/exec.c b/exec.c > >>>index 91513c6..e6777d0 100644 > >>>--- a/exec.c > >>>+++ b/exec.c > >>>@@ -42,6 +42,7 @@ > >>> #else /* !CONFIG_USER_ONLY */ > >>> #include "sysemu/xen-mapcache.h" > >>> #include "trace.h" > >>>+#include "hw/virtio/virtio.h" > >>> #endif > >>> #include "exec/cpu-all.h" > >>>@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, > >>>target_ulong addr, > >>> * A helper function for the _utterly broken_ virtio device model to > >>> find out if > >>> * it's running on a big endian machine. Don't do this at home kid
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:55, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote:
On 14.04.14 14:37, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
On 14.04.14 14:24, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 15:46:34 +0300 "Michael S. Tsirkin" wrote: > On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > > > > On 14.04.14 14:37, Michael S. Tsirkin wrote: > > >On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > > >>On 14.04.14 14:24, Michael S. Tsirkin wrote: > > >>>On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote: > > On 14.04.14 13:58, Greg Kurz wrote: > > >From: Rusty Russell > > > > > >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 it all little > > >endian. > > > > > >We introduce memory accessors to be used accross the virtio code where > > >needed. These accessors should support both legacy and 1.0 devices. > > >A good way to do it is to introduce a per-device property to store the > > >endianness. We choose to set this flag at device reset time because it > > >is reasonnable to assume the endianness won't change unless we reboot > > >or > > >kexec another kernel. And it is also reasonnable to assume the new > > >kernel > > >will reset the devices before using them (otherwise it will break). > > > > > >We reuse the virtio_is_big_endian() helper since it provides the right > > >value for legacy devices with most of the targets, that have fixed > > >endianness. It can then be overriden to support endian-ambivalent > > >targets. > > > > > >To support migration, we need to set the flag in virtio_load() as well. > > > > > >(a) One solution would be to add it to the stream, but it have some > > > drawbacks: > > >- since this only affects a few targets, the field should be put into a > > > subsection > > >- virtio migration code should be ported to vmstate to be able to > > >introduce > > > such a subsection > > > > > >(b) If we assume the following to be true: > > >- target endianness falls under some cpu state > > >- cpu state is always restored before virtio devices state because they > > > get initialized in this order in main(). > > >Then an alternative is to rely on virtio_is_big_endian() again at > > >load time. No need to mess around with the migration stream in this > > >case. > > > > > >This patch implements (b). > > > > > >Note that the tswap helpers are implemented in virtio.c so that > > >virtio-access.h stays platform independant. Most of the virtio code > > >will be buildable under common-obj instead of obj then, and spare > > >some cycles when building for multiple targets. > > > > > >Signed-off-by: Rusty Russell > > >[ ldq_phys() API change, > > > relicensed virtio-access.h to GPLv2+ on Rusty's request, > > > introduce a per-device is_big_endian flag (supersedes > > > needs_byteswap global) > > > add VirtIODevice * arg to virtio helpers, > > > use the existing virtio_is_big_endian() helper, > > > virtio-pci: use the device is_big_endian flag, > > > introduce virtio tswap16 and tswap64 helpers, > > > move calls to tswap* out of virtio-access.h to make it platform > > > independant, > > > migration support, > > > Greg Kurz ] > > >Cc: Cédric Le Goater > > >Signed-off-by: Greg Kurz > > >--- > > > > > >Changes since v6: > > >- merge the virtio_needs_byteswap() helper from v6 and existing > > > virtio_is_big_endian() > > >- virtio-pci: now supports endianness changes > > >- virtio-access.h fixes (target independant) > > > > > > exec.c|2 - > > > hw/virtio/Makefile.objs |2 - > > > hw/virtio/virtio-pci.c| 11 +-- > > > hw/virtio/virtio.c| 35 + > > > include/hw/virtio/virtio-access.h | 138 > > > + > > > include/hw/virtio/virtio.h|2 + > > > vl.c |4 + > > > 7 files changed, 185 insertions(+), 9 deletions(-) > > > create mode 100644 include/hw/virtio/virtio-access.h > > > > > >diff --git a/exec.c b/exec.c > > >index 91513c6..e6777d0 100644 > > >--- a/exec.c > > >+++ b/exec.c > > >@@ -42,6 +42,7 @@ > > > #else /* !CONFIG_USER_ONLY */ > > > #include "sysemu/xen-mapcache.h" > > > #include "trace.h" > > >+#include "hw/virtio/virtio.h" > > > #endif > > > #include "exec/cpu-all.h" > > >@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, > > >target_ulong addr, > > > * A helper function for the _utterly broken_ virtio device model to > > > find out if > > > * it's running on a big endian machine. Don't do this at home kids! > > > */ > > >-bool virtio_is_bi
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:56, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:49:31PM +0200, Alexander Graf wrote: On 14.04.14 14:46, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: On 14.04.14 14:37, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: On 14.04.14 14:24, Michael S. Tsirkin wrote: This will have to be measured and proved by whoever's proposing the patch, not by reviewers. Platforms such as AMD which don't do prediction well would be especially interesting to test on. Sure, Greg, can you do that? I'm sure Michael has test cases available he can give you to measure performance on this. Speaking of which, how does all of this work with vhost? Alex I think that's missing. As a first step, we need to disable vhost when host/guest endian-ness do not match. We could also add cross-endian support to vhost. Or just wait a couple more months for virtio 1.0 which is fixed endian-ness. That won't help for current ppc64le guests, so I suppose we will need cross-endian vhost. It's quite hard to backport a driver onto installation media :). We already have openSUSE isos out there that support virtio and ppc64le. Alex
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 04/14/2014 02:49 PM, Alexander Graf wrote: > > On 14.04.14 14:46, Michael S. Tsirkin wrote: >> On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: >>> On 14.04.14 14:37, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > On 14.04.14 14:24, Michael S. Tsirkin wrote: > This will have to be measured and proved by whoever's proposing the patch, not by reviewers. Platforms such as AMD which don't do prediction well would be especially interesting to test on. >>> Sure, Greg, can you do that? I'm sure Michael has test cases >>> available he can give you to measure performance on this. >>> >>> Speaking of which, how does all of this work with vhost? >>> >>> >>> Alex >> I think that's missing. >> As a first step, we need to disable vhost when >> host/guest endian-ness do not match. >> >> We could also add cross-endian support to vhost. >> >> Or just wait a couple more months for virtio 1.0 which is fixed >> endian-ness. > > That won't help for current ppc64le guests, so I suppose we will > need cross-endian vhost. Yes. For the moment, vhost=off is needed and even with that, qemu needs to byteswap a few attributes (see virtio-net header in patch 3). I am looking for a simple way to propagate the vring endianness to the host without breaking the compatibility with the current virtio drivers. Hopefully this is feasible. C.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:49:31PM +0200, Alexander Graf wrote: > > On 14.04.14 14:46, Michael S. Tsirkin wrote: > >On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: > >>On 14.04.14 14:37, Michael S. Tsirkin wrote: > >>>On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: > On 14.04.14 14:24, Michael S. Tsirkin wrote: > > >>>This will have to be measured and proved by whoever's proposing the > >>>patch, not by reviewers. Platforms such as AMD which don't do > >>>prediction well would be especially interesting to test on. > >>Sure, Greg, can you do that? I'm sure Michael has test cases > >>available he can give you to measure performance on this. > >> > >>Speaking of which, how does all of this work with vhost? > >> > >> > >>Alex > >I think that's missing. > >As a first step, we need to disable vhost when > >host/guest endian-ness do not match. > > > >We could also add cross-endian support to vhost. > > > >Or just wait a couple more months for virtio 1.0 which is fixed > >endian-ness. > > That won't help for current ppc64le guests, so I suppose we will > need cross-endian vhost. > > > Alex It's kernel level work anyway. Just backport a new driver. Seems less work for more benefit. -- MST
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote:
>
> On 14.04.14 14:37, Michael S. Tsirkin wrote:
> >On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
> >>On 14.04.14 14:24, Michael S. Tsirkin wrote:
> >>>On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
> On 14.04.14 13:58, Greg Kurz wrote:
> >From: Rusty Russell
> >
> >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 it all little endian.
> >
> >We introduce memory accessors to be used accross the virtio code where
> >needed. These accessors should support both legacy and 1.0 devices.
> >A good way to do it is to introduce a per-device property to store the
> >endianness. We choose to set this flag at device reset time because it
> >is reasonnable to assume the endianness won't change unless we reboot or
> >kexec another kernel. And it is also reasonnable to assume the new kernel
> >will reset the devices before using them (otherwise it will break).
> >
> >We reuse the virtio_is_big_endian() helper since it provides the right
> >value for legacy devices with most of the targets, that have fixed
> >endianness. It can then be overriden to support endian-ambivalent
> >targets.
> >
> >To support migration, we need to set the flag in virtio_load() as well.
> >
> >(a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> >- since this only affects a few targets, the field should be put into a
> > subsection
> >- virtio migration code should be ported to vmstate to be able to
> >introduce
> > such a subsection
> >
> >(b) If we assume the following to be true:
> >- target endianness falls under some cpu state
> >- cpu state is always restored before virtio devices state because they
> > get initialized in this order in main().
> >Then an alternative is to rely on virtio_is_big_endian() again at
> >load time. No need to mess around with the migration stream in this case.
> >
> >This patch implements (b).
> >
> >Note that the tswap helpers are implemented in virtio.c so that
> >virtio-access.h stays platform independant. Most of the virtio code
> >will be buildable under common-obj instead of obj then, and spare
> >some cycles when building for multiple targets.
> >
> >Signed-off-by: Rusty Russell
> >[ ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> > add VirtIODevice * arg to virtio helpers,
> > use the existing virtio_is_big_endian() helper,
> > virtio-pci: use the device is_big_endian flag,
> > introduce virtio tswap16 and tswap64 helpers,
> > move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> > migration support,
> > Greg Kurz ]
> >Cc: Cédric Le Goater
> >Signed-off-by: Greg Kurz
> >---
> >
> >Changes since v6:
> >- merge the virtio_needs_byteswap() helper from v6 and existing
> > virtio_is_big_endian()
> >- virtio-pci: now supports endianness changes
> >- virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> >diff --git a/exec.c b/exec.c
> >index 91513c6..e6777d0 100644
> >--- a/exec.c
> >+++ b/exec.c
> >@@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> >+#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu,
> >target_ulong addr,
> > * A helper function for the _utterly broken_ virtio device model to
> > find out if
> > * it's running on a big endian machine. Don't do this at home kids!
> > */
> >-bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> >diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >index 1ba53d9..68c3064 100644
> >--- a/hw/virtio/Makefile.objs
> >+++ b/hw/virtio/Makefile.objs
> >@@ -4,5 +4
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:50, Greg Kurz wrote:
On Mon, 14 Apr 2014 14:22:36 +0200
Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
[...]
+
+uint64_t virtio_tswap64(VirtIODevice *vdev, uint64_t s);
+
+static inline void virtio_tswap64s(VirtIODevice *vdev, uint64_t *s)
+{
+*s = virtio_tswap64(vdev, *s);
+}
Could we try to poison any non-virtio, non-endian-specific memory
accessors when this file is included? That way we don't fall into
pitfalls where we end up running stl_p in a tiny corner case somewhere
still.
Alex
Hmmm... there are still some stl_p users in virtio.c that I could not
port to the new virtio memory accessors... These are the virtio_config_*
helpers that gets called from virtio-pci and virtio-mmio.
Why couldn't you port them to the new virtio memory accessors? Are you
missing a vdev parameter? Could you add one?
IIRC config space is a weird mix of target endianness and little endian.
Alex
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 14:22:36 +0200
Alexander Graf wrote:
>
> On 14.04.14 13:58, Greg Kurz wrote:
> > From: Rusty Russell
> >
> > 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 it all little endian.
> >
> > We introduce memory accessors to be used accross the virtio code where
> > needed. These accessors should support both legacy and 1.0 devices.
> > A good way to do it is to introduce a per-device property to store the
> > endianness. We choose to set this flag at device reset time because it
> > is reasonnable to assume the endianness won't change unless we reboot or
> > kexec another kernel. And it is also reasonnable to assume the new kernel
> > will reset the devices before using them (otherwise it will break).
> >
> > We reuse the virtio_is_big_endian() helper since it provides the right
> > value for legacy devices with most of the targets, that have fixed
> > endianness. It can then be overriden to support endian-ambivalent targets.
> >
> > To support migration, we need to set the flag in virtio_load() as well.
> >
> > (a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> > - since this only affects a few targets, the field should be put into a
> >subsection
> > - virtio migration code should be ported to vmstate to be able to introduce
> >such a subsection
> >
> > (b) If we assume the following to be true:
> > - target endianness falls under some cpu state
> > - cpu state is always restored before virtio devices state because they
> >get initialized in this order in main().
> > Then an alternative is to rely on virtio_is_big_endian() again at
> > load time. No need to mess around with the migration stream in this case.
> >
> > This patch implements (b).
> >
> > Note that the tswap helpers are implemented in virtio.c so that
> > virtio-access.h stays platform independant. Most of the virtio code
> > will be buildable under common-obj instead of obj then, and spare
> > some cycles when building for multiple targets.
> >
> > Signed-off-by: Rusty Russell
> > [ ldq_phys() API change,
> >relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> >add VirtIODevice * arg to virtio helpers,
> >use the existing virtio_is_big_endian() helper,
> >virtio-pci: use the device is_big_endian flag,
> >introduce virtio tswap16 and tswap64 helpers,
> >move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> >migration support,
> >Greg Kurz ]
> > Cc: Cédric Le Goater
> > Signed-off-by: Greg Kurz
> > ---
> >
> > Changes since v6:
> > - merge the virtio_needs_byteswap() helper from v6 and existing
> >virtio_is_big_endian()
> > - virtio-pci: now supports endianness changes
> > - virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> > diff --git a/exec.c b/exec.c
> > index 91513c6..e6777d0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> > +#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >
> > @@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> > addr,
> >* A helper function for the _utterly broken_ virtio device model to find
> > out if
> >* it's running on a big endian machine. Don't do this at home kids!
> >*/
> > -bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> > index 1ba53d9..68c3064 100644
> > --- a/hw/virtio/Makefile.objs
> > +++ b/hw/virtio/Makefile.objs
> > @@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> > common-obj-y += virtio-mmio.o
> > common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >
> > -obj-y += virtio.o virtio-balloon.o
> > +obj-y += virtio.o virtio-balloon.o
> > obj-$(CONFIG_LINUX) += vhost.o
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ce97514..82a1689 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -89,9 +89,6 @@
> > /* Flags track per-device state like workarounds for quirks in older
> > guests. */
> > #define VIRTIO_PCI_FLAG_BUS_MASTER_
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:46, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote: On 14.04.14 14:37, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote: On 14.04.14 14:24, Michael S. Tsirkin wrote: This will have to be measured and proved by whoever's proposing the patch, not by reviewers. Platforms such as AMD which don't do prediction well would be especially interesting to test on. Sure, Greg, can you do that? I'm sure Michael has test cases available he can give you to measure performance on this. Speaking of which, how does all of this work with vhost? Alex I think that's missing. As a first step, we need to disable vhost when host/guest endian-ness do not match. We could also add cross-endian support to vhost. Or just wait a couple more months for virtio 1.0 which is fixed endian-ness. That won't help for current ppc64le guests, so I suppose we will need cross-endian vhost. Alex
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:40:04PM +0200, Alexander Graf wrote:
>
> On 14.04.14 14:37, Michael S. Tsirkin wrote:
> >On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
> >>On 14.04.14 14:24, Michael S. Tsirkin wrote:
> >>>On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
> On 14.04.14 13:58, Greg Kurz wrote:
> >From: Rusty Russell
> >
> >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 it all little endian.
> >
> >We introduce memory accessors to be used accross the virtio code where
> >needed. These accessors should support both legacy and 1.0 devices.
> >A good way to do it is to introduce a per-device property to store the
> >endianness. We choose to set this flag at device reset time because it
> >is reasonnable to assume the endianness won't change unless we reboot or
> >kexec another kernel. And it is also reasonnable to assume the new kernel
> >will reset the devices before using them (otherwise it will break).
> >
> >We reuse the virtio_is_big_endian() helper since it provides the right
> >value for legacy devices with most of the targets, that have fixed
> >endianness. It can then be overriden to support endian-ambivalent
> >targets.
> >
> >To support migration, we need to set the flag in virtio_load() as well.
> >
> >(a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> >- since this only affects a few targets, the field should be put into a
> > subsection
> >- virtio migration code should be ported to vmstate to be able to
> >introduce
> > such a subsection
> >
> >(b) If we assume the following to be true:
> >- target endianness falls under some cpu state
> >- cpu state is always restored before virtio devices state because they
> > get initialized in this order in main().
> >Then an alternative is to rely on virtio_is_big_endian() again at
> >load time. No need to mess around with the migration stream in this case.
> >
> >This patch implements (b).
> >
> >Note that the tswap helpers are implemented in virtio.c so that
> >virtio-access.h stays platform independant. Most of the virtio code
> >will be buildable under common-obj instead of obj then, and spare
> >some cycles when building for multiple targets.
> >
> >Signed-off-by: Rusty Russell
> >[ ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> > add VirtIODevice * arg to virtio helpers,
> > use the existing virtio_is_big_endian() helper,
> > virtio-pci: use the device is_big_endian flag,
> > introduce virtio tswap16 and tswap64 helpers,
> > move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> > migration support,
> > Greg Kurz ]
> >Cc: Cédric Le Goater
> >Signed-off-by: Greg Kurz
> >---
> >
> >Changes since v6:
> >- merge the virtio_needs_byteswap() helper from v6 and existing
> > virtio_is_big_endian()
> >- virtio-pci: now supports endianness changes
> >- virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> >diff --git a/exec.c b/exec.c
> >index 91513c6..e6777d0 100644
> >--- a/exec.c
> >+++ b/exec.c
> >@@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> >+#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu,
> >target_ulong addr,
> > * A helper function for the _utterly broken_ virtio device model to
> > find out if
> > * it's running on a big endian machine. Don't do this at home kids!
> > */
> >-bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> >diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >index 1ba53d9..68c3064 100644
> >--- a/hw/virtio/Makefile.objs
> >+++ b/hw/virtio/Makefile.objs
> >@@ -4,5 +4
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:38:14PM +0200, Alexander Graf wrote: > > On 14.04.14 14:34, Michael S. Tsirkin wrote: > >On Mon, Apr 14, 2014 at 02:31:06PM +0200, Alexander Graf wrote: > >>On 14.04.14 14:28, Michael S. Tsirkin wrote: > >>>On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf wrote: > Could we try to poison any non-virtio, non-endian-specific memory > accessors when this file is included? That way we don't fall into > pitfalls where we end up running stl_p in a tiny corner case > somewhere still. > > > Alex > >>>Not sure about all users but it looks like the only file including this > >>>is virtio.c right? > >>>I'm guessing that's safe there. > >>any virtio device implementations, since they need to communicate > >>with the guest :). > >In that case how can we poison regular memory accesses? > >Devices normally do more than virtio related things. > > Device should never do anything based on the guest endianness > (because they don't know), so stx_p and ldx_p should always be > poisoned. > > > Alex Makes sense.
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:37, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
On 14.04.14 14:24, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_confi
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, 14 Apr 2014 14:16:03 +0200
Alexander Graf wrote:
>
> On 14.04.14 13:58, Greg Kurz wrote:
> > From: Rusty Russell
> >
> > 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 it all little endian.
> >
> > We introduce memory accessors to be used accross the virtio code where
> > needed. These accessors should support both legacy and 1.0 devices.
> > A good way to do it is to introduce a per-device property to store the
> > endianness. We choose to set this flag at device reset time because it
> > is reasonnable to assume the endianness won't change unless we reboot or
> > kexec another kernel. And it is also reasonnable to assume the new kernel
> > will reset the devices before using them (otherwise it will break).
> >
> > We reuse the virtio_is_big_endian() helper since it provides the right
> > value for legacy devices with most of the targets, that have fixed
> > endianness. It can then be overriden to support endian-ambivalent targets.
> >
> > To support migration, we need to set the flag in virtio_load() as well.
> >
> > (a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> > - since this only affects a few targets, the field should be put into a
> >subsection
> > - virtio migration code should be ported to vmstate to be able to introduce
> >such a subsection
> >
> > (b) If we assume the following to be true:
> > - target endianness falls under some cpu state
> > - cpu state is always restored before virtio devices state because they
> >get initialized in this order in main().
> > Then an alternative is to rely on virtio_is_big_endian() again at
> > load time. No need to mess around with the migration stream in this case.
> >
> > This patch implements (b).
> >
> > Note that the tswap helpers are implemented in virtio.c so that
> > virtio-access.h stays platform independant. Most of the virtio code
> > will be buildable under common-obj instead of obj then, and spare
> > some cycles when building for multiple targets.
> >
> > Signed-off-by: Rusty Russell
> > [ ldq_phys() API change,
> >relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> >add VirtIODevice * arg to virtio helpers,
> >use the existing virtio_is_big_endian() helper,
> >virtio-pci: use the device is_big_endian flag,
> >introduce virtio tswap16 and tswap64 helpers,
> >move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> >migration support,
> >Greg Kurz ]
> > Cc: Cédric Le Goater
> > Signed-off-by: Greg Kurz
> > ---
> >
> > Changes since v6:
> > - merge the virtio_needs_byteswap() helper from v6 and existing
> >virtio_is_big_endian()
> > - virtio-pci: now supports endianness changes
> > - virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> > diff --git a/exec.c b/exec.c
> > index 91513c6..e6777d0 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> > +#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >
> > @@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> > addr,
> >* A helper function for the _utterly broken_ virtio device model to find
> > out if
> >* it's running on a big endian machine. Don't do this at home kids!
> >*/
> > -bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> > diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> > index 1ba53d9..68c3064 100644
> > --- a/hw/virtio/Makefile.objs
> > +++ b/hw/virtio/Makefile.objs
> > @@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> > common-obj-y += virtio-mmio.o
> > common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >
> > -obj-y += virtio.o virtio-balloon.o
> > +obj-y += virtio.o virtio-balloon.o
> > obj-$(CONFIG_LINUX) += vhost.o
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index ce97514..82a1689 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -89,9 +89,6 @@
> > /* Flags track per-device state like workarounds for quirks in older
> > guests. */
> > #define VIRTIO_PCI_FLAG_BUS_MASTER_
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:34, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:31:06PM +0200, Alexander Graf wrote: On 14.04.14 14:28, Michael S. Tsirkin wrote: On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf wrote: Could we try to poison any non-virtio, non-endian-specific memory accessors when this file is included? That way we don't fall into pitfalls where we end up running stl_p in a tiny corner case somewhere still. Alex Not sure about all users but it looks like the only file including this is virtio.c right? I'm guessing that's safe there. any virtio device implementations, since they need to communicate with the guest :). In that case how can we poison regular memory accesses? Devices normally do more than virtio related things. Device should never do anything based on the guest endianness (because they don't know), so stx_p and ldx_p should always be poisoned. Alex
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:29:20PM +0200, Alexander Graf wrote:
>
> On 14.04.14 14:24, Michael S. Tsirkin wrote:
> >On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
> >>On 14.04.14 13:58, Greg Kurz wrote:
> >>>From: Rusty Russell
> >>>
> >>>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 it all little endian.
> >>>
> >>>We introduce memory accessors to be used accross the virtio code where
> >>>needed. These accessors should support both legacy and 1.0 devices.
> >>>A good way to do it is to introduce a per-device property to store the
> >>>endianness. We choose to set this flag at device reset time because it
> >>>is reasonnable to assume the endianness won't change unless we reboot or
> >>>kexec another kernel. And it is also reasonnable to assume the new kernel
> >>>will reset the devices before using them (otherwise it will break).
> >>>
> >>>We reuse the virtio_is_big_endian() helper since it provides the right
> >>>value for legacy devices with most of the targets, that have fixed
> >>>endianness. It can then be overriden to support endian-ambivalent targets.
> >>>
> >>>To support migration, we need to set the flag in virtio_load() as well.
> >>>
> >>>(a) One solution would be to add it to the stream, but it have some
> >>> drawbacks:
> >>>- since this only affects a few targets, the field should be put into a
> >>> subsection
> >>>- virtio migration code should be ported to vmstate to be able to introduce
> >>> such a subsection
> >>>
> >>>(b) If we assume the following to be true:
> >>>- target endianness falls under some cpu state
> >>>- cpu state is always restored before virtio devices state because they
> >>> get initialized in this order in main().
> >>>Then an alternative is to rely on virtio_is_big_endian() again at
> >>>load time. No need to mess around with the migration stream in this case.
> >>>
> >>>This patch implements (b).
> >>>
> >>>Note that the tswap helpers are implemented in virtio.c so that
> >>>virtio-access.h stays platform independant. Most of the virtio code
> >>>will be buildable under common-obj instead of obj then, and spare
> >>>some cycles when building for multiple targets.
> >>>
> >>>Signed-off-by: Rusty Russell
> >>>[ ldq_phys() API change,
> >>> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >>> introduce a per-device is_big_endian flag (supersedes needs_byteswap
> >>> global)
> >>> add VirtIODevice * arg to virtio helpers,
> >>> use the existing virtio_is_big_endian() helper,
> >>> virtio-pci: use the device is_big_endian flag,
> >>> introduce virtio tswap16 and tswap64 helpers,
> >>> move calls to tswap* out of virtio-access.h to make it platform
> >>> independant,
> >>> migration support,
> >>> Greg Kurz ]
> >>>Cc: Cédric Le Goater
> >>>Signed-off-by: Greg Kurz
> >>>---
> >>>
> >>>Changes since v6:
> >>>- merge the virtio_needs_byteswap() helper from v6 and existing
> >>> virtio_is_big_endian()
> >>>- virtio-pci: now supports endianness changes
> >>>- virtio-access.h fixes (target independant)
> >>>
> >>> exec.c|2 -
> >>> hw/virtio/Makefile.objs |2 -
> >>> hw/virtio/virtio-pci.c| 11 +--
> >>> hw/virtio/virtio.c| 35 +
> >>> include/hw/virtio/virtio-access.h | 138
> >>> +
> >>> include/hw/virtio/virtio.h|2 +
> >>> vl.c |4 +
> >>> 7 files changed, 185 insertions(+), 9 deletions(-)
> >>> create mode 100644 include/hw/virtio/virtio-access.h
> >>>
> >>>diff --git a/exec.c b/exec.c
> >>>index 91513c6..e6777d0 100644
> >>>--- a/exec.c
> >>>+++ b/exec.c
> >>>@@ -42,6 +42,7 @@
> >>> #else /* !CONFIG_USER_ONLY */
> >>> #include "sysemu/xen-mapcache.h"
> >>> #include "trace.h"
> >>>+#include "hw/virtio/virtio.h"
> >>> #endif
> >>> #include "exec/cpu-all.h"
> >>>@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> >>>addr,
> >>> * A helper function for the _utterly broken_ virtio device model to
> >>> find out if
> >>> * it's running on a big endian machine. Don't do this at home kids!
> >>> */
> >>>-bool virtio_is_big_endian(void);
> >>> bool virtio_is_big_endian(void)
> >>> {
> >>> #if defined(TARGET_WORDS_BIGENDIAN)
> >>>diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >>>index 1ba53d9..68c3064 100644
> >>>--- a/hw/virtio/Makefile.objs
> >>>+++ b/hw/virtio/Makefile.objs
> >>>@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> >>> common-obj-y += virtio-mmio.o
> >>> common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >>>-obj-y += virtio.o virtio-balloon.o
> >>>+obj-y += virtio.o virtio-balloon.o
> >>> obj-$(CONFIG_LINUX) += vhost.o
> >>>diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>index ce97514..82a1
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:31:06PM +0200, Alexander Graf wrote:
>
> On 14.04.14 14:28, Michael S. Tsirkin wrote:
> >On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf wrote:
> >>On 14.04.14 13:58, Greg Kurz wrote:
> >>>From: Rusty Russell
> >>>
> >>>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 it all little endian.
> >>>
> >>>We introduce memory accessors to be used accross the virtio code where
> >>>needed. These accessors should support both legacy and 1.0 devices.
> >>>A good way to do it is to introduce a per-device property to store the
> >>>endianness. We choose to set this flag at device reset time because it
> >>>is reasonnable to assume the endianness won't change unless we reboot or
> >>>kexec another kernel. And it is also reasonnable to assume the new kernel
> >>>will reset the devices before using them (otherwise it will break).
> >>>
> >>>We reuse the virtio_is_big_endian() helper since it provides the right
> >>>value for legacy devices with most of the targets, that have fixed
> >>>endianness. It can then be overriden to support endian-ambivalent targets.
> >>>
> >>>To support migration, we need to set the flag in virtio_load() as well.
> >>>
> >>>(a) One solution would be to add it to the stream, but it have some
> >>> drawbacks:
> >>>- since this only affects a few targets, the field should be put into a
> >>> subsection
> >>>- virtio migration code should be ported to vmstate to be able to introduce
> >>> such a subsection
> >>>
> >>>(b) If we assume the following to be true:
> >>>- target endianness falls under some cpu state
> >>>- cpu state is always restored before virtio devices state because they
> >>> get initialized in this order in main().
> >>>Then an alternative is to rely on virtio_is_big_endian() again at
> >>>load time. No need to mess around with the migration stream in this case.
> >>>
> >>>This patch implements (b).
> >>>
> >>>Note that the tswap helpers are implemented in virtio.c so that
> >>>virtio-access.h stays platform independant. Most of the virtio code
> >>>will be buildable under common-obj instead of obj then, and spare
> >>>some cycles when building for multiple targets.
> >>>
> >>>Signed-off-by: Rusty Russell
> >>>[ ldq_phys() API change,
> >>> relicensed virtio-access.h to GPLv2+ on Rusty's request,
> >>> introduce a per-device is_big_endian flag (supersedes needs_byteswap
> >>> global)
> >>> add VirtIODevice * arg to virtio helpers,
> >>> use the existing virtio_is_big_endian() helper,
> >>> virtio-pci: use the device is_big_endian flag,
> >>> introduce virtio tswap16 and tswap64 helpers,
> >>> move calls to tswap* out of virtio-access.h to make it platform
> >>> independant,
> >>> migration support,
> >>> Greg Kurz ]
> >>>Cc: Cédric Le Goater
> >>>Signed-off-by: Greg Kurz
> >>>---
> >>>
> >>>Changes since v6:
> >>>- merge the virtio_needs_byteswap() helper from v6 and existing
> >>> virtio_is_big_endian()
> >>>- virtio-pci: now supports endianness changes
> >>>- virtio-access.h fixes (target independant)
> >>>
> >>> exec.c|2 -
> >>> hw/virtio/Makefile.objs |2 -
> >>> hw/virtio/virtio-pci.c| 11 +--
> >>> hw/virtio/virtio.c| 35 +
> >>> include/hw/virtio/virtio-access.h | 138
> >>> +
> >>> include/hw/virtio/virtio.h|2 +
> >>> vl.c |4 +
> >>> 7 files changed, 185 insertions(+), 9 deletions(-)
> >>> create mode 100644 include/hw/virtio/virtio-access.h
> >>>
> >>>diff --git a/exec.c b/exec.c
> >>>index 91513c6..e6777d0 100644
> >>>--- a/exec.c
> >>>+++ b/exec.c
> >>>@@ -42,6 +42,7 @@
> >>> #else /* !CONFIG_USER_ONLY */
> >>> #include "sysemu/xen-mapcache.h"
> >>> #include "trace.h"
> >>>+#include "hw/virtio/virtio.h"
> >>> #endif
> >>> #include "exec/cpu-all.h"
> >>>@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> >>>addr,
> >>> * A helper function for the _utterly broken_ virtio device model to
> >>> find out if
> >>> * it's running on a big endian machine. Don't do this at home kids!
> >>> */
> >>>-bool virtio_is_big_endian(void);
> >>> bool virtio_is_big_endian(void)
> >>> {
> >>> #if defined(TARGET_WORDS_BIGENDIAN)
> >>>diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >>>index 1ba53d9..68c3064 100644
> >>>--- a/hw/virtio/Makefile.objs
> >>>+++ b/hw/virtio/Makefile.objs
> >>>@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> >>> common-obj-y += virtio-mmio.o
> >>> common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >>>-obj-y += virtio.o virtio-balloon.o
> >>>+obj-y += virtio.o virtio-balloon.o
> >>> obj-$(CONFIG_LINUX) += vhost.o
> >>>diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >>>index ce97514..82a1
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:28, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque,
hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, add
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 14:24, Michael S. Tsirkin wrote:
On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque,
hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, add
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:22:36PM +0200, Alexander Graf wrote:
>
> On 14.04.14 13:58, Greg Kurz wrote:
> >From: Rusty Russell
> >
> >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 it all little endian.
> >
> >We introduce memory accessors to be used accross the virtio code where
> >needed. These accessors should support both legacy and 1.0 devices.
> >A good way to do it is to introduce a per-device property to store the
> >endianness. We choose to set this flag at device reset time because it
> >is reasonnable to assume the endianness won't change unless we reboot or
> >kexec another kernel. And it is also reasonnable to assume the new kernel
> >will reset the devices before using them (otherwise it will break).
> >
> >We reuse the virtio_is_big_endian() helper since it provides the right
> >value for legacy devices with most of the targets, that have fixed
> >endianness. It can then be overriden to support endian-ambivalent targets.
> >
> >To support migration, we need to set the flag in virtio_load() as well.
> >
> >(a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> >- since this only affects a few targets, the field should be put into a
> > subsection
> >- virtio migration code should be ported to vmstate to be able to introduce
> > such a subsection
> >
> >(b) If we assume the following to be true:
> >- target endianness falls under some cpu state
> >- cpu state is always restored before virtio devices state because they
> > get initialized in this order in main().
> >Then an alternative is to rely on virtio_is_big_endian() again at
> >load time. No need to mess around with the migration stream in this case.
> >
> >This patch implements (b).
> >
> >Note that the tswap helpers are implemented in virtio.c so that
> >virtio-access.h stays platform independant. Most of the virtio code
> >will be buildable under common-obj instead of obj then, and spare
> >some cycles when building for multiple targets.
> >
> >Signed-off-by: Rusty Russell
> >[ ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> > add VirtIODevice * arg to virtio helpers,
> > use the existing virtio_is_big_endian() helper,
> > virtio-pci: use the device is_big_endian flag,
> > introduce virtio tswap16 and tswap64 helpers,
> > move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> > migration support,
> > Greg Kurz ]
> >Cc: Cédric Le Goater
> >Signed-off-by: Greg Kurz
> >---
> >
> >Changes since v6:
> >- merge the virtio_needs_byteswap() helper from v6 and existing
> > virtio_is_big_endian()
> >- virtio-pci: now supports endianness changes
> >- virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> >diff --git a/exec.c b/exec.c
> >index 91513c6..e6777d0 100644
> >--- a/exec.c
> >+++ b/exec.c
> >@@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> >+#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> >addr,
> > * A helper function for the _utterly broken_ virtio device model to find
> > out if
> > * it's running on a big endian machine. Don't do this at home kids!
> > */
> >-bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> >diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >index 1ba53d9..68c3064 100644
> >--- a/hw/virtio/Makefile.objs
> >+++ b/hw/virtio/Makefile.objs
> >@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> > common-obj-y += virtio-mmio.o
> > common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >-obj-y += virtio.o virtio-balloon.o
> >+obj-y += virtio.o virtio-balloon.o
> > obj-$(CONFIG_LINUX) += vhost.o
> >diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >index ce97514..82a1689 100644
> >--- a/hw/virtio/virtio-pci.c
> >+++ b/hw/virtio/virtio-pci.c
> >@@ -89,9 +89,6 @@
> > /* Flags track per-device state like workarounds for quirks in older
> > guests. */
> > #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
> >-/* HACK for virtio to determine if it's running a big endian guest */
> >-bool virtio_is
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On Mon, Apr 14, 2014 at 02:16:03PM +0200, Alexander Graf wrote:
>
> On 14.04.14 13:58, Greg Kurz wrote:
> >From: Rusty Russell
> >
> >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 it all little endian.
> >
> >We introduce memory accessors to be used accross the virtio code where
> >needed. These accessors should support both legacy and 1.0 devices.
> >A good way to do it is to introduce a per-device property to store the
> >endianness. We choose to set this flag at device reset time because it
> >is reasonnable to assume the endianness won't change unless we reboot or
> >kexec another kernel. And it is also reasonnable to assume the new kernel
> >will reset the devices before using them (otherwise it will break).
> >
> >We reuse the virtio_is_big_endian() helper since it provides the right
> >value for legacy devices with most of the targets, that have fixed
> >endianness. It can then be overriden to support endian-ambivalent targets.
> >
> >To support migration, we need to set the flag in virtio_load() as well.
> >
> >(a) One solution would be to add it to the stream, but it have some
> > drawbacks:
> >- since this only affects a few targets, the field should be put into a
> > subsection
> >- virtio migration code should be ported to vmstate to be able to introduce
> > such a subsection
> >
> >(b) If we assume the following to be true:
> >- target endianness falls under some cpu state
> >- cpu state is always restored before virtio devices state because they
> > get initialized in this order in main().
> >Then an alternative is to rely on virtio_is_big_endian() again at
> >load time. No need to mess around with the migration stream in this case.
> >
> >This patch implements (b).
> >
> >Note that the tswap helpers are implemented in virtio.c so that
> >virtio-access.h stays platform independant. Most of the virtio code
> >will be buildable under common-obj instead of obj then, and spare
> >some cycles when building for multiple targets.
> >
> >Signed-off-by: Rusty Russell
> >[ ldq_phys() API change,
> > relicensed virtio-access.h to GPLv2+ on Rusty's request,
> > introduce a per-device is_big_endian flag (supersedes needs_byteswap
> > global)
> > add VirtIODevice * arg to virtio helpers,
> > use the existing virtio_is_big_endian() helper,
> > virtio-pci: use the device is_big_endian flag,
> > introduce virtio tswap16 and tswap64 helpers,
> > move calls to tswap* out of virtio-access.h to make it platform
> > independant,
> > migration support,
> > Greg Kurz ]
> >Cc: Cédric Le Goater
> >Signed-off-by: Greg Kurz
> >---
> >
> >Changes since v6:
> >- merge the virtio_needs_byteswap() helper from v6 and existing
> > virtio_is_big_endian()
> >- virtio-pci: now supports endianness changes
> >- virtio-access.h fixes (target independant)
> >
> > exec.c|2 -
> > hw/virtio/Makefile.objs |2 -
> > hw/virtio/virtio-pci.c| 11 +--
> > hw/virtio/virtio.c| 35 +
> > include/hw/virtio/virtio-access.h | 138
> > +
> > include/hw/virtio/virtio.h|2 +
> > vl.c |4 +
> > 7 files changed, 185 insertions(+), 9 deletions(-)
> > create mode 100644 include/hw/virtio/virtio-access.h
> >
> >diff --git a/exec.c b/exec.c
> >index 91513c6..e6777d0 100644
> >--- a/exec.c
> >+++ b/exec.c
> >@@ -42,6 +42,7 @@
> > #else /* !CONFIG_USER_ONLY */
> > #include "sysemu/xen-mapcache.h"
> > #include "trace.h"
> >+#include "hw/virtio/virtio.h"
> > #endif
> > #include "exec/cpu-all.h"
> >@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
> >addr,
> > * A helper function for the _utterly broken_ virtio device model to find
> > out if
> > * it's running on a big endian machine. Don't do this at home kids!
> > */
> >-bool virtio_is_big_endian(void);
> > bool virtio_is_big_endian(void)
> > {
> > #if defined(TARGET_WORDS_BIGENDIAN)
> >diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> >index 1ba53d9..68c3064 100644
> >--- a/hw/virtio/Makefile.objs
> >+++ b/hw/virtio/Makefile.objs
> >@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
> > common-obj-y += virtio-mmio.o
> > common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
> >-obj-y += virtio.o virtio-balloon.o
> >+obj-y += virtio.o virtio-balloon.o
> > obj-$(CONFIG_LINUX) += vhost.o
> >diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >index ce97514..82a1689 100644
> >--- a/hw/virtio/virtio-pci.c
> >+++ b/hw/virtio/virtio-pci.c
> >@@ -89,9 +89,6 @@
> > /* Flags track per-device state like workarounds for quirks in older
> > guests. */
> > #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
> >-/* HACK for virtio to determine if it's running a big endian guest */
> >-bool virtio_is
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
-if (virtio_is_big_endian()) {
+if (vdev->is_big_endian) {
val
Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell
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 it all little endian.
We introduce memory accessors to be used accross the virtio code where
needed. These accessors should support both legacy and 1.0 devices.
A good way to do it is to introduce a per-device property to store the
endianness. We choose to set this flag at device reset time because it
is reasonnable to assume the endianness won't change unless we reboot or
kexec another kernel. And it is also reasonnable to assume the new kernel
will reset the devices before using them (otherwise it will break).
We reuse the virtio_is_big_endian() helper since it provides the right
value for legacy devices with most of the targets, that have fixed
endianness. It can then be overriden to support endian-ambivalent targets.
To support migration, we need to set the flag in virtio_load() as well.
(a) One solution would be to add it to the stream, but it have some
drawbacks:
- since this only affects a few targets, the field should be put into a
subsection
- virtio migration code should be ported to vmstate to be able to introduce
such a subsection
(b) If we assume the following to be true:
- target endianness falls under some cpu state
- cpu state is always restored before virtio devices state because they
get initialized in this order in main().
Then an alternative is to rely on virtio_is_big_endian() again at
load time. No need to mess around with the migration stream in this case.
This patch implements (b).
Note that the tswap helpers are implemented in virtio.c so that
virtio-access.h stays platform independant. Most of the virtio code
will be buildable under common-obj instead of obj then, and spare
some cycles when building for multiple targets.
Signed-off-by: Rusty Russell
[ ldq_phys() API change,
relicensed virtio-access.h to GPLv2+ on Rusty's request,
introduce a per-device is_big_endian flag (supersedes needs_byteswap global)
add VirtIODevice * arg to virtio helpers,
use the existing virtio_is_big_endian() helper,
virtio-pci: use the device is_big_endian flag,
introduce virtio tswap16 and tswap64 helpers,
move calls to tswap* out of virtio-access.h to make it platform independant,
migration support,
Greg Kurz ]
Cc: Cédric Le Goater
Signed-off-by: Greg Kurz
---
Changes since v6:
- merge the virtio_needs_byteswap() helper from v6 and existing
virtio_is_big_endian()
- virtio-pci: now supports endianness changes
- virtio-access.h fixes (target independant)
exec.c|2 -
hw/virtio/Makefile.objs |2 -
hw/virtio/virtio-pci.c| 11 +--
hw/virtio/virtio.c| 35 +
include/hw/virtio/virtio-access.h | 138 +
include/hw/virtio/virtio.h|2 +
vl.c |4 +
7 files changed, 185 insertions(+), 9 deletions(-)
create mode 100644 include/hw/virtio/virtio-access.h
diff --git a/exec.c b/exec.c
index 91513c6..e6777d0 100644
--- a/exec.c
+++ b/exec.c
@@ -42,6 +42,7 @@
#else /* !CONFIG_USER_ONLY */
#include "sysemu/xen-mapcache.h"
#include "trace.h"
+#include "hw/virtio/virtio.h"
#endif
#include "exec/cpu-all.h"
@@ -2745,7 +2746,6 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
* A helper function for the _utterly broken_ virtio device model to find out
if
* it's running on a big endian machine. Don't do this at home kids!
*/
-bool virtio_is_big_endian(void);
bool virtio_is_big_endian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 1ba53d9..68c3064 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -4,5 +4,5 @@ common-obj-y += virtio-bus.o
common-obj-y += virtio-mmio.o
common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += dataplane/
-obj-y += virtio.o virtio-balloon.o
+obj-y += virtio.o virtio-balloon.o
obj-$(CONFIG_LINUX) += vhost.o
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..82a1689 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 << 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
-if (virtio_is_big_endian()) {
+if (vdev->is_big_endian) {
val
