On Sat, Aug 2, 2014 at 1:28 AM, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 26 June 2014 06:02, Alistair Francis <alistair.fran...@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.crosthwa...@xilinx.com>
>> Signed-off-by: Alistair Francis <alistair.fran...@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
>
