From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45332) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIowW-0000iv-6h for qemu-devel@nongnu.org; Mon, 11 Jan 2016 21:44:21 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aIowU-0002GF-Ev for qemu-devel@nongnu.org; Mon, 11 Jan 2016 21:44:20 -0500 Date: Tue, 12 Jan 2016 13:44:21 +1100 From: David Gibson Message-ID: <20160112024421.GH22925@voom.redhat.com> References: <1452104533-8516-1-git-send-email-mark.cave-ayland@ilande.co.uk> <1452104533-8516-5-git-send-email-mark.cave-ayland@ilande.co.uk> <568F235B.1010506@ozlabs.ru> <568FC5FF.9070606@ilande.co.uk> <569302E7.4080404@ozlabs.ru> <20160111045519.GE22925@voom.redhat.com> <56935D3A.1030709@ilande.co.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="a7XSrSxqzVsaECgU" Content-Disposition: inline In-Reply-To: <56935D3A.1030709@ilande.co.uk> Subject: Re: [Qemu-devel] [PATCH 4/4] target-ppc: ensure we include the decrementer value during migration List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Cave-Ayland Cc: quintela@redhat.com, Alexey Kardashevskiy , agraf@suse.de, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, amit.shah@redhat.com --a7XSrSxqzVsaECgU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jan 11, 2016 at 07:43:54AM +0000, Mark Cave-Ayland wrote: > On 11/01/16 04:55, David Gibson wrote: >=20 > > On Mon, Jan 11, 2016 at 12:18:31PM +1100, Alexey Kardashevskiy wrote: > >> On 01/09/2016 01:21 AM, Mark Cave-Ayland wrote: > >>> On 08/01/16 02:47, Alexey Kardashevskiy wrote: > >>> > >>>> On 01/07/2016 05:22 AM, Mark Cave-Ayland wrote: > >>>>> During local testing with TCG, intermittent errors were found when > >>>>> trying to > >>>>> migrate Darwin OS images. > >>>>> > >>>>> The underlying cause was that Darwin resets the decrementer value to > >>>>> fairly > >>>>> small values on each interrupt. cpu_ppc_set_tb_clk() sets the defau= lt > >>>>> value > >>>>> of the decrementer to 0xffffffff during initialisation which typica= lly > >>>>> corresponds to several seconds. Hence when restoring the image, the= guest > >>>>> would effectively "lose" decrementer interrupts during this time ca= using > >>>>> confusion in the guest. > >>>>> > >>>>> NOTE: there does seem to be some overlap here with the > >>>>> vmstate_ppc_timebase > >>>>> code, however it doesn't seem to handle multiple CPUs which is why > >>>>> I've gone > >>>>> for an independent implementation. > >>>> > >>>> It does handle multiple CPUs: > >>>> > >>>> static int timebase_post_load(void *opaque, int version_id) > >>>> { > >>>> ... > >>>> /* Set new offset to all CPUs */ > >>>> CPU_FOREACH(cpu) { > >>>> PowerPCCPU *pcpu =3D POWERPC_CPU(cpu); > >>>> pcpu->env.tb_env->tb_offset =3D tb_off_adj; > >>>> } > >>>> > >>>> > >>>> It does not transfer DECR though, it transfers the offset instead, o= ne > >>>> for all CPUs. > >>>> > >>>> > >>>> The easier solution would be just adding this instead of the whole p= atch: > >>>> > >>>> spr_register(env, SPR_DECR, "DECR", > >>>> SPR_NOACCESS, SPR_NOACCESS, > >>>> &spr_read_decr, &spr_write_decr, > >>>> 0x00000000); > >>>> > >>>> somewhere in target-ppc/translate_init.c for CPUs you want to suppor= t, > >>>> gen_tbl() seems to be the right place for this. As long as it is just > >>>> spr_register() and not spr_register_kvm(), I suppose it should not b= reak > >>>> KVM and pseries. > >>> > >>> I've just tried adding that but it then gives the following error on > >>> startup: > >>> > >>> Error: Trying to register SPR 22 (016) twice ! > >>> > >>> Based upon this, the existing registration seems fine. I think the > >>> problem is that I can't see anything in __cpu_ppc_store_decr() that > >>> updates the spr[SPR_DECR] value when the decrementer register is > >>> changed, so it needs to be explicitly added to > >>> cpu_pre_save()/cpu_post_load(): > >>> > >>> > >>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c > >>> index 251a84b..495e58d 100644 > >>> --- a/target-ppc/machine.c > >>> +++ b/target-ppc/machine.c > >>> @@ -141,6 +141,7 @@ static void cpu_pre_save(void *opaque) > >>> env->spr[SPR_CFAR] =3D env->cfar; > >>> #endif > >>> env->spr[SPR_BOOKE_SPEFSCR] =3D env->spe_fscr; > >>> + env->spr[SPR_DECR] =3D cpu_ppc_load_decr(env); > >>> > >>> for (i =3D 0; (i < 4) && (i < env->nb_BATs); i++) { > >>> env->spr[SPR_DBAT0U + 2*i] =3D env->DBAT[0][i]; > >>> @@ -175,6 +176,7 @@ static int cpu_post_load(void *opaque, int versio= n_id) > >>> env->cfar =3D env->spr[SPR_CFAR]; > >>> #endif > >>> env->spe_fscr =3D env->spr[SPR_BOOKE_SPEFSCR]; > >>> + cpu_ppc_store_decr(env, env->spr[SPR_DECR]); > >>> > >>> for (i =3D 0; (i < 4) && (i < env->nb_BATs); i++) { > >>> env->DBAT[0][i] =3D env->spr[SPR_DBAT0U + 2*i]; > >>> > >>> > >>> I've just tried the diff above instead of my original version and > >>> repeated my savevm/loadvm pair test with a Darwin installation and it > >>> also fixes the random hang I was seeing on loadvm. > >>> > >>> Seemingly this should work on KVM in that cpu_ppc_load_decr() and > >>> cpu_ppc_store_decr() become no-ops as long as KVM maintains > >>> env->spr[SPR_DECR], but a second set of eyeballs would be useful here. > >>> > >>> If you can let me know if this is suitable then I'll update the patch= set > >>> based upon your feedback and send out a v2. > >> > >> > >> Looks good to me, I'd just wait a day or two in the case if David want= s to > >> comment. > >=20 > > I was on holiday and missed the start of this thread, I'm afraid, so I > > don't fully understand the context here. >=20 > It's part of a patchset I posted which fixes up problems I had with > migrating g3beige/mac99 machines under TCG: > https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg00544.html. >=20 > Apologies for not adding you as CC directly - are you still helping to > cover ppc-next for Alex? Yes, I am. > > Am I right in thinking that this change will essentially freeze the > > decrementer across the migration downtime? That doesn't seem right, > > since the decrementer is supposed to be linked to the timebase and > > represent real time passing. >=20 > Yes, that's correct. >=20 > > In other words, isn't this just skipping the decrementer interrupts at > > the qemu level rather than the guest level? > >=20 > > It seems that instead we should be reconstructing the decrementer on > > the destination based on an offset from the timebase. >=20 > Well I haven't really looked at how time warping works during in > migration for QEMU, however this seems to be the method used by > hw/ppc/ppc.c's timebase_post_load() function but my understanding is > that this isn't currently available for the g3beige/mac99 machines? Ah.. yes, it looks like the timebase migration stuff is only hooked in on the pseries machine type. As far as I can tell it should be trivial to add it to other machines though - it doesn't appear to rely on anything outside the common ppc timebase stuff. > Should the patch in fact do this but also add decrementer support? And > if it did, would this have a negative effect on pseries? Yes, I think that's the right approach. Note that rather than duplicating the logic to adjust the decrementer over migration, it should be possible to encode the decrementer as a diff from the timebase across the migration. In fact.. I'm not sure it ever makes sense to store the decrementer value as a direct value, since it's constantly changing - probably makes more sense to derive it from the timebase whenever it is needed. As far as I know that should be fine for pseries. I think the current behaviour is probably technically wrong for pseries as well, but the timing code of our Linux guests is robust enough to handle a small displacement to the time of the next decrementer interrupt. > But yes, assuming that the guest time warp is handled correctly (which I > assume is handled correctly elsewhere since this would also be required > for KVM) then I think that this should work. >=20 >=20 > ATB, >=20 > Mark. >=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 --a7XSrSxqzVsaECgU Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJWlGiFAAoJEGw4ysog2bOSzJsQAIAf7KFeLu6TLybJis8cQN5q KfCyYOjfjtjDEgPu/jOIt9qJ//8nVuVrDEtKaSnNUBZS32xGe2nnPfVpXY1w/vng CdnlRSuMJECGoM6ITAAg34sNNunNEXQXhKBEhIl52SLxIKX76zNj5NTAg0zYjGjS KTXhl2rhp9ynSHMJmLP5FU2h6UkzqBYQIMn+glMR2GwCKooUX2miFDJRnbEHAHr0 rPWHaxHT9VoPRPlyeab5LDi6lP6JTm5E6t6A5n77cxGKFqU9sJK0WezazrTp7mTL U/lhUp4Gcue3Vk3LgBlNN3LRmx0L7Ynj7yklWp9XkaPwAF6NiPnEcxvpcoT9Kb2K kwGRUBSotnsPiqdoEp0s7Dz3L7gRV/57sklcLN52vu4BRP6nyNFo2kzarTSO16ZI K2Qw3TEWSgYNUE+xNPFEFGlC6LVmWNZ9E23XmBxFoU3H7YjpI298WuzI8e4Ckwwv Fh7VfMxG5ml4OG8L4n3ZW2F7Ju+Yk9NIT1Nd0yxITuA0DFsAAN2uobx5Egt2ZBDV BTmj9WW52aUBtU2Hv7mHErmWinPVw4AmWAFe4niYEFXfCZnnnGxtdjS94ZHMtXXV Gm+mj0wQ0P3Mp+zYEZhobtEEimWS/mU22KQUyU7HRZKkVsVHUpPRA4y9q/e1p6fH 9IwKsDEqV2lHQsju8Uqo =LA/F -----END PGP SIGNATURE----- --a7XSrSxqzVsaECgU--