On Mon, 14 Apr 2014 14:53:03 +0200 Alexander Graf <ag...@suse.de> wrote: > > On 14.04.14 14:50, Greg Kurz wrote: > > On Mon, 14 Apr 2014 14:22:36 +0200 > > Alexander Graf <ag...@suse.de> wrote: > > > >> On 14.04.14 13:58, Greg Kurz wrote: > >>> From: Rusty Russell <ru...@rustcorp.com.au> > >>> > >>> virtio data structures are defined as "target endian", which assumes > >>> that's a fixed value. In fact, that actually means it's > >>> platform-specific. > >>> The OASIS virtio 1.0 spec will fix this, by making 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 <ru...@rustcorp.com.au> > >>> [ 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 <gk...@linux.vnet.ibm.com> ] > >>> Cc: Cédric Le Goater <c...@fr.ibm.com> > >>> Signed-off-by: Greg Kurz <gk...@linux.vnet.ibm.com> > >>> --- > > [...] > > >>> + > >>> +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.