Re: [Qemu-devel] [PATCH] MIPS: exceptions handling in icount mode

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:address@hidden
> 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.

> > 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.

> From my point of view, it looks like the problem is actually elsewhere
> in the common icount code. Do we know if it works correctly on other
> emulated architectures?

This problem can be solved only if someone passes PC to cpu_restore_state 
function.
It cannot be done without altering exceptions processing of the targets.

Other architectures have bugs in icount processing in case of the exceptions.
I will fix them and submit as separate patches.

> Also do you have a quick example to reproduce
> the issue?

I have no quick example, because I've got this situation in the middle 
of booting Linux process. It is hard to catch, because in most cases 
this bug does not affect the guest behavior.

> 
> 
> > Signed-off-by: Pavel Dovgalyuk <address@hidden>
> > ---
> >  target-mips/cpu.h        |   28 +++++++++++++++++++++++++
> >  target-mips/msa_helper.c |    5 +++-
> >  target-mips/op_helper.c  |   52 
> > +++++++++++-----------------------------------
> >  target-mips/translate.c  |    2 ++
> >  4 files changed, 45 insertions(+), 42 deletions(-)
> 
> [ snip ]
> 
> > diff --git a/target-mips/translate.c b/target-mips/translate.c
> > index fd063a2..9c2ff7c 100644
> > --- a/target-mips/translate.c
> > +++ b/target-mips/translate.c
> > @@ -1675,6 +1675,7 @@ generate_exception_err (DisasContext *ctx, int excp, 
> > int err)
> >      TCGv_i32 terr = tcg_const_i32(err);
> >      save_cpu_state(ctx, 1);
> >      gen_helper_raise_exception_err(cpu_env, texcp, terr);
> > +    ctx->bstate = BS_STOP;
> >      tcg_temp_free_i32(terr);
> >      tcg_temp_free_i32(texcp);
> >  }
> > @@ -1684,6 +1685,7 @@ generate_exception (DisasContext *ctx, int excp)
> >  {
> >      save_cpu_state(ctx, 1);
> >      gen_helper_0e0i(raise_exception, excp);
> > +    ctx->bstate = BS_STOP;
> >  }
> >
> 
> Why do we need to stop the translation here? The exception might be
> conditional (for example for ADDU or SUBU).

Right, it would be better to add another helper, which recovers the PC.

Pavel Dovgalyuk