Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice

[Greg Kurz]

On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz <address@hidden> 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.
> 
> Of course this property must be part of the migration stream as most of
> the virtio code will depend on it. It is hence migrated in a subsection
> to preserve compatibility with older migration streams. The tricky part
> is that the endianness gets known quite late and we must ensure no access
> is made to virtio data before that. It is for example invalid to call
> vring_avail_idx() as does the actual migration code: the VQ indexes sanity
> checks had to be moved from virtio_load() to virtio_load_subsections()
> because of that.
> 
> We enforce some paranoia by making the endianness a 3-value enum so
> that we can temporarily poison it while loading state.
> 
> Signed-off-by: Greg Kurz <address@hidden>
> ---
>  exec.c                     |    8 +---
>  hw/virtio/virtio-pci.c     |   11 ++----
>  hw/virtio/virtio.c         |   80 
> +++++++++++++++++++++++++++++++++++++-------
>  include/exec/cpu-common.h  |    1 +
>  include/hw/virtio/virtio.h |   13 +++++++
>  5 files changed, 87 insertions(+), 26 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 91513c6..4e83588 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong 
> addr,
>  #endif
> 
>  #if !defined(CONFIG_USER_ONLY)
> -
> -/*
> - * A helper function for the _utterly broken_ virtio device model to find 
> out if
> - * it's running on a big endian machine. Don't do this at home kids!
> - */
> -bool virtio_is_big_endian(void);
> -bool virtio_is_big_endian(void)
> +bool target_words_bigendian(void)
>  {
>  #if defined(TARGET_WORDS_BIGENDIAN)
>      return true;
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index ce97514..2ffceb8 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -89,9 +89,6 @@
>  /* Flags track per-device state like workarounds for quirks in older guests. 
> */
>  #define VIRTIO_PCI_FLAG_BUS_MASTER_BUG  (1 << 0)
> 
> -/* HACK for virtio to determine if it's running a big endian guest */
> -bool virtio_is_big_endian(void);
> -
>  static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
>                                 VirtIOPCIProxy *dev);
> 
> @@ -409,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;
> @@ -443,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 7fbad29..6578854 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -539,6 +539,15 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
>      vdev->status = val;
>  }
> 
> +static void virtio_set_endian_target_default(VirtIODevice *vdev)
> +{
> +    if (target_words_bigendian()) {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
> +    } else {
> +        vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
> +    }
> +}
> +
>  void virtio_reset(void *opaque)
>  {
>      VirtIODevice *vdev = opaque;
> @@ -546,6 +555,7 @@ void virtio_reset(void *opaque)
>      int i;
> 
>      virtio_set_status(vdev, 0);
> +    virtio_set_endian_target_default(vdev);
> 
>      if (k->reset) {
>          k->reset(vdev);
> @@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
>      int version_id;
>      void (*save)(VirtIODevice *vdev, QEMUFile *f);
>      int (*load)(VirtIODevice *vdev, QEMUFile *f);
> -    int (*needed)(VirtIODevice *vdev);
> +    bool (*needed)(VirtIODevice *vdev);
>  } VirtIOSubsection;
> 
> +static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    qemu_put_byte(f, vdev->device_endian);
> +}
> +
> +static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
> +{
> +    vdev->device_endian = qemu_get_byte(f);
> +    return 0;
> +}
> +
> +static bool virtio_device_endian_needed(VirtIODevice *vdev)
> +{
> +    /* No migration is supposed to occur while we are loading state.
> +     */
> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +    if (target_words_bigendian()) {
> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
> +    } else {
> +        return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +    }
> +}
> +
>  static const VirtIOSubsection virtio_subsection[] = {
> +    { .name = "virtio/device_endian",

Can anyone comment the subsection name ? Is there a chance the
VMState port would come up with the same ?

> +      .version_id = 1,
> +      .save = virtio_save_device_endian,
> +      .load = virtio_load_device_endian,
> +      .needed = virtio_device_endian_needed,
> +    },
>      { .name = NULL }
>  };
> 
> @@ -868,6 +907,8 @@ static void virtio_save_subsections(VirtIODevice *vdev, 
> QEMUFile *f)
> 
>  static int virtio_load_subsections(VirtIODevice *vdev, QEMUFile *f)
>  {
> +    int i;
> +
>      while (qemu_peek_byte(f, 0) == QEMU_VM_SUBSECTION) {
>          char idstr[256];
>          uint8_t len, size;
> @@ -904,6 +945,26 @@ static int virtio_load_subsections(VirtIODevice *vdev, 
> QEMUFile *f)
>          }
>      }
> 
> +    for (i = 0; i < VIRTIO_PCI_QUEUE_MAX; i++) {
> +        if (vdev->vq[i].vring.num == 0) {
> +            break;
> +        }
> +
> +        if (vdev->vq[i].pa) {
> +            uint16_t nheads;
> +            nheads = vring_avail_idx(&vdev->vq[i]) - 
> vdev->vq[i].last_avail_idx;
> +            /* Check it isn't doing strange things with descriptor numbers. 
> */
> +            if (nheads > vdev->vq[i].vring.num) {
> +                error_report("VQ %d size 0x%x Guest index 0x%x "
> +                             "inconsistent with Host index 0x%x: delta 0x%x",
> +                             i, vdev->vq[i].vring.num,
> +                             vring_avail_idx(&vdev->vq[i]),
> +                             vdev->vq[i].last_avail_idx, nheads);
> +                return -1;
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
> 
> @@ -979,6 +1040,11 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(vdev);
> 
> +    /* We poison the endianness to ensure it does not get used before
> +     * subsections have been loaded.
> +     */
> +    vdev->device_endian = VIRTIO_DEVICE_ENDIAN_UNKNOWN;
> +
>      if (k->load_config) {
>          ret = k->load_config(qbus->parent, f);
>          if (ret)
> @@ -1012,18 +1078,7 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int 
> version_id)
>          vdev->vq[i].notification = true;
> 
>          if (vdev->vq[i].pa) {
> -            uint16_t nheads;
>              virtqueue_init(&vdev->vq[i]);
> -            nheads = vring_avail_idx(&vdev->vq[i]) - 
> vdev->vq[i].last_avail_idx;
> -            /* Check it isn't doing very strange things with descriptor 
> numbers. */
> -            if (nheads > vdev->vq[i].vring.num) {
> -                error_report("VQ %d size 0x%x Guest index 0x%x "
> -                             "inconsistent with Host index 0x%x: delta 0x%x",
> -                             i, vdev->vq[i].vring.num,
> -                             vring_avail_idx(&vdev->vq[i]),
> -                             vdev->vq[i].last_avail_idx, nheads);
> -                return -1;
> -            }
>          } else if (vdev->vq[i].last_avail_idx) {
>              error_report("VQ %d address 0x0 "
>                           "inconsistent with Host index 0x%x",
> @@ -1100,6 +1155,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>      }
>      vdev->vmstate = qemu_add_vm_change_state_handler(virtio_vmstate_change,
>                                                       vdev);
> +    virtio_set_endian_target_default(vdev);
>  }
> 
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n)
> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index a21b65a..eb798c1 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -122,4 +122,5 @@ void qemu_ram_foreach_block(RAMBlockIterFunc func, void 
> *opaque);
> 
>  #endif
> 
> +bool target_words_bigendian(void);
>  #endif /* !CPU_COMMON_H */
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 3505ce5..1c4a736 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -104,6 +104,12 @@ typedef struct VirtQueueElement
>  #define VIRTIO_DEVICE(obj) \
>          OBJECT_CHECK(VirtIODevice, (obj), TYPE_VIRTIO_DEVICE)
> 
> +enum virtio_device_endian {
> +    VIRTIO_DEVICE_ENDIAN_UNKNOWN,
> +    VIRTIO_DEVICE_ENDIAN_LITTLE,
> +    VIRTIO_DEVICE_ENDIAN_BIG,
> +};
> +
>  struct VirtIODevice
>  {
>      DeviceState parent_obj;
> @@ -121,6 +127,7 @@ struct VirtIODevice
>      bool vm_running;
>      VMChangeStateEntry *vmstate;
>      char *bus_name;
> +    enum virtio_device_endian device_endian;
>  };
> 
>  typedef struct VirtioDeviceClass {
> @@ -255,4 +262,10 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue 
> *vq, bool assign,
>                                                 bool set_handler);
>  void virtio_queue_notify_vq(VirtQueue *vq);
>  void virtio_irq(VirtQueue *vq);
> +
> +static inline bool virtio_is_big_endian(VirtIODevice *vdev)
> +{
> +    assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
> +    return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
> +}
>  #endif
> 
> 



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