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

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

[Wei Huang]

Changes from v10:
* Change the name of loop test function to precise_instrs_loop()
* Minor comment fixes to measure_instrs() and to explain isb() in loop funcs 

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.

Thanks,
-Wei

Christopher Covington (3):
  arm: Add PMU test
  arm: pmu: Check cycle count increases
  arm: pmu: Add CPI checking

 arm/Makefile.common |   3 +-
 arm/pmu.c           | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg   |  19 +++
 3 files changed, 372 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

-- 
1.8.3.1

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

[Wei Huang]

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>
Signed-off-by: Wei Huang <wei@redhat.com>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 arm/Makefile.common |  3 ++-
 arm/pmu.c           | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg   |  5 ++++
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f37b5c2..5da2fdd 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -12,7 +12,8 @@ endif
 tests-common = \
 	$(TEST_DIR)/selftest.flat \
 	$(TEST_DIR)/spinlock-test.flat \
-	$(TEST_DIR)/pci-test.flat
+	$(TEST_DIR)/pci-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..9d9c53b
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,74 @@
+/*
+ * Test the ARM Performance Monitors Unit (PMU).
+ *
+ * Copyright (c) 2015-2016, 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"
+#include "asm/barrier.h"
+
+#define PMU_PMCR_N_SHIFT   11
+#define PMU_PMCR_N_MASK    0x1f
+#define PMU_PMCR_ID_SHIFT  16
+#define PMU_PMCR_ID_MASK   0xff
+#define PMU_PMCR_IMP_SHIFT 24
+#define PMU_PMCR_IMP_MASK  0xff
+
+#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;
+}
+#elif defined(__aarch64__)
+static inline uint32_t pmcr_read(void)
+{
+	uint32_t ret;
+
+	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
+	return ret;
+}
+#endif
+
+/*
+ * 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 2016, 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)
+{
+	uint32_t pmcr;
+
+	pmcr = pmcr_read();
+
+	printf("PMU implementer:     %c\n",
+	       (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
+	printf("Identification code: 0x%x\n",
+	       (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
+	printf("Event counters:      %d\n",
+	       (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
+
+	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 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 ae32a42..816f494 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -58,3 +58,8 @@ groups = selftest
 [pci-test]
 file = pci-test.flat
 groups = pci
+
+# Test PMU support
+[pmu]
+file = pmu.flat
+groups = pmu
-- 
1.8.3.1

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

[Andre Przywara]

Hi,

On 22/11/16 18:29, Wei Huang wrote:
> 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).

Mmh, the output of this is a bit confusing. How about to join some
information? I changed it to give me:
INFO: pmu: PMU implementer/ID code:     "A"(0x41)/0x0
INFO: pmu: Event counters:      0
PASS: pmu: Control register

... by using the newly introduced report_info() to make it look nicer.

> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> Signed-off-by: Wei Huang <wei@redhat.com>
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/Makefile.common |  3 ++-
>  arm/pmu.c           | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg   |  5 ++++
>  3 files changed, 81 insertions(+), 1 deletion(-)
>  create mode 100644 arm/pmu.c
> 
> diff --git a/arm/Makefile.common b/arm/Makefile.common
> index f37b5c2..5da2fdd 100644
> --- a/arm/Makefile.common
> +++ b/arm/Makefile.common
> @@ -12,7 +12,8 @@ endif
>  tests-common = \
>  	$(TEST_DIR)/selftest.flat \
>  	$(TEST_DIR)/spinlock-test.flat \
> -	$(TEST_DIR)/pci-test.flat
> +	$(TEST_DIR)/pci-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..9d9c53b
> --- /dev/null
> +++ b/arm/pmu.c
> @@ -0,0 +1,74 @@
> +/*
> + * Test the ARM Performance Monitors Unit (PMU).
> + *
> + * Copyright (c) 2015-2016, 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"
> +#include "asm/barrier.h"
> +
> +#define PMU_PMCR_N_SHIFT   11
> +#define PMU_PMCR_N_MASK    0x1f
> +#define PMU_PMCR_ID_SHIFT  16
> +#define PMU_PMCR_ID_MASK   0xff
> +#define PMU_PMCR_IMP_SHIFT 24
> +#define PMU_PMCR_IMP_MASK  0xff
> +
> +#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.

> +static inline uint32_t pmcr_read(void)
> +{
> +	uint32_t ret;
> +
> +	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> +	return ret;
> +}
> +#elif defined(__aarch64__)
> +static inline uint32_t pmcr_read(void)
> +{
> +	uint32_t ret;
> +
> +	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
> +	return ret;
> +}
> +#endif
> +
> +/*
> + * 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 2016, 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)
> +{
> +	uint32_t pmcr;
> +
> +	pmcr = pmcr_read();
> +
> +	printf("PMU implementer:     %c\n",
> +	       (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);

If this register reads as zero, the output is mangled (since it cuts off
the string before the newline):
=====
PMU implementer:     Identification code: 0x0
=====

I guess you need something like:
(pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK ?: ' '

> +	printf("Identification code: 0x%x\n",
> +	       (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);

As mentioned above this should use report_info() now, also it would be
nice to merge this with the message above into one line of output.

Cheers,
Andre

> +	printf("Event counters:      %d\n",
> +	       (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
> +
> +	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 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 ae32a42..816f494 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -58,3 +58,8 @@ groups = selftest
>  [pci-test]
>  file = pci-test.flat
>  groups = pci
> +
> +# Test PMU support
> +[pmu]
> +file = pmu.flat
> +groups = pmu
> 

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

[Wei Huang]

On 11/23/2016 07:16 AM, Andre Przywara wrote:
> Hi,
> 
> On 22/11/16 18:29, Wei Huang wrote:
>> 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).
> 
> Mmh, the output of this is a bit confusing. How about to join some
> information? I changed it to give me:
> INFO: pmu: PMU implementer/ID code:     "A"(0x41)/0x0
> INFO: pmu: Event counters:      0
> PASS: pmu: Control register
> 
> ... by using the newly introduced report_info() to make it look nicer.
> 
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> Signed-off-by: Wei Huang <wei@redhat.com>
>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>> ---
>>  arm/Makefile.common |  3 ++-
>>  arm/pmu.c           | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg   |  5 ++++
>>  3 files changed, 81 insertions(+), 1 deletion(-)
>>  create mode 100644 arm/pmu.c
>>
>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>> index f37b5c2..5da2fdd 100644
>> --- a/arm/Makefile.common
>> +++ b/arm/Makefile.common
>> @@ -12,7 +12,8 @@ endif
>>  tests-common = \
>>  	$(TEST_DIR)/selftest.flat \
>>  	$(TEST_DIR)/spinlock-test.flat \
>> -	$(TEST_DIR)/pci-test.flat
>> +	$(TEST_DIR)/pci-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..9d9c53b
>> --- /dev/null
>> +++ b/arm/pmu.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * Test the ARM Performance Monitors Unit (PMU).
>> + *
>> + * Copyright (c) 2015-2016, 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"
>> +#include "asm/barrier.h"
>> +
>> +#define PMU_PMCR_N_SHIFT   11
>> +#define PMU_PMCR_N_MASK    0x1f
>> +#define PMU_PMCR_ID_SHIFT  16
>> +#define PMU_PMCR_ID_MASK   0xff
>> +#define PMU_PMCR_IMP_SHIFT 24
>> +#define PMU_PMCR_IMP_MASK  0xff
>> +
>> +#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.

We planned to add it after this series is merged. However if GIC series
has a similar support, we can piggy-back on it.

> 
>> +static inline uint32_t pmcr_read(void)
>> +{
>> +	uint32_t ret;
>> +
>> +	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>> +	return ret;
>> +}
>> +#elif defined(__aarch64__)
>> +static inline uint32_t pmcr_read(void)
>> +{
>> +	uint32_t ret;
>> +
>> +	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>> +	return ret;
>> +}
>> +#endif
>> +
>> +/*
>> + * 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 2016, 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)
>> +{
>> +	uint32_t pmcr;
>> +
>> +	pmcr = pmcr_read();
>> +
>> +	printf("PMU implementer:     %c\n",
>> +	       (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
> 
> If this register reads as zero, the output is mangled (since it cuts off
> the string before the newline):
> =====
> PMU implementer:     Identification code: 0x0
> =====
> 
> I guess you need something like:
> (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK ?: ' '
> 
>> +	printf("Identification code: 0x%x\n",
>> +	       (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
> 
> As mentioned above this should use report_info() now, also it would be
> nice to merge this with the message above into one line of output.

I will take them into consideration. Thanks.

> 
> Cheers,
> Andre
> 
>> +	printf("Event counters:      %d\n",
>> +	       (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
>> +
>> +	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 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 ae32a42..816f494 100644
>> --- a/arm/unittests.cfg
>> +++ b/arm/unittests.cfg
>> @@ -58,3 +58,8 @@ groups = selftest
>>  [pci-test]
>>  file = pci-test.flat
>>  groups = pci
>> +
>> +# Test PMU support
>> +[pmu]
>> +file = pmu.flat
>> +groups = pmu
>>

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

[Andrew Jones]

On Wed, Nov 23, 2016 at 01:16:08PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 22/11/16 18:29, Wei Huang wrote:
> > 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).
> 
> Mmh, the output of this is a bit confusing. How about to join some
> information? I changed it to give me:
> INFO: pmu: PMU implementer/ID code:     "A"(0x41)/0x0
> INFO: pmu: Event counters:      0
> PASS: pmu: Control register
> 
> ... by using the newly introduced report_info() to make it look nicer.

Agreed. That would look nicer and make good use of report_info. Let's
do that.

> 
> > 
> > Signed-off-by: Christopher Covington <cov@codeaurora.org>
> > Signed-off-by: Wei Huang <wei@redhat.com>
> > Reviewed-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/Makefile.common |  3 ++-
> >  arm/pmu.c           | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  arm/unittests.cfg   |  5 ++++
> >  3 files changed, 81 insertions(+), 1 deletion(-)
> >  create mode 100644 arm/pmu.c
> > 
> > diff --git a/arm/Makefile.common b/arm/Makefile.common
> > index f37b5c2..5da2fdd 100644
> > --- a/arm/Makefile.common
> > +++ b/arm/Makefile.common
> > @@ -12,7 +12,8 @@ endif
> >  tests-common = \
> >  	$(TEST_DIR)/selftest.flat \
> >  	$(TEST_DIR)/spinlock-test.flat \
> > -	$(TEST_DIR)/pci-test.flat
> > +	$(TEST_DIR)/pci-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..9d9c53b
> > --- /dev/null
> > +++ b/arm/pmu.c
> > @@ -0,0 +1,74 @@
> > +/*
> > + * Test the ARM Performance Monitors Unit (PMU).
> > + *
> > + * Copyright (c) 2015-2016, 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"
> > +#include "asm/barrier.h"
> > +
> > +#define PMU_PMCR_N_SHIFT   11
> > +#define PMU_PMCR_N_MASK    0x1f
> > +#define PMU_PMCR_ID_SHIFT  16
> > +#define PMU_PMCR_ID_MASK   0xff
> > +#define PMU_PMCR_IMP_SHIFT 24
> > +#define PMU_PMCR_IMP_MASK  0xff
> > +
> > +#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.

> 
> > +static inline uint32_t pmcr_read(void)
> > +{
> > +	uint32_t ret;
> > +
> > +	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> > +	return ret;
> > +}
> > +#elif defined(__aarch64__)
> > +static inline uint32_t pmcr_read(void)
> > +{
> > +	uint32_t ret;
> > +
> > +	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
> > +	return ret;
> > +}
> > +#endif
> > +
> > +/*
> > + * 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 2016, 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)
> > +{
> > +	uint32_t pmcr;
> > +
> > +	pmcr = pmcr_read();
> > +
> > +	printf("PMU implementer:     %c\n",
> > +	       (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
> 
> If this register reads as zero, the output is mangled (since it cuts off
> the string before the newline):
> =====
> PMU implementer:     Identification code: 0x0
> =====
> 
> I guess you need something like:
> (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK ?: ' '

Good idea.

> 
> > +	printf("Identification code: 0x%x\n",
> > +	       (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
> 
> As mentioned above this should use report_info() now, also it would be
> nice to merge this with the message above into one line of output.

Agreed.

Thanks,
drew

> 
> Cheers,
> Andre
> 
> > +	printf("Event counters:      %d\n",
> > +	       (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
> > +
> > +	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 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 ae32a42..816f494 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -58,3 +58,8 @@ groups = selftest
> >  [pci-test]
> >  file = pci-test.flat
> >  groups = pci
> > +
> > +# Test PMU support
> > +[pmu]
> > +file = pmu.flat
> > +groups = pmu
> > 
> 

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

[Wei Huang]

On 11/23/2016 11:15 AM, Andrew Jones wrote:
> On Wed, Nov 23, 2016 at 01:16:08PM +0000, Andre Przywara wrote:
>> Hi,
>>
>> On 22/11/16 18:29, Wei Huang wrote:
>>> 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).
>>
>> Mmh, the output of this is a bit confusing. How about to join some
>> information? I changed it to give me:
>> INFO: pmu: PMU implementer/ID code:     "A"(0x41)/0x0
>> INFO: pmu: Event counters:      0
>> PASS: pmu: Control register
>>
>> ... by using the newly introduced report_info() to make it look nicer.
> 
> Agreed. That would look nicer and make good use of report_info. Let's
> do that.

I have adjusted v12 using report_info(), with all PMU PMCR fields
printed in the same line. Implementer info was printed with Hex first,
then ASCII representation, to match MIDR table in ARM manual:

INFO: pmu: PMU implementer/ID code/counters: 0x41("A")/0x1/6


> 
>>
>>>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> Signed-off-by: Wei Huang <wei@redhat.com>
>>> Reviewed-by: Andrew Jones <drjones@redhat.com>
>>> ---
>>>  arm/Makefile.common |  3 ++-
>>>  arm/pmu.c           | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  arm/unittests.cfg   |  5 ++++
>>>  3 files changed, 81 insertions(+), 1 deletion(-)
>>>  create mode 100644 arm/pmu.c
>>>
>>> diff --git a/arm/Makefile.common b/arm/Makefile.common
>>> index f37b5c2..5da2fdd 100644
>>> --- a/arm/Makefile.common
>>> +++ b/arm/Makefile.common
>>> @@ -12,7 +12,8 @@ endif
>>>  tests-common = \
>>>  	$(TEST_DIR)/selftest.flat \
>>>  	$(TEST_DIR)/spinlock-test.flat \
>>> -	$(TEST_DIR)/pci-test.flat
>>> +	$(TEST_DIR)/pci-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..9d9c53b
>>> --- /dev/null
>>> +++ b/arm/pmu.c
>>> @@ -0,0 +1,74 @@
>>> +/*
>>> + * Test the ARM Performance Monitors Unit (PMU).
>>> + *
>>> + * Copyright (c) 2015-2016, 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"
>>> +#include "asm/barrier.h"
>>> +
>>> +#define PMU_PMCR_N_SHIFT   11
>>> +#define PMU_PMCR_N_MASK    0x1f
>>> +#define PMU_PMCR_ID_SHIFT  16
>>> +#define PMU_PMCR_ID_MASK   0xff
>>> +#define PMU_PMCR_IMP_SHIFT 24
>>> +#define PMU_PMCR_IMP_MASK  0xff
>>> +
>>> +#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.

I left accessors in-place. We can always come back to refactor them later.

> 
>>
>>> +static inline uint32_t pmcr_read(void)
>>> +{
>>> +	uint32_t ret;
>>> +
>>> +	asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>>> +	return ret;
>>> +}
>>> +#elif defined(__aarch64__)
>>> +static inline uint32_t pmcr_read(void)
>>> +{
>>> +	uint32_t ret;
>>> +
>>> +	asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>>> +	return ret;
>>> +}
>>> +#endif
>>> +
>>> +/*
>>> + * 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 2016, 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)
>>> +{
>>> +	uint32_t pmcr;
>>> +
>>> +	pmcr = pmcr_read();
>>> +
>>> +	printf("PMU implementer:     %c\n",
>>> +	       (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
>>
>> If this register reads as zero, the output is mangled (since it cuts off
>> the string before the newline):
>> =====
>> PMU implementer:     Identification code: 0x0
>> =====
>>
>> I guess you need something like:
>> (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK ?: ' '
> 
> Good idea.

Done

> 
>>
>>> +	printf("Identification code: 0x%x\n",
>>> +	       (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
>>
>> As mentioned above this should use report_info() now, also it would be
>> nice to merge this with the message above into one line of output.
> 
> Agreed.

Done. I removed your name from Reviewed-by from Patch 1. You can ACK it
if ok.

> 
> Thanks,
> drew
> 
>>
>> Cheers,
>> Andre
>>
>>> +	printf("Event counters:      %d\n",
>>> +	       (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
>>> +
>>> +	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 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 ae32a42..816f494 100644
>>> --- a/arm/unittests.cfg
>>> +++ b/arm/unittests.cfg
>>> @@ -58,3 +58,8 @@ groups = selftest
>>>  [pci-test]
>>>  file = pci-test.flat
>>>  groups = pci
>>> +
>>> +# Test PMU support
>>> +[pmu]
>>> +file = pmu.flat
>>> +groups = pmu
>>>
>>

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

[Andre Przywara]

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?

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

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

[Andrew Jones]

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 :-)

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
> 
> 

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

[Andre Przywara]

Hi,

On 25/11/16 14:26, 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),

Mmh, where is this easy to read? Especially the v7 syntax is really
error prone and hard to grasp with all these c9s, c12s and friends, in
addition to the weird ordering and mixing in of register names in the
(inline) assembly syntax. Blame the company that came up with this
architecture ;-)

Compare:
mrc p15, 0, %0, c9, c12, 0
with:
DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0)

The "0, %0" in there always kills me when reading this.

On top of this a list of one-line-per-sysreg declarations is much easier
to compare with the manual than the open coded versions.

Another benefit is that we don't need to encode versions for both
bitnesses in each test. For shared sysregs we could put the definitions
in processor.h, the code then just uses one architecture agnostic version.

But I leave it up to you guys to decide on this, you don't break my
heart if you stick with inline assembly ;-)

Cheers,
Andre.

 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 :-)
> 
> 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
>>
>>

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

[Wei Huang]

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

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

Thanks,
-Wei

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

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

[Wei Huang]

From: Christopher Covington <cov@codeaurora.org>

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>
Reviewed-by: Andrew Jones <drjones@redhat.com>
---
 arm/pmu.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 9d9c53b..176b070 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -15,6 +15,9 @@
 #include "libcflat.h"
 #include "asm/barrier.h"
 
+#define PMU_PMCR_E         (1 << 0)
+#define PMU_PMCR_C         (1 << 2)
+#define PMU_PMCR_LC        (1 << 6)
 #define PMU_PMCR_N_SHIFT   11
 #define PMU_PMCR_N_MASK    0x1f
 #define PMU_PMCR_ID_SHIFT  16
@@ -22,6 +25,14 @@
 #define PMU_PMCR_IMP_SHIFT 24
 #define PMU_PMCR_IMP_MASK  0xff
 
+#define ID_DFR0_PERFMON_SHIFT 24
+#define ID_DFR0_PERFMON_MASK  0xf
+
+#define PMU_CYCLE_IDX         31
+
+#define NR_SAMPLES 10
+
+static unsigned int pmu_version;
 #if defined(__arm__)
 static inline uint32_t pmcr_read(void)
 {
@@ -30,6 +41,69 @@ static inline uint32_t pmcr_read(void)
 	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)
+{
+	uint32_t lo, hi = 0;
+
+	if (pmu_version == 0x3)
+		asm volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
+	else
+		asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (lo));
+
+	return ((uint64_t)hi << 32) | lo;
+}
+
+static inline void pmccntr_write(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));
+	else
+		asm volatile("mcr p15, 0, %0, c9, c13, 0" : : "r" (lo));
+}
+
+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)
+{
+	pmselr_write(PMU_CYCLE_IDX);
+	pmxevtyper_write(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;
+}
 #elif defined(__aarch64__)
 static inline uint32_t pmcr_read(void)
 {
@@ -38,6 +112,44 @@ static inline uint32_t pmcr_read(void)
 	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;
+}
 #endif
 
 /*
@@ -64,11 +176,55 @@ static bool check_pmcr(void)
 	return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
 }
 
+/*
+ * Ensure that the cycle counter progresses between back-to-back reads.
+ */
+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);
+
+	pmcr_write(pmcr_read() | 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();
+
+		if (a >= b) {
+			printf("Read %"PRId64" then %"PRId64".\n", a, b);
+			success = false;
+			break;
+		}
+	}
+
+	pmcr_write(pmcr_read() & ~PMU_PMCR_E);
+
+	return success;
+}
+
+void pmu_init(void)
+{
+	uint32_t dfr0;
+
+	/* probe pmu version */
+	dfr0 = id_dfr0_read();
+	pmu_version = (dfr0 >> ID_DFR0_PERFMON_SHIFT) & ID_DFR0_PERFMON_MASK;
+	printf("PMU version: %d\n", pmu_version);
+}
+
 int main(void)
 {
 	report_prefix_push("pmu");
 
+	pmu_init();
 	report("Control register", check_pmcr());
+	report("Monotonically increasing cycle count", check_cycles_increase());
 
 	return report_summary();
 }
-- 
1.8.3.1

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

[Wei Huang]

From: Christopher Covington <cov@codeaurora.org>

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 in the configuration file.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
Signed-off-by: Wei Huang <wei@redhat.com>
---
 arm/pmu.c         | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 arm/unittests.cfg |  14 +++++++
 2 files changed, 136 insertions(+), 1 deletion(-)

diff --git a/arm/pmu.c b/arm/pmu.c
index 176b070..f667676 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -104,6 +104,27 @@ static inline uint32_t id_dfr0_read(void)
 	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
+ * to start and stop counting. isb instructions were inserted to make sure
+ * pmccntr read after this function returns the exact instructions executed in
+ * the controlled block. Total instrs = isb + mcr + 2*loop = 2 + 2*loop.
+ */
+static inline void precise_instrs_loop(int loop, uint32_t pmcr)
+{
+	asm volatile(
+	"	mcr	p15, 0, %[pmcr], c9, c12, 0\n"
+	"	isb\n"
+	"1:	subs	%[loop], %[loop], #1\n"
+	"	bgt	1b\n"
+	"	mcr	p15, 0, %[z], c9, c12, 0\n"
+	"	isb\n"
+	: [loop] "+r" (loop)
+	: [pmcr] "r" (pmcr), [z] "r" (0)
+	: "cc");
+}
 #elif defined(__aarch64__)
 static inline uint32_t pmcr_read(void)
 {
@@ -150,6 +171,27 @@ static inline uint32_t id_dfr0_read(void)
 	asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
 	return id;
 }
+
+/*
+ * 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. isb instructions are inserted to make sure
+ * pmccntr read after this function returns the exact instructions executed
+ * in the controlled block. Total instrs = isb + msr + 2*loop = 2 + 2*loop.
+ */
+static inline void precise_instrs_loop(int loop, uint32_t pmcr)
+{
+	asm volatile(
+	"	msr	pmcr_el0, %[pmcr]\n"
+	"	isb\n"
+	"1:	subs	%[loop], %[loop], #1\n"
+	"	b.gt	1b\n"
+	"	msr	pmcr_el0, xzr\n"
+	"	isb\n"
+	: [loop] "+r" (loop)
+	: [pmcr] "r" (pmcr)
+	: "cc");
+}
 #endif
 
 /*
@@ -208,6 +250,79 @@ static bool check_cycles_increase(void)
 	return success;
 }
 
+/*
+ * Execute a known number of guest instructions. Only even instruction counts
+ * greater than or equal to 4 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 loop = (num - 2) / 2;
+
+	assert(num >= 4 && ((num - 2) % 2 == 0));
+	precise_instrs_loop(loop, 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)
+{
+	uint32_t pmcr = pmcr_read() | 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 */
+
+	if (cpi > 0)
+		printf("Checking for CPI=%d.\n", cpi);
+	printf("instrs : cycles0 cycles1 ...\n");
+
+	for (unsigned int i = 4; i < 300; i += 32) {
+		uint64_t avg, sum = 0;
+
+		printf("%d :", i);
+		for (int j = 0; j < NR_SAMPLES; j++) {
+			uint64_t cycles;
+
+			pmccntr_write(0);
+			measure_instrs(i, pmcr);
+			cycles = pmccntr_read();
+			printf(" %"PRId64"", cycles);
+
+			if (!cycles) {
+				printf("\ncycles not incrementing!\n");
+				return false;
+			} else if (cpi > 0 && cycles != i * cpi) {
+				printf("\nunexpected cycle count received!\n");
+				return false;
+			} else if ((cycles >> 32) != 0) {
+				/* The cycles taken by the loop above should
+				 * fit in 32 bits easily. We check the upper
+				 * 32 bits of the cycle counter to make sure
+				 * there is no supprise. */
+				printf("\ncycle count bigger than 32bit!\n");
+				return false;
+			}
+
+			sum += cycles;
+		}
+		avg = sum / NR_SAMPLES;
+		printf(" sum=%"PRId64" avg=%"PRId64" avg_ipc=%"PRId64" "
+		       "avg_cpi=%"PRId64"\n", sum, avg, i / avg, avg / i);
+	}
+
+	return true;
+}
+
 void pmu_init(void)
 {
 	uint32_t dfr0;
@@ -218,13 +333,19 @@ void pmu_init(void)
 	printf("PMU version: %d\n", pmu_version);
 }
 
-int main(void)
+int main(int argc, char *argv[])
 {
+	int cpi = 0;
+
+	if (argc > 1)
+		cpi = atol(argv[1]);
+
 	report_prefix_push("pmu");
 
 	pmu_init();
 	report("Control register", check_pmcr());
 	report("Monotonically increasing cycle count", check_cycles_increase());
+	report("Cycle/instruction ratio", check_cpi(cpi));
 
 	return report_summary();
 }
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index 816f494..044d97c 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -63,3 +63,17 @@ groups = pci
 [pmu]
 file = pmu.flat
 groups = pmu
+
+# 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 PATCH v11 3/3] arm: pmu: Add CPI checking

[Andrew Jones]

On Tue, Nov 22, 2016 at 12:29:14PM -0600, Wei Huang wrote:
> From: Christopher Covington <cov@codeaurora.org>
> 
> 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 in the configuration file.
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  arm/pmu.c         | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  arm/unittests.cfg |  14 +++++++
>  2 files changed, 136 insertions(+), 1 deletion(-)


Reviewed-by: Andrew Jones <drjones@redhat.com>

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

[Andrew Jones]

On Tue, Nov 22, 2016 at 12:29:11PM -0600, Wei Huang wrote:
> Changes from v10:
> * Change the name of loop test function to precise_instrs_loop()
> * Minor comment fixes to measure_instrs() and to explain isb() in loop funcs 
> 
> 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.
> 
> Thanks,
> -Wei
> 
> Christopher Covington (3):
>   arm: Add PMU test
>   arm: pmu: Check cycle count increases
>   arm: pmu: Add CPI checking
> 
>  arm/Makefile.common |   3 +-
>  arm/pmu.c           | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg   |  19 +++
>  3 files changed, 372 insertions(+), 1 deletion(-)
>  create mode 100644 arm/pmu.c
> 
> -- 
> 1.8.3.1
>

I'm pretty happy with this series. Andre has good suggestions though.
If you send a v12 soon, and nobody complains about my v7 gic series,
then I'll group this with the gic series into a single PULL request
for Radim and Paolo.

Thanks,
drew