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

[Aurelien Jarno <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.

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

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net