Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targets

[Amit Shah <amit.shah@redhat.com>]

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 <david@gibson.dropbear.id.au>
> ---
>  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