[Qemu-devel] [kvm-unit-tests PATCHv7 0/3] ARM PMU tests

[Qemu-devel] [kvm-unit-tests PATCHv7 0/3] ARM PMU tests

[Wei Huang]

Changes from v6:
* Add a new pmu testing for KVM mode in config file
* Add additional init code, including setting PMCNTENSET and PMCCFILTR, 
  before reading PMCCNTR. ARMv7 support is also provided
* Add cycle counter init code for CPI testing
* Fix pmu_data compilation issue (for gcc 4.8.5)
* Commit comments were updated

NOTE:
1) Current KVM code has bugs in handling PMCCFILTR write. A fix (see
below) is required for this unit testing code to work correctly under
KVM mode.
https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022134.html.
2) Because the code was changed, Drew's original reviewed-by needs to
be acknowledged by him again.

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

[Wei Huang]

Beginning with a simple sanity check of the control register, add
a unit test for the ARM Performance Monitors Unit (PMU).

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 arm/Makefile.common |  3 +-
 arm/pmu.c           | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg   | 20 +++++++++++++
 3 files changed, 104 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 3f6fa45..b647b69 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -54,3 +54,23 @@ file = selftest.flat
 smp = $MAX_SMP
 extra_params = -append 'smp'
 groups = selftest
+
+# Test PMU support (KVM)
+[pmu-kvm]
+file = pmu.flat
+groups = pmu
+accel = kvm
+
+# Test PMU support (TCG) with -icount IPC=1
+[pmu-tcg-icount-1]
+file = pmu.flat
+extra_params = -icount 0 -append '1'
+groups = pmu
+accel = tcg
+
+# Test PMU support (TCG) with -icount IPC=256
+[pmu-tcg-icount-256]
+file = pmu.flat
+extra_params = -icount 8 -append '256'
+groups = pmu
+accel = tcg
-- 
1.8.3.1

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

[Andrew Jones]

On Wed, Nov 02, 2016 at 05:22:15PM -0500, Wei Huang wrote:

Missing
 From: Christopher Covington <cov@codeaurora.org>


> Beginning with a simple sanity check of the control register, add
> a unit test for the ARM Performance Monitors Unit (PMU).
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>

Missing
  Signed-off-by: Wei Huang <wei@redhat.com>

> ---
>  arm/Makefile.common |  3 +-
>  arm/pmu.c           | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg   | 20 +++++++++++++
>  3 files changed, 104 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.

Is the Linux Foundation correct for codeaurora patches? Should bump
the year to 2016.

> + *
> + * 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;
> +		};
> +	};
> +};

I know I already reviewed/agreed to this bitfield, but I'm having second
thoughts. I think I'd prefer a bunch of defined shifts like the kernel uses.

> +
> +/*
> + * 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

s/2015/2016/   how time flies...

> + * 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 3f6fa45..b647b69 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -54,3 +54,23 @@ file = selftest.flat
>  smp = $MAX_SMP
>  extra_params = -append 'smp'
>  groups = selftest
> +
> +# Test PMU support (KVM)
> +[pmu-kvm]
> +file = pmu.flat
> +groups = pmu
> +accel = kvm

No need to specify kvm when it works for both. Both is assumed.
tcg-only or kvm-only tests are exceptions requiring the 'accel'
parameter and a comment explaining why it doesn't work on the
other.

> +
> +# Test PMU support (TCG) with -icount IPC=1
> +[pmu-tcg-icount-1]
> +file = pmu.flat
> +extra_params = -icount 0 -append '1'
> +groups = pmu
> +accel = tcg
> +
> +# Test PMU support (TCG) with -icount IPC=256
> +[pmu-tcg-icount-256]
> +file = pmu.flat
> +extra_params = -icount 8 -append '256'
> +groups = pmu
> +accel = tcg

Why are these entries added now. These tests aren't yet implemented.

Thanks,
drew

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

[cov]

On 2016-11-03 04:14, Andrew Jones wrote:
> On Wed, Nov 02, 2016 at 05:22:15PM -0500, Wei Huang wrote:
> 
> Missing
>  From: Christopher Covington <cov@codeaurora.org>
> 
> 
>> Beginning with a simple sanity check of the control register, add
>> a unit test for the ARM Performance Monitors Unit (PMU).
>> 
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> 
> Missing
>   Signed-off-by: Wei Huang <wei@redhat.com>
> 
>> ---
>>  arm/Makefile.common |  3 +-
>>  arm/pmu.c           | 82 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg   | 20 +++++++++++++
>>  3 files changed, 104 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.
> 
> Is the Linux Foundation correct for codeaurora patches? Should bump
> the year to 2016.
> 
>> + *
>> + * 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;
>> +		};
>> +	};
>> +};
> 
> I know I already reviewed/agreed to this bitfield, but I'm having 
> second
> thoughts. I think I'd prefer a bunch of defined shifts like the kernel 
> uses.
> 
>> +
>> +/*
>> + * 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
> 
> s/2015/2016/   how time flies...
> 
>> + * 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 3f6fa45..b647b69 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -54,3 +54,23 @@ file = selftest.flat
>>  smp = $MAX_SMP
>>  extra_params = -append 'smp'
>>  groups = selftest
>> +
>> +# Test PMU support (KVM)
>> +[pmu-kvm]
>> +file = pmu.flat
>> +groups = pmu
>> +accel = kvm
> 
> No need to specify kvm when it works for both. Both is assumed.
> tcg-only or kvm-only tests are exceptions requiring the 'accel'
> parameter and a comment explaining why it doesn't work on the
> other.
> 
>> +
>> +# Test PMU support (TCG) with -icount IPC=1
>> +[pmu-tcg-icount-1]
>> +file = pmu.flat
>> +extra_params = -icount 0 -append '1'
>> +groups = pmu
>> +accel = tcg
>> +
>> +# Test PMU support (TCG) with -icount IPC=256
>> +[pmu-tcg-icount-256]
>> +file = pmu.flat
>> +extra_params = -icount 8 -append '256'
>> +groups = pmu
>> +accel = tcg
> 
> Why are these entries added now. These tests aren't yet implemented.

What makes you say they aren't implemented? They're running the
same binary with a different command line arguments (that turns on
stricter TCG-specific checking).

Thanks,
Cov

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

[Andrew Jones]

On Thu, Nov 03, 2016 at 08:29:57AM -0600, cov@codeaurora.org wrote:
> On 2016-11-03 04:14, Andrew Jones wrote:
> > On Wed, Nov 02, 2016 at 05:22:15PM -0500, Wei Huang wrote:
> > 
> > Missing
> >  From: Christopher Covington <cov@codeaurora.org>
> > 
> > 
> > > Beginning with a simple sanity check of the control register, add
> > > a unit test for the ARM Performance Monitors Unit (PMU).
> > > 
> > > Signed-off-by: Christopher Covington <cov@codeaurora.org>
> > 
> > Missing
> >   Signed-off-by: Wei Huang <wei@redhat.com>
> > 
> > > ---
> > >  arm/Makefile.common |  3 +-
> > >  arm/pmu.c           | 82
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  arm/unittests.cfg   | 20 +++++++++++++
> > >  3 files changed, 104 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.
> > 
> > Is the Linux Foundation correct for codeaurora patches? Should bump
> > the year to 2016.
> > 
> > > + *
> > > + * 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;
> > > +		};
> > > +	};
> > > +};
> > 
> > I know I already reviewed/agreed to this bitfield, but I'm having second
> > thoughts. I think I'd prefer a bunch of defined shifts like the kernel
> > uses.
> > 
> > > +
> > > +/*
> > > + * 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
> > 
> > s/2015/2016/   how time flies...
> > 
> > > + * 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 3f6fa45..b647b69 100644
> > > --- a/arm/unittests.cfg
> > > +++ b/arm/unittests.cfg
> > > @@ -54,3 +54,23 @@ file = selftest.flat
> > >  smp = $MAX_SMP
> > >  extra_params = -append 'smp'
> > >  groups = selftest
> > > +
> > > +# Test PMU support (KVM)
> > > +[pmu-kvm]
> > > +file = pmu.flat
> > > +groups = pmu
> > > +accel = kvm
> > 
> > No need to specify kvm when it works for both. Both is assumed.
> > tcg-only or kvm-only tests are exceptions requiring the 'accel'
> > parameter and a comment explaining why it doesn't work on the
> > other.
> > 
> > > +
> > > +# Test PMU support (TCG) with -icount IPC=1
> > > +[pmu-tcg-icount-1]
> > > +file = pmu.flat
> > > +extra_params = -icount 0 -append '1'
> > > +groups = pmu
> > > +accel = tcg
> > > +
> > > +# Test PMU support (TCG) with -icount IPC=256
> > > +[pmu-tcg-icount-256]
> > > +file = pmu.flat
> > > +extra_params = -icount 8 -append '256'
> > > +groups = pmu
> > > +accel = tcg
> > 
> > Why are these entries added now. These tests aren't yet implemented.
> 
> What makes you say they aren't implemented? They're running the
> same binary with a different command line arguments (that turns on
> stricter TCG-specific checking).

Not in this patch, they're not. 'int main(void)' <-- arguments are
ignored. Please only introduce unittests.cfg blocks with the patch
that implements them.

Thanks,
drew

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

[cov]

On 2016-11-03 09:04, Andrew Jones wrote:
> On Thu, Nov 03, 2016 at 08:29:57AM -0600, cov@codeaurora.org wrote:
>> On 2016-11-03 04:14, Andrew Jones wrote:
>> > On Wed, Nov 02, 2016 at 05:22:15PM -0500, Wei Huang wrote:
>> >
>> > Missing
>> >  From: Christopher Covington <cov@codeaurora.org>
>> >
>> >
>> > > Beginning with a simple sanity check of the control register, add
>> > > a unit test for the ARM Performance Monitors Unit (PMU).
>> > >
>> > > Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> >
>> > Missing
>> >   Signed-off-by: Wei Huang <wei@redhat.com>
>> >
>> > > ---
>> > >  arm/Makefile.common |  3 +-
>> > >  arm/pmu.c           | 82
>> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++
>> > >  arm/unittests.cfg   | 20 +++++++++++++
>> > >  3 files changed, 104 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.
>> >
>> > Is the Linux Foundation correct for codeaurora patches? Should bump
>> > the year to 2016.
>> >
>> > > + *
>> > > + * 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;
>> > > +		};
>> > > +	};
>> > > +};
>> >
>> > I know I already reviewed/agreed to this bitfield, but I'm having second
>> > thoughts. I think I'd prefer a bunch of defined shifts like the kernel
>> > uses.
>> >
>> > > +
>> > > +/*
>> > > + * 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
>> >
>> > s/2015/2016/   how time flies...
>> >
>> > > + * 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 3f6fa45..b647b69 100644
>> > > --- a/arm/unittests.cfg
>> > > +++ b/arm/unittests.cfg
>> > > @@ -54,3 +54,23 @@ file = selftest.flat
>> > >  smp = $MAX_SMP
>> > >  extra_params = -append 'smp'
>> > >  groups = selftest
>> > > +
>> > > +# Test PMU support (KVM)
>> > > +[pmu-kvm]
>> > > +file = pmu.flat
>> > > +groups = pmu
>> > > +accel = kvm
>> >
>> > No need to specify kvm when it works for both. Both is assumed.
>> > tcg-only or kvm-only tests are exceptions requiring the 'accel'
>> > parameter and a comment explaining why it doesn't work on the
>> > other.
>> >
>> > > +
>> > > +# Test PMU support (TCG) with -icount IPC=1
>> > > +[pmu-tcg-icount-1]
>> > > +file = pmu.flat
>> > > +extra_params = -icount 0 -append '1'
>> > > +groups = pmu
>> > > +accel = tcg
>> > > +
>> > > +# Test PMU support (TCG) with -icount IPC=256
>> > > +[pmu-tcg-icount-256]
>> > > +file = pmu.flat
>> > > +extra_params = -icount 8 -append '256'
>> > > +groups = pmu
>> > > +accel = tcg
>> >
>> > Why are these entries added now. These tests aren't yet implemented.
>> 
>> What makes you say they aren't implemented? They're running the
>> same binary with a different command line arguments (that turns on
>> stricter TCG-specific checking).
> 
> Not in this patch, they're not. 'int main(void)' <-- arguments are
> ignored. Please only introduce unittests.cfg blocks with the patch
> that implements them.

Whoops, that's a rebase error. Sorry about that.

Cov

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

[Wei Huang]

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>
Signed-off-by: Wei Huang <wei@redhat.com>
---
 arm/pmu.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 100 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 42d0ee1..65b7df1 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -14,6 +14,9 @@
  */
 #include "libcflat.h"
 
+#define NR_SAMPLES 10
+#define ARMV8_PMU_CYCLE_IDX 31
+
 #if defined(__arm__)
 static inline uint32_t get_pmcr(void)
 {
@@ -22,6 +25,43 @@ 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));
+}
+
+static inline void set_pmccfiltr(uint32_t filter)
+{
+	uint32_t cycle_idx = ARMV8_PMU_CYCLE_IDX;
+
+	asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (cycle_idx));
+	asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (filter));
+}
+
+/*
+ * 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;
+}
+
+static inline void enable_counter(uint32_t idx)
+{
+	asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
+}
+
+static inline void disable_counter(uint32_t idx)
+{
+	asm volatile("mrc p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
+}
 #elif defined(__aarch64__)
 static inline uint32_t get_pmcr(void)
 {
@@ -30,6 +70,34 @@ 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 void set_pmccfiltr(uint32_t filter)
+{
+	asm volatile("msr pmccfiltr_el0, %0" : : "r" (filter));
+}
+
+static inline unsigned long get_pmccntr(void)
+{
+	unsigned long cycles;
+
+	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
+	return cycles;
+}
+
+static inline void enable_counter(uint32_t idx)
+{
+	asm volatile("msr pmcntenset_el0, %0" : : "r" (1 << idx));
+}
+
+static inline void disable_counter(uint32_t idx)
+{
+	asm volatile("msr pmcntensclr_el0, %0" : : "r" (1 << idx));
+}
 #endif
 
 struct pmu_data {
@@ -72,11 +140,43 @@ 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}};
+
+	enable_counter(ARMV8_PMU_CYCLE_IDX);
+	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+
+	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;
+		}
+	}
+
+	pmu.enable = 0;
+	set_pmcr(pmu.pmcr_el0);
+
+	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();
 }
-- 
1.8.3.1

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

[cov]

Hi Wei,

Thanks for your work on this.

On 2016-11-02 16:22, Wei Huang 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>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  arm/pmu.c | 100 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 42d0ee1..65b7df1 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,9 @@
>   */
>  #include "libcflat.h"
> 
> +#define NR_SAMPLES 10
> +#define ARMV8_PMU_CYCLE_IDX 31
> +
>  #if defined(__arm__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -22,6 +25,43 @@ 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));
> +}
> +
> +static inline void set_pmccfiltr(uint32_t filter)
> +{
> +	uint32_t cycle_idx = ARMV8_PMU_CYCLE_IDX;
> +
> +	asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (cycle_idx));
> +	asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (filter));
> +}

Down the road I'd like to add tests for the regular events. What if you 
added
separate PMSELR and PMXEVTYPER accessors now and used them (with 
PMSELR.SEL = 31)
for setting PMCCFILTR? Then we wouldn't need a specialized set_pmccfiltr
function for the cycle counter versus PMSELR and PMXEVTYPER for the 
regular events.

> +/*
> + * 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;
> +}
> +
> +static inline void enable_counter(uint32_t idx)
> +{
> +	asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
> +}

My personal preference, that I think would make this function look and
act like the other system register accessor functions, would be to
name the function "set_pmcntenset" and do a plain write of the input
parameter without a shift, letting the shift be done in the C code.
(As we scale up, the system register accessor functions should probably
be generated by macros from a concise table.)

> +static inline void disable_counter(uint32_t idx)
> +{
> +	asm volatile("mrc p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
> +}

This function doesn't seem to be used yet. Consider whether it might
make sense to defer introducing it until there is a user.

>  #elif defined(__aarch64__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -30,6 +70,34 @@ 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 void set_pmccfiltr(uint32_t filter)
> +{
> +	asm volatile("msr pmccfiltr_el0, %0" : : "r" (filter));
> +}

As above, consider whether using PMSELR and PMXEVTYPER might be a more
reusable pair of accessors.

> +static inline unsigned long get_pmccntr(void)
> +{
> +	unsigned long cycles;
> +
> +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> +	return cycles;
> +}
> +
> +static inline void enable_counter(uint32_t idx)
> +{
> +	asm volatile("msr pmcntenset_el0, %0" : : "r" (1 << idx));
> +}

Same thought as above about uniformity and generatability.

> +static inline void disable_counter(uint32_t idx)
> +{
> +	asm volatile("msr pmcntensclr_el0, %0" : : "r" (1 << idx));
> +}

As above, this function doesn't seem to be used yet.

Thanks,
Cov

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

[Andrew Jones]

On Wed, Nov 02, 2016 at 05:22:16PM -0500, Wei Huang wrote:

Missing From: Christopher

> 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>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  arm/pmu.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 100 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 42d0ee1..65b7df1 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,9 @@
>   */
>  #include "libcflat.h"
>  
> +#define NR_SAMPLES 10
> +#define ARMV8_PMU_CYCLE_IDX 31

Now I'd really like to drop the bitfields and go to bit defines if we're
going to start mixing them anyway...

> +
>  #if defined(__arm__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -22,6 +25,43 @@ 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));
> +}
> +
> +static inline void set_pmccfiltr(uint32_t filter)
> +{
> +	uint32_t cycle_idx = ARMV8_PMU_CYCLE_IDX;
> +
> +	asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (cycle_idx));
> +	asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (filter));
> +}

I agree with Chistopher that we should create per-register accessors.
Enabling the test code the flexibility to do what it wants but without
requiring a bunch of armv7/armv8 sysreg clutter.

> +
> +/*
> + * 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

Except for the fact this is a test and we should confirm those upper bits
are whatever we expect, even if we expect zero.

> + * 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;
> +}
> +
> +static inline void enable_counter(uint32_t idx)
> +{
> +	asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
> +}
> +
> +static inline void disable_counter(uint32_t idx)
> +{
> +	asm volatile("mrc p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
> +}

I'll echo Chistopher here as well with the opinion that the test
should supply the shifted value and the accessor should just be a
simply read or write.

Please name the accessors <regname>_read/write

>  #elif defined(__aarch64__)
>  static inline uint32_t get_pmcr(void)
>  {
> @@ -30,6 +70,34 @@ 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 void set_pmccfiltr(uint32_t filter)
> +{
> +	asm volatile("msr pmccfiltr_el0, %0" : : "r" (filter));
> +}
> +
> +static inline unsigned long get_pmccntr(void)
> +{
> +	unsigned long cycles;
> +
> +	asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> +	return cycles;
> +}
> +
> +static inline void enable_counter(uint32_t idx)
> +{
> +	asm volatile("msr pmcntenset_el0, %0" : : "r" (1 << idx));
> +}
> +
> +static inline void disable_counter(uint32_t idx)
> +{
> +	asm volatile("msr pmcntensclr_el0, %0" : : "r" (1 << idx));
> +}
>  #endif
>  
>  struct pmu_data {
> @@ -72,11 +140,43 @@ 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}};
> +
> +	enable_counter(ARMV8_PMU_CYCLE_IDX);
> +	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> +
> +	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;
> +		}
> +	}
> +
> +	pmu.enable = 0;
> +	set_pmcr(pmu.pmcr_el0);
> +
> +	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();
>  }

What happens with this test running on tcg? Do we just fail? Does it
explode? Is there a register we can probe and when it indicates things
won't work we can invoke a report_skip?

Thanks,
drew

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

[cov]

On 2016-11-03 04:35, Andrew Jones wrote:

>> +/*
>> + * Ensure that the cycle counter progresses between back-to-back 
>> reads.
>> + */
>> +static bool check_cycles_increase(void)
>> +{
>> +	struct pmu_data pmu = {{0}};
>> +
>> +	enable_counter(ARMV8_PMU_CYCLE_IDX);
>> +	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
>> +
>> +	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;
>> +		}
>> +	}
>> +
>> +	pmu.enable = 0;
>> +	set_pmcr(pmu.pmcr_el0);
>> +
>> +	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();
>>  }
> 
> What happens with this test running on tcg? Do we just fail? Does it
> explode? Is there a register we can probe and when it indicates things
> won't work we can invoke a report_skip?

A monotonically increasing value (but not any attempt at approximating 
actual cycle
values) in cycle counter is pretty much the only piece of the PMU it's 
had implemented
for the past while. We'll have to check whether TCG can handle the 
filter and enable set
registers, but if it doesn't yet, that's something we can improve :).

Thanks,
Cov

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

[Wei Huang]

Calculate the numbers of cycles per instruction (CPI) implied by ARM
PMU cycle counter values.

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

diff --git a/arm/pmu.c b/arm/pmu.c
index 65b7df1..ca00422 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -62,6 +62,23 @@ static inline void disable_counter(uint32_t idx)
 {
 	asm volatile("mrc p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
 }
+
+/*
+ * 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)
 {
@@ -98,6 +115,23 @@ static inline void disable_counter(uint32_t idx)
 {
 	asm volatile("msr pmcntensclr_el0, %0" : : "r" (1 << idx));
 }
+
+/*
+ * 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 {
@@ -171,12 +205,85 @@ 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}};
+
+	enable_counter(ARMV8_PMU_CYCLE_IDX);
+	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
+
+	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);
+	}
+
+	pmu.enable = 0;
+	set_pmcr(pmu.pmcr_el0);
+
+	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();
 }
-- 
1.8.3.1

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

[Andrew Jones]

From: Christopher
s-o-b: Wei

On Wed, Nov 02, 2016 at 05:22:17PM -0500, Wei Huang wrote:
> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> PMU cycle counter values.
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  arm/pmu.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 65b7df1..ca00422 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -62,6 +62,23 @@ static inline void disable_counter(uint32_t idx)
>  {
>  	asm volatile("mrc p15, 0, %0, c9, c12, 1" : : "r" (1 << idx));
>  }
> +
> +/*
> + * 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)
>  {
> @@ -98,6 +115,23 @@ static inline void disable_counter(uint32_t idx)
>  {
>  	asm volatile("msr pmcntensclr_el0, %0" : : "r" (1 << idx));
>  }
> +
> +/*
> + * 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 {
> @@ -171,12 +205,85 @@ 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}};
> +
> +	enable_counter(ARMV8_PMU_CYCLE_IDX);
> +	set_pmccfiltr(0); /* count cycles in EL0, EL1, but not EL2 */
> +
> +	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);
> +	}
> +
> +	pmu.enable = 0;
> +	set_pmcr(pmu.pmcr_el0);
> +
> +	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();
>  }
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [kvm-unit-tests PATCHv7 0/3] ARM PMU tests

[Andrew Jones]

echo $SUBJECT | s/PATCHv7/PATCH v7/  (otherwise busts my filters)

On Wed, Nov 02, 2016 at 05:22:14PM -0500, Wei Huang wrote:
> Changes from v6:
> * Add a new pmu testing for KVM mode in config file
> * Add additional init code, including setting PMCNTENSET and PMCCFILTR, 
>   before reading PMCCNTR. ARMv7 support is also provided
> * Add cycle counter init code for CPI testing
> * Fix pmu_data compilation issue (for gcc 4.8.5)
> * Commit comments were updated
> 
> NOTE:
> 1) Current KVM code has bugs in handling PMCCFILTR write. A fix (see
> below) is required for this unit testing code to work correctly under
> KVM mode.
> https://lists.cs.columbia.edu/pipermail/kvmarm/2016-November/022134.html.
> 2) Because the code was changed, Drew's original reviewed-by needs to
> be acknowledged by him again.
>

So you guys will hate me for just now bringing it up, but I'd really
prefer we switch from the bitfields to bit defines. We should also
minimize the amount of functions that depend on asms, i.e. only one
function per sysreg should be enough. The rest can be done in C. Please
also make sure the tests run with both tcg and kvm. That may mean probing
something and then reporting a SKIP where a feature isn't implemented.

Thanks,
drew