Re: [Qemu-devel] [PATCH v2 2/7] target-arm: Implement PMCCNTR_EL0 and related registers

[Peter Crosthwaite]

On Sat, Aug 2, 2014 at 1:28 AM, Peter Maydell <address@hidden> wrote:
> 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().
>

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
>