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 <address@hidden> wrote:
> On 6 February 2016 at 00:55, Alistair Francis
> <address@hidden> wrote:
>> Signed-off-by: Aaron Lindsay <address@hidden>
>> Signed-off-by: Alistair Francis <address@hidden>
>> Tested-by: Nathan Rossi <address@hidden>
>> ---
>>
>>  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
>