Re: [Qemu-devel] [kvm-unit-tests PATCH v11 1/3] arm: Add PMU test

[Wei Huang]

On 11/25/2016 08:26 AM, Andrew Jones wrote:
> On Fri, Nov 25, 2016 at 12:32:24PM +0000, Andre Przywara wrote:
>> Hi Drew,
>>
>> ....
>>
>> On 23/11/16 17:15, Andrew Jones wrote:
>>>>> +
>>>>> +#if defined(__arm__)
>>>>
>>>> I guess you should use the arch specific header files we have in place
>>>> for that (lib/arm{.64}/asm/processor.h). Also there are sysreg read
>>>> wrappers (at least for arm64) in there already, can't we base this
>>>> function on them: DEFINE_GET_SYSREG32(pmcr, el0)?
>>>> (Requires a small change to get rid of the forced "_el1" suffix)
>>>>
>>>> We should wait for the GIC series to be merged, as this contains some
>>>> changes in this area.
>>>
>>> As this unit test is the only consumer of PMC registers so far, then
>>> I'd prefer the defines and accessors stay here for now. Once we see
>>> a use in other unit tests then we can move some of it out.
>>
>> Well, I was more thinking of something like below.
>> I am fine with keeping the PMU sysregs private to pmu.c, but we can still
>> use the sysreg wrappers, can't we?
>> This is on top of Wei's series, so doesn't have your SYSREG32/64
>> unification, but I leave this as an exercise to the reader.
>> There is some churn in pmu.c below due to the change of <sysreg>_write to
>> set_<sysreg>, but the rest looks like simplification to me.
>>
>> Does that make sense?
> 
> Ah, now I see what you mean, and I think I like that. The question is
> whether or not I like my SYSREG macros :-) I see value in having the
> asm's easy to read (open-coded), as well as value in making sure we
> only have to review sysreg functions once. Let's ask for Wei's and
> Cov's votes. If they like the SYSREG direction, then they can vote
> with another version of this series :-)

Let us use SYSREG macros then, because it makes coding easier. V13 has
been sent. I think this PMU patcheset is a bit bloated now. So hopefully
this is the last version. After it is accepted, we can always come back
to re-factor SYSREG r/w further (if need).

Thanks,
-Wei

> 
> Thanks,
> drew
> 
>>
>> Cheers,
>> Andre.
>>
>> ---
>>  arm/pmu.c                 | 159 
>> +++++++++++++---------------------------------
>>  lib/arm/asm/processor.h   |  34 ++++++++--
>>  lib/arm64/asm/processor.h |  23 ++++++-
>>  3 files changed, 92 insertions(+), 124 deletions(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index f667676..f0ad02a 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -14,6 +14,7 @@
>>   */
>>  #include "libcflat.h"
>>  #include "asm/barrier.h"
>> +#include "asm/processor.h"
>>  
>>  #define PMU_PMCR_E         (1 << 0)
>>  #define PMU_PMCR_C         (1 << 2)
>> @@ -33,78 +34,42 @@
>>  #define NR_SAMPLES 10
>>  
>>  static unsigned int pmu_version;
>> -#if defined(__arm__)
>> -static inline uint32_t pmcr_read(void)
>> -{
>> -    uint32_t ret;
>> -
>> -    asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>> -    return ret;
>> -}
>> -
>> -static inline void pmcr_write(uint32_t value)
>> -{
>> -    asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
>> -    isb();
>> -}
>>  
>> -static inline void pmselr_write(uint32_t value)
>> -{
>> -    asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
>> -    isb();
>> -}
>> -
>> -static inline void pmxevtyper_write(uint32_t value)
>> -{
>> -    asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
>> -}
>> -
>> -static inline uint64_t pmccntr_read(void)
>> +#if defined(__arm__)
>> +DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0)
>> +DEFINE_SET_SYSREG32(pmcr, 0, c9, c12, 0)
>> +DEFINE_GET_SYSREG32(id_dfr0, 0, c0, c1, 2)
>> +DEFINE_SET_SYSREG32(pmselr, 0, c9, c12, 5)
>> +DEFINE_SET_SYSREG32(pmxevtyper, 0, c9, c13, 1)
>> +DEFINE_GET_SYSREG32(pmccntr32, 0, c9, c13, 0)
>> +DEFINE_SET_SYSREG32(pmccntr32, 0, c9, c13, 0)
>> +DEFINE_GET_SYSREG64(pmccntr64, 0, c9)
>> +DEFINE_SET_SYSREG64(pmccntr64, 0, c9)
>> +DEFINE_SET_SYSREG32(pmcntenset, 0, c9, c12, 1)
>> +
>> +static inline uint64_t get_pmccntr(void)
>>  {
>> -    uint32_t lo, hi = 0;
>> -
>>      if (pmu_version == 0x3)
>> -            asm volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
>> +            return get_pmccntr32();
>>      else
>> -            asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (lo));
>> -
>> -    return ((uint64_t)hi << 32) | lo;
>> +            return get_pmccntr64();
>>  }
>>  
>> -static inline void pmccntr_write(uint64_t value)
>> +static inline void set_pmccntr(uint64_t value)
>>  {
>> -    uint32_t lo, hi;
>> -
>> -    lo = value & 0xffffffff;
>> -    hi = (value >> 32) & 0xffffffff;
>> -
>>      if (pmu_version == 0x3)
>> -            asm volatile("mcrr p15, 0, %0, %1, c9" : : "r" (lo), "r" (hi));
>> +            set_pmccntr64(value);
>>      else
>> -            asm volatile("mcr p15, 0, %0, c9, c13, 0" : : "r" (lo));
>> +            set_pmccntr64(value & 0xffffffff);
>>  }
>> -
>> -static inline void pmcntenset_write(uint32_t value)
>> -{
>> -    asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (value));
>> -}
>> -
>>  /* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
>> -static inline void pmccfiltr_write(uint32_t value)
>> +static inline void set_pmccfiltr(uint32_t value)
>>  {
>> -    pmselr_write(PMU_CYCLE_IDX);
>> -    pmxevtyper_write(value);
>> +    set_pmselr(PMU_CYCLE_IDX);
>> +    set_pmxevtyper(value);
>>      isb();
>>  }
>>  
>> -static inline uint32_t id_dfr0_read(void)
>> -{
>> -    uint32_t val;
>> -
>> -    asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
>> -    return val;
>> -}
>> -
>>  /*
>>   * Extra instructions inserted by the compiler would be difficult to 
>> compensate
>>   * for, so hand assemble everything between, and including, the PMCR 
>> accesses
>> @@ -126,51 +91,13 @@ static inline void precise_instrs_loop(int loop, 
>> uint32_t pmcr)
>>      : "cc");
>>  }
>>  #elif defined(__aarch64__)
>> -static inline uint32_t pmcr_read(void)
>> -{
>> -    uint32_t ret;
>> -
>> -    asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>> -    return ret;
>> -}
>> -
>> -static inline void pmcr_write(uint32_t value)
>> -{
>> -    asm volatile("msr pmcr_el0, %0" : : "r" (value));
>> -    isb();
>> -}
>> -
>> -static inline uint64_t pmccntr_read(void)
>> -{
>> -    uint64_t cycles;
>> -
>> -    asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>> -    return cycles;
>> -}
>> -
>> -static inline void pmccntr_write(uint64_t value)
>> -{
>> -    asm volatile("msr pmccntr_el0, %0" : : "r" (value));
>> -}
>> -
>> -static inline void pmcntenset_write(uint32_t value)
>> -{
>> -    asm volatile("msr pmcntenset_el0, %0" : : "r" (value));
>> -}
>> -
>> -static inline void pmccfiltr_write(uint32_t value)
>> -{
>> -    asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
>> -    isb();
>> -}
>> -
>> -static inline uint32_t id_dfr0_read(void)
>> -{
>> -    uint32_t id;
>> -
>> -    asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
>> -    return id;
>> -}
>> +DEFINE_GET_SYSREG32(pmcr, el0)
>> +DEFINE_SET_SYSREG32(pmcr, el0)
>> +DEFINE_GET_SYSREG32(id_dfr0, el1)
>> +DEFINE_GET_SYSREG64(pmccntr, el0);
>> +DEFINE_SET_SYSREG64(pmccntr, el0);
>> +DEFINE_SET_SYSREG32(pmcntenset, el0);
>> +DEFINE_SET_SYSREG32(pmccfiltr, el0);
>>  
>>  /*
>>   * Extra instructions inserted by the compiler would be difficult to 
>> compensate
>> @@ -206,7 +133,7 @@ static bool check_pmcr(void)
>>  {
>>      uint32_t pmcr;
>>  
>> -    pmcr = pmcr_read();
>> +    pmcr = get_pmcr();
>>  
>>      printf("PMU implementer:     %c\n",
>>             (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
>> @@ -226,17 +153,17 @@ static bool check_cycles_increase(void)
>>      bool success = true;
>>  
>>      /* init before event access, this test only cares about cycle count */
>> -    pmcntenset_write(1 << PMU_CYCLE_IDX);
>> -    pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */
>> -    pmccntr_write(0);
>> +    set_pmcntenset(1 << PMU_CYCLE_IDX);
>> +    set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>> +    set_pmccntr(0);
>>  
>> -    pmcr_write(pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
>> +    set_pmcr(get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
>>  
>>      for (int i = 0; i < NR_SAMPLES; i++) {
>>              uint64_t a, b;
>>  
>> -            a = pmccntr_read();
>> -            b = pmccntr_read();
>> +            a = get_pmccntr();
>> +            b = get_pmccntr();
>>  
>>              if (a >= b) {
>>                      printf("Read %"PRId64" then %"PRId64".\n", a, b);
>> @@ -245,7 +172,7 @@ static bool check_cycles_increase(void)
>>              }
>>      }
>>  
>> -    pmcr_write(pmcr_read() & ~PMU_PMCR_E);
>> +    set_pmcr(get_pmcr() & ~PMU_PMCR_E);
>>  
>>      return success;
>>  }
>> @@ -276,11 +203,11 @@ static void measure_instrs(int num, uint32_t pmcr)
>>   */
>>  static bool check_cpi(int cpi)
>>  {
>> -    uint32_t pmcr = pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
>> +    uint32_t pmcr = get_pmcr() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
>>  
>>      /* init before event access, this test only cares about cycle count */
>> -    pmcntenset_write(1 << PMU_CYCLE_IDX);
>> -    pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */
>> +    set_pmcntenset(1 << PMU_CYCLE_IDX);
>> +    set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>>  
>>      if (cpi > 0)
>>              printf("Checking for CPI=%d.\n", cpi);
>> @@ -293,9 +220,9 @@ static bool check_cpi(int cpi)
>>              for (int j = 0; j < NR_SAMPLES; j++) {
>>                      uint64_t cycles;
>>  
>> -                    pmccntr_write(0);
>> +                    set_pmccntr(0);
>>                      measure_instrs(i, pmcr);
>> -                    cycles = pmccntr_read();
>> +                    cycles = get_pmccntr();
>>                      printf(" %"PRId64"", cycles);
>>  
>>                      if (!cycles) {
>> @@ -328,7 +255,7 @@ void pmu_init(void)
>>      uint32_t dfr0;
>>  
>>      /* probe pmu version */
>> -    dfr0 = id_dfr0_read();
>> +    dfr0 = get_id_dfr0();
>>      pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK;
>>      printf("PMU version: %d\n", pmu_version);
>>  }
>> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
>> index f25e7ee..6e7ffa3 100644
>> --- a/lib/arm/asm/processor.h
>> +++ b/lib/arm/asm/processor.h
>> @@ -33,12 +33,36 @@ static inline unsigned long current_cpsr(void)
>>  
>>  #define current_mode() (current_cpsr() & MODE_MASK)
>>  
>> -static inline unsigned int get_mpidr(void)
>> -{
>> -    unsigned int mpidr;
>> -    asm volatile("mrc p15, 0, %0, c0, c0, 5" : "=r" (mpidr));
>> -    return mpidr;
>> +#define DEFINE_GET_SYSREG32(name, opc1, crn, crm, opc2)                     
>> \
>> +static inline unsigned long get_##name(void)                                
>> \
>> +{                                                                   \
>> +    unsigned long reg;                                              \
>> +    asm volatile("mrc p15, " #opc1 ", %0, " #crn ", " #crm ", " #opc2  \
>> +                 : "=r" (reg));                                     \
>> +    return reg;                                                     \
>> +}
>> +#define DEFINE_SET_SYSREG32(name, opc1, crn, crm, opc2)                     
>> \
>> +static inline void set_##name(unsigned int value)                   \
>> +{                                                                   \
>> +    asm volatile("mcr p15, " #opc1 ", %0, " #crn ", " #crm ", " #opc2  \
>> +                 :: "r" (value));                                   \
>> +}
>> +#define DEFINE_GET_SYSREG64(name, opc1, crm)                                
>> \
>> +static inline uint64_t get_##name(void)                                     
>> \
>> +{                                                                   \
>> +    uint32_t lo, hi;                                                \
>> +    asm volatile("mrrc p15, " #opc1 ", %0, %1, " #crm               \
>> +                 : "=r" (lo), "=r" (hi));                           \
>> +    return lo | (uint64_t)hi << 32;                                 \
>>  }
>> +#define DEFINE_SET_SYSREG64(name, opc1, crm)                                
>> \
>> +static inline void set_##name(uint64_t value)                               
>> \
>> +{                                                                   \
>> +    asm volatile("mcrr p15, " #opc1 ", %0, %1, " #crm               \
>> +                 :: "r" (value & 0xffffffff), "r" (value >> 32));   \
>> +}
>> +
>> +DEFINE_GET_SYSREG32(mpidr, 0, c0, c0, 5)
>>  
>>  /* Only support Aff0 for now, up to 4 cpus */
>>  #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff))
>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 84d5c7c..cf06c41 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -66,14 +66,31 @@ static inline unsigned long current_level(void)
>>      return el & 0xc;
>>  }
>>  
>> -#define DEFINE_GET_SYSREG32(reg)                            \
>> +#define DEFINE_GET_SYSREG32(reg, el)                                \
>>  static inline unsigned int get_##reg(void)                  \
>>  {                                                           \
>>      unsigned int reg;                                       \
>> -    asm volatile("mrs %0, " #reg "_el1" : "=r" (reg));      \
>> +    asm volatile("mrs %0, " #reg "_" #el : "=r" (reg));     \
>>      return reg;                                             \
>>  }
>> -DEFINE_GET_SYSREG32(mpidr)
>> +#define DEFINE_SET_SYSREG32(reg, el)                                \
>> +static inline void set_##reg(unsigned int value)            \
>> +{                                                           \
>> +    asm volatile("msr " #reg "_" #el ", %0" :: "r" (value));\
>> +}
>> +#define DEFINE_GET_SYSREG64(reg, el)                                \
>> +static inline uint64_t get_##reg(void)                              \
>> +{                                                           \
>> +    uint64_t reg;                                           \
>> +    asm volatile("mrs %0, " #reg "_" #el : "=r" (reg));     \
>> +    return reg;                                             \
>> +}
>> +#define DEFINE_SET_SYSREG64(reg, el)                                \
>> +static inline void set_##reg(uint64_t value)                        \
>> +{                                                           \
>> +    asm volatile("msr " #reg "_" #el ", %0" :: "r" (value));\
>> +}
>> +DEFINE_GET_SYSREG32(mpidr, el1)
>>  
>>  /* Only support Aff0 for now, gicv2 only */
>>  #define mpidr_to_cpu(mpidr) ((int)((mpidr) & 0xff))
>> -- 
>> 2.9.0
>>
>>