| Message ID | 20181205134243.4791-9-aaron@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
On Dec 05 08:43, Aaron Lindsay wrote: > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > --- > target/arm/cpu.h | 4 ++-- > target/arm/helper.c | 18 ++++++++++++++++-- > 2 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 304e6e47b3..4216fe22db 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -837,8 +837,8 @@ struct ARMCPU { > uint32_t id_pfr0; > uint32_t id_pfr1; > uint32_t id_dfr0; > - uint32_t pmceid0; > - uint32_t pmceid1; > + uint64_t pmceid0; > + uint64_t pmceid1; > uint32_t id_afr0; > uint32_t id_mmfr0; > uint32_t id_mmfr1; > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 71be6fb578..fb6939e99c 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5256,6 +5256,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) > } else { > define_arm_cp_regs(cpu, not_v7_cp_reginfo); > } > + if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) { After further discussion on my last version, this should be if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4 && FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) != 0xf) { to guard against defining these registers for implementation-defined PMUs. > + ARMCPRegInfo v81_pmu_regs[] = { > + { .name = "PMCEID2", .state = ARM_CP_STATE_AA32, > + .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 4, > + .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > + .resetvalue = extract64(cpu->pmceid0, 32, 32) }, > + { .name = "PMCEID3", .state = ARM_CP_STATE_AA32, > + .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 5, > + .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > + .resetvalue = extract64(cpu->pmceid1, 32, 32) }, > + REGINFO_SENTINEL > + }; > + define_arm_cp_regs(cpu, v81_pmu_regs); > + } > if (arm_feature(env, ARM_FEATURE_V8)) { > /* AArch64 ID registers, which all have impdef reset values. > * Note that within the ID register ranges the unused slots > @@ -5432,7 +5446,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "PMCEID0", .state = ARM_CP_STATE_AA32, > .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 6, > .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > - .resetvalue = cpu->pmceid0 }, > + .resetvalue = extract64(cpu->pmceid0, 0, 32) }, > { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6, > .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > @@ -5440,7 +5454,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) > { .name = "PMCEID1", .state = ARM_CP_STATE_AA32, > .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 7, > .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > - .resetvalue = cpu->pmceid1 }, > + .resetvalue = extract64(cpu->pmceid1, 0, 32) }, > { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64, > .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7, > .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, > -- > 2.19.1 >
On Wed, 5 Dec 2018 at 15:32, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > On Dec 05 08:43, Aaron Lindsay wrote: > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > > --- > > target/arm/cpu.h | 4 ++-- > > target/arm/helper.c | 18 ++++++++++++++++-- > > 2 files changed, 18 insertions(+), 4 deletions(-) > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 304e6e47b3..4216fe22db 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -837,8 +837,8 @@ struct ARMCPU { > > uint32_t id_pfr0; > > uint32_t id_pfr1; > > uint32_t id_dfr0; > > - uint32_t pmceid0; > > - uint32_t pmceid1; > > + uint64_t pmceid0; > > + uint64_t pmceid1; > > uint32_t id_afr0; > > uint32_t id_mmfr0; > > uint32_t id_mmfr1; > > diff --git a/target/arm/helper.c b/target/arm/helper.c > > index 71be6fb578..fb6939e99c 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -5256,6 +5256,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) > > } else { > > define_arm_cp_regs(cpu, not_v7_cp_reginfo); > > } > > + if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) { > > After further discussion on my last version, this should be > > if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4 && > FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) != 0xf) { > > to guard against defining these registers for implementation-defined > PMUs. With that change Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 12/5/18 9:32 AM, Aaron Lindsay wrote: > On Dec 05 08:43, Aaron Lindsay wrote: >> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> >> --- >> target/arm/cpu.h | 4 ++-- >> target/arm/helper.c | 18 ++++++++++++++++-- >> 2 files changed, 18 insertions(+), 4 deletions(-) >> >> diff --git a/target/arm/cpu.h b/target/arm/cpu.h >> index 304e6e47b3..4216fe22db 100644 >> --- a/target/arm/cpu.h >> +++ b/target/arm/cpu.h >> @@ -837,8 +837,8 @@ struct ARMCPU { >> uint32_t id_pfr0; >> uint32_t id_pfr1; >> uint32_t id_dfr0; >> - uint32_t pmceid0; >> - uint32_t pmceid1; >> + uint64_t pmceid0; >> + uint64_t pmceid1; >> uint32_t id_afr0; >> uint32_t id_mmfr0; >> uint32_t id_mmfr1; >> diff --git a/target/arm/helper.c b/target/arm/helper.c >> index 71be6fb578..fb6939e99c 100644 >> --- a/target/arm/helper.c >> +++ b/target/arm/helper.c >> @@ -5256,6 +5256,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) >> } else { >> define_arm_cp_regs(cpu, not_v7_cp_reginfo); >> } >> + if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) { > > After further discussion on my last version, this should be > > if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4 && > FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) != 0xf) { > > to guard against defining these registers for implementation-defined > PMUs. When id fields define values like 0b1111, that is a hint that the field should be interpreted as signed, and you should still use a >= comparison. (See D12.1.4, Principles of the ID scheme for fields in ID registers.) A patch adding #define FIELD_EX32S(storage, reg, field) \ sextract32((storage), R_ ## reg ## _ ## field ## _SHIFT, \ R_ ## reg ## _ ## field ## _LENGTH) #define FIELD_EX64S(storage, reg, field) \ sextract64((storage), R_ ## reg ## _ ## field ## _SHIFT, \ R_ ## reg ## _ ## field ## _LENGTH) to include/hw/registerfields.h would be welcome and appropriate, I think. r~
On Fri, 7 Dec 2018 at 18:00, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 12/5/18 9:32 AM, Aaron Lindsay wrote: > > On Dec 05 08:43, Aaron Lindsay wrote: > >> Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > >> + if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) { > > > > After further discussion on my last version, this should be > > > > if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4 && > > FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) != 0xf) { > > > > to guard against defining these registers for implementation-defined > > PMUs. > > When id fields define values like 0b1111, that is a hint that the field should > be interpreted as signed, and you should still use a >= comparison. (See > D12.1.4, Principles of the ID scheme for fields in ID registers.) That section calls out these PMU ID registers as exceptions which do not follow the scheme and specifically notes that the "not 0xf and greater than or equal to 4" is the kind of comparison required here... thanks -- PMM
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 304e6e47b3..4216fe22db 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -837,8 +837,8 @@ struct ARMCPU { uint32_t id_pfr0; uint32_t id_pfr1; uint32_t id_dfr0; - uint32_t pmceid0; - uint32_t pmceid1; + uint64_t pmceid0; + uint64_t pmceid1; uint32_t id_afr0; uint32_t id_mmfr0; uint32_t id_mmfr1; diff --git a/target/arm/helper.c b/target/arm/helper.c index 71be6fb578..fb6939e99c 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5256,6 +5256,20 @@ void register_cp_regs_for_features(ARMCPU *cpu) } else { define_arm_cp_regs(cpu, not_v7_cp_reginfo); } + if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) { + ARMCPRegInfo v81_pmu_regs[] = { + { .name = "PMCEID2", .state = ARM_CP_STATE_AA32, + .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 4, + .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, + .resetvalue = extract64(cpu->pmceid0, 32, 32) }, + { .name = "PMCEID3", .state = ARM_CP_STATE_AA32, + .cp = 15, .opc1 = 0, .crn = 9, .crm = 14, .opc2 = 5, + .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, + .resetvalue = extract64(cpu->pmceid1, 32, 32) }, + REGINFO_SENTINEL + }; + define_arm_cp_regs(cpu, v81_pmu_regs); + } if (arm_feature(env, ARM_FEATURE_V8)) { /* AArch64 ID registers, which all have impdef reset values. * Note that within the ID register ranges the unused slots @@ -5432,7 +5446,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "PMCEID0", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 6, .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, - .resetvalue = cpu->pmceid0 }, + .resetvalue = extract64(cpu->pmceid0, 0, 32) }, { .name = "PMCEID0_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 6, .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, @@ -5440,7 +5454,7 @@ void register_cp_regs_for_features(ARMCPU *cpu) { .name = "PMCEID1", .state = ARM_CP_STATE_AA32, .cp = 15, .opc1 = 0, .crn = 9, .crm = 12, .opc2 = 7, .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST, - .resetvalue = cpu->pmceid1 }, + .resetvalue = extract64(cpu->pmceid1, 0, 32) }, { .name = "PMCEID1_EL0", .state = ARM_CP_STATE_AA64, .opc0 = 3, .opc1 = 3, .crn = 9, .crm = 12, .opc2 = 7, .access = PL0_R, .accessfn = pmreg_access, .type = ARM_CP_CONST,
Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> --- target/arm/cpu.h | 4 ++-- target/arm/helper.c | 18 ++++++++++++++++-- 2 files changed, 18 insertions(+), 4 deletions(-)