From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4PfZ-0004Gi-6N for qemu-devel@nongnu.org; Mon, 15 Jun 2015 04:23:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z4PfX-0006jk-Pn for qemu-devel@nongnu.org; Mon, 15 Jun 2015 04:23:01 -0400 Received: from hall.aurel32.net ([2001:bc8:30d7:101::1]:45386) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z4PfX-0006je-GK for qemu-devel@nongnu.org; Mon, 15 Jun 2015 04:22:59 -0400 Date: Mon, 15 Jun 2015 10:22:57 +0200 From: Aurelien Jarno Message-ID: <20150615082257.GA931@aurel32.net> References: <20150610083306.5492.31869.stgit@PASHA-ISP> <20150611223727.GB13281@aurel32.net> <000601d0a727$41271d70$c3755850$@Dovgaluk@ispras.ru> <20150615072640.GA23868@aurel32.net> <000701d0a73e$745bf960$5d13ec20$@Dovgaluk@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000701d0a73e$745bf960$5d13ec20$@Dovgaluk@ispras.ru> Subject: Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgaluk Cc: pbonzini@redhat.com, leon.alrae@imgtec.com, qemu-devel@nongnu.org On 2015-06-15 10:39, Pavel Dovgaluk wrote: > > From: Aurelien Jarno [mailto:aurelien@aurel32.net] > > On 2015-06-15 07:53, Pavel Dovgaluk wrote: > > > > From: Aurelien Jarno [mailto:aurelien@aurel32.net] > > > > On 2015-06-10 11:33, Pavel Dovgalyuk wrote: > > > > > This patch fixes exception handling in MIPS. > > > > > MIPS instructions generate several types of exceptions. > > > > > When exception is generated, it breaks the execution of the current translation > > > > > block. Implementation of the exceptions handling in MIPS does not correctly > > > > > restore icount for the instruction which caused the exception. In most cases > > > > > icount will be decreased by the value equal to the size of TB. > > > > > > > > I don't think it is correct. There is no real point of always doing > > > > retranslation for an exception triggered from the helpers, especially > > > > when the CPU state has been saved before anyway? > > > > > > As you know, icount is processed as follows: > > > > > > TB: > > > if icount < n then exit > > > icount -= n > > > instr1 > > > instr2 > > > ... > > > instrn > > > exit > > > > > > When one of the instructions initiates an exception, then icount should be restored > > > and adjusted number of instructions should be subtracted instead of initial n. > > > > > > E.g., tlb_fill function passes retaddr to raise_exception, which allows restoring > > > current instructions in TB and correct icount calculation. > > > > > > When exception triggered with other function (e.g. by embedding call to > > > helper_raise_exception_err into TB), then PC is not passed as retaddr and > > > correct icount is not recovered. > > > > > > This behavior leads to incorrect values of timers and non-deterministic execution > > > of the code. > > > > Ok, this therefore doesn't looks something MIPS specific, but rather a > > flaw in the icount design. Instead of fixing blindly one target, we > > should try to fix it globally, or if not possible at least agree on a > > way to fix that for all target and provide the infrastructure for that > > (for example provide load/store functions which accept a return > > address). Paolo any opinion on that? > > > > Also retranslation has a cost (actually on MIPS we spend more time in > > *retranslation* than in translation due to the way the MMU works), we > > should avoid it if we already have to save the CPU state for another > > reason. At least your patch should remove the code saving the CPU state > > when possible if the helper uses retranslation instead. > > Ok, I'll remove CPU saving where it is not actually used because of the exception. > > > > > > This patch passes pointer to the translation block internals to the exception > > > > > handler. It allows correct restoring of the icount value. > > > > > > > > Your patch doesn't do that for all the helpers, for example all the > > > > memory access helpers. It probably improves the situation but therefore > > > > doesn't fix it. > > > > > > Right, I missed these helpers. I'll add them in the next version. > > > > Except we currently don't provide a way in the softmmu code to provide a > > return address... > > Softmmu passes return address into tlb_fill function, which passes it to raise_exception. > I also fixed HELPER_LD_ATOMIC and HELPER_ST_ATOMIC to recover PC value. > Do you mean something different? Yes, for example in your patch: > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c > index 73a8e45..1b798a2 100644 > --- a/target-mips/op_helper.c > +++ b/target-mips/op_helper.c > @@ -309,7 +281,7 @@ target_ulong helper_##name(CPUMIPSState *env, target_ulong arg, int mem_idx) \ > { \ > if (arg & almask) { \ > env->CP0_BadVAddr = arg; \ > - helper_raise_exception(env, EXCP_AdEL); \ > + do_raise_exception(env, EXCP_AdEL, GETPC()); \ > } \ > env->lladdr = do_translate_address(env, arg, 0); \ > env->llval = do_##insn(env, arg, mem_idx); \ do_##insn is actually a load/store instruction, but you doesn't pass the return address as an argument (and you can't with the current code). -- Aurelien Jarno GPG: 4096R/1DDD8C9B aurelien@aurel32.net http://www.aurel32.net