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

[Aaron Lindsay <alindsay@codeaurora.org>]

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