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

[Alexander Graf]

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