Re: [PATCH v2 4/4] MIPS: perf: Add support for 64-bit perf counters.

[Ralf Baechle <ralf@linux-mips.org>]

On Thu, Feb 17, 2011 at 06:46:39PM +0800, Deng-Cheng Zhu wrote:

> The reason of the perf-record failure on 32bit platforms is that the 32bit
> counter read function mipsxx_pmu_read_counter() returns wrong 64bit values.
> For example, the counter value 0x12345678 will be returned as
> 0xffffffff12345678. So in mipspmu_event_update(), the delta will be wrong.
> So here's a possible fix for your reference:
> 
> --- a/arch/mips/kernel/perf_event_mipsxx.c
> +++ b/arch/mips/kernel/perf_event_mipsxx.c
> @@ -184,19 +184,21 @@ static unsigned int
> mipsxx_pmu_swizzle_perf_idx(unsigned int idx)
>         return idx;
>  }
> 
> +#define U32_MASK 0xffffffff
> +
>  static u64 mipsxx_pmu_read_counter(unsigned int idx)
>  {
>         idx = mipsxx_pmu_swizzle_perf_idx(idx);
> 
>         switch (idx) {
>         case 0:
> -               return read_c0_perfcntr0();
> +               return read_c0_perfcntr0() & U32_MASK;
>         case 1:
> -               return read_c0_perfcntr1();
> +               return read_c0_perfcntr1() & U32_MASK;
>         case 2:
> -               return read_c0_perfcntr2();
> +               return read_c0_perfcntr2() & U32_MASK;
>         case 3:
> -               return read_c0_perfcntr3();
> +               return read_c0_perfcntr3() & U32_MASK;

read_c0_perfctrl0 etc. are defined in mipsregs.h as 32-bit reads returning
a signed int.  That was ok on 32-bit kernels.  To support the optional
64-bit counters the code will have to be changed to something like:

static u64 mipsxx_pmu_read_counter(unsigned int idx)
{
        idx = mipsxx_pmu_swizzle_perf_idx(idx);

        switch (idx) {
        case 0:
                if (read_c0_perfctrl0() & M_PERFCTL_WIDE)
                        return read_c0_64_bit_perfcntr0();
                else
                        return read_c0_32_bit_perfcntr0();
        case 1:
                if (read_c0_perfctrl1() & M_PERFCTL_WIDE)
                        return read_c0_64_bit_perfcntr1();
                else
                        return read_c0_32_bit_perfcntr1();
...

And read_c0_32_bit_perfcntrX need to zero-extend their return value.

  Ralf