[Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian targets

[Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian targets

[David Gibson]

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

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/char/virtio-serial-bus.c       | 43 ++++++++++++++++++---------------------
 include/hw/virtio/virtio-serial.h |  2 --
 2 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index a7b1b68..901a502 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -482,10 +482,13 @@ 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;
+    VirtIOSerial *vser = VIRTIO_SERIAL(vdev);
+    struct virtio_console_config *config = config_data;
 
-    vser = VIRTIO_SERIAL(vdev);
-    memcpy(config_data, &vser->config, sizeof(struct virtio_console_config));
+    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 +536,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 +551,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));
 
     /* The ports map */
-    max_nr_ports = virtio_tswap32(vdev, s->config.max_nr_ports);
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -715,13 +714,14 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
     qemu_get_be16s(f, (uint16_t *) &tmp);
     qemu_get_be32s(f, &tmp);
 
-    /* Note: this is the only location where we use tswap32() instead of
-     * virtio_tswap32() because:
-     * - virtio_tswap32() only makes sense when the device is fully restored
-     * - the target endianness that was used to populate s->config is
-     *   necessarly the default one
+    /* Note: Usually we get the maximum number of ports from config
+     * space.  Unfortunately there it's in guest endian, and we don't
+     * yet know what that is, because it hasn't been loaded from the
+     * migration stream.  We use the host endian copy in the
+     * virtio_serial_conf structure (in fact, config space is
+     * initially populated from there)
      */
-    max_nr_ports = tswap32(s->config.max_nr_ports);
+    max_nr_ports = s->serial.max_virtserial_ports;
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_get_be32s(f, &ports_map);
 
@@ -784,10 +784,9 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
 /* This function is only used if a port id is not provided by the user */
 static uint32_t find_free_port_id(VirtIOSerial *vser)
 {
-    VirtIODevice *vdev = VIRTIO_DEVICE(vser);
     unsigned int i, max_nr_ports;
 
-    max_nr_ports = virtio_tswap32(vdev, vser->config.max_nr_ports);
+    max_nr_ports = vser->serial.max_virtserial_ports;
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         uint32_t map, bit;
 
@@ -890,7 +889,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    max_nr_ports = virtio_tswap32(vdev, port->vser->config.max_nr_ports);
+    max_nr_ports = port->vser->serial.max_virtserial_ports;
     if (port->id >= max_nr_ports) {
         error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
                          "max. allowed: %u", max_nr_ports - 1);
@@ -995,8 +994,6 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
         vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
     }
 
-    vser->config.max_nr_ports =
-        virtio_tswap32(vdev, vser->serial.max_virtserial_ports);
     vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32)
         * sizeof(vser->ports_map[0]));
     /*
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index a679e54..11af978 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -207,8 +207,6 @@ struct VirtIOSerial {
     /* bitmap for identifying active ports */
     uint32_t *ports_map;
 
-    struct virtio_console_config config;
-
     struct VirtIOSerialPostLoad *post_load;
 
     virtio_serial_conf serial;
-- 
2.1.0

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3479 bytes --]

On Fri, Dec 12, 2014 at 03:52:45PM +1100, 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.
>  * "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.

[snip]
> @@ -715,13 +714,14 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
>      qemu_get_be16s(f, (uint16_t *) &tmp);
>      qemu_get_be32s(f, &tmp);
>  
> -    /* Note: this is the only location where we use tswap32() instead of
> -     * virtio_tswap32() because:
> -     * - virtio_tswap32() only makes sense when the device is fully restored
> -     * - the target endianness that was used to populate s->config is
> -     *   necessarly the default one
> +    /* Note: Usually we get the maximum number of ports from config
> +     * space.  Unfortunately there it's in guest endian, and we don't
> +     * yet know what that is, because it hasn't been loaded from the
> +     * migration stream.  We use the host endian copy in the
> +     * virtio_serial_conf structure (in fact, config space is
> +     * initially populated from there)
>       */

Realised too late that this comment is no longer correct for the final
version and should just be dropped.  I'll resend without it, if people
think the patch is basically sane.can resend with it removed if
people think the patch is basically sane.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3776 bytes --]

On Fri, Dec 12, 2014 at 04:04:35PM +1100, David Gibson wrote:
> On Fri, Dec 12, 2014 at 03:52:45PM +1100, 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.
> >  * "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.
> 
> [snip]
> > @@ -715,13 +714,14 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f,
> >      qemu_get_be16s(f, (uint16_t *) &tmp);
> >      qemu_get_be32s(f, &tmp);
> >  
> > -    /* Note: this is the only location where we use tswap32() instead of
> > -     * virtio_tswap32() because:
> > -     * - virtio_tswap32() only makes sense when the device is fully restored
> > -     * - the target endianness that was used to populate s->config is
> > -     *   necessarly the default one
> > +    /* Note: Usually we get the maximum number of ports from config
> > +     * space.  Unfortunately there it's in guest endian, and we don't
> > +     * yet know what that is, because it hasn't been loaded from the
> > +     * migration stream.  We use the host endian copy in the
> > +     * virtio_serial_conf structure (in fact, config space is
> > +     * initially populated from there)
> >       */
> 
> Realised too late that this comment is no longer correct for the final
> version and should just be dropped.  I'll resend without it, if people
> think the patch is basically sane.can resend with it removed if
> people think the patch is basically sane.

Ugh.  Found some other problems.  Thought I'd compiled and tested it,
but apparently not :(.  I'll send a v2.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]