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

[Richard Henderson <richard.henderson@linaro.org>]

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 <alindsay@codeaurora.org>
>>> ---
>>>  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.

> @@ -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

> @@ -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.


r~