From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57988) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aHXw5-0001vT-R7 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 09:22:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aHXw4-0004Is-Q5 for qemu-devel@nongnu.org; Fri, 08 Jan 2016 09:22:37 -0500 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> From: Mark Cave-Ayland Message-ID: <568FC5FF.9070606@ilande.co.uk> Date: Fri, 8 Jan 2016 14:21:51 +0000 MIME-Version: 1.0 In-Reply-To: <568F235B.1010506@ozlabs.ru> Content-Type: text/plain; charset=koi8-r 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: Alexey Kardashevskiy , qemu-devel@nongnu.org, qemu-ppc@nongnu.org, agraf@suse.de, quintela@redhat.com, amit.shah@redhat.com, David Gibson 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. ATB, Mark.