From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44463) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIR85-0007Hz-4G for qemu-devel@nongnu.org; Sun, 10 Jan 2016 20:18:42 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aIR81-00048W-TH for qemu-devel@nongnu.org; Sun, 10 Jan 2016 20:18:41 -0500 Received: from mail-pf0-x242.google.com ([2607:f8b0:400e:c00::242]:36413) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aIR81-00048P-Gz for qemu-devel@nongnu.org; Sun, 10 Jan 2016 20:18:37 -0500 Received: by mail-pf0-x242.google.com with SMTP id n128so3010702pfn.3 for ; Sun, 10 Jan 2016 17:18:37 -0800 (PST) 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> From: Alexey Kardashevskiy Message-ID: <569302E7.4080404@ozlabs.ru> Date: Mon, 11 Jan 2016 12:18:31 +1100 MIME-Version: 1.0 In-Reply-To: <568FC5FF.9070606@ilande.co.uk> Content-Type: text/plain; charset=koi8-r; format=flowed Content-Transfer-Encoding: 7bit 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 , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, quintela@redhat.com, amit.shah@redhat.com, David Gibson 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 default >>> value >>> of the decrementer to 0xffffffff during initialisation which typically >>> corresponds to several seconds. Hence when restoring the image, the guest >>> would effectively "lose" decrementer interrupts during this time causing >>> 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 = POWERPC_CPU(cpu); >> pcpu->env.tb_env->tb_offset = tb_off_adj; >> } >> >> >> It does not transfer DECR though, it transfers the offset instead, one >> for all CPUs. >> >> >> The easier solution would be just adding this instead of the whole patch: >> >> 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 support, >> 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 break >> 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] = env->cfar; > #endif > env->spr[SPR_BOOKE_SPEFSCR] = env->spe_fscr; > + env->spr[SPR_DECR] = cpu_ppc_load_decr(env); > > for (i = 0; (i < 4) && (i < env->nb_BATs); i++) { > env->spr[SPR_DBAT0U + 2*i] = env->DBAT[0][i]; > @@ -175,6 +176,7 @@ static int cpu_post_load(void *opaque, int version_id) > env->cfar = env->spr[SPR_CFAR]; > #endif > env->spe_fscr = env->spr[SPR_BOOKE_SPEFSCR]; > + cpu_ppc_store_decr(env, env->spr[SPR_DECR]); > > for (i = 0; (i < 4) && (i < env->nb_BATs); i++) { > env->DBAT[0][i] = 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 patchset > 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 wants to comment. -- Alexey