From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49878) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0jWT-0006sY-WE for qemu-devel@nongnu.org; Mon, 15 Dec 2014 23:14:14 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0jWP-0002Zx-M0 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 23:14:09 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55695) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0jWP-0002Zd-DY for qemu-devel@nongnu.org; Mon, 15 Dec 2014 23:14:05 -0500 Date: Tue, 16 Dec 2014 09:43:30 +0530 From: Amit Shah Message-ID: <20141216041330.GA19675@grmbl.mre> References: <1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [PATCHv2] Fix virtio-serial migration on bi-endian targets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mst@redhat.com, aik@ozlabs.ru, rusty@rustcorp.com.au, qemu-devel@nongnu.org, agraf@suse.de, borntraeger@de.ibm.com, mdroth@us.ibm.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 > --- > 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