On Nov 16 15:06, Peter Maydell wrote:
> On 5 November 2018 at 18:51, Aaron Lindsay <aa...@os.amperecomputing.com>
> wrote:
> > --- a/target/arm/cpu.c
> > +++ b/target/arm/cpu.c
> > @@ -957,9 +957,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 = extract64(pmceid, 0, 32);
> > + cpu->pmceid1 = extract64(pmceid, 32, 32);
> > +
> > + 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 now sets one of the ID registers for "we have no
> PMU" in the first "if (!cpu->has_pmu)" statement (id_aa64dfr0),
> and some of them (pmceid0, pcmeid1) in this else clause at the
> end. We should put them all in the same place.
I agree that would be cleaner - I'll move the id_aa64dfr0 update to
later else clause.
> > +/*
> > + * 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);
>
> This doesn't look like the right API, because in AArch64
> PMCEID0_EL0 and PMCEID1_EL0 are both 64-bit registers,
> so you can't fit them both into a single uint64_t.
Heh, I think I started working on this long enough ago that I was using
a copy of the ARMv8.0 ARM - before the statistical profiling extensions
were announced. I believe those are the only currently-defined common
events >= 0x4000, so they're the only bits defined in the upper 32 bits
of PMCEID[01].
Now that I look at it, pmceid[01] are currently only defined as
uint32_t, and we don't have PMCEID[23] for the high bits for AArch32.
Perhaps that should be a separate patch.
At any rate, I'll plan to update this the following signature for the
next version, where `which` is [01] for which PMCEID is being requested.
For now I'll probably just put a comment about not supporting the 0x40xx
events, since I don't know that coming up with a sparse array is worth
it, but the signature will be appropriate if we add support later:
uint64_t get_pmceid(CPUARMState *env, unsigned which);
-Aaron
