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

[Alexander Graf]

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.


Alex