* [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
@ 2014-01-21 23:40 Alistair Francis
2014-01-23 13:56 ` Peter Crosthwaite
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2014-01-21 23:40 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.maydell, peter.crosthwaite
This patch implements the ARM PMCCNTR register including
the disable and reset components of the PMCR register.
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
This patch assumes that non-invasive debugging is not permitted
when determining if the counter is disabled
V3: Fixed up incorrect reset, disable and enable handling that
was submitted in V2. The patch should now also correctly handle
on the fly changing of the clock scaling factor.
V2: Incorporated the comments that Peter Maydell and Peter
Crosthwaite had. Now the implementation only requires one
CPU state
target-arm/cpu.h | 3 ++
target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 71 insertions(+), 2 deletions(-)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 198b6b8..2fdab58 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -215,6 +215,9 @@ typedef struct CPUARMState {
uint32_t c15_diagnostic; /* diagnostic register */
uint32_t c15_power_diagnostic;
uint32_t c15_power_control; /* power control */
+ /* If the counter is enabled, this stores the last time the counter
+ * was reset. Otherwise it stores the counter value */
+ uint32_t c15_ccnt;
} cp15;
/* System registers (AArch64) */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index c708f15..ad87136 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
target_ulong *page_size);
#endif
+/* Definitions for the PMCCNTR and PMCR registers */
+#define PMCRDP 0x20
+#define PMCRD 0x8
+#define PMCRC 0x4
+#define PMCRE 0x1
+
static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
{
int nregs;
@@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t value)
{
+ uint32_t temp_ticks, diff;
+
if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
return EXCP_UDEF;
}
+
+ temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
+ get_ticks_per_sec())/1000000000) >> 8) &
+ 0xFFFFFFFF);
+
+ /* This assumes that non-invasive debugging is not permitted */
+ if (!(env->cp15.c9_pmcr & PMCRDP) ||
+ env->cp15.c9_pmcr & PMCRE) {
+ /* If the counter is enabled */
+ env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+ }
+
+ if (value & PMCRC) {
+ /* The counter has been reset */
+ env->cp15.c15_ccnt = 0;
+ }
+
/* only the DP, X, D and E bits are writable */
env->cp15.c9_pmcr &= ~0x39;
env->cp15.c9_pmcr |= (value & 0x39);
+
+ /* This assumes that non-invasive debugging is not permitted */
+ if (!(env->cp15.c9_pmcr & PMCRDP) ||
+ env->cp15.c9_pmcr & PMCRE) {
+ if (env->cp15.c9_pmcr & PMCRDP) {
+ /* Increment once every 64 processor clock cycles */
+ diff = (temp_ticks/64) - env->cp15.c15_ccnt;
+ } else {
+ diff = temp_ticks - env->cp15.c15_ccnt;
+ }
+ env->cp15.c15_ccnt += diff;
+ }
+
return 0;
}
@@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
return 0;
}
+static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+ uint64_t *value)
+{
+ uint32_t total_ticks;
+
+ /* This assumes that non-invasive debugging is not permitted */
+ if (env->cp15.c9_pmcr & PMCRDP ||
+ !(env->cp15.c9_pmcr & PMCRE)) {
+ /* Counter is disabled, do not change value */
+ *value = env->cp15.c15_ccnt;
+ return 0;
+ }
+
+ total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
+ get_ticks_per_sec())/1000000000) >> 8) &
+ 0xFFFFFFFF);
+
+ if (env->cp15.c9_pmcr & PMCRDP) {
+ /* Increment once every 64 processor clock cycles */
+ *value = (total_ticks/64) - env->cp15.c15_ccnt;
+ } else {
+ *value = total_ticks - env->cp15.c15_ccnt;
+ }
+
+ return 0;
+}
+
static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
uint64_t *value)
{
@@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
*/
{ .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
.access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
- /* Unimplemented, RAZ/WI. XXX PMUSERENR */
{ .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
- .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+ .access = PL1_RW, .readfn = pmccntr_read,
+ .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
+ .resetvalue = 0, .type = ARM_CP_IO },
{ .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
.access = PL0_RW,
.fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
--
1.7.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
2014-01-21 23:40 [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register Alistair Francis
@ 2014-01-23 13:56 ` Peter Crosthwaite
2014-01-28 1:31 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Peter Crosthwaite @ 2014-01-23 13:56 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Maydell, qemu-devel@nongnu.org Developers
Same subject-prefix as commented by Andreas. Should be "target-arm".
On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch implements the ARM PMCCNTR register including
> the disable and reset components of the PMCR register.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> This patch assumes that non-invasive debugging is not permitted
> when determining if the counter is disabled
> V3: Fixed up incorrect reset, disable and enable handling that
> was submitted in V2. The patch should now also correctly handle
> on the fly changing of the clock scaling factor.
> V2: Incorporated the comments that Peter Maydell and Peter
> Crosthwaite had. Now the implementation only requires one
> CPU state
>
> target-arm/cpu.h | 3 ++
> target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 71 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 198b6b8..2fdab58 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -215,6 +215,9 @@ typedef struct CPUARMState {
> uint32_t c15_diagnostic; /* diagnostic register */
> uint32_t c15_power_diagnostic;
> uint32_t c15_power_control; /* power control */
> + /* If the counter is enabled, this stores the last time the counter
> + * was reset. Otherwise it stores the counter value */
> + uint32_t c15_ccnt;
> } cp15;
>
> /* System registers (AArch64) */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c708f15..ad87136 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
> target_ulong *page_size);
> #endif
>
> +/* Definitions for the PMCCNTR and PMCR registers */
> +#define PMCRDP 0x20
> +#define PMCRD 0x8
> +#define PMCRC 0x4
> +#define PMCRE 0x1
> +
> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> {
> int nregs;
> @@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
> static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t value)
> {
> + uint32_t temp_ticks, diff;
> +
> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
> return EXCP_UDEF;
> }
> +
> + temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
> + get_ticks_per_sec())/1000000000) >> 8) &
> + 0xFFFFFFFF);
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
> + env->cp15.c9_pmcr & PMCRE) {
> + /* If the counter is enabled */
> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
Should this be frequency scalar aware? I think its calculating and
saving the current timer value but assuming the X64 prescale is
disabled. But since the intent is to save the current timer value to
c15_ccnt, is it as simple as just calling pmccntr_read() here to get
the desired to-be-saved value?
> + }
> +
> + if (value & PMCRC) {
> + /* The counter has been reset */
> + env->cp15.c15_ccnt = 0;
> + }
> +
> /* only the DP, X, D and E bits are writable */
> env->cp15.c9_pmcr &= ~0x39;
> env->cp15.c9_pmcr |= (value & 0x39);
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
> + env->cp15.c9_pmcr & PMCRE) {
> + if (env->cp15.c9_pmcr & PMCRDP) {
> + /* Increment once every 64 processor clock cycles */
> + diff = (temp_ticks/64) - env->cp15.c15_ccnt;
I think you can simplify with just
diff = (temp_ticks/64);
and ...
> + } else {
> + diff = temp_ticks - env->cp15.c15_ccnt;
> + }
> + env->cp15.c15_ccnt += diff;
... env->cp15.c15_ccnt = diff;
Although it's probably even simpler to just drop this and assign
env->cp15.c15_ccnt in the two if-else branches directly.
> + }
> +
> return 0;
> }
>
> @@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
> return 0;
> }
>
> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> + uint64_t *value)
> +{
> + uint32_t total_ticks;
> +
> + /* This assumes that non-invasive debugging is not permitted */
> + if (env->cp15.c9_pmcr & PMCRDP ||
> + !(env->cp15.c9_pmcr & PMCRE)) {
> + /* Counter is disabled, do not change value */
> + *value = env->cp15.c15_ccnt;
> + return 0;
> + }
> +
> + total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
> + get_ticks_per_sec())/1000000000) >> 8) &
Theres an un-needed parenthesis of multiplication before division.
Whats the shift by 8 for?
> + 0xFFFFFFFF);
The mask-to-32bit is un-needed I think.
> +
> + if (env->cp15.c9_pmcr & PMCRDP) {
> + /* Increment once every 64 processor clock cycles */
> + *value = (total_ticks/64) - env->cp15.c15_ccnt;
You can save on a common sub-express by just
total_ticks /= 64;
Then ...
> + } else {
> + *value = total_ticks - env->cp15.c15_ccnt;
.. you can do this unconditionally.
> + }
> +
> + return 0;
> +}
> +
> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> uint64_t *value)
> {
> @@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> */
> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */
> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> + .access = PL1_RW, .readfn = pmccntr_read,
> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
You don't implement write, so I'm not sure exactly what you would do
to implement readfn with UNIMP writefn. But this as-is will allow the
guest to overwrite the "diff" value with an absolute, which would
cause very strange behaviour.
Regards,
Peter
> + .resetvalue = 0, .type = ARM_CP_IO },
> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
> .access = PL0_RW,
> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
2014-01-23 13:56 ` Peter Crosthwaite
@ 2014-01-28 1:31 ` Alistair Francis
2014-01-28 2:01 ` Peter Crosthwaite
0 siblings, 1 reply; 5+ messages in thread
From: Alistair Francis @ 2014-01-28 1:31 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Alistair Francis
Thanks, I'll rename the patch in the next version
On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> Same subject-prefix as commented by Andreas. Should be "target-arm".
>
> On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch implements the ARM PMCCNTR register including
>> the disable and reset components of the PMCR register.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>> This patch assumes that non-invasive debugging is not permitted
>> when determining if the counter is disabled
>> V3: Fixed up incorrect reset, disable and enable handling that
>> was submitted in V2. The patch should now also correctly handle
>> on the fly changing of the clock scaling factor.
>> V2: Incorporated the comments that Peter Maydell and Peter
>> Crosthwaite had. Now the implementation only requires one
>> CPU state
>>
>> target-arm/cpu.h | 3 ++
>> target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 71 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 198b6b8..2fdab58 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -215,6 +215,9 @@ typedef struct CPUARMState {
>> uint32_t c15_diagnostic; /* diagnostic register */
>> uint32_t c15_power_diagnostic;
>> uint32_t c15_power_control; /* power control */
>> + /* If the counter is enabled, this stores the last time the counter
>> + * was reset. Otherwise it stores the counter value */
>> + uint32_t c15_ccnt;
>> } cp15;
>>
>> /* System registers (AArch64) */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index c708f15..ad87136 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
>> target_ulong *page_size);
>> #endif
>>
>> +/* Definitions for the PMCCNTR and PMCR registers */
>> +#define PMCRDP 0x20
>> +#define PMCRD 0x8
>> +#define PMCRC 0x4
>> +#define PMCRE 0x1
>> +
>> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
>> {
>> int nregs;
>> @@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> uint64_t value)
>> {
>> + uint32_t temp_ticks, diff;
>> +
>> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>> return EXCP_UDEF;
>> }
>> +
>> + temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>> + get_ticks_per_sec())/1000000000) >> 8) &
>> + 0xFFFFFFFF);
>> +
>> + /* This assumes that non-invasive debugging is not permitted */
>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>> + env->cp15.c9_pmcr & PMCRE) {
>> + /* If the counter is enabled */
>> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>
> Should this be frequency scalar aware? I think its calculating and
> saving the current timer value but assuming the X64 prescale is
> disabled. But since the intent is to save the current timer value to
> c15_ccnt, is it as simple as just calling pmccntr_read() here to get
> the desired to-be-saved value?
>
Yes, it should be frequency scalar aware. I have fixed that up
>> + }
>> +
>> + if (value & PMCRC) {
>> + /* The counter has been reset */
>> + env->cp15.c15_ccnt = 0;
>> + }
>> +
>> /* only the DP, X, D and E bits are writable */
>> env->cp15.c9_pmcr &= ~0x39;
>> env->cp15.c9_pmcr |= (value & 0x39);
>> +
>> + /* This assumes that non-invasive debugging is not permitted */
>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>> + env->cp15.c9_pmcr & PMCRE) {
>> + if (env->cp15.c9_pmcr & PMCRDP) {
>> + /* Increment once every 64 processor clock cycles */
>> + diff = (temp_ticks/64) - env->cp15.c15_ccnt;
>
> I think you can simplify with just
>
> diff = (temp_ticks/64);
>
> and ...
>
>> + } else {
>> + diff = temp_ticks - env->cp15.c15_ccnt;
>> + }
>> + env->cp15.c15_ccnt += diff;
>
> ... env->cp15.c15_ccnt = diff;
>
> Although it's probably even simpler to just drop this and assign
> env->cp15.c15_ccnt in the two if-else branches directly.
>
Done!
>> + }
>> +
>> return 0;
>> }
>>
>> @@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> return 0;
>> }
>>
>> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> + uint64_t *value)
>> +{
>> + uint32_t total_ticks;
>> +
>> + /* This assumes that non-invasive debugging is not permitted */
>> + if (env->cp15.c9_pmcr & PMCRDP ||
>> + !(env->cp15.c9_pmcr & PMCRE)) {
>> + /* Counter is disabled, do not change value */
>> + *value = env->cp15.c15_ccnt;
>> + return 0;
>> + }
>> +
>> + total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>> + get_ticks_per_sec())/1000000000) >> 8) &
>
> Theres an un-needed parenthesis of multiplication before division.
>
> Whats the shift by 8 for?
The shift by 8 is because qemu_clock_get_ns() returns a 64-bit number.
I have always though that the values look too big when using the
bottom half. The register jumps from: 4289154 to 3339447850 after a 1
second sleep. That's why I have been using the top half, do you think
I should use the bottom half of the number?
>
>> + 0xFFFFFFFF);
>
> The mask-to-32bit is un-needed I think.
>
>> +
>> + if (env->cp15.c9_pmcr & PMCRDP) {
>> + /* Increment once every 64 processor clock cycles */
>> + *value = (total_ticks/64) - env->cp15.c15_ccnt;
>
> You can save on a common sub-express by just
>
> total_ticks /= 64;
>
> Then ...
>
>> + } else {
>> + *value = total_ticks - env->cp15.c15_ccnt;
>
> .. you can do this unconditionally.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> uint64_t *value)
>> {
>> @@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>> */
>> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */
>> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> + .access = PL1_RW, .readfn = pmccntr_read,
>> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
>
> You don't implement write, so I'm not sure exactly what you would do
> to implement readfn with UNIMP writefn. But this as-is will allow the
> guest to overwrite the "diff" value with an absolute, which would
> cause very strange behaviour.
I have changed the permissions to make it a write only register. I
have the fieldoffset attribute as that is required if there is no
write function. Would you rather I remove fieldoffset and have an
empty write function?
>
> Regards,
> Peter
>
>> + .resetvalue = 0, .type = ARM_CP_IO },
>> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>> .access = PL0_RW,
>> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>> --
>> 1.7.1
>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
2014-01-28 1:31 ` Alistair Francis
@ 2014-01-28 2:01 ` Peter Crosthwaite
2014-01-28 4:08 ` Alistair Francis
0 siblings, 1 reply; 5+ messages in thread
From: Peter Crosthwaite @ 2014-01-28 2:01 UTC (permalink / raw)
To: Alistair Francis; +Cc: Peter Maydell, qemu-devel@nongnu.org Developers
On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Thanks, I'll rename the patch in the next version
>
> On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite
> <peter.crosthwaite@xilinx.com> wrote:
>> Same subject-prefix as commented by Andreas. Should be "target-arm".
>>
>> On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> This patch implements the ARM PMCCNTR register including
>>> the disable and reset components of the PMCR register.
>>>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> ---
>>> This patch assumes that non-invasive debugging is not permitted
>>> when determining if the counter is disabled
>>> V3: Fixed up incorrect reset, disable and enable handling that
>>> was submitted in V2. The patch should now also correctly handle
>>> on the fly changing of the clock scaling factor.
>>> V2: Incorporated the comments that Peter Maydell and Peter
>>> Crosthwaite had. Now the implementation only requires one
>>> CPU state
>>>
>>> target-arm/cpu.h | 3 ++
>>> target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>> 2 files changed, 71 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 198b6b8..2fdab58 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -215,6 +215,9 @@ typedef struct CPUARMState {
>>> uint32_t c15_diagnostic; /* diagnostic register */
>>> uint32_t c15_power_diagnostic;
>>> uint32_t c15_power_control; /* power control */
>>> + /* If the counter is enabled, this stores the last time the counter
>>> + * was reset. Otherwise it stores the counter value */
>>> + uint32_t c15_ccnt;
>>> } cp15;
>>>
>>> /* System registers (AArch64) */
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index c708f15..ad87136 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
>>> target_ulong *page_size);
>>> #endif
>>>
>>> +/* Definitions for the PMCCNTR and PMCR registers */
>>> +#define PMCRDP 0x20
>>> +#define PMCRD 0x8
>>> +#define PMCRC 0x4
>>> +#define PMCRE 0x1
>>> +
>>> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
>>> {
>>> int nregs;
>>> @@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>> static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> uint64_t value)
>>> {
>>> + uint32_t temp_ticks, diff;
>>> +
>>> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>>> return EXCP_UDEF;
>>> }
>>> +
>>> + temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>> + get_ticks_per_sec())/1000000000) >> 8) &
>>> + 0xFFFFFFFF);
>>> +
>>> + /* This assumes that non-invasive debugging is not permitted */
>>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>>> + env->cp15.c9_pmcr & PMCRE) {
>>> + /* If the counter is enabled */
>>> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>>
>> Should this be frequency scalar aware? I think its calculating and
>> saving the current timer value but assuming the X64 prescale is
>> disabled. But since the intent is to save the current timer value to
>> c15_ccnt, is it as simple as just calling pmccntr_read() here to get
>> the desired to-be-saved value?
>>
>
> Yes, it should be frequency scalar aware. I have fixed that up
>
>>> + }
>>> +
>>> + if (value & PMCRC) {
>>> + /* The counter has been reset */
>>> + env->cp15.c15_ccnt = 0;
>>> + }
>>> +
>>> /* only the DP, X, D and E bits are writable */
>>> env->cp15.c9_pmcr &= ~0x39;
>>> env->cp15.c9_pmcr |= (value & 0x39);
>>> +
>>> + /* This assumes that non-invasive debugging is not permitted */
>>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>>> + env->cp15.c9_pmcr & PMCRE) {
>>> + if (env->cp15.c9_pmcr & PMCRDP) {
>>> + /* Increment once every 64 processor clock cycles */
>>> + diff = (temp_ticks/64) - env->cp15.c15_ccnt;
>>
>> I think you can simplify with just
>>
>> diff = (temp_ticks/64);
>>
>> and ...
>>
>>> + } else {
>>> + diff = temp_ticks - env->cp15.c15_ccnt;
>>> + }
>>> + env->cp15.c15_ccnt += diff;
>>
>> ... env->cp15.c15_ccnt = diff;
>>
>> Although it's probably even simpler to just drop this and assign
>> env->cp15.c15_ccnt in the two if-else branches directly.
>>
>
> Done!
>
>>> + }
>>> +
>>> return 0;
>>> }
>>>
>>> @@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> return 0;
>>> }
>>>
>>> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>> + uint64_t *value)
>>> +{
>>> + uint32_t total_ticks;
>>> +
>>> + /* This assumes that non-invasive debugging is not permitted */
>>> + if (env->cp15.c9_pmcr & PMCRDP ||
>>> + !(env->cp15.c9_pmcr & PMCRE)) {
>>> + /* Counter is disabled, do not change value */
>>> + *value = env->cp15.c15_ccnt;
>>> + return 0;
>>> + }
>>> +
>>> + total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>> + get_ticks_per_sec())/1000000000) >> 8) &
>>
>> Theres an un-needed parenthesis of multiplication before division.
>>
>> Whats the shift by 8 for?
>
> The shift by 8 is because qemu_clock_get_ns() returns a 64-bit number.
>
> I have always though that the values look too big when using the
> bottom half. The register jumps from: 4289154 to 3339447850 after a 1
> second sleep.
Virtual or real time? Perhaps a worthwhile experiment is to see if the
qemu_get_clock_ns values are correlating to real time (separately from
this timer). It could be that qemu_clock_get_ns is running too fast.
That's why I have been using the top half, do you think
> I should use the bottom half of the number?
>
>>
>>> + 0xFFFFFFFF);
>>
>> The mask-to-32bit is un-needed I think.
>>
>>> +
>>> + if (env->cp15.c9_pmcr & PMCRDP) {
>>> + /* Increment once every 64 processor clock cycles */
>>> + *value = (total_ticks/64) - env->cp15.c15_ccnt;
>>
>> You can save on a common sub-express by just
>>
>> total_ticks /= 64;
>>
>> Then ...
>>
>>> + } else {
>>> + *value = total_ticks - env->cp15.c15_ccnt;
>>
>> .. you can do this unconditionally.
>>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>> uint64_t *value)
>>> {
>>> @@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>> */
>>> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>>> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */
>>> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>>> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>> + .access = PL1_RW, .readfn = pmccntr_read,
>>> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
>>
>> You don't implement write, so I'm not sure exactly what you would do
>> to implement readfn with UNIMP writefn. But this as-is will allow the
>> guest to overwrite the "diff" value with an absolute, which would
>> cause very strange behaviour.
>
> I have changed the permissions to make it a write only register. I
> have the fieldoffset attribute as that is required if there is no
> write function. Would you rather I remove fieldoffset and have an
> empty write function?
>
That will cause the CP Register interface to throw an exception on
write, whereas original implementation will just NOP it so the guest
happily moves on. Although I would suggest that what you have done is
more accurate, as a silent NOP due to non-implementation is much
harder to debug than a EXCP_UDEF. But someone will have added that
dummy for a reason so I think you need to preserve the NOP write.
Regards,
Peter
>>
>> Regards,
>> Peter
>>
>>> + .resetvalue = 0, .type = ARM_CP_IO },
>>> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>>> .access = PL0_RW,
>>> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>>> --
>>> 1.7.1
>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register
2014-01-28 2:01 ` Peter Crosthwaite
@ 2014-01-28 4:08 ` Alistair Francis
0 siblings, 0 replies; 5+ messages in thread
From: Alistair Francis @ 2014-01-28 4:08 UTC (permalink / raw)
To: Peter Crosthwaite
Cc: Peter Maydell, qemu-devel@nongnu.org Developers, Alistair Francis
On Tue, Jan 28, 2014 at 12:01 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Tue, Jan 28, 2014 at 11:31 AM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Thanks, I'll rename the patch in the next version
>>
>> On Thu, Jan 23, 2014 at 11:56 PM, Peter Crosthwaite
>> <peter.crosthwaite@xilinx.com> wrote:
>>> Same subject-prefix as commented by Andreas. Should be "target-arm".
>>>
>>> On Wed, Jan 22, 2014 at 9:40 AM, Alistair Francis
>>> <alistair.francis@xilinx.com> wrote:
>>>> This patch implements the ARM PMCCNTR register including
>>>> the disable and reset components of the PMCR register.
>>>>
>>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>>> ---
>>>> This patch assumes that non-invasive debugging is not permitted
>>>> when determining if the counter is disabled
>>>> V3: Fixed up incorrect reset, disable and enable handling that
>>>> was submitted in V2. The patch should now also correctly handle
>>>> on the fly changing of the clock scaling factor.
>>>> V2: Incorporated the comments that Peter Maydell and Peter
>>>> Crosthwaite had. Now the implementation only requires one
>>>> CPU state
>>>>
>>>> target-arm/cpu.h | 3 ++
>>>> target-arm/helper.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> 2 files changed, 71 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>>> index 198b6b8..2fdab58 100644
>>>> --- a/target-arm/cpu.h
>>>> +++ b/target-arm/cpu.h
>>>> @@ -215,6 +215,9 @@ typedef struct CPUARMState {
>>>> uint32_t c15_diagnostic; /* diagnostic register */
>>>> uint32_t c15_power_diagnostic;
>>>> uint32_t c15_power_control; /* power control */
>>>> + /* If the counter is enabled, this stores the last time the counter
>>>> + * was reset. Otherwise it stores the counter value */
>>>> + uint32_t c15_ccnt;
>>>> } cp15;
>>>>
>>>> /* System registers (AArch64) */
>>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>>> index c708f15..ad87136 100644
>>>> --- a/target-arm/helper.c
>>>> +++ b/target-arm/helper.c
>>>> @@ -13,6 +13,12 @@ static inline int get_phys_addr(CPUARMState *env, uint32_t address,
>>>> target_ulong *page_size);
>>>> #endif
>>>>
>>>> +/* Definitions for the PMCCNTR and PMCR registers */
>>>> +#define PMCRDP 0x20
>>>> +#define PMCRD 0x8
>>>> +#define PMCRC 0x4
>>>> +#define PMCRE 0x1
>>>> +
>>>> static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
>>>> {
>>>> int nregs;
>>>> @@ -502,12 +508,44 @@ static int pmreg_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>>> static int pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>> uint64_t value)
>>>> {
>>>> + uint32_t temp_ticks, diff;
>>>> +
>>>> if (arm_current_pl(env) == 0 && !env->cp15.c9_pmuserenr) {
>>>> return EXCP_UDEF;
>>>> }
>>>> +
>>>> + temp_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>>> + get_ticks_per_sec())/1000000000) >> 8) &
>>>> + 0xFFFFFFFF);
>>>> +
>>>> + /* This assumes that non-invasive debugging is not permitted */
>>>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>>>> + env->cp15.c9_pmcr & PMCRE) {
>>>> + /* If the counter is enabled */
>>>> + env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>>>
>>> Should this be frequency scalar aware? I think its calculating and
>>> saving the current timer value but assuming the X64 prescale is
>>> disabled. But since the intent is to save the current timer value to
>>> c15_ccnt, is it as simple as just calling pmccntr_read() here to get
>>> the desired to-be-saved value?
>>>
>>
>> Yes, it should be frequency scalar aware. I have fixed that up
>>
>>>> + }
>>>> +
>>>> + if (value & PMCRC) {
>>>> + /* The counter has been reset */
>>>> + env->cp15.c15_ccnt = 0;
>>>> + }
>>>> +
>>>> /* only the DP, X, D and E bits are writable */
>>>> env->cp15.c9_pmcr &= ~0x39;
>>>> env->cp15.c9_pmcr |= (value & 0x39);
>>>> +
>>>> + /* This assumes that non-invasive debugging is not permitted */
>>>> + if (!(env->cp15.c9_pmcr & PMCRDP) ||
>>>> + env->cp15.c9_pmcr & PMCRE) {
>>>> + if (env->cp15.c9_pmcr & PMCRDP) {
>>>> + /* Increment once every 64 processor clock cycles */
>>>> + diff = (temp_ticks/64) - env->cp15.c15_ccnt;
>>>
>>> I think you can simplify with just
>>>
>>> diff = (temp_ticks/64);
>>>
>>> and ...
>>>
>>>> + } else {
>>>> + diff = temp_ticks - env->cp15.c15_ccnt;
>>>> + }
>>>> + env->cp15.c15_ccnt += diff;
>>>
>>> ... env->cp15.c15_ccnt = diff;
>>>
>>> Although it's probably even simpler to just drop this and assign
>>> env->cp15.c15_ccnt in the two if-else branches directly.
>>>
>>
>> Done!
>>
>>>> + }
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -584,6 +622,33 @@ static int vbar_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>>> return 0;
>>>> }
>>>>
>>>> +static int pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>>> + uint64_t *value)
>>>> +{
>>>> + uint32_t total_ticks;
>>>> +
>>>> + /* This assumes that non-invasive debugging is not permitted */
>>>> + if (env->cp15.c9_pmcr & PMCRDP ||
>>>> + !(env->cp15.c9_pmcr & PMCRE)) {
>>>> + /* Counter is disabled, do not change value */
>>>> + *value = env->cp15.c15_ccnt;
>>>> + return 0;
>>>> + }
>>>> +
>>>> + total_ticks = (uint32_t) ((((qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) *
>>>> + get_ticks_per_sec())/1000000000) >> 8) &
>>>
>>> Theres an un-needed parenthesis of multiplication before division.
>>>
>>> Whats the shift by 8 for?
>>
>> The shift by 8 is because qemu_clock_get_ns() returns a 64-bit number.
>>
>> I have always though that the values look too big when using the
>> bottom half. The register jumps from: 4289154 to 3339447850 after a 1
>> second sleep.
>
> Virtual or real time? Perhaps a worthwhile experiment is to see if the
> qemu_get_clock_ns values are correlating to real time (separately from
> this timer). It could be that qemu_clock_get_ns is running too fast.
I'm using the virtual timer, so it should be an accurate time of the
emulated guest.
I just did a test then and the raw nano second value does seem to be a
bit fast. It incremented three seconds after my test 1s wait (which
probably isn't very accurate, but still shouldn't be that bad).
qemu_clock_get_ns() is the most accurate timer that I could find for
the guests running time. So you think I should remove the bit shift
and just use the top half of qemu_clock_get_ns()?
>
> That's why I have been using the top half, do you think
>> I should use the bottom half of the number?
>>
>>>
>>>> + 0xFFFFFFFF);
>>>
>>> The mask-to-32bit is un-needed I think.
>>>
>>>> +
>>>> + if (env->cp15.c9_pmcr & PMCRDP) {
>>>> + /* Increment once every 64 processor clock cycles */
>>>> + *value = (total_ticks/64) - env->cp15.c15_ccnt;
>>>
>>> You can save on a common sub-express by just
>>>
>>> total_ticks /= 64;
>>>
>>> Then ...
>>>
>>>> + } else {
>>>> + *value = total_ticks - env->cp15.c15_ccnt;
>>>
>>> .. you can do this unconditionally.
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int ccsidr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>>>> uint64_t *value)
>>>> {
>>>> @@ -644,9 +709,10 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>>> */
>>>> { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>>>> .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>>> - /* Unimplemented, RAZ/WI. XXX PMUSERENR */
>>>> { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>>>> - .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>>> + .access = PL1_RW, .readfn = pmccntr_read,
>>>> + .fieldoffset = offsetof(CPUARMState, cp15.c15_ccnt),
>>>
>>> You don't implement write, so I'm not sure exactly what you would do
>>> to implement readfn with UNIMP writefn. But this as-is will allow the
>>> guest to overwrite the "diff" value with an absolute, which would
>>> cause very strange behaviour.
>>
>> I have changed the permissions to make it a write only register. I
>> have the fieldoffset attribute as that is required if there is no
>> write function. Would you rather I remove fieldoffset and have an
>> empty write function?
>>
>
> That will cause the CP Register interface to throw an exception on
> write, whereas original implementation will just NOP it so the guest
> happily moves on. Although I would suggest that what you have done is
> more accurate, as a silent NOP due to non-implementation is much
> harder to debug than a EXCP_UDEF. But someone will have added that
> dummy for a reason so I think you need to preserve the NOP write.
Ok, I will change it to a NOP write (Just having it as a read only
register causes problems with my test case as well). Do you think I
should put a debug print in the NOP write, or just leave it empty?
>
> Regards,
> Peter
>
>>>
>>> Regards,
>>> Peter
>>>
>>>> + .resetvalue = 0, .type = ARM_CP_IO },
>>>> { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>>>> .access = PL0_RW,
>>>> .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-01-28 4:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-21 23:40 [Qemu-devel] [PATCH arm-ccnt v3 1/1] ARM-CCNT: Implements the ARM PMCCNTR register Alistair Francis
2014-01-23 13:56 ` Peter Crosthwaite
2014-01-28 1:31 ` Alistair Francis
2014-01-28 2:01 ` Peter Crosthwaite
2014-01-28 4:08 ` Alistair Francis
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).