diff mbox

[kvm-unit-tests,v13,2/4] arm: Add PMU test

Message ID 1480569402-8848-3-git-send-email-wei@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Wei Huang Dec. 1, 2016, 5:16 a.m. UTC 
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). PMU register
was read using the newly defined macros.

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           | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 arm/unittests.cfg   |  5 +++++
 3 files changed, 69 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

Comments

Andrew Jones Dec. 1, 2016, 9:03 a.m. UTC | #1 
On Wed, Nov 30, 2016 at 11:16:40PM -0600, 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). PMU register
> was read using the newly defined macros.
> 
> 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           | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  arm/unittests.cfg   |  5 +++++
>  3 files changed, 69 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..1fe2b1a
> --- /dev/null
> +++ b/arm/pmu.c
> @@ -0,0 +1,62 @@
> +/*
> + * 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"
> +#include "asm/processor.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__)
> +DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0)
> +#elif defined(__aarch64__)
> +DEFINE_GET_SYSREG32(pmcr, el0)
> +#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;

So based on my comments from the previous patch, pmcr should be
'unsigned int'

> +
> +	pmcr = get_pmcr();
> +
> +	report_info("PMU implementer/ID code/counters: 0x%x(\"%c\")/0x%x/%d",
> +		    (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK,
> +		    ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) ? : ' ',
> +		    (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK,
> +		    (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
> 
>

drew 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andre Przywara Dec. 1, 2016, 11:28 a.m. UTC | #2 
Hi,

On 01/12/16 09:03, Andrew Jones wrote:
> On Wed, Nov 30, 2016 at 11:16:40PM -0600, 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). PMU register
>> was read using the newly defined macros.
>>
>> 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           | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  arm/unittests.cfg   |  5 +++++
>>  3 files changed, 69 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..1fe2b1a
>> --- /dev/null
>> +++ b/arm/pmu.c
>> @@ -0,0 +1,62 @@
>> +/*
>> + * 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"
>> +#include "asm/processor.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__)
>> +DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0)
>> +#elif defined(__aarch64__)
>> +DEFINE_GET_SYSREG32(pmcr, el0)
>> +#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;
> 
> So based on my comments from the previous patch, pmcr should be
> 'unsigned int'

I don't think so. At least here as a _variable_ type uint32_t is
probably the right one, as the ARMv8 ARM explicitly says that PMCR is a
32-bit register, for both bitnesses. I find it only natural to express
it here accordingly.
I believe this is a different (though related) discussion from the
return type of the accessor functions.

Cheers,
Andre.

>> +
>> +	pmcr = get_pmcr();
>> +
>> +	report_info("PMU implementer/ID code/counters: 0x%x(\"%c\")/0x%x/%d",
>> +		    (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK,
>> +		    ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) ? : ' ',
>> +		    (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK,
>> +		    (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
>>
>>
> 
> drew 
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Maydell Dec. 1, 2016, 12:02 p.m. UTC | #3 
On 1 December 2016 at 11:28, Andre Przywara <andre.przywara@arm.com> wrote:
> I don't think so. At least here as a _variable_ type uint32_t is
> probably the right one, as the ARMv8 ARM explicitly says that PMCR is a
> 32-bit register, for both bitnesses.

For 64-bit ARM this is strictly speaking just shorthand for "64-bit
register with the top 32-bit being RES0". It is in theory possible that
a future architecture extension might define uses for those RES0
bits.

thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andre Przywara Dec. 1, 2016, 12:19 p.m. UTC | #4 
Hi,

On 01/12/16 12:02, Peter Maydell wrote:
> On 1 December 2016 at 11:28, Andre Przywara <andre.przywara@arm.com> wrote:
>> I don't think so. At least here as a _variable_ type uint32_t is
>> probably the right one, as the ARMv8 ARM explicitly says that PMCR is a
>> 32-bit register, for both bitnesses.
> 
> For 64-bit ARM this is strictly speaking just shorthand for "64-bit
> register with the top 32-bit being RES0". It is in theory possible that
> a future architecture extension might define uses for those RES0
> bits.

I trade: "in theory possible that a future architecture extension might"
(that's four speculative terms, right?) against:

ARMv8 ARM, D7.4.7  PMCR_EL0, Performance Monitors Control Register:
Attributes
            PMCR_EL0 is a 32-bit register.

If this ever gets extended, we would need extra code to deal with the
new bits, so we would need to touch the code anyway. And again, it's
just a local variable, not an interface.

Cheers,
Andre.

P.S. We really should save this discussion for a Friday afternoon ;-)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Maydell Dec. 1, 2016, 12:36 p.m. UTC | #5 
On 1 December 2016 at 12:19, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 01/12/16 12:02, Peter Maydell wrote:
>> On 1 December 2016 at 11:28, Andre Przywara <andre.przywara@arm.com> wrote:
>>> I don't think so. At least here as a _variable_ type uint32_t is
>>> probably the right one, as the ARMv8 ARM explicitly says that PMCR is a
>>> 32-bit register, for both bitnesses.
>>
>> For 64-bit ARM this is strictly speaking just shorthand for "64-bit
>> register with the top 32-bit being RES0". It is in theory possible that
>> a future architecture extension might define uses for those RES0
>> bits.
>
> I trade: "in theory possible that a future architecture extension might"
> (that's four speculative terms, right?) against:
>
> ARMv8 ARM, D7.4.7  PMCR_EL0, Performance Monitors Control Register:
> Attributes
>             PMCR_EL0 is a 32-bit register.

As I say, this just means "64 bit with 32 RES0 bits". See DDI0487A.k
C5.1.1 "System register width":

# In AArch64 state, each encoding in the System instruction space can
# provide access to a 64-bit register. An AArch64 System register is
# described as either a 32-bit register or a 64-bit register. For
# a 32-bit register, the upper bits, bits[63:32], are RES0.

(ie the register is 64-bits, it's just "described as" 32-bits for
convenience.)

thanks
-- PMM
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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..1fe2b1a
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,62 @@ 
+/*
+ * 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"
+#include "asm/processor.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__)
+DEFINE_GET_SYSREG32(pmcr, 0, c9, c12, 0)
+#elif defined(__aarch64__)
+DEFINE_GET_SYSREG32(pmcr, el0)
+#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 = get_pmcr();
+
+	report_info("PMU implementer/ID code/counters: 0x%x(\"%c\")/0x%x/%d",
+		    (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK,
+		    ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) ? : ' ',
+		    (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK,
+		    (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