Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters

[Aurelien Jarno]

On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote:
> On 26 April 2011 11:23, Aurelien Jarno <address@hidden> wrote:
> > On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote:
> >> On 25 April 2011 23:31, Aurelien Jarno <address@hidden> wrote:
> >> > On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote:
> >> >> On 25 April 2011 22:09, Aurelien Jarno <address@hidden> wrote:
> >> >> > Instead of having this complex test for all cp15 access, but only for
> >> >> > catching a few access to performance registers, wouldn't it make more
> >> >> > sense to have this test and an exception triggering directly in
> >> >> > helper.c?
> >> >>
> >> >> That was what my first design did, but in discussions on IRC
> >> >> with Paul Brook he basically said that you can't generate an
> >> >> exception in the helper routine, you have to either generate
> >> >> runtime code to do the test or throw away the TBs. Unfortunately
> >> >> I forget the exact rationale, so I've cc'd Paul to remind me :-)
> >> >
> >> > This is something strange, plenty of targets are raising exceptions from
> >> > helpers without any problem.
> >>
> >> You'd at minimum need to move the cp15 helper functions to a different
> >> file, they're currently in helper.c which doesn't get compiled
> >> with access to the global 'env' register.
> 
> > I agree, but it's something that has to be done sooner or later anyway.
> 
> I just spoke with Paul on IRC about this. In summary:
>  * for a helper to cause an exception then it has (a) to make sure CPU
> state (pc, condflags) is sync'd before the call to the helper and (b)
> the helper has to be in a file with access to global env, because it
> needs to call cpu_loop_exit()

I don't think (a) is true. It is possible to use the same way as for
load/store operations, that is call cpu_restore_state() before calling
cpu_loop_exit().

>  * (a) is a bit fragile because it's easy to forget and if TCG gets
> cleverer things might break in a hard-to-track down way
>  * (b) is bad because it increases the set of helper functions accessing
> global env
>  * and therefore helpers which throw exceptions are a bad idea
> 
> For rationale for/arguing about (b) see the comment on this patch:
>  http://patchwork.ozlabs.org/patch/94384/
> 

If you don't really like having env as a fixed host registers (recent
patch series from Blue Swirl seems to want to get rid of this), it is 
possible to pass env as an argument of the helper to do that.

What ever solution is used, we need env for the load/store functions,
and theses functions already provide a framework to restore the CPU
state. I think it's a good idea to use this framework instead of having
a second framework using TCG code for that.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net