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 <address@hidden> 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 <address@hidden>
> Signed-off-by: Alistair Francis <address@hidden>
> ---
>
>  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