[Qemu-devel] [PATCHv3 0/2] Fix virtio-serial migration on bi-endian targets

[Qemu-devel] [PATCHv3 0/2] 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 in fact we
already have a host endian copy of the max number of ports readily
available, so it's simpler to just use that instead.  Patch 1/2 does
that.

Once that's done, it becomes clear that there's really no reason to
keep the guest-endian copy of the config space around persistently
(config accesses aren't a fast path, so it can be constructed when
necessary).  Patch 2/2 makes that cleanup.

[Qemu-devel] [PATCHv3 1/2] virtio_serial: Don't use vser->config.max_nr_ports internally

[David Gibson]

I number of places in the virtio_serial driver retrieve the number of ports
from vser->config.max_nr_ports, which is guest-endian.  But for internal
users, we already have a host-endian copy of the number of ports in
vser->serial.max_virtserial_ports.  Using that instead of the config field
removes the need for easy-to-forget byteswapping.

In particular this fixes a bug on incoming migration, where we don't adjust
the endianness vser->config correctly, because it hasn't yet been loaded
from the migration stream when virtio_serial_load_device() is called.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/char/virtio-serial-bus.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index a7b1b68..e013504 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -559,7 +559,7 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
     qemu_put_be32s(f, &s->config.max_nr_ports);
 
     /* The ports map */
-    max_nr_ports = virtio_tswap32(vdev, s->config.max_nr_ports);
+    max_nr_ports = s->serial.max_virtserial_ports;
     for (i = 0; i < (max_nr_ports + 31) / 32; i++) {
         qemu_put_be32s(f, &s->ports_map[i]);
     }
@@ -715,13 +715,7 @@ 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
-     */
-    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 +778,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;
 
@@ -848,7 +841,6 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp)
     VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
     VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
     VirtIOSerialBus *bus = VIRTIO_SERIAL_BUS(qdev_get_parent_bus(dev));
-    VirtIODevice *vdev = VIRTIO_DEVICE(bus->vser);
     int max_nr_ports;
     bool plugging_port0;
     Error *err = NULL;
@@ -890,7 +882,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);
-- 
2.1.0

[Qemu-devel] [PATCHv3 2/2] virtio-serial: Don't keep a persistent copy of config space

[David Gibson]

The 'config' field in the VirtIOSerial structure keeps a copy of the virtio
console's config space as visible to the guest, that is to say, in guest
endianness.  This is fiddly to maintain, because on some targets, such as
powerpc, the "guest endianness" can change when a new guest OS boots.

In fact, there's no need to maintain such a guest view of config space -
instead we can reconstruct it from host-format data when it is accessed
with get_config.

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

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index e013504..37a6f44 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)
@@ -551,12 +551,13 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f)
     VirtIOSerialPort *port;
     uint32_t nr_active_ports;
     unsigned int i, max_nr_ports;
+    struct virtio_console_config config;
 
-    /* The config space */
-    qemu_put_be16s(f, &s->config.cols);
-    qemu_put_be16s(f, &s->config.rows);
-
-    qemu_put_be32s(f, &s->config.max_nr_ports);
+    /* The config space (ignored on the far end in current versions) */
+    get_config(vdev, (uint8_t *)&config);
+    qemu_put_be16s(f, &config.cols);
+    qemu_put_be16s(f, &config.rows);
+    qemu_put_be32s(f, &config.max_nr_ports);
 
     /* The ports map */
     max_nr_ports = s->serial.max_virtserial_ports;
@@ -987,8 +988,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] [PATCHv3 0/2] Fix virtio-serial migration on bi-endian targets

[Alexander Graf]

On 19.12.14 04:57, 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 in fact we
> already have a host endian copy of the max number of ports readily
> available, so it's simpler to just use that instead.  Patch 1/2 does
> that.
> 
> Once that's done, it becomes clear that there's really no reason to
> keep the guest-endian copy of the config space around persistently
> (config accesses aren't a fast path, so it can be constructed when
> necessary).  Patch 2/2 makes that cleanup.

Please provide a version history of changes between versions. Also for
patches you'd expect PPC people to read, CC qemu-ppc (maybe doesn't
apply to this patch, but does to others) ;)


Alex

Re: [Qemu-devel] [PATCHv3 0/2] Fix virtio-serial migration on bi-endian targets

[Alexander Graf]

On 19.12.14 04:57, 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 in fact we
> already have a host endian copy of the max number of ports readily
> available, so it's simpler to just use that instead.  Patch 1/2 does
> that.
> 
> Once that's done, it becomes clear that there's really no reason to
> keep the guest-endian copy of the config space around persistently
> (config accesses aren't a fast path, so it can be constructed when
> necessary).  Patch 2/2 makes that cleanup.

Reviewed-by: Alexander Graf <agraf@suse.de>

Re: [Qemu-devel] [PATCHv3 0/2] Fix virtio-serial migration on bi-endian targets

[Amit Shah]

On (Fri) 19 Dec 2014 [14:57:25], 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 in fact we
> already have a host endian copy of the max number of ports readily
> available, so it's simpler to just use that instead.  Patch 1/2 does
> that.
> 
> Once that's done, it becomes clear that there's really no reason to
> keep the guest-endian copy of the config space around persistently
> (config accesses aren't a fast path, so it can be constructed when
> necessary).  Patch 2/2 makes that cleanup.

Thanks, applied.

		Amit