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

[Aurelien Jarno <aurelien@aurel32.net>]

On Mon, May 16, 2011 at 10:59:47AM +0100, Peter Maydell wrote:
> On 14 May 2011 22:32, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote:
> >> 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().
> 
> Yes, I think you're right here, I'm not sure why I didn't think that
> would work. (b) still applies, though.
> 
> > 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.
> 
> I think really my position on this patch is that it adds
> functionality that means you can't actually boot recent Linux
> kernels with hw breakpoint support enabled, and I'd rather not
> have it get tangled up with either the ongoing "remove AREG0"
> discussions or a more general overhaul of how cp15 registers
> are handled. It just handles this handful of new registers in
> the same way we currently handle all the other cp14/cp15 regs.

Well the current discussion is about to know if env access should be
implicit (through a fixed register), or explicit (passed as an argument
to the helper). Blue Swirl is working towards the second solution to see
if it could work or not, so currently I think both options are still
acceptable (both options are currently in use in the current code).

> > 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.
> 
> Do you mean you'd like to see us using cpu_restore_state() instead
> of explicit state-syncing TCG code for the cases where the exception
> is "expected" like SVC instructions? (ie what most targets have
> a gen_exception() function for.)
> 

Well maybe this patch is not the best example to use
cpu_restore_state(), but I think we should go toward this direction.
Exceptions as their name suggests are not the rules, so we should avoid
generating code to handle them (like state-syncing TCG code), and use
CPU state restoration, even if it is not fast (that's not a problem, as
exceptions are not the rules).

That said given this patch is more or less an extension of an existing
code, we may want to apply it anyway.

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