Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 16 May 2011 18:51, Peter Maydell peter.mayd...@linaro.org wrote: On 16 May 2011 18:29, Aurelien Jarno aurel...@aurel32.net wrote: That said given this patch is more or less an extension of an existing code, we may want to apply it anyway. That is the conclusion I'm hoping to persuade you to, yes :-) This patch seems to have got stuck in limbo -- any chance of a definite ack/nak-and-do-it-like-this/commit from anybody? [This bug is one of the items on my ARM issues that must be fixed in some manner for 0.15 release.] thanks -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 14 May 2011 22:32, Aurelien Jarno aurel...@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. 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.) -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
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(). To call cpu_restore_state you need to know searched_pc. To find that you need to unwind the host stack all the way back to translated code. Paul
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 16 May 2011 17:10, Paul Brook p...@codesourcery.com 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(). To call cpu_restore_state you need to know searched_pc. To find that you need to unwind the host stack all the way back to translated code. You can do this by calling GETPC() from the top level helper function though, right? [OK, we'd need to move the definition out of dyngen-exec.h.] -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Mon, May 16, 2011 at 05:37:03PM +0100, Peter Maydell wrote: On 16 May 2011 17:10, Paul Brook p...@codesourcery.com 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(). To call cpu_restore_state you need to know searched_pc. To find that you need to unwind the host stack all the way back to translated code. You can do this by calling GETPC() from the top level helper function though, right? [OK, we'd need to move the definition out of dyngen-exec.h.] No we don't need to move it out of dyngen-exec.h. dyngen-exec.h is included in target-*/exec.h, as the softmmu helpers, which are included in target-*/op_helper.c, call cpu_restore_state(). For an actual usage of cpu_restore_state() outside of the softmmu helpers, you can have a look at target-sh4/op-helper.c, which uses this technique for raising most exceptions, and especially the FPU ones. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Mon, May 16, 2011 at 10:59:47AM +0100, Peter Maydell wrote: On 14 May 2011 22:32, Aurelien Jarno aurel...@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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 16 May 2011 18:29, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, May 16, 2011 at 05:37:03PM +0100, Peter Maydell wrote: You can do this by calling GETPC() from the top level helper function though, right? [OK, we'd need to move the definition out of dyngen-exec.h.] No we don't need to move it out of dyngen-exec.h. dyngen-exec.h is included in target-*/exec.h, as the softmmu helpers, which are included in target-*/op_helper.c, call cpu_restore_state(). I meant, assuming we want to reduce the set of helpers which use the implicit-global-env (ie: back out the patches which made helpers other than op_helper.c include exec.h). At the moment you can't get GETPC() without also getting the global-env which means you have to be in a source file compiled with the right CFLAGS. Sorry for the lack of clarity. For an actual usage of cpu_restore_state() outside of the softmmu helpers, you can have a look at target-sh4/op-helper.c, which uses this technique for raising most exceptions, and especially the FPU ones. Sure. It's in a file which has access to global-env, though. -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 16 May 2011 18:29, Aurelien Jarno aurel...@aurel32.net wrote: That said given this patch is more or less an extension of an existing code, we may want to apply it anyway. That is the conclusion I'm hoping to persuade you to, yes :-) -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Mon, May 16, 2011 at 06:47:42PM +0100, Peter Maydell wrote: On 16 May 2011 18:29, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, May 16, 2011 at 05:37:03PM +0100, Peter Maydell wrote: You can do this by calling GETPC() from the top level helper function though, right? [OK, we'd need to move the definition out of dyngen-exec.h.] No we don't need to move it out of dyngen-exec.h. dyngen-exec.h is included in target-*/exec.h, as the softmmu helpers, which are included in target-*/op_helper.c, call cpu_restore_state(). I meant, assuming we want to reduce the set of helpers which use the implicit-global-env (ie: back out the patches which made helpers other than op_helper.c include exec.h). At the moment you can't get GETPC() without also getting the global-env which means you have to be in a source file compiled with the right CFLAGS. Sorry for the lack of clarity. Well, I haven't tried, but it seems you can simply include dyngen-exec.h directly in the file you want. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote: On 26 April 2011 11:23, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote: On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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, 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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Sun, May 15, 2011 at 12:32 AM, Aurelien Jarno aurel...@aurel32.net wrote: On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote: On 26 April 2011 11:23, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote: On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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.
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Sun, May 15, 2011 at 01:01:40AM +0300, Blue Swirl wrote: On Sun, May 15, 2011 at 12:32 AM, Aurelien Jarno aurel...@aurel32.net wrote: On Fri, May 06, 2011 at 03:32:27PM +0100, Peter Maydell wrote: On 26 April 2011 11:23, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote: On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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. 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 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 26 April 2011 11:23, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote: On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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() * (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/ -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Mon, Apr 25, 2011 at 11:35:54PM +0100, Peter Maydell wrote: On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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. But I got the impression there was something more significant than that. I agree, but it's something that has to be done sooner or later anyway. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Thu, Apr 21, 2011 at 05:01:48PM +0100, Peter Maydell wrote:
Newer Linux kernels assume the existence of the performance counter
cp15 registers. Provide a minimal implementation of these registers.
We support no events. This should be compliant with the ARM ARM,
except that we don't implement the cycle counter.
Signed-off-by: Peter Maydell peter.mayd...@linaro.org
---
This is the last fix required to be able to boot a stock Linaro
versatile express image on upstream QEMU...
target-arm/cpu.h |8 ++-
target-arm/helper.c| 159
target-arm/machine.c | 12
target-arm/translate.c | 20 ++-
4 files changed, 183 insertions(+), 16 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index d5af644..b8e7419 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -129,6 +129,12 @@ typedef struct CPUARMState {
uint32_t c7_par; /* Translation result. */
uint32_t c9_insn; /* Cache lockdown registers. */
uint32_t c9_data;
+uint32_t c9_pmcr; /* performance monitor control register */
+uint32_t c9_pmcnten; /* perf monitor counter enables */
+uint32_t c9_pmovsr; /* perf monitor overflow status */
+uint32_t c9_pmxevtyper; /* perf monitor event type */
+uint32_t c9_pmuserenr; /* perf monitor user enable */
+uint32_t c9_pminten; /* perf monitor interrupt enables */
uint32_t c13_fcse; /* FCSE PID. */
uint32_t c13_context; /* Context ID. */
uint32_t c13_tls1; /* User RW Thread register. */
@@ -434,7 +440,7 @@ void cpu_arm_set_cp_io(CPUARMState *env, int cpnum,
#define cpu_signal_handler cpu_arm_signal_handler
#define cpu_list arm_cpu_list
-#define CPU_SAVE_VERSION 3
+#define CPU_SAVE_VERSION 4
/* MMU modes definitions */
#define MMU_MODE0_SUFFIX _kernel
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 62ae72e..b051b8c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -270,6 +270,10 @@ void cpu_reset(CPUARMState *env)
}
env-vfp.xregs[ARM_VFP_FPEXC] = 0;
env-cp15.c2_base_mask = 0xc000u;
+/* v7 performance monitor control register: same implementor
+ * field as main ID register, and we implement no event counters.
+ */
+env-cp15.c9_pmcr = (id 0xff00);
#endif
set_flush_to_zero(1, env-vfp.standard_fp_status);
set_flush_inputs_to_zero(1, env-vfp.standard_fp_status);
@@ -1587,6 +1591,81 @@ void HELPER(set_cp15)(CPUState *env, uint32_t insn,
uint32_t val)
case 1: /* TCM memory region registers. */
/* Not implemented. */
goto bad_reg;
+case 12: /* Performance monitor control */
+/* Performance monitors are implementation defined in v7,
+ * but with an ARM recommended set of registers, which we
+ * follow (although we don't actually implement any counters)
+ */
+if (!arm_feature(env, ARM_FEATURE_V7)) {
+goto bad_reg;
+}
+switch (op2) {
+case 0: /* performance monitor control register */
+/* only the DP, X, D and E bits are writable */
+env-cp15.c9_pmcr = ~0x39;
+env-cp15.c9_pmcr |= (val 0x39);
+break;
+case 1: /* Count enable set register */
+val = (1 31);
+env-cp15.c9_pmcnten |= val;
+break;
+case 2: /* Count enable clear */
+val = (1 31);
+env-cp15.c9_pmcnten = ~val;
+break;
+case 3: /* Overflow flag status */
+env-cp15.c9_pmovsr = ~val;
+break;
+case 4: /* Software increment */
+/* RAZ/WI since we don't implement the software-count event
*/
+break;
+case 5: /* Event counter selection register */
+/* Since we don't implement any events, writing to this
register
+ * is actually UNPREDICTABLE. So we choose to RAZ/WI.
+ */
+break;
+default:
+goto bad_reg;
+}
+break;
+case 13: /* Performance counters */
+if (!arm_feature(env, ARM_FEATURE_V7)) {
+goto bad_reg;
+}
+switch (op2) {
+case 0: /* Cycle count register: not implemented, so RAZ/WI */
+break;
+case 1: /* Event type select */
+env-cp15.c9_pmxevtyper = val 0xff;
+break;
+case 2: /* Event count register */
+/* Unimplemented (we have no events), RAZ/WI */
+break;
+default:
+goto bad_reg;
+}
+break;
+case 14:
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 25 April 2011 22:09, Aurelien Jarno aurel...@aurel32.net wrote: On Thu, Apr 21, 2011 at 05:01:48PM +0100, Peter Maydell wrote: + tb_flush(env); If you flush all tbs, you also have to ensure that on the translate.c side, this is the last instruction of the tb. Otherwise, the rest of the TB will be executed with the wrong access rights. This is OK, because we can't get here unless we're in privileged mode (PMUSERENR is never writable in user mode), and changing PMUSERENR doesn't affect the access rights for privileged mode. And a switch into user mode will be a change of TB anyway. (Compare the handling of the TEECR, which also doesn't need to change TB after a tb_flush(), for the same reasons.) 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 :-) On the subject of complexity: I have vague plans for overhauling the cp15 support code anyway, so you can effectively register handler functions for the cp15 registers you care about rather than having to have one enormous function full of nested case statements. You could then have the access checking code not so wildly far away from the register read/write implementation. (Plus we need support for banked cp15 registers at some point.) -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@aurel32.net wrote: On Thu, Apr 21, 2011 at 05:01:48PM +0100, Peter Maydell wrote: + tb_flush(env); If you flush all tbs, you also have to ensure that on the translate.c side, this is the last instruction of the tb. Otherwise, the rest of the TB will be executed with the wrong access rights. This is OK, because we can't get here unless we're in privileged mode (PMUSERENR is never writable in user mode), and changing PMUSERENR doesn't affect the access rights for privileged mode. And a switch into user mode will be a change of TB anyway. (Compare the handling of the TEECR, which also doesn't need to change TB after a tb_flush(), for the same reasons.) Ok, fine then. 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. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 25 April 2011 23:31, Aurelien Jarno aurel...@aurel32.net wrote: On Mon, Apr 25, 2011 at 10:59:52PM +0100, Peter Maydell wrote: On 25 April 2011 22:09, Aurelien Jarno aurel...@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. But I got the impression there was something more significant than that. -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Friday 22 April 2011 02:01:48 Peter Maydell wrote: Newer Linux kernels assume the existence of the performance counter cp15 registers. Provide a minimal implementation of these registers. We support no events. This should be compliant with the ARM ARM, except that we don't implement the cycle counter. I tried to apply this to git master, and got a reject (attached). It looks like that area of target-arm/helper.c hasn't been touched in a while. Is it possible you have other changes for this? Did I miss a pre-requisite? Brad helper.c.rej Description: application/reject
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On 22 April 2011 08:23, Brad Hards br...@frogmouth.net wrote: On Friday 22 April 2011 02:01:48 Peter Maydell wrote: Newer Linux kernels assume the existence of the performance counter cp15 registers. Provide a minimal implementation of these registers. We support no events. This should be compliant with the ARM ARM, except that we don't implement the cycle counter. I tried to apply this to git master, and got a reject (attached). It looks like that area of target-arm/helper.c hasn't been touched in a while. Is it possible you have other changes for this? Did I miss a pre-requisite? Works for me: $ git pull Already up-to-date. $ git log --oneline -1 ec5 target-arm: Set Invalid flag for NaN in float-to-int conversions $ wget -O perfcounters.patch http://patchwork.ozlabs.org/patch/92423/mbox/ $ git am perfcounters.patch Applying: target-arm: Minimal implementation of performance counters $ Looking at your .rej file it seems to have lost the hardcoded tab characters[*] that are in the patch; I suspect something in your mailer is turning them back into spaces. Try downloading the patch from patchwork instead. [*] the tab-indents are in the existing code, the patch is removing them for the lines it touches -- PMM
Re: [Qemu-devel] [PATCH] target-arm: Minimal implementation of performance counters
On Friday 22 April 2011 19:48:09 Peter Maydell wrote: Looking at your .rej file it seems to have lost the hardcoded tab characters[*] that are in the patch; I suspect something in your mailer is turning them back into spaces. Try downloading the patch from patchwork instead. Yep, that worked. Sorry for the noise. Brad
