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

[Greg Kurz]

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. :)

-- 
Gregory Kurz                                     address@hidden
                                                 address@hidden
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.