From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34549) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0l2W-0006GW-P1 for qemu-devel@nongnu.org; Tue, 16 Dec 2014 00:51:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0l2O-0001Yn-8p for qemu-devel@nongnu.org; Tue, 16 Dec 2014 00:51:20 -0500 Received: from ozlabs.org ([103.22.144.67]:51503) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0l2N-0001Xh-MQ for qemu-devel@nongnu.org; Tue, 16 Dec 2014 00:51:12 -0500 Date: Tue, 16 Dec 2014 16:38:15 +1100 From: David Gibson Message-ID: <20141216053815.GL23547@voom.fritz.box> References: <1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au> <20141216041330.GA19675@grmbl.mre> <20141216043354.GK23547@voom.fritz.box> <20141216043849.GB19675@grmbl.mre> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="YQEH9CATo+4lan7A" Content-Disposition: inline In-Reply-To: <20141216043849.GB19675@grmbl.mre> 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: Amit Shah 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 --YQEH9CATo+4lan7A Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Dec 16, 2014 at 10:08:49AM +0530, Amit Shah wrote: > On (Tue) 16 Dec 2014 [15:33:54], David Gibson wrote: > > On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote: > > > 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. > > > >=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() att= empts > > > > to interpret VirtIOSerial->config.max_nr_ports in current endiannes= s 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 i= s just > > > > to get rid of VirtIOSerial->config entirely: > > > > * The virtio-serial config space is not settable, it always contai= ns the > > > > values set at initialization > > > > * AFAICT "rows" and "cols" have never actually been used for anyth= ing and > > > > are always zero. > > >=20 > > > There were patches on the list a few years back to add resizing > > > support. > >=20 > > Well, apparently they were never merged, because I can't see a thing > > in the git history. >=20 > Yes, the submitter didn't follow up after review. >=20 > > > 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. > > >=20 > > > > * "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 gu= est > > > > endian in the get_config hook. Current users of ->config can inste= ad 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. > > >=20 > > > I'd agree with this approach when I have confirmation no one actually > > > uses the {rows,cols}. > >=20 > > I've grepped the tree and searched through git history. I'm pretty > > sure they're not used. > >=20 > > And even if we do want to use them, this approach would still be sound > > - it would make more sense to hold the rows and cols info in host > > endian (which is well defined and static, at least) and just byteswap > > it on get_config() where necessary. > >=20 > > > 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. > >=20 > > Right, I came to the same conclusion (see my reply to agraf). > >=20 > > [snip] > > > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIOD= evice *vdev, QEMUFile *f) > > > > uint32_t nr_active_ports; > > > > unsigned int i, max_nr_ports; > > > > =20 > > > > - /* The config space */ > > > > - qemu_put_be16s(f, &s->config.cols); > > > > - qemu_put_be16s(f, &s->config.rows); > > > > + max_nr_ports =3D s->serial.max_virtserial_ports; > > > > =20 > > > > - 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)); > > >=20 > > > 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? > >=20 > > Um.. I'm not exactly sure where you're drawing the distinction between > > the two parts. Are you thinking > > patch 1) make config.max_nr_ports unused, by using > > max_virtserial_ports instead > > patch 2) eliminate the config field entirely >=20 > For this patch, I'm just suggesting to only touch cols and rows, and > lines that touch the max_nr_ports can be put in 2/2. Eliminating > config entirely may not be desirable, but if you want to do that, and > mark all fields as 'unused' in the savevm/loadvm functions, go for > it :-) That division really doesn't make sense to me. If rows and cols are removed, but max_nr_ports stays, then get_config has to become a weird hybrid where it copies some stuff directly from ->config and other bits from elsewhere. I really don't see any reason keeping config would be a desirable thing: it's a cache of information that doesn't need to be cached, and just adds complexity because of the endian issues. --=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 --YQEH9CATo+4lan7A Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUj8VHAAoJEGw4ysog2bOSTVkP/R3K4HDfGZayGqgsfYox7/zq iwVJRRXS1KFrcSG5eLHDIOzSWlGc7LdS8N/RIH/fZSHlL3Rm2nAmJIQwH90g7F0G bYZ6v53E+stxjyWUpB9bYDy8+u/edN9KaiimEaaF1stgdaiyJlDDm7y00VGpoee+ +1XF3b6KCqWUji3sxrTcK7qqCfxkYWMFQsVDk6mVysZz/x8v+olcD3O3fylnbLW6 aY9oANX8te9ypnbOv4pp1JVV7dZC76sC1aTaZkMmGZKZNbpxege0yMeh2RlqAig0 p8hq9zEI0vKLblqei9oTy3IcOWCjsBjhSOapr/ISobEU3/4eQDBncqoJT6ziF7iE pQ6fkLG2l0ESgMqCfQJBI2yvWUyT41LCyIB8HbLi/UwHvUtYa5Ye55vavIKgaJdS VLXMiuUxIxVLtUp0lXzCnrHjy2jYfypI7dD4Y2oPXXFEW2R/4MiLX0ovqzrjNTTc ABtUwyVqnUcuPmzVQYUjQoJ6GyK6eLDQ1Q4udxBOyiuFMdLSsllxODVhj6M4MeeM mmxQ/eYQ5xQXYyleQtGTCtXf+bfxOB7lLABPMipyVyCmOXNWTB63LUZql5Rs4jKx hyBAaMOmwipYGFr4JyJ6fyhANWxfJHHEdusEEwHphbNXQ7gnjGBUgIkMMrmMR1P7 Vl3jUeO8zEEXZwGR7+gG =WkCf -----END PGP SIGNATURE----- --YQEH9CATo+4lan7A--