From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58931) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0gni-0005MY-S7 for qemu-devel@nongnu.org; Mon, 15 Dec 2014 20:19:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Y0gne-0000qc-Jf for qemu-devel@nongnu.org; Mon, 15 Dec 2014 20:19:46 -0500 Received: from ozlabs.org ([2401:3900:2:1::2]:55178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Y0gne-0000lH-0N for qemu-devel@nongnu.org; Mon, 15 Dec 2014 20:19:42 -0500 Date: Tue, 16 Dec 2014 12:19:29 +1100 From: David Gibson Message-ID: <20141216011929.GG23547@voom.fritz.box> References: <1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au> <548EF76C.9000704@suse.de> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ChQOR20MqfxkMJg9" Content-Disposition: inline In-Reply-To: <548EF76C.9000704@suse.de> 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: Alexander Graf Cc: mst@redhat.com, aik@ozlabs.ru, rusty@rustcorp.com.au, qemu-devel@nongnu.org, mdroth@us.ibm.com, amit.shah@redhat.com --ChQOR20MqfxkMJg9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Dec 15, 2014 at 03:59:56PM +0100, Alexander Graf wrote: >=20 >=20 > On 12.12.14 06:26, 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 > > Signed-off-by: David Gibson >=20 > In general I agree with the patch, but ... [snip] > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevic= e *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); >=20 > ... are you 100% sure these are always unused and will stay unused? I > think we'd be better off just treating them the same way as max_nr_ports > and migrate them over - or at least add a comment here what the > variables mean so that whenever someone adds support for rows/cols they > know where to put them. Well, so. Here's what I know: * The rows and cols fields certainly aren't used at present (and we don't advertise the VIRTIO_CONSOLE_F_SIZE feature bit which says they are valid) * I've looked through the git history and I'm confident they've never been used. * The current incoming side migration code will ignore these fields. Prior to e38e943a it would load them, but then not do anything with the cols and rows fields as before. Thinking about it, even if rows/cols are used in future, it doesn't actually make sense to migrate them. They'll change in response to host events, not guest events, and the suitable values might change on migration. So it seems like it would be more sensible for the migration destination to generate correct values for itself and advertise them to the guest with an unconditional config_changed interrupt. --=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 --ChQOR20MqfxkMJg9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUj4ihAAoJEGw4ysog2bOSIGoP/A5p3oOPO48fLI7oFeyQ1YAL Yw+QWmiyt02ETXcLSIvknxzcyQBNaM/MgqWGDs6oQvORmLzNLnYsPgh4uIIapbd/ JeKqnHho3Rlo091gHLHnyo93Vl6H/zhbl+TE3Ae3KL4Sby1yqCkIDlOGmP3eBDqb T8Jfa5VJxVhl25pO/XHAc2beTbuF/m1M7sxY0OWaTCUdBDa71j+Vr9ztsyiSGxTc rM2HeDE7Fv8FppiP9T+Fh2D5aFNYqZ6JgK/5Vjt8yaxlKVC/lqW4oHppwWp9DnVF ED4Lv41R0qBJADNSPkaKPZZEm/VWMFraEg+Kcwty5rlplEeu/KcKTNuzeWX4PhYF oxPcjsA+LDqWiB2a7Xmq3hBmUiXUt1j4lFnrJeZPx25sgPlJQ/uoglVfzNH0e+Gu 0y3it4Zrzp80BXB2qbgdb+1v/+q4Nvs8BpwAAqxfDiqNU3X46IxWwiP5jOHfDWLg 3TzClc+5JWmfx6BsVqctkv2WzsDRimWMNP9uN/WPEFGEjq1FUq7tVs54KFa+QI/r 4nDYZeeD37iETF4m1B3R1mKymXLwExUklTdCa3A2kUMJz2Y8496f/EmkKJ8cyrCn IL4veFsJ/clmvGoX7a/5XagnL/NtGBvVzfyR8K0Re6cnq7F34CfUYQgxUzjEi3py 6tIK1nfqcBETEfYMvPMl =shW2 -----END PGP SIGNATURE----- --ChQOR20MqfxkMJg9--