[Qemu-devel] [PATCH v2 0/7] target-arm: Extend PMCCNTR for ARMv8

[Qemu-devel] [PATCH v2 0/7] target-arm: Extend PMCCNTR for ARMv8

[Alistair Francis]

This patch series continues on from my original PMCCNTR patch
work to extend the counter to be 64-bit and support for the
PMCCFILTR_EL0 register which allows the counter to be
disabled based on the current EL

V2:
 -Fix some typos identified by Christopher Covington
 -Convert the CCNT_ENABLED macro to the arm_ccnt_enabled
  function


Alistair Francis (7):
  target-arm: Make the ARM PMCCNTR register 64-bit
  target-arm: Implement PMCCNTR_EL0 and related registers
  target-arm: Add arm_ccnt_enabled function
  target-arm: Implement pmccntr_sync function
  target-arm: Remove old code and replace with new functions
  target-arm: Implement pmccfiltr_write function
  target-arm: Call the pmccntr_sync function when swapping ELs

 target-arm/cpu.h        |   14 ++++-
 target-arm/helper-a64.c |    5 ++
 target-arm/helper.c     |  150 ++++++++++++++++++++++++++++++++++++++---------
 target-arm/op_helper.c  |    6 ++
 4 files changed, 146 insertions(+), 29 deletions(-)

[Qemu-devel] [PATCH v2 1/7] target-arm: Make the ARM PMCCNTR register 64-bit

[Alistair Francis]

This makes the PMCCNTR register 64-bit to allow for the
64-bit ARMv8 version

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/cpu.h    |    2 +-
 target-arm/helper.c |   19 +++++++++----------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 369d472..cd1c7b6 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -223,7 +223,7 @@ typedef struct CPUARMState {
         /* If the counter is enabled, this stores the last time the counter
          * was reset. Otherwise it stores the counter value
          */
-        uint32_t c15_ccnt;
+        uint64_t c15_ccnt;
     } cp15;
 
     struct {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ed4d2bb..ac10564 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -550,11 +550,10 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    /* Don't computer the number of ticks in user mode */
-    uint32_t temp_ticks;
+    uint64_t temp_ticks;
 
-    temp_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                          get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRE) {
         /* If the counter is enabled */
@@ -586,15 +585,15 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
-    uint32_t total_ticks;
+    uint64_t total_ticks;
 
     if (!(env->cp15.c9_pmcr & PMCRE)) {
         /* Counter is disabled, do not change value */
         return env->cp15.c15_ccnt;
     }
 
-    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                  get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
@@ -606,7 +605,7 @@ static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                         uint64_t value)
 {
-    uint32_t total_ticks;
+    uint64_t total_ticks;
 
     if (!(env->cp15.c9_pmcr & PMCRE)) {
         /* Counter is disabled, set the absolute value */
@@ -614,8 +613,8 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
         return;
     }
 
-    total_ticks = qemu_clock_get_us(QEMU_CLOCK_VIRTUAL) *
-                  get_ticks_per_sec() / 1000000;
+    total_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                  get_ticks_per_sec(), 1000000);
 
     if (env->cp15.c9_pmcr & PMCRD) {
         /* Increment once every 64 processor clock cycles */
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[Alistair Francis]

This patch adds support for the ARMv8 version of the PMCCNTR and
related registers. It also starts to implement the PMCCFILTR_EL0
register.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/cpu.h    |    1 +
 target-arm/helper.c |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cd1c7b6..6a2efd8 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -224,6 +224,7 @@ typedef struct CPUARMState {
          * was reset. Otherwise it stores the counter value
          */
         uint64_t c15_ccnt;
+        uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
     } cp15;
 
     struct {
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ac10564..ce986ee 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .writefn = pmcntenset_write,
       .accessfn = pmreg_access,
       .raw_writefn = raw_write },
+    { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+      .opc2 = 1, .access = PL0_RW, .resetvalue = 0,
+      .state = ARM_CP_STATE_AA64,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
+      .writefn = pmcntenset_write,
+      .accessfn = pmreg_access,
+      .raw_writefn = raw_write },
     { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2,
+      .access = PL0_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
+      .accessfn = pmreg_access,
+      .writefn = pmcntenclr_write,
+      .type = ARM_CP_NO_MIGRATE },
+    { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+      .opc2 = 2,
+      .state = ARM_CP_STATE_AA64,
       .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
       .accessfn = pmreg_access,
       .writefn = pmcntenclr_write,
@@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
       .readfn = pmccntr_read, .writefn = pmccntr_write,
       .accessfn = pmreg_access },
+    { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3,
+      .opc2 = 0,
+      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
+      .state = ARM_CP_STATE_AA64,
+      .readfn = pmccntr_read, .writefn = pmccntr_write,
+      .accessfn = pmreg_access },
 #endif
+    { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .state = ARM_CP_STATE_AA64,
+      .resetvalue = 0,
+      .type = ARM_CP_IO,
+      .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), },
     { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
       .access = PL0_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
       .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .resetvalue = 0,
       .raw_writefn = raw_write },
     /* Unimplemented, RAZ/WI. */
     { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
@@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             .raw_writefn = raw_write,
         };
         define_one_arm_cp_reg(cpu, &pmcr);
+        ARMCPRegInfo pmcr64 = {
+            .name = "PMCR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
+            .opc2 = 0,
+            .state = ARM_CP_STATE_AA64,
+            .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000,
+            .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
+            .accessfn = pmreg_access, .writefn = pmcr_write,
+            .raw_writefn = raw_write,
+        };
+        define_one_arm_cp_reg(cpu, &pmcr64);
 #endif
         ARMCPRegInfo clidr = {
             .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[Peter Maydell]

On 26 June 2014 06:02, Alistair Francis <alistair.francis@xilinx.com> wrote:
> This patch adds support for the ARMv8 version of the PMCCNTR and
> related registers. It also starts to implement the PMCCFILTR_EL0
> register.
>
> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/cpu.h    |    1 +
>  target-arm/helper.c |   39 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cd1c7b6..6a2efd8 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -224,6 +224,7 @@ typedef struct CPUARMState {
>           * was reset. Otherwise it stores the counter value
>           */
>          uint64_t c15_ccnt;
> +        uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
>      } cp15;
>
>      struct {
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ac10564..ce986ee 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .writefn = pmcntenset_write,
>        .accessfn = pmreg_access,
>        .raw_writefn = raw_write },
> +    { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
> +      .opc2 = 1, .access = PL0_RW, .resetvalue = 0,
> +      .state = ARM_CP_STATE_AA64,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),

fieldoffset for an AArch64 register has to point at a uint64_t;
this is going to read the state next to cp15.c9_pmcnten as
well, resulting in garbage in half the register. You need to widen
that field to uint64_t and change the AArch32 accessor to
use offsetoflow32().

> +      .writefn = pmcntenset_write,
> +      .accessfn = pmreg_access,
> +      .raw_writefn = raw_write },

This is a pretty random order for the fields in a reginfo struct
(though existing code is not great here either).
Preferred is to put the .name first, then .state, then the
encoding in the same order as the v8 ARM ARM:
    .cp [if needed], .opc0, .opc1, .crn, .crm, .opc2
then .access and .accessfn
then .fieldoffset and .resetvalue, then read and writefns.

>      { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2,
> +      .access = PL0_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> +      .accessfn = pmreg_access,
> +      .writefn = pmcntenclr_write,
> +      .type = ARM_CP_NO_MIGRATE },
> +    { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
> +      .opc2 = 2,
> +      .state = ARM_CP_STATE_AA64,
>        .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>        .accessfn = pmreg_access,
>        .writefn = pmcntenclr_write,
> @@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
>        .readfn = pmccntr_read, .writefn = pmccntr_write,
>        .accessfn = pmreg_access },
> +    { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3,
> +      .opc2 = 0,
> +      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> +      .state = ARM_CP_STATE_AA64,
> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
> +      .accessfn = pmreg_access },

The AArch32 reginfo now needs a NO_MIGRATE marker, or we'll
migrate the underlying state twice.

You don't need to specify a .cp for a STATE_AA64 only reginfo.

The ARM ARM says the semantics of a 32 bit write to PMCCNTR
are to write the lower 32 bits and not touch the high bits. I suspect
your code will zero them instead. (Maybe this should have been a
comment on patch 1...)

>  #endif
> +    { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
> +      .access = PL0_RW, .accessfn = pmreg_access,
> +      .state = ARM_CP_STATE_AA64,
> +      .resetvalue = 0,
> +      .type = ARM_CP_IO,
> +      .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), },
>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>        .access = PL0_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>        .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> +      .resetvalue = 0,

This seems like a stray unnecessary change.

>        .raw_writefn = raw_write },
>      /* Unimplemented, RAZ/WI. */
>      { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
> @@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              .raw_writefn = raw_write,
>          };
>          define_one_arm_cp_reg(cpu, &pmcr);
> +        ARMCPRegInfo pmcr64 = {
> +            .name = "PMCR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
> +            .opc2 = 0,
> +            .state = ARM_CP_STATE_AA64,
> +            .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000,
> +            .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
> +            .accessfn = pmreg_access, .writefn = pmcr_write,
> +            .raw_writefn = raw_write,
> +        };

Don't put variable definitions in the middle of code blocks, please.

Same remarks as above about need for NO_MIGRATE and
need for widening a uint32_t field apply here.

> +        define_one_arm_cp_reg(cpu, &pmcr64);
>  #endif
>          ARMCPRegInfo clidr = {
>              .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
> --
> 1.7.1
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[Peter Crosthwaite]

On Sat, Aug 2, 2014 at 1:28 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 June 2014 06:02, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> This patch adds support for the ARMv8 version of the PMCCNTR and
>> related registers. It also starts to implement the PMCCFILTR_EL0
>> register.
>>
>> Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/cpu.h    |    1 +
>>  target-arm/helper.c |   39 +++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cd1c7b6..6a2efd8 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -224,6 +224,7 @@ typedef struct CPUARMState {
>>           * was reset. Otherwise it stores the counter value
>>           */
>>          uint64_t c15_ccnt;
>> +        uint64_t pmccfiltr_el0; /* Performance Monitor Filter Register */
>>      } cp15;
>>
>>      struct {
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index ac10564..ce986ee 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -738,7 +738,22 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>        .writefn = pmcntenset_write,
>>        .accessfn = pmreg_access,
>>        .raw_writefn = raw_write },
>> +    { .name = "PMCNTENSET_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
>> +      .opc2 = 1, .access = PL0_RW, .resetvalue = 0,
>> +      .state = ARM_CP_STATE_AA64,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>
> fieldoffset for an AArch64 register has to point at a uint64_t;
> this is going to read the state next to cp15.c9_pmcnten as
> well, resulting in garbage in half the register. You need to widen
> that field to uint64_t and change the AArch32 accessor to
> use offsetoflow32().
>

Fixed.

>> +      .writefn = pmcntenset_write,
>> +      .accessfn = pmreg_access,
>> +      .raw_writefn = raw_write },
>
> This is a pretty random order for the fields in a reginfo struct
> (though existing code is not great here either).
> Preferred is to put the .name first, then .state, then the
> encoding in the same order as the v8 ARM ARM:
>     .cp [if needed], .opc0, .opc1, .crn, .crm, .opc2
> then .access and .accessfn
> then .fieldoffset and .resetvalue, then read and writefns.
>

Done. I like the new scheme:

 792     { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
 793       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12,.opc2 = 1,
 794       .access = PL0_RW, .accessfn = pmreg_access,
 795       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
.resetvalue = 0,
 796       .writefn = pmcntenset_write, .raw_writefn = raw_write },

For completeness where does .type fit in?

>>      { .name = "PMCNTENCLR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 2,
>> +      .access = PL0_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>> +      .accessfn = pmreg_access,
>> +      .writefn = pmcntenclr_write,
>> +      .type = ARM_CP_NO_MIGRATE },
>> +    { .name = "PMCNTENCLR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
>> +      .opc2 = 2,
>> +      .state = ARM_CP_STATE_AA64,
>>        .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
>>        .accessfn = pmreg_access,
>>        .writefn = pmcntenclr_write,
>> @@ -762,11 +777,25 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>        .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
>>        .readfn = pmccntr_read, .writefn = pmccntr_write,
>>        .accessfn = pmreg_access },
>> +    { .name = "PMCCNTR_EL0", .cp = 15, .crn = 9, .crm = 13, .opc1 = 3,
>> +      .opc2 = 0,
>> +      .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
>> +      .state = ARM_CP_STATE_AA64,
>> +      .readfn = pmccntr_read, .writefn = pmccntr_write,
>> +      .accessfn = pmreg_access },
>
> The AArch32 reginfo now needs a NO_MIGRATE marker, or we'll
> migrate the underlying state twice.
>

Yep. And ditto for CNTENSET I guess.

> You don't need to specify a .cp for a STATE_AA64 only reginfo.
>
> The ARM ARM says the semantics of a 32 bit write to PMCCNTR
> are to write the lower 32 bits and not touch the high bits. I suspect
> your code will zero them instead. (Maybe this should have been a
> comment on patch 1...)
>

Hmm slightly tricky. Added the RMW logic to do this.

>>  #endif
>> +    { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
>> +      .access = PL0_RW, .accessfn = pmreg_access,
>> +      .state = ARM_CP_STATE_AA64,
>> +      .resetvalue = 0,
>> +      .type = ARM_CP_IO,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0), },
>>      { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>>        .access = PL0_RW,
>>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>>        .accessfn = pmreg_access, .writefn = pmxevtyper_write,
>> +      .resetvalue = 0,
>
> This seems like a stray unnecessary change.
>

Dropped.

>>        .raw_writefn = raw_write },
>>      /* Unimplemented, RAZ/WI. */
>>      { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
>> @@ -2324,6 +2353,16 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>              .raw_writefn = raw_write,
>>          };
>>          define_one_arm_cp_reg(cpu, &pmcr);
>> +        ARMCPRegInfo pmcr64 = {
>> +            .name = "PMCR_EL0", .crn = 9, .crm = 12, .opc0 = 3, .opc1 = 3,
>> +            .opc2 = 0,
>> +            .state = ARM_CP_STATE_AA64,
>> +            .access = PL0_RW, .resetvalue = cpu->midr & 0xff000000,
>> +            .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
>> +            .accessfn = pmreg_access, .writefn = pmcr_write,
>> +            .raw_writefn = raw_write,
>> +        };
>
> Don't put variable definitions in the middle of code blocks, please.
>

Fixed. Although we have that problem from CLIDR in the context below.

> Same remarks as above about need for NO_MIGRATE and
> need for widening a uint32_t field apply here.
>

Fixed.

Thanks,

Regards,
Peter

>> +        define_one_arm_cp_reg(cpu, &pmcr64);
>>  #endif
>>          ARMCPRegInfo clidr = {
>>              .name = "CLIDR", .state = ARM_CP_STATE_BOTH,
>> --
>> 1.7.1
>>
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[Peter Maydell]

On 18 August 2014 04:38, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sat, Aug 2, 2014 at 1:28 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This is a pretty random order for the fields in a reginfo struct
>> (though existing code is not great here either).
>> Preferred is to put the .name first, then .state, then the
>> encoding in the same order as the v8 ARM ARM:
>>     .cp [if needed], .opc0, .opc1, .crn, .crm, .opc2
>> then .access and .accessfn
>> then .fieldoffset and .resetvalue, then read and writefns.
>>
>
> Done. I like the new scheme:
>
>  792     { .name = "PMCNTENSET_EL0", .state = ARM_CP_STATE_AA64,
>  793       .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12,.opc2 = 1,
>  794       .access = PL0_RW, .accessfn = pmreg_access,
>  795       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcnten),
> .resetvalue = 0,
>  796       .writefn = pmcntenset_write, .raw_writefn = raw_write },
>
> For completeness where does .type fit in?

After .access and .accessfn, I think. (There's also a fair amount
of existing code which puts it just before those two, which doesn't
look too awful, but after is slightly more logical.)

-- PMM

[Qemu-devel] [PATCH v2 3/7] target-arm: Add arm_ccnt_enabled function

[Alistair Francis]

Include a helper function to determine if the CCNT counter
is enabled as well as the constants used to mask the pmccfiltr_el0
and c9_pmxevtyper registers.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index ce986ee..141e252 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
 }
 
 #ifndef CONFIG_USER_ONLY
+#define PMCCFILTR_NSH 0x8000000
+#define PMCCFILTR_P 0x80000000
+#define PMCCFILTR_U 0x40000000
+
+#define PMXEVTYPER_P 0x80000000
+#define PMXEVTYPER_U 0x40000000
+
+static bool arm_ccnt_enabled(CPUARMState *env)
+{
+    /* This does not support checking for the secure/non-secure
+     * components of the PMCCFILTR_EL0 register
+     */
+
+    if (!(env->cp15.c9_pmcr & PMCRE)) {
+        return 0;
+    }
+
+    if (arm_current_pl(env) == 2) {
+        if (!(env->cp15.pmccfiltr_el0 & PMCCFILTR_NSH)) {
+            return 0;
+        }
+    } else if (arm_current_pl(env) == 1) {
+        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_P) {
+            return 0;
+        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_P) {
+            return 0;
+        }
+    } else if (arm_current_pl(env) == 0) {
+        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_U) {
+            return 0;
+        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_U) {
+            return 0;
+        }
+    }
+
+    return 1;
+}
+#endif
+
+#ifndef CONFIG_USER_ONLY
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 3/7] target-arm: Add arm_ccnt_enabled function

[Peter Crosthwaite]

On Thu, Jun 26, 2014 at 3:02 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Include a helper function to determine if the CCNT counter
> is enabled as well as the constants used to mask the pmccfiltr_el0
> and c9_pmxevtyper registers.
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 40 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ce986ee..141e252 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
>  }
>
>  #ifndef CONFIG_USER_ONLY

Blank line here for readability.

> +#define PMCCFILTR_NSH 0x8000000
> +#define PMCCFILTR_P 0x80000000
> +#define PMCCFILTR_U 0x40000000
> +
> +#define PMXEVTYPER_P 0x80000000
> +#define PMXEVTYPER_U 0x40000000
> +
> +static bool arm_ccnt_enabled(CPUARMState *env)
> +{
> +    /* This does not support checking for the secure/non-secure
> +     * components of the PMCCFILTR_EL0 register
> +     */
> +
> +    if (!(env->cp15.c9_pmcr & PMCRE)) {
> +        return 0;
> +    }
> +
> +    if (arm_current_pl(env) == 2) {

switch(arm_current_pl(env))

> +        if (!(env->cp15.pmccfiltr_el0 & PMCCFILTR_NSH)) {
> +            return 0;

Use "true" and "false" for boolean function return values.

> +        }
> +    } else if (arm_current_pl(env) == 1) {
> +        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_P) {
> +            return 0;
> +        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_P) {

use an || to merge the two if branches with same function body.

> +            return 0;
> +        }
> +    } else if (arm_current_pl(env) == 0) {
> +        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_U) {
> +            return 0;
> +        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_U) {
> +            return 0;
> +        }
> +    }
> +
> +    return 1;
> +}
> +#endif
> +
> +#ifndef CONFIG_USER_ONLY

Just drop the extra #endif #ifndef CONFIG_USER_ONLY.

Regards,
Peter

>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t value)
>  {
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH v2 3/7] target-arm: Add arm_ccnt_enabled function

[Peter Crosthwaite]

On Thu, Jun 26, 2014 at 9:28 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Jun 26, 2014 at 3:02 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Include a helper function to determine if the CCNT counter
>> is enabled as well as the constants used to mask the pmccfiltr_el0
>> and c9_pmxevtyper registers.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/helper.c |   40 ++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index ce986ee..141e252 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -547,6 +547,46 @@ static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri)
>>  }
>>
>>  #ifndef CONFIG_USER_ONLY
>
> Blank line here for readability.
>
>> +#define PMCCFILTR_NSH 0x8000000
>> +#define PMCCFILTR_P 0x80000000
>> +#define PMCCFILTR_U 0x40000000
>> +
>> +#define PMXEVTYPER_P 0x80000000
>> +#define PMXEVTYPER_U 0x40000000
>> +
>> +static bool arm_ccnt_enabled(CPUARMState *env)
>> +{
>> +    /* This does not support checking for the secure/non-secure
>> +     * components of the PMCCFILTR_EL0 register
>> +     */
>> +
>> +    if (!(env->cp15.c9_pmcr & PMCRE)) {
>> +        return 0;
>> +    }
>> +
>> +    if (arm_current_pl(env) == 2) {
>
> switch(arm_current_pl(env))
>
>> +        if (!(env->cp15.pmccfiltr_el0 & PMCCFILTR_NSH)) {
>> +            return 0;
>
> Use "true" and "false" for boolean function return values.
>
>> +        }
>> +    } else if (arm_current_pl(env) == 1) {
>> +        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_P) {
>> +            return 0;
>> +        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_P) {
>
> use an || to merge the two if branches with same function body.
>
>> +            return 0;
>> +        }
>> +    } else if (arm_current_pl(env) == 0) {
>> +        if (env->cp15.pmccfiltr_el0 & PMCCFILTR_U) {
>> +            return 0;
>> +        } else if (env->cp15.c9_pmxevtyper & PMXEVTYPER_U) {
>> +            return 0;
>> +        }
>> +    }
>> +
>> +    return 1;
>> +}
>> +#endif
>> +
>> +#ifndef CONFIG_USER_ONLY
>
> Just drop the extra #endif #ifndef CONFIG_USER_ONLY.
>

All fixed (in a respun version I got from Alistair).

Regards,
Peter

> Regards,
> Peter
>
>>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t value)
>>  {
>> --
>> 1.7.1
>>
>>

[Qemu-devel] [PATCH v2 4/7] target-arm: Implement pmccntr_sync function

[Alistair Francis]

This is used to synchronise the PMCCNTR counter and swap its
state between enabled and disabled if required. It must always
be called twice, both before and after any logic that could
change the state of the PMCCNTR counter.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---
Remembering that the c15_ccnt register stores the last time
the counter was reset if enabled. If disabled it stores the
counter value (when it was disabled).

The three use cases are as below:
--
Starts enabled/disabled and is staying enabled/disabled
--
The two calls to pmccntr_sync cancel each other out.
Each call implements this logic:
env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt

Which expands to:
env->cp15.c15_ccnt = temp_ticks -
                     (temp_ticks - env->cp15.c15_ccnt)
env->cp15.c15_ccnt = env->cp15.c15_ccnt

--
Starts enabled, gets disabled
--
The logic is run during the first call while during
the second call it is not. That means that c15_ccnt
changes from storing the last time the counter was
reset, to storing the absolute value of the counter.

--
Starts disabled, gets enabled
--
During the fist call no changes are made, while during
the second call the register is changed. This changes it
from storing the absolute value to storing the last time
the counter was reset.


 target-arm/cpu.h    |   11 +++++++++++
 target-arm/helper.c |   19 +++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 6a2efd8..fc1a70d 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -1032,6 +1032,17 @@ static inline bool cp_access_ok(int current_pl,
 }
 
 /**
+ * pmccntr_sync
+ * @cpu: ARMCPU
+ *
+ * Syncronises the counter in the PMCCNTR. This must always be called twice,
+ * once before any action that might effect the timer and again afterwards.
+ * The function is used to swap the state of the register if required.
+ * This only happens when not in user mode (!CONFIG_USER_ONLY)
+ */
+void pmccntr_sync(CPUARMState *env);
+
+/**
  * write_list_to_cpustate
  * @cpu: ARMCPU
  *
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 141e252..016fe47 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -586,6 +586,25 @@ static bool arm_ccnt_enabled(CPUARMState *env)
 }
 #endif
 
+void pmccntr_sync(CPUARMState *env)
+{
+#ifndef CONFIG_USER_ONLY
+    uint64_t temp_ticks;
+
+    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
+                          get_ticks_per_sec(), 1000000);
+
+    if (env->cp15.c9_pmcr & PMCRD) {
+        /* Increment once every 64 processor clock cycles */
+        temp_ticks /= 64;
+    }
+
+    if (arm_ccnt_enabled(env)) {
+        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
+    }
+#endif
+}
+
 #ifndef CONFIG_USER_ONLY
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
-- 
1.7.1

[Qemu-devel] [PATCH v2 5/7] target-arm: Remove old code and replace with new functions

[Alistair Francis]

Remove the old PMCCNTR code and replace it with calls to the new
pmccntr_sync() function and the CCNT_ENABLED macro

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c |   27 ++++-----------------------
 1 files changed, 4 insertions(+), 23 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 016fe47..0bd00cb 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)
 static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                        uint64_t value)
 {
-    uint64_t temp_ticks;
-
-    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
-                          get_ticks_per_sec(), 1000000);
-
-    if (env->cp15.c9_pmcr & PMCRE) {
-        /* If the counter is enabled */
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
-            env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
-        } else {
-            env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
-        }
-    }
+    pmccntr_sync(env);
 
     if (value & PMCRC) {
         /* The counter has been reset */
@@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
     env->cp15.c9_pmcr &= ~0x39;
     env->cp15.c9_pmcr |= (value & 0x39);
 
-    if (env->cp15.c9_pmcr & PMCRE) {
-        if (env->cp15.c9_pmcr & PMCRD) {
-            /* Increment once every 64 processor clock cycles */
-            temp_ticks /= 64;
-        }
-        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
-    }
+    pmccntr_sync(env);
 }
 
 static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
 {
     uint64_t total_ticks;
 
-    if (!(env->cp15.c9_pmcr & PMCRE)) {
+    if (!arm_ccnt_enabled(env)) {
         /* Counter is disabled, do not change value */
         return env->cp15.c15_ccnt;
     }
@@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 {
     uint64_t total_ticks;
 
-    if (!(env->cp15.c9_pmcr & PMCRE)) {
+    if (!arm_ccnt_enabled(env)) {
         /* Counter is disabled, set the absolute value */
         env->cp15.c15_ccnt = value;
         return;
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 5/7] target-arm: Remove old code and replace with new functions

[Peter Crosthwaite]

On Thu, Jun 26, 2014 at 3:02 PM, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Remove the old PMCCNTR code and replace it with calls to the new
> pmccntr_sync() function and the CCNT_ENABLED macro

arm_ccnt_enabled() function.

>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper.c |   27 ++++-----------------------
>  1 files changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 016fe47..0bd00cb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)
>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                         uint64_t value)
>  {
> -    uint64_t temp_ticks;
> -
> -    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
> -                          get_ticks_per_sec(), 1000000);
> -
> -    if (env->cp15.c9_pmcr & PMCRE) {
> -        /* If the counter is enabled */
> -        if (env->cp15.c9_pmcr & PMCRD) {
> -            /* Increment once every 64 processor clock cycles */
> -            env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
> -        } else {
> -            env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
> -        }
> -    }
> +    pmccntr_sync(env);
>
>      if (value & PMCRC) {
>          /* The counter has been reset */
> @@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      env->cp15.c9_pmcr &= ~0x39;
>      env->cp15.c9_pmcr |= (value & 0x39);
>
> -    if (env->cp15.c9_pmcr & PMCRE) {
> -        if (env->cp15.c9_pmcr & PMCRD) {
> -            /* Increment once every 64 processor clock cycles */
> -            temp_ticks /= 64;
> -        }
> -        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
> -    }
> +    pmccntr_sync(env);
>  }
>
>  static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>  {
>      uint64_t total_ticks;
>
> -    if (!(env->cp15.c9_pmcr & PMCRE)) {
> +    if (!arm_ccnt_enabled(env)) {
>          /* Counter is disabled, do not change value */
>          return env->cp15.c15_ccnt;
>      }
> @@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>  {
>      uint64_t total_ticks;
>
> -    if (!(env->cp15.c9_pmcr & PMCRE)) {
> +    if (!arm_ccnt_enabled(env)) {

Is it valid to write to the running counter?

If so you can sync(), update sync() to implement easily now and drop the if.

Regards,
Peter

>          /* Counter is disabled, set the absolute value */
>          env->cp15.c15_ccnt = value;
>          return;
> --
> 1.7.1
>
>

Re: [Qemu-devel] [PATCH v2 5/7] target-arm: Remove old code and replace with new functions

[Alistair Francis]

On Thu, Jun 26, 2014 at 9:38 PM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Thu, Jun 26, 2014 at 3:02 PM, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Remove the old PMCCNTR code and replace it with calls to the new
>> pmccntr_sync() function and the CCNT_ENABLED macro
>
> arm_ccnt_enabled() function.
>
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/helper.c |   27 ++++-----------------------
>>  1 files changed, 4 insertions(+), 23 deletions(-)
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 016fe47..0bd00cb 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -609,20 +609,7 @@ void pmccntr_sync(CPUARMState *env)
>>  static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                         uint64_t value)
>>  {
>> -    uint64_t temp_ticks;
>> -
>> -    temp_ticks = muldiv64(qemu_clock_get_us(QEMU_CLOCK_VIRTUAL),
>> -                          get_ticks_per_sec(), 1000000);
>> -
>> -    if (env->cp15.c9_pmcr & PMCRE) {
>> -        /* If the counter is enabled */
>> -        if (env->cp15.c9_pmcr & PMCRD) {
>> -            /* Increment once every 64 processor clock cycles */
>> -            env->cp15.c15_ccnt = (temp_ticks/64) - env->cp15.c15_ccnt;
>> -        } else {
>> -            env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>> -        }
>> -    }
>> +    pmccntr_sync(env);
>>
>>      if (value & PMCRC) {
>>          /* The counter has been reset */
>> @@ -633,20 +620,14 @@ static void pmcr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>      env->cp15.c9_pmcr &= ~0x39;
>>      env->cp15.c9_pmcr |= (value & 0x39);
>>
>> -    if (env->cp15.c9_pmcr & PMCRE) {
>> -        if (env->cp15.c9_pmcr & PMCRD) {
>> -            /* Increment once every 64 processor clock cycles */
>> -            temp_ticks /= 64;
>> -        }
>> -        env->cp15.c15_ccnt = temp_ticks - env->cp15.c15_ccnt;
>> -    }
>> +    pmccntr_sync(env);
>>  }
>>
>>  static uint64_t pmccntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>>  {
>>      uint64_t total_ticks;
>>
>> -    if (!(env->cp15.c9_pmcr & PMCRE)) {
>> +    if (!arm_ccnt_enabled(env)) {
>>          /* Counter is disabled, do not change value */
>>          return env->cp15.c15_ccnt;
>>      }
>> @@ -666,7 +647,7 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>  {
>>      uint64_t total_ticks;
>>
>> -    if (!(env->cp15.c9_pmcr & PMCRE)) {
>> +    if (!arm_ccnt_enabled(env)) {
>
> Is it valid to write to the running counter?

It is a RW register, so I'm assuming it can be set

>
> If so you can sync(), update sync() to implement easily now and drop the if.

It's not the same process as the other syncs. This logic includes the value that
is being written, which the sync function doesn't. It would require
changes to the
sync function, with values to be passed in. It doesn't seem like it's
worth doing

>
> Regards,
> Peter
>
>>          /* Counter is disabled, set the absolute value */
>>          env->cp15.c15_ccnt = value;
>>          return;
>> --
>> 1.7.1
>>
>>
>

[Qemu-devel] [PATCH v2 6/7] target-arm: Implement pmccfiltr_write function

[Alistair Francis]

This is the function that is called when writing to the
PMCCFILTR_EL0 register

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 0bd00cb..e78c5a7 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -664,6 +664,14 @@ static void pmccntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
 }
 #endif
 
+static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                            uint64_t value)
+{
+    pmccntr_sync(env);
+    env->cp15.pmccfiltr_el0 = value & 0x7E000000;
+    pmccntr_sync(env);
+}
+
 static void pmcntenset_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -826,6 +834,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
 #endif
     { .name = "PMCCFILTR_EL0", .state = ARM_CP_STATE_AA64,
       .opc0 = 3, .opc1 = 3, .crn = 14, .crm = 15, .opc2 = 7,
+      .writefn = pmccfiltr_write,
       .access = PL0_RW, .accessfn = pmreg_access,
       .state = ARM_CP_STATE_AA64,
       .resetvalue = 0,
-- 
1.7.1

[Qemu-devel] [PATCH v2 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Alistair Francis]

Call the new pmccntr_sync() function when there is a possibility
of swapping ELs (I.E. when there is an exception)

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper-a64.c |    5 +++++
 target-arm/helper.c     |    7 +++++++
 target-arm/op_helper.c  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
index 2b4ce6a..b61174f 100644
--- a/target-arm/helper-a64.c
+++ b/target-arm/helper-a64.c
@@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
     target_ulong addr = env->cp15.vbar_el[1];
     int i;
 
+    pmccntr_sync(env);
+
     if (arm_current_pl(env) == 0) {
         if (env->aarch64) {
             addr += 0x400;
@@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
         addr += 0x100;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
     }
 
@@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
 
     env->pc = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
diff --git a/target-arm/helper.c b/target-arm/helper.c
index e78c5a7..f05d912 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
 
     assert(!IS_M(env));
 
+    pmccntr_sync(env);
+
     arm_log_exception(cs->exception_index);
 
     /* TODO: Vectored interrupt controller.  */
@@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                   && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
                 env->regs[15] += 2;
                 env->regs[0] = do_arm_semihosting(env);
                 qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
+                pmccntr_sync(env);
                 return;
             }
         }
@@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
         offset = 4;
         break;
     default:
+        pmccntr_sync(env);
         cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
         return; /* Never happens.  Keep compiler happy.  */
     }
@@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
     env->regs[14] = env->regs[15] + offset;
     env->regs[15] = addr;
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+    pmccntr_sync(env);
 }
 
 /* Check section/page access permissions.
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 9c1ef52..07ab30b 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
     uint32_t spsr = env->banked_spsr[spsr_idx];
     int new_el, i;
 
+    pmccntr_sync(env);
+
     if (env->pstate & PSTATE_SP) {
         env->sp_el[cur_el] = env->xregs[31];
     } else {
@@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
         env->pc = env->elr_el[cur_el];
     }
 
+    pmccntr_sync(env);
+
     return;
 
 illegal_return:
@@ -433,6 +437,8 @@ illegal_return:
     spsr &= PSTATE_NZCV | PSTATE_DAIF;
     spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
     pstate_write(env, spsr);
+
+    pmccntr_sync(env);
 }
 
 /* ??? Flag setting arithmetic is awkward because we need to do comparisons.
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Peter Maydell]

On 26 June 2014 06:02, Alistair Francis <alistair.francis@xilinx.com> wrote:
> Call the new pmccntr_sync() function when there is a possibility
> of swapping ELs (I.E. when there is an exception)
>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper-a64.c |    5 +++++
>  target-arm/helper.c     |    7 +++++++
>  target-arm/op_helper.c  |    6 ++++++
>  3 files changed, 18 insertions(+), 0 deletions(-)
>
> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
> index 2b4ce6a..b61174f 100644
> --- a/target-arm/helper-a64.c
> +++ b/target-arm/helper-a64.c
> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>      target_ulong addr = env->cp15.vbar_el[1];
>      int i;
>
> +    pmccntr_sync(env);
> +
>      if (arm_current_pl(env) == 0) {
>          if (env->aarch64) {
>              addr += 0x400;
> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>          addr += 0x100;
>          break;
>      default:
> +        pmccntr_sync(env);
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>      }
>
> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>
>      env->pc = addr;
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +    pmccntr_sync(env);
>  }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index e78c5a7..f05d912 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>
>      assert(!IS_M(env));
>
> +    pmccntr_sync(env);
> +
>      arm_log_exception(cs->exception_index);
>
>      /* TODO: Vectored interrupt controller.  */
> @@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>                    && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
>                  env->regs[0] = do_arm_semihosting(env);
>                  qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
> +                pmccntr_sync(env);
>                  return;
>              }
>          }
> @@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>                  env->regs[15] += 2;
>                  env->regs[0] = do_arm_semihosting(env);
>                  qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
> +                pmccntr_sync(env);
>                  return;
>              }
>          }
> @@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>          offset = 4;
>          break;
>      default:
> +        pmccntr_sync(env);
>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>          return; /* Never happens.  Keep compiler happy.  */
>      }
> @@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>      env->regs[14] = env->regs[15] + offset;
>      env->regs[15] = addr;
>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> +
> +    pmccntr_sync(env);
>  }
>
>  /* Check section/page access permissions.
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index 9c1ef52..07ab30b 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
>      uint32_t spsr = env->banked_spsr[spsr_idx];
>      int new_el, i;
>
> +    pmccntr_sync(env);
> +
>      if (env->pstate & PSTATE_SP) {
>          env->sp_el[cur_el] = env->xregs[31];
>      } else {
> @@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
>          env->pc = env->elr_el[cur_el];
>      }
>
> +    pmccntr_sync(env);
> +
>      return;
>
>  illegal_return:
> @@ -433,6 +437,8 @@ illegal_return:
>      spsr &= PSTATE_NZCV | PSTATE_DAIF;
>      spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
>      pstate_write(env, spsr);
> +
> +    pmccntr_sync(env);
>  }

This feels way too fragile to me to be scattering these sync
calls all over the place like this. We need to find a better
approach than this.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 7/7] target-arm: Call the pmccntr_sync function when swapping ELs

[Peter Crosthwaite]

On Sat, Aug 2, 2014 at 1:35 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 June 2014 06:02, Alistair Francis <alistair.francis@xilinx.com> wrote:
>> Call the new pmccntr_sync() function when there is a possibility
>> of swapping ELs (I.E. when there is an exception)
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  target-arm/helper-a64.c |    5 +++++
>>  target-arm/helper.c     |    7 +++++++
>>  target-arm/op_helper.c  |    6 ++++++
>>  3 files changed, 18 insertions(+), 0 deletions(-)
>>
>> diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c
>> index 2b4ce6a..b61174f 100644
>> --- a/target-arm/helper-a64.c
>> +++ b/target-arm/helper-a64.c
>> @@ -446,6 +446,8 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>      target_ulong addr = env->cp15.vbar_el[1];
>>      int i;
>>
>> +    pmccntr_sync(env);
>> +
>>      if (arm_current_pl(env) == 0) {
>>          if (env->aarch64) {
>>              addr += 0x400;
>> @@ -484,6 +486,7 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>          addr += 0x100;
>>          break;
>>      default:
>> +        pmccntr_sync(env);
>>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>>      }
>>
>> @@ -511,4 +514,6 @@ void aarch64_cpu_do_interrupt(CPUState *cs)
>>
>>      env->pc = addr;
>>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>> +
>> +    pmccntr_sync(env);
>>  }
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index e78c5a7..f05d912 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3457,6 +3457,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>
>>      assert(!IS_M(env));
>>
>> +    pmccntr_sync(env);
>> +
>>      arm_log_exception(cs->exception_index);
>>
>>      /* TODO: Vectored interrupt controller.  */
>> @@ -3487,6 +3489,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>                    && (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
>>                  env->regs[0] = do_arm_semihosting(env);
>>                  qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
>> +                pmccntr_sync(env);
>>                  return;
>>              }
>>          }
>> @@ -3505,6 +3508,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>                  env->regs[15] += 2;
>>                  env->regs[0] = do_arm_semihosting(env);
>>                  qemu_log_mask(CPU_LOG_INT, "...handled as semihosting call\n");
>> +                pmccntr_sync(env);
>>                  return;
>>              }
>>          }
>> @@ -3548,6 +3552,7 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>          offset = 4;
>>          break;
>>      default:
>> +        pmccntr_sync(env);
>>          cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index);
>>          return; /* Never happens.  Keep compiler happy.  */
>>      }
>> @@ -3580,6 +3585,8 @@ void arm_cpu_do_interrupt(CPUState *cs)
>>      env->regs[14] = env->regs[15] + offset;
>>      env->regs[15] = addr;
>>      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>> +
>> +    pmccntr_sync(env);
>>  }
>>
>>  /* Check section/page access permissions.
>> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
>> index 9c1ef52..07ab30b 100644
>> --- a/target-arm/op_helper.c
>> +++ b/target-arm/op_helper.c
>> @@ -376,6 +376,8 @@ void HELPER(exception_return)(CPUARMState *env)
>>      uint32_t spsr = env->banked_spsr[spsr_idx];
>>      int new_el, i;
>>
>> +    pmccntr_sync(env);
>> +
>>      if (env->pstate & PSTATE_SP) {
>>          env->sp_el[cur_el] = env->xregs[31];
>>      } else {
>> @@ -418,6 +420,8 @@ void HELPER(exception_return)(CPUARMState *env)
>>          env->pc = env->elr_el[cur_el];
>>      }
>>
>> +    pmccntr_sync(env);
>> +
>>      return;
>>
>>  illegal_return:
>> @@ -433,6 +437,8 @@ illegal_return:
>>      spsr &= PSTATE_NZCV | PSTATE_DAIF;
>>      spsr |= pstate_read(env) & ~(PSTATE_NZCV | PSTATE_DAIF);
>>      pstate_write(env, spsr);
>> +
>> +    pmccntr_sync(env);
>>  }
>
> This feels way too fragile to me to be scattering these sync
> calls all over the place like this. We need to find a better
> approach than this.
>

If we could consolidate all the code paths that actually switch el
(both ways) into a helper fn we could then add the pmccntr
el-switching side effects (i.e. the sync) to that helper once. Would
you accept such an approach?

Regards,
Peter

> thanks
> -- PMM
>