Re: [Qemu-devel] [PATCH v6 12/14] target/arm: PMU: Set PMCR.N to 4

[Aaron Lindsay]

On Oct 17 12:34, Richard Henderson wrote:
> On 10/17/18 12:20 PM, Aaron Lindsay wrote:
> > On Oct 16 17:09, Richard Henderson wrote:
> >> On 10/10/18 1:37 PM, Aaron Lindsay wrote:
> >>> This both advertises that we support four counters and enables them
> >>> because the pmu_num_counters() reads this value from PMCR.
> >>>
> >>> Signed-off-by: Aaron Lindsay <address@hidden>
> >>> ---
> >>>  target/arm/helper.c | 8 ++++----
> >>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> It looks like this should have been folded into patch 10,
> >> or patch 10 split to include the code changes that go with
> >> these comment changes.
> > 
> > By setting PMCR.N to 4, this commit is what causes all the previous
> > implementation of non-PMCCNTR counters to become usable by the guest -
> > both because the OS can now detect their presence in PMCR, and because
> > the system registers used to setup/access the counters are gated on
> > pmu_num_counters(), which uses PMCR.N to determine if a counter is
> > implemented.
> 
> But e.g.
> 
> > @@ -5412,8 +5412,8 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >      }
> >      if (arm_feature(env, ARM_FEATURE_V7)) {
> >          /* v7 performance monitor control register: same implementor
> > -         * field as main ID register, and we implement only the cycle
> > -         * count register.
> > +         * field as main ID register, and we implement four counters in
> > +         * addition to the cycle count register.
> >           */
> >          unsigned int i, pmcrn = 4;
> >          ARMCPRegInfo pmcr = {
> 
> clearly belongs with the previous patch, since pmcrn is already 4.

I suppose pmcrn (the local variable) should've been set to 0 before this
patch and updated here to be 4. However, even though it was already set
to 4 my amendment to the comment was still not true until this patch,
because it wasn't written into PMCR.N (the CP register).

> > @@ -1706,7 +1706,7 @@ static const ARMCPRegInfo v7_cp_reginfo[] = {
> >        .access = PL1_W, .type = ARM_CP_NOP },
> >      /* Performance monitors are implementation defined in v7,
> >       * but with an ARM recommended set of registers, which we
> > -     * follow (although we don't actually implement any counters)
> > +     * follow.
> >       *
> >       * Performance registers fall into three categories:
> >       *  (a) always UNDEF in PL0, RW in PL1 (PMINTENSET, PMINTENCLR)
> 
> And this is out of date as well, and probably belonged elsewhere.
> Which leaves only

Why is this out of date? Because PMCR.N still contained zero, no
counters were implemented from the guest's standpoint until this patch.

> > @@ -5430,7 +5430,7 @@ void register_cp_regs_for_features(ARMCPU *cpu)
> >              .access = PL0_RW, .accessfn = pmreg_access,
> >              .type = ARM_CP_IO,
> >              .fieldoffset = offsetof(CPUARMState, cp15.c9_pmcr),
> > -            .resetvalue = cpu->midr & 0xff000000,
> > +            .resetvalue = (cpu->midr & 0xff000000) | (pmcrn << 
> > PMCRN_SHIFT),
> >              .writefn = pmcr_write, .raw_writefn = raw_write,
> >          };
> 
> which suggests that this one line belonged to the patch that set pmcrn in the
> first place.

I get the feeling we may be talking past each other/misunderstanding
somehow, but I'm not sure why/how. Maybe I'm thinking about whether or
not counters are implemented from the guest's standpoint and you're
thinking about it from the standpoint of whether the code to implement
them exists in QEMU?

Anyway, by splitting these patches apart, my intention was to separate
the plumbing work from turning on the water. If you believe it is better
to do both in the same patch, I'll make that change for v7.

-Aaron