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

[Blue Swirl <blauwirbel@gmail.com>]

On Sun, May 15, 2011 at 12:32 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote:
>> On 26 April 2011 11:23, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> > On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote:
>> >> On 25 April 2011 23:31, Aurelien Jarno <aurelien@aurel32.net> wrote:
>> >> > On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote:
>> >> >> On 25 April 2011 22:09, Aurelien Jarno <aurelien@aurel32.net> 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.