Re: [Qemu-devel] [PATCH v7 07/12] target/arm: Add array for supported PMU events, generate PMCEID[01]

[Aaron Lindsay <aaron@os.amperecomputing.com>]

On Nov 16 15:06, Peter Maydell wrote:
> On 5 November 2018 at 18:51, Aaron Lindsay <aaron@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