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

[Aurelien Jarno]

On Sun, May 15, 2011 at 01:01:40AM +0300, Blue Swirl wrote:
> On Sun, May 15, 2011 at 12:32 AM, Aurelien Jarno <address@hidden> wrote:
> > 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,
> 
> Currently this is true, but actually qemu_ld/st only need a pointer to
> the TLB. If the address is a constant, some of the TLB calculations
> could be performed at translation time. This would need a very
> different approach to qemu_ld/st than current one.
> 

That's true for the fastpath, but the slowpath really need to have
access to the env register (for example to access mem_io_pc,
mem_io_vaddr, iotlb).

Also looking at the softmmu code, it seems to be possible to call
cpu_loop_exit() without env, using cpu_single_env instead.

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