From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43061) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIbP-0001N6-PN for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:17:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XzIbO-0004Mt-6y for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:17:19 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:57943) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIbN-0004MX-Rx for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:17:18 -0500 Date: Fri, 12 Dec 2014 16:04:35 +1100 From: David Gibson Message-ID: <20141212050435.GD3735@voom.redhat.com> References: <1418359965-14041-1-git-send-email-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3Gf/FFewwPeBMqCJ" Content-Disposition: inline In-Reply-To: <1418359965-14041-1-git-send-email-david@gibson.dropbear.id.au> Subject: Re: [Qemu-devel] [PATCH] Fix virtio-serial migration on bi-endian targets List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: mst@redhat.com, amit.shah@redhat.com, rusty@rustcorp.com.au Cc: aik@ozlabs.ru, agraf@suse.de, mdroth@us.ibm.com, qemu-devel@nongnu.org --3Gf/FFewwPeBMqCJ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > 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(). >=20 > 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. >=20 > 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) >=20 > 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); > =20 > - /* 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 rest= ored > - * - 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. --=20 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 --3Gf/FFewwPeBMqCJ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUindjAAoJEGw4ysog2bOSFUcP/2r5s+4CXkQ1XRoVphxKfnbw hJcMQmU+EO/AeObSjpVuE2FvD3gimDa6UyaIh+dwTsowTsQ2NfhTKxkJjpfKzA0M N50W//EHnAT/h5jnpNpO90PzNJ9RI5eK1hyf/9jVvw/obfhG7NkmOECBrIS9iuq5 zZlG0I2qsh91w7NP7+x1LZouKAVclWlyTUCn8aCZss8/0ymzKI4Z8OV9qX2K2Wj1 +BZd89tENz2unHbLzGcxbb4LdXXv9IzfLWDON9Cztdc3LLqjuPjfA6mSloGRwUc/ JjZmJ1B4djo9ayE14L8aR8YZDjuxluPgLFy0G/zdNHLed2QWE2ETYpvVE9Q11Fjw 5LwB8cwEtn8jPk+6tupOFWcgnHTUlsx5zznrPggB1YzPOR5uY/OJW9IWARfnxvDS 3duwB0h/9kr0bDzNgLppU5ejYH3B2afV2Szz0oye7utBqCCH27H9e/Ku3RvFx/fQ zECLVie8i4OEy6Ko0IRYFAlAtaF0SG8s9brZXITVKgKpUmymB6apMf3WM0gzdTbz 8PnnwlnnmNfswtiaiPTewnT2v0IqBByeF3lj7wbMMZTcY3SMm4uUXTgm3k7W35b5 ihWvichkRN+t+TY1jdk8ZUqA0Rf6moOGAUCvUJwOz2W5NJP3CAbTbZb8dTfv3lSE 8Wz5ERuQ6/5g/TanCxvm =x4Li -----END PGP SIGNATURE----- --3Gf/FFewwPeBMqCJ--