From patchwork Fri Dec 12 05:26:35 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Gibson X-Patchwork-Id: 420365 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id E0DEA14003E for ; Fri, 12 Dec 2014 16:27:15 +1100 (AEDT) Received: from localhost ([::1]:55584 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIkz-0003g4-P4 for incoming@patchwork.ozlabs.org; Fri, 12 Dec 2014 00:27:13 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44324) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIkY-00033C-Hn for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XzIkU-00072n-3U for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:46 -0500 Received: from ozlabs.org ([103.22.144.67]:39830) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIkT-00072e-Gq for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:42 -0500 Received: by ozlabs.org (Postfix, from userid 1007) id 9BE2F14003E; Fri, 12 Dec 2014 16:26:39 +1100 (AEDT) From: David Gibson To: mst@redhat.com, amit.shah@redhat.com, rusty@rustcorp.com.au Date: Fri, 12 Dec 2014 16:26:35 +1100 Message-Id: <1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.1.0 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 103.22.144.67 Cc: aik@ozlabs.ru, David Gibson , agraf@suse.de, mdroth@us.ibm.com, qemu-devel@nongnu.org Subject: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targets X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 --- 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)); /* 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 +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); @@ -995,8 +987,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;