On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz <gk...@linux.vnet.ibm.com> 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 <gk...@linux.vnet.ibm.com>
> ---
> 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 kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
"Anarchy is about taking complete responsibility for yourself."
Alan Moore.
