Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Fri, 13 Jun 2014 13:46:39 +0200
Alexander Graf <address@hidden> wrote:

> 
> On 13.06.14 13:23, Greg Kurz wrote:
> > Some CPU families can dynamically change their endianness. This means we
> > can have little endian ppc or big endian arm guests for example. This has
> > an impact on legacy virtio data structures since they are target endian.
> > We hence introduce a new property to track the endianness of each virtio
> > device. It is reasonnably assumed that endianness won't change while the
> > device is in use : we hence capture the device endianness when it gets
> > reset.
> >
> > We migrate this property in a subsection, after the device descriptor. This
> > means the load code must not rely on it until it is restored. As a 
> > consequence,
> > the vring sanity checks had to be moved after the call to 
> > vmstate_load_state().
> > We enforce paranoia by poisoning the property at the begining of 
> > virtio_load().
> >
> > Signed-off-by: Greg Kurz <address@hidden>
> > ---
> >
> >   Changes since last version (virtio migration RFC):
> >   - endianness cached at device reset time
> >   - reworked migration support
> >
> >   hw/virtio/virtio-pci.c     |    8 ++--
> >   hw/virtio/virtio.c         |  100 
> > +++++++++++++++++++++++++++++++++++++++-----
> >   include/hw/virtio/virtio.h |   12 ++++-
> >   3 files changed, 102 insertions(+), 18 deletions(-)
> >
> > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > index 390c8d2..2ffceb8 100644
> > --- a/hw/virtio/virtio-pci.c
> > +++ b/hw/virtio/virtio-pci.c
> > @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, 
> > hwaddr addr,
> >           break;
> >       case 2:
> >           val = virtio_config_readw(vdev, addr);
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap16(val);
> >           }
> >           break;
> >       case 4:
> >           val = virtio_config_readl(vdev, addr);
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap32(val);
> >           }
> >           break;
> > @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, 
> > hwaddr addr,
> >           virtio_config_writeb(vdev, addr, val);
> >           break;
> >       case 2:
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap16(val);
> >           }
> >           virtio_config_writew(vdev, addr, val);
> >           break;
> >       case 4:
> > -        if (virtio_is_big_endian()) {
> > +        if (virtio_is_big_endian(vdev)) {
> >               val = bswap32(val);
> >           }
> >           virtio_config_writel(vdev, addr, val);
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 16b73d9..6235df8 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
> >       vdev->status = val;
> >   }
> >   
> > +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> > +{
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> > +#else
> > +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +#endif
> > +}
> > +
> > +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu)
> > +{
> > +    if (cpu_virtio_is_big_endian(cpu)) {
> > +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> > +    } else {
> > +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +    }
> > +}
> > +
> >   void virtio_reset(void *opaque)
> >   {
> >       VirtIODevice *vdev = opaque;
> > @@ -552,6 +570,13 @@ void virtio_reset(void *opaque)
> >       int i;
> >   
> >       virtio_set_status(vdev, 0);
> > +    if (current_cpu) {
> > +        /* Guest initiated reset */
> > +        virtio_set_endian_cpu(vdev, current_cpu);
> > +    } else {
> > +        /* System reset */
> > +        virtio_set_endian_target_default(vdev);
> > +    }
> >   
> >       if (k->reset) {
> >           k->reset(vdev);
> > @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev)
> >       virtio_notify_vector(vdev, vdev->config_vector);
> >   }
> >   
> > +static bool virtio_device_endian_needed(void *opaque)
> > +{
> > +    VirtIODevice *vdev = opaque;
> > +
> > +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> > +#else
> > +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> > +#endif
> 
> if (target_words_bigendian()) {
>      return virtio_is_big_endian();
> } else {
>      return !virtio_is_big_endian();
> }
> 
> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a 
> step towards compiling virtio once ;).
> 
> 
> Alex
> 

Heh ! It used to be target_words_bigendian() in the first place and I turned
it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common
why don't you sort this out at compile time?" remarks. :)

Since we are definitely not on a fastpath here, I'll gladly fix this.

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