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

[Pavel Dovgaluk]

> From: Aurelien Jarno [mailto:address@hidden
> On 2015-06-15 07:53, Pavel Dovgaluk wrote:
> > > 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.
> 
> 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?

Pavel Dovgalyuk