[v2,2/7] target-arm: Implement PMCCNTR_EL0 and related registers

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(-)

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

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
>

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