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

[Aurelien Jarno <aurelien@aurel32.net>]

On 2015-06-15 10:48, 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?
> 
> Recovering from is a tricky mechanism. It can break the correct execution
> if used inaccurately even when icount is disabled.
> I already posted a patch for maskmov instruction in i386:
> http://lists.nongnu.org/archive/html/qemu-devel/2014-09/msg02960.html

One solution there is to save the CPU state before calling the maskmov
helper. That would fix the bug, but not the icount problem you reported.

If we decided that we must always be able to do retranslation, we should
go with the second possibility from Richard:

| (2) Add helpers that accept the GETRA value from the top-level helper.  And not
| hidden within a macro or always_inline function.  This helps us see what
| portions of the code have been audited for the new interface.  This will
| involve quite a bit more code churn, but shouldn't been too difficult for any
| single function.

And we should do it a way that it works for both user and softmmu modes,
to avoid too many #ifdef in the targets code, which in general is a
source of bugs.

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