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

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

[Christopher Covington]

Beginning with a simple sanity check of the control register, add
a unit test for the ARM Performance Monitors Unit (PMU). As of
October 2016, whether KVM mode has a PMU at all is a tricky
question of which QEMU / mach-virt version is used. So only enable
the TCG mode tests for now.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 arm/Makefile.common |  3 +-
 arm/pmu.c           | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg   | 14 +++++++++
 3 files changed, 98 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index ccb554d..f98f422 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -11,7 +11,8 @@ endif
 
 tests-common = \
 	$(TEST_DIR)/selftest.flat \
-	$(TEST_DIR)/spinlock-test.flat
+	$(TEST_DIR)/spinlock-test.flat \
+	$(TEST_DIR)/pmu.flat
 
 all: test_cases
 
diff --git a/arm/pmu.c b/arm/pmu.c
new file mode 100644
index 0000000..42d0ee1
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,82 @@
+/*
+ * Test the ARM Performance Monitors Unit (PMU).
+ *
+ * Copyright 2015 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 2.1 and
+ * only version 2.1 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
+ * for more details.
+ */
+#include "libcflat.h"
+
+#if defined(__arm__)
+static inline uint32_t get_pmcr(void)
+{
+	uint32_t ret;
+
+	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
+	return ret;
+}
+#elif defined(__aarch64__)
+static inline uint32_t get_pmcr(void)
+{
+	uint32_t ret;
+
+	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
+	return ret;
+}
+#endif
+
+struct pmu_data {
+	union {
+		uint32_t pmcr_el0;
+		struct {
+			uint32_t enable:1;
+			uint32_t event_counter_reset:1;
+			uint32_t cycle_counter_reset:1;
+			uint32_t cycle_counter_clock_divider:1;
+			uint32_t event_counter_export:1;
+			uint32_t cycle_counter_disable_when_prohibited:1;
+			uint32_t cycle_counter_long:1;
+			uint32_t reserved:4;
+			uint32_t counters:5;
+			uint32_t identification_code:8;
+			uint32_t implementer:8;
+		};
+	};
+};
+
+/*
+ * As a simple sanity check on the PMCR_EL0, ensure the implementer field isn't
+ * null. Also print out a couple other interesting fields for diagnostic
+ * purposes. For example, as of fall 2015, QEMU TCG mode doesn't implement
+ * event counters and therefore reports zero event counters, but hopefully
+ * support for at least the instructions event will be added in the future and
+ * the reported number of event counters will become nonzero.
+ */
+static bool check_pmcr(void)
+{
+	struct pmu_data pmu;
+
+	pmu.pmcr_el0 = get_pmcr();
+
+	printf("PMU implementer:     %c\n", pmu.implementer);
+	printf("Identification code: 0x%x\n", pmu.identification_code);
+	printf("Event counters:      %d\n", pmu.counters);
+
+	return pmu.implementer != 0;
+}
+
+int main(void)
+{
+	report_prefix_push("pmu");
+
+	report("Control register", check_pmcr());
+
+	return report_summary();
+}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ffd12e5..edaed4a 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -51,3 +51,17 @@ file = selftest.flat
 smp = $MAX_SMP
 extra_params = -append 'smp'
 groups = selftest
+
+# Test PMU support with -icount IPC=1
+[pmu-icount-1]
+file = pmu.flat
+extra_params = -icount 0 -append '1'
+groups = pmu
+accel = tcg
+
+# Test PMU support with -icount IPC=256
+[pmu-icount-256]
+file = pmu.flat
+extra_params = -icount 8 -append '256'
+groups = pmu
+accel = tcg
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [kvm-unit-tests PATCHv6 2/3] arm: pmu: Check cycle count increases

[Christopher Covington]

Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
even for the smallest delta of two subsequent reads.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 arm/pmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 42d0ee1..4334de4 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -14,6 +14,8 @@
  */
 #include "libcflat.h"
 
+#define NR_SAMPLES 10
+
 #if defined(__arm__)
 static inline uint32_t get_pmcr(void)
 {
@@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
 	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
 	return ret;
 }
+
+static inline void set_pmcr(uint32_t pmcr)
+{
+	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
+}
+
+/*
+ * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 64
+ * bits doesn't seem worth the trouble when differential usage of the result is
+ * expected (with differences that can easily fit in 32 bits). So just return
+ * the lower 32 bits of the cycle count in AArch32.
+ */
+static inline unsigned long get_pmccntr(void)
+{
+	unsigned long cycles;
+
+	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
+	return cycles;
+}
 #elif defined(__aarch64__)
 static inline uint32_t get_pmcr(void)
 {
@@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
 	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
 	return ret;
 }
+
+static inline void set_pmcr(uint32_t pmcr)
+{
+	asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
+}
+
+static inline unsigned long get_pmccntr(void)
+{
+	unsigned long cycles;
+
+	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
+	return cycles;
+}
 #endif
 
 struct pmu_data {
@@ -72,11 +106,37 @@ static bool check_pmcr(void)
 	return pmu.implementer != 0;
 }
 
+/*
+ * Ensure that the cycle counter progresses between back-to-back reads.
+ */
+static bool check_cycles_increase(void)
+{
+	struct pmu_data pmu = {0};
+
+	pmu.enable = 1;
+	set_pmcr(pmu.pmcr_el0);
+
+	for (int i = 0; i < NR_SAMPLES; i++) {
+		unsigned long a, b;
+
+		a = get_pmccntr();
+		b = get_pmccntr();
+
+		if (a >= b) {
+			printf("Read %ld then %ld.\n", a, b);
+			return false;
+		}
+	}
+
+	return true;
+}
+
 int main(void)
 {
 	report_prefix_push("pmu");
 
 	report("Control register", check_pmcr());
+	report("Monotonically increasing cycle count", check_cycles_increase());
 
 	return report_summary();
 }
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.

[Qemu-devel] [kvm-unit-tests PATCHv6 3/3] arm: pmu: Add CPI checking

[Christopher Covington]

Calculate the numbers of cycles per instruction (CPI) implied by ARM
PMU cycle counter values. The code includes a strict checking facility
intended for the -icount option in TCG mode but it is not yet enabled
in the configuration file. Enabling it must wait on infrastructure
improvements which allow for different tests to be run on TCG versus
KVM.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 arm/pmu.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 102 insertions(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 4334de4..788886a 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -43,6 +43,23 @@ static inline unsigned long get_pmccntr(void)
 	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
 	return cycles;
 }
+
+/*
+ * Extra instructions inserted by the compiler would be difficult to compensate
+ * for, so hand assemble everything between, and including, the PMCR accesses
+ * to start and stop counting.
+ */
+static inline void loop(int i, uint32_t pmcr)
+{
+	asm volatile(
+	"	mcr	p15, 0, %[pmcr], c9, c12, 0\n"
+	"1:	subs	%[i], %[i], #1\n"
+	"	bgt	1b\n"
+	"	mcr	p15, 0, %[z], c9, c12, 0\n"
+	: [i] "+r" (i)
+	: [pmcr] "r" (pmcr), [z] "r" (0)
+	: "cc");
+}
 #elif defined(__aarch64__)
 static inline uint32_t get_pmcr(void)
 {
@@ -64,6 +81,23 @@ static inline unsigned long get_pmccntr(void)
 	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
 	return cycles;
 }
+
+/*
+ * Extra instructions inserted by the compiler would be difficult to compensate
+ * for, so hand assemble everything between, and including, the PMCR accesses
+ * to start and stop counting.
+ */
+static inline void loop(int i, uint32_t pmcr)
+{
+	asm volatile(
+	"	msr	pmcr_el0, %[pmcr]\n"
+	"1:	subs	%[i], %[i], #1\n"
+	"	b.gt	1b\n"
+	"	msr	pmcr_el0, xzr\n"
+	: [i] "+r" (i)
+	: [pmcr] "r" (pmcr)
+	: "cc");
+}
 #endif
 
 struct pmu_data {
@@ -131,12 +165,79 @@ static bool check_cycles_increase(void)
 	return true;
 }
 
-int main(void)
+/*
+ * Execute a known number of guest instructions. Only odd instruction counts
+ * greater than or equal to 3 are supported by the in-line assembly code. The
+ * control register (PMCR_EL0) is initialized with the provided value (allowing
+ * for example for the cycle counter or event counters to be reset). At the end
+ * of the exact instruction loop, zero is written to PMCR_EL0 to disable
+ * counting, allowing the cycle counter or event counters to be read at the
+ * leisure of the calling code.
+ */
+static void measure_instrs(int num, uint32_t pmcr)
+{
+	int i = (num - 1) / 2;
+
+	assert(num >= 3 && ((num - 1) % 2 == 0));
+	loop(i, pmcr);
+}
+
+/*
+ * Measure cycle counts for various known instruction counts. Ensure that the
+ * cycle counter progresses (similar to check_cycles_increase() but with more
+ * instructions and using reset and stop controls). If supplied a positive,
+ * nonzero CPI parameter, also strictly check that every measurement matches
+ * it. Strict CPI checking is used to test -icount mode.
+ */
+static bool check_cpi(int cpi)
+{
+	struct pmu_data pmu = {0};
+
+	pmu.cycle_counter_reset = 1;
+	pmu.enable = 1;
+
+	if (cpi > 0)
+		printf("Checking for CPI=%d.\n", cpi);
+	printf("instrs : cycles0 cycles1 ...\n");
+
+	for (int i = 3; i < 300; i += 32) {
+		int avg, sum = 0;
+
+		printf("%d :", i);
+		for (int j = 0; j < NR_SAMPLES; j++) {
+			int cycles;
+
+			measure_instrs(i, pmu.pmcr_el0);
+			cycles = get_pmccntr();
+			printf(" %d", cycles);
+
+			if (!cycles || (cpi > 0 && cycles != i * cpi)) {
+				printf("\n");
+				return false;
+			}
+
+			sum += cycles;
+		}
+		avg = sum / NR_SAMPLES;
+		printf(" sum=%d avg=%d avg_ipc=%d avg_cpi=%d\n",
+			sum, avg, i / avg, avg / i);
+	}
+
+	return true;
+}
+
+int main(int argc, char *argv[])
 {
+	int cpi = 0;
+
+	if (argc >= 1)
+		cpi = atol(argv[0]);
+
 	report_prefix_push("pmu");
 
 	report("Control register", check_pmcr());
 	report("Monotonically increasing cycle count", check_cycles_increase());
+	report("Cycle/instruction ratio", check_cpi(cpi));
 
 	return report_summary();
 }
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 3/3] arm: pmu: Add CPI checking

[Andrew Jones]

On Tue, Oct 11, 2016 at 02:40:44PM -0400, Christopher Covington wrote:
> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> PMU cycle counter values. The code includes a strict checking facility
> intended for the -icount option in TCG mode but it is not yet enabled
> in the configuration file. Enabling it must wait on infrastructure
> improvements which allow for different tests to be run on TCG versus
> KVM.
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>

This one should already have my r-b as well. At least I don't see any
difference from the one I reviewed way-back-when.

I'd like to merge these along with additional patches enabling running
with KVM. I think Wei was looking into that. Or, even just refreshing
these patches with KVM support being in from the beginning would be
good. Can you guys decide among yourselves how to proceed, and then
send a fresh series that works with both TCG and KVM?

Thanks,
drew

> ---
>  arm/pmu.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 102 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 4334de4..788886a 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -43,6 +43,23 @@ static inline unsigned long get_pmccntr(void)
>  	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
>  	return cycles;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)
> +{
> +	asm volatile(
> +	"	mcr	p15, 0, %[pmcr], c9, c12, 0\n"
> +	"1:	subs	%[i], %[i], #1\n"
> +	"	bgt	1b\n"
> +	"	mcr	p15, 0, %[z], c9, c12, 0\n"
> +	: [i] "+r" (i)
> +	: [pmcr] "r" (pmcr), [z] "r" (0)
> +	: "cc");
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -64,6 +81,23 @@ static inline unsigned long get_pmccntr(void)
>  	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>  	return cycles;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)
> +{
> +	asm volatile(
> +	"	msr	pmcr_el0, %[pmcr]\n"
> +	"1:	subs	%[i], %[i], #1\n"
> +	"	b.gt	1b\n"
> +	"	msr	pmcr_el0, xzr\n"
> +	: [i] "+r" (i)
> +	: [pmcr] "r" (pmcr)
> +	: "cc");
> +}
>  #endif
>  
>  struct pmu_data {
> @@ -131,12 +165,79 @@ static bool check_cycles_increase(void)
>  	return true;
>  }
>  
> -int main(void)
> +/*
> + * Execute a known number of guest instructions. Only odd instruction counts
> + * greater than or equal to 3 are supported by the in-line assembly code. The
> + * control register (PMCR_EL0) is initialized with the provided value (allowing
> + * for example for the cycle counter or event counters to be reset). At the end
> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
> + * counting, allowing the cycle counter or event counters to be read at the
> + * leisure of the calling code.
> + */
> +static void measure_instrs(int num, uint32_t pmcr)
> +{
> +	int i = (num - 1) / 2;
> +
> +	assert(num >= 3 && ((num - 1) % 2 == 0));
> +	loop(i, pmcr);
> +}
> +
> +/*
> + * Measure cycle counts for various known instruction counts. Ensure that the
> + * cycle counter progresses (similar to check_cycles_increase() but with more
> + * instructions and using reset and stop controls). If supplied a positive,
> + * nonzero CPI parameter, also strictly check that every measurement matches
> + * it. Strict CPI checking is used to test -icount mode.
> + */
> +static bool check_cpi(int cpi)
> +{
> +	struct pmu_data pmu = {0};
> +
> +	pmu.cycle_counter_reset = 1;
> +	pmu.enable = 1;
> +
> +	if (cpi > 0)
> +		printf("Checking for CPI=%d.\n", cpi);
> +	printf("instrs : cycles0 cycles1 ...\n");
> +
> +	for (int i = 3; i < 300; i += 32) {
> +		int avg, sum = 0;
> +
> +		printf("%d :", i);
> +		for (int j = 0; j < NR_SAMPLES; j++) {
> +			int cycles;
> +
> +			measure_instrs(i, pmu.pmcr_el0);
> +			cycles = get_pmccntr();
> +			printf(" %d", cycles);
> +
> +			if (!cycles || (cpi > 0 && cycles != i * cpi)) {
> +				printf("\n");
> +				return false;
> +			}
> +
> +			sum += cycles;
> +		}
> +		avg = sum / NR_SAMPLES;
> +		printf(" sum=%d avg=%d avg_ipc=%d avg_cpi=%d\n",
> +			sum, avg, i / avg, avg / i);
> +	}
> +
> +	return true;
> +}
> +
> +int main(int argc, char *argv[])
>  {
> +	int cpi = 0;
> +
> +	if (argc >= 1)
> +		cpi = atol(argv[0]);
> +
>  	report_prefix_push("pmu");
>  
>  	report("Control register", check_pmcr());
>  	report("Monotonically increasing cycle count", check_cycles_increase());
> +	report("Cycle/instruction ratio", check_cpi(cpi));
>  
>  	return report_summary();
>  }
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
> Forum, a Linux Foundation Collaborative Project.
> 
> 

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

[Wei Huang]

On 10/11/2016 01:40 PM, Christopher Covington wrote:
> Beginning with a simple sanity check of the control register, add
> a unit test for the ARM Performance Monitors Unit (PMU). As of
> October 2016, whether KVM mode has a PMU at all is a tricky
> question of which QEMU / mach-virt version is used. So only enable
> the TCG mode tests for now.

Given that we have KVM-based vPMU enabled in QEMU, it is very desirable
for these unit testing cases to work on KVM. Right now both
check_cycles_increase() and check_cpi() fail under KVM. I will help
debug it.

> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/Makefile.common |  3 +-
>  arm/pmu.c           | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg   | 14 +++++++++
>  3 files changed, 98 insertions(+), 1 deletion(-)
>  create mode 100644 arm/pmu.c
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index ccb554d..f98f422 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -11,7 +11,8 @@ endif
>  
>  tests-common = \
>  	$(TEST_DIR)/selftest.flat \
> -	$(TEST_DIR)/spinlock-test.flat
> +	$(TEST_DIR)/spinlock-test.flat \
> +	$(TEST_DIR)/pmu.flat
>  
>  all: test_cases
>  
> diff --git a/arm/pmu.c b/arm/pmu.c
> new file mode 100644
> index 0000000..42d0ee1
> --- /dev/null
> +++ b/arm/pmu.c
> @@ -0,0 +1,82 @@
> +/*
> + * Test the ARM Performance Monitors Unit (PMU).
> + *
> + * Copyright 2015 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU Lesser General Public License version 2.1 and
> + * only version 2.1 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
> + * for more details.
> + */
> +#include "libcflat.h"
> +
> +#if defined(__arm__)
> +static inline uint32_t get_pmcr(void)
> +{
> +	uint32_t ret;
> +
> +	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> +	return ret;
> +}
> +#elif defined(__aarch64__)
> +static inline uint32_t get_pmcr(void)
> +{
> +	uint32_t ret;
> +
> +	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
> +	return ret;
> +}
> +#endif
> +
> +struct pmu_data {
> +	union {
> +		uint32_t pmcr_el0;
> +		struct {
> +			uint32_t enable:1;
> +			uint32_t event_counter_reset:1;
> +			uint32_t cycle_counter_reset:1;
> +			uint32_t cycle_counter_clock_divider:1;
> +			uint32_t event_counter_export:1;
> +			uint32_t cycle_counter_disable_when_prohibited:1;
> +			uint32_t cycle_counter_long:1;
> +			uint32_t reserved:4;
> +			uint32_t counters:5;
> +			uint32_t identification_code:8;
> +			uint32_t implementer:8;
> +		};
> +	};
> +};
> +
> +/*
> + * As a simple sanity check on the PMCR_EL0, ensure the implementer field isn't
> + * null. Also print out a couple other interesting fields for diagnostic
> + * purposes. For example, as of fall 2015, QEMU TCG mode doesn't implement
> + * event counters and therefore reports zero event counters, but hopefully
> + * support for at least the instructions event will be added in the future and
> + * the reported number of event counters will become nonzero.
> + */
> +static bool check_pmcr(void)
> +{
> +	struct pmu_data pmu;
> +
> +	pmu.pmcr_el0 = get_pmcr();
> +
> +	printf("PMU implementer:     %c\n", pmu.implementer);
> +	printf("Identification code: 0x%x\n", pmu.identification_code);
> +	printf("Event counters:      %d\n", pmu.counters);
> +
> +	return pmu.implementer != 0;
> +}
> +
> +int main(void)
> +{
> +	report_prefix_push("pmu");
> +
> +	report("Control register", check_pmcr());
> +
> +	return report_summary();
> +}
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index ffd12e5..edaed4a 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -51,3 +51,17 @@ file = selftest.flat
>  smp = $MAX_SMP
>  extra_params = -append 'smp'
>  groups = selftest
> +
> +# Test PMU support with -icount IPC=1
> +[pmu-icount-1]
> +file = pmu.flat
> +extra_params = -icount 0 -append '1'
> +groups = pmu
> +accel = tcg
> +
> +# Test PMU support with -icount IPC=256
> +[pmu-icount-256]
> +file = pmu.flat
> +extra_params = -icount 8 -append '256'
> +groups = pmu
> +accel = tcg
> 

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

On 10/11/2016 01:40 PM, Christopher Covington wrote:
> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> even for the smallest delta of two subsequent reads.
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/pmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 42d0ee1..4334de4 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,8 @@
>   */
>  #include "libcflat.h"
>  
> +#define NR_SAMPLES 10
> +
>  #if defined(__arm__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
>  	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>  	return ret;
>  }
> +
> +static inline void set_pmcr(uint32_t pmcr)
> +{
> +	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
> +}
> +
> +/*
> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 64
> + * bits doesn't seem worth the trouble when differential usage of the result is
> + * expected (with differences that can easily fit in 32 bits). So just return
> + * the lower 32 bits of the cycle count in AArch32.
> + */
> +static inline unsigned long get_pmccntr(void)
> +{
> +	unsigned long cycles;
> +
> +	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
> +	return cycles;
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
>  	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>  	return ret;
>  }
> +
> +static inline void set_pmcr(uint32_t pmcr)
> +{
> +	asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
> +}
> +
> +static inline unsigned long get_pmccntr(void)
> +{
> +	unsigned long cycles;
> +
> +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> +	return cycles;
> +}
>  #endif
>  
>  struct pmu_data {
> @@ -72,11 +106,37 @@ static bool check_pmcr(void)
>  	return pmu.implementer != 0;
>  }
>  
> +/*
> + * Ensure that the cycle counter progresses between back-to-back reads.
> + */
> +static bool check_cycles_increase(void)
> +{
> +	struct pmu_data pmu = {0};

Compilation error on my machine:

arm/pmu.c: In function ‘check_cycles_increase’:
arm/pmu.c:148:9: error: missing braces around initializer
[-Werror=missing-braces]
  struct pmu_data pmu = {0};

Same for Patch 3.


> +
> +	pmu.enable = 1;
> +	set_pmcr(pmu.pmcr_el0);
> +
> +	for (int i = 0; i < NR_SAMPLES; i++) {
> +		unsigned long a, b;
> +
> +		a = get_pmccntr();
> +		b = get_pmccntr();
> +
> +		if (a >= b) {
> +			printf("Read %ld then %ld.\n", a, b);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  int main(void)
>  {
>  	report_prefix_push("pmu");
>  
>  	report("Control register", check_pmcr());
> +	report("Monotonically increasing cycle count", check_cycles_increase());
>  
>  	return report_summary();
>  }
> 

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 2/3] arm: pmu: Check cycle count increases

[Christopher Covington]

Hi Wei,

On 10/12/2016 11:49 AM, Wei Huang wrote:
> On 10/11/2016 01:40 PM, Christopher Covington wrote:
>> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
>> even for the smallest delta of two subsequent reads.
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  arm/pmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 60 insertions(+)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 42d0ee1..4334de4 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -14,6 +14,8 @@
>>   */
>>  #include "libcflat.h"
>>  
>> +#define NR_SAMPLES 10
>> +
>>  #if defined(__arm__)
>>  static inline uint32_t get_pmcr(void)
>>  {
>> @@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
>>  	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>>  	return ret;
>>  }
>> +
>> +static inline void set_pmcr(uint32_t pmcr)
>> +{
>> +	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
>> +}
>> +
>> +/*
>> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 64
>> + * bits doesn't seem worth the trouble when differential usage of the result is
>> + * expected (with differences that can easily fit in 32 bits). So just return
>> + * the lower 32 bits of the cycle count in AArch32.
>> + */
>> +static inline unsigned long get_pmccntr(void)
>> +{
>> +	unsigned long cycles;
>> +
>> +	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
>> +	return cycles;
>> +}
>>  #elif defined(__aarch64__)
>>  static inline uint32_t get_pmcr(void)
>>  {
>> @@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
>>  	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>>  	return ret;
>>  }
>> +
>> +static inline void set_pmcr(uint32_t pmcr)
>> +{
>> +	asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
>> +}
>> +
>> +static inline unsigned long get_pmccntr(void)
>> +{
>> +	unsigned long cycles;
>> +
>> +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>> +	return cycles;
>> +}
>>  #endif
>>  
>>  struct pmu_data {
>> @@ -72,11 +106,37 @@ static bool check_pmcr(void)
>>  	return pmu.implementer != 0;
>>  }
>>  
>> +/*
>> + * Ensure that the cycle counter progresses between back-to-back reads.
>> + */
>> +static bool check_cycles_increase(void)
>> +{
>> +	struct pmu_data pmu = {0};
> 
> Compilation error on my machine:
> 
> arm/pmu.c: In function ‘check_cycles_increase’:
> arm/pmu.c:148:9: error: missing braces around initializer
> [-Werror=missing-braces]
>   struct pmu_data pmu = {0};
> 
> Same for Patch 3.

"...So your compiler complains about {0}? Is there a problem besides the
warning? If not, then I'm still a bit inclined to keep the code neat. The
warnings will go away with compiler updates."

https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06064.html

Thanks,
Cov
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

On 10/12/2016 01:10 PM, Christopher Covington wrote:
> Hi Wei,
> 
> On 10/12/2016 11:49 AM, Wei Huang wrote:
>> On 10/11/2016 01:40 PM, Christopher Covington wrote:
>>> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
>>> even for the smallest delta of two subsequent reads.
>>>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  arm/pmu.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 60 insertions(+)
>>>
>>> diff --git a/arm/pmu.c b/arm/pmu.c
>>> index 42d0ee1..4334de4 100644
>>> --- a/arm/pmu.c
>>> +++ b/arm/pmu.c
>>> @@ -14,6 +14,8 @@
>>>   */
>>>  #include "libcflat.h"
>>>  
>>> +#define NR_SAMPLES 10
>>> +
>>>  #if defined(__arm__)
>>>  static inline uint32_t get_pmcr(void)
>>>  {
>>> @@ -22,6 +24,25 @@ static inline uint32_t get_pmcr(void)
>>>  	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>>>  	return ret;
>>>  }
>>> +
>>> +static inline void set_pmcr(uint32_t pmcr)
>>> +{
>>> +	asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (pmcr));
>>> +}
>>> +
>>> +/*
>>> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 64
>>> + * bits doesn't seem worth the trouble when differential usage of the result is
>>> + * expected (with differences that can easily fit in 32 bits). So just return
>>> + * the lower 32 bits of the cycle count in AArch32.
>>> + */
>>> +static inline unsigned long get_pmccntr(void)
>>> +{
>>> +	unsigned long cycles;
>>> +
>>> +	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
>>> +	return cycles;
>>> +}
>>>  #elif defined(__aarch64__)
>>>  static inline uint32_t get_pmcr(void)
>>>  {
>>> @@ -30,6 +51,19 @@ static inline uint32_t get_pmcr(void)
>>>  	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>>>  	return ret;
>>>  }
>>> +
>>> +static inline void set_pmcr(uint32_t pmcr)
>>> +{
>>> +	asm volatile("msr pmcr_el0, %0" : : "r" (pmcr));
>>> +}
>>> +
>>> +static inline unsigned long get_pmccntr(void)
>>> +{
>>> +	unsigned long cycles;
>>> +
>>> +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>>> +	return cycles;
>>> +}
>>>  #endif
>>>  
>>>  struct pmu_data {
>>> @@ -72,11 +106,37 @@ static bool check_pmcr(void)
>>>  	return pmu.implementer != 0;
>>>  }
>>>  
>>> +/*
>>> + * Ensure that the cycle counter progresses between back-to-back reads.
>>> + */
>>> +static bool check_cycles_increase(void)
>>> +{
>>> +	struct pmu_data pmu = {0};
>>
>> Compilation error on my machine:
>>
>> arm/pmu.c: In function ‘check_cycles_increase’:
>> arm/pmu.c:148:9: error: missing braces around initializer
>> [-Werror=missing-braces]
>>   struct pmu_data pmu = {0};
>>
>> Same for Patch 3.
> 
> "...So your compiler complains about {0}? Is there a problem besides the
> warning? If not, then I'm still a bit inclined to keep the code neat. The
> warnings will go away with compiler updates."

Indeed my stock GCC compiler is a bit old - 4.8.5; newer version can fix
it. But note this a compilation error which prevents the binary from
being built and will last a while. Could we use double-braces, pmu =
{{0}}, as a solution?

-Wei

> 
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06064.html
> 
> Thanks,
> Cov
> 

Re: [Qemu-devel] [kvm-unit-tests PATCHv6 3/3] arm: pmu: Add CPI checking

[Wei Huang]

On 10/12/2016 07:05 AM, Andrew Jones wrote:
> On Tue, Oct 11, 2016 at 02:40:44PM -0400, Christopher Covington wrote:
>> Calculate the numbers of cycles per instruction (CPI) implied by ARM
>> PMU cycle counter values. The code includes a strict checking facility
>> intended for the -icount option in TCG mode but it is not yet enabled
>> in the configuration file. Enabling it must wait on infrastructure
>> improvements which allow for different tests to be run on TCG versus
>> KVM.
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> This one should already have my r-b as well. At least I don't see any
> difference from the one I reviewed way-back-when.
> 
> I'd like to merge these along with additional patches enabling running
> with KVM. I think Wei was looking into that. Or, even just refreshing
> these patches with KVM support being in from the beginning would be
> good. Can you guys decide among yourselves how to proceed, and then
> send a fresh series that works with both TCG and KVM?

Here is the issues I found so far. After they are fixed, KVM mode can be
supported.

Basically, as stated by Cov, it failed under KVM mode with 2 out of 3
tests (cycle-increase, cpi). The main problem is that: reading PMCCNTR
always returns the same value; so the delta is always 0 between two
cycle_count reads, causing failures in unit tests.

Debugging it showed that both unit-testing (pmu.c) and kvm-vpmu code
have issues. Details below:

1. kvm-unit-tests/arm/pmu.c
* Bit 31 of PMCNTENSET_EL0 needs to be set 1. According to ARM doc,
PMCNTENSET_EL0 register "enables the Cycle Count Register, PMCCNTR_EL0,
and any implemented event counters PMEVCNTR<n>_EL0..." Without it, KVM
vPMU can't enables cycle counter.

* The PMCCFILTER needs to be setup for KVM to create perf_event for
cycle counter. A fix like below should be enough.

+       /* init PMCCFILTER */
+       val = 0;
+       asm volatile("msr pmccfiltr_el0, %0" :: "r" (val));

Note: this requirement shouldn't be needed. I think it is a KVM's bug
(see analysis below)

2. virt/kvm/arm/pmu.c (KVM)
kvm_pmu_set_counter_event_type() has some bugs and wrong assumption.

* First of all, KVM vPMU kvm_pmu_set_counter_event_type() is called to
create perf_event when PMXEVTYPER_EL0 is set. This is OK for
PMEVCNTRn_EL0. But unlike PMEVCNTRn_EL0, setting event type shouldn't be
needed for PMCCNTR; instead PMCCNTR should be always available to read.
In other words, KVM should create a perf_event and make it
available for reading PMCCNTR.

* Secondly, ARMV8_PMU_EVTYPE_EVENT_SW_INCR is 0, same as PMCCFILTER
reserved bits. So KVM shouldn't return if eventsel is
ARMV8_PMU_EVTYPE_EVENT_SW_INCR. Instead it needs to further check if
select_idx is ARMV8_PMU_CYCLE_IDX or not. See below:

-       if (eventsel == ARMV8_PMU_EVTYPE_EVENT_SW_INCR)
+       if (eventsel == ARMV8_PMU_EVTYPE_EVENT_SW_INCR &&
+           select_idx != ARMV8_PMU_CYCLE_IDX)
                return;

* perf_event.attr configuration shouldn't copy eventsel bits of
PMCCFILTER directly. Instead KVM should convert it to type 0x11 (i.e.
the "cpu cycle" event type) when guest reading cpu cycle is detected.

I will send out patches to address the problems above.

Thanks,
-Wei

> 
> Thanks,
> drew
> 
>> ---
>>  arm/pmu.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 102 insertions(+), 1 deletion(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 4334de4..788886a 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -43,6 +43,23 @@ static inline unsigned long get_pmccntr(void)
>>  	asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
>>  	return cycles;
>>  }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to compensate
>> + * for, so hand assemble everything between, and including, the PMCR accesses
>> + * to start and stop counting.
>> + */
>> +static inline void loop(int i, uint32_t pmcr)
>> +{
>> +	asm volatile(
>> +	"	mcr	p15, 0, %[pmcr], c9, c12, 0\n"
>> +	"1:	subs	%[i], %[i], #1\n"
>> +	"	bgt	1b\n"
>> +	"	mcr	p15, 0, %[z], c9, c12, 0\n"
>> +	: [i] "+r" (i)
>> +	: [pmcr] "r" (pmcr), [z] "r" (0)
>> +	: "cc");
>> +}
>>  #elif defined(__aarch64__)
>>  static inline uint32_t get_pmcr(void)
>>  {
>> @@ -64,6 +81,23 @@ static inline unsigned long get_pmccntr(void)
>>  	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
>>  	return cycles;
>>  }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to compensate
>> + * for, so hand assemble everything between, and including, the PMCR accesses
>> + * to start and stop counting.
>> + */
>> +static inline void loop(int i, uint32_t pmcr)
>> +{
>> +	asm volatile(
>> +	"	msr	pmcr_el0, %[pmcr]\n"
>> +	"1:	subs	%[i], %[i], #1\n"
>> +	"	b.gt	1b\n"
>> +	"	msr	pmcr_el0, xzr\n"
>> +	: [i] "+r" (i)
>> +	: [pmcr] "r" (pmcr)
>> +	: "cc");
>> +}
>>  #endif
>>  
>>  struct pmu_data {
>> @@ -131,12 +165,79 @@ static bool check_cycles_increase(void)
>>  	return true;
>>  }
>>  
>> -int main(void)
>> +/*
>> + * Execute a known number of guest instructions. Only odd instruction counts
>> + * greater than or equal to 3 are supported by the in-line assembly code. The
>> + * control register (PMCR_EL0) is initialized with the provided value (allowing
>> + * for example for the cycle counter or event counters to be reset). At the end
>> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
>> + * counting, allowing the cycle counter or event counters to be read at the
>> + * leisure of the calling code.
>> + */
>> +static void measure_instrs(int num, uint32_t pmcr)
>> +{
>> +	int i = (num - 1) / 2;
>> +
>> +	assert(num >= 3 && ((num - 1) % 2 == 0));
>> +	loop(i, pmcr);
>> +}
>> +
>> +/*
>> + * Measure cycle counts for various known instruction counts. Ensure that the
>> + * cycle counter progresses (similar to check_cycles_increase() but with more
>> + * instructions and using reset and stop controls). If supplied a positive,
>> + * nonzero CPI parameter, also strictly check that every measurement matches
>> + * it. Strict CPI checking is used to test -icount mode.
>> + */
>> +static bool check_cpi(int cpi)
>> +{
>> +	struct pmu_data pmu = {0};
>> +
>> +	pmu.cycle_counter_reset = 1;
>> +	pmu.enable = 1;
>> +
>> +	if (cpi > 0)
>> +		printf("Checking for CPI=%d.\n", cpi);
>> +	printf("instrs : cycles0 cycles1 ...\n");
>> +
>> +	for (int i = 3; i < 300; i += 32) {
>> +		int avg, sum = 0;
>> +
>> +		printf("%d :", i);
>> +		for (int j = 0; j < NR_SAMPLES; j++) {
>> +			int cycles;
>> +
>> +			measure_instrs(i, pmu.pmcr_el0);
>> +			cycles = get_pmccntr();
>> +			printf(" %d", cycles);
>> +
>> +			if (!cycles || (cpi > 0 && cycles != i * cpi)) {
>> +				printf("\n");
>> +				return false;
>> +			}
>> +
>> +			sum += cycles;
>> +		}
>> +		avg = sum / NR_SAMPLES;
>> +		printf(" sum=%d avg=%d avg_ipc=%d avg_cpi=%d\n",
>> +			sum, avg, i / avg, avg / i);
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +int main(int argc, char *argv[])
>>  {
>> +	int cpi = 0;
>> +
>> +	if (argc >= 1)
>> +		cpi = atol(argv[0]);
>> +
>>  	report_prefix_push("pmu");
>>  
>>  	report("Control register", check_pmcr());
>>  	report("Monotonically increasing cycle count", check_cycles_increase());
>> +	report("Cycle/instruction ratio", check_cpi(cpi));
>>  
>>  	return report_summary();
>>  }
>> -- 
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
>> Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora
>> Forum, a Linux Foundation Collaborative Project.
>>
>>
>