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
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
