qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alexander Graf
Subject: Re: [Qemu-devel] [PATCH v7 1/8] virtio: endian-ambivalent targets using legacy virtio
Date: Mon, 14 Apr 2014 15:15:13 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:24.0) Gecko/20100101 Thunderbird/24.4.0


On 14.04.14 15:12, Greg Kurz wrote:
On Mon, 14 Apr 2014 14:53:03 +0200
Alexander Graf <address@hidden> wrote:
On 14.04.14 14:50, Greg Kurz wrote:
On Mon, 14 Apr 2014 14:22:36 +0200
Alexander Graf <address@hidden> wrote:

On 14.04.14 13:58, Greg Kurz wrote:
From: Rusty Russell <address@hidden>

virtio data structures are defined as "target endian", which assumes
that's a fixed value.  In fact, that actually means it's platform-specific.
The OASIS virtio 1.0 spec will fix this, by making 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 <address@hidden>
[ 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 <address@hidden> ]
Cc: Cédric Le Goater <address@hidden>
Signed-off-by: Greg Kurz <address@hidden>
---
[...]

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]