| Message ID | 1529699547-17044-8-git-send-email-alindsay@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | More fully implement ARM PMUv3 | expand |
On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote: > This commit doesn't add any supported events, but provides the framework > for adding them. We store the pm_event structs in a simple array, and > provide the mapping from the event numbers to array indexes in the > supported_event_map array. Because the value of PMCEID[01] depends upon > which events are supported at runtime, generate it dynamically. > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > --- > target/arm/cpu.c | 20 +++++++++++++------- > target/arm/cpu.h | 10 ++++++++++ > target/arm/cpu64.c | 2 -- > target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ > 4 files changed, 60 insertions(+), 9 deletions(-) > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > index 98790b1..2f5b16a 100644 > --- a/target/arm/cpu.c > +++ b/target/arm/cpu.c > @@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > if (!cpu->has_pmu) { > unset_feature(env, ARM_FEATURE_PMU); > cpu->id_aa64dfr0 &= ~0xf00; > - } else if (!kvm_enabled()) { > - arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); > - arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); > + } > + if (arm_feature(env, ARM_FEATURE_PMU)) { > + uint64_t pmceid = get_pmceid(&cpu->env); > + cpu->pmceid0 = pmceid & 0xffffffff; > + cpu->pmceid1 = (pmceid >> 32) & 0xffffffff; > + > + if (!kvm_enabled()) { > + arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); > + arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); > + } > + } else { > + cpu->pmceid0 = 0x00000000; > + cpu->pmceid1 = 0x00000000; This is ok for now, but we really need to sort out what we're doing for the interplay between feature bits and ID register values more generally (notably for '-cpu max'), so depending on when we do that work this might end up needing to change a bit to fit in. > } > > if (!arm_feature(env, ARM_FEATURE_EL2)) { > @@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) > cpu->id_pfr0 = 0x00001131; > cpu->id_pfr1 = 0x00011011; > cpu->id_dfr0 = 0x02010555; > - cpu->pmceid0 = 0x00000000; > - cpu->pmceid1 = 0x00000000; > cpu->id_afr0 = 0x00000000; > cpu->id_mmfr0 = 0x10101105; > cpu->id_mmfr1 = 0x40000000; > @@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) > cpu->id_pfr0 = 0x00001131; > cpu->id_pfr1 = 0x00011011; > cpu->id_dfr0 = 0x02010555; > - cpu->pmceid0 = 0x0000000; > - cpu->pmceid1 = 0x00000000; > cpu->id_afr0 = 0x00000000; > cpu->id_mmfr0 = 0x10201105; > cpu->id_mmfr1 = 0x20000000; > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 852743b..430b8d5 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env); > void pmu_pre_el_change(ARMCPU *cpu, void *ignored); > void pmu_post_el_change(ARMCPU *cpu, void *ignored); > > +/* > + * get_pmceid > + * @env: CPUARMState > + * > + * Return the PMCEID[01] register values corresponding to the counters which > + * are supported given the current configuration (0 is low 32, 1 is high 32 > + * bits) > + */ > +uint64_t get_pmceid(CPUARMState *env); > + > /* SCTLR bit meanings. Several bits have been reused in newer > * versions of the architecture; in that case we define constants > * for both old and new bit meanings. Code which tests against those > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > index c50dcd4..28482a0 100644 > --- a/target/arm/cpu64.c > +++ b/target/arm/cpu64.c > @@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj) > cpu->id_isar5 = 0x00011121; > cpu->id_aa64pfr0 = 0x00002222; > cpu->id_aa64dfr0 = 0x10305106; > - cpu->pmceid0 = 0x00000000; > - 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 5d83446..9f81747 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env) > return (1 << 31) | ((1 << pmu_num_counters(env)) - 1); > } > > +typedef struct pm_event { > + uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */ > + /* If the event is supported on this CPU (used to generate PMCEID[01]) */ > + bool (*supported)(CPUARMState *); > + /* Retrieve the current count of the underlying event. The programmed > + * counters hold a difference from the return value from this function */ Don't leave the */ dangling on the RHS of a multiline comment. (CODING_STYLE now recommends Linux-kernel style.) > + uint64_t (*get_count)(CPUARMState *); > +} pm_event; > + > +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX > +static const pm_event pm_events[] = { > + { .number = SUPPORTED_EVENT_SENTINEL } > +}; > +static uint16_t supported_event_map[0x3f]; What is this hardcoded 0x3f ? > + > +/* > + * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high > + * 32). We also use it to build a map of ARM event numbers to indices in > + * our pm_events array. > + */ > +uint64_t get_pmceid(CPUARMState *env) > +{ > + uint64_t pmceid = 0; > + unsigned int i = 0; > + while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) { Are we ever going to run through some other array than the constant pm_events[] ? We could just use ARRAY_SIZE rather than requiring a sentinel member. > + const pm_event *cnt = &pm_events[i]; > + if (cnt->number < 0x3f && cnt->supported(env)) { ...another 0x3f. Are we ever really going to see an out-of-range .number field, or should we just assert() that it's in range ? > + pmceid |= (1 << cnt->number); > + supported_event_map[cnt->number] = i; > + } else { > + supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL; This will leave us with supported_event_map[] entries in one of three states: * SUPPORTED_EVENT_SENTINEL if they have a specific pm_events[] entry marked as not supported * an index if they are supported * zero if there is no pm_events[] entry Don't the ones in the last category get confused with the real index 0? Maybe this should initialize the whole array first to the sentinel value and then just fill in the supported ones with their index values? > + } > + i++; > + } > + return pmceid; > +} > + thanks -- PMM
On Jul 17 17:06, Peter Maydell wrote: > On 22 June 2018 at 21:32, Aaron Lindsay <alindsay@codeaurora.org> wrote: > > This commit doesn't add any supported events, but provides the framework > > for adding them. We store the pm_event structs in a simple array, and > > provide the mapping from the event numbers to array indexes in the > > supported_event_map array. Because the value of PMCEID[01] depends upon > > which events are supported at runtime, generate it dynamically. > > > > Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> > > --- > > target/arm/cpu.c | 20 +++++++++++++------- > > target/arm/cpu.h | 10 ++++++++++ > > target/arm/cpu64.c | 2 -- > > target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ > > 4 files changed, 60 insertions(+), 9 deletions(-) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 98790b1..2f5b16a 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) > > if (!cpu->has_pmu) { > > unset_feature(env, ARM_FEATURE_PMU); > > cpu->id_aa64dfr0 &= ~0xf00; > > - } else if (!kvm_enabled()) { > > - arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); > > - arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); > > + } > > + if (arm_feature(env, ARM_FEATURE_PMU)) { > > + uint64_t pmceid = get_pmceid(&cpu->env); > > + cpu->pmceid0 = pmceid & 0xffffffff; > > + cpu->pmceid1 = (pmceid >> 32) & 0xffffffff; > > + > > + if (!kvm_enabled()) { > > + arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); > > + arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); > > + } > > + } else { > > + cpu->pmceid0 = 0x00000000; > > + cpu->pmceid1 = 0x00000000; > > This is ok for now, but we really need to sort out what we're > doing for the interplay between feature bits and ID register > values more generally (notably for '-cpu max'), so depending on > when we do that work this might end up needing to change a bit > to fit in. Okay, I'll try to be on the lookout for this work. > > } > > > > if (!arm_feature(env, ARM_FEATURE_EL2)) { > > @@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) > > cpu->id_pfr0 = 0x00001131; > > cpu->id_pfr1 = 0x00011011; > > cpu->id_dfr0 = 0x02010555; > > - cpu->pmceid0 = 0x00000000; > > - cpu->pmceid1 = 0x00000000; > > cpu->id_afr0 = 0x00000000; > > cpu->id_mmfr0 = 0x10101105; > > cpu->id_mmfr1 = 0x40000000; > > @@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) > > cpu->id_pfr0 = 0x00001131; > > cpu->id_pfr1 = 0x00011011; > > cpu->id_dfr0 = 0x02010555; > > - cpu->pmceid0 = 0x0000000; > > - cpu->pmceid1 = 0x00000000; > > cpu->id_afr0 = 0x00000000; > > cpu->id_mmfr0 = 0x10201105; > > cpu->id_mmfr1 = 0x20000000; > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > > index 852743b..430b8d5 100644 > > --- a/target/arm/cpu.h > > +++ b/target/arm/cpu.h > > @@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env); > > void pmu_pre_el_change(ARMCPU *cpu, void *ignored); > > void pmu_post_el_change(ARMCPU *cpu, void *ignored); > > > > +/* > > + * get_pmceid > > + * @env: CPUARMState > > + * > > + * Return the PMCEID[01] register values corresponding to the counters which > > + * are supported given the current configuration (0 is low 32, 1 is high 32 > > + * bits) > > + */ > > +uint64_t get_pmceid(CPUARMState *env); > > + > > /* SCTLR bit meanings. Several bits have been reused in newer > > * versions of the architecture; in that case we define constants > > * for both old and new bit meanings. Code which tests against those > > diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c > > index c50dcd4..28482a0 100644 > > --- a/target/arm/cpu64.c > > +++ b/target/arm/cpu64.c > > @@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj) > > cpu->id_isar5 = 0x00011121; > > cpu->id_aa64pfr0 = 0x00002222; > > cpu->id_aa64dfr0 = 0x10305106; > > - cpu->pmceid0 = 0x00000000; > > - 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 5d83446..9f81747 100644 > > --- a/target/arm/helper.c > > +++ b/target/arm/helper.c > > @@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env) > > return (1 << 31) | ((1 << pmu_num_counters(env)) - 1); > > } > > > > +typedef struct pm_event { > > + uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */ > > + /* If the event is supported on this CPU (used to generate PMCEID[01]) */ > > + bool (*supported)(CPUARMState *); > > + /* Retrieve the current count of the underlying event. The programmed > > + * counters hold a difference from the return value from this function */ > > Don't leave the */ dangling on the RHS of a multiline comment. > (CODING_STYLE now recommends Linux-kernel style.) > > > + uint64_t (*get_count)(CPUARMState *); > > +} pm_event; > > + > > +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX > > +static const pm_event pm_events[] = { > > + { .number = SUPPORTED_EVENT_SENTINEL } > > +}; > > +static uint16_t supported_event_map[0x3f]; > > What is this hardcoded 0x3f ? This was the maximum common event ID (i.e. bits in PMCEID[01]) as of ARMv8.0. Obviously it should be a macro instead of a magic constant. I'm also debating whether this should be the maximum supported by the architecture or just the maximum supported by QEMU (particularly because later ARM versions raise this number significantly). > > + > > +/* > > + * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high > > + * 32). We also use it to build a map of ARM event numbers to indices in > > + * our pm_events array. > > + */ > > +uint64_t get_pmceid(CPUARMState *env) > > +{ > > + uint64_t pmceid = 0; > > + unsigned int i = 0; > > + while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) { > > Are we ever going to run through some other array than the > constant pm_events[] ? We could just use ARRAY_SIZE rather > than requiring a sentinel member. No, I don't think so. > > > + const pm_event *cnt = &pm_events[i]; > > + if (cnt->number < 0x3f && cnt->supported(env)) { > > ...another 0x3f. Are we ever really going to see an > out-of-range .number field, or should we just assert() > that it's in range ? In some places this can be set by user code via PMEVTYPER.evtCount, but I think in this code an assert is the right way to handle it. > > > + pmceid |= (1 << cnt->number); > > + supported_event_map[cnt->number] = i; > > + } else { > > + supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL; > > This will leave us with supported_event_map[] entries in > one of three states: > * SUPPORTED_EVENT_SENTINEL if they have a specific pm_events[] > entry marked as not supported > * an index if they are supported > * zero if there is no pm_events[] entry > > Don't the ones in the last category get confused with the > real index 0? > > Maybe this should initialize the whole array first to the > sentinel value and then just fill in the supported ones with > their index values? I've re-written this code based on your feedback for v6, eliminating the sentinel and initializing the entirety of supported_event_map. -Aaron
diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 98790b1..2f5b16a 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -927,9 +927,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) if (!cpu->has_pmu) { unset_feature(env, ARM_FEATURE_PMU); cpu->id_aa64dfr0 &= ~0xf00; - } else if (!kvm_enabled()) { - arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); - arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); + } + if (arm_feature(env, ARM_FEATURE_PMU)) { + uint64_t pmceid = get_pmceid(&cpu->env); + cpu->pmceid0 = pmceid & 0xffffffff; + cpu->pmceid1 = (pmceid >> 32) & 0xffffffff; + + if (!kvm_enabled()) { + arm_register_pre_el_change_hook(cpu, &pmu_pre_el_change, 0); + arm_register_el_change_hook(cpu, &pmu_post_el_change, 0); + } + } else { + cpu->pmceid0 = 0x00000000; + cpu->pmceid1 = 0x00000000; } if (!arm_feature(env, ARM_FEATURE_EL2)) { @@ -1538,8 +1548,6 @@ static void cortex_a7_initfn(Object *obj) cpu->id_pfr0 = 0x00001131; cpu->id_pfr1 = 0x00011011; cpu->id_dfr0 = 0x02010555; - cpu->pmceid0 = 0x00000000; - cpu->pmceid1 = 0x00000000; cpu->id_afr0 = 0x00000000; cpu->id_mmfr0 = 0x10101105; cpu->id_mmfr1 = 0x40000000; @@ -1581,8 +1589,6 @@ static void cortex_a15_initfn(Object *obj) cpu->id_pfr0 = 0x00001131; cpu->id_pfr1 = 0x00011011; cpu->id_dfr0 = 0x02010555; - cpu->pmceid0 = 0x0000000; - cpu->pmceid1 = 0x00000000; cpu->id_afr0 = 0x00000000; cpu->id_mmfr0 = 0x10201105; cpu->id_mmfr1 = 0x20000000; diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 852743b..430b8d5 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -964,6 +964,16 @@ void pmu_op_finish(CPUARMState *env); void pmu_pre_el_change(ARMCPU *cpu, void *ignored); void pmu_post_el_change(ARMCPU *cpu, void *ignored); +/* + * get_pmceid + * @env: CPUARMState + * + * Return the PMCEID[01] register values corresponding to the counters which + * are supported given the current configuration (0 is low 32, 1 is high 32 + * bits) + */ +uint64_t get_pmceid(CPUARMState *env); + /* SCTLR bit meanings. Several bits have been reused in newer * versions of the architecture; in that case we define constants * for both old and new bit meanings. Code which tests against those diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c index c50dcd4..28482a0 100644 --- a/target/arm/cpu64.c +++ b/target/arm/cpu64.c @@ -141,8 +141,6 @@ static void aarch64_a57_initfn(Object *obj) cpu->id_isar5 = 0x00011121; cpu->id_aa64pfr0 = 0x00002222; cpu->id_aa64dfr0 = 0x10305106; - cpu->pmceid0 = 0x00000000; - 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 5d83446..9f81747 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -964,6 +964,43 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env) return (1 << 31) | ((1 << pmu_num_counters(env)) - 1); } +typedef struct pm_event { + uint16_t number; /* PMEVTYPER.evtCount is 10 bits wide */ + /* If the event is supported on this CPU (used to generate PMCEID[01]) */ + bool (*supported)(CPUARMState *); + /* Retrieve the current count of the underlying event. The programmed + * counters hold a difference from the return value from this function */ + uint64_t (*get_count)(CPUARMState *); +} pm_event; + +#define SUPPORTED_EVENT_SENTINEL UINT16_MAX +static const pm_event pm_events[] = { + { .number = SUPPORTED_EVENT_SENTINEL } +}; +static uint16_t supported_event_map[0x3f]; + +/* + * Called upon initialization to build PMCEID0 (low 32 bits) and PMCEID1 (high + * 32). We also use it to build a map of ARM event numbers to indices in + * our pm_events array. + */ +uint64_t get_pmceid(CPUARMState *env) +{ + uint64_t pmceid = 0; + unsigned int i = 0; + while (pm_events[i].number != SUPPORTED_EVENT_SENTINEL) { + const pm_event *cnt = &pm_events[i]; + if (cnt->number < 0x3f && cnt->supported(env)) { + pmceid |= (1 << cnt->number); + supported_event_map[cnt->number] = i; + } else { + supported_event_map[cnt->number] = SUPPORTED_EVENT_SENTINEL; + } + i++; + } + return pmceid; +} + static CPAccessResult pmreg_access(CPUARMState *env, const ARMCPRegInfo *ri, bool isread) {
This commit doesn't add any supported events, but provides the framework for adding them. We store the pm_event structs in a simple array, and provide the mapping from the event numbers to array indexes in the supported_event_map array. Because the value of PMCEID[01] depends upon which events are supported at runtime, generate it dynamically. Signed-off-by: Aaron Lindsay <alindsay@codeaurora.org> --- target/arm/cpu.c | 20 +++++++++++++------- target/arm/cpu.h | 10 ++++++++++ target/arm/cpu64.c | 2 -- target/arm/helper.c | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 60 insertions(+), 9 deletions(-)