[Qemu-devel] [PATCH v2 0/5] Extend the performance monitoring registers

[Qemu-devel] [PATCH v2 0/5] Extend the performance monitoring registers

[Alistair Francis]

This patch set is based on the patch sent by Christopher Covington and
written by Aaron Lindsay which was sent as an RFC (Implement remaining
PMU functionality).

It adds a few performance monitoring related registers.

V2:
 - Add Aaron to the signed off lines
 - Add the tested-by lines from Nathan
 - Add two more patches from Chris


Alistair Francis (5):
  target-arm: Add the pmceid0 and pmceid1 registers
  target-arm: Add Some of the performance monitor registers
  target-arm: Add the pmovsclr_el0 and pmintenclr_el1 registers
  target-arm: Add PMUSERENR_EL0 register
  target-arm: Unmask PMU bits in debug feature register

 target-arm/cpu-qom.h |   2 +
 target-arm/cpu.c     |   2 +
 target-arm/cpu.h     |   6 +++
 target-arm/cpu64.c   |   2 +
 target-arm/helper.c  | 122 ++++++++++++++++++++++++++++++++++++++++++++-------
 5 files changed, 117 insertions(+), 17 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Alistair Francis]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Nathan Rossi <nathan@nathanrossi.com>
---

 target-arm/cpu-qom.h | 2 ++
 target-arm/cpu.c     | 2 ++
 target-arm/cpu64.c   | 2 ++
 target-arm/helper.c  | 8 ++++++++
 4 files changed, 14 insertions(+)

diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
index 07c0a71..1cc4502 100644
--- a/target-arm/cpu-qom.h
+++ b/target-arm/cpu-qom.h
@@ -148,6 +148,8 @@ typedef struct ARMCPU {
     uint32_t id_pfr0;
     uint32_t id_pfr1;
     uint32_t id_dfr0;
+    uint32_t pmceid0;
+    uint32_t pmceid1;
     uint32_t id_afr0;
     uint32_t id_mmfr0;
     uint32_t id_mmfr1;
diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 7ddbf3d..937f845 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
     cpu->id_mmfr1 = 0x20000000;
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index c847513..8c4b6fd 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
+    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;
     cpu->dbgdidr = 0x3516d000;
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5ea507f..66aa406 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
               .resetvalue = cpu->id_aa64dfr1 },
+            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid0},
+            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
+              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
+              .access = PL1_R, .type = ARM_CP_CONST,
+              .resetvalue = cpu->pmceid1},
             { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
               .access = PL1_R, .type = ARM_CP_CONST,
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Peter Maydell]

On 6 February 2016 at 00:55, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
> ---
>
>  target-arm/cpu-qom.h | 2 ++
>  target-arm/cpu.c     | 2 ++
>  target-arm/cpu64.c   | 2 ++
>  target-arm/helper.c  | 8 ++++++++
>  4 files changed, 14 insertions(+)
>
> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> index 07c0a71..1cc4502 100644
> --- a/target-arm/cpu-qom.h
> +++ b/target-arm/cpu-qom.h
> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
>      uint32_t id_pfr0;
>      uint32_t id_pfr1;
>      uint32_t id_dfr0;
> +    uint32_t pmceid0;
> +    uint32_t pmceid1;
>      uint32_t id_afr0;
>      uint32_t id_mmfr0;
>      uint32_t id_mmfr1;
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 7ddbf3d..937f845 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
>      cpu->id_pfr0 = 0x00001131;
>      cpu->id_pfr1 = 0x00011011;
>      cpu->id_dfr0 = 0x02010555;
> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */

These are:
 SW_INCR   # insn architecturally executed, cc pass, software increment
 INST_RETIRED # insn architecturally executed
 CPU_CYCLES # cycle

However we don't actually implement any of these, so should
we be advertising them?

> +    cpu->pmceid1 = 0x00000000;
>      cpu->id_afr0 = 0x00000000;
>      cpu->id_mmfr0 = 0x10201105;
>      cpu->id_mmfr1 = 0x20000000;
> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> index c847513..8c4b6fd 100644
> --- a/target-arm/cpu64.c
> +++ b/target-arm/cpu64.c
> @@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
>      cpu->id_isar5 = 0x00011121;
>      cpu->id_aa64pfr0 = 0x00002222;
>      cpu->id_aa64dfr0 = 0x10305106;
> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> +    cpu->pmceid1 = 0x00000000;
>      cpu->id_aa64isar0 = 0x00011120;
>      cpu->id_aa64mmfr0 = 0x00001124;
>      cpu->dbgdidr = 0x3516d000;
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 5ea507f..66aa406 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
>                .access = PL1_R, .type = ARM_CP_CONST,
>                .resetvalue = cpu->id_aa64dfr1 },
> +            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
> +              .access = PL1_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->pmceid0},

These have 32-bit versions from v8 and up (sadly not with the
right opc values to use STATE_BOTH, so second stanza needed).

These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN,
so you want
   .access = PL0_R, .accessfn = pmreg_access

Space before final "}", please.

Can we move these down so they're not placed right in the
middle of the ID_AA64* registers ?

> +            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
> +              .access = PL1_R, .type = ARM_CP_CONST,
> +              .resetvalue = cpu->pmceid1},

Ditto.

>              { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
>                .access = PL1_R, .type = ARM_CP_CONST,
> --
> 2.5.0

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Christopher Covington]

On 02/09/2016 12:19 PM, Peter Maydell wrote:
> On 6 February 2016 at 00:55, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
>> ---
>>
>>  target-arm/cpu-qom.h | 2 ++
>>  target-arm/cpu.c     | 2 ++
>>  target-arm/cpu64.c   | 2 ++
>>  target-arm/helper.c  | 8 ++++++++
>>  4 files changed, 14 insertions(+)
>>
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 07c0a71..1cc4502 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
>>      uint32_t id_pfr0;
>>      uint32_t id_pfr1;
>>      uint32_t id_dfr0;
>> +    uint32_t pmceid0;
>> +    uint32_t pmceid1;
>>      uint32_t id_afr0;
>>      uint32_t id_mmfr0;
>>      uint32_t id_mmfr1;
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 7ddbf3d..937f845 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
>>      cpu->id_pfr0 = 0x00001131;
>>      cpu->id_pfr1 = 0x00011011;
>>      cpu->id_dfr0 = 0x02010555;
>> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> 
> These are:
>  SW_INCR   # insn architecturally executed, cc pass, software increment
>  INST_RETIRED # insn architecturally executed
>  CPU_CYCLES # cycle
> 
> However we don't actually implement any of these, so should
> we be advertising them?

Perhaps I'm missing something, but I was under the impression that CPU
cycle accounting was implemented as pmccntr_read/write in
target-arm/helper.c.

The instruction count event may need a wrapper around cpu_get_icount().

SWINC is pretty trivial, but I don't think we actually use it, other
than for some testing (but unfortunately not yet part of the
kvm-unit-tests patchset).

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Peter Maydell]

On 9 February 2016 at 17:48, Christopher Covington <cov@codeaurora.org> wrote:
> On 02/09/2016 12:19 PM, Peter Maydell wrote:
>> On 6 February 2016 at 00:55, Alistair Francis
>> <alistair.francis@xilinx.com> wrote:
>>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>>> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
>>> ---
>>>
>>>  target-arm/cpu-qom.h | 2 ++
>>>  target-arm/cpu.c     | 2 ++
>>>  target-arm/cpu64.c   | 2 ++
>>>  target-arm/helper.c  | 8 ++++++++
>>>  4 files changed, 14 insertions(+)
>>>
>>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>>> index 07c0a71..1cc4502 100644
>>> --- a/target-arm/cpu-qom.h
>>> +++ b/target-arm/cpu-qom.h
>>> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
>>>      uint32_t id_pfr0;
>>>      uint32_t id_pfr1;
>>>      uint32_t id_dfr0;
>>> +    uint32_t pmceid0;
>>> +    uint32_t pmceid1;
>>>      uint32_t id_afr0;
>>>      uint32_t id_mmfr0;
>>>      uint32_t id_mmfr1;
>>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>>> index 7ddbf3d..937f845 100644
>>> --- a/target-arm/cpu.c
>>> +++ b/target-arm/cpu.c
>>> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
>>>      cpu->id_pfr0 = 0x00001131;
>>>      cpu->id_pfr1 = 0x00011011;
>>>      cpu->id_dfr0 = 0x02010555;
>>> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
>>
>> These are:
>>  SW_INCR   # insn architecturally executed, cc pass, software increment
>>  INST_RETIRED # insn architecturally executed
>>  CPU_CYCLES # cycle
>>
>> However we don't actually implement any of these, so should
>> we be advertising them?
>
> Perhaps I'm missing something, but I was under the impression that CPU
> cycle accounting was implemented as pmccntr_read/write in
> target-arm/helper.c.

Yes, but that's not the same as implementing it as an event
visible and controllable via the PM[X]EVCNTR and PM[X]EVTYPER
register interface. (The cycle count has its own set of
dedicated registers.)

> The instruction count event may need a wrapper around cpu_get_icount().
>
> SWINC is pretty trivial, but I don't think we actually use it, other
> than for some testing (but unfortunately not yet part of the
> kvm-unit-tests patchset).

Yep, so we could implement an event or two. The question is
(a) is it useful to do so? (quite possibly, though trying to get any
  kind of legitimate perf data out of a model is at best dicey)
(b) is it useful to advertise their support in this patchset but
  not actually implement the events? (much less clear)

If we want to add real events I think that should probably
be its own patchset, and for this patchset we should stick
to bringing the 64-bit PMU implementation into line with our
existing 32-bit implementation (which is a "we support no
event counters, only the separate cycle counter" implementation,
which I think is architecturally permitted: PMCR_EL0.N == 0)

Similarly in a later patch I don't think we should implement
the non-trivial PM[X]EVCNTR/PM[X]EVTYPER registers and
the selectors until we actually implement some real events
to count, ie we should postpone that to that future patchset,
not put it in this one.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Alistair Francis]

On Tue, Feb 9, 2016 at 9:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 February 2016 at 00:55, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
>> ---
>>
>>  target-arm/cpu-qom.h | 2 ++
>>  target-arm/cpu.c     | 2 ++
>>  target-arm/cpu64.c   | 2 ++
>>  target-arm/helper.c  | 8 ++++++++
>>  4 files changed, 14 insertions(+)
>>
>> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
>> index 07c0a71..1cc4502 100644
>> --- a/target-arm/cpu-qom.h
>> +++ b/target-arm/cpu-qom.h
>> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
>>      uint32_t id_pfr0;
>>      uint32_t id_pfr1;
>>      uint32_t id_dfr0;
>> +    uint32_t pmceid0;
>> +    uint32_t pmceid1;
>>      uint32_t id_afr0;
>>      uint32_t id_mmfr0;
>>      uint32_t id_mmfr1;
>> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> index 7ddbf3d..937f845 100644
>> --- a/target-arm/cpu.c
>> +++ b/target-arm/cpu.c
>> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
>>      cpu->id_pfr0 = 0x00001131;
>>      cpu->id_pfr1 = 0x00011011;
>>      cpu->id_dfr0 = 0x02010555;
>> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
>
> These are:
>  SW_INCR   # insn architecturally executed, cc pass, software increment
>  INST_RETIRED # insn architecturally executed
>  CPU_CYCLES # cycle
>
> However we don't actually implement any of these, so should
> we be advertising them?

So this part I took directly from Chris's RFC. I'm happy to take it
out if you would like.

>
>> +    cpu->pmceid1 = 0x00000000;
>>      cpu->id_afr0 = 0x00000000;
>>      cpu->id_mmfr0 = 0x10201105;
>>      cpu->id_mmfr1 = 0x20000000;
>> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
>> index c847513..8c4b6fd 100644
>> --- a/target-arm/cpu64.c
>> +++ b/target-arm/cpu64.c
>> @@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
>>      cpu->id_isar5 = 0x00011121;
>>      cpu->id_aa64pfr0 = 0x00002222;
>>      cpu->id_aa64dfr0 = 0x10305106;
>> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
>> +    cpu->pmceid1 = 0x00000000;
>>      cpu->id_aa64isar0 = 0x00011120;
>>      cpu->id_aa64mmfr0 = 0x00001124;
>>      cpu->dbgdidr = 0x3516d000;
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 5ea507f..66aa406 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>>                .resetvalue = cpu->id_aa64dfr1 },
>> +            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
>> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
>> +              .access = PL1_R, .type = ARM_CP_CONST,
>> +              .resetvalue = cpu->pmceid0},
>
> These have 32-bit versions from v8 and up (sadly not with the
> right opc values to use STATE_BOTH, so second stanza needed).

Ok, I have added PMCEID0 and PMCEID1.

>
> These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN,
> so you want
>    .access = PL0_R, .accessfn = pmreg_access
>
> Space before final "}", please.
>
> Can we move these down so they're not placed right in the
> middle of the ID_AA64* registers ?

Fixed the rest.

Thanks,

Alistair

>
>> +            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
>> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
>> +              .access = PL1_R, .type = ARM_CP_CONST,
>> +              .resetvalue = cpu->pmceid1},
>
> Ditto.
>
>>              { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
>>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
>>                .access = PL1_R, .type = ARM_CP_CONST,
>> --
>> 2.5.0
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Aaron Lindsay]

On Feb 09 15:11, Alistair Francis wrote:
> On Tue, Feb 9, 2016 at 9:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 6 February 2016 at 00:55, Alistair Francis
> > <alistair.francis@xilinx.com> wrote:
> >> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> >> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> >> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
> >> ---
> >>
> >>  target-arm/cpu-qom.h | 2 ++
> >>  target-arm/cpu.c     | 2 ++
> >>  target-arm/cpu64.c   | 2 ++
> >>  target-arm/helper.c  | 8 ++++++++
> >>  4 files changed, 14 insertions(+)
> >>
> >> diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h
> >> index 07c0a71..1cc4502 100644
> >> --- a/target-arm/cpu-qom.h
> >> +++ b/target-arm/cpu-qom.h
> >> @@ -148,6 +148,8 @@ typedef struct ARMCPU {
> >>      uint32_t id_pfr0;
> >>      uint32_t id_pfr1;
> >>      uint32_t id_dfr0;
> >> +    uint32_t pmceid0;
> >> +    uint32_t pmceid1;
> >>      uint32_t id_afr0;
> >>      uint32_t id_mmfr0;
> >>      uint32_t id_mmfr1;
> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> >> index 7ddbf3d..937f845 100644
> >> --- a/target-arm/cpu.c
> >> +++ b/target-arm/cpu.c
> >> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
> >>      cpu->id_pfr0 = 0x00001131;
> >>      cpu->id_pfr1 = 0x00011011;
> >>      cpu->id_dfr0 = 0x02010555;
> >> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> >
> > These are:
> >  SW_INCR   # insn architecturally executed, cc pass, software increment
> >  INST_RETIRED # insn architecturally executed
> >  CPU_CYCLES # cycle
> >
> > However we don't actually implement any of these, so should
> > we be advertising them?
> 
> So this part I took directly from Chris's RFC. I'm happy to take it
> out if you would like.

I think removing the PMCEID0 change makes sense since these patches
don't implement the advertised counters. We have other patches which do
implement them, but they need some more work, so we can make this change
if/when they're actually implemented.

-Aaron

> 
> >
> >> +    cpu->pmceid1 = 0x00000000;
> >>      cpu->id_afr0 = 0x00000000;
> >>      cpu->id_mmfr0 = 0x10201105;
> >>      cpu->id_mmfr1 = 0x20000000;
> >> diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
> >> index c847513..8c4b6fd 100644
> >> --- a/target-arm/cpu64.c
> >> +++ b/target-arm/cpu64.c
> >> @@ -134,6 +134,8 @@ static void aarch64_a57_initfn(Object *obj)
> >>      cpu->id_isar5 = 0x00011121;
> >>      cpu->id_aa64pfr0 = 0x00002222;
> >>      cpu->id_aa64dfr0 = 0x10305106;
> >> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
> >> +    cpu->pmceid1 = 0x00000000;
> >>      cpu->id_aa64isar0 = 0x00011120;
> >>      cpu->id_aa64mmfr0 = 0x00001124;
> >>      cpu->dbgdidr = 0x3516d000;
> >> diff --git a/target-arm/helper.c b/target-arm/helper.c
> >> index 5ea507f..66aa406 100644
> >> --- a/target-arm/helper.c
> >> +++ b/target-arm/helper.c
> >> @@ -4192,6 +4192,14 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
> >>                .access = PL1_R, .type = ARM_CP_CONST,
> >>                .resetvalue = cpu->id_aa64dfr1 },
> >> +            { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64,
> >> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6,
> >> +              .access = PL1_R, .type = ARM_CP_CONST,
> >> +              .resetvalue = cpu->pmceid0},
> >
> > These have 32-bit versions from v8 and up (sadly not with the
> > right opc values to use STATE_BOTH, so second stanza needed).
> 
> Ok, I have added PMCEID0 and PMCEID1.
> 
> >
> > These are configurably RO from EL0, controlled by PMUSERENR_EL0.EN,
> > so you want
> >    .access = PL0_R, .accessfn = pmreg_access
> >
> > Space before final "}", please.
> >
> > Can we move these down so they're not placed right in the
> > middle of the ID_AA64* registers ?
> 
> Fixed the rest.
> 
> Thanks,
> 
> Alistair
> 
> >
> >> +            { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64,
> >> +              .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7,
> >> +              .access = PL1_R, .type = ARM_CP_CONST,
> >> +              .resetvalue = cpu->pmceid1},
> >
> > Ditto.
> >
> >>              { .name = "ID_AA64AFR0_EL1", .state = ARM_CP_STATE_AA64,
> >>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 4,
> >>                .access = PL1_R, .type = ARM_CP_CONST,
> >> --
> >> 2.5.0
> >
> > thanks
> > -- PMM
> >

Re: [Qemu-devel] [PATCH v2 1/5] target-arm: Add the pmceid0 and pmceid1 registers

[Peter Maydell]

On 10 February 2016 at 13:52, Aaron Lindsay <alindsay@codeaurora.org> wrote:
> On Feb 09 15:11, Alistair Francis wrote:
>> On Tue, Feb 9, 2016 at 9:19 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > On 6 February 2016 at 00:55, Alistair Francis
>> > <alistair.francis@xilinx.com> wrote:
>> >> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
>> >> index 7ddbf3d..937f845 100644
>> >> --- a/target-arm/cpu.c
>> >> +++ b/target-arm/cpu.c
>> >> @@ -1156,6 +1156,8 @@ static void cortex_a15_initfn(Object *obj)
>> >>      cpu->id_pfr0 = 0x00001131;
>> >>      cpu->id_pfr1 = 0x00011011;
>> >>      cpu->id_dfr0 = 0x02010555;
>> >> +    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
>> >
>> > These are:
>> >  SW_INCR   # insn architecturally executed, cc pass, software increment
>> >  INST_RETIRED # insn architecturally executed
>> >  CPU_CYCLES # cycle
>> >
>> > However we don't actually implement any of these, so should
>> > we be advertising them?
>>
>> So this part I took directly from Chris's RFC. I'm happy to take it
>> out if you would like.
>
> I think removing the PMCEID0 change makes sense since these patches
> don't implement the advertised counters. We have other patches which do
> implement them, but they need some more work, so we can make this change
> if/when they're actually implemented.

I agree, so I propose to take Alistair's v3 series into target-arm.next
with the following change:

diff --git a/target-arm/cpu.c b/target-arm/cpu.c
index 1203783..e95b030 100644
--- a/target-arm/cpu.c
+++ b/target-arm/cpu.c
@@ -1156,7 +1156,7 @@ static void cortex_a15_initfn(Object *obj)
     cpu->id_pfr0 = 0x00001131;
     cpu->id_pfr1 = 0x00011011;
     cpu->id_dfr0 = 0x02010555;
-    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid0 = 0x0000000;
     cpu->pmceid1 = 0x00000000;
     cpu->id_afr0 = 0x00000000;
     cpu->id_mmfr0 = 0x10201105;
diff --git a/target-arm/cpu64.c b/target-arm/cpu64.c
index fc336e1..fa5eda2 100644
--- a/target-arm/cpu64.c
+++ b/target-arm/cpu64.c
@@ -135,7 +135,7 @@ static void aarch64_a57_initfn(Object *obj)
     cpu->id_isar5 = 0x00011121;
     cpu->id_aa64pfr0 = 0x00002222;
     cpu->id_aa64dfr0 = 0x10305106;
-    cpu->pmceid0 = 0x00000481; /* PMUv3 events 0x0, 0x8, and 0x11 */
+    cpu->pmceid0 = 0x00000000;
     cpu->pmceid1 = 0x00000000;
     cpu->id_aa64isar0 = 0x00011120;
     cpu->id_aa64mmfr0 = 0x00001124;

If anybody disagrees let me know; otherwise this will go into a pullreq
later this week.

thanks
-- PMM

[Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers

[Alistair Francis]

This patch adds the following registers including read and write functions:
PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Nathan Rossi <nathan@nathanrossi.com>
---

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

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index b8b3364..5c31c56 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
 #define GTIMER_SEC  3
 #define NUM_GTIMERS 4
 
+#define NUM_PMU_COUNTERS 4
+
 typedef struct {
     uint64_t raw_tcr;
     uint32_t mask;
@@ -300,6 +302,7 @@ typedef struct CPUARMState {
         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 c9_pmselr; /* perf monitor event counter selection */
         union { /* Memory attribute redirection */
             struct {
 #ifdef HOST_WORDS_BIGENDIAN
@@ -361,6 +364,9 @@ typedef struct CPUARMState {
             uint64_t tpidruro_ns;
             uint64_t tpidrro_el[1];
         };
+        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
+        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
+        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
         uint64_t c14_cntfrq; /* Counter Frequency register */
         uint64_t c14_cntkctl; /* Timer Control register */
         uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 66aa406..164853f 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env)
 
 #endif
 
+static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                           uint64_t value)
+{
+    env->cp15.c9_pmselr = value & 31;
+}
+
+static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                              const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);
+    }
+    return env->cp15.c14_pmevcntr[idx];
+}
+
+static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
+static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                             uint64_t value, const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_write_ignore(env, ri, value);
+    }
+    env->cp15.c14_pmevcntr[idx] = value;
+}
+
+static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
+                            uint64_t value)
+{
+    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
+}
+
+static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
+                               const uint8_t idx)
+{
+    if (idx >= NUM_PMU_COUNTERS) {
+        return arm_cp_read_zero(env, ri);
+    }
+    return env->cp15.c14_pmevtyper[idx];
+}
+
+static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
+{
+    if (!!(env->cp15.c9_pmselr & 31)) {
+        return env->cp15.c14_pmccfiltr;
+    }
+    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
+}
+
 static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
                             uint64_t value)
 {
@@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
      * We choose to RAZ/WI.
      */
     { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access },
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmselr_write,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) },
+    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
+      .writefn = pmselr_write, .resetvalue = 0 },
+    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
+      .readfn = pmxevcntr_read },
+    { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
+      .readfn = pmxevcntr_read },
+    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .readfn = pmxevtyper_read },
+    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
+      .access = PL0_RW, .type = ARM_CP_ALIAS,
+      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
+      .readfn = pmxevtyper_read },
 #ifndef CONFIG_USER_ONLY
     { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
       .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
@@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .type = ARM_CP_IO,
       .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
       .resetvalue = 0, },
-    { .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,
-      .raw_writefn = raw_write },
-    /* Unimplemented, RAZ/WI. */
-    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
-      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
-      .accessfn = pmreg_access },
     { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
       .access = PL0_R | PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers

[Peter Maydell]

On 6 February 2016 at 00:55, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> This patch adds the following registers including read and write functions:
> PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.
>
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Tested-by: Nathan Rossi <nathan@nathanrossi.com>

Is there any benefit to implementing all the type and counter
registers and the multiplex selection, when we don't actually
support any counters yet?

> ---
>
>  target-arm/cpu.h    |  6 ++++
>  target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 84 insertions(+), 11 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index b8b3364..5c31c56 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
>  #define GTIMER_SEC  3
>  #define NUM_GTIMERS 4
>
> +#define NUM_PMU_COUNTERS 4
> +
>  typedef struct {
>      uint64_t raw_tcr;
>      uint32_t mask;
> @@ -300,6 +302,7 @@ typedef struct CPUARMState {
>          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 c9_pmselr; /* perf monitor event counter selection */
>          union { /* Memory attribute redirection */
>              struct {
>  #ifdef HOST_WORDS_BIGENDIAN
> @@ -361,6 +364,9 @@ typedef struct CPUARMState {
>              uint64_t tpidruro_ns;
>              uint64_t tpidrro_el[1];
>          };
> +        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
> +        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
> +        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
>          uint64_t c14_cntfrq; /* Counter Frequency register */
>          uint64_t c14_cntkctl; /* Timer Control register */
>          uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 66aa406..164853f 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env)
>
>  #endif
>
> +static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    env->cp15.c9_pmselr = value & 31;
> +}
> +
> +static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
> +                              const uint8_t idx)
> +{
> +    if (idx >= NUM_PMU_COUNTERS) {
> +        return arm_cp_read_zero(env, ri);
> +    }
> +    return env->cp15.c14_pmevcntr[idx];
> +}
> +
> +static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
> +}
> +
> +static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                             uint64_t value, const uint8_t idx)
> +{
> +    if (idx >= NUM_PMU_COUNTERS) {
> +        return arm_cp_write_ignore(env, ri, value);

arm_cp_write_ignore() does nothing, as you might expect from the name,
so why call it?

You might mention that this is CONSTRAINED UNPREDICTABLE and we
are choosing to RAZ/WI.

> +    }
> +    env->cp15.c14_pmevcntr[idx] = value;
> +}
> +
> +static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                            uint64_t value)
> +{
> +    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
> +}
> +
> +static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
> +                               const uint8_t idx)
> +{
> +    if (idx >= NUM_PMU_COUNTERS) {
> +        return arm_cp_read_zero(env, ri);

You could just return 0.

> +    }
> +    return env->cp15.c14_pmevtyper[idx];
> +}
> +
> +static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +{
> +    if (!!(env->cp15.c9_pmselr & 31)) {
> +        return env->cp15.c14_pmccfiltr;
> +    }
> +    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
> +}
> +
>  static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
>  {
> @@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>       * We choose to RAZ/WI.
>       */
>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> -      .accessfn = pmreg_access },
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .accessfn = pmreg_access, .writefn = pmselr_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) },
> +    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
> +      .access = PL0_RW, .accessfn = pmreg_access,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
> +      .writefn = pmselr_write, .resetvalue = 0 },
> +    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
> +      .readfn = pmxevcntr_read },
> +    { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2,
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
> +      .readfn = pmxevcntr_read },
> +    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> +      .readfn = pmxevtyper_read },
> +    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
> +      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
> +      .readfn = pmxevtyper_read },

This isn't going to handle migration correctly, because there are
a whole pile of data values behind these registers.

It may be easiest to implement the PMEVTYPER<n>_EL0 and PMEVCNTR<n>_EL0
registers (which are PMUv3 only and provide un-multiplexed access to
the type and counter registers), gate them so they're present but not
guest-accessible on PMUv2 and below, and use them for migrating the
state. Then the X multiplexed registers can be simple CP_ALIASes as
you have them here.

>  #ifndef CONFIG_USER_ONLY
>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>        .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
> @@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .type = ARM_CP_IO,
>        .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
>        .resetvalue = 0, },
> -    { .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,
> -      .raw_writefn = raw_write },
> -    /* Unimplemented, RAZ/WI. */
> -    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> -      .accessfn = pmreg_access },
>      { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
>        .access = PL0_R | PL1_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> --
> 2.5.0
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 2/5] target-arm: Add Some of the performance monitor registers

[Alistair Francis]

On Tue, Feb 9, 2016 at 9:32 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 6 February 2016 at 00:55, Alistair Francis
> <alistair.francis@xilinx.com> wrote:
>> This patch adds the following registers including read and write functions:
>> PMSELR, PMSELR_EL0, PMXEVCNTR, PMXEVCNTR_EL0, PMXEVTYPER and PMXEVTYPER_EL0.
>>
>> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
>
> Is there any benefit to implementing all the type and counter
> registers and the multiplex selection, when we don't actually
> support any counters yet?

Thanks for reviewing these. The others (patch 1, 3 and 4) are more
important to me, so I'm sending them out so they can be applied then
I'll look at these after.

Thanks,

Alistair

>
>> ---
>>
>>  target-arm/cpu.h    |  6 ++++
>>  target-arm/helper.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 84 insertions(+), 11 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index b8b3364..5c31c56 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -117,6 +117,8 @@ typedef struct ARMGenericTimer {
>>  #define GTIMER_SEC  3
>>  #define NUM_GTIMERS 4
>>
>> +#define NUM_PMU_COUNTERS 4
>> +
>>  typedef struct {
>>      uint64_t raw_tcr;
>>      uint32_t mask;
>> @@ -300,6 +302,7 @@ typedef struct CPUARMState {
>>          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 c9_pmselr; /* perf monitor event counter selection */
>>          union { /* Memory attribute redirection */
>>              struct {
>>  #ifdef HOST_WORDS_BIGENDIAN
>> @@ -361,6 +364,9 @@ typedef struct CPUARMState {
>>              uint64_t tpidruro_ns;
>>              uint64_t tpidrro_el[1];
>>          };
>> +        uint32_t c14_pmccfiltr; /* Performance Monitor Filter Register */
>> +        uint32_t c14_pmevcntr[NUM_PMU_COUNTERS];
>> +        uint32_t c14_pmevtyper[NUM_PMU_COUNTERS];
>>          uint64_t c14_cntfrq; /* Counter Frequency register */
>>          uint64_t c14_cntkctl; /* Timer Control register */
>>          uint32_t cnthctl_el2; /* Counter/Timer Hyp Control register */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 66aa406..164853f 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -807,6 +807,58 @@ void pmccntr_sync(CPUARMState *env)
>>
>>  #endif
>>
>> +static void pmselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                           uint64_t value)
>> +{
>> +    env->cp15.c9_pmselr = value & 31;
>> +}
>> +
>> +static uint64_t pmevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                              const uint8_t idx)
>> +{
>> +    if (idx >= NUM_PMU_COUNTERS) {
>> +        return arm_cp_read_zero(env, ri);
>> +    }
>> +    return env->cp15.c14_pmevcntr[idx];
>> +}
>> +
>> +static uint64_t pmxevcntr_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    return pmevcntr_read(env, ri, env->cp15.c9_pmselr & 31);
>> +}
>> +
>> +static void pmevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                             uint64_t value, const uint8_t idx)
>> +{
>> +    if (idx >= NUM_PMU_COUNTERS) {
>> +        return arm_cp_write_ignore(env, ri, value);
>
> arm_cp_write_ignore() does nothing, as you might expect from the name,
> so why call it?
>
> You might mention that this is CONSTRAINED UNPREDICTABLE and we
> are choosing to RAZ/WI.
>
>> +    }
>> +    env->cp15.c14_pmevcntr[idx] = value;
>> +}
>> +
>> +static void pmxevcntr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                            uint64_t value)
>> +{
>> +    pmevcntr_write(env, ri, value, env->cp15.c9_pmselr & 31);
>> +}
>> +
>> +static uint64_t pmevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri,
>> +                               const uint8_t idx)
>> +{
>> +    if (idx >= NUM_PMU_COUNTERS) {
>> +        return arm_cp_read_zero(env, ri);
>
> You could just return 0.
>
>> +    }
>> +    return env->cp15.c14_pmevtyper[idx];
>> +}
>> +
>> +static uint64_t pmxevtyper_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> +{
>> +    if (!!(env->cp15.c9_pmselr & 31)) {
>> +        return env->cp15.c14_pmccfiltr;
>> +    }
>> +    return pmevtyper_read(env, ri, env->cp15.c9_pmselr & 31);
>> +}
>> +
>>  static void pmccfiltr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>                              uint64_t value)
>>  {
>> @@ -986,8 +1038,32 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>       * We choose to RAZ/WI.
>>       */
>>      { .name = "PMSELR", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 5,
>> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
>> -      .accessfn = pmreg_access },
>> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>> +      .accessfn = pmreg_access, .writefn = pmselr_write,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr) },
>> +    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 5,
>> +      .access = PL0_RW, .accessfn = pmreg_access,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmselr),
>> +      .writefn = pmselr_write, .resetvalue = 0 },
>> +    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
>> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>> +      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
>> +      .readfn = pmxevcntr_read },
>> +    { .name = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 2,
>> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>> +      .accessfn = pmreg_access, .writefn = pmxevcntr_write,
>> +      .readfn = pmxevcntr_read },
>> +    { .name = "PMXEVTYPER", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 1,
>> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>> +      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
>> +      .readfn = pmxevtyper_read },
>> +    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 13, .opc2 = 1,
>> +      .access = PL0_RW, .type = ARM_CP_ALIAS,
>> +      .accessfn = pmreg_access, .writefn = pmxevtyper_write,
>> +      .readfn = pmxevtyper_read },
>
> This isn't going to handle migration correctly, because there are
> a whole pile of data values behind these registers.
>
> It may be easiest to implement the PMEVTYPER<n>_EL0 and PMEVCNTR<n>_EL0
> registers (which are PMUv3 only and provide un-multiplexed access to
> the type and counter registers), gate them so they're present but not
> guest-accessible on PMUv2 and below, and use them for migrating the
> state. Then the X multiplexed registers can be simple CP_ALIASes as
> you have them here.
>
>>  #ifndef CONFIG_USER_ONLY
>>      { .name = "PMCCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 0,
>>        .access = PL0_RW, .resetvalue = 0, .type = ARM_CP_IO,
>> @@ -1006,15 +1082,6 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>>        .type = ARM_CP_IO,
>>        .fieldoffset = offsetof(CPUARMState, cp15.pmccfiltr_el0),
>>        .resetvalue = 0, },
>> -    { .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,
>> -      .raw_writefn = raw_write },
>> -    /* Unimplemented, RAZ/WI. */
>> -    { .name = "PMXEVCNTR", .cp = 15, .crn = 9, .crm = 13, .opc1 = 0, .opc2 = 2,
>> -      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
>> -      .accessfn = pmreg_access },
>>      { .name = "PMUSERENR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 0,
>>        .access = PL0_R | PL1_RW,
>>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>> --
>> 2.5.0
>>
>
> thanks
> -- PMM
>

[Qemu-devel] [PATCH v2 3/5] target-arm: Add the pmovsclr_el0 and pmintenclr_el1 registers

[Alistair Francis]

Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Tested-by: Nathan Rossi <nathan@nathanrossi.com>
---

 target-arm/helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 164853f..8d401c6 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1031,6 +1031,13 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .accessfn = pmreg_access,
       .writefn = pmovsr_write,
       .raw_writefn = raw_write },
+    { .name = "PMOVSCLR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 3,
+      .access = PL0_RW, .accessfn = pmreg_access,
+      .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .writefn = pmovsr_write,
+      .raw_writefn = raw_write },
     /* Unimplemented so WI. */
     { .name = "PMSWINC", .cp = 15, .crn = 9, .crm = 12, .opc1 = 0, .opc2 = 4,
       .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
@@ -1096,6 +1103,11 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .access = PL1_RW, .type = ARM_CP_ALIAS,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .writefn = pmintenclr_write, },
+    { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 2,
+      .access = PL1_RW, .type = ARM_CP_ALIAS,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .writefn = pmintenclr_write },
     { .name = "VBAR", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .crn = 12, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL1_RW, .writefn = vbar_write,
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 3/5] target-arm: Add the pmovsclr_el0 and pmintenclr_el1 registers

[Peter Maydell]

On 6 February 2016 at 00:55, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> Tested-by: Nathan Rossi <nathan@nathanrossi.com>
> ---
>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v2 4/5] target-arm: Add PMUSERENR_EL0 register

[Alistair Francis]

The Linux kernel accesses this register early in its setup.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8d401c6..b4bf6fa 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1094,6 +1094,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
+    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
+      .access = PL0_R | PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
+      .resetvalue = 0,
+      .writefn = pmuserenr_write, .raw_writefn = raw_write },
     { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
       .access = PL1_RW,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 4/5] target-arm: Add PMUSERENR_EL0 register

[Peter Maydell]

On 6 February 2016 at 00:55, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> The Linux kernel accesses this register early in its setup.
>
> Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8d401c6..b4bf6fa 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1094,6 +1094,12 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
> +    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 14, .opc2 = 0,
> +      .access = PL0_R | PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> +      .resetvalue = 0,
> +      .writefn = pmuserenr_write, .raw_writefn = raw_write },

Either this or the 32-bit version need to be marked ARM_CP_ALIAS.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[Qemu-devel] [PATCH v2 5/5] target-arm: Unmask PMU bits in debug feature register

[Alistair Francis]

The previously missing registers are now present in QEMU.

Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 target-arm/helper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b4bf6fa..1775768 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -4267,12 +4267,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
             { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
               .access = PL1_R, .type = ARM_CP_CONST,
-              /* We mask out the PMUVer field, because we don't currently
-               * implement the PMU. Not advertising it prevents the guest
-               * from trying to use it and getting UNDEFs on registers we
-               * don't implement.
-               */
-              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
+              .resetvalue = cpu->id_aa64dfr0 },
             { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
               .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
               .access = PL1_R, .type = ARM_CP_CONST,
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 5/5] target-arm: Unmask PMU bits in debug feature register

[Peter Maydell]

On 6 February 2016 at 00:55, Alistair Francis
<alistair.francis@xilinx.com> wrote:
> The previously missing registers are now present in QEMU.

I think we're still missing some:

32 bit version present, 64 bit version missing:
PMSWINC
PMINTENSET

not implemented in 32 bit either:
PMOVSCLR (I think this is only in PMUv3 so needs care as to
          which CPUs we define it on)

> Signed-off-by: Christopher Covington <christopher.covington@linaro.org>
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
>
>  target-arm/helper.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b4bf6fa..1775768 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -4267,12 +4267,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>              { .name = "ID_AA64DFR0_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
>                .access = PL1_R, .type = ARM_CP_CONST,
> -              /* We mask out the PMUVer field, because we don't currently
> -               * implement the PMU. Not advertising it prevents the guest
> -               * from trying to use it and getting UNDEFs on registers we
> -               * don't implement.
> -               */
> -              .resetvalue = cpu->id_aa64dfr0 & ~0xf00 },
> +              .resetvalue = cpu->id_aa64dfr0 },
>              { .name = "ID_AA64DFR1_EL1", .state = ARM_CP_STATE_AA64,
>                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 1,
>                .access = PL1_R, .type = ARM_CP_CONST,
> --
> 2.5.0
>

thanks
-- PMM