[Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Chengyu Song]

In AA64 mode, certain system registers are access through MSR/MRS 
instructions instead of MCR/MRC. This patch added more such registers:

/* ARMv8 manual, D8.4.10 */
PMINTENCLR_EL1

/* ARMv8 manual, D8.4.11 */
PMINTENSET_EL1

/* ARMv8 manual, D8.4.12 */
PMOVSCLR_EL0

/* ARMv8 manual, D8.4.14 */
PMSELR_EL0

/* ARMv8 manual, D8.4.15 */
PMSWINC_EL0

/* ARMv8 manual, D8.4.16 */
PMUSERENR_EL0

/* ARMv8 manual, D8.4.17 */
PMXEVCNTR_EL0

/* ARMv8 manual, D8.4.18 */
PMXEVTYPER_EL0

Signed-off-by: Chengyu Song <csong84@gatech.edu>
---
 target-arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index b74d348..43b5b06 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -843,15 +843,28 @@ 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, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 3,
+      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
+      .accessfn = pmreg_access,
+      .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 },
+    { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 4,
+      .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
     /* Since we don't implement any events, writing to PMSELR is UNPREDICTABLE.
      * 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 },
+    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 5,
+      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
+      .accessfn = pmreg_access },
 #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,
@@ -875,24 +888,51 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
       .accessfn = pmreg_access, .writefn = pmxevtyper_write,
       .raw_writefn = raw_write },
+    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 13, .opc1 = 3, .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 = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 13, .opc1 = 3, .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),
       .resetvalue = 0,
       .writefn = pmuserenr_write, .raw_writefn = raw_write },
+    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 3, .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),
       .resetvalue = 0,
       .writefn = pmintenset_write, .raw_writefn = raw_write },
+    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
+      .access = PL1_RW,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .resetvalue = 0,
+      .writefn = pmintenset_write, .raw_writefn = raw_write },
     { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
       .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
       .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
       .resetvalue = 0, .writefn = pmintenclr_write, },
+    { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
+      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
+      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
+      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
+      .resetvalue = 0, .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.1.0

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Christopher Covington]

Hi Chengyu,

On 12/03/2014 02:12 AM, Chengyu Song wrote:
> In AA64 mode, certain system registers are access through MSR/MRS 
> instructions instead of MCR/MRC. This patch added more such registers:

If you don't mind sharing, I'm curious what motivated this patch. I have a
particular interest in having `perf stat -e instructions:u echo` inside
qemu-system-aarch64 function correctly, and this looks like a good step in
that direction.

> /* ARMv8 manual, D8.4.10 */
> PMINTENCLR_EL1

It'd probably good to mention the version of the document.

> /* ARMv8 manual, D8.4.11 */
> PMINTENSET_EL1
> 
> /* ARMv8 manual, D8.4.12 */
> PMOVSCLR_EL0
> 
> /* ARMv8 manual, D8.4.14 */
> PMSELR_EL0
> 
> /* ARMv8 manual, D8.4.15 */
> PMSWINC_EL0
> 
> /* ARMv8 manual, D8.4.16 */
> PMUSERENR_EL0
> 
> /* ARMv8 manual, D8.4.17 */
> PMXEVCNTR_EL0
> 
> /* ARMv8 manual, D8.4.18 */
> PMXEVTYPER_EL0
> 
> Signed-off-by: Chengyu Song <csong84@gatech.edu>
> ---
>  target-arm/helper.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index b74d348..43b5b06 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -843,15 +843,28 @@ 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, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 3,

It might be helpful to order the fields opc0, opc1, crn, crm, opc2 to match
the documentation and some (most?) of the other A64 QEMU code.

> +      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
> +      .accessfn = pmreg_access,
> +      .writefn = pmovsr_write,
> +      .raw_writefn = raw_write },

Should this contain .type = ARM_CP_NO_MIGRATE, if PMOVSCLR_EL0, PMOVSSET_EL0,
and PMOVSR all refer to the same underlying variable?

Should PMOVSSET_EL0 also be added?

>      /* 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 },
> +    { .name = "PMSWINC_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 4,
> +      .access = PL0_W, .accessfn = pmreg_access, .type = ARM_CP_NOP },
>      /* Since we don't implement any events, writing to PMSELR is UNPREDICTABLE.
>       * 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 },
> +    { .name = "PMSELR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 5,
> +      .access = PL0_RW, .type = ARM_CP_CONST, .resetvalue = 0,
> +      .accessfn = pmreg_access },
>  #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,
> @@ -875,24 +888,51 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pmxevtyper),
>        .accessfn = pmreg_access, .writefn = pmxevtyper_write,
>        .raw_writefn = raw_write },
> +    { .name = "PMXEVTYPER_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 13, .opc1 = 3, .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 = "PMXEVCNTR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 13, .opc1 = 3, .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),
>        .resetvalue = 0,
>        .writefn = pmuserenr_write, .raw_writefn = raw_write },
> +    { .name = "PMUSERENR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 3, .opc2 = 0,
> +      .access = PL0_R | PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pmuserenr),
> +      .resetvalue = 0,
> +      .writefn = pmuserenr_write, .raw_writefn = raw_write },

Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
PMUSERENR?

>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
>        .access = PL1_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0,
>        .writefn = pmintenset_write, .raw_writefn = raw_write },
> +    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
> +      .access = PL1_RW,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> +      .resetvalue = 0,
> +      .writefn = pmintenset_write, .raw_writefn = raw_write },

Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
PMINTSET?

>      { .name = "PMINTENCLR", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
>        .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>        .resetvalue = 0, .writefn = pmintenclr_write, },
> +    { .name = "PMINTENCLR_EL1", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 2,
> +      .access = PL1_RW, .type = ARM_CP_NO_MIGRATE,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
> +      .resetvalue = 0, .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,

Thanks,
Chris

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

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Peter Maydell]

On 3 December 2014 at 20:25, Christopher Covington <cov@codeaurora.org> wrote:
> Hi Chengyu,
>
> On 12/03/2014 02:12 AM, Chengyu Song wrote:

>>      { .name = "PMINTENSET", .cp = 15, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
>>        .access = PL1_RW,
>>        .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>>        .resetvalue = 0,
>>        .writefn = pmintenset_write, .raw_writefn = raw_write },
>> +    { .name = "PMINTENSET_EL1", .state = ARM_CP_STATE_AA64,
>> +      .opc0 = 3, .crn = 9, .crm = 14, .opc1 = 0, .opc2 = 1,
>> +      .access = PL1_RW,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_pminten),
>> +      .resetvalue = 0,
>> +      .writefn = pmintenset_write, .raw_writefn = raw_write },
>
> Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
> PMINTSET?

I think in this case and PMINTENCLR you can actually share a
single STATE_BOTH regdef, since the crn/crm/opc encodings
line up. Generally we prefer to do that where it works out.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Chengyu Song]

Hi Christopher,

> On Dec 3, 2014, at 3:25 PM, Christopher Covington <cov@codeaurora.org> wrote:
> On 12/03/2014 02:12 AM, Chengyu Song wrote:
>> In AA64 mode, certain system registers are access through MSR/MRS 
>> instructions instead of MCR/MRC. This patch added more such registers:
> 
> If you don't mind sharing, I'm curious what motivated this patch. I have a
> particular interest in having `perf stat -e instructions:u echo` inside
> qemu-system-aarch64 function correctly, and this looks like a good step in
> that direction.

I'm playing with the Nexus 9 kernel and it died with a undef instruction exception caused by those MSR/MRS instructions.

>> /* ARMv8 manual, D8.4.10 */
>> PMINTENCLR_EL1
> 
> It'd probably good to mention the version of the document.

The version I use is A.a Non-Confidential Beta, release on 04 September 2013. I don't have any later version.

>> +      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 3,
> 
> It might be helpful to order the fields opc0, opc1, crn, crm, opc2 to match
> the documentation and some (most?) of the other A64 QEMU code.

Good point. Let me try to resubmit the patch.

> 
>> +      .access = PL0_RW, .fieldoffset = offsetof(CPUARMState, cp15.c9_pmovsr),
>> +      .accessfn = pmreg_access,
>> +      .writefn = pmovsr_write,
>> +      .raw_writefn = raw_write },
> 
> Should this contain .type = ARM_CP_NO_MIGRATE, if PMOVSCLR_EL0, PMOVSSET_EL0,
> and PMOVSR all refer to the same underlying variable?

I don't know. I'm not an expert here, so I simply copied from the AA32 version. It would be nicer if you can create a correct patch :)

> Should PMOVSSET_EL0 also be added?

PMOVSSET is not available in QEMU, so I'm not sure how it should be added

> Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
> PMUSERENR?

Same as above.

> Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
> PMINTSET?

Same as above.

BTW, I think the current mask (0x7E000000) in the pmccfiltr_write function is not right.

Thanks,
Chengyu

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Peter Maydell]

On 3 December 2014 at 20:53, Chengyu Song <csong84@gatech.edu> wrote:
> The version I use is A.a Non-Confidential Beta, release on 04 September
> 2013. I don't have any later version.

We're up to rev A.d now; you can get it from
http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.d/index.html
(if you haven't registered on the ARM website before you'll need
to fill in a form but it's a fairly painless process.)

>>> +      .opc0 = 3, .crn = 9, .crm = 12, .opc1 = 3, .opc2 = 3,
>>
>> It might be helpful to order the fields opc0, opc1, crn, crm, opc2 to match
>> the documentation and some (most?) of the other A64 QEMU code.
>
> Good point. Let me try to resubmit the patch.

The order we're preferring is the v8 ARM ARM order, so
that's .opc0, .opc1, .crn, .crm, .opc2. (Some of our existing
code doesn't follow that, unfortunately.)

>> Should PMOVSSET_EL0 also be added?
>
> PMOVSSET is not available in QEMU, so I'm not sure how it should be added

By implementing the register's behaviour; there's only one
bit of state we care about since we don't model any
perf counters except the cycle counter. The overflow bit
is handled by a set/clear pair of register accessors,
and we also need to check on reads to see if the
cycle counter would have overflowed (in which case we
need to set the bit to 1), using a similar logic to how
we deal with reads of the cycle counter itself. (A similar
check also needs to be done when the cycle counter is
written to or when it is disabled.)

Are there any other bits of new-in-v8 perf counter
functionality we're missing?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Chengyu Song]

Hello Peter,

Sorry for replying late.

> We're up to rev A.d now; you can get it from
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0487a.d/index.html
> (if you haven't registered on the ARM website before you'll need
> to fill in a form but it's a fairly painless process.)

Thank you for the information, I have resubmitted the patch according to the new revision.

> By implementing the register's behaviour; there's only one
> bit of state we care about since we don't model any
> perf counters except the cycle counter. The overflow bit
> is handled by a set/clear pair of register accessors,
> and we also need to check on reads to see if the
> cycle counter would have overflowed (in which case we
> need to set the bit to 1), using a similar logic to how
> we deal with reads of the cycle counter itself. (A similar
> check also needs to be done when the cycle counter is
> written to or when it is disabled.)

Thank you for the instruction, I have added the support for PMOVSSET.

> Are there any other bits of new-in-v8 perf counter
> functionality we're missing?

Yes. But the rest ones are all related to events:

PMCEID0_EL0, PMCEID1_EL0, PMEVCNTR<n>_EL0, PMEVTYPER<n>_EL0, PMXEVCNTR_EL0

and their corresponding AA32 mappings.

Best,
Chengyu

Re: [Qemu-devel] [PATCH] ARM64: support access to more performance registers in AA64 mode

[Chengyu Song]

Hello Christopher,

>> Again, should .type = ARM_CP_NO_MIGRATE be used as this shares a variable with
>> PMUSERENR?
> 

I went back and re-read the code and the manual, but I'm not sure if PMUSERENR should be NO_MIGRATE or not. It's only been read/written with this register, but it also controls EL0 access (i.e., used in pmreg_access). Should we still add the NO_MIGRATE type to it?

Thanks,
Chengyu