| Message ID | 20181120212553.8480-8-aaron@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
On Tue, 20 Nov 2018 at 21:26, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > --- > target/arm/cpu.h | 4 ++-- > target/arm/helper.c | 12 ++++++++++-- > 2 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 627e5c1995..50de58e4a2 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..75f054fe79 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -5432,7 +5432,11 @@ 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 = "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 = "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 +5444,11 @@ 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 = "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) }, > { .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, > -- PMCEID2 and PMCEID3 are only defined from ARMv8.1; before that they are UNDEFINED. So these registers need to be only defined if a suitable feature bit or ID register field check passes. thanks -- PMM
On Nov 30 16:10, Peter Maydell wrote: > On Tue, 20 Nov 2018 at 21:26, Aaron Lindsay > <aaron@os.amperecomputing.com> wrote: > > > > Signed-off-by: Aaron Lindsay <aaron@os.amperecomputing.com> > > --- > > target/arm/cpu.h | 4 ++-- > > target/arm/helper.c | 12 ++++++++++-- > > 2 files changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 627e5c1995..50de58e4a2 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..75f054fe79 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -5432,7 +5432,11 @@ 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 = "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 = "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 +5444,11 @@ 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 = "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) }, > > { .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, > > -- > > PMCEID2 and PMCEID3 are only defined from ARMv8.1; before that they > are UNDEFINED. So these registers need to be only defined if a > suitable feature bit or ID register field check passes. It looks like we don't currently support any ARMv8.1+ CPUs and don't have an entry in the `arm_features` enum for it. I'll plan to add ARM_FEATURE_V81 and make defining these registers depend on it, assuming any future CPUs supporting it will use that, unless you feel I should do something different. -Aaron
On Mon, 3 Dec 2018 at 20:45, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > On Nov 30 16:10, Peter Maydell wrote: > > PMCEID2 and PMCEID3 are only defined from ARMv8.1; before that they > > are UNDEFINED. So these registers need to be only defined if a > > suitable feature bit or ID register field check passes. > > It looks like we don't currently support any ARMv8.1+ CPUs and don't > have an entry in the `arm_features` enum for it. I'll plan to add > ARM_FEATURE_V81 and make defining these registers depend on it, assuming > any future CPUs supporting it will use that, unless you feel I should do > something different. I think that the idea going forward is to prefer an ID register check of some kind -- Richard ? thanks -- PMM
On 12/3/18 4:19 PM, Peter Maydell wrote: > On Mon, 3 Dec 2018 at 20:45, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: >> >> On Nov 30 16:10, Peter Maydell wrote: >>> PMCEID2 and PMCEID3 are only defined from ARMv8.1; before that they >>> are UNDEFINED. So these registers need to be only defined if a >>> suitable feature bit or ID register field check passes. >> >> It looks like we don't currently support any ARMv8.1+ CPUs and don't >> have an entry in the `arm_features` enum for it. I'll plan to add >> ARM_FEATURE_V81 and make defining these registers depend on it, assuming >> any future CPUs supporting it will use that, unless you feel I should do >> something different. > > I think that the idea going forward is to prefer an ID > register check of some kind -- Richard ? Yes. It would appear that this feature should be controlled by ID_DFR0.PerfMon. So, if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) once the appropriate FIELDs are added to cpu.h. Since this test is not used within translate*.c, there is no need to move id_dfr* into ARMISARegisters. Since these are only aliases, they do not affect migration, and so do not (yet) need to be filled in by kvm. r~
On Dec 03 16:57, Richard Henderson wrote: > On 12/3/18 4:19 PM, Peter Maydell wrote: > > On Mon, 3 Dec 2018 at 20:45, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > >> > >> On Nov 30 16:10, Peter Maydell wrote: > >>> PMCEID2 and PMCEID3 are only defined from ARMv8.1; before that they > >>> are UNDEFINED. So these registers need to be only defined if a > >>> suitable feature bit or ID register field check passes. > >> > >> It looks like we don't currently support any ARMv8.1+ CPUs and don't > >> have an entry in the `arm_features` enum for it. I'll plan to add > >> ARM_FEATURE_V81 and make defining these registers depend on it, assuming > >> any future CPUs supporting it will use that, unless you feel I should do > >> something different. > > > > I think that the idea going forward is to prefer an ID > > register check of some kind -- Richard ? > > Yes. It would appear that this feature should be controlled by > ID_DFR0.PerfMon. So, > > if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) > > once the appropriate FIELDs are added to cpu.h. > > Since this test is not used within translate*.c, there is no need to move > id_dfr* into ARMISARegisters. Since these are only aliases, they do not affect > migration, and so do not (yet) need to be filled in by kvm. Sounds reasonable to me. One clarification - do we also need to guard against the 0b1111 value for ID_DFR0.PerfMon, which implies an implementation-defined, non-PMUv3 PMU, or is it safe to assume no one will attempt that flavor in QEMU? -Aaron
On Wed, 5 Dec 2018 at 13:00, Aaron Lindsay <aaron@os.amperecomputing.com> wrote: > > On Dec 03 16:57, Richard Henderson wrote: > > Yes. It would appear that this feature should be controlled by > > ID_DFR0.PerfMon. So, > > > > if (FIELD_EX32(cpu->id_dfr0, ID_DFR0, PERFMON) >= 4) > > > > once the appropriate FIELDs are added to cpu.h. > > > > Since this test is not used within translate*.c, there is no need to move > > id_dfr* into ARMISARegisters. Since these are only aliases, they do not affect > > migration, and so do not (yet) need to be filled in by kvm. > > Sounds reasonable to me. One clarification - do we also need to guard > against the 0b1111 value for ID_DFR0.PerfMon, which implies an > implementation-defined, non-PMUv3 PMU, or is it safe to assume no one > will attempt that flavor in QEMU? We should check for 0xf, yes, following the recommended check logic described in the Arm ARM section "Alternative ID scheme used for the Performance Monitors Extension version". It doesn't seem likely that we'll end up with the 0xf case but we might as well avoid the question in advance... thanks -- PMM
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 627e5c1995..50de58e4a2 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..75f054fe79 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5432,7 +5432,11 @@ 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 = "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 = "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 +5444,11 @@ 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 = "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) }, { .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 | 12 ++++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-)