From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51506) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6BAE-0000uS-OZ for qemu-devel@nongnu.org; Mon, 14 Nov 2016 01:54:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6BAB-00047e-KS for qemu-devel@nongnu.org; Mon, 14 Nov 2016 01:54:46 -0500 Date: Mon, 14 Nov 2016 17:08:06 +1100 From: David Gibson Message-ID: <20161114060806.GB24747@umbus.fritz.box> References: <1477825928-10803-1-git-send-email-david@gibson.dropbear.id.au> <1477825928-10803-17-git-send-email-david@gibson.dropbear.id.au> <57db31f3-e8b6-cdb4-da1e-e08f06bebd5c@ozlabs.ru> <20161108053108.GW28688@umbus.fritz.box> <20161111191334.3ce8161c@bahia> <3ab10188-0444-20b4-b7e4-10177b10490f@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ADZbWkCsHQ7r3kzd" Content-Disposition: inline In-Reply-To: <3ab10188-0444-20b4-b7e4-10177b10490f@ozlabs.ru> Subject: Re: [Qemu-devel] [Qemu-ppc] [RFC 16/17] ppc: Remove counter-productive "sanity checks" in migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Greg Kurz , lvivier@redhat.com, thuth@redhat.com, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --ADZbWkCsHQ7r3kzd Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 14, 2016 at 01:34:55PM +1100, Alexey Kardashevskiy wrote: > On 12/11/16 05:13, Greg Kurz wrote: > > On Tue, 8 Nov 2016 16:31:08 +1100 > > David Gibson wrote: > >=20 > >> On Fri, Nov 04, 2016 at 04:52:39PM +1100, Alexey Kardashevskiy wrote: > >>> On 30/10/16 22:12, David Gibson wrote: =20 > >>>> When vmstate for the ppc cpu was introduced in a90db158 "target-ppc: > >>>> Convert ppc cpu savevm to VMStateDescription", several "sanity check" > >>>> fields were included, verifying that certain cpu parameters matched = between > >>>> source and destination. > >>>> > >>>> This turns out not to have been a good idea. For one thing it's red= undant > >>>> with existing checks for a compatible cpu version at either end. Bu= t more > >>>> importantly the insns_flags and insns_flags2 checks actively break t= hings: > >>>> they expose what's essentially an internal TCG implementation detail= in the > >>>> migration stream. That means that when new instruction classes are = added > >>>> or rearranged, migration can break. > >>>> > >>>> This removes these ill-considered sanity checks. > >>>> > >>>> Signed-off-by: David Gibson > >>>> --- > >>>> target-ppc/machine.c | 8 ++++---- > >>>> 1 file changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c > >>>> index 62b9e94..453ef0a 100644 > >>>> --- a/target-ppc/machine.c > >>>> +++ b/target-ppc/machine.c > >>>> @@ -602,10 +602,10 @@ const VMStateDescription vmstate_ppc_cpu =3D { > >>>> /* FIXME: access_type? */ > >>>> =20 > >>>> /* Sanity checking */ > >>>> - VMSTATE_UINTTL_EQUAL(env.msr_mask, PowerPCCPU), > >>>> - VMSTATE_UINT64_EQUAL(env.insns_flags, PowerPCCPU), > >>>> - VMSTATE_UINT64_EQUAL(env.insns_flags2, PowerPCCPU), > >>>> - VMSTATE_UINT32_EQUAL(env.nb_BATs, PowerPCCPU), > >>>> + VMSTATE_UNUSED(sizeof(target_ulong) /* msr_mask */ > >>>> + + sizeof(uint64_t) /* insns_flags */ > >>>> + + sizeof(uint64_t) /* insns_flags2 */ > >>>> + + sizeof(uint32_t)), /* nb_BATs */ =20 > >>> > >>> > >>> This breaks migration to older QEMU: > >>> > >>> 25055@1478238734.537761:vmstate_load_field_error field "env.msr_mask"= load > >>> failed, ret =3D -22 =20 > >> > >> Again, I don't think we generally support backwards migration. > >> > >> That said, it would be nice here to do a "set to this field on > >> ourgoing migration, but ignore on incoming migration". Do you know a > >> way to do that? > >> > >=20 > > This doesn't exist in vmstate but it is certainly doable. An alternative > > would be to always send the fields, but have the destination to use > > pre_load and post_load callbacks to preserve its state. >=20 >=20 > A simpler way would be: >=20 > 1. add a copy for each field (s/msr_mask/mig_msr_mask/), > 2. get rid of _EQUAL bits, > 3. implement .pre_save callback which would initialize mig_xxx >=20 > And that's it, .pre_load/.post_load is not needed. Yeah, it works, but crufts up the runtime structures with extra fields related only to migration with old versions. I think just dropping the backwards migration is a better option in this case. >=20 >=20 >=20 >=20 > >=20 > >> a > >>> > >>> > >>> =20 > >>>> VMSTATE_END_OF_LIST() > >>>> }, > >>>> .subsections =3D (const VMStateDescription*[]) { > >>>> =20 > >>> > >>> =20 > >> > >=20 >=20 >=20 --=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 --ADZbWkCsHQ7r3kzd Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYKVTDAAoJEGw4ysog2bOSLFkP/0J6TMBAKyS/U+v/WLi9tfoY oNB2MM/578laF88Q0G6Z8+x8oWWjbU+TZiPKnwcQoSGHHRA8/R3lOLDsEsT7jYJB sk3GbBBR/HNliwz8dalkw7FAOaG/JpIYYA8lceOAyfSHibNkH5fBQB0UwE/KpHrX ZsUJKodvVTSuwJJHB43+QQKtbjk23boSzeBJdHI/SCX3mXCB+IMpTtH9H+Q3qKPV wQ9G8+0pMYWCmPDQIoRTikHVnvnhEjPR/eP9EpPbBr+GvzFE10MkqXAQWNIn49c1 BxBMo7O5wvLIzXSycaLUlXc/QUroYmnt/RzD32dRtv7a9rLl5lVEqs32nc0HYDMo 0aq5Y28XxGtb5JV5grHGDZu4tHDhDJYxaBObvr1Oqq0UBTVKG+84i1YFow9KckEA bxZzJu/iMIoCX354sfdrH26WkwCILA/RmS4CxNFRSBK9YxuXJXSUhF/yjKyHIqyi IsJUIIQdloUVIV3D6dcDd73Pt/pIrc/SRlq/OFYnWTfihmcuImqeVFK/q6pfrNQd Igx/PW9o3nSXyUuwLP045i2HS2B26+fdi5CtRBSKnJa8S8/cXk/Zn3138wIFUf+l gflNIDrzYXwihR750ltoXsU4NLVF+PTyBVCjM+lok+kxxqibik+LQXxliRc35HZk wAMvoBdUKow4ney633Ic =9zw2 -----END PGP SIGNATURE----- --ADZbWkCsHQ7r3kzd--