From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44316) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIkW-00032x-8u for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:45 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XzIkU-00072t-56 for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:44 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:52819) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XzIkT-00072f-Fj for qemu-devel@nongnu.org; Fri, 12 Dec 2014 00:26:42 -0500 Date: Fri, 12 Dec 2014 16:26:27 +1100 From: David Gibson Message-ID: <20141212052627.GE3735@voom.redhat.com> References: <1418359965-14041-1-git-send-email-david@gibson.dropbear.id.au> <20141212050435.GD3735@voom.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="GxcwvYAGnODwn7V8" Content-Disposition: inline In-Reply-To: <20141212050435.GD3735@voom.redhat.com> 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 --GxcwvYAGnODwn7V8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > The problem is that virtio_serial_save_device() (and other places) expe= ct > > VirtIOSerial->config to be in current guest endianness. On a fresh boo= t, > > 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 gue= st > > 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 wh= en > > 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 ju= st > > to get rid of VirtIOSerial->config entirely: > > * The virtio-serial config space is not settable, it always contains t= he > > 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 conf= ig > > data, we can just construct it directly into the correct current guest > > endian in the get_config hook. Current users of ->config can instead u= se > > 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. >=20 > [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 re= stored > > - * - 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) > > */ >=20 > 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. --=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 --GxcwvYAGnODwn7V8 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUinyCAAoJEGw4ysog2bOSwO8QAKfEgXUu6BJEfd4FDPWCIpJj 4Z2Drg2P3oFALbFR4yp0yygd6+AWz6JDoD8+6C3kWQny7o1iCAp1GBdREqphRroU JGHWzyRMsW2hFJj5Zs3LbnJtRUjlb7J3R+6QdDpk9Yz1qVvnt1nNvuIEUF2PiFIB KS4ZOsl4BMxbTsFaVUIlePO3Q2Gvdw7RLgT5WS7QlmXnLoCxFMAUIRyDE8jMGj72 QaITNRaenzbyyICkUHUWS9K3rDyMVyk3ad1swX4f6fj3DGLnRKTzCUCO2C15jWyL fhfhJg/yMn4rEKph9na1dH8qJqLmvlYQ1OkmpWACmtU9lMSd1F2G/QTxLMMFHtKm CTdnOQPNDd2A4Jxa0fS9IEr2QcWbfqD0aDmmSJ9k3QORW3dNKA6139tzuQ4wvCKZ Rwh3VGOzDRX5PuVwsq0jTkJ8qEUbxPIs9w5LtCMdN/wE3aJXGczB2dOctab/8dhT 4cFwt0cNILwR6PG/lVn5nYzasqwO7odiCO1cfquO+6HevEfF/7STahUxNQ3bTXsU eP55KDLFlHcSN/YbA3PIF5FZpN76lfQf5EJeAA/EXGV7TqDOscglKXDvO6xw6Lx6 2SkPtT42gD91CNhezX7lbU8Q1RsIHLqU34lC3kWInzeEPl4gZ47U0itPjHjVqbbU yv0zCVm/joqdJMjxUpIC =2njW -----END PGP SIGNATURE----- --GxcwvYAGnODwn7V8--