qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Aurelien Jarno <aurelien@aurel32.net>
To: Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
Cc: pbonzini@redhat.com, leon.alrae@imgtec.com, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode
Date: Mon, 15 Jun 2015 10:22:57 +0200	[thread overview]
Message-ID: <20150615082257.GA931@aurel32.net> (raw)
In-Reply-To: <000701d0a73e$745bf960$5d13ec20$@Dovgaluk@ispras.ru>

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

  reply	other threads:[~2015-06-15  8:23 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-06-10  8:33 [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode Pavel Dovgalyuk
2015-06-11 22:37 ` Aurelien Jarno
2015-06-15  4:53   ` Pavel Dovgaluk
2015-06-15  7:26     ` Aurelien Jarno
2015-06-15  7:39       ` Pavel Dovgaluk
2015-06-15  8:22         ` Aurelien Jarno [this message]
2015-06-15  7:48       ` Pavel Dovgaluk
2015-06-15  8:28         ` Aurelien Jarno

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20150615082257.GA931@aurel32.net \
    --to=aurelien@aurel32.net \
    --cc=Pavel.Dovgaluk@ispras.ru \
    --cc=leon.alrae@imgtec.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).