[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targ
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targets |
Date: |
Tue, 16 Dec 2014 09:43:30 +0530 |
On (Fri) 12 Dec 2014 [16:26:35], David Gibson wrote:
> On a bi-endian target, with a guest in the non-default endian mode,
> attempting to migrate twice in a row with a virtio-serial device wil
> cause a qemu SEGV on the second outgoing migration.
>
> The problem is that virtio_serial_save_device() (and other places) expect
> VirtIOSerial->config to be in current guest endianness. On a fresh boot,
> virtio_serial_device_realize() will initialize VirtIOSerial->config in
> default endianness. It's assumed the guest OS will make its true
> endianness known before the device is reset and initialized, then
> vser_reset adjusts VirtIOSerial->config into the new endianness.
>
> But on an incoming migration, the device isn't reset (after all the guest
> has a running driver as far as it's concerned), which means that
> VirtIOSerial->config retains its default endianness value from
> virtio_serial_device_realize().
>
> On a subsequent outgoing migration, virtio_serial_save_device() attempts
> to interpret VirtIOSerial->config.max_nr_ports in current endianness when
> its actually in default endianness and then runs off the end of the
> ports_map array in the loop immediately afterwards.
>
> We could fix this by adjusting VirtIOSerial->config into the correct
> current endianness after an incoming migration. But a better fix is just
> to get rid of VirtIOSerial->config entirely:
> * The virtio-serial config space is not settable, it always contains the
> values set at initialization
> * AFAICT "rows" and "cols" have never actually been used for anything and
> are always zero.
There were patches on the list a few years back to add resizing
support.
Also, ppc and s390 people were using this feature (why else would it
have been implemented?) -- since you're saying they're not in use, I
suppose ppc doesn't use it. CC'ing s390 people for comment.
> * "max_nr_ports" is initialized from
> VirtIOSerial->serial.max_virtserial_ports (host endian)
>
> So instead of maintaining this pointless guest-endian cache of the config
> data, we can just construct it directly into the correct current guest
> endian in the get_config hook. Current users of ->config can instead use
> the sources from which the config values were derived, which means they
> don't have to mess about with converting from guest endian at all.
I'd agree with this approach when I have confirmation no one actually
uses the {rows,cols}.
Since qemu doesn't use the rows and cols, it doesn't matter what
settings the dest host has; otherwise the dest host would have had to
adjust the guest to use the dest's settings for rows and cols after a
migration. Also, for "new" guests, they should use the control vq
command to adjust the rows and cols -- and they're not migrated since
they're not guest state.
> Signed-off-by: David Gibson <address@hidden>
> ---
> hw/char/virtio-serial-bus.c | 42
> +++++++++++++++------------------------
> include/hw/virtio/virtio-serial.h | 2 --
> 2 files changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index a7b1b68..dd5d7ec 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -482,10 +482,14 @@ static uint32_t get_features(VirtIODevice *vdev,
> uint32_t features)
> /* Guest requested config info */
> static void get_config(VirtIODevice *vdev, uint8_t *config_data)
> {
> - VirtIOSerial *vser;
> -
> - vser = VIRTIO_SERIAL(vdev);
> - memcpy(config_data, &vser->config, sizeof(struct virtio_console_config));
> + VirtIOSerial *vser = VIRTIO_SERIAL(vdev);
> + struct virtio_console_config *config =
> + (struct virtio_console_config *)config_data;
> +
> + config->cols = 0;
> + config->rows = 0;
> + config->max_nr_ports = virtio_tswap32(vdev,
> + vser->serial.max_virtserial_ports);
> }
>
> static void guest_reset(VirtIOSerial *vser)
> @@ -533,10 +537,6 @@ static void vser_reset(VirtIODevice *vdev)
>
> vser = VIRTIO_SERIAL(vdev);
> guest_reset(vser);
> -
> - /* In case we have switched endianness */
> - vser->config.max_nr_ports =
> - virtio_tswap32(vdev, vser->serial.max_virtserial_ports);
> }
>
> static void virtio_serial_save(QEMUFile *f, void *opaque)
> @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice
> *vdev, QEMUFile *f)
> uint32_t nr_active_ports;
> unsigned int i, max_nr_ports;
>
> - /* The config space */
> - qemu_put_be16s(f, &s->config.cols);
> - qemu_put_be16s(f, &s->config.rows);
> + max_nr_ports = s->serial.max_virtserial_ports;
>
> - qemu_put_be32s(f, &s->config.max_nr_ports);
> + /* Used to be config space, now redundant */
> + qemu_put_be16(f, 0);
> + qemu_put_be16(f, 0);
> + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports));
Can you split this patch so the config change and the max_nr_ports
change are separate? The max_nr_ports could similarly be ignored by
dest, right?
Thanks,
Amit